From 11dfa9462f1565a838848cfad2f1185296490d88 Mon Sep 17 00:00:00 2001 From: Alexander Fisher Date: Wed, 8 May 2019 12:28:11 +0100 Subject: [PATCH 1/2] Fix acceptance tests broken since nginx 1.16 --- spec/acceptance/nginx_server_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/acceptance/nginx_server_spec.rb b/spec/acceptance/nginx_server_spec.rb index c740192f2..4afaa5fc9 100644 --- a/spec/acceptance/nginx_server_spec.rb +++ b/spec/acceptance/nginx_server_spec.rb @@ -69,7 +69,8 @@ class { 'nginx': } describe file('/etc/nginx/sites-available/www.puppetlabs.com.conf') do it { is_expected.to be_file } - it { is_expected.to contain 'ssl on;' } + it { is_expected.not_to contain 'ssl on;' } # As of nginx 1.15 (1.16 stable), this will not be set. + it { is_expected.to contain 'listen *:443 ssl;' } end describe file('/etc/nginx/sites-enabled/www.puppetlabs.com.conf') do From 0ff826527ffb95f5a68662e8b88bc364d29228a5 Mon Sep 17 00:00:00 2001 From: Alexander Fisher Date: Wed, 8 May 2019 13:45:22 +0100 Subject: [PATCH 2/2] Replace `add_listen_directive` with `nginx_version` Nginx 1.16 has recently been released. Instead of updating the README to recommend users set the `add_listen_directive` parameter, this parameter has been replaced by an `nginx_version` parameter that defaults to the fact (if available). If the fact isn't available, it defaults to a very conservative `1.6.0` (the version that ships in Debian 8). Future enhancements could make the non-fact default be OS specific. --- README.md | 7 +++++ manifests/init.pp | 10 ++++++- manifests/params.pp | 6 ---- manifests/resource/server.pp | 2 -- spec/acceptance/nginx_mail_spec.rb | 29 +++++++++++++++++++ spec/defines/resource_mailhost_spec.rb | 37 +++++++++++++++++++++++- templates/mailhost/mailhost.erb | 2 +- templates/mailhost/mailhost_ssl.erb | 6 ++-- templates/server/server_ssl_settings.erb | 2 +- 9 files changed, 86 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 8394fc2b1..e7368006b 100644 --- a/README.md +++ b/README.md @@ -144,6 +144,13 @@ To create only a HTTPS server, set `ssl => true` and also set `listen_port` to t same value as `ssl_port`. Setting these to the same value disables the HTTP server. The resulting server will be listening on `ssl_port`. +### Idempotency with nginx 1.15.0 and later + +By default, this module might configure the deprecated `ssl on` directive. When +you next run puppet, this will be removed since the `nginx_version` fact will now +be available. To avoid this idempotency issue, you can manually set the base +class's `nginx_version` parameter. + ### Locations Locations require specific settings depending on whether they should be included diff --git a/manifests/init.pp b/manifests/init.pp index dc30d807b..1f8136c3d 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -24,6 +24,14 @@ # node default { # include nginx # } +# +# @param nginx_version +# The version of nginx installed (or being installed). +# Unfortunately, different versions of nginx may need configuring +# differently. The default is derived from the version of nginx +# already installed. If the fact is unavailable, it defaults to '1.6.0'. +# You may need to set this manually to get a working and idempotent +# configuration. class nginx ( ### START Nginx Configuration ### Variant[Stdlib::Absolutepath, Boolean] $client_body_temp_path = $nginx::params::client_body_temp_path, @@ -185,7 +193,7 @@ Hash $nginx_upstreams = {}, Nginx::UpstreamDefaults $nginx_upstreams_defaults = {}, Boolean $purge_passenger_repo = true, - Boolean $add_listen_directive = $nginx::params::add_listen_directive, + String[1] $nginx_version = pick(fact('nginx_version'), '1.6.0'), ### END Hiera Lookups ### ) inherits nginx::params { diff --git a/manifests/params.pp b/manifests/params.pp index 9670e4e33..11e3f2b7e 100644 --- a/manifests/params.pp +++ b/manifests/params.pp @@ -227,11 +227,5 @@ $sites_available_group = $_module_parameters['root_group'] $sites_available_mode = '0644' $super_user = true - if fact('nginx_version') { - # enable only for releases that are older than 1.15.0 - $add_listen_directive = versioncmp(fact('nginx_version'), '1.15.0') < 0 - } else { - $add_listen_directive = true - } ### END Referenced Variables } diff --git a/manifests/resource/server.pp b/manifests/resource/server.pp index 654480b8b..d8235c4a5 100644 --- a/manifests/resource/server.pp +++ b/manifests/resource/server.pp @@ -131,7 +131,6 @@ # [*error_pages*] - Hash: setup errors pages, hash key is the http code and hash value the page # [*locations*] - Hash of servers resources used by this server # [*locations_defaults*] - Hash of location default settings -# [*add_listen_directive*] - Boolean to determine if we should add 'ssl on;' to the vhost or not. defaults to true for nginx 1.14 and older, otherwise false # Actions: # # Requires: @@ -269,7 +268,6 @@ $error_pages = undef, Hash $locations = {}, Hash $locations_defaults = {}, - Boolean $add_listen_directive = $nginx::add_listen_directive, ) { if ! defined(Class['nginx']) { diff --git a/spec/acceptance/nginx_mail_spec.rb b/spec/acceptance/nginx_mail_spec.rb index e1325ce6a..93b2070c6 100644 --- a/spec/acceptance/nginx_mail_spec.rb +++ b/spec/acceptance/nginx_mail_spec.rb @@ -35,4 +35,33 @@ class { 'nginx': describe port(465) do it { is_expected.to be_listening } end + + context 'when configured for nginx 1.14' do + it 'runs successfully' do + pp = " + class { 'nginx': + mail => true, + nginx_version => '1.14.0', + } + nginx::resource::mailhost { 'domain1.example': + ensure => present, + auth_http => 'localhost/cgi-bin/auth', + protocol => 'smtp', + listen_port => 587, + ssl => true, + ssl_port => 465, + ssl_cert => '/etc/pki/tls/certs/blah.cert', + ssl_key => '/etc/pki/tls/private/blah.key', + xclient => 'off', + } + " + + apply_manifest(pp, catch_failures: true) + end + describe file('/etc/nginx/conf.mail.d/domain1.example.conf') do + it 'does\'t contain `ssl` on `listen` line' do + is_expected.to contain 'listen *:465;' + end + end + end end diff --git a/spec/defines/resource_mailhost_spec.rb b/spec/defines/resource_mailhost_spec.rb index e4bb307ac..17a42f336 100644 --- a/spec/defines/resource_mailhost_spec.rb +++ b/spec/defines/resource_mailhost_spec.rb @@ -485,7 +485,7 @@ title: 'should set the IPv4 SSL listen port', attr: 'ssl_port', value: 45, - match: ' listen *:45 ssl;' + match: ' listen *:45;' }, { title: 'should enable IPv6', @@ -600,6 +600,41 @@ end end end + context 'on nginx 1.16' do + let(:params) do + { + listen_port: 25, + ssl_port: 587, + ipv6_enable: true, + ssl: true, + ssl_protocols: 'default-protocols', + ssl_ciphers: 'default-ciphers', + ssl_cert: 'dummy.crt', + ssl_key: 'dummy.key' + } + end + + context 'when version comes from fact' do + let(:facts) do + facts.merge(nginx_version: '1.16.0') + end + + let(:pre_condition) { ['include ::nginx'] } + + it 'has `ssl` at end of listen directive' do + content = catalogue.resource('concat::fragment', "#{title}-ssl").send(:parameters)[:content] + expect(content).to include('listen *:587 ssl;') + end + end + context 'when version comes from parameter' do + let(:pre_condition) { ['class { "nginx": nginx_version => "1.16.0"}'] } + + it 'also has `ssl` at end of listen directive' do + content = catalogue.resource('concat::fragment', "#{title}-ssl").send(:parameters)[:content] + expect(content).to include('listen *:587 ssl;') + end + end + end end context 'attribute resources' do diff --git a/templates/mailhost/mailhost.erb b/templates/mailhost/mailhost.erb index 21e8c198f..468a64be4 100644 --- a/templates/mailhost/mailhost.erb +++ b/templates/mailhost/mailhost.erb @@ -38,7 +38,7 @@ server { <%- end -%> <%= scope.function_template(["nginx/mailhost/mailhost_common.erb"]) -%> -<% if @add_listen_directive -%> +<% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) < 0 -%> ssl off; <% end -%> starttls <%= @starttls %>; diff --git a/templates/mailhost/mailhost_ssl.erb b/templates/mailhost/mailhost_ssl.erb index 56c5fc75a..9592c4bff 100644 --- a/templates/mailhost/mailhost_ssl.erb +++ b/templates/mailhost/mailhost_ssl.erb @@ -20,10 +20,10 @@ server { <% end -%> <%- if @listen_ip.is_a?(Array) then -%> <%- @listen_ip.each do |ip| -%> - listen <%= ip %>:<%= @ssl_port %><% unless @add_listen_directive -%> ssl<% end -%>; + listen <%= ip %>:<%= @ssl_port %><% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) >= 0 -%> ssl<% end -%>; <%- end -%> <%- else -%> - listen <%= @listen_ip %>:<%= @ssl_port %><% unless @add_listen_directive -%> ssl<% end -%>; + listen <%= @listen_ip %>:<%= @ssl_port %><% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) >= 0 -%> ssl<% end -%>; <%- end -%> <%# check to see if ipv6 support exists in the kernel before applying -%> <%# FIXME this logic is duplicated all over the place -%> @@ -38,7 +38,7 @@ server { <%- end -%> <%= scope.function_template(["nginx/mailhost/mailhost_common.erb"]) -%> -<% if @add_listen_directive -%> +<% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) < 0 -%> ssl on; <% end -%> starttls off; diff --git a/templates/server/server_ssl_settings.erb b/templates/server/server_ssl_settings.erb index 0c8b041ec..342db3a24 100644 --- a/templates/server/server_ssl_settings.erb +++ b/templates/server/server_ssl_settings.erb @@ -1,4 +1,4 @@ -<% if @add_listen_directive -%> +<% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) < 0 -%> ssl on; <% end -%> <% if @ssl_cert -%>