Add namespace scoped ParametersReference to IngressClass #99275

hbagdi · 2021-02-20T20:16:39Z

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 and Namespace to IngressClass.spec.Parameters (feature gated).

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

IngressClass resource can now reference a resource in a specific namespace 
for implementation-specific configuration(previously only Cluster-level resources were allowed). 
This feature can be enabled using the IngressClassNamespacedParams feature gate.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig network
/cc @robscott
/priority important-soon

pkg/apis/networking/v1beta1/conversion.go

robscott

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.

pkg/apis/networking/v1/defaults_test.go

robscott · 2021-02-24T20:33:30Z

pkg/apis/networking/validation/validation.go

-	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)


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)

@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.

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?

hbagdi · 2021-02-25T10:06:05Z

/retest

robscott

Thanks for all your work on this!

pkg/apis/networking/types.go

pkg/apis/networking/v1/defaults.go

pkg/apis/networking/v1/defaults_test.go

pkg/features/kube_features.go

pkg/apis/networking/types.go

staging/src/k8s.io/api/networking/v1/types.go

staging/src/k8s.io/api/networking/v1beta1/types.go

hbagdi · 2021-03-05T12:30:28Z

@thockin Thanks for the review. I've updated the PR, except #99275 (comment), where I've a question. PTAL when you get a chance.

thockin · 2021-03-05T17:03:20Z

pkg/apis/networking/types.go

+	// 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


minor: We're adding a +featureGate=IngressClassNamespacedParams tag for gated fields, which would be both of these

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.

We really only needed in the versioned files, but most people sync those comments to the unversioned

thockin · 2021-03-05T17:07:58Z

pkg/apis/networking/v1/defaults.go

@@ -43,3 +46,12 @@ func SetDefaults_NetworkPolicy(obj *networkingv1.NetworkPolicy) {
 		}
 	}
 }
+
+func SetDefaults_IngressClass(obj *networkingv1.IngressClass) {


minor: you could make this named and typed for the Params struct, so you don't need the additional nil-check (I think)

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.

pkg/apis/networking/validation/validation.go

hbagdi · 2021-03-05T21:00:49Z

/test pull-kubernetes-unit
/test pull-kubernetes-e2e-ubuntu-gce-network-policies

thockin · 2021-03-05T20:30:47Z

pkg/apis/networking/types.go

+	// 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


We really only needed in the versioned files, but most people sync those comments to the unversioned

thockin · 2021-03-05T20:39:54Z

pkg/apis/networking/validation/validation.go

+	}, fldPath)...)
+
+	if utilfeature.DefaultFeatureGate.Enabled(features.IngressClassNamespacedParams) && params.Scope == nil {
+		allErrs = append(allErrs, field.Required(fldPath.Child("scope"), "scope is required"))


nit: the last arg should be "" - the field name and error are encoded in the type

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.

thockin · 2021-03-05T20:42:15Z

pkg/apis/networking/validation/validation.go

+		} else {
+
+			if scope == networking.IngressClassParametersReferenceScopeNamespace {
+				if namespace == "" {


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)

DerefOr is OK for scope because you check the set of allowed values

I got rid off DerefOr for namespace.

thockin · 2021-03-05T20:45:12Z

pkg/apis/networking/validation/validation.go

+
+			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"))


last arg should be (note the quoting)

"`parameters.scope` is set to 'Namespace'"

thockin · 2021-03-05T20:48:19Z

pkg/apis/networking/validation/validation.go

+			}
+
+			if scope == networking.IngressClassParametersReferenceScopeCluster && namespace != "" {
+				allErrs = append(allErrs, field.Forbidden(fldPath.Child("namespace"), "namespace is forbidden when spec.parameters.scope is set to Cluster"))


last arg should be

"`parameters.scope` is set to 'Cluster'"

The rest of the human-friendly error will be assembed from the error type and field path

thockin · 2021-03-05T20:51:37Z

pkg/apis/networking/validation/validation.go

+				}
+			}
+
+			if scope == networking.IngressClassParametersReferenceScopeCluster && namespace != "" {


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 "".

thockin · 2021-03-05T21:03:53Z

pkg/apis/networking/validation/validation_test.go

+			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": {


Add another - same this case but set namespace to "" - it should fail

thockin · 2021-03-05T23:01:07Z

I can't find anything left to complain about. No fun!

/lgtm
/approve

k8s-ci-robot · 2021-03-05T23:01:40Z

[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:

~~api/OWNERS~~ [thockin]
~~pkg/apis/networking/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [thockin]
~~pkg/printers/OWNERS~~ [thockin]
~~pkg/registry/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/client-go/OWNERS~~ [thockin]
~~staging/src/k8s.io/kubectl/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hbagdi · 2021-03-06T19:16:58Z

@thockin @robscott Could I get a /lgtm again? I had to rebase due to a conflict in pkg/features/kube_features.go.

robscott · 2021-03-06T20:34:59Z

/lgtm

hbagdi · 2021-03-06T21:36:56Z

/test pull-kubernetes-e2e-kind-ipv6

k8s-ci-robot · 2021-03-06T23:02:34Z

@hbagdi: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-integration	`a7fc920`	link	`/test pull-kubernetes-integration`

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.

robscott · 2021-03-06T23:14:38Z

/retest

k8s-ci-robot requested a review from robscott February 20, 2021 20:16

hbagdi force-pushed the feat/ingress-class-namespaced-params branch from a9198c6 to 0118acd Compare February 20, 2021 20:18

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2021

hbagdi force-pushed the feat/ingress-class-namespaced-params branch from 0118acd to 127f554 Compare February 20, 2021 20:20

hbagdi commented Feb 20, 2021

View reviewed changes

pkg/apis/networking/v1beta1/conversion.go Outdated Show resolved Hide resolved

kendallroden mentioned this pull request Feb 24, 2021

Namespace Scoped Ingress Class Parameters kubernetes/enhancements#2365

Closed

4 tasks

robscott reviewed Feb 24, 2021

View reviewed changes

hbagdi force-pushed the feat/ingress-class-namespaced-params branch from 127f554 to adb13ce Compare February 25, 2021 10:00

hbagdi force-pushed the feat/ingress-class-namespaced-params branch from adb13ce to 9bd1697 Compare February 25, 2021 11:32

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 25, 2021

hbagdi force-pushed the feat/ingress-class-namespaced-params branch 2 times, most recently from 08bcbb5 to cfa029e Compare February 25, 2021 12:58

robscott reviewed Feb 26, 2021

View reviewed changes

hbagdi force-pushed the feat/ingress-class-namespaced-params branch 2 times, most recently from 0f8d184 to 88f6bb8 Compare February 26, 2021 09:02

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 26, 2021

hbagdi force-pushed the feat/ingress-class-namespaced-params branch 2 times, most recently from f6f6508 to 2ae2f10 Compare March 5, 2021 08:28

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 5, 2021

hbagdi force-pushed the feat/ingress-class-namespaced-params branch 2 times, most recently from 8dd9aec to dba3cd9 Compare March 5, 2021 11:52

thockin reviewed Mar 5, 2021

View reviewed changes

hbagdi force-pushed the feat/ingress-class-namespaced-params branch from dba3cd9 to ae7a13b Compare March 5, 2021 19:16

thockin reviewed Mar 5, 2021

View reviewed changes

hbagdi force-pushed the feat/ingress-class-namespaced-params branch from ae7a13b to 967aa14 Compare March 5, 2021 21:33

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2021

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 5, 2021

Add namespace scoped ParametersReference to IngressClass

a7fc920

hbagdi force-pushed the feat/ingress-class-namespaced-params branch from 967aa14 to a7fc920 Compare March 6, 2021 19:15

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2021

k8s-ci-robot merged commit 4bf8503 into kubernetes:master Mar 6, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 6, 2021

hbagdi deleted the feat/ingress-class-namespaced-params branch March 7, 2021 07:28

liggitt moved this from Assigned to API review completed, 1.21 in API Reviews Mar 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespace scoped ParametersReference to IngressClass #99275

Add namespace scoped ParametersReference to IngressClass #99275

hbagdi commented Feb 20, 2021 •

edited

robscott left a comment

robscott Feb 24, 2021

hbagdi Feb 25, 2021

hbagdi Feb 25, 2021

hbagdi commented Feb 25, 2021

robscott left a comment

hbagdi commented Mar 5, 2021

thockin Mar 5, 2021

hbagdi Mar 5, 2021

thockin Mar 5, 2021

thockin Mar 5, 2021

hbagdi Mar 5, 2021

hbagdi commented Mar 5, 2021

thockin Mar 5, 2021

thockin Mar 5, 2021

hbagdi Mar 5, 2021

thockin Mar 5, 2021

thockin Mar 5, 2021

hbagdi Mar 5, 2021

thockin Mar 5, 2021

thockin Mar 5, 2021

thockin Mar 5, 2021

thockin Mar 5, 2021

thockin Mar 5, 2021

thockin commented Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

hbagdi commented Mar 6, 2021

robscott commented Mar 6, 2021

hbagdi commented Mar 6, 2021

k8s-ci-robot commented Mar 6, 2021

robscott commented Mar 6, 2021

Add namespace scoped ParametersReference to IngressClass #99275

Add namespace scoped ParametersReference to IngressClass #99275

Conversation

hbagdi commented Feb 20, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Feb 25, 2021

robscott left a comment

Choose a reason for hiding this comment

hbagdi commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

hbagdi commented Mar 6, 2021

robscott commented Mar 6, 2021

hbagdi commented Mar 6, 2021

k8s-ci-robot commented Mar 6, 2021

robscott commented Mar 6, 2021

hbagdi commented Feb 20, 2021 •

edited