-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Provide access to admission.Request in custom validator/defaulter #1950
✨ Provide access to admission.Request in custom validator/defaulter #1950
Conversation
type requestContextKey struct{} | ||
|
||
// RequestFromContext returns an admission.Request from ctx. | ||
func RequestFromContext(ctx context.Context) (Request, error) { |
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.
It would potentially (?) be a nicer UX if we could omit the error return parameter. But returning an empty Request if it is not found feels 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.
+1, an error return parameter will be nicer, even if that should not happen for custom validator/defaulter.
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.
The error return message seems ok, better to have the clarity rather than checking nils?
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.
Yup and a lot better then checking empty fields of an empty struct (as it's not a pointer)
@@ -253,3 +253,21 @@ func StandaloneWebhook(hook *Webhook, opts StandaloneOptions) (http.Handler, err | |||
} | |||
return metrics.InstrumentedHook(opts.MetricsPath, hook), nil | |||
} | |||
|
|||
// requestContextKey is how we find the admission.Request in a context.Context. | |||
type requestContextKey struct{} |
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.
How about a const string like "webhookRequest"
?
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 think the idea behind using an unexported struct is to only make it possible to retrieve it with the exported RequestFromContext
func.
(I took this pattern from how go-logr stores a Logger in a context)
type requestContextKey struct{} | ||
|
||
// RequestFromContext returns an admission.Request from ctx. | ||
func RequestFromContext(ctx context.Context) (Request, error) { |
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.
+1, an error return parameter will be nicer, even if that should not happen for custom validator/defaulter.
03350a5
to
608fd80
Compare
LGTM pending tests |
608fd80
to
bd4fe84
Compare
@vincepri PTAL Added a test for the ctx utils and extended the tests which were added when we introduced the custom validator/defaulter |
bd4fe84
to
3eaf673
Compare
Seems like an independent flake to me (https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-runtime/1950/pull-controller-runtime-test-master/1544692736802689024) /retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Would be nice if we can cherry-pick this to v0.12.x /cherry-pick release-0.12 |
@sbueringer: new pull request created: #1952 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR provides access to the admisison.Request in custom validator and defaulter.
Fixes #1949