-
Notifications
You must be signed in to change notification settings - Fork 468
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
change SupportedFeatures API to a list of subobjects #3200
change SupportedFeatures API to a list of subobjects #3200
Conversation
|
||
type SupportedFeature struct { | ||
Name FeatureName `json:"name"` | ||
} |
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.
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)
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 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.
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.
that rationale makes sense @robscott, a follow question is, are there any implementations that would like to implement this API in this current form ?
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.
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.
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.
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.
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 don't think user documentation belongs in CRD Status at all. IMO this should be removed from the API and documented in implementations websites.
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 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.
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 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.
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 +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.
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.
+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.
@@ -261,9 +261,10 @@ type GatewayClassStatus struct { | |||
Conditions []metav1.Condition `json:"conditions,omitempty"` | |||
|
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.
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.
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 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?
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.
just to learn more, what do you mean old setting? the supportedFeatures API in its current form?
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 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
This field provides no value but causes a lot of harm: * kubernetes-sigs/gateway-api#3200 (comment) * istio#50851
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. |
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 |
* 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
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>
* 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>
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) |
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 ok with moving this change forward as is, thanks @LiorLieberman!
/lgtm
|
||
type SupportedFeature struct { | ||
Name FeatureName `json:"name"` | ||
} |
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.
+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.
[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:
Approvers can indicate their approval by writing |
Unhold this after as agreed in the community meeting yesterday. |
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 |
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?: