[WIP] Fix Webhooks wrongly handling CRD conversion #73839
Conversation
|
@mbohlool: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
@mbohlool: PR needs rebase. 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. |
k8s-ci-robot
added
needs-kind
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mbohlool If they are not already assigned, you can assign the PR to them by writing 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
area/apiserver
sig/api-machinery
|
@mbohlool: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
| @@ -69,7 +69,6 @@ func NewMutatingWebhook(configFile io.Reader) (*Plugin, error) { | |||
|
|
|||
| // SetScheme sets a serializer(NegotiatedSerializer) which is derived from the scheme | |||
| func (a *Plugin) SetScheme(scheme *runtime.Scheme) { | |||
| a.Webhook.SetScheme(scheme) | |||
| if scheme != nil { | |||
| a.scheme = scheme | |||
There was a problem hiding this comment.
what do we use this scheme for now? I'd expect it to have similar issues wherever it was used
There was a problem hiding this comment.
looks like we special-case unstructured in mutatingDispatcher#callAttrMutatingHook and avoid calling a.scheme.New... plumbing the request ObjectCreator there as well would likely be more correct
| GetObjectConverter() runtime.ObjectConvertor | ||
|
|
||
| // TODO(mehdy): Doc | ||
| GetObjectCreator() runtime.ObjectCreater |
There was a problem hiding this comment.
admission uses the defaulter as well:
this seems like it will converge to the set of runtime.Object* interfaces held by request scope over time
There was a problem hiding this comment.
I think a struct of all of these interfaces make more sense as we would need to pass a lot of parameters (and many nils in tests) if we don't do so. Something like a `type ObjectInterfaces struct {'. We can add interfaces to that as needed. Or we can just give access to the RequestScope here. I will read code to see which level of encapsulation make sense here. Recommendations are welcome :)
There was a problem hiding this comment.
I think a struct of all of these interfaces make more sense as we would need to pass a lot of parameters (and many nils in tests) if we don't do so. Something like a `type ObjectInterfaces struct {'.
I agree gathering them makes sense.
What if we made the wrapper an interface:
type ObjectInterfaces interface {
GetObjectCreater() runtime.ObjectCreater
GetObjectTyper() runtime.ObjectTyper
GetObjectDefaulter() runtime.ObjectDefaulter
GetObjectConvertor() runtime.ObjectConvertor
}which RequestScope could implement:
func (r *RequestScope) GetObjectCreater() runtime.ObjectCreater { return r.Creater }
func (r *RequestScope) GetObjectTyper() runtime.ObjectTyper { return r.Typer }
func (r *RequestScope) GetObjectDefaulter() runtime.ObjectDefaulter { return r.Defaulter }
func (r *RequestScope) GetObjectConvertor() runtime.ObjectConvertor { return r.Convertor }and could be passed alongside attributes to admit/validate methods:
type MutationInterface interface {
Interface
// Admit makes an admission decision based on the request attributes
Admit(a Attributes, i ObjectInterfaces) (err error)
}
// ValidationInterface is an abstract, pluggable interface for Admission Control decisions.
type ValidationInterface interface {
Interface
// Validate makes an admission decision based on the request attributes. It is NOT allowed to mutate
Validate(a Attributes, i ObjectInterfaces) (err error)
}that keeps a clearer separation between the attributes being validated and the tools admission can make use of to do the validation
There was a problem hiding this comment.
I'm not thrilled about the signature changes, but I don't think we have any great options:
- put new items into a separate parameter to Admit/Validate
- pro: separates data and helpers
- pro: keeps interface of admission.Attributes the same
- con: changes interface of Admit/Validate (impacts admission plugin implementers)
- put new items into admission.Attributes
- pro: keeps Admit/Validate interfaces the same
- con: mixes data being validated and helpers used to validate
- con: changes interface of admission.Attributes (impacts admission plugin implementers)
- let plugins implement an alternate interface (
Admit2,AdmitWithHelpers, etc) for getting called with helpers- pro: no interface changes
- con: wrapping/unioning/chaining/delegation layers make it likely the alternate method would get masked
- let attributes implement an alternate interface interested admission plugins can attempt to cast it to to extract object helpers
- pro: no interface changes
- con: similar issues with attributes wrapping masking the second implemented interface
- con: this is insane
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = c.Scheme.Convert(obj, out, nil) | ||
| err = converter.Convert(obj, out, nil) |
There was a problem hiding this comment.
also a call here that needs to be switched to the request scope convertor:
| @@ -69,7 +69,6 @@ func NewMutatingWebhook(configFile io.Reader) (*Plugin, error) { | |||
|
|
|||
| // SetScheme sets a serializer(NegotiatedSerializer) which is derived from the scheme | |||
| func (a *Plugin) SetScheme(scheme *runtime.Scheme) { | |||
| a.Webhook.SetScheme(scheme) | |||
| if scheme != nil { | |||
| a.scheme = scheme | |||
| a.jsonSerializer = json.NewSerializer(json.DefaultMetaFactory, scheme, scheme, false) | |||
There was a problem hiding this comment.
and I think we'd need a per-request serializer, which means we need the request-scoped ObjectTyper
| @@ -20,12 +20,13 @@ import ( | |||
| "testing" | |||
|
|
|||
| "k8s.io/apiserver/pkg/admission" | |||
| webhooktesting "k8s.io/apiserver/pkg/admission/plugin/webhook/testing" | |||
There was a problem hiding this comment.
making all of these packages take a dependency on webhooktesting seems weird. most admission plugins don't use the Object{Typer,Converter,Defaulter,Creater} attributes... can we just set them to nil in all the test files for admission plugins that don't use them?
liggitt
added
the
priority/important-soon
k8s-ci-robot
removed
the
needs-priority
|
closing in favor of #74154 |
mbohlool
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
mbohlool
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
APIServer uses a converter to convert the object to external version before sending it to the Mutating or Validation webhook. The converter was a static scheme converter that understands buildin types but not CRDs. This change uses the RequestScope object converter and creator instead which allows both standard types and CRDs to work properly.
fixes #73752
@parhamdoustdar @liggitt this is still work in progress. I need to find a way to add a test for this. I may not also catch all of the changes required for this. Just sending this early to see if you guys have any suggestion about the way I am fixing it.