RFC: Upstream vs module-shipped configurations

[3flex]

Related PRs: #777, #855, #499, #500, #682.

This module currently ships two configuration files that override upstream defaults, fastcgi_params and uwsgi_params. There are a couple of differences:

Module fastcgi_params differs from upstream fastcgi_params as follows:

- fastcgi_param  REQUEST_SCHEME     $scheme;
- fastcgi_param  HTTPS              $https if_not_empty;
+ fastcgi_param  HTTPS              $https;
+ fastcgi_param  SCRIPT_FILENAME    $request_filename;
+ # Mitigate httpoxy, see https://httpoxy.org/#fix-now
+ fastcgi_param  HTTP_PROXY          "";

Module uwsgi_params differs from upstream:

- uwsgi_param  REQUEST_SCHEME     $scheme;
- uwsgi_param  HTTPS              $https if_not_empty;
+ uwsgi_param  SERVER_ADDR        $server_addr;

The "modern" fastcgi.conf changes SCRIPT_FILENAME declaration to fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name; line as well which this module doesn't ship with.

As a matter of policy, should this module take upstream's config as is, or should it provide its own? The problem is upstream's config includes new-ish parameters like if_not_empty (added in 1.1.11) which if added to the module may cause configuration errors on machines where older versions are being run.

This may (will?) be a breaking change for some users.

[bastelfreak]

My idea: A new release that announces the breaking change in the next major release. Then a new major release that ships the upstream configuration. We should be able to implement a warning somehow if the person uses an older nginx version? There is already a fact that returns the nginx version which we could use here.

[3flex]

I'm not sure what the behaviour is for brand new installs - is the fact available on the initial Puppet run? My understanding is Facter runs prior to Puppet, so on a first run the fact wouldn't be available and the warning wouldn't trigger. Could be wrong.

But yes, a release announcing the upcoming breaking change would be welcome. There are other breaking changes that could be made (e.g. #863) at the same time.

[igalic]

facter runs before puppet, but if your new module rolls out a new fact that you rely on, you have a bootstrapping issue

[wyardley]

I kind of like the idea of having the nginx module itself not force its version of things like fastcgi_params. I think it would be better to take any defaults, but still allow the user to override the include file name and / or set / override parameters within locations.

Lack of being able to customize that file or use a different template is also kind of a problem; recently, I worked around the need to add things to the generic fastcgi params (rather than per location or vhost) in certain cases by using concat to add a fragment to the default file and write it to a new file, then switched the include to that file.

[wyardley]

I guess one middle path would be to have an option for whether or not to manage those?

But assuming vendors' packages ship configs that are in a predictable location, maybe we could code those locations into params (if necessary), and include those files, with an option for the user to set a custom include path?

I'm thinking we should target the next major release for this, since we already have one release with a bunch of changes coming up pretty shortly, and still not much discussion about this one?

[zachfi]

Distro shipped configurations are increasingly irrelevant, and we are replacing much of the core files and patterns (conf.d vs sites-enabled, etc) that are shipped by the distros already. I've not paid close enough attention to know if the repos that we add are for the nginx repos, but for a while it seemed like if you wanted a recent nginx, you used repos outside of the distro from https://nginx.org/en/linux_packages.html etc, but this approach might vary considerably between Linuxes. Assuming where folks get the package from is hard, since some changes require recompilation of nginx.

Personally, the patterns and approaches that this module lays out are the ones that I will use. As a Debian veteran, I found it strange that other systems don't use sites-enabled, but now that I run CentOS professionally (and BSD personally), it doesn't matter even a little.

Lets aim to not weaken security or stability. Lets provide clear CHANGELOGs for updates to defaults, and allow folks who need to modify those defaults we ship the ability to do so. People can always use older versions of the module if they need to, but ideally the good choices we make here will be ones that users want to upgrade the module's version for.

[wyardley]

@xaque208: I don't doubt that you're right about the vendor shipped configs being increasingly irrelevant. My concern is mostly the scope of the module, and I'd argue that it may be better for the module to err on the side of 'do no harm'; we don't know which exact versions or directives the user's implementation of fastcgi / uwsgi supports, and we don't know whether or not the user has customized those files or is managing them with something else (i.e., pushing them with site-specific templates from Puppet or whatever). I don't have an objection to having an option to push configs based on a preset template without customizability, and of course there's always the option to reference a different include file, which can be done already. But I do think there's a case to be made for not overwriting files that are arguably outside the scope of this module, especially when there's no way for the end-user to modify the pushed files, only to include a different one.

What about the idea of keeping the existing files, adding some of the proposed additions, but add a $manage_{fastcgi,uwsgi}_params directive for each, or something similar? Or a way for a user to reference a custom template? Usually I'm more in favor of getting rid of knobs and whistles than adding them, but maybe this is a case where it's the right approach?

I do have a feature to have the module use conf.d vs. the sites-XYZ (funny, I also primarily used Debian early in my career, though I think it pre-dated that convention). I don't really object to the sites-X, but I'd argue that declarative config management makes being able to enable / disable sites somewhat less necessary / useful, and I do think there are some benefits to following a particular vendor's convention, though I'll admit it comes at the cost of adding some spaghetti code.

As far as RHEL / CentOS go, at least, the official or semi-official packages have tended to lag, but EPEL got a bump to 1.10.x recently, so it's more or less in line with the version distributed by mainline nginx repo as well as the one distributed by passenger.

[zachfi]

@wyardley Perhaps your right. My comment was mostly about the fact that we replace the nginx.conf by default in this module, so what use is the default configuration shipped by the distribution? This is, I suppose, new ground considering that we don't manage the CGI params files today. If we replace the CGI files as is proposed here, then again, the distro shipped configuration is less important. Where the binary for nginx comes from is all that we'll be getting from the distro. Is there something else I'm overlooking? In many ways, I think the use of this module circumvents the distributions for how to lay out configurations, and as a user of this module, we forfeit the decision process of our packagers for which options to include on a sane default. That puts the responsibility of configuration of, as you mention, 'do no harm' on us, the community.

I agree that the sites-enabled method of enabling vhosts is outdated and made more sense before config management was as in-production as today. That pattern came from Debian apache before nginx was around too, so we're carrying forward some old patterns by using it in this module.

The proposal you've mentioned @wyardley about adding $manage_{fastcgi,uwsgi}_params, would you then also have a ${fastcgi,uwsgi}_template for which template to use or just allow users to disable management, and ship a file themselves? Something along those lines seems reasonable to me.

[wyardley]

@xaque208: I think we exactly do manage the uwsgi_params and fastcgi_params files in the existing module, which is part of the reason for this discussion.

https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/vhost.pp#L662-L676
They're only managed if $fastcgi or $uwsgi are set, but those parameters also control whether those things are enabled in the config.

[jyaworski]

We should be overriding configs. Without it, we can't be sure of the system state.

[wyardley]

@jyaworski I respectfully disagree, by default, we should be trying to do no harm to things outside of the module's scope (nginx). We can't possibly anticipate all versions and settings of fastcgi / uwsgi params that might be expected, and the existing config doesn't allow users not to override those files or to customize them (with a custom template, say).

I think it's best to either not manage them, or add a parameter (off by default) that pushes a default that we think is sane. I put this one (and some other issues) on the agenda for the community-modules meeting Thurs, so hopefully we can get some consensus (or at least discuss some more) then.

[jyaworski]

Ah, nevermind. fastcgi is outside of nginx's scope. In that case, I vote that we use a @wyardley's suggestion above with parameters. To ensure them, we should use augeas. Outside of that, maybe require a fastcgi module?

[wyardley]

@jyaworski: Actually, after the community modules meeting (and about 3 seconds of investigation), I realized that I'm totally in the wrong here. Most nginx packages ship fastcgi_params and uwsgi_params, so it's in scope.

@hunner had some recommendations, which were: ship a default that fulfills the 80/20 rule, he also suggested that the config could be built somehow, to make it more easy to support things that are different depending on platform / package source. Tho since you can set params per location, as well as specify an alternate params file, I'd argue that there's some ability for users to customize this already, just not in an ideal way.

In the short term, in light of all this, it seems reasonable (to me) to merge in the parameters that are shipped in newer versions of nginx into the fastcgi_params / uwsgi_params files in the module, but that longer term, we should look into some way to make them more customizable. I think there are some approaches for this that could be looked into if someone is interested in trying to implement it.

[zachfi]

@wyardley +1. Folks can always use an older version of the module, and if we allow an override, then custom settings can still be applied as required.

[3flex]

So coming back to the original reason I opened this, sounds like consensus is that the module ships its own config, and since we want something recent, it will be the config that nginx itself uses for fastcgi and uwsgi? i.e. this (fastcgi.conf, newer than fastcgi_params) and this (uwsgi_params)?

And if anyone adds support for SCGI, that would be from https://trac.nginx.org/nginx/browser/nginx/conf/scgi_params

IMHO this default config would be the least surprising for new users while also ensuring the config is stable and managed by the module, not the user's distro.

[wyardley]

It turns out that
Optional[String] $foo = $bar doesn't do what I thought it does, so I need to revert that part of this config (where certain params were made optional, but with defaults).