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

set nginx host with custom port #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nicokant
Copy link

@nicokant nicokant commented Feb 1, 2024

Nginx configuration uses $host variable that doesn't provide custom ports, causing errors with request.build_absolute_url. This PR aims to fix providing also non-standard port specified as HTTP_PORT/HTTPS_PORT.

Related issues:

@t-book
Copy link
Contributor

t-book commented May 20, 2024

@giohappy good from my side.

@ridoo
Copy link

ridoo commented May 22, 2024

This issue seems to be wide spread and there are different solutions:

  • Add a new variable (as proposed here by PUBLIC_HOST)
  • Use $http_host instead of $host (#11734)

Both would work, but I tend to pick the $http_host solution as it does not introduce yet another URL variable in GeoNode settings. @ricardogsilva created a PR to close #11734 linked by @nicokant . I tested successfully to use $http_host already as I had to use a non-default HTTPS port in one of those blueprint setups.

@ricardogsilva
Copy link
Member

ricardogsilva commented May 22, 2024

I'd also prefer the changes proposed in GeoNode/geonode#11736, (regardless of being the author of that PR 😜) as they seem adequate for fixing the issue and are just a small modification of a single line in a config file.

WRT this PR, I would rather prefer the number of configuration variables for GeoNode to decrease, so adding yet another one is not the way, in my opinion.

@giohappy
Copy link
Contributor

I haven't the opportunity to test the $http_host approach now, but I also prefee it over additional variables.

If a custom host is needed it generally means that we are forwarding from anothe gateway / reverse proxy. In that case I guess the expected result is to have the oroginal http header forwarded automatically as is, without having to configure it.

The only concern is that $http_host assumes different values depending on whether the host header is set or not., In that case it uses the host in the request line, or the server_name as a fallback.
Maybe these are the cases where forcing its configuration might be required?

@ridoo
Copy link

ridoo commented May 23, 2024

If a custom host is needed it generally means that we are forwarding from anothe gateway / reverse proxy. In that case I guess the expected result is to have the oroginal http header forwarded automatically as is, without having to configure it.

@giohappy maybe this could be done by just setting proxy_set_header Host $http_host. Did not test it, though.

The only concern is that $http_host assumes different values depending on whether the host header is set or not., In that case it uses the host in the request line, or the server_name as a fallback.
Maybe these are the cases where forcing its configuration might be required?

MDN documentation on Host request header says this:

A Host header field must be sent in all HTTP/1.1 request messages. A 400 (Bad Request) status code may be sent to any HTTP/1.1 request message that lacks or contains more than one Host header field.

Do you know cases other than HTTP 1.0 where Host header can be absent or empty? Do we want to support HTTP 1.0 requests (in cases where non-default HTTPS port is configured)? Then we would have to introduce another variable. IMO, however, that is be a very special case, for which I still would vote for $http_host.

@giohappy
Copy link
Contributor

@giohappy maybe this could be done by just setting proxy_set_header Host $http_host. Did not test it, though.

My comment was to agree with the proposal from @ricardogsilva, but it needs testing.

Ok for the required host header. Nginx is designed to cover the edge cases, but I think we can assume the more usual case.

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.

5 participants