Remove unnecessary limitation for allowed hosts in development environments #2137
Conversation
| "docs.djangoproject.localhost", | ||
| "dashboard.djangoproject.localhost", | ||
| ] + SECRETS.get("allowed_hosts", []) | ||
| ALLOWED_HOSTS = ["*"] |
There was a problem hiding this comment.
Technically this could just be done in the docker settings because I think it's only related to running with docker due to the docker network.
There was a problem hiding this comment.
I agree with @marksweb , what is the purpose of changing this part?
There was a problem hiding this comment.
I couldn't find a case where we need to keep a whitelist of hosts for development purposes
The purpose is to remove a broken setting that creating extra complexity with no benefit; refactoring.
There was a problem hiding this comment.
I think it's only related to running with docker due to the docker network.
0.0.0.0 is not related to the Docker network.
There was a problem hiding this comment.
Have you run the site with this configuration, accessed all of the different domains listed in the file, and confirmed that they display the appropriate site?
If the proper host name is not getting through to Django, it seems like the problem needs to be fixed somewhere else.
There was a problem hiding this comment.
About foreman, not sure why you think it's wrong but accessing a local project over 0.0.0.0, 127.0.0.1 or localhost is common practice, which I see no issue in having mentioned in a doc. Having the mention of foreman, and the procfile in the repo, is something I'd like to discuss but that would be a separate refactoring discussion.
There was a problem hiding this comment.
As I said before, my recommendation is to update the README. This is not a workaround; these are the actual domains you need to use to access the development site, as listed in the README, the ALLOWED_HOSTS setting, the dev_sites fixture, and the django-hosts configuration (djangoproject/hosts.py). They're also the domains used by the test suite. It's the same for the non-Docker setup, and the ALLOWED_HOSTS settings helps you see that you're not using the development server as it was intended.
If you would like to propose a change to allow the main site to be access on localhost:8000 instead of or in addition to www.djangoproject.localhost:8000, the change deserves its own issue where the @django/django-website team and others who work on the site can discuss and decide how to move forward.
There was a problem hiding this comment.
Thanks for the clarification. So, we need developers to access the project locally via specific hostnames 👍🏻 Blocking undesired hosts via ALLOWED_HOSTS provides only a partial solution for that need and creates further confusion. It blocks access in particular scenarios, but it doesn't help to find the right entry points or provide any information that the developer is doing something undesired. I think a better solution could be to apply the changes in this PR, and then follow up with forwarding requests from common entry points, such as "0.0.0.0" and "127.0.0.1", to the desired hostnames. I'll create an issue to have a healthier discussion 🌻
There was a problem hiding this comment.
I think it's only related to running with docker due to the docker network.
0.0.0.0 is not related to the Docker network.
Yeah I don't think that's what I was getting at.
The zero IP allows you to bind all interfaces. It's an easy "allow all" type approach.
Looking back at the screenshots, it's not something you'd then use to connect from the browser.
There was a problem hiding this comment.
This is not a workaround; these are the actual domains you need to use to access the development site, as listed in the README, the ALLOWED_HOSTS setting, the dev_sites fixture, and the django-hosts configuration (djangoproject/hosts.py)
the dev_sites fixture
This can be updated/improved, but I don't have the energy left to start a discussion about another arbitrary choice.
django-hosts configuration (djangoproject/hosts.py)
This is not correct. Hosts config doesn't check for the domain:
host_patterns = [
host(r"www", settings.ROOT_URLCONF, name="www"),
host(r"docs", "djangoproject.urls.docs", name="docs"),
host(r"dashboard", "dashboard.urls", name="dashboard"),
]
So django-hosts doesn't care if you access it via docs.0.0.0.0, docs.localhost or docs.djangoproject.localhost
|
I updated the description and the title to further clarify this is a refactoring. |
ulgens
changed the title
|
@ulgens @tobiasmcnulty Is this draft PR still relevant given #2207 and other recent Docker-related changes? |
No, I think it can be closed. #2198 (merged) and the follow-on work in #2207 (ready for review) get the Docker setup working again for tests and local dev, respectively. Please try it out though. :) |
|
@adamzap only partially. I think I found a misunferstanding about what does this config do but I couldnt find the time to explain it yet. I'd be happier if we keep the PR open for now. |
If we can't finalize the discussion until the next meeting, let's give it a one last shot in the meeting and then I'm okay with closing it if that would be the consensus. |
|
There was strong opposition to this PR, arguing that there is a technical reason to have this limitation in place, but it appears to be more a matter of preference than a technical requirement. The fixture that has the hardcoded domain name can be updated, but I already spent more than 10x of the effort I initially allocated for this PR, and I'm not interested in keeping it up. |
My original goal was to resolve the issue with the Docker setup and
0.0.0.0.But after a quick investigation, I couldn't find a case where we need to keep a whitelist of hosts for development purposes. Because the setting wasn't providing any benefit but adding complexity, I used this as as a small refactoring opportunity. Please let me know if there is any case that we need to limit the allowed hosts in non-prod environments.
If we actually need this limitation, I'll ask for lists of allowed hosts for dev and docker envs. It seems the dev is missing
0.0.0.0, and docker doesn't have any of the entries the dev has - which makes me think both settings need to be revamped.