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

Pass through the full request_uri to these proxied services #60

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

PeterJCLaw
Copy link
Member

Summary

It turns out that when proxy_pass is given a value with variables in then it always proxies to exactly that value, without any of the normal appending of the rest of the path or query. This means we need to explicitly pass the request uri through in such cases.

Exactly what this means for processing of the content returned from the proxied host is somewhat unclear, however testing of the code-submitter suggests at least that its redirects work as desired.

Code review

Testing

  • applied the configuration locally
  • manually validated the new behaviour

Links

https://discord.com/channels/1154145712943153252/1154145715518459950/1207444719655849994

Note: given the urgency here and that this is currently broken I've already applied this fix in prod.

It turns out that when proxy_pass is given a value with variables
in then it always proxies to exactly that value, without any of
the normal appending of the rest of the path or query. This means
we need to explicitly pass the request uri through in such cases.

Exactly what this means for processing of the content returned
from the proxied host is somewhat unclear, however testing of the
code-submitter suggests at least that its redirects work as desired.
@PeterJCLaw PeterJCLaw merged commit a18a613 into main Feb 15, 2024
1 check passed
@PeterJCLaw PeterJCLaw deleted the fix-sub-path-proxying branch February 15, 2024 22:12
@PeterJCLaw
Copy link
Member Author

Merging to reflect that this is deployed (given the urgency & impact on teams of it being broken), however post-merge review would still be welcome.

Copy link
Member

@RealOrangeOne RealOrangeOne left a comment

Choose a reason for hiding this comment

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

✔️ This change looks ok to me, although it falls squarely in the "How did this ever work" category

@PeterJCLaw
Copy link
Member Author

Prior to #51 we had literals for our proxy_pass arguments. In such cases there are different rules for what's passed to the proxy.
My understanding (though may be in correct) is that given:

location /foo/ {
    proxy_pass http://internal/bar/
}

then a request to http://proxy/foo/something would be transformed into a request for http://internal/bar/something. However with the introduction of variables in #51 we lots that automatic mapping.

One minor gotcha we do now have is that the public-facing root name now must match the underlying one, or we'd need to manually adjust the prefix of the $request_uri to match. That does seem to be something which is possible, though keeping them the same probably simplifies things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants