✨ Provide access to admission.Request in custom validator/defaulter #1950
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
| type requestContextKey struct{} | ||
|
|
||
| // RequestFromContext returns an admission.Request from ctx. | ||
| func RequestFromContext(ctx context.Context) (Request, error) { |
There was a problem hiding this comment.
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.
+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.
The error return message seems ok, better to have the clarity rather than checking nils?
There was a problem hiding this comment.
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.
How about a const string like "webhookRequest"?
There was a problem hiding this comment.
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.
+1, an error return parameter will be nicer, even if that should not happen for custom validator/defaulter.
k8s-ci-robot
added
the
cncf-cla: yes
sbueringer
force-pushed
the
pr-webhook-adm-request
branch
from
03350a5
to
608fd80
Compare
sbueringer
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
sbueringer
changed the title
|
LGTM pending tests |
sbueringer
force-pushed
the
pr-webhook-adm-request
branch
from
608fd80
to
bd4fe84
Compare
k8s-ci-robot
added
size/M
|
@vincepri PTAL Added a test for the ctx utils and extended the tests which were added when we introduced the custom validator/defaulter |
sbueringer
force-pushed
the
pr-webhook-adm-request
branch
from
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 |
k8s-ci-robot
added
the
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 |
k8s-ci-robot
added
the
approved
|
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