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

Move default nginx root outside of first location #1338

Merged
merged 1 commit into from
Sep 22, 2021

Conversation

nbauernfeind
Copy link
Member

According to https://www.nginx.com/resources/wiki/start/topics/tutorials/config_pitfalls/#root-inside-location-block

It is bad practice to declare the primary root inside of each location of an nginx.conf.

Note that this block does not specify a location, but should share the same location as the / location.

    location ~* \.chunk.(?:css|js)$ {
        expires 1y;
        add_header Cache-Control 'public, immutable, max-age=31536000';
    }

Took me a hot minute to find out why I couldn't fetch static resources from a non-default location.

@nbauernfeind nbauernfeind added this to the Sept 2021 milestone Sep 22, 2021
@nbauernfeind nbauernfeind self-assigned this Sep 22, 2021
@devinrsmith
Copy link
Member

LGTM, but I'll defer to @mofojed

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

What error are you getting right now? What's the use case for serving up static files outside of our specified locations?

@nbauernfeind
Copy link
Member Author

The error is a 404.

I've written up a blog post on how to deploy our setup without docker. It still relies on nginx (specifically because of the dependency on webdav). In my post I cluster all deephaven related items into a single folder. jsapi and the ide end up in /deephaven/html. The deephaven-core jars end up in /deephaven/lib. The logging configuration in /deephaven/resources. Storage for notebooks and layouts in /deephaven/storage. Envoy and SSL configuration ends up in /deephaven/config. I felt that this was more clear to a reader; whether or not they choose to set it up like this is up to them.

In particular the relevant piece of configuration is this:

    location / {
        root   /usr/share/nginx/html;
        index  index.html index.htm;

        # Caching is taken care of by etags
        # Settings expires -1 also sets the 'no-cache' Cache-Control header (http://nginx.org/en/docs/http/ngx_http_headers_module.html)
        expires -1;
        add_header Cache-Control 'must-revalidate, max-age=0';
    }

    # .chunk.* files are safe to cache, as they contain a sha in their file name
    location ~* \.chunk.(?:css|js)$ {
        expires 1y;
        add_header Cache-Control 'public, immutable, max-age=31536000';
    }

Notice how the second location block does not specify a root. It is root less. On Ubuntu this default is /usr/share/nginx/html -- which is why this works inside of the docker environment. But, IMO is inconsistent with how the first location is configured. There are three valid options forward: 1) drop the root in the first location block (as it is being set to the default), 2) specify the root in both locations, or 3) set a default root for the entire server block.

Nginx recommends the third option.

If you read the linked "config pitfalls" article -- specifically the section I've linked to, it actually says that it's a common mistake to add the root config option to each location. Instead it recommends doing as I've done in the PR; creating a default root and only overwriting if necessary (Note: it is overwritten for notebooks and layouts).

@nbauernfeind nbauernfeind merged commit 4bbe4bb into deephaven:main Sep 22, 2021
@nbauernfeind nbauernfeind deleted the nginx_root branch September 22, 2021 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build DocumentationNeeded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants