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.

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

Redirects add port to Location header #3589

Closed
benediktwerner opened this issue Jun 11, 2024 · 2 comments · Fixed by #3628
Closed

Redirects add port to Location header #3589

benediktwerner opened this issue Jun 11, 2024 · 2 comments · Fixed by #3628
Assignees
Labels
area/conformance Gateway API Conformance Related Issues kind/bug Something isn't working
Milestone

Comments

@benediktwerner
Copy link

benediktwerner commented Jun 11, 2024

Description

When setting up any redirect, e.g. following the initial example at https://gateway.envoyproxy.io/v1.0.1/tasks/traffic/http-redirect/ (though removing the backendRefs which aren't actually allowed together with a redirect), Envoy Gateway will add an explicit port (in this case 443) to the location header:

$ curl -L -v --header "Host: redirect.example" "http://localhost:8888/get"
...
< HTTP/1.1 301 Moved Permanently
< location: https://www.example.com:443/get

(The example curl output in the docs actually doesn't show the port but that's not the behavior I observe.)

This is counter to the recommendation given by the Gateway API which on the ports field of the redirect filter (https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRequestRedirectFilter) states:

Implementations SHOULD NOT add the port number in the ‘Location’ header in the following cases:

  • A Location header that will use HTTP (whether that is determined via the Listener protocol or the Scheme field) and use port 80.
  • A Location header that will use HTTPS (whether that is determined via the Listener protocol or the Scheme field) and use port 443.

This happens even if the port field isn't actually set in the redirect and even if it's only a path redirect, with no change to the hostname, scheme, or port.

Using egctl, it looks like the following Envoy configuration is generated for the route for the example from the docs:

              redirect:
                portRedirect: 443
                schemeRedirect: https

Presumably, the portRedirect shouldn't be set in this case.

This probably doesn't matter in most cases since browsers apparently silently remove the port when following the redirect but some http clients don't which for us then lead to an issue in combination with OIDC because the URL with the port wasn't registered as a valid callback URL.

Environment

  • Envoy Gateway v1.0.1
  • Envoy v1.29.3
@benediktwerner benediktwerner changed the title Scheme redirect adds port to Location header Redirects add port to Location header Jun 11, 2024
arkodg added a commit to arkodg/gateway that referenced this issue Jun 11, 2024
* dont use `port: 443` in the redirect example
* dont specify a backendRefs when the filter is a redirect filter

Fixes: envoyproxy#3589

Signed-off-by: Arko Dasgupta <[email protected]>
arkodg added a commit to arkodg/gateway that referenced this issue Jun 11, 2024
* dont use `port: 443` in the redirect example
* dont specify a backendRefs when the filter is a redirect filter

Fixes: envoyproxy#3589

Signed-off-by: Arko Dasgupta <[email protected]>
arkodg added a commit to arkodg/gateway that referenced this issue Jun 11, 2024
* dont use `port: 443` in the redirect example
* dont specify a backendRefs when the filter is a redirect filter

Fixes: envoyproxy#3589

Signed-off-by: Arko Dasgupta <[email protected]>
arkodg added a commit to arkodg/gateway that referenced this issue Jun 11, 2024
* dont use `port: 443` in the redirect example
* dont specify a backendRefs when the filter is a redirect filter

Fixes: envoyproxy#3589

Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg
Copy link
Contributor

arkodg commented Jun 11, 2024

digging into this, looks like the change was made #1601 to make upstream conformance tests pass kubernetes-sigs/gateway-api#1880
@shawnh2 is there a way to make sure the conformance tests pass and we dont append the port in the Location header

@shawnh2 shawnh2 self-assigned this Jun 14, 2024
@shawnh2 shawnh2 added kind/bug Something isn't working kind/question Further information is requested area/conformance Gateway API Conformance Related Issues and removed triage labels Jun 14, 2024
@shawnh2
Copy link
Contributor

shawnh2 commented Jun 19, 2024

Thanks @benediktwerner for spotting this!

The implementation of upstream GW-API redirPort were missing in EG:

Implementations SHOULD NOT add the port number in the ‘Location’ header in the following cases:

A Location header that will use HTTP (whether that is determined via the Listener protocol or the Scheme field) and use port 80.
A Location header that will use HTTPS (whether that is determined via the Listener protocol or the Scheme field) and use port 443.

Yes @arkodg, can be fixed in the xds translator:

if redirection.Port != nil {

By adding extra checks, update redirPort only if it is not 80 nor 443.

I've done the test, the well-known port number is not shown in ‘Location’ header anymore.

@shawnh2 shawnh2 removed the kind/question Further information is requested label Jun 19, 2024
@arkodg arkodg added this to the v1.1.0 milestone Jun 21, 2024
arkodg added a commit to arkodg/gateway that referenced this issue Jun 25, 2024
* dont use `port: 443` in the redirect example
* dont specify a backendRefs when the filter is a redirect filter

Fixes: envoyproxy#3589

Signed-off-by: Arko Dasgupta <[email protected]>
arkodg added a commit that referenced this issue Jun 25, 2024
docs: update redirect tasks

* dont use `port: 443` in the redirect example
* dont specify a backendRefs when the filter is a redirect filter

Fixes: #3589

Signed-off-by: Arko Dasgupta <[email protected]>
bjlhlin pushed a commit to bjlhlin/gateway that referenced this issue Jun 26, 2024
docs: update redirect tasks

* dont use `port: 443` in the redirect example
* dont specify a backendRefs when the filter is a redirect filter

Fixes: envoyproxy#3589

Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: bjlhlin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance Gateway API Conformance Related Issues kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants