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

Already on GitHub? Sign in to your account

Jump to bottom

Fix broken AdmissionReview #3574

Merged
merged 1 commit into from
Oct 13, 2020
Merged

Fix broken AdmissionReview #3574

merged 1 commit into from
Oct 13, 2020

Conversation

kfox1111
Copy link
Contributor

@kfox1111 kfox1111 commented Oct 7, 2020 •
edited
Loading

The webhooks were failing as they aren't returning an apiVersion or kind in the response.

Closes #3473

@kfox1111 kfox1111 requested a review from a team as a code owner October 7, 2020 23:59
kfox1111
kfox1111 commented Oct 8, 2020
View reviewed changes
pkg/admission/admission.go
@@ -256,6 +258,9 @@ func (a *Admission) serve(w http.ResponseWriter, r *http.Request, admit admitFun

responseAdmissionReview.Response.UID = requestedAdmissionReview.Request.UID

responseAdmissionReview.APIVersion = requestedAdmissionReview.APIVersion
responseAdmissionReview.Kind = requestedAdmissionReview.Kind

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'm not sure if the second function "serve" is actually used. It may be dead code.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed

$ golangci-lint run --disable-all -E unused ./pkg/admission/.
pkg/admission/admission.go:224:21: func `(*Admission).serve` is unused (unused)

I've filed #3576 as a follow-up.

simonpasquier
simonpasquier reviewed Oct 8, 2020
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure why the e2e tests didn't catch it. Is it related to the Kubernetes version?

pkg/admission/admission.go
@@ -256,6 +258,9 @@ func (a *Admission) serve(w http.ResponseWriter, r *http.Request, admit admitFun

responseAdmissionReview.Response.UID = requestedAdmissionReview.Request.UID

responseAdmissionReview.APIVersion = requestedAdmissionReview.APIVersion
responseAdmissionReview.Kind = requestedAdmissionReview.Kind

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed

$ golangci-lint run --disable-all -E unused ./pkg/admission/.
pkg/admission/admission.go:224:21: func `(*Admission).serve` is unused (unused)

I've filed #3576 as a follow-up.

@lilic
Copy link
Contributor

lilic commented Oct 8, 2020

Thanks for the PR, do you mind just editing the title to explain what you fix, this makes it much easier when writing a CHANGELOG :) Thanks!

@kfox1111 kfox1111 changed the title Fix issue #3473 Fix broken AdmissionReview Oct 8, 2020
@s-urbaniak
Copy link
Contributor

LGTM and thank you so much for the contribution!
indeed, I am very curious why this was not catched in e2e 🤔

@simonpasquier
Copy link
Contributor

I'm testing locally with Kube v1.17.11 as it was originally reported in #3473.

@simonpasquier
Copy link
Contributor

Ok, I know why we didn't reproduce it in the CI. We still use and document admissionregistration.k8s.io/v1beta1 (instead of admissionregistration.k8s.io/v1) and in the tests, we don't explicitly set admissionReviewVersions when we create the webhook configuration (because it's not mandatory with v1beta1).
If I configure the webhook with admissionReviewVersions: [v1, v1beta1] then I see the same error as the OP. I've also tested that the issue goes away with the PR.

@brancz should we switch to admissionregistration.k8s.io/v1 both in tests and documentation? From the k8s docs, it requires at least Kubernetes 1.16 which matches with the prometheus operator requirements. Not sure if we want the operator to support both v1 and v1beta1 admission review requests but for now, the code assumes v1 only.

@lilic
Copy link
Contributor

lilic commented Oct 13, 2020

v1 only, and we fix the docs and just add it to the CHANGES in the CHANGELOG, if code assumes that makes no sense to test against both.

@simonpasquier simonpasquier merged commit 4484e49 into prometheus-operator:master Oct 13, 2020
@simonpasquier
Copy link
Contributor

Thanks!

This was referenced Oct 13, 2020
Closed
Merged
@bismarck bismarck mentioned this pull request Oct 27, 2020
Merged
3 tasks
@simonpasquier simonpasquier mentioned this pull request Nov 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonpasquier simonpasquier simonpasquier left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Admission Webhooks not working in v0.40.0+
4 participants
@kfox1111 @lilic @s-urbaniak @simonpasquier