Skip to content
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

multiple fixes to HTTPRequestRedirect filter #863

Merged
merged 1 commit into from
Sep 15, 2021
Merged

multiple fixes to HTTPRequestRedirect filter #863

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
14 changes: 7 additions & 7 deletions apis/v1alpha2/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ type HTTPRouteFilter struct {
// Support: Core
//
// +optional
RequestRedirect *HTTPRequestRedirect `json:"requestRedirect,omitempty"`
RequestRedirect *HTTPRequestRedirectFilter `json:"requestRedirect,omitempty"`

// ExtensionRef is an optional, implementation-specific extension to the
// "filter" behavior. For example, resource "myroutefilter" in group
Expand Down Expand Up @@ -646,17 +646,17 @@ type HTTPRequestHeaderFilter struct {
Remove []string `json:"remove,omitempty"`
}

// HTTPRequestRedirect defines configuration for the RequestRedirect filter.
type HTTPRequestRedirect struct {
// Protocol is the protocol to be used in the value of the `Location`
// HTTPRequestRedirectFilter defines configuration for the RequestRedirect filter.
type HTTPRequestRedirectFilter struct {
// Scheme is the scheme to be used in the value of the `Location`
// header in the response.
// When empty, the protocol of the request is used.
// When empty, the scheme of the request is used.
//
// Support: Extended
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can remove the support levels here. I can't find/remember where this was documented, but I thought we agreed that support level was essentially a min of field and parent support level. This was particularly important for filters since they could be defined per Route Rule or per Backend. In this case, we'd be removing an "Extended" support annotation and deferring to the "Core" support of redirects in general which does not seem accurate here. This comment applies throughout, but just adding it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow up, this PR recovers the docs that got lost around this: #864.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected some discussion on this front.

I'm a bit confused here so making sure I understand you right.

support level was essentially a min of field and parent support level

I think we agree with this statement. I removed the Extended support level because the filter is marked as "Core". Marking the filter as "Extended" is another option to adhere to this rule.

Or are you saying that we should keep the filter as "Core" and have some fields marked as "Extended" (leave as is)?
The concern I've with this approach is that as a user, I might get surprised when I'm using a Core filter with a conformant implementation but something doesn't work and the reason is a field-level documentation that I didn't peruse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I agree this could get confusing, I think the alternatives are worse. The way I see it we have two options:

  1. Features can be marked core but may include some extended capabilities/fields that are not required to use the feature
  2. Features must be marked as extended if they include at least one field that is not supported by every implementation.

The current PR actually is more of a third option - it removes the "Extended" annotation on several fields, which may lead users to believe the field had "Core" support. Unfortunately there's at least one implementation that can't support the field, so that seems inaccurate.

Of the options I listed above, I strongly prefer 1 because otherwise I think we'd end up avoiding new fields or with a lot of "Extended" features.

Copy link
Contributor Author

@hbagdi hbagdi Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd end up avoiding new fields or with a lot of "Extended" features.

An alternative is to have a filter "RequestRedirectExtended" (with a better name) in addition to the core filter. That lays out the contract much more explicitly and makes the end-user experience free of confusion.
We can't take this approach with more involved features within the guts of the API, but with filters, we can.

Having said that, I've reverted the changes related to conformance levels to minimize the scope of the PR.

//
// +optional
// +kubebuilder:validation:Enum=HTTP;HTTPS
Protocol *string `json:"protocol,omitempty"`
// +kubebuilder:validation:Enum=http;https
Scheme *string `json:"scheme,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the protocol was "HTTP", I guess the scheme should be "http"?


// Hostname is the hostname to be used in the value of the `Location`
// header in the response.
Expand Down
14 changes: 7 additions & 7 deletions apis/v1alpha2/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.

28 changes: 14 additions & 14 deletions config/crd/v1alpha2/gateway.networking.k8s.io_httproutes.yaml
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion examples/v1alpha2/http-redirect.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ spec:
- filters:
- type: RequestRedirect
requestRedirect:
protocol: HTTPS
scheme: https
---
apiVersion: gateway.networking.k8s.io/v1alpha2
kind: HTTPRoute
Expand Down