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

Add logic for defining port in redirect based on scheme #801

Merged

Conversation

ciarams87
Copy link
Member

@ciarams87 ciarams87 commented Jun 28, 2023

Proposed changes

Problem: The port in the location header for an HTTPRoute with a requestRedirect filter must be set based on the rules in the spec

We're missing the case where scheme is set to a well-known (http or https) value, but no port is specified. In these cases, we should set the port in the redirect location to 80 for scheme==http, and 443 for scheme==https.

Additionally, when the port is the well known port associated with the scheme, we should not actually include the port in the redirect location. This means that we should ONLY include the port in the redirect url if the scheme is unset, or if the port is specified in the filter to something other than 80 or 443.

Solution: Remove the port in redirect location if it corresponds to the specified scheme.

Testing: Added unit-tests, confirmed the HTTPRouteRedirectScheme and HTTPRouteRedirectPortAndScheme conformance tests are now passing

Closes #795

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@ciarams87 ciarams87 requested a review from a team as a code owner June 28, 2023 14:17
@github-actions github-actions bot added the bug Something isn't working label Jun 28, 2023
internal/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/nginx/config/servers_test.go Outdated Show resolved Hide resolved
@ciarams87 ciarams87 force-pushed the fix/incorrect-port-scheme-redirect branch from 2f4f375 to 2739a2e Compare June 29, 2023 09:38
internal/nginx/config/servers_test.go Outdated Show resolved Hide resolved
internal/nginx/config/servers_test.go Outdated Show resolved Hide resolved
internal/nginx/config/servers_test.go Show resolved Hide resolved
internal/nginx/config/servers.go Outdated Show resolved Hide resolved
@ciarams87 ciarams87 requested a review from pleshakov June 29, 2023 18:07
@ciarams87 ciarams87 force-pushed the fix/incorrect-port-scheme-redirect branch from b0769c3 to 8fe223a Compare June 30, 2023 09:55
@ciarams87 ciarams87 force-pushed the fix/incorrect-port-scheme-redirect branch from 8fe223a to bd644e0 Compare July 3, 2023 08:43
internal/nginx/config/servers.go Outdated Show resolved Hide resolved
@ciarams87 ciarams87 force-pushed the fix/incorrect-port-scheme-redirect branch from bd644e0 to a4e9fed Compare July 6, 2023 10:32
@ciarams87 ciarams87 merged commit 51b9ff4 into nginxinc:main Jul 6, 2023
16 checks passed
@ciarams87 ciarams87 deleted the fix/incorrect-port-scheme-redirect branch July 6, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Incorrect port in the location header for redirects
4 participants