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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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{} | ||
|
||
// 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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) |
||
if v, ok := ctx.Value(requestContextKey{}).(Request); ok { | ||
return v, nil | ||
} | ||
|
||
return Request{}, errors.New("admission.Request not found in context") | ||
} | ||
|
||
// NewContextWithRequest returns a new Context, derived from ctx, which carries the | ||
// provided admission.Request. | ||
func NewContextWithRequest(ctx context.Context, req Request) context.Context { | ||
return context.WithValue(ctx, requestContextKey{}, req) | ||
} |
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)