Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drop support for versions < 1.15.0 #1596

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ nginx::nginx_upstreams:

## Nginx with precompiled Passenger

Example configuration for Debian and RHEL / CentOS (>6), pulling the Nginx and
Example configuration for Debian and RHEL / CentOS, pulling the Nginx and
Passenger packages from the Phusion repo. See additional notes in
[https://github.com/voxpupuli/puppet-nginx/blob/master/docs/quickstart.md](https://github.com/voxpupuli/puppet-nginx/blob/master/docs/quickstart.md)

Expand Down
4 changes: 2 additions & 2 deletions REFERENCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,11 +277,11 @@ Data type: `String[1]`
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'.
already installed. If the fact is unavailable, it defaults to '1.16.0'.
You may need to set this manually to get a working and idempotent
configuration.

Default value: `pick(fact('nginx_version'), '1.6.0')`
Default value: `pick(fact('nginx_version'), '1.16.0')`

##### <a name="-nginx--debug_connections"></a>`debug_connections`

Expand Down
7 changes: 5 additions & 2 deletions manifests/init.pp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
# 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'.
# already installed. If the fact is unavailable, it defaults to '1.16.0'.
# You may need to set this manually to get a working and idempotent
# configuration.
#
Expand Down Expand Up @@ -240,7 +240,7 @@
Hash $nginx_upstreams = {},
Nginx::UpstreamDefaults $nginx_upstreams_defaults = {},
Boolean $purge_passenger_repo = true,
String[1] $nginx_version = pick(fact('nginx_version'), '1.6.0'),
String[1] $nginx_version = pick(fact('nginx_version'), '1.16.0'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we can not just default to installed?

Suggested change
String[1] $nginx_version = pick(fact('nginx_version'), '1.16.0'),
String[1] $nginx_version = 'installed',

Putting an arbitrary (outdated) version does not look good, and if we do a breaking release maybe it is time to improve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me, I was just trying to change as little as possible

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the code does version comparisons all over the place to see if it's compatible. The version comparison with installed wouldn't work.

Note it's also using a fact so the default should most of the time be dynamic.


### END Hiera Lookups ###
) inherits nginx::params {
Expand All @@ -251,6 +251,9 @@
deprecation('keepalive_requests', 'Passing a String is deprecated, please pass a Integer')
}

if versioncmp($nginx_version, '1.15.0') < 0 {
fail("nginx::nginx_version must be at least 1.15.0, got ${nginx_version}")
}
contain 'nginx::package'
contain 'nginx::config'
contain 'nginx::service'
Expand Down
40 changes: 0 additions & 40 deletions spec/acceptance/nginx_mail_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,45 +79,5 @@ 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 = "
if fact('os.family') == 'RedHat' {
package { 'nginx-mod-mail':
ensure => installed,
}
}

class { 'nginx':
mail => true,
nginx_version => '1.14.0',
dynamic_modules => fact('os.family') ? {
'RedHat' => ['/usr/lib64/nginx/modules/ngx_mail_module.so'],
default => [],
}
}
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
end
13 changes: 7 additions & 6 deletions spec/classes/nginx_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@
it { is_expected.to contain_nginx__resource__streamhost('streamhost1').with_proxy('streamproxy') }
end

describe 'unsupported version' do
let(:params) { { nginx_version: '1.14.0' } }

it { is_expected.to compile.and_raise_error(%r{nginx::nginx_version must be at least 1.15.0, got 1.14.0}) }
end

context 'nginx::package' do
it { is_expected.to compile.with_all_deps }

Expand Down Expand Up @@ -189,13 +195,8 @@
let(:params) { { package_source: 'passenger' } }

it { is_expected.to contain_package('nginx') }
it { is_expected.to contain_package('libnginx-mod-http-passenger') }

if (facts.dig(:os, 'name') == 'Debian' && %w[11].include?(facts.dig(:os, 'release', 'major'))) ||
(facts.dig(:os, 'name') == 'Ubuntu' && %w[bionic focal jammy].include?(facts.dig(:os, 'distro', 'codename')))
it { is_expected.to contain_package('libnginx-mod-http-passenger') }
else
it { is_expected.to contain_package('passenger') }
end
it do
is_expected.to contain_apt__source('nginx').with(
'location' => 'https://oss-binaries.phusionpassenger.com/apt/passenger',
Expand Down
2 changes: 1 addition & 1 deletion spec/defines/resource_mailhost_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@
title: 'should set the IPv4 SSL listen port',
attr: 'ssl_port',
value: 45,
match: ' listen *:45;'
match: ' listen *:45 ssl;'
},
{
title: 'should enable IPv6',
Expand Down
14 changes: 0 additions & 14 deletions spec/defines/resource_server_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -667,20 +667,6 @@
)
end

context 'without a value for the nginx_version fact do' do
let :facts do
facts[:nginx_version] ? facts.delete(:nginx_version) : facts
end

it { is_expected.to contain_concat__fragment("#{title}-ssl-header").with_content(%r{ ssl on;}) }
end

context 'with fact nginx_version=1.14.1' do
let(:facts) { facts.merge(nginx_version: '1.14.1') }

it { is_expected.to contain_concat__fragment("#{title}-ssl-header").with_content(%r{ ssl on;}) }
end

context 'with fact nginx_version=1.15.1' do
let(:facts) { facts.merge(nginx_version: '1.15.1') }

Expand Down Expand Up @@ -1448,7 +1434,7 @@

# TODO: implement test after this can be tested
# msg = %r{nginx: ssl must be true if listen_port is the same as ssl_port}
it 'Testing for warnings not yet implemented in classes'

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on virtuozzolinux-7-x86_64 with Facter 3.14.5 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on almalinux-8-x86_64 with Facter 4.2.14 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on almalinux-9-x86_64 with Facter 4.2.14 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on archlinux-rolling-x86_64 with Facter 4.2.9 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on centos-7-x86_64 with Facter 4.2.2 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on centos-8-x86_64 with Facter 4.2.14 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on centos-9-x86_64 with Facter 4.2.14 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on debian-11-x86_64 with Facter 4.2.14 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on redhat-7-x86_64 with Facter 4.2.13 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 8 (Ruby 3.2)

nginx::resource::server on redhat-8-x86_64 with Facter 4.2.14 and Puppet 8.6.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on virtuozzolinux-7-x86_64 with Facter 3.14.5 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on almalinux-8-x86_64 with Facter 4.2.14 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on almalinux-9-x86_64 with Facter 4.2.14 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on archlinux-rolling-x86_64 with Facter 4.2.9 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on centos-7-x86_64 with Facter 4.2.2 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on centos-8-x86_64 with Facter 4.2.14 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on centos-9-x86_64 with Facter 4.2.14 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on debian-11-x86_64 with Facter 4.2.14 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on redhat-7-x86_64 with Facter 4.2.13 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented

Check warning on line 1437 in spec/defines/resource_server_spec.rb

View workflow job for this annotation

GitHub Actions / Puppet / 7 (Ruby 2.7)

nginx::resource::server on redhat-8-x86_64 with Facter 4.2.14 and Puppet 7.30.0 os-independent items attribute resources when listen_port == ssl_port but ssl = false Testing for warnings not yet implemented in classes Skipped: Not yet implemented
end

context 'when listen_port != ssl_port' do
Expand Down
3 changes: 0 additions & 3 deletions templates/mailhost/mailhost.epp
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@ server {
<%- } -%>
<%= $mailhost_common -%>

<%- if versioncmp($nginx_version, '1.15.0') < 0 { -%>
ssl off;
<% } %>
starttls <%= $starttls %>;

<% if $starttls != 'off' { %>
Expand Down
5 changes: 1 addition & 4 deletions templates/mailhost/mailhost_ssl.epp
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,13 @@
server {
<%= $mailhost_prepend -%>
<%- $listen_ip.each |$ip| { -%>
listen <%= $ip %>:<%= $ssl_port %><% if versioncmp($nginx_version, '1.15.0') >= 0 { %> ssl<% } %>;
listen <%= $ip %>:<%= $ssl_port %> ssl;
<%- } -%>
<%- $ipv6_listen_ip.each |$ipv6| { -%>
listen [<%= $ipv6 %>]:<%= $ssl_port %> <% if $ipv6_listen_options { %><%= $ipv6_listen_options %><% } %>;
<%- } -%>
<%= $mailhost_common -%>

<%- if versioncmp($nginx_version, '1.15.0') < 0 { -%>
ssl on;
<% } %>
starttls off;

<%= $mailhost_ssl_settings -%>
Expand Down
3 changes: 0 additions & 3 deletions templates/server/server_ssl_settings.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
<% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) < 0 -%>
ssl on;
<% end -%>
<% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.25.1']) >= 0 && @http2 -%>
http2 <%= @http2 %>;
<% end -%>
Expand Down
Loading