Skip to content

change SupportedFeatures API to a list of subobjects #3200

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 31, 2024
Merged

change SupportedFeatures API to a list of subobjects #3200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions apis/applyconfiguration/apis/v1/gatewayclassstatus.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 43 additions & 0 deletions apis/applyconfiguration/apis/v1/supportedfeature.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion apis/applyconfiguration/internal/internal.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions apis/applyconfiguration/utils.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 9 additions & 4 deletions apis/v1/gatewayclass_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,10 @@ type GatewayClassStatus struct {
Conditions []metav1.Condition `json:"conditions,omitempty"`

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Contributor

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.

// SupportedFeatures is the set of features the GatewayClass support.
// It MUST be sorted in ascending alphabetical order.
// It MUST be sorted in ascending alphabetical order by the Name key.
// +optional
// +listType=set
// +listType=map
// +listMapKey=name
// <gateway:experimental>
// +kubebuilder:validation:MaxItems=64
SupportedFeatures []SupportedFeature `json:"supportedFeatures,omitempty"`
Expand All @@ -278,6 +279,10 @@ type GatewayClassList struct {
Items []GatewayClass `json:"items"`
}

// SupportedFeature is used to describe distinct features that are covered by
// FeatureName is used to describe distinct features that are covered by
// conformance tests.
type SupportedFeature string
type FeatureName string

type SupportedFeature struct {
Name FeatureName `json:"name"`
}
Copy link
Contributor

@arkodg arkodg Jul 17, 2024 •
edited
Loading

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)

Copy link
Member

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.

Copy link
Contributor

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 ?

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

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.

15 changes: 15 additions & 0 deletions apis/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 26 additions & 12 deletions config/crd/experimental/gateway.networking.k8s.io_gatewayclasses.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 29 additions & 6 deletions pkg/generated/openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.