add support for passenger on CentOS/RHEL

[wyardley]

This contains elements of @jlambert121's #786, but addresses @3flex's feedback from back in June, as well as improving test coverage slightly. It's also based against current upstream, so shouldn't need a rebase.

If preferred, I can submit on top of #786, but wasn't sure how to do that.

[ffrank]

Well, it would be nice if proper attribution to @jlambert121 could be maintained.

You could try amending 4b946c2 with --reset-author and change it to the OP. I feel that would be a fair compromise.

All other commits should probably be squashed.

[wyardley]

Ok. There were 2 minor changes of mine that I left in the first commit, but just changed the author to @jlambert121 and squashed my other commits into a single one, let me know if that looks better.

[wyardley]

Side note, as best I can tell, Phusion doesn't GPG sign the package data itself. For me, installing only works with the change I added to only check metadata signatures, so the package itself isn't getting its signature checked. This seems to be how they are distributing it at present, but not ideal, obviously.

[ffrank]

👍 LGTM

[wyardley]

One other thing, if the module runs first with the default$package_source, there could be some potential confusion, as the nginx-release repo will remain there and installed, which could cause problems, since the available versions aren't that different. I don't want to hard-code the version, and I don't think there's a way to tell the yum provider which source to pull a package from. Would it be too drastic (or potentially problematic for users) if I added:

      yumrepo { 'nginx-release':
          ensure => absent,

here:
https://github.com/voxpupuli/puppet-nginx/pull/876/files#diff-a20b01568fc70c912920cfd80dd3b114R68

[ffrank]

That does sound reasonable to me. If I understand correctly, the user needs to switch nginx implementations in order to run Passenger, so it makes sense to exclude upstream nginx.

I'd like a second review on this. Randomly pinging @bastelfreak, who can escalate that on IRC or in here if appropriate.

[wyardley]

Thanks, I can add this. We can also do the same in reverse for the passenger repo...

I guess I'm a little nervous about ensuring 'absent' on a resource that theoretically could have been created by something other than Puppet (i.e., if a person happened to make a repo named 'nginx-release' or 'passenger', or whatever, but still seems like the way to go.

That said, with the default $package_ensure value, if the package gets installed from the wrong source (or already exists on the system before puppet runs), the user may still have to intervene to get the right "puppet" installed. I added some notes in the quickstart.

I also standardized on present / absent without quotes in package/redhat.pp - the module currently uses a mix.

I'm rarely on IRC, but I am on Puppet community slack in #voxpupuli as @wby.

[wyardley]

I squashed the most recent commits too, but we can back out if you want to take out the ensure => absent changes.

[ffrank]

It's cool, let's just wait for others to chime in here.

Wrt. absent vs. 'absent', it's funny that I just had this very discussion. I'm in favor of removing quotes 👍

[wyardley]

Sounds good.

I've been trying to work on some acceptance tests for this too, but it seems like they're not heavily used and haven't been worked on in a while. I have something that's more or less working, but need to work on it a bit more before pushing.

[wyardley]

ps - On CentOS 6, the acceptance test I was working on seemed to show a problem with the GeoIP package being missing... thinking we may still need to depend on EPEL repo being present in this case as a soft dependency, but will try to test more.

[wyardley]

Leaving my acceptance test changes as a separate commit (not squashed), just to make it easier to pull them out if needed... @3flex: sorry to bug you again, but if you've got time, can you tell me if the way I'm doing the acceptance test looks reasonable (and whether the passenger stuff should be broken out more into its own file or a separate run of the test)?

5d659ad

I tried avoiding using as many shell commands and weird exceptions (in favor of things like host.uninstall_package('foo'), but couldn't get it to work.

I did some weird git stuff to rebase against master; from a quick look, it looks like the commits are finally in a good place again.

[3flex]

@bastelfreak did you confirm the acceptance tests were working? I was looking into that since @wyardley has asked for feedback.

[wyardley]

@3flex: Thanks for taking a look. I verified they were working on centos6 and ubuntu 14.

I just realized the newest version failed on centos7, which was working before (centos-7-x64 specifically). It seems as if cent7 may also depend on epel for the rubygem-rack package (in which case the test needs to be fixed, but the docs also should be updated). I'm also getting another weird failure which seems to be resolved if I do a stop / start rather than 'restart' of nginx.

[root@centos-7-x64 sites-enabled]# service nginx restart
Redirecting to /bin/systemctl restart  nginx.service
Job for nginx.service failed. See 'systemctl status nginx.service' and 'journalctl -xn' for details.
[root@centos-7-x64 sites-enabled]# systemctl status nginx -l
nginx.service - nginx - high performance web server
   Loaded: loaded (/usr/lib/systemd/system/nginx.service; enabled)
   Active: failed (Result: exit-code) since Mon 2016-09-26 23:26:56 UTC; 2s ago
     Docs: http://nginx.org/en/docs/
  Process: 8400 ExecStop=/bin/kill -s QUIT $MAINPID (code=exited, status=0/SUCCESS)
  Process: 8196 ExecStart=/usr/sbin/nginx -c /etc/nginx/nginx.conf (code=exited, status=0/SUCCESS)
  Process: 9130 ExecStartPre=/usr/sbin/nginx -t -c /etc/nginx/nginx.conf (code=exited, status=1/FAILURE)
 Main PID: 8198 (code=exited, status=0/SUCCESS)

Sep 26 23:26:56 centos-7-x64 systemd[1]: Starting nginx - high performance web server...
Sep 26 23:26:56 centos-7-x64 nginx[9130]: nginx: [emerg] BIO_new_file("/tmp/blah.cert") failed (SSL: error:02001002:system library:fopen:No such file or directory:fopen('/tmp/blah.cert','r') error:2006D080:BIO routines:BIO_new_file:no such file)
Sep 26 23:26:56 centos-7-x64 nginx[9130]: nginx: configuration file /etc/nginx/nginx.conf test failed
Sep 26 23:26:56 centos-7-x64 systemd[1]: nginx.service: control process exited, code=exited status=1
Sep 26 23:26:56 centos-7-x64 systemd[1]: Failed to start nginx - high performance web server.
Sep 26 23:26:56 centos-7-x64 systemd[1]: Unit nginx.service entered failed state.
[root@centos-7-x64 sites-enabled]# nginx -t -c /etc/nginx/nginx.conf
nginx: the configuration file /etc/nginx/nginx.conf syntax is ok
nginx: configuration file /etc/nginx/nginx.conf test is successful
[root@centos-7-x64 sites-enabled]# echo $?
0

www.puppetlabs.com.conf has:

  ssl_certificate           '/tmp/blah.cert';
  ssl_certificate_key       '/tmp/blah.key';

(the files were actually there; a stop / start on the service did resolve the error).

I couldn't get the el5 ones to run on my machine at all for some reason.

Since this PR is merged already, I am guessing it would be easier to make a new one for some additional minor fixes?

One other thought is that the 'curl' calls (in the vhost acceptance tests) that are relying on exit code should probably use the -f (--fail) option.

One other interesting side note: It's possible to install passenger-enabled nginx of about the same version via EPEL rather than from the Phusion repo, however, the current solution is probably the one that works in the most similar way on the most platforms. Since there is no easy way to make sure the 'nginx' package comes from a particular place, I could see some issues for users with EPEL repo configured (probably fairly common) coming up if the EPEL version somehow got higher than the ones from the Nginx / Phusion repos; hopefully this is unlikely to happen?

[3flex]

I have a fix for the SSL file issue in #885. It's due to SELinux configuration which probably changed in Red Hat 7.

Regarding EPEL, perhaps we should include https://forge.puppet.com/stahnma/epel?

[wyardley]

perhaps we should include https://forge.puppet.com/stahnma/epel?

Yeah, I'm on the fence about this. This can cause problems if people are already including EPEL anway, and could also be an issue in terms of causing unpredictable behavior or installing stuff people don't want, since the repo has to be setup with enabled=1 to work properly.

I think the puppetlabs-rabbitmq module deals with this with an extra $epel_enable variable, which lets you do an include of an module named '::epel':
https://github.com/garethr/garethr-erlang/blob/master/manifests/init.pp#L29

[wyardley]

Continuing discussion in #886