change SupportedFeatures API to a list of subobjects #3200

LiorLieberman · 2024-07-17T10:55:21Z

Note THIS PR IS A BREAKING CHANGE OF AN EXPERIMENTAL FEATURE

https://gateway-api.sigs.k8s.io/contributing/devguide/?h=experimental#adding-experimental-fields does not specify a protocol to align all implementations that implemented the experimental field with the breaking change, so feedback is most welcomed.

AFAIK, to date, this feature is only supported in envoy-gw and istio.

/cc @arkodg @howardjohn @robscott @youngnick @shaneutt @mlavacca

What type of PR is this?
/kind feature
/kind gep
/kind cleanup

What this PR does / why we need it:
This PR aligns supportedFeatures GEP implementation according to #3164 with the feedback we got from the community.

Which issue(s) this PR fixes:

#3164, #2163

Does this PR introduce a user-facing change?:

SupportedFeatures field in Gateway Class status are now presented as a list of objects

arkodg · 2024-07-17T19:16:14Z

apis/v1/gatewayclass_types.go

+
+type SupportedFeature struct {
+	Name FeatureName `json:"name"`
+}


speaking on behalf of Envoy Gateway, where we enabled this feature by default, our users have not found just the name to be very useful. As mentioned earlier an additional optional field like Message / Description / Info can inform the user, specific caveats that may be implementation specific that have been intentionally kept open in the API (where SHOULD and SHOULD NOT are used for e.g. in https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.HTTPRequestRedirectFilter)

I think the rationale for name is that it leaves room to add additional fields in the future, but we couldn't quite agree on what those should look like yet. I do generally agree that a message field could be helpful here, but I think we'd want to agree on formatting, when/how it should be populated, etc, before actually adding it to the API.

that rationale makes sense @robscott, a follow question is, are there any implementations that would like to implement this API in this current form ?

IMO, this iteration of the API is primarily about automation. From the manual form of kubectl get gatewayclass -oyaml | grep Redirect to the more ideal goal where we have tooling like gwctl that can automatically warn if a user is trying to use an unsupported feature. Presumably in that case gwctl would also warn if a GatewayClass had not populated supported features.

I think what you're describing is primarily useful for someone that is trying to look at the full GatewayClass status and understand the nuance of what's supported, which I think is helpful, but not quite as high priority as the UX I described above.

This API is the current form will benefit tooling like gwctl, but imo is not useful enough for end users who do not use tools like gwctl, which imo should drive the design of this API.

I don't think user documentation belongs in CRD Status at all. IMO this should be removed from the API and documented in implementations websites.

I don't think user documentation belongs in CRD Status at all. IMO this should be removed from the API and documented in implementations websites.

I am mostly plus one to this, except from the supported features/profiles which could be really useful and improve the UX(through warning and automations) for an api where implementations not necessarily support all features.

I don't think user documentation belongs in CRD Status at all. IMO this should be removed from the API and documented in implementations websites.

By "this", @howardjohn, do you mean SupportedFeatures in general, or the idea of a message field on each?

Personally, I'd like to see us do a few things once we get the current change in:

Define all the SupportedFeatures in Gateway API code, with a standard description of each. Currently the only definition is in the test suite, but I think we may need to promote this into a separate package somewhere so that both tooling and the conformance suite can use it. Conceivably, we could also include support state and "lastTransitionVersion" or something to record the last time it changed too.

Have a generated page on the website that lists all the supported features and their associated fields.

Ensure that any Gateway API guides on the project page list which features are required for that guide, and encourage guide and documentation authors to list the required features on their guides as well.

If we do the above, a lot of the reason for having this information inlined in the GatewayClass status goes away, because the SupportedFeature name allows users or code to look up any associated data in the canonical location.

Having some form of SupportedFeature in GatewayClass status, along with a canonical list of SupportedFeature names, allows us to move towards having the conformance suite determine which tests to run by looking at the GatewayClass as well, which makes it theoretically possible to run the vanilla conformance test suite against any Gateway API implementation, similarly to how you can run upstream Kubernetes conformance against any cluster and get a reasonable result.

I think that it's still worth keeping this list as a list of named subobjects, even if we only have name for now, because of the expandability it gives us later - for the cost of a bunch of superfluous (for now) name: strings.

I'm +1 to the idea proposed here and how it's implemented (using a slice of objects with a single name field). I initially may have had some second thoughts around the need to have SupportedFeatures but have gotten around that after reading all the discussions :)

I like the vision that @youngnick has portrayed around the overlap with Conformance Suite / Profiles and how eventually this may serve as an input to generating the Conformance reports.

For this to be really useful with tooling like gwctl, we definitely should work towards a follow up for this change to come with with that canonical list of SupportedFeatures, or, I'm assuming we intend to use https://github.com/kubernetes-sigs/gateway-api/blob/d400a8b69ef19578d55896852b75d7d5e6f9a30c/pkg/features/features.go as the set of possible values while the field is in experimental.

+1 on having this map of struct with just the name field for the time being. I agree that putting documentation or link to docs in the resource status is something we should try to avoid and lean toward creating a proper documentation in the canonical place instead.

I +1 the plan stated above by Nick, with just a small nit: we already have all the features in a separate package, but we need to write appropriate go docs for all the features, as currently, they are mostly in the form of This Option indicates support for feature X, which is not useful at all for users.

Conceivably, we could also include support state and "lastTransitionVersion" or something to record the last time it changed too.

Big +1 . Just listing the features is not enough. The addition of some metadata on the features is something fundamental in my opinion, to track the feature lifecycle and its implementation state.

howardjohn · 2024-07-17T22:46:03Z

apis/v1/gatewayclass_types.go

@@ -261,9 +261,10 @@ type GatewayClassStatus struct {
 	Conditions []metav1.Condition `json:"conditions,omitempty"`



Istio basically messed up by not having a feature flag for the old setting (like Envoy Gateway). This is the only field we have this issue on since its the only experimental write (not read) field.

So if this is merged, all users who upgrade to a newer GW API are going to get ~10 pages of error logs in Istio. I can understand its experimental and that may not factor in much, but just saying.

I was about to leave a comment to that extent, asking about failure modes of existing controllers here. Does the failure become any less painful if we rename/remove the field as part of this transition?

just to learn more, what do you mean old setting? the supportedFeatures API in its current form?

I agree that it sucks that a breaking change will produce a bunch of errors, but it's the risk we all run when supporting experimental features.

Renaming the field would make things easier, but I can't think of a name that makes sense.

This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * istio#50851

LiorLieberman · 2024-07-18T15:17:29Z

Unfortunately we can't get enough feedback in regards to what users would really value here while this feature is experimental and doesn't have many implementations. Also, given that it is a UI change first, and additional benefits, such as warning or blocking offending/unsupported configurations from being applied to the cluster, can only come later after this feature is implemented, its even harder to get the feedback.

Maybe we should consider pausing this effort for now, until we have more open issues that this api can solve. OR until other implementations want to implement that and believe that their users would benefit from this.

I still believe it is valuable, and as a user, I would very much value a tool or an automation that warns me when I apply a CRD with an unsupported feature. But I think we would need to get more concise arguments AND an implementation that is willing to push it forward.

robscott · 2024-07-22T18:12:45Z

Thanks @LiorLieberman! This is a remarkably tricky transition, and unfortunately results in a breaking change, but these kinds of changes are allowed in experimental channel of our versioning policy.

/approve
/hold for more approvals/feedback

* Drop SupportedFeatures from gateway-api This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * #50851 * add note

This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * istio#50851

* Drop SupportedFeatures from gateway-api This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * #50851 * add note --------- Co-authored-by: John Howard <john.howard@solo.io>

youngnick · 2024-07-29T07:08:00Z

I think that this is a good basis for moving forward, so I'm happy to LGTM as it stands. Would like to see some more LGTMs from other folks though. I'm happy to make sure that Cilium will implement this iteration of SupportedFeatures once this is locked down.

(I didn't use the command just in case though)

mlavacca

I'm ok with moving this change forward as is, thanks @LiorLieberman!

/lgtm

mlavacca · 2024-07-29T08:37:08Z

apis/v1/gatewayclass_types.go

+
+type SupportedFeature struct {
+	Name FeatureName `json:"name"`
+}


+1 on having this map of struct with just the name field for the time being. I agree that putting documentation or link to docs in the resource status is something we should try to avoid and lean toward creating a proper documentation in the canonical place instead.

I +1 the plan stated above by Nick, with just a small nit: we already have all the features in a separate package, but we need to write appropriate go docs for all the features, as currently, they are mostly in the form of This Option indicates support for feature X, which is not useful at all for users.

Conceivably, we could also include support state and "lastTransitionVersion" or something to record the last time it changed too.

Big +1 . Just listing the features is not enough. The addition of some metadata on the features is something fundamental in my opinion, to track the feature lifecycle and its implementation state.

k8s-ci-robot · 2024-07-29T08:39:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiorLieberman, mlavacca, robscott

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:

~~OWNERS~~ [robscott]

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

LiorLieberman · 2024-07-31T10:36:01Z

Unhold this after as agreed in the community meeting yesterday.
/unhold

…#3200)

LiorLieberman · 2024-08-09T09:03:46Z

I just found that Cilium also implemented it. I opened cilium/cilium#34266.

We need to make sure that the release of 1.2 would not break the controller. Maybe this is another opportunity to re-announce that this is an experimental breaking api change, in case other implementations implemented it.

/cc @robscott @youngnick FYI before the release

change SupportedFeatures API to a list of subobjects

7b11d39

k8s-ci-robot requested a review from arkodg July 17, 2024 10:55

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 17, 2024

k8s-ci-robot requested review from howardjohn, mlavacca and robscott July 17, 2024 10:55

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 17, 2024

k8s-ci-robot requested review from shaneutt and youngnick July 17, 2024 10:55

arkodg reviewed Jul 17, 2024

View reviewed changes

howardjohn reviewed Jul 17, 2024

View reviewed changes

howardjohn added a commit to howardjohn/istio that referenced this pull request Jul 17, 2024

Drop SupportedFeatures from gateway-api

da94aed

This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * istio#50851

howardjohn mentioned this pull request Jul 17, 2024

Drop SupportedFeatures from gateway-api istio/istio#52132

Merged

howardjohn added a commit to howardjohn/istio that referenced this pull request Jul 17, 2024

Drop SupportedFeatures from gateway-api

3aa8cb6

This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * istio#50851

arkodg mentioned this pull request Jul 18, 2024

disable writing into GatewayClass.Status.SupportedFeatures envoyproxy/gateway#3888

Merged

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 22, 2024

istio-testing pushed a commit to istio-testing/istio that referenced this pull request Jul 22, 2024

Drop SupportedFeatures from gateway-api

8db6a6b

This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * istio#50851

istio-testing pushed a commit to istio-testing/istio that referenced this pull request Jul 22, 2024

Drop SupportedFeatures from gateway-api

789e5fc

This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * istio#50851

mlavacca approved these changes Jul 29, 2024

View reviewed changes

k8s-ci-robot assigned mlavacca Jul 29, 2024

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

mlavacca mentioned this pull request Jul 29, 2024

Gateway API SupportedFeatures field Kong/gateway-operator#434

Open

1 task

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2024

k8s-ci-robot merged commit a46405c into kubernetes-sigs:main Jul 31, 2024
8 checks passed

mikemorris pushed a commit to mikemorris/gateway-api that referenced this pull request Aug 1, 2024

change SupportedFeatures API to a list of subobjects (kubernetes-sigs…

e244a1b

…#3200)

xtineskim pushed a commit to xtineskim/gateway-api that referenced this pull request Aug 8, 2024

change SupportedFeatures API to a list of subobjects (kubernetes-sigs…

67b173d

…#3200)

LiorLieberman mentioned this pull request Aug 9, 2024

Gateway API SupportedFeatures API change cilium/cilium#34266

Closed

3 tasks

LiorLieberman mentioned this pull request Aug 9, 2024

gateway-api: Align GatewayClass SupportedFeatures cilium/cilium#34271

Merged

levikobi mentioned this pull request Aug 15, 2024

Align gateway class supported features with upstream changes envoyproxy/gateway#4059

Open

rtribotte mentioned this pull request Sep 17, 2024

Add supported features to the Gateway API GatewayClass status traefik/traefik#11056

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change SupportedFeatures API to a list of subobjects #3200

change SupportedFeatures API to a list of subobjects #3200

LiorLieberman commented Jul 17, 2024 •

edited

Loading

arkodg Jul 17, 2024 •

edited

Loading

robscott Jul 17, 2024

arkodg Jul 17, 2024

robscott Jul 17, 2024

arkodg Jul 17, 2024

howardjohn Jul 17, 2024

LiorLieberman Jul 18, 2024

youngnick Jul 23, 2024

gauravkghildiyal Jul 24, 2024

mlavacca Jul 29, 2024

howardjohn Jul 17, 2024

robscott Jul 17, 2024

LiorLieberman Jul 18, 2024

youngnick Jul 23, 2024

LiorLieberman commented Jul 18, 2024

robscott commented Jul 22, 2024

youngnick commented Jul 29, 2024 •

edited

Loading

mlavacca left a comment

mlavacca Jul 29, 2024

k8s-ci-robot commented Jul 29, 2024

LiorLieberman commented Jul 31, 2024

LiorLieberman commented Aug 9, 2024 •

edited

Loading

		@@ -261,9 +261,10 @@ type GatewayClassStatus struct {
		Conditions []metav1.Condition `json:"conditions,omitempty"`

change SupportedFeatures API to a list of subobjects #3200

change SupportedFeatures API to a list of subobjects #3200

Conversation

LiorLieberman commented Jul 17, 2024 • edited Loading

arkodg Jul 17, 2024 • 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

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

LiorLieberman commented Jul 18, 2024

robscott commented Jul 22, 2024

youngnick commented Jul 29, 2024 • edited Loading

mlavacca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 29, 2024

LiorLieberman commented Jul 31, 2024

LiorLieberman commented Aug 9, 2024 • edited Loading

LiorLieberman commented Jul 17, 2024 •

edited

Loading

arkodg Jul 17, 2024 •

edited

Loading

youngnick commented Jul 29, 2024 •

edited

Loading

LiorLieberman commented Aug 9, 2024 •

edited

Loading