nginx::locations puts locations in wrong order #971

kwisatz · 2016-11-14T19:03:14Z

Puppet: 3.7.5
Ruby: 2.1.5
Distribution: debian
Module version: git-master

I realize this might not necessarily belong here, but it is specifically creating problems with nginx and I have the feeling that it actually changed with my upgrade from 0.5.0 to the git-master branch.
Also, I haven't found any information in the documentation on create_resources that would point to it being responsible for this behavior.

Our problem is, that nginx locations are put in the wrong order. With regex locations, order matters and in our case it puts a return 404 in front of the location it should actually match first, which makes my site 404 on all requests that should go to the main controller.

It does not seem to order by order of definition, nor alphabetically by the hash key/name. But I can't find the place where the locations are sorted.

wyardley · 2016-11-14T19:07:01Z

When did you clone master? There was a recent PR that just reworked the logic of locations.
CCing @xaque208

Can you provide an example of the expected vs. actual ordering?

FWIW, this is where most of the location logic happens:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/location.pp
https://github.com/voxpupuli/puppet-nginx/blob/master/templates/vhost/location.erb
https://github.com/voxpupuli/puppet-nginx/tree/master/templates/vhost/locations

called from
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/vhost.pp#L750

kwisatz · 2016-11-14T20:07:23Z

Cloned it today, a few hours back, approx. 5.

I'm not using

nginx::nginx_vhosts:
  locations:

but rather

nginx::nginx_locations:

So I think rather than https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/vhost.pp#L750 this line applies: https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/init.pp#L276?

The expected ordering would be as I have it in hiera:

  'dev':
    ensure: present
    vhost: "%{hiera('vhost')}"
    www_root: "/var/www/%{hiera('vhost')}/current/web"
    fastcgi: phpfcgi
    location: '~ ^/(app_dev|config)\.php(/|$)'
    location_cfg_append:
      fastcgi_split_path_info: '^(.+\.php)(.*)$'
      fastcgi_param SCRIPT_FILENAME: $realpath_root$fastcgi_script_name
      fastcgi_param DOCUMENT_ROOT: $realpath_root
  'prod':
    ensure: present
    vhost: "%{hiera('vhost')}"
    www_root: "/var/www/%{hiera('vhost')}/current/web"
    fastcgi: phpfcgi
    location: '~ ^/app\.php(/|$)'
    location_cfg_append:
      fastcgi_split_path_info: '^(.+\.php)(.*)$'
      fastcgi_param SCRIPT_FILENAME: $realpath_root$fastcgi_script_name
      fastcgi_param DOCUMENT_ROOT: $realpath_root
    internal: true
  '404':
    ensure: absent
    www_root: "/var/www/%{hiera('vhost')}/current/web"
    vhost: "%{hiera('vhost')}"
    location: '~ \.php'
    location_cfg_append:
      return: '404'

And when applying it, we can actually see that it is the 404 location, which previously (0.5.0) was at the bottom, is now moved above the other regex locations:

--- /etc/nginx/sites-available/vhost.conf   2016-11-14 16:22:56.813807481 +0100
+++ /tmp/puppet-file20161114-7901-nwepnc    2016-11-14 21:02:44.348400299 +0100
@@ -1,7 +1,7 @@
 # MANAGED BY PUPPET
 server {
   listen *:80;
-  server_name           vhost www.vhost;
+  server_name           vhost;

   index  index.html index.htm index.php;

@@ -15,14 +15,6 @@
     access_log /var/log/nginx/static.log;
   }

  location / {
     root      /var/www/vhost/current/web;
     index     index.html index.htm index.php;
@@ -60,9 +60,13 @@
     add_header Cache-Control "public";
   }

+  location ~ \.php {
+    root      /var/www/vhost/current/web;
+    index     index.html index.htm index.php;
+    return 404;
+  }

   location ~ ^/(app_dev|config)\.php(/|$) {
-    root          /var/www/vhost/current/web;
     include       /etc/nginx/fastcgi_params;

     fastcgi_pass  phpfcgi;
@@ -73,7 +77,6 @@

   location ~ ^/app\.php(/|$) {
     internal;
-    root          /var/www/vhost/current/web;
     include       /etc/nginx/fastcgi_params;

     fastcgi_pass  phpfcgi;
@@ -81,10 +84,4 @@
     fastcgi_param SCRIPT_FILENAME $realpath_root$fastcgi_script_name;
     fastcgi_split_path_info ^(.+\.php)(.*)$;
   }
-
-  location ~ \.php {
-    root      /var/www/vhost/current/web;
-    index     index.html index.htm index.php;
-    return 404;
-  }
 }

kwisatz · 2016-11-14T20:32:30Z

OK, I'd say case closed. I didn't realize there was a priority parameter. When setting this to e.g. 899 on the 404 location, it is left at the end, as expected.

kwisatz · 2016-11-14T21:27:57Z

To follow up, even though I marked it as closed:

The change doesn't seem to have happened in this module, but rather in the concat module.
I had two environments, of which one observed priority/order and the other did not. Then I deleted the concat module from the one that did and had to copy it over from the non-working environment because it wouldn't install anymore:

root@puppet:~# puppet module --modulepath '/etc/puppet/environments/cl1_dev/modules' install --ignore-dependencies puppetlabs/concat
Notice: Preparing to install into /etc/puppet/environments/cl1_dev/modules ...
Notice: Downloading from https://forgeapi.puppetlabs.com ...
Error: Could not install module 'puppetlabs-concat' (???)
  No version of 'puppetlabs-concat' can satisfy all dependencies
    Use `puppet module install --ignore-dependencies` to install only this module

And now both environments disregard the order.

The version of concat I had in one environment, installed without explicitly specifying a version, was 1.2.5. When I specify 2.2.0 explicitly, it only installs with a --force.

With a priority of 899 and concat 2.2.0, it actually puts that location outside the server definition:

server {
[…]
}

  location ~ \.php {
    root      /var/www/vhost/current/web;
    index     index.html index.htm index.php;
    return 404;
  }

With a priority of 599, it's fine:

server {
  […]
  location ~ \.php {
    root      /var/www/vhost/current/web;
    index     index.html index.htm index.php;
    return 404;
  }
}

Maybe someone would like to open this ticket back up ;)

wyardley · 2016-11-14T21:54:41Z

Interesting, thanks for checking into this, and glad it doesn't seem to be directly related to the recent changes, but if concat's behavior is now different, we may need to make some adjustments or add some more tests of the ordering.

I haven't used the priority feature much myself. I checked, and with a similar type of config (and a version of the module before the most recent changes), I do get some variance in the order compared to how I'm specifying things in hiera.

I do note that the docs suggest a range between 401 and 599 for location. It's possible that using a value outside of this range is what's causing your problems with the placement within the generated file, though the module, perhaps, should throw an error if the user does this. If you keep your priority values within the stated ranges, does that help?

#   [*priority*]              - Location priority. Default: 500. User priority
#     401-499, 501-599. If the priority is higher than the default priority,
#     the location will be defined after root, or before root.

wyardley · 2016-11-14T21:58:11Z

Hrm, it does validate it, however, it sets the range higher (899) than the docs suggest.

https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/location.pp#L361-L366

  validate_array($rewrite_rules)
  if (($priority + 0) < 401) or (($priority + 0) > 899) {
    fail('$priority must be in the range 401-899.')

If this is just a typo (rather than a feature), we could adjust the upper end of the range to 599.

kwisatz · 2016-11-14T21:59:17Z

If you keep your priority values within the stated ranges, does that help?

It does, as I mentioned in my edit. I had seen that mention of 599 as a maximum value, but then the code itself checks for values between 401 and 899, as you just mentioned here above, an I wanted to make sure my location block was as much at the bottom as possible, but not that much though. ;)

wyardley · 2016-11-14T22:07:56Z

@kwisatz: I created #972 for this tiny fix

People may not know how to submit this. I'm not sure if there's anything we can do to influence the sorting order. From a quick look seems like @foo gets put before /, so maybe there is some kind of quasi-alphanumeric sort, and it's just behaving a bit oddly? I'll try to do some testing if I get a chance, it might be good to do it unordered if possible (preserving the users' ordering), but I think if ordering is important, using priority probably is the right way to go.

kwisatz · 2016-11-14T22:11:05Z

Preserving the user's ordering would certainly be best I suppose. I tried alphanumeric, renaming my '404' location to 'z404' but that didn't change anything, so I don't think it's anything like that.

wyardley · 2016-11-14T22:27:41Z

It looks like ssl priority adds 300, which might be why it's 899, not 599?

https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/location.pp#L436
But since it's adding 300 already in the code, I think the validation step should still have 599 as the max?

zachfi · 2016-11-15T17:23:56Z

I was thinking that for this reason, there might be a case for $locations to be an array of hashes, rather than a hash. This way, users who aren't being clever (with exported resources or some such), could just list the locations in hiera and since arrays are ordered, have an expectation of order in the config, as seen here. I've not looked too close at the code to know what change would be required to support this, but conceptually, I think it makes sense. This would avoid having to name resources something special to get them in the proper order, etc.

wyardley · 2016-11-15T17:29:00Z

@xaque208 I think post Ruby 1.9 (the supported version), hashes are ordered anyway, no?
However, because concat sets each fragment to have a priority, and ordering is based on that, I think it may be concat's "tiebreaker" ordering that's coming up. I'm not an expert with concat, but I assume it falls back on something else for secondary sorting when multiple items have the same priority and 'order' is by priority.

zachfi · 2016-11-15T17:34:03Z

Oh @wyardley, I'd no idea about ordered hashes post 1.9. Neat. I'd have to look at the code more closely to know where we might apply a clean ordering.

kwisatz closed this as completed Nov 14, 2016

wyardley mentioned this issue Nov 14, 2016

fix validation range for location priority #972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nginx::locations puts locations in wrong order #971

nginx::locations puts locations in wrong order #971

kwisatz commented Nov 14, 2016

wyardley commented Nov 14, 2016 •

edited

Loading

kwisatz commented Nov 14, 2016 •

edited

Loading

kwisatz commented Nov 14, 2016

kwisatz commented Nov 14, 2016 •

edited

Loading

wyardley commented Nov 14, 2016

wyardley commented Nov 14, 2016

kwisatz commented Nov 14, 2016

wyardley commented Nov 14, 2016

kwisatz commented Nov 14, 2016

wyardley commented Nov 14, 2016

zachfi commented Nov 15, 2016

wyardley commented Nov 15, 2016

zachfi commented Nov 15, 2016

nginx::locations puts locations in wrong order #971

nginx::locations puts locations in wrong order #971

Comments

kwisatz commented Nov 14, 2016

wyardley commented Nov 14, 2016 • edited Loading

kwisatz commented Nov 14, 2016 • edited Loading

kwisatz commented Nov 14, 2016

kwisatz commented Nov 14, 2016 • edited Loading

wyardley commented Nov 14, 2016

wyardley commented Nov 14, 2016

kwisatz commented Nov 14, 2016

wyardley commented Nov 14, 2016

kwisatz commented Nov 14, 2016

wyardley commented Nov 14, 2016

zachfi commented Nov 15, 2016

wyardley commented Nov 15, 2016

zachfi commented Nov 15, 2016

wyardley commented Nov 14, 2016 •

edited

Loading

kwisatz commented Nov 14, 2016 •

edited

Loading

kwisatz commented Nov 14, 2016 •

edited

Loading