Skip to content

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

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/webhook/conversion/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
return
}

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

@sbueringer sbueringer Sep 26, 2022 •
edited
Loading

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