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

Remove all references to settings.BASE_PATH #189

Merged
merged 12 commits into from
Mar 26, 2021

Conversation

jathanism
Copy link
Contributor

@jathanism jathanism commented Mar 24, 2021

Fixes: #147

This now relies on the HTTP CGI environment variable SCRIPT_NAME to be passed in upstream from the HTTP server (e.g. NGINX) paired with matching uWSGI configuration and that FORCE_SCRIPT_NAME be set in nautobot_config.py to match the configured path.

This also refactors the social auth URL to never rely upon BASE_PATH and instead replace hard-coding LOGIN_URL w/ a template condition on settings.SOCIAL_AUTH_ENABLED. Further, this means that the social auth URL can naturally inherit the derived base path sent from the HTTP server without having to hard-code it.

The default values of STATIC_URL and MEDIA_URL are now relative URL paths to allow for them to inherit the derived base path. Under normal circumstances this is equivalent to them being absolute.

In the event that a HTTP server sets the base path with the HTTP environment variable SCRIPT_NAME, (e.g. /nautobot) then ALL URLs automatically get prefixed (e.g. /nautobot/static/). If a user requires custom static/media paths, they may still customize these values for their setup.

Other updates:

  • Split core API URLs into nautobot.core.api.urls and include those URLs in nautobot.core.urls to reduce the amount of circular dependencies going on in nautobot.core
  • Corrected various references to hard-coded URLs and instead let them reverse properly
  • Default values for LOGIN_URL and LOGIN_REDIRECT_URL are now using URL route names and not hard-coded paths
  • Update uWSGI settings to include optional setting for subdir hosting
  • Update NGINX settings to explicitly use uWSGI (uwsgi_param) config directives, and to include optional setting for subdir hosting
  • Revise uWSGI to use socket (uWSGI protocol) vs. http-socket when behind NGINX and uwsgi_param to configure NGINX
  • Generic view for nautobot.core.views.HomeView updated to TemplateView to help in template debugging.
  • Added template context processors from social_django so that various auth backend variables can be used in templates.
  • Added social-auth-app-django as a hard dependency to simplify dynamism in the SSO login URL.
  • Update production configs to remind use of FORCE_SCRIPT_NAME to match that of HTTP config
  • Update config init template to include FORCE_SCRIPT_NAME
  • Added a basic unit test for FORCE_SCRIPT_NAME
  • Removes contrib/ directory once and for all and all references to contrib/ (except in upgrading doc, which will come later)

nautobot/core/admin.py Show resolved Hide resolved
@@ -156,6 +152,9 @@ EXEMPT_VIEW_PERMISSIONS = [
# 'ipam.prefix',
]

# If hosting Nautobot in a subdirectory, you must set this value to match the base URL prefix configured in your HTTP server (e.g. `/nautobot`). When not set, URLs will default to being prefixed by `/`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still learning how this works. If the HTTP server is setting SCRIPT_NAME correctly, then FORCE_SCRIPT_NAME is not required, correct? This would only be needed if SCRIPT_NAME is incorrect?

Copy link
Contributor Author

@jathanism jathanism Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MOST of the application works correctly without deliberately setting FORCE_SCRIPT_NAME, but there are a few edge cases mostly related to not being able to leverage django.urls.reverse() consistently due to circular imports caused by having to end up loading nautobot.core.urls where nearly every single model in the project is loaded to facilitate nautobot.core.views.HomeView and SearchView.

So for now, to allow for places where APISelect widgets are being used, namely the Device.position dynamic lookup, this is the only way to assert that we can reliably make the app receive the correct SCRIPT_NAME. If we can refactor enough to be able to use reverse() everywhere we need to specify URLs, we can stop hard-coding API URLs in the code and can simplify deployment even further.

nautobot/core/tests/test_views.py Outdated Show resolved Hide resolved
nautobot/docs/configuration/optional-settings.md Outdated Show resolved Hide resolved
@jathanism jathanism added this to Review in progress in Release v1.0.0 Mar 24, 2021
@jathanism jathanism added this to the v1.0.0 milestone Mar 24, 2021
contrib/nginx.conf Outdated Show resolved Hide resolved
contrib/uwsgi.ini Outdated Show resolved Hide resolved
nautobot/core/tests/test_views.py Show resolved Hide resolved
@jathanism
Copy link
Contributor Author

Fixes: #166

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's also a reference to contrib in nautobot/docs/installation/upgrading.md that probably needs to be updated as well.


If the location of SSL certificates had to be changed in `Obtain an SSL Certificate` step, then the location will need to be changed in the `apache.conf` file as well.
```apache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you will need to add apache to the hljs_languages: list in mkdocs.yml for this to get highlighted appropriately, though I'd love to be wrong on this point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be great to add nginx highlighting to the NGINX config earlier in this file, and enable it in the mkdocs.yml as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it and it didn't seem to do anything. 🤷

@glennmatthews glennmatthews mentioned this pull request Mar 26, 2021
@jathanism
Copy link
Contributor Author

Looks like there's also a reference to contrib in nautobot/docs/installation/upgrading.md that probably needs to be updated as well.

Yeah I wasn't touching that doc just yet. It's still on the todo list and I figured I'd just do that then.

This now relies on the HTTP CGI variable `SCRIPT_NAME` to be passed in upstream from the HTTP server (e.g. NGINX) paired with matching uWSGI configuration.

This also refactors social auth URL to never rely upon `BASE_PATH` and instead replace hard-coding `LOGIN_URL` w/ a template condition on `settings.SOCIAL_AUTH_ENABLED`. Further, this means that the social auth URL can naturally inherit the derived base path sent from the HTTP server without having to hard-code it.

- Correct references to hard-coded URLs and instead let them reverse properly
- Update uwsgi settings to include optional setting for subdir hosting

- The default values of `STATIC_URL` and `MEDIA_URL` are now relative URL paths to allow for them to inherit the derived base path. Under normal circumstatnces this is equivalent to them being absolute.
- In the event that a HTTP server sets the base path with `SCRIPT_NAME`, (e.g. `/nautobot`) then these automatically get prefixed (e.g. `/nautobot/static/`). If a user requires custom static/media paths, they may still customize these values for their setup.
- Generic view for `nautobot.core.views.HomeView` updated to `TemplateView` to help in template debugging.
- Make extras forms call api_url w/ a route name vs. URL path
This also splits core API URLs into `nautobot.core.api.urls` and `include` those URLs in `nautobot.core.urls` to reduce the amount of circular dependencies going on in `nautobot.core`
- Update production configs to remind use of `FORCE_SCRIPT_NAME` to match that of HTTP config
- Update config init template to include `FORCE_SCRIPT_NAME`
- Fix URL join in `widgets.py`
- Added a basic unit test for `FORCE_SCRIPT_NAME`
Update docs to reference subdir prefix using trailing slash for consistency, since this is what Django emits it as.
- Fold `apache.conf` into install docs for http-server
- Update `FORCE_SCRIPT_NAME` test to use `try...finally`
@jathanism
Copy link
Contributor Author

Just rebased against upstream.

Release v1.0.0 automation moved this from Review in progress to Reviewer approved Mar 26, 2021
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jathanism jathanism merged commit 7540deb into nautobot:develop Mar 26, 2021
Release v1.0.0 automation moved this from Reviewer approved to Done Mar 26, 2021
@jathanism jathanism deleted the jathanism-replace-base_path branch March 26, 2021 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Login failed when BASE_PATH is set
2 participants