Skip to content

Conversation

@nicolasfranck
Copy link
Contributor

Socket.gethostname does not always return the correct hostname as used on the internet,
especially if the public name is just an alias.

@briri
Copy link
Contributor

briri commented Nov 19, 2020

We need to do some testing on this one to see what impact it has on servers behind a load balancer. Our concern is that request.host may not have the hostname for the LB.

@nicolasfranck
Copy link
Contributor Author

Shouldn't the load balancer add headers like X-Forwarded-Host to make the rails happy?
That's the way I do it. Socket.gethostname would in that case generate urls that cannot
be resolved by the browser..

@briri
Copy link
Contributor

briri commented Nov 19, 2020

Our LB setup and admin is performed by another team so I will need to check in with them and experiment to see what we're getting for this new approach. @raycarrick-ed or @xsrust do you want to try it on your end when you have a chance to see what impact it would have for your instances?

@xsrust xsrust requested a review from briri November 23, 2020 16:12
@xsrust
Copy link
Contributor

xsrust commented Nov 23, 2020

The request.host version is what we're using in our Multi-tenant customization to check the URL the user's coming in from so this shouldn't cause problems from our end.

Just adding @briri as an explicit reviewer so we can ensure you've been able to check in with your infrastructure team before we merge.

@briri
Copy link
Contributor

briri commented Nov 23, 2020

we override that particular file, and I would imagine most installations do as well, so I think any risk is limited.
With that said, I updated my branded version of that file and the suggested change worked without issue.

@briri briri merged commit ff2729a into DMPRoadmap:development Nov 23, 2020
portagenetwork pushed a commit to portagenetwork/roadmap that referenced this pull request Feb 24, 2022
use request.host instead of relying on possibly faulty/internal hostname
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.

3 participants