diff --git a/.gitreview b/.gitreview index 19b5cb4..2b4d2a0 100644 --- a/.gitreview +++ b/.gitreview @@ -2,3 +2,4 @@ host=review.opendev.org port=29418 project=openstack/puppet-openstacklib.git +defaultbranch=stable/2024.2 diff --git a/lib/puppet/provider/openstack.rb b/lib/puppet/provider/openstack.rb index 5c261e3..9a403fe 100644 --- a/lib/puppet/provider/openstack.rb +++ b/lib/puppet/provider/openstack.rb @@ -78,6 +78,23 @@ def self.request_without_retry(&block) rc end + # Copy of Puppet::Util::withenv but that filters out + # env variables starting with OS_ from the existing + # environment. + # + # @param hash [Hash] Hash of environment variables + def self.os_withenv(hash) + saved = ENV.to_hash + begin + cleaned_env = ENV.to_hash.reject { |k, _| k.start_with?('OS_') } + ENV.replace(cleaned_env) + ENV.merge!(hash.transform_keys(&:to_s)) + yield + ensure + ENV.replace(saved) + end + end + # Returns an array of hashes, where the keys are the downcased CSV headers # with underscores instead of spaces # @@ -87,7 +104,7 @@ def self.request(service, action, properties, credentials=nil, options={}) env = credentials ? credentials.to_env : {} no_retry = options[:no_retry_exception_msgs] - Puppet::Util.withenv(env) do + os_withenv(env) do rv = nil end_time = current_time + request_timeout start_time = current_time diff --git a/lib/puppet/provider/openstack/auth.rb b/lib/puppet/provider/openstack/auth.rb index 5a181da..677c954 100644 --- a/lib/puppet/provider/openstack/auth.rb +++ b/lib/puppet/provider/openstack/auth.rb @@ -19,7 +19,7 @@ def get_os_vars_from_env end def get_os_vars_from_cloudsfile(scope) - cloudsfile = clouds_filenames.detect { |f| File.exists? f} + cloudsfile = clouds_filenames.detect { |f| File.exist? f} unless cloudsfile.nil? { 'OS_CLOUD' => scope, @@ -32,7 +32,7 @@ def get_os_vars_from_cloudsfile(scope) def get_os_vars_from_rcfile(filename) env = {} - rcfile = [filename, '/root/openrc'].detect { |f| File.exists? f } + rcfile = [filename, '/root/openrc'].detect { |f| File.exist? f } unless rcfile.nil? File.open(rcfile).readlines.delete_if{|l| l=~ /^#|^$/ }.each do |line| # we only care about the OS_ vars from the file LP#1699950 diff --git a/releasenotes/notes/os-withenv-3ca466fde75f6441.yaml b/releasenotes/notes/os-withenv-3ca466fde75f6441.yaml new file mode 100644 index 0000000..e381ca6 --- /dev/null +++ b/releasenotes/notes/os-withenv-3ca466fde75f6441.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + The ``Puppet::Provider::Openstack.request`` now filters out all external + ``OS_*`` environment variables to prevent collisions with environment + variables in the shell where Puppet is running. diff --git a/spec/acceptance/mysql_spec.rb b/spec/acceptance/mysql_spec.rb index 8286782..4b3b4ca 100644 --- a/spec/acceptance/mysql_spec.rb +++ b/spec/acceptance/mysql_spec.rb @@ -10,7 +10,7 @@ class { 'mysql::server': } - $charset = $::operatingsystem ? { + $charset = $facts['os']['name'] ? { 'Ubuntu' => 'utf8mb3', default => 'utf8', } diff --git a/spec/unit/provider/openstack/auth_spec.rb b/spec/unit/provider/openstack/auth_spec.rb index 6f33d35..2dc0b99 100644 --- a/spec/unit/provider/openstack/auth_spec.rb +++ b/spec/unit/provider/openstack/auth_spec.rb @@ -88,7 +88,7 @@ class Puppet::Provider::Openstack::AuthTester < Puppet::Provider::Openstack describe '#get_os_vars_from_cloudsfile' do context 'with a clouds.yaml present' do it 'provides a hash' do - expect(File).to receive(:exists?).with('/etc/openstack/puppet/clouds.yaml').and_return(true) + expect(File).to receive(:exist?).with('/etc/openstack/puppet/clouds.yaml').and_return(true) response = klass.get_os_vars_from_cloudsfile('project') expect(response).to eq({ @@ -100,8 +100,8 @@ class Puppet::Provider::Openstack::AuthTester < Puppet::Provider::Openstack context 'with a admin-clouds.yaml present' do it 'provides a hash' do - expect(File).to receive(:exists?).with('/etc/openstack/puppet/clouds.yaml').and_return(false) - expect(File).to receive(:exists?).with('/etc/openstack/puppet/admin-clouds.yaml').and_return(true) + expect(File).to receive(:exist?).with('/etc/openstack/puppet/clouds.yaml').and_return(false) + expect(File).to receive(:exist?).with('/etc/openstack/puppet/admin-clouds.yaml').and_return(true) response = klass.get_os_vars_from_cloudsfile('project') expect(response).to eq({ @@ -113,8 +113,8 @@ class Puppet::Provider::Openstack::AuthTester < Puppet::Provider::Openstack context 'with a clouds.yaml not present' do it 'provides an empty hash' do - expect(File).to receive(:exists?).with('/etc/openstack/puppet/clouds.yaml').and_return(false) - expect(File).to receive(:exists?).with('/etc/openstack/puppet/admin-clouds.yaml').and_return(false) + expect(File).to receive(:exist?).with('/etc/openstack/puppet/clouds.yaml').and_return(false) + expect(File).to receive(:exist?).with('/etc/openstack/puppet/admin-clouds.yaml').and_return(false) response = klass.get_os_vars_from_cloudsfile('project') expect(response).to eq({}) @@ -127,7 +127,7 @@ class Puppet::Provider::Openstack::AuthTester < Puppet::Provider::Openstack it 'provides a hash' do content = "export OS_USERNAME='test'\nexport OS_PASSWORD='abc123'\nexport OS_PROJECT_NAME='test'\nexport OS_AUTH_URL='http://127.0.0.1:5000'" filename = 'file' - expect(File).to receive(:exists?).with('file').and_return(true) + expect(File).to receive(:exist?).with('file').and_return(true) expect(File).to receive(:open).with('file').and_return(StringIO.new(content)) response = klass.get_os_vars_from_rcfile(filename) @@ -143,7 +143,7 @@ class Puppet::Provider::Openstack::AuthTester < Puppet::Provider::Openstack it 'provides a hash' do content = "export OS_USERNAME='test'\nexport OS_PASSWORD='abc123'\nexport OS_PROJECT_NAME='test'\nexport OS_AUTH_URL='http://127.0.0.1:5000'\n_openstack() {\n foo\n} " filename = 'file' - expect(File).to receive(:exists?).with('file').and_return(true) + expect(File).to receive(:exist?).with('file').and_return(true) expect(File).to receive(:open).with('file').and_return(StringIO.new(content)) response = klass.get_os_vars_from_rcfile(filename) @@ -158,7 +158,7 @@ class Puppet::Provider::Openstack::AuthTester < Puppet::Provider::Openstack context 'with an empty file' do it 'provides an empty hash' do filename = 'file' - expect(File).to receive(:exists?).with(filename).and_return(true) + expect(File).to receive(:exist?).with(filename).and_return(true) expect(File).to receive(:open).with(filename).and_return(StringIO.new("")) response = klass.get_os_vars_from_rcfile(filename) @@ -172,8 +172,8 @@ class Puppet::Provider::Openstack::AuthTester < Puppet::Provider::Openstack content = "export OS_USERNAME='user'\nexport OS_PASSWORD='secret'\nexport OS_PROJECT_NAME='project'\nexport OS_AUTH_URL='http://127.0.0.1:5000'" filename = '/root/openrc' - expect(File).to receive(:exists?).with("#{ENV['HOME']}/openrc").and_return(false) - expect(File).to receive(:exists?).with(filename).and_return(true) + expect(File).to receive(:exist?).with("#{ENV['HOME']}/openrc").and_return(false) + expect(File).to receive(:exist?).with(filename).and_return(true) expect(File).to receive(:open).with(filename).and_return(StringIO.new(content)) expect(klass.get_os_vars_from_rcfile("#{ENV['HOME']}/openrc")).to eq({ @@ -252,7 +252,7 @@ class Puppet::Provider::Openstack::AuthTester expect(klass).to receive(:get_os_vars_from_env) .and_return({ 'OS_USERNAME' => 'incompleteusername', 'OS_AUTH_URL' => 'incompleteauthurl' }) - expect(File).to receive(:exists?).with('/etc/openstack/puppet/clouds.yaml').and_return(true) + expect(File).to receive(:exist?).with('/etc/openstack/puppet/clouds.yaml').and_return(true) expect(klass).to receive(:openstack) .with('project', 'list', '--quiet', '--format', 'csv', ['--long']) .and_return('"ID","Name","Description","Enabled" @@ -275,9 +275,9 @@ class Puppet::Provider::Openstack::AuthTester .and_return({ 'OS_USERNAME' => 'incompleteusername', 'OS_AUTH_URL' => 'incompleteauthurl' }) content = "export OS_USERNAME='test'\nexport OS_PASSWORD='abc123'\nexport OS_PROJECT_NAME='test'\nexport OS_AUTH_URL='http://127.0.0.1:5000'\nexport OS_NOT_VALID='notvalid'" - expect(File).to receive(:exists?).with("/etc/openstack/puppet/clouds.yaml").and_return(false) - expect(File).to receive(:exists?).with("/etc/openstack/puppet/admin-clouds.yaml").and_return(false) - expect(File).to receive(:exists?).with("#{ENV['HOME']}/openrc").and_return(true) + expect(File).to receive(:exist?).with("/etc/openstack/puppet/clouds.yaml").and_return(false) + expect(File).to receive(:exist?).with("/etc/openstack/puppet/admin-clouds.yaml").and_return(false) + expect(File).to receive(:exist?).with("#{ENV['HOME']}/openrc").and_return(true) expect(File).to receive(:open).with("#{ENV['HOME']}/openrc").and_return(StringIO.new(content)) expect(klass).to receive(:openstack) .with('project', 'list', '--quiet', '--format', 'csv', ['--long']) @@ -302,9 +302,9 @@ class Puppet::Provider::Openstack::AuthTester expect(klass).to receive(:get_os_vars_from_env) .and_return({ 'OS_TOKEN' => 'incomplete' }) content = "export OS_TOKEN='test'\nexport OS_ENDPOINT='abc123'\nexport OS_NOT_VALID='notvalid'\n" - expect(File).to receive(:exists?).with("/etc/openstack/puppet/clouds.yaml").and_return(false) - expect(File).to receive(:exists?).with("/etc/openstack/puppet/admin-clouds.yaml").and_return(false) - expect(File).to receive(:exists?).with("#{ENV['HOME']}/openrc").and_return(true) + expect(File).to receive(:exist?).with("/etc/openstack/puppet/clouds.yaml").and_return(false) + expect(File).to receive(:exist?).with("/etc/openstack/puppet/admin-clouds.yaml").and_return(false) + expect(File).to receive(:exist?).with("#{ENV['HOME']}/openrc").and_return(true) expect(File).to receive(:open).with("#{ENV['HOME']}/openrc").and_return(StringIO.new(content)) expect(klass).to receive(:openstack) .with('project', 'list', '--quiet', '--format', 'csv', ['--long']) diff --git a/spec/unit/provider/openstack_spec.rb b/spec/unit/provider/openstack_spec.rb index 81b636a..37aa005 100644 --- a/spec/unit/provider/openstack_spec.rb +++ b/spec/unit/provider/openstack_spec.rb @@ -80,7 +80,7 @@ end it 'uses provided credentials' do - expect(Puppet::Util).to receive(:withenv).with(credentials.to_env) + expect(provider.class).to receive(:os_withenv).with(credentials.to_env) Puppet::Provider::Openstack.request('project', 'list', ['--long'], credentials) end @@ -210,4 +210,45 @@ end end end + + describe '#os_withenv' do + around do |example| + @original_env = ENV.to_hash + example.run + ENV.replace(@original_env) + end + + it 'removes environment variables starting with OS_' do + ENV['OS_FOO'] = 'should be removed' + ENV['OTHER'] = 'should stay' + + Puppet::Provider::Openstack.os_withenv({}) do + expect(ENV.key?('OS_FOO')).to be false + expect(ENV['OTHER']).to eq('should stay') + end + end + + it 'merges the given environment variables' do + Puppet::Provider::Openstack.os_withenv({'MY_VAR' => '123'}) do + expect(ENV['MY_VAR']).to eq('123') + end + end + + it 'restores the original environment after the block' do + original = ENV.to_hash + Puppet::Provider::Openstack.os_withenv({'TEMP_VAR' => 'test'}) do + ENV['INSIDE_BLOCK'] = 'yep' + end + + expect(ENV.key?('TEMP_VAR')).to be false + expect(ENV.key?('INSIDE_BLOCK')).to be false + expect(ENV.to_hash).to eq(original) + end + + it 'handles string and symbol keys in the input hash' do + Puppet::Provider::Openstack.os_withenv({:FOO => 'bar'}) do + expect(ENV['FOO']).to eq('bar') + end + end + end end diff --git a/tox.ini b/tox.ini index 5bf76d2..7e06327 100644 --- a/tox.ini +++ b/tox.ini @@ -8,6 +8,6 @@ ignore_basepython_conflict = True basepython = python3 [testenv:releasenotes] -deps = -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} +deps = -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2024.2} -r{toxinidir}/doc/requirements.txt commands = sphinx-build -a -E -W -d releasenotes/build/doctrees -b html releasenotes/source releasenotes/build/html