-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
ValidatingAdmissionPolicy: support namespace access #118267
Conversation
7607329
to
b8e9012
Compare
/triage accepted |
/cc |
/assign |
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.
Overall it's a bit unclear what happens for cluster-scoped objects without a namespace. I did not see a test for this situation either. Otherwise pretty clean and concise.
staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/interface.go
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go
Outdated
Show resolved
Hide resolved
// Certain nested fields in Namespace (e.g. managedFields, ownerReferences etc.) are omitted in the generated DeclType | ||
// by design. |
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'm concerned about the maintainability of this function because if later a new field is added to metadata or to to namespace.spec, it will be very hard for a PR author to know to also modify this function to add the field. We useSchemaDeclType()
conversion for object
even though it has managedFields
and ownerReferences
. Is it really essential that we omit those fields? Could we just call SchemaDeclType()
here? If we REALLY don't want those fields, are we going to remove them from object
and oldObject
and self
and oldSelf
when they refer to a root object as well? (the latter is a breaking change)
If we decide to omit the fields (after addressing my above concerns), should we instead offer a SchemaDeclType()
style function that always omits the fields from root objects (note that the function does take a "isRoot" param).
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.
If use the schemaResolver that the type checker uses, these SSA fields are already omitted.
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.
In this way, the type is built from namespace openAPI definition. The building process does not require networking either but there is a JSON marshall/unmarshall of a small schema object.
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.
Anything we can do here to avoid redefining the schema for namespace (and incurring the maintenance cost of having the schema defined in yet another place) would be really really nice.
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've reconsidered this matter. I think it is not worthwhile to wire in the whole OpenAPI definitions only for one type. I am planning to compromise with a test to ensure this hand-crafted namespace schema matches that of OpenAPI. What do you think?
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.
Let's postponed the check a bit later. The handcrafted type looks consistent with v1.Namespace
. We can find a way to sync this later in next release since there is no change of behavior on the user's side.
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've reconsidered this matter. I think it is not worthwhile to wire in the whole OpenAPI definitions only for one type. I am planning to compromise with a test to ensure this hand-crafted namespace schema matches that of OpenAPI. What do you think?
SGTM!
staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go
Outdated
Show resolved
Hide resolved
/label tide/merge-method-squash |
/lgtm /assign @deads2k |
LGTM label has been added. Git tree hash: 2de2a6fbe8f2e12b744a5f183aa447489ac03dce
|
Typo fix. |
re-run codegen |
api change lgtm /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cici37, deads2k, jpbetz 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 |
/lgtm Thank you! |
/lgtm |
LGTM label has been added. Git tree hash: b423d5ce4055695071544d148d0d50803ea46fe1
|
* Support namespace access from cel expression in validatingadmissionpolicy. * Whitelist the exposed fields in namespace object and add test * better handling of cluster-scoped resources. * [API REVIEW] namespaceObject in Expression doc. * compatibility with composition. * generated: ./hack/update-codegen.sh && ./hack/update-openapi-spec.sh * workaround namespace of namespace is unexpectedly set. * basic test coverage for namespaceObject. --------- Co-authored-by: Jiahui Feng <jhf@google.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
Support namespace access from CEL expression.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: