Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
馃尡 adding TokenReview.auth.k8s.io/v1 webhook support #1440
馃尡 adding TokenReview.auth.k8s.io/v1 webhook support #1440
Changes from all commits
6ae3ee1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the godocs of net/http.Request:
So this doesn't need to be an
else
but simply at the top level after ther.Body != nil
and check for the length ofvar body
insteadThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another carryover from the admission package. https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/webhook/admission/http.go#L53-L67
That being said since we're not actually checking the HTTP Method, I believe this is what this block is meant to confirm that this isn't a GET, but I could have also understood the goals of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think I should remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've switched around the way this is handled, the first check
r.Body == nil
now checks if it's a request that doesn't support a body, like aGet
and fails out then we go on to reading the body.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check needed? Deserialization will fail if its not json or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was carried over from the admission package. https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/webhook/admission/http.go#L69-L77 I'd guess this is to give a bit more visibility into what actually went wrong in the request. Should I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I guess that isn't what happens, the decoder doesn't seem to check what the actual headers are. If you don't have this check and the body is valid the request could be successful or could fail other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you get better errors this way too -- if you try to deserialize proto as json, for instance, you just end up with a really confusing error, whereas if you explicitly check
content-type
, you can provide a better error (plus, it's more correct, and like maybe theoretically avoids weird behavior where some blob of non-json looks enough like json to be deserialized, but is subtly wrong).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is maybe more dangerous that it might seem? What if a new field is added to v1beta1 that's actually different than v1 (they're not guarnateed to be the same, just round-trippable). We could end up making a decision on bad data, right?
I suppose for new named fields this is the same as operating on old types against a new apiserver.
If
v1
andv1beta1
both introduced a field calledfoo
with different types though, this would just break entirely.Is
v1beta1
deprecated? May be worth commenting, cause that makes the chances of that happening much less.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1beta1
is deprecated in 1.19 as per - https://github.com/kubernetes/api/blob/master/authentication/v1beta1/types.go#L25-L31 currently it looks likev1
andv1beta1
are in parity. Given it's deprecated I assume that means we can support this with the expectation that v1beta1 shouldn't chnage, right?If so I can add a note to the comment above. A lot of this was carried over from the Admissions webhook, even this comment tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I've added a comment, mentioning that
v1beta1
is deprecated as of1.19
and will be removed in1.22
per the v1.22 deprecation guide - https://kubernetes.io/docs/reference/using-api/deprecation-guide/#tokenreview-v122