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
Add namespace scoped ParametersReference to IngressClass #99275
Add namespace scoped ParametersReference to IngressClass #99275
Conversation
a9198c6
to
0118acd
Compare
0118acd
to
127f554
Compare
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.
Thanks for your work on this @hbagdi! I took a quick look at this and I think you're headed in the right direction, just a few comments but likely missed some things. Will try to take a more thorough look later this week.
return allErrs | ||
// ValidateIngressClassCreate ensures that IngressClass resources are valid as they are created. | ||
func ValidateIngressClassCreate(ingressClass *networking.IngressClass) field.ErrorList { | ||
allowScope := utilfeature.DefaultFeatureGate.Enabled(features.IngressClassNamespacedParams) |
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.
Although I know this has been done in validation (including by me), I think the proper way to do this is in strategy like what NetworkPolicy is doing (https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/networking/networkpolicy/strategy.go#L52)
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.
@robscott I tried taking up the strategy route but ran into a problem in validation code:
Scope is a new field with a default value. If Scope is force reset in strategy and validation package performs validation on the field, then the validation fails because Scope is an empty string.
We can update the validation code to run only if Scope is not empty but that doesn't seem correct.
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 went ahead and updated the code as per #97058 with one exception that validation for Scope field is feature gated. Does that seem reasonable?
127f554
to
adb13ce
Compare
/retest |
adb13ce
to
9bd1697
Compare
08bcbb5
to
cfa029e
Compare
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.
Thanks for all your work on this!
0f8d184
to
88f6bb8
Compare
f6f6508
to
2ae2f10
Compare
8dd9aec
to
dba3cd9
Compare
@thockin Thanks for the review. I've updated the PR, except #99275 (comment), where I've a question. PTAL when you get a chance. |
// Scope represents if this refers to a cluster or namespace scoped resource. | ||
// This may be set to "Cluster" (default) or "Namespace". | ||
// Field can be enabled with IngressClassNamespacedParams feature gate. | ||
// +optional |
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.
minor: We're adding a +featureGate=IngressClassNamespacedParams
tag for gated fields, which would be both of these
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.
A quick grep revealed no prior art.
I added the tag here as well as in k8s.io/api/networking/*
. Let me know if we only need this in pkg/apis.
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.
We really only needed in the versioned files, but most people sync those comments to the unversioned
@@ -43,3 +46,12 @@ func SetDefaults_NetworkPolicy(obj *networkingv1.NetworkPolicy) { | |||
} | |||
} | |||
} | |||
|
|||
func SetDefaults_IngressClass(obj *networkingv1.IngressClass) { |
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.
minor: you could make this named and typed for the Params struct, so you don't need the additional nil-check (I 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.
Tried but this didn't work. I'm not familiar with codegen here but based on some reading of a few zz_generated.defaults.go
that's not the case.
Furthermore, scheme.AddTypeDefaultingFunc()
requires a type which implements runtime.Object
.
dba3cd9
to
ae7a13b
Compare
/test pull-kubernetes-unit |
// Scope represents if this refers to a cluster or namespace scoped resource. | ||
// This may be set to "Cluster" (default) or "Namespace". | ||
// Field can be enabled with IngressClassNamespacedParams feature gate. | ||
// +optional |
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.
We really only needed in the versioned files, but most people sync those comments to the unversioned
}, fldPath)...) | ||
|
||
if utilfeature.DefaultFeatureGate.Enabled(features.IngressClassNamespacedParams) && params.Scope == nil { | ||
allErrs = append(allErrs, field.Required(fldPath.Child("scope"), "scope is required")) |
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.
nit: the last arg should be "" - the field name and error are encoded in the type
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.
Thanks for this comment. I came to the same conclusion but then my confidence gave way to the code already in this file. Changing it back.
} else { | ||
|
||
if scope == networking.IngressClassParametersReferenceScopeNamespace { | ||
if namespace == "" { |
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.
minor: the downside of using DerefOr is that you will return a "required" error when you should return an "invalid". If the user went out of their way to specify namespace: ""
that is different than not specifying anything (nil)
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.
DerefOr is OK for scope because you check the set of allowed values
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 got rid off DerefOr for namespace.
|
||
if scope == networking.IngressClassParametersReferenceScopeNamespace { | ||
if namespace == "" { | ||
allErrs = append(allErrs, field.Required(fldPath.Child("namespace"), "namespace is required when spec.parameters.scope is set to Namespace")) |
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.
last arg should be (note the quoting)
"`parameters.scope` is set to 'Namespace'"
} | ||
|
||
if scope == networking.IngressClassParametersReferenceScopeCluster && namespace != "" { | ||
allErrs = append(allErrs, field.Forbidden(fldPath.Child("namespace"), "namespace is forbidden when spec.parameters.scope is set to Cluster")) |
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.
last arg should be
"`parameters.scope` is set to 'Cluster'"
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.
The rest of the human-friendly error will be assembed from the error type and field path
} | ||
} | ||
|
||
if scope == networking.IngressClassParametersReferenceScopeCluster && namespace != "" { |
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.
Even worse than above, this will allow a user to set scope: Cluster
and namespace: ""
and it will pass validation. namespace must be nil, not "".
expectedErrs: field.ErrorList{field.Required(field.NewPath("spec.parameters.namespace"), | ||
"namespace is required when spec.parameters.scope is set to Namespace")}, | ||
}, | ||
"namespace is forbidden when scope is Cluster": { |
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.
Add another - same this case but set namespace to "" - it should fail
ae7a13b
to
967aa14
Compare
I can't find anything left to complain about. No fun! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbagdi, thockin 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 |
967aa14
to
a7fc920
Compare
/lgtm |
/test pull-kubernetes-e2e-kind-ipv6 |
@hbagdi: The following test 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. |
/retest |
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
This PR adds two new field
Scope
andNamespace
toIngressClass.spec.Parameters
(feature gated).Which issue(s) this PR fixes:
Fixes #97396
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig network
/cc @robscott
/priority important-soon