From 891d0d854f4bb5a2fef5e7e5e482379f4cb790b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Wed, 12 Jul 2023 08:17:15 -1000 Subject: [PATCH 1/5] Make sure acceptance tests are idempotent Rely on voxpupuli-acceptance helper to ensur our test catalogs are working as expected. --- spec/acceptance/init_spec.rb | 51 ++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/spec/acceptance/init_spec.rb b/spec/acceptance/init_spec.rb index 38b1d1c..da83b42 100644 --- a/spec/acceptance/init_spec.rb +++ b/spec/acceptance/init_spec.rb @@ -3,49 +3,44 @@ require 'spec_helper_acceptance' describe 'class caddy:' do - context 'with defaults:' do - pp = 'include caddy' - it 'runs successfully' do - apply_manifest(pp, catch_failures: true) do |r| - expect(r.stderr).not_to match(%r{error}i) - end - end - - it 'runs without changes' do - apply_manifest(pp, catch_failures: true) do |r| - expect(r.exit_code).to be_zero + context 'with default settings' do + it_behaves_like 'an idempotent resource' do + let(:manifest) do + <<~PUPPET + class { 'caddy': + } + PUPPET end end end - context 'from github:' do - pp = "class { 'caddy': + context 'when installing from GitHub' do + it_behaves_like 'an idempotent resource' do + let(:manifest) do + <<~PUPPET + class { 'caddy': install_method => 'github', - }" - it 'installs successfully' do - apply_manifest(pp, catch_failures: true) do |r| - expect(r.stderr).not_to match(%r{error}i) + } + PUPPET end end end context 'with vhosts' do - pp = "include caddy + it_behaves_like 'an idempotent resource' do + let(:manifest) do + <<~PUPPET + class { 'caddy': + } + caddy::vhost {'example1': source => 'puppet:///modules/caddy/etc/caddy/config/example1.conf', } + caddy::vhost {'example2': source => 'puppet:///modules/caddy/etc/caddy/config/example2.conf', - }" - it 'runs successfully' do - apply_manifest(pp, catch_failures: true) do |r| - expect(r.stderr).not_to match(%r{error}i) - end - end - - it 'runs without changes' do - apply_manifest(pp, catch_failures: true) do |r| - expect(r.exit_code).to be_zero + } + PUPPET end end end From a91f4cbba662cc9f72a7f371cc0a3eae45028e0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Wed, 12 Jul 2023 09:02:35 -1000 Subject: [PATCH 2/5] Rework capabilities management Setting capabilities on the binary works fine in docker but the acceptance tests fail when running in Vagrant. While trying to identify the root cause of the issue, it feels inadequate to set the capabilities on the binary itself because it allows any user on the system to run caddy and attach it on a priviled port. Removing the explicit file capabilities management and passing this responsibility to systemd helps solving the above issue, and also fix running the acceptance tests in Vagrant. --- .fixtures.yml | 1 - REFERENCE.md | 4 ++-- manifests/init.pp | 2 +- manifests/install.pp | 8 -------- metadata.json | 4 ---- spec/classes/init_spec.rb | 11 +---------- 6 files changed, 4 insertions(+), 26 deletions(-) diff --git a/.fixtures.yml b/.fixtures.yml index 33e31e0..160f990 100644 --- a/.fixtures.yml +++ b/.fixtures.yml @@ -1,6 +1,5 @@ fixtures: repositories: archive: https://github.com/voxpupuli/puppet-archive.git - file_capability: https://github.com/smoeding/puppet-file_capability.git stdlib: https://github.com/puppetlabs/puppetlabs-stdlib.git systemd: https://github.com/voxpupuli/puppet-systemd.git diff --git a/REFERENCE.md b/REFERENCE.md index 66c5fc3..bea8b05 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -231,11 +231,11 @@ Default value: `undef` ##### `systemd_ambient_capabilities` -Data type: `Optional[String[1]]` +Data type: `String[1]` Controls which capabilities to include in the ambient capability set for the executed process. -Default value: `undef` +Default value: `'CAP_NET_BIND_SERVICE'` ##### `systemd_no_new_privileges` diff --git a/manifests/init.pp b/manifests/init.pp index 60e1ae5..39b7847 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -98,7 +98,7 @@ Integer[0] $systemd_limit_processes = 64, Boolean $systemd_private_devices = true, Optional[String[1]] $systemd_capability_bounding_set = undef, - Optional[String[1]] $systemd_ambient_capabilities = undef, + String[1] $systemd_ambient_capabilities = 'CAP_NET_BIND_SERVICE', Optional[Boolean] $systemd_no_new_privileges = undef, ) { case $caddy_architecture { diff --git a/manifests/install.pp b/manifests/install.pp index 3186e0d..2ba1582 100644 --- a/manifests/install.pp +++ b/manifests/install.pp @@ -24,7 +24,6 @@ group => 'root', creates => $bin_file, cleanup => true, - notify => File_capability[$bin_file], require => File[$caddy::install_path], } } else { @@ -38,7 +37,6 @@ mode => '0755', source => $caddy_dl_url, replace => false, # Don't download the file on every run - notify => File_capability[$bin_file], } } @@ -48,10 +46,4 @@ group => $caddy::caddy_group, mode => '0755', } - - include file_capability - file_capability { $bin_file: - ensure => present, - capability => 'cap_net_bind_service=ep', - } } diff --git a/metadata.json b/metadata.json index a2bf4ee..70a5da8 100644 --- a/metadata.json +++ b/metadata.json @@ -19,10 +19,6 @@ { "name": "puppetlabs/stdlib", "version_requirement": ">= 4.25.0 < 10.0.0" - }, - { - "name": "stm/file_capability", - "version_requirement": ">= 3.0.0 < 7.0.0" } ], "operatingsystem_support": [ diff --git a/spec/classes/init_spec.rb b/spec/classes/init_spec.rb index f5d677f..ee4cf37 100644 --- a/spec/classes/init_spec.rb +++ b/spec/classes/init_spec.rb @@ -57,17 +57,9 @@ with_mode('0755'). with_source('https://caddyserver.com/api/download?os=linux&arch=amd64&plugins=http.git,http.filter,http.ipfilter&license=personal&telemetry=off'). with_replace(false). - that_notifies('File_capability[/opt/caddy/caddy]'). that_requires('File[/opt/caddy]') end - it do - expect(subject).to contain_file_capability('/opt/caddy/caddy').with( - 'ensure' => 'present', - 'capability' => 'cap_net_bind_service=ep' - ).that_subscribes_to('File[/opt/caddy/caddy]') - end - it do expect(subject).to contain_file('/var/lib/caddy').with( 'ensure' => 'directory', @@ -159,8 +151,7 @@ 'creates' => '/opt/caddy/caddy', 'cleanup' => 'true' ). - that_requires('File[/opt/caddy]'). - that_notifies('File_capability[/opt/caddy/caddy]') + that_requires('File[/opt/caddy]') end end end From 7def4c92f34107dc70bdda23468ddf295c866d9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Wed, 12 Jul 2023 11:12:06 -1000 Subject: [PATCH 3/5] Test if the installed version is the requested one --- spec/acceptance/init_spec.rb | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/spec/acceptance/init_spec.rb b/spec/acceptance/init_spec.rb index da83b42..f69e57a 100644 --- a/spec/acceptance/init_spec.rb +++ b/spec/acceptance/init_spec.rb @@ -2,6 +2,12 @@ require 'spec_helper_acceptance' +# The default configuration download the latest available release. In order to +# avoid to maintain the test suite to match each release, query GitHub API to +# find the last release. +latest_release = JSON.parse(URI.open('https://api.github.com/repos/caddyserver/caddy/releases/latest').read)['tag_name'] + +# rubocop:disable RSpec/RepeatedExampleGroupDescription describe 'class caddy:' do context 'with default settings' do it_behaves_like 'an idempotent resource' do @@ -12,6 +18,10 @@ class { 'caddy': PUPPET end end + + describe command('/opt/caddy/caddy version') do + its(:stdout) { is_expected.to start_with latest_release } + end end context 'when installing from GitHub' do @@ -20,10 +30,30 @@ class { 'caddy': <<~PUPPET class { 'caddy': install_method => 'github', + version => '2.6.0', } PUPPET end end + + describe command('/opt/caddy/caddy version') do + its(:stdout) { is_expected.to start_with 'v2.6.0' } + end + + it_behaves_like 'an idempotent resource' do + let(:manifest) do + <<~PUPPET + class { 'caddy': + install_method => 'github', + version => '#{latest_release.sub(%r{\Av}, '')}', + } + PUPPET + end + end + + describe command('/opt/caddy/caddy version') do + its(:stdout) { is_expected.to start_with latest_release } + end end context 'with vhosts' do @@ -45,3 +75,4 @@ class { 'caddy': end end end +# rubocop:enable RSpec/RepeatedExampleGroupDescription From c8cc29a1378a82106496d07be2ba9611fdaac3d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Wed, 12 Jul 2023 11:42:00 -1000 Subject: [PATCH 4/5] Fix installation of a specific version The module previously never touched caddy once it had been installed. While we legitimately do not want to update the binary when we fetch the latest version form the internet, we want to see an acutal change when we update the catalog configuration to change the installation method or the specific version we want to install. --- manifests/install.pp | 30 +++++++++++++++++++++++++----- spec/classes/init_spec.rb | 30 +++++++++++++++++++++++------- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/manifests/install.pp b/manifests/install.pp index 2ba1582..2516c1c 100644 --- a/manifests/install.pp +++ b/manifests/install.pp @@ -13,24 +13,36 @@ $caddy_dl_url = "${caddy_url}/v${caddy::version}/caddy_${caddy::version}_linux_${caddy::arch}.tar.gz" $caddy_dl_dir = "${caddy::caddy_tmp_dir}/caddy_${caddy::version}_linux_${$caddy::arch}.tar.gz" + $extract_path = "${caddy::caddy_tmp_dir}/caddy-${caddy::version}" + + file { $extract_path: + ensure => directory, + owner => 'root', + group => 'root', + mode => '0755', + } + archive { $caddy_dl_dir: ensure => present, extract => true, - extract_path => $caddy::install_path, + extract_path => $extract_path, source => $caddy_dl_url, username => $caddy::caddy_account_id, password => $caddy::caddy_api_key, user => 'root', group => 'root', - creates => $bin_file, - cleanup => true, - require => File[$caddy::install_path], + require => File[$extract_path], + before => File[$bin_file], } + + $caddy_source = "${caddy::caddy_tmp_dir}/caddy-${caddy::version}/caddy" } else { $caddy_url = 'https://caddyserver.com/api/download' $caddy_dl_url = "${caddy_url}?os=linux&arch=${caddy::arch}&plugins=${caddy::caddy_features}&license=${caddy::caddy_license}&telemetry=${caddy::caddy_telemetry}" - file { $bin_file: + $caddy_source = "${caddy::caddy_tmp_dir}/caddy-latest" + + file { $caddy_source: ensure => file, owner => 'root', group => 'root', @@ -46,4 +58,12 @@ group => $caddy::caddy_group, mode => '0755', } + + file { $bin_file: + ensure => file, + owner => 'root', + group => 'root', + mode => '0755', + source => $caddy_source, + } } diff --git a/spec/classes/init_spec.rb b/spec/classes/init_spec.rb index ee4cf37..ba4ff46 100644 --- a/spec/classes/init_spec.rb +++ b/spec/classes/init_spec.rb @@ -50,13 +50,22 @@ end it do - expect(subject).to contain_file('/opt/caddy/caddy'). + expect(subject).to contain_file('/tmp/caddy-latest'). with_ensure('file'). with_owner('root'). with_group('root'). with_mode('0755'). with_source('https://caddyserver.com/api/download?os=linux&arch=amd64&plugins=http.git,http.filter,http.ipfilter&license=personal&telemetry=off'). - with_replace(false). + with_replace(false) + end + + it do + expect(subject).to contain_file('/opt/caddy/caddy'). + with_ensure('file'). + with_owner('root'). + with_group('root'). + with_mode('0755'). + with_source('/tmp/caddy-latest'). that_requires('File[/opt/caddy]') end @@ -144,13 +153,20 @@ expect(subject).to contain_archive('/tmp/caddy_2.0.0_linux_amd64.tar.gz').with( 'ensure' => 'present', 'extract' => 'true', - 'extract_path' => '/opt/caddy', + 'extract_path' => '/tmp/caddy-2.0.0', 'source' => 'https://github.com/caddyserver/caddy/releases/download/v2.0.0/caddy_2.0.0_linux_amd64.tar.gz', 'user' => 'root', - 'group' => 'root', - 'creates' => '/opt/caddy/caddy', - 'cleanup' => 'true' - ). + 'group' => 'root' + ) + end + + it do + expect(subject).to contain_file('/opt/caddy/caddy'). + with_ensure('file'). + with_owner('root'). + with_group('root'). + with_mode('0755'). + with_source('/tmp/caddy-2.0.0/caddy'). that_requires('File[/opt/caddy]') end end From adeb011467f087b602fb5995cb437377de121e4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Wed, 12 Jul 2023 12:40:15 -1000 Subject: [PATCH 5/5] Do not cache files in /tmp The "intermediate" downloaded files needed to unbreak version change are intended to be preserved between reboots. Drop the `caddy::caddy_tmp_dir` parameter and store cached downloads in `/var/cache` instead of the temporary directory. --- REFERENCE.md | 9 --------- manifests/init.pp | 4 ---- manifests/install.pp | 8 ++++---- spec/classes/init_spec.rb | 10 +++++----- 4 files changed, 9 insertions(+), 22 deletions(-) diff --git a/REFERENCE.md b/REFERENCE.md index bea8b05..ce9b2f7 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -62,7 +62,6 @@ The following parameters are available in the `caddy` class: * [`caddy_group`](#-caddy--caddy_group) * [`caddy_shell`](#-caddy--caddy_shell) * [`caddy_log_dir`](#-caddy--caddy_log_dir) -* [`caddy_tmp_dir`](#-caddy--caddy_tmp_dir) * [`caddy_home`](#-caddy--caddy_home) * [`caddy_ssl_dir`](#-caddy--caddy_ssl_dir) * [`caddy_license`](#-caddy--caddy_license) @@ -133,14 +132,6 @@ Directory where the log files are stored. Default value: `'/var/log/caddy'` -##### `caddy_tmp_dir` - -Data type: `Stdlib::Absolutepath` - -Directory where the Caddy archive is stored. - -Default value: `'/tmp'` - ##### `caddy_home` Data type: `Stdlib::Absolutepath` diff --git a/manifests/init.pp b/manifests/init.pp index 39b7847..0089bb5 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -36,9 +36,6 @@ # @param caddy_log_dir # Directory where the log files are stored. # -# @param caddy_tmp_dir -# Directory where the Caddy archive is stored. -# # @param caddy_home # Directory where the Caddy data is stored. # @@ -86,7 +83,6 @@ String[1] $caddy_group = 'caddy', Stdlib::Absolutepath $caddy_shell = '/sbin/nologin', Stdlib::Absolutepath $caddy_log_dir = '/var/log/caddy', - Stdlib::Absolutepath $caddy_tmp_dir = '/tmp', Stdlib::Absolutepath $caddy_home = '/var/lib/caddy', Stdlib::Absolutepath $caddy_ssl_dir = '/etc/ssl/caddy', Enum['personal', 'commercial'] $caddy_license = 'personal', diff --git a/manifests/install.pp b/manifests/install.pp index 2516c1c..cfa14ba 100644 --- a/manifests/install.pp +++ b/manifests/install.pp @@ -11,9 +11,9 @@ if $caddy::install_method == 'github' { $caddy_url = 'https://github.com/caddyserver/caddy/releases/download' $caddy_dl_url = "${caddy_url}/v${caddy::version}/caddy_${caddy::version}_linux_${caddy::arch}.tar.gz" - $caddy_dl_dir = "${caddy::caddy_tmp_dir}/caddy_${caddy::version}_linux_${$caddy::arch}.tar.gz" + $caddy_dl_dir = "/var/cache/caddy_${caddy::version}_linux_${$caddy::arch}.tar.gz" - $extract_path = "${caddy::caddy_tmp_dir}/caddy-${caddy::version}" + $extract_path = "/var/cache/caddy-${caddy::version}" file { $extract_path: ensure => directory, @@ -35,12 +35,12 @@ before => File[$bin_file], } - $caddy_source = "${caddy::caddy_tmp_dir}/caddy-${caddy::version}/caddy" + $caddy_source = "/var/cache/caddy-${caddy::version}/caddy" } else { $caddy_url = 'https://caddyserver.com/api/download' $caddy_dl_url = "${caddy_url}?os=linux&arch=${caddy::arch}&plugins=${caddy::caddy_features}&license=${caddy::caddy_license}&telemetry=${caddy::caddy_telemetry}" - $caddy_source = "${caddy::caddy_tmp_dir}/caddy-latest" + $caddy_source = '/var/cache/caddy-latest' file { $caddy_source: ensure => file, diff --git a/spec/classes/init_spec.rb b/spec/classes/init_spec.rb index ba4ff46..c45dd90 100644 --- a/spec/classes/init_spec.rb +++ b/spec/classes/init_spec.rb @@ -50,7 +50,7 @@ end it do - expect(subject).to contain_file('/tmp/caddy-latest'). + expect(subject).to contain_file('/var/cache/caddy-latest'). with_ensure('file'). with_owner('root'). with_group('root'). @@ -65,7 +65,7 @@ with_owner('root'). with_group('root'). with_mode('0755'). - with_source('/tmp/caddy-latest'). + with_source('/var/cache/caddy-latest'). that_requires('File[/opt/caddy]') end @@ -150,10 +150,10 @@ end it do - expect(subject).to contain_archive('/tmp/caddy_2.0.0_linux_amd64.tar.gz').with( + expect(subject).to contain_archive('/var/cache/caddy_2.0.0_linux_amd64.tar.gz').with( 'ensure' => 'present', 'extract' => 'true', - 'extract_path' => '/tmp/caddy-2.0.0', + 'extract_path' => '/var/cache/caddy-2.0.0', 'source' => 'https://github.com/caddyserver/caddy/releases/download/v2.0.0/caddy_2.0.0_linux_amd64.tar.gz', 'user' => 'root', 'group' => 'root' @@ -166,7 +166,7 @@ with_owner('root'). with_group('root'). with_mode('0755'). - with_source('/tmp/caddy-2.0.0/caddy'). + with_source('/var/cache/caddy-2.0.0/caddy'). that_requires('File[/opt/caddy]') end end