Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 Conversion webhook should not panic when conversion request is nil #1970

Merged
merged 1 commit into from Sep 26, 2022
Merged

馃悰 Conversion webhook should not panic when conversion request is nil #1970

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/webhook/conversion/conversion.go
View file
Open in desktop
Expand Up @@ -69,6 +69,12 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
return
}

Tomy2e marked this conversation as resolved.
Show resolved Hide resolved
if convertReview.Request == nil {
log.Error(nil, "conversion request is nil")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an info instead of error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Error should be used as we're logging an error message here. Also the documentation says "Info logs a non-error message with the given key/value pairs as context" so it may not be appropriate to use Info. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error seems okay to me. It definitely seems to fall in the same category as l.67

	if err != nil {
		log.Error(err, "failed to read conversion request")
		w.WriteHeader(http.StatusBadRequest)
		return
	}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is that we're passing nil to log.Error which seems a bit backwards, let's either create an error, or spit out an Info as warning for users?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

    log.Error(errors.New("conversion request is nil"), "failed to handle conversion request")

?

Still feels a bit strange, but maybe better than logging one error as an error and the other as info

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any issue with err being nil, the docs clearly say this is ok (https://pkg.go.dev/github.com/go-logr/logr#Logger.Error and https://github.com/kubernetes-sigs/controller-runtime/blob/master/TMP-LOGGING.md#logging-errors).

Also, there are already many instances of Error being called with a nil error in this repo (https://github.com/kubernetes-sigs/controller-runtime/search?q=log.Error%28nil)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We went through multiple logger iterations before we got here, but anyways, that's fine if we want to log Error

w.WriteHeader(http.StatusBadRequest)
return
}

// TODO(droot): may be move the conversion logic to a separate module to
// decouple it from the http layer ?
resp, err := wh.handleConvertRequest(convertReview.Request)
Expand Down