log_dir works in vhost context, but not in main context

[ubellavance]

Hi,

I defined nginx::config::conf_dir and it works perfectly for vhosts (it puts access log in $log_dir/vhostname.access.log), but if we also want to use a different path for the base nginx logs, we have to explicitely define them, even when nginx::config::conf_dir is defined.

nginx::config::log_dir: '/var/opt/rh/rh-nginx18/log/nginx'
nginx::http_access_log: '/var/opt/rh/rh-nginx18/log/nginx/access.log'
nginx::nginx_error_log: '/var/opt/rh/rh-nginx18/log/nginx/error.log'

[wyardley]

You mean log_dir, not conf_dir, right?

I see what you're saying tho. it's getting pulled directly from params.pp, so config.pp override isn't being looked at. This is a good one, I'll work on a PR to fix this.

https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/params.pp#L98-L100
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/config.pp#L26-L28

[wyardley]

I'm proposing one fix for this, but would like some input from others about whether there's a better way to accomplish this. It works in my tests if you'd like to test it out as well.

[ubellavance]

Yes, you got it right. It is log_dir. Thanks a lot for your work on that one. With your change, will it still be possible to override it? I think I should be able to test your proposed code next week.

Also, could you demystify something for me: I don't know if that's the case when setting the configs in the classes, but when using Hiera, some parameters are nginx::paramX and some others are nging::config::paramY. Is there a reason why?

Example from my config:

Directly under nginx::

nginx::server_tokens: 'off'

Needs a "config" before:

nginx::config::client_body_temp_path: '/var/opt/rh/rh-nginx18/lib/nginx/tmp/client_body'

[wyardley]

As far as the history on which configs are in nginx::config, I think the idea was to reduce the clutter in params, as well as to separate the parameters configuring the module itself (i.e., where nginx is installed from, say), vs. the parameters that relate to configuring nginx itself.

You don't have to use hiera, but you may need to use the weird pattern used in doc/hiera.md if you want to alter default configs.

There was also at one point an attempt to move to 'puppet-module-data' pattern; this text from an old version of doc/hiera.md explains it somewhat:
9bd63d3?short_path=160a765#diff-160a76552604be96a1065027a35cebd8

#501 also has some background.

In any event, the module is now maintained by Voxpupuli, which maintains it on a volunteer basis. I don't directly work with them, but have been trying to help make some incremental improvements to the module lately. @bastelfreak and I did discuss trying to clean up some of the cruft again, and simplify the namespaces a bit. Pull requests are always welcome. You're also welcome to join the discussion in #voxpupuli on the Puppet Community Slack or on IRC (chat.freenode.net).

[ubellavance]

I understand the history a bit, but I don't really understand how to figure out when to use nginx::config::param and when to use nginx::param except trial and error. To me, nginx::server_tokens is an nginx config, not a module parameter (like manage_repo for example). I looked at the code but couldn't really figure it out.

[wyardley]

Yeah, you pretty much have to look at the code, but because of the history, the code is a bit mixed up too, and sometimes it's hard to figure out. It's a known issue that, while the vhost and location resources have parameters documented at the top of the manifests, the README, init.pp and config.pp don't include docs for all the top level config params, which is obviously not a good situation.

#451 is an old PR that's by now way out of date, that was attempting to address some of this. I do hope we can make some improvements in this area. There's been some discussion of exactly how (whether puppet-strings is ready and whether we'll be using it or not), but so far, I'm leaning towards just fixing the docs by hand for now, which is do-able, but will be quite labor intensive.

[wyardley]

btw, patch for my fix. It's also been merged into master, so you could try just cloning current master into your modules directory, which will get you some of the other fixes mentioned as well.

https://patch-diff.githubusercontent.com/raw/voxpupuli/puppet-nginx/pull/904.diff

[ubellavance]

Thanks,

I have 2 questions:

• Even at looking at the code, I'm still using trial and errors to determine if I add a config:: or not. How can I find, by looking a the code, if a parameter is nginx:: or nginx::cofig::?
• How can I tell what changes have been included in master since the "official" version?

PS: what timezone are you in? (Eastern for me)

[wyardley]

Yeah, like I said, the module is in kind of a convoluted state at this point. As I understand it, the stuff listed in config.pp in these lines:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/config.pp#L17-L117
should be in the nginx::config namespace. If you're setting any of those parameters in nginx::foo instead of nginx::config::foo, that may also be why you're seeing an issue in the other ticket. That's even though many of those variables are also declared in init.pp's parameters section:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/init.pp#L29-L103
because they're then deprecated here:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/init.pp#L133-L217

But like I was saying, basically, anything that's not about managing the package or service was moved to the nginx::config namespace. Make sense?

I'm in Pacific timezone.

[wyardley]

And you can see the commit history here:
https://github.com/voxpupuli/puppet-nginx/commits/master

You can see release history here:
https://github.com/voxpupuli/puppet-nginx/releases

[ubellavance]

Thanks for the links to releases. I guess that a release is what is pushed to the forge?

Regarding the nginx::config namespace, I understand what you are saying but it doesn't seem to be consistent with the code, unless I've got something wrong in my config or there have been many changes in master since the release.

Here's what I have in the nginx::config namespace:

nginx::config::http_cfg_append:
nginx::config::conf_dir: '/etc/opt/rh/rh-nginx18/nginx'
nginx::config::client_body_temp_path: '/var/opt/rh/rh-nginx18/lib/nginx/tmp/client_body'
nginx::config::proxy_temp_path: '/var/opt/rh/rh-nginx18/lib/nginx/tmp/proxy'
nginx::config::vhost_purge: true
nginx::config::confd_purge: true
nginx::config::log_dir: '/var/opt/rh/rh-nginx18/log/nginx'
nginx::config::gzip_types: 'text/plain text/css application/json application/x-javascript text/xml application/xml application/xml+rss text/javascript'

```Here's what I have in the nginx:: namespace:

nginx::worker_processes: 'auto'
nginx::manage_repo: false
nginx::package_name: 'rh-nginx18-nginx'
nginx::pid: '/var/opt/rh/rh-nginx18/run/nginx/nginx.pid'
nginx::http_access_log: '/var/opt/rh/rh-nginx18/log/nginx/access.log'
nginx::nginx_error_log: '/var/opt/rh/rh-nginx18/log/nginx/error.log'
nginx::server_tokens: 'off'
nginx::log_format:
nginx::proxy_connect_timeout: '5400'
nginx::proxy_read_timeout: '5400'
nginx::proxy_send_timeout: '5400'
nginx::proxy_hide_header:
nginx::proxy_set_header:
nginx::nginx_vhosts:
nginx::nginx_locations:

Honestly, I may not know puppet or nginx enough, but I can't easily make the difference between what needs to be in `nginx::config`

Thanks,

[wyardley]

#904 is merged.

[ubellavance]

FYI:

I tried to move all my configs, but 2 can't (running 0.5.0 by the way):

If I remove the '::config' from these two, the value it's like if I had put nothing:

nginx::config::gzip_types:
nginx::config::log_dir

Thanks,