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

Clarify semantics when port_redirect is unset #1740

Merged
merged 5 commits into from
Mar 10, 2023
Merged

Clarify semantics when port_redirect is unset #1740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bf771a
Clarify semantics when port_redirect is unset
arkodg Feb 17, 2023
f5cbbb8
also clarify empty host_redirect case
arkodg Feb 17, 2023
5297de1
update based on GAPI community meeting guidance
arkodg Feb 28, 2023
89250ac
gen crds
arkodg Feb 28, 2023
4d51132
incorporate Nick's suggestion
arkodg Mar 3, 2023
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
8 changes: 6 additions & 2 deletions apis/v1beta1/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,7 +882,8 @@ type HTTPRequestRedirectFilter struct {

// Hostname is the hostname to be used in the value of the `Location`
// header in the response.
// When empty, the hostname of the request is used.
// When empty, the hostname in the `Host` header
// of the request is used.
//
// Support: Core
//
Expand All @@ -901,7 +902,10 @@ type HTTPRequestRedirectFilter struct {

// Port is the port to be used in the value of the `Location`
// header in the response.
// When empty, port (if specified) of the request is used.
// When empty, the Gateway Listener port is used.
// In all cases, when the protocol is http and the port is port 80,
// then the port must be omitted. Similarly, when the protocol is
// https and the port is port 443, then the port must also be omitted.
Comment on lines +905 to +908
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing to me, when we say the "port must be omitted" who are we talking to? Is this a direction for users or implementers?

cc @arkodg @shaneutt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I change must to will so it caters to both parties ?

Copy link
Member

@robscott robscott Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think what we're trying to say is the following:

If the port is not specified in the Redirect Filter, implementations should use the port from 
the listener for redirects _unless_ the Listener or Redirect Filter protocol is HTTP or HTTPS, 
in which case it should not specify a port in the redirect configuration.

I'm not actually sure that's what we're trying to say, and what I wrote above is still rather confusing, but maybe it's at least clear that the guidance is intended for implementers?

Copy link
Contributor Author

@arkodg arkodg Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

suggest reading through this thread #1740 (comment) and sharing your feedback, we are saying ports 80 and 443 should never be appended to the Location header

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 that overall idea makes sense, it's just not clear to me in the current variation of the spec, moving this to #1806 so we don't lose track of the discussion.

//
// Support: Extended
//
Expand Down
46 changes: 32 additions & 14 deletions config/crd/experimental/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.

46 changes: 32 additions & 14 deletions config/crd/standard/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.