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

Update nextcloud's instructions #684

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

Update nextcloud's instructions #684

wants to merge 1 commit into from

Conversation

quietsy
Copy link
Member

@quietsy quietsy commented Jul 6, 2024

We're getting a lot of support questions regarding trusted_proxies and gethostbyname, changing the instructions to be more fool-proof

@quietsy quietsy requested a review from a team July 6, 2024 14:52
@thespad
Copy link
Member

thespad commented Jul 7, 2024

Need to be careful as you're making assumptions about 172.16.0.0/12. Docker can also allocate /20s from 192.168.0.0/16 and, in the case of overlay networks, /24s from 10.0.0.0/8.

Technically it can allocate any sized subnet from those ranges, but those are the defaults and you'd hope if people have the wherewithal to change them they also know how to configure this without handholding.

@quietsy
Copy link
Member Author

quietsy commented Jul 7, 2024

Need to be careful as you're making assumptions about 172.16.0.0/12. Docker can also allocate /20s from 192.168.0.0/16 and, in the case of overlay networks, /24s from 10.0.0.0/8.

Technically it can allocate any sized subnet from those ranges, but those are the defaults and you'd hope if people have the wherewithal to change them they also know how to configure this without handholding.

It will work in most cases and specifically in the common compose created network, and avoid the issues with gethostbyname. (which probably doesn't work anymore, definitely not for me)

@nemchik
Copy link
Member

nemchik commented Jul 7, 2024

I searched through discord briefly and the only real issues I found with gethostbyname were that at some point NC (or a plugin) replaced it in the config.php with an actual IP address for SWAG at the time (which could later change). I can confirm this actually happened to me. The rest of what I saw regarding trusted_domains was mostly users who didn't have gethostbyname involved, or didn't have their NC and their proxy container (swag or otherwise) on the same docker network.

I had put gethostbyname in my config a few months ago and found today it was replaced with 172.16.0.8 at some point. I think it makes the most sense to try our best to explain what's actually going on in the comments in the file so users can decide what value they want to put in their config.php, potentially from multiple known working options. I would consider gethostbyname a known working option with a caveat (that it will likely get replaced by a static IP and not work long term). Aside from that, using the CIDR for the docker network if the user has their containers on the same docker network, or using the CIDR for their LAN as a last resort.

@quietsy
Copy link
Member Author

quietsy commented Jul 8, 2024

I don't see a need to over-complicate it beyond: gethostbyname is known to break, using the CIDR doesn't break.

if the user has their containers on the same docker network

It shouldn't be an if, that is our recommended configuration with swag and what we officially support.

@nemchik
Copy link
Member

nemchik commented Jul 9, 2024

I don't see a need to over-complicate it beyond: gethostbyname is known to break, using the CIDR doesn't break.

if the user has their containers on the same docker network

It shouldn't be an if, that is our recommended configuration with swag and what we officially support.

I can agree with both of those statements. Would it be worth writing a PHP script that detects the user's CIDR from within the container and displays it as a recommended value in the init log? Potentially even as an occ command the user can just copy/paste to set the correct value?

P.s. I don't think setting it automatically is the right idea, but displaying a recommendation for settings we support based on detected values in the user environment makes sense.

Also could be done in bash, but I figure if PHP can see the value it's more likely what nextcloud would actually see (depending on their implementation I guess).

@LinuxServer-CI
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. This might be due to missing feedback from OP. It will be closed if no further activity occurs. Thank you for your contributions.

@spider1163

This comment was marked as off-topic.

@thespad thespad added awaiting-approval Stale exempt and removed no-pr-activity labels Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-approval Stale exempt
Projects
Status: PRs Ready For Team Review
Development

Successfully merging this pull request may close these issues.

None yet

5 participants