Fixes unexpected 404 error

[toadjaune]

This modification fixes a bug I just encountered with a very basic configuration of this role.

Here are the properties of my system :

• Role version : v0.4.1
• Nginx version : nginx/1.12.0
• Ubuntu 16.04

Here are my variables :

nginx_sites:
  api-staging:
    domains: "{{ domains }}"
    default_server: True
    upstreams:
      - name: 'api'
        servers: ['unix:///home/some/path.sock']

Which generates me, as expected :

  location / {
    try_files $uri $uri.html $uri/ @api =404;
  }

  location @api {
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-Proto $scheme;
    proxy_set_header Host $http_host;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_redirect off;
    proxy_pass http://api;
  }

However, calling a working endpoint gives me a 404 Error.

After a few tries, I found two ways of modifying this configuration to fix the error :

  location / {
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-Proto $scheme;
    proxy_set_header Host $http_host;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_redirect off;
    proxy_pass http://api;
  }

and

  location / {
    try_files $uri $uri.html $uri/ @api;
  }

  location @api {
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-Proto $scheme;
    proxy_set_header Host $http_host;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_redirect off;
    proxy_pass http://api;
  }

This pull request implements the second solution.

After some research, i found in the documentation that "The last parameter can also point to a named location, as shown in examples below. Starting from version 0.7.51, the last parameter can also be a code".

I'm no Nginx expert, but from what I understand, only the last parameter can be a named parameter, so this modification seems necessary.

I've tried to imagine what regressions this change could introduce, but so far can't see any :

• Users using a custom_root_location_try_files are not affected
• Users with no upstream are not affected
• The last parameter of a try_files MUST be defined, but in our case, it is necessarily so, because of the structure of the template, so this shouldn't be an issue
• in case of unavailability of the upstream, we get a 502 error. I'm not sure how the current configuration is supposed to behave in such a case, but the 502 error sounds like what we want in such a situation.

Does this sound like a sensible change ?

[nickjj]

Hi,

Thanks for the PR, and you are correct. I reviewed the docs and yep, you can't have both an upstream and code defined like that. The code approach works ok for static sites tho (no upstream), and in the case of an upstream, it will just use the error_pages generated by this role.

Have you tested your code change?

The current implementation is:

... $uri/{{ (' @' + item.upstreams[0].name) if (item.upstreams) else '' }} =404;

Your patch is:

... $uri/{{ (' @' + item.upstreams[0].name) if (item.upstreams) else '=404' }};

I'm a little concerned that your patch might result in $uri/=404 without a space. I didn't test it, so I might be wrong. I'm not sure offhand if Jinja will add a space, but I'm thinking no.

Perhaps your patch should be adjusted to:

try_files $uri $uri.html $uri/ {{ ('@' + item.upstreams[0].name) if (item.upstreams) else '=404' }};

This way there's always a space in both conditions of the if statement.

Also, on your last point, yeah a 502 is normal. That's what happens when your upstream can't be reached by nginx (502 bad gateway).

[toadjaune]

You're right, I had not tested this new version without upstream, but I guess it wouldn't work.

Anyway, i made the modification that you suggested, and tested both with and without upstream, everything seems to work fine (either when the static file is present, or when it is not).

[nickjj]

Thanks. I'll test and merge it tonight.