From 92b3c0ce1568ad99a1b0aa7aaf2e1c04edcdc35a Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Tue, 18 Feb 2020 09:11:51 -0500 Subject: [PATCH 1/5] Fix file permissions of /opt/stackstorm/packs and /opt/stackstorm/virtualenvs not being idempotent and also being inefficient. --- .fixtures.yml | 1 + CHANGELOG.md | 5 +++ manifests/init.pp | 3 ++ manifests/pack.pp | 41 ----------------- manifests/profile/server.pp | 44 +++++++++++++++++++ metadata.json | 4 ++ .../stackstorm/controls/st2_test.rb | 21 +++++++++ 7 files changed, 78 insertions(+), 41 deletions(-) diff --git a/.fixtures.yml b/.fixtures.yml index 576564a3..a36b92b3 100644 --- a/.fixtures.yml +++ b/.fixtures.yml @@ -21,3 +21,4 @@ fixtures: selinux: "puppet/selinux" nginx: "puppet/nginx" nodejs: "puppet/nodejs" + recursive_file_permissions: "npwalker/recursive_file_permissions" diff --git a/CHANGELOG.md b/CHANGELOG.md index 136a1c20..58f7be44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Development +- Fixed `/opt/stackstorm/packs` and `/opt/stackstorm/virtualenv` resources to be idempotent + and manage the ownership of these directories recursively in a much more efficient manner. + Instead of using the `file` resource with `recurse => true` we now utilize the module + `npwalker/recursive_file_permissions`. #278 (Bugfix) (Enhancement) + Contributed by @nmaludy ## 1.6.0 (Feb 17, 2020) diff --git a/manifests/init.pp b/manifests/init.pp index 1475c537..055005f3 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -65,6 +65,8 @@ # Set the number of actionrunner processes to start # @param packs # Hash of st2 packages to be installed +# @param packs_group +# Name of the group that will own the /opt/stackstorm/packs directory (default: st2packs) # @param index_url # Url to the StackStorm Exchange index file. (default undef) # @param mistral_db_host @@ -245,6 +247,7 @@ $cli_auth_url = "http://${::st2::params::hostname}:${::st2::params::auth_port}", $actionrunner_workers = $::st2::params::actionrunner_workers, $packs = {}, + $packs_group = $::st2::params::packs_group_name, $index_url = undef, $mistral_db_host = $::st2::params::hostname, $mistral_db_name = $::st2::params::mistral_db_name, diff --git a/manifests/pack.pp b/manifests/pack.pp index dbbb9985..199474c8 100644 --- a/manifests/pack.pp +++ b/manifests/pack.pp @@ -24,43 +24,6 @@ include st2 $_cli_username = $::st2::cli_username $_cli_password = $::st2::cli_password - $_st2_packs_group = $::st2::params::packs_group_name - - ensure_resource('group', $_st2_packs_group, { - 'ensure' => present, - }) - - ensure_resource('file', '/opt/stackstorm', { - 'ensure' => 'directory', - 'owner' => 'root', - 'group' => 'root', - 'mode' => '0755', - }) - - ensure_resource('file', '/opt/stackstorm/packs', { - 'ensure' => 'directory', - 'owner' => 'root', - 'group' => $_st2_packs_group, - 'recurse' => true, - 'tag' => 'st2::subdirs', - }) - - ensure_resource('file', '/opt/stackstorm/configs', { - 'ensure' => 'directory', - 'owner' => 'st2', - 'group' => 'root', - 'mode' => '0755', - 'tag' => 'st2::subdirs', - }) - - ensure_resource('file', '/opt/stackstorm/virtualenvs', { - 'ensure' => 'directory', - 'owner' => 'root', - 'group' => $_st2_packs_group, - 'mode' => '0755', - 'tag' => 'st2::subdirs', - }) - st2_pack { $pack: ensure => $ensure, @@ -86,10 +49,6 @@ ~> Exec<| tag == 'st2::register-configs' |> } - Group[$_st2_packs_group] - -> File['/opt/stackstorm'] - -> File<| tag == 'st2::subdirs' |> - Service<| tag == 'st2::service' |> -> St2_pack<||> Exec<| tag == 'st2::reload' |> -> St2_pack<||> } diff --git a/manifests/profile/server.pp b/manifests/profile/server.pp index 37bf95e3..c1643a04 100644 --- a/manifests/profile/server.pp +++ b/manifests/profile/server.pp @@ -65,6 +65,7 @@ $rabbitmq_port = $::st2::rabbitmq_port, $rabbitmq_vhost = $::st2::rabbitmq_vhost, $index_url = $::st2::index_url, + $packs_group = $::st2::packs_group_name, ) inherits st2 { include st2::notices include st2::params @@ -100,6 +101,46 @@ 'tag' => 'st2::server', }) + ensure_resource('group', $packs_group, { + 'ensure' => present, + }) + + ensure_resource('file', '/opt/stackstorm/configs', { + 'ensure' => 'directory', + 'owner' => 'st2', + 'group' => 'root', + 'mode' => '0755', + 'tag' => 'st2::server', + }) + + ensure_resource('file', '/opt/stackstorm/packs', { + 'ensure' => 'directory', + 'owner' => 'root', + 'group' => $packs_group, + 'mode' => '0775', + 'tag' => 'st2::server', + }) + + ensure_resource('file', '/opt/stackstorm/virtualenvs', { + 'ensure' => 'directory', + 'owner' => 'root', + 'group' => $packs_group, + 'mode' => '0755', + 'tag' => 'st2::server', + }) + + recursive_file_permissions { '/opt/stackstorm/packs': + owner => 'root', + group => $packs_group, + tag => 'st2::server', + } + + recursive_file_permissions { '/opt/stackstorm/virtualenvs': + owner => 'root', + group => $packs_group, + tag => 'st2::server', + } + ######################################## ## Config file { $conf_dir: @@ -371,4 +412,7 @@ Service<| tag == 'st2::service' |> ~> Exec<| tag == 'st2::reload' |> + + St2_pack<||> + ~> Recursive_file_permissions<| tag == 'st2::server' |> } diff --git a/metadata.json b/metadata.json index acd037bb..5cd42592 100644 --- a/metadata.json +++ b/metadata.json @@ -67,6 +67,10 @@ { "name": "puppet/nodejs", "version_requirement": ">= 1.3.0 < 8.0.0" + }, + { + "name": "npwalker/recursive_file_permissions", + "version_requirement": ">= 0.6.0 < 1.0.0" } ], "operatingsystem_support": [ diff --git a/test/integration/stackstorm/controls/st2_test.rb b/test/integration/stackstorm/controls/st2_test.rb index eedcf447..4d385c40 100644 --- a/test/integration/stackstorm/controls/st2_test.rb +++ b/test/integration/stackstorm/controls/st2_test.rb @@ -48,4 +48,25 @@ it { should_not be_executable.by('owner') } it { should_not be_executable.by('group') } end + + describe file('/opt/stackstorm/configs') do + it { should exist } + its('owner') { should eq 'root' } + its('group') { should eq 'st2' } + its('mode') { should cmp '0755' } + end + + describe file('/opt/stackstorm/packs') do + it { should exist } + its('owner') { should eq 'root' } + its('group') { should eq 'st2packs' } + its('mode') { should cmp '0775' } + end + + describe file('/opt/stackstorm/virtualenvs') do + it { should exist } + its('owner') { should eq 'root' } + its('group') { should eq 'st2packs' } + its('mode') { should cmp '0755' } + end end From f04ba2da8b82f6bdb545aa18fcca55f9a2ff4a1f Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Tue, 18 Feb 2020 09:16:23 -0500 Subject: [PATCH 2/5] Added new recursive_file_permissions module to Puppetfiles --- build/centos6-puppet5/Puppetfile | 1 + build/centos6-puppet6/Puppetfile | 1 + build/centos7-puppet5/Puppetfile | 1 + build/centos7-puppet6/Puppetfile | 1 + build/ubuntu14-puppet5/Puppetfile | 1 + build/ubuntu14-puppet6/Puppetfile | 1 + build/ubuntu16-puppet5/Puppetfile | 1 + build/ubuntu16-puppet6/Puppetfile | 1 + build/ubuntu18-puppet5/Puppetfile | 1 + build/ubuntu18-puppet6/Puppetfile | 1 + 10 files changed, 10 insertions(+) diff --git a/build/centos6-puppet5/Puppetfile b/build/centos6-puppet5/Puppetfile index 25115941..01c7bb79 100644 --- a/build/centos6-puppet5/Puppetfile +++ b/build/centos6-puppet5/Puppetfile @@ -62,3 +62,4 @@ mod 'computology-packagecloud' mod 'puppet-selinux' mod 'puppet-nginx' mod 'puppet-nodejs' +mod 'npwalker-recursive_file_permissions' diff --git a/build/centos6-puppet6/Puppetfile b/build/centos6-puppet6/Puppetfile index 25115941..01c7bb79 100644 --- a/build/centos6-puppet6/Puppetfile +++ b/build/centos6-puppet6/Puppetfile @@ -62,3 +62,4 @@ mod 'computology-packagecloud' mod 'puppet-selinux' mod 'puppet-nginx' mod 'puppet-nodejs' +mod 'npwalker-recursive_file_permissions' diff --git a/build/centos7-puppet5/Puppetfile b/build/centos7-puppet5/Puppetfile index 25115941..01c7bb79 100644 --- a/build/centos7-puppet5/Puppetfile +++ b/build/centos7-puppet5/Puppetfile @@ -62,3 +62,4 @@ mod 'computology-packagecloud' mod 'puppet-selinux' mod 'puppet-nginx' mod 'puppet-nodejs' +mod 'npwalker-recursive_file_permissions' diff --git a/build/centos7-puppet6/Puppetfile b/build/centos7-puppet6/Puppetfile index 25115941..01c7bb79 100644 --- a/build/centos7-puppet6/Puppetfile +++ b/build/centos7-puppet6/Puppetfile @@ -62,3 +62,4 @@ mod 'computology-packagecloud' mod 'puppet-selinux' mod 'puppet-nginx' mod 'puppet-nodejs' +mod 'npwalker-recursive_file_permissions' diff --git a/build/ubuntu14-puppet5/Puppetfile b/build/ubuntu14-puppet5/Puppetfile index 25115941..01c7bb79 100644 --- a/build/ubuntu14-puppet5/Puppetfile +++ b/build/ubuntu14-puppet5/Puppetfile @@ -62,3 +62,4 @@ mod 'computology-packagecloud' mod 'puppet-selinux' mod 'puppet-nginx' mod 'puppet-nodejs' +mod 'npwalker-recursive_file_permissions' diff --git a/build/ubuntu14-puppet6/Puppetfile b/build/ubuntu14-puppet6/Puppetfile index 25115941..01c7bb79 100644 --- a/build/ubuntu14-puppet6/Puppetfile +++ b/build/ubuntu14-puppet6/Puppetfile @@ -62,3 +62,4 @@ mod 'computology-packagecloud' mod 'puppet-selinux' mod 'puppet-nginx' mod 'puppet-nodejs' +mod 'npwalker-recursive_file_permissions' diff --git a/build/ubuntu16-puppet5/Puppetfile b/build/ubuntu16-puppet5/Puppetfile index 25115941..01c7bb79 100644 --- a/build/ubuntu16-puppet5/Puppetfile +++ b/build/ubuntu16-puppet5/Puppetfile @@ -62,3 +62,4 @@ mod 'computology-packagecloud' mod 'puppet-selinux' mod 'puppet-nginx' mod 'puppet-nodejs' +mod 'npwalker-recursive_file_permissions' diff --git a/build/ubuntu16-puppet6/Puppetfile b/build/ubuntu16-puppet6/Puppetfile index 25115941..01c7bb79 100644 --- a/build/ubuntu16-puppet6/Puppetfile +++ b/build/ubuntu16-puppet6/Puppetfile @@ -62,3 +62,4 @@ mod 'computology-packagecloud' mod 'puppet-selinux' mod 'puppet-nginx' mod 'puppet-nodejs' +mod 'npwalker-recursive_file_permissions' diff --git a/build/ubuntu18-puppet5/Puppetfile b/build/ubuntu18-puppet5/Puppetfile index 72f2b1e8..621c9f49 100644 --- a/build/ubuntu18-puppet5/Puppetfile +++ b/build/ubuntu18-puppet5/Puppetfile @@ -64,3 +64,4 @@ mod 'puppet-nginx' mod 'puppet-nodejs' mod 'puppetlabs-inifile' mod 'puppetlabs-stdlib' +mod 'npwalker-recursive_file_permissions' diff --git a/build/ubuntu18-puppet6/Puppetfile b/build/ubuntu18-puppet6/Puppetfile index 72f2b1e8..621c9f49 100644 --- a/build/ubuntu18-puppet6/Puppetfile +++ b/build/ubuntu18-puppet6/Puppetfile @@ -64,3 +64,4 @@ mod 'puppet-nginx' mod 'puppet-nodejs' mod 'puppetlabs-inifile' mod 'puppetlabs-stdlib' +mod 'npwalker-recursive_file_permissions' From 2800a01e83de9d431fa300cf449fcb5d8567d29f Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Tue, 18 Feb 2020 12:37:16 -0500 Subject: [PATCH 3/5] Fixing integration test for file permissions --- test/integration/stackstorm/controls/st2_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/stackstorm/controls/st2_test.rb b/test/integration/stackstorm/controls/st2_test.rb index 4d385c40..dbef22dd 100644 --- a/test/integration/stackstorm/controls/st2_test.rb +++ b/test/integration/stackstorm/controls/st2_test.rb @@ -51,8 +51,8 @@ describe file('/opt/stackstorm/configs') do it { should exist } - its('owner') { should eq 'root' } - its('group') { should eq 'st2' } + its('owner') { should eq 'st2' } + its('group') { should eq 'root' } its('mode') { should cmp '0755' } end From 58b7e8b2633142a53af09c3c8a40ac0078598c07 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Wed, 19 Feb 2020 08:04:59 -0500 Subject: [PATCH 4/5] Fixing unit tests for new file permissions --- spec/classes/profile/server_spec.rb | 83 +++++++++++++++++++++++++++++ spec/defines/pack_spec.rb | 27 ---------- 2 files changed, 83 insertions(+), 27 deletions(-) create mode 100644 spec/classes/profile/server_spec.rb diff --git a/spec/classes/profile/server_spec.rb b/spec/classes/profile/server_spec.rb new file mode 100644 index 00000000..c4b00d47 --- /dev/null +++ b/spec/classes/profile/server_spec.rb @@ -0,0 +1,83 @@ +require 'spec_helper' + +describe 'st2::profile::server' do + context 'on RedHat' do + redhat = { + supported_os: [ + { + 'operatingsystem' => 'RedHat', + 'operatingsystemrelease' => ['7'], + }, + ], + } + + on_supported_os(redhat).each do |os, os_facts| + let(:facts) do + os_facts.merge({ + sudoversion: '1.8.23', + }) + end + + context 'with default options' do + it { is_expected.to compile.with_all_deps } + it { is_expected.to contain_class('st2::profile::server') } + + it do + is_expected.to contain_file('/opt/stackstorm').with( + ensure: 'directory', + owner: 'root', + group: 'root', + mode: '0755', + tag: 'st2::server', + ) + end + + it { is_expected.to contain_group('st2packs') } + + it do + is_expected.to contain_file('/opt/stackstorm/configs').with( + ensure: 'directory', + owner: 'st2', + group: 'root', + mode: '0755', + tag: 'st2::server', + ) + end + + it do + is_expected.to contain_file('/opt/stackstorm/packs').with( + ensure: 'directory', + owner: 'root', + group: 'st2packs', + tag: 'st2::server', + ) + end + + it do + is_expected.to contain_file('/opt/stackstorm/virtualenvs').with( + ensure: 'directory', + owner: 'root', + group: 'st2packs', + tag: 'st2::server', + ) + end + + it do + is_expected.to contain_recursive_file_permissions('/opt/stackstorm/packs').with( + owner: 'root', + group: 'st2packs', + tag: 'st2::server', + ) + end + + it do + is_expected.to contain_recursive_file_permissions('/opt/stackstorm/virtualenvs').with( + owner: 'root', + group: 'st2packs', + tag: 'st2::server', + ) + end + end # context 'with default options' + end # on_supported_os(redhat) + end # context 'on RedHat' +end # describe 'st2::profile::server' diff --git a/spec/defines/pack_spec.rb b/spec/defines/pack_spec.rb index 201d7327..9e2fb300 100644 --- a/spec/defines/pack_spec.rb +++ b/spec/defines/pack_spec.rb @@ -17,33 +17,6 @@ } end - it do - is_expected.to contain_file('/opt/stackstorm').with( - ensure: 'directory', - owner: 'root', - group: 'root', - mode: '0755', - ) - end - it do - is_expected.to contain_file('/opt/stackstorm/configs').with( - ensure: 'directory', - owner: 'st2', - group: 'root', - mode: '0755', - ) - end - it do - is_expected.to contain_file('/opt/stackstorm/packs').with( - ensure: 'directory', - owner: 'root', - group: 'st2packs', - ) - end - it do - is_expected.to contain_group('st2packs') - end - context 'when declared with config' do let(:params) do { From 0376f5517ab9a3235b1ff911b9f957815a866247 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Wed, 19 Feb 2020 08:12:42 -0500 Subject: [PATCH 5/5] Fixing unit tests for new file permissions --- spec/classes/profile/server_spec.rb | 142 ++++++++++++++-------------- 1 file changed, 72 insertions(+), 70 deletions(-) diff --git a/spec/classes/profile/server_spec.rb b/spec/classes/profile/server_spec.rb index c4b00d47..9fac4d3f 100644 --- a/spec/classes/profile/server_spec.rb +++ b/spec/classes/profile/server_spec.rb @@ -1,83 +1,85 @@ require 'spec_helper' describe 'st2::profile::server' do - context 'on RedHat' do - redhat = { - supported_os: [ - { - 'operatingsystem' => 'RedHat', - 'operatingsystemrelease' => ['7'], - }, - ], - } + all_os = { + supported_os: [ + { + 'operatingsystem' => 'RedHat', + 'operatingsystemrelease' => ['6', '7'], + }, + { + 'operatingsystem' => 'Ubuntu', + 'operatingsystemrelease' => ['16.04', '18.04'], + }, + ], + } - on_supported_os(redhat).each do |os, os_facts| - let(:facts) do - os_facts.merge({ - sudoversion: '1.8.23', - }) - end + on_supported_os(all_os).each do |os, os_facts| + let(:facts) do + os_facts.merge( + sudoversion: '1.8.23', + ) + end - context 'with default options' do - it { is_expected.to compile.with_all_deps } - it { is_expected.to contain_class('st2::profile::server') } + context "on #{os} with default options" do + it { is_expected.to compile.with_all_deps } + it { is_expected.to contain_class('st2::profile::server') } - it do - is_expected.to contain_file('/opt/stackstorm').with( - ensure: 'directory', - owner: 'root', - group: 'root', - mode: '0755', - tag: 'st2::server', - ) - end + it do + is_expected.to contain_file('/opt/stackstorm').with( + ensure: 'directory', + owner: 'root', + group: 'root', + mode: '0755', + tag: 'st2::server', + ) + end - it { is_expected.to contain_group('st2packs') } + it { is_expected.to contain_group('st2packs') } - it do - is_expected.to contain_file('/opt/stackstorm/configs').with( - ensure: 'directory', - owner: 'st2', - group: 'root', - mode: '0755', - tag: 'st2::server', - ) - end + it do + is_expected.to contain_file('/opt/stackstorm/configs').with( + ensure: 'directory', + owner: 'st2', + group: 'root', + mode: '0755', + tag: 'st2::server', + ) + end - it do - is_expected.to contain_file('/opt/stackstorm/packs').with( - ensure: 'directory', - owner: 'root', - group: 'st2packs', - tag: 'st2::server', - ) - end + it do + is_expected.to contain_file('/opt/stackstorm/packs').with( + ensure: 'directory', + owner: 'root', + group: 'st2packs', + tag: 'st2::server', + ) + end - it do - is_expected.to contain_file('/opt/stackstorm/virtualenvs').with( - ensure: 'directory', - owner: 'root', - group: 'st2packs', - tag: 'st2::server', - ) - end + it do + is_expected.to contain_file('/opt/stackstorm/virtualenvs').with( + ensure: 'directory', + owner: 'root', + group: 'st2packs', + tag: 'st2::server', + ) + end - it do - is_expected.to contain_recursive_file_permissions('/opt/stackstorm/packs').with( - owner: 'root', - group: 'st2packs', - tag: 'st2::server', - ) - end + it do + is_expected.to contain_recursive_file_permissions('/opt/stackstorm/packs').with( + owner: 'root', + group: 'st2packs', + tag: 'st2::server', + ) + end - it do - is_expected.to contain_recursive_file_permissions('/opt/stackstorm/virtualenvs').with( - owner: 'root', - group: 'st2packs', - tag: 'st2::server', - ) - end - end # context 'with default options' - end # on_supported_os(redhat) - end # context 'on RedHat' + it do + is_expected.to contain_recursive_file_permissions('/opt/stackstorm/virtualenvs').with( + owner: 'root', + group: 'st2packs', + tag: 'st2::server', + ) + end + end # context 'on #{os} with default options' + end # on_supported_os(all_os) end # describe 'st2::profile::server'