Adding KEP 2365: IngressClass Namespaced Params #2366

robscott · 2021-01-29T02:43:46Z

This proposes adding a new optional Namespace field to IngressClass Parameters references.

Enhancement Issue: #2365

/sig network
/cc @danehans @cmluciano @hbagdi @bowei
/assign @johnbelamaric @thockin

thockin

Proposal looks OK from an API POV

thockin · 2021-01-29T07:38:13Z

/lgtm

bowei · 2021-01-29T07:53:54Z

keps/sig-network/2365-ingressclass-namespaced-params/README.md

+  // Namespace is the namespace of the resource being referenced. When empty,
+  // this reference will refer to a cluster-scoped resource.
+  // +optional
+  Namespace string


Does this need to be a pointer?

My understanding was that we only need a pointer when there is a distinction between a nil and empty value. In this case "" and nil would be interpreted in the same way. Wherever possible I try to avoid pointers because they can be a pain to work with. We can use omitempty to ensure that "" would be omitted from serialization here. This matches the approach we've taken with service-apis which I believe was inspired by upstream.

No. Convention is that all optional fields should be pointers. Yes, there's an optimization we could (and sometimes have) made when the Go zero-value is not a valid value or is the correct default, but until we have machinery that automates that (i.e. and IDL) I'd rather stick to convention, unless we have a good reason (e.g. scale) not to.

The confusion of +optional and pointers strikes again. 😁

@apelisse has spent time on this problem and @DirectXMan12 is thinking around IDL. No action needed, but something to think about - if we have an IDL should we make that optimization in all possible cases?

How broadly should we be applying this pattern? Should we adjust our types in service-apis as well to use pointers for all optional fields, even if nil and zero values are treated identically?

Please don't have an unset field mean cluster scoped. I recommend an explicit "scope: cluster/namespace" in addition to having the namespace field.

Unset fields are best used to indicate "I don't have an opinion". Our API sadly has lots of anti-examples.

Good call, I've expanded this to add a scope field as well. Is it still appropriate to consider the namespace field optional if it will be required when scope==cluster?

thockin · 2021-01-29T07:55:20Z

Yes, thank you! This is why I want an IDL

…

On Thu, Jan 28, 2021 at 11:54 PM Bowei Du ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In keps/sig-network/2365-ingressclass-namespaced-params/README.md <#2366 (comment)> : > +// IngressClassParametersReference identifies an API object. This can be used +// to specify a cluster-scoped or namespace-scoped resource. +type IngressClassParametersReference struct { + // APIGroup is the group for the resource being referenced. If APIGroup is not + // specified, the specified Kind must be in the core API group. For any other + // third-party types, APIGroup is required. + // +optional + APIGroup *string + // Kind is the type of resource being referenced. + Kind string + // Name is the name of resource being referenced. + Name string + // Namespace is the namespace of the resource being referenced. When empty, + // this reference will refer to a cluster-scoped resource. + // +optional + Namespace string Does this need to be a pointer? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#2366 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVGKBBFP4SYZRZ32B23S4JSSBANCNFSM4WYDYMUQ> .

hbagdi · 2021-01-29T12:01:12Z

keps/sig-network/2365-ingressclass-namespaced-params/README.md

+```
+
+Use of this new `Namespace` field will be guarded a new
+`IngressClassNamespacedParams` feature gate.


Question: How is this going to be incorporated into IngressClassSpec struct in a non-breaking fashion?

// Parameters is a link to a custom resource containing additional // configuration for the controller. This is optional if the controller does // not require extra parameters. // +optional Parameters *api.TypedLocalObjectReference

That's a great question. My understanding was that changes to underlying Go types were acceptable between releases. I could be wrong on that though. Maybe @thockin knows if changing Parameters *api.TypedLocalObjectReference to Parameters *IngressClassNamespacedParams would be a breaking change here.

The Go type is not really part of the API (though it does get exposed in client-go). I think this is why folks like @deads2k are giving guidance to COPY the type you want into your own scope.

I don't know the client-go rules on this sort of 'breaking" change, but ISTR it was somewhat looser than the k8s APIs.

@lavalamp ?

@DirectXMan12 - this might be something an IDL could automate - "clone a library type into my group" ?

The Go type is not really part of the API (though it does get exposed in client-go). I think this is why folks like @deads2k are giving guidance to COPY the type you want into your own scope.

TIL. I had wrongly assumed it to be part of the API.
I do recall to have ran into issues where breaking changes in client-go required some minor refactors in controllers using it. Although in my limited experience, the changes I've seen up to this point have been in client-go itself and not k8s API types.

EDIT: To be clear, I'm not against this change. I simply want to know the compat promise.

The thing we make promises about is the wire format of the API. So the question is what will old/new clients do with the new/old api? That probably works out OK (although I didn't review this super carefully).

Do we care about breaking the public go interface? kinda sorta. We often find a "good reason" to break it. :/ We don't typically consider that when looking at API changes. This would be less problematic if we ever managed to factor the client to split the api version into a separate package from all the logic.

johnbelamaric · 2021-01-29T22:00:42Z

keps/sig-network/2365-ingressclass-namespaced-params/README.md

+
+### Goals
+
+- Allow referencing namespace-scoped Parameters resources.


Are there any security implications here? For example, this would allow user with only access to Namespace "a" to reference an IngressClass that has references to Namespace "b". Is there anyway that can be exploited?

That's a great question. We've thought of parameters as descriptive resources that should not include any sensitive information. Configuration might include the type of infrastructure provisioned for a resource and/or whether it should be exposed externally. In general, these resources should be difficult to write to but easy to read from.

Of course that's all how we intend for this resource to be used. A rather unfortunate consequence of this change is that it would be possible to reference secrets in another namespace. I would hope that controller implementations would not support this, but if this did happen, I think it would have significant security implications. Maybe better guidance for how Parameters resources should be used would suffice here?

Yes, probably documentation for controller authors is a good start. My initial concern, which maybe is totally unfounded, would be that some controller may treat IngressClass like a preset: that is, allow multiple ingress classes for their controller that preset some values for the Ingress, allowing teams to sort of independently attach to that preset...in particular the TLS cert. This is probably paranoia.

Yeah that's a real concern, certainly not the intent of this change but a real potential use of it. I think that specific example is not new here though. This would still be possible with cluster-scoped params since this would theoretically be a reference from params to a secret. I guess someone could embed a cert directly into the params, but of course that's not a good idea.

We will need some much improved documentation for how to use params resources, and especially how not to. I've added a note about this in the alpha release criteria.

johnbelamaric · 2021-01-29T22:06:20Z

PRR looks good but will hold off on approve as it will approve the whole thing.

robscott · 2021-02-05T22:04:32Z

@johnbelamaric @thockin I believe all comments have been resolved, PTAL.

thockin

API nits can be followups or just fix in code PR

thockin · 2021-02-05T23:08:29Z

keps/sig-network/2365-ingressclass-namespaced-params/README.md

+  // Name is the name of resource being referenced.
+  Name string
+  // Scope represents if this refers to a cluster or namespace scoped resource.
+  // This may be set to "cluster" or "namespace".


constants start with Capitals - Cluster, Namespace.

thockin · 2021-02-05T23:09:04Z

keps/sig-network/2365-ingressclass-namespaced-params/README.md

+  // Default: "cluster"
+  Scope string
+  // Namespace is the namespace of the resource being referenced. This field is
+  // required when scope is set to "namespace".


...and not allowed when unset or set to Cluster

thockin · 2021-02-05T23:10:07Z

keps/sig-network/2365-ingressclass-namespaced-params/README.md

+  // Scope represents if this refers to a cluster or namespace scoped resource.
+  // This may be set to "cluster" or "namespace".
+  // Default: "cluster"
+  Scope string


Will need to be a pointer, too

thockin · 2021-02-05T23:10:56Z

Thanks!

/lgtm
/approve

johnbelamaric · 2021-02-06T00:45:50Z

/approve

k8s-ci-robot · 2021-02-06T00:45:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, robscott, 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:

~~keps/prod-readiness/OWNERS~~ [johnbelamaric]
~~keps/sig-network/OWNERS~~ [johnbelamaric,thockin]

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

This patch adds a namespace field to the parametersRef reference. This allows cluster-scoped GatewayClass resource to reference a namespaced-scoped parameters resource. This is in-line with upstream KEP 2365: https://github.com/kubernetes/enhancements/blob/master/keps/prod-readiness/sig-network/2365.yaml Why is it done the way it is done? - Namespace field was not added to LocalObjectReference because that type is referenced in a lot of places. We don't want to add in an optional namespace field in all these places and increase security issues with cross-namespace references. - ObjectReference was not used because upstream discourages its use: https://pkg.go.dev/k8s.io/api/core/v1#ObjectReference. Instead, a new type was introduced as per upstream's guidance. - A new "Cluster" field was added as advised upstream: kubernetes/enhancements#2366 (comment)

k8s-ci-robot assigned johnbelamaric and thockin Jan 29, 2021

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jan 29, 2021

k8s-ci-robot requested review from bowei, cmluciano, danehans and hbagdi January 29, 2021 02:43

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 29, 2021

robscott mentioned this pull request Jan 29, 2021

Namespace Scoped Ingress Class Parameters #2365

Closed

4 tasks

robscott force-pushed the ingressclass-params-namespace branch from c11506e to 2aaa713 Compare January 29, 2021 02:45

thockin reviewed Jan 29, 2021

View reviewed changes

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

bowei reviewed Jan 29, 2021

View reviewed changes

hbagdi reviewed Jan 29, 2021

View reviewed changes

johnbelamaric reviewed Jan 29, 2021

View reviewed changes

robscott force-pushed the ingressclass-params-namespace branch from 2aaa713 to b2ca5e1 Compare January 30, 2021 02:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2021

robscott force-pushed the ingressclass-params-namespace branch 2 times, most recently from 575ef79 to 8d4440c Compare January 30, 2021 02:18

This was referenced Feb 3, 2021

Updates ParametersRef Godocs kubernetes-sigs/gateway-api#523

Closed

Aligns API Fields with K8s Conventions kubernetes-sigs/gateway-api#538

Merged

Adding KEP 2365: IngressClass Namespaced Params

1e409a5

robscott force-pushed the ingressclass-params-namespace branch from 8d4440c to 1e409a5 Compare February 5, 2021 22:03

thockin reviewed Feb 5, 2021

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2021

k8s-ci-robot merged commit 582a8e8 into kubernetes:master Feb 6, 2021

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

hbagdi mentioned this pull request Feb 11, 2021

allow namespace-scoped parametersRef kubernetes-sigs/gateway-api#543

Merged

hbagdi mentioned this pull request Aug 2, 2021

Adding GEP 724: Refresh Route-Gateway Binding kubernetes-sigs/gateway-api#725

Merged

robscott mentioned this pull request Aug 20, 2021

SIG-NETWORK v1alpha2 Review kubernetes-sigs/gateway-api#780

Closed

robscott mentioned this pull request Sep 27, 2021

Removing scope fields from object references kubernetes-sigs/gateway-api#882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding KEP 2365: IngressClass Namespaced Params #2366

Adding KEP 2365: IngressClass Namespaced Params #2366

robscott commented Jan 29, 2021 •

edited

Loading

thockin left a comment

thockin commented Jan 29, 2021

bowei Jan 29, 2021

robscott Jan 29, 2021

thockin Jan 29, 2021

hbagdi Jan 29, 2021

thockin Jan 29, 2021

robscott Jan 29, 2021

lavalamp Jan 29, 2021

robscott Jan 30, 2021

thockin commented Jan 29, 2021 via email

hbagdi Jan 29, 2021

robscott Jan 29, 2021

thockin Jan 29, 2021

thockin Jan 29, 2021

hbagdi Jan 29, 2021 •

edited

Loading

lavalamp Jan 29, 2021

johnbelamaric Jan 29, 2021

robscott Jan 29, 2021

johnbelamaric Jan 30, 2021

robscott Jan 30, 2021

johnbelamaric commented Jan 29, 2021

robscott commented Feb 5, 2021

thockin left a comment

thockin Feb 5, 2021

thockin Feb 5, 2021

thockin Feb 5, 2021

thockin commented Feb 5, 2021

johnbelamaric commented Feb 6, 2021

k8s-ci-robot commented Feb 6, 2021


		### Goals

		- Allow referencing namespace-scoped Parameters resources.

Adding KEP 2365: IngressClass Namespaced Params #2366

Adding KEP 2365: IngressClass Namespaced Params #2366

Conversation

robscott commented Jan 29, 2021 • edited Loading

thockin left a comment

Choose a reason for hiding this comment

thockin commented Jan 29, 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

thockin commented Jan 29, 2021 via email

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 Jan 29, 2021 • edited Loading

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

johnbelamaric commented Jan 29, 2021

robscott commented Feb 5, 2021

thockin 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

thockin commented Feb 5, 2021

johnbelamaric commented Feb 6, 2021

k8s-ci-robot commented Feb 6, 2021

robscott commented Jan 29, 2021 •

edited

Loading

hbagdi Jan 29, 2021 •

edited

Loading