-
Notifications
You must be signed in to change notification settings - Fork 70
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
handle http headers when behind a well configured proxy #70
Conversation
Fixes Unidata/thredds#1310 This should work with the following Nginx proxy config: ``` location /thredds/ { proxy_pass http://thredds:8080/thredds/; proxy_set_header Host $host; # pass the original public hostname to Thredds proxy_set_header X-Forwarded-Proto $scheme; # pass the original httpS proto to Thredds proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; # pass the original client IP to Thredds } ```
Thanks for this PR. Unfortunately, I don't think this really belongs here. To borrow from OO philosophy, the Law of Demeter or principle of least knowledge applies here. Nginx reverse proxy concerns for THREDDS are not germane to this project. If you really need to make customizations that is what docker volume mounts are for. Something like docker run -it --rm -p 8888:8080 -v /home/joe-user/server.xml:/usr/local/tomcat/conf/server.xml unidata/thredds-docker:latest with your customizations in Alternatively, you can build your own container that inherits from As an aside, I've done this reverse proxy type setup before. I recall it not being totally easy to get right. You can find what I did at this now defunct project. It uses a technology called traefik in conjunction with nginx. |
Totally agreed with the philosophy. I probably should have explained my changes better, my bad. The changes is not specific to Nginx, it was tested with Nginx because that is what I have. The change is to enable Tomcat to handle the 2 http headers "X-Forwarded-Proto" and "X-forwarded-For" out of the box. Out of the box means much better onboarding experience for new users deploying Thredds in their organization. Being able to simply handle http headers opens up Thredds to behave nicely behind multiple different proxies, not just Apache with mod_jk or mod_proxy as documented here https://www.unidata.ucar.edu/software/tds/current/reference/TomcatBehindProxyServer.html. This docs feels like only Apache is supported as front proxy to Thredds, which is not true. If the 2 http headers are not set by the front proxy, then it's just like now where there is no handling for them. In fact, setting these 2 headers is not default behavior for Nginx, I had to set them in the Nginx config. Those 2 http headers together with the "Host" header (this "Host" header already works out of the box with Thredds) should still be documented in how to configure front proxy for Thredds. It's a best practice to set those http headers for front proxies and when best practices are followed, Thredds docker image will "just work".
Yes, this is another avenue but generally, imho for a good user experience, things should just work.
No experience with Another reason for "any proxies" solution is that Thredds might not be directly behind the proxy serving as SSL termination. In large scale deployment, it might possibly be this set up instead: customer client/browser -> Apache/Nginx for SSL termination -> HAProxy for load balancing -> some customer built custom proxy for authentication and access control -> then finally Thredds. There might be even more proxies in the middle, now that containers and micro-services are popular. So this PR will improve the user experience when users are using the Thredds docker image and their proxy is properly configured. If user install and configure Thredds by hand, this But any bits to improve generic out of the box user experience is good. So I would like to request reconsideration to accept my PR. |
Your comments presume In addition, where does the What I would suggest as your next step is to make a PR to https://github.com/docker-library/tomcat/pulls since every argument you have made thus far would also apply to |
Fixes Unidata/thredds#1310
This should work with the following Nginx proxy config: