Use Request Object interfaces instead of static scheme that is more appropriate for CRDs #74154
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
area/apiserver
sig/api-machinery
|
Approach looks good overall. One compile issue, and a couple import order issues |
|
/test pull-kubernetes-integration |
mbohlool
force-pushed
the
gimli
branch
5 times, most recently
from
6886175
to
1b09deb
Compare
| */ | ||
|
|
||
| package admission | ||
|
|
There was a problem hiding this comment.
name this something other than test.go. is there a reason to limit this impl to tests?
There was a problem hiding this comment.
It is only used in test now. I think I can just move it somewhere else.
There was a problem hiding this comment.
renamed to util.go
|
One nit on the helper filename, and the comment about not making patcher use the admission interface to collect object methods (leaving as is or declaring its own interface would be ok… I'd probably leave as is for this PR) |
|
All green. Ready to go! @liggitt |
| // ObjectInterfaces is an interface used by AdmissionController to get object interfaces | ||
| // such as Converter or Defaulter. These interfaces are normally coming from Request Scope | ||
| // to handle special cases like CRDs. | ||
| type ObjectInterfaces interface { |
There was a problem hiding this comment.
@mbohlool @liggitt adding this as a parameter to all admission controller would widely change all the admissions' interface. how about injecting a dynamic scheme getter func from the initializers instead a fixed scheme? sth like func GetScheme(gvk), if the gvk's not registered in the legacy scheme(which is, the requesting resource is not standard), it returns the extension scheme.
There was a problem hiding this comment.
how about injecting a dynamic scheme getter func from the initializers instead a fixed scheme? sth like
func GetScheme(gvk), if the gvk's not registered in the legacy scheme(which is, the requesting resource is not standard), it returns the extension scheme.
That seems more complex and error-prone. The choice is not between the legacy scheme and the extensions scheme, but between the legacy scheme and a per-resource converter/typer/defaulter for each custom resource. The object interfaces for the object being handled are in the rest handler, and fit much more naturally as a parameter to admit/validate.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, mbohlool 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
approved
liggitt
added
kind/bug
k8s-ci-robot
removed
needs-kind
| } | ||
|
|
||
| // 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) (err error) | ||
| Validate(a Attributes, o ObjectInterfaces) (err error) |
There was a problem hiding this comment.
This silently disabled validation plugins of 3rdparty code bases. We should start caring about compatibility. We could have added ObjectInterfaces to the attributes without damaging third parties.
There was a problem hiding this comment.
Changing either interface would have broken compatibility. I'd strongly recommend type assertions downstream to ensure interface matching.
The admission plugin implementation uses an static scheme (set by .SetScheme method of the Plugin). While this works for standard types, it does not work for CRDs and resulted in bugs such as #73752. This change remove the static schema, and added an ObjectInterfaces which implemented by RequestScope and passed to .Admit and .Validate calls.
fixes #73752
@parhamdoustdar @liggitt