From 4f6a2ea27ffa4451a327002bca045194d937f7aa Mon Sep 17 00:00:00 2001 From: Christopher Thorn Date: Tue, 14 Nov 2023 13:29:45 -0800 Subject: [PATCH] (PE-36365) Puppet resource service should error if service is not present Previously if a service was not present on a system and a user tried to manage that service with `puppet resource service`, and that service wasn't there an error message would appear on the screen but the puppet command would return a 0 exit code. This commit is meant to bubble up those error messages so they will be reflected on the command line properly. First pass will be to update the Windows and Systemd providers. blank --- lib/puppet/application/resource.rb | 1 + lib/puppet/provider/service/windows.rb | 2 +- lib/puppet/type/service.rb | 4 +++ spec/unit/application/resource_spec.rb | 11 ++++++- spec/unit/provider/service/systemd_spec.rb | 38 ++++++++++++++++++---- spec/unit/provider/service/windows_spec.rb | 4 +-- spec/unit/type/service_spec.rb | 6 ++++ 7 files changed, 56 insertions(+), 10 deletions(-) diff --git a/lib/puppet/application/resource.rb b/lib/puppet/application/resource.rb index 1e692f642cb..0144b604c17 100644 --- a/lib/puppet/application/resource.rb +++ b/lib/puppet/application/resource.rb @@ -237,6 +237,7 @@ def find_or_save_resources(type, name, params) save_result, report = Puppet::Resource.indirection.save(resource, key) status = report.resource_statuses[resource.ref] raise "Failed to manage resource #{resource.ref}" if status&.failed? + [ save_result ] end else diff --git a/lib/puppet/provider/service/windows.rb b/lib/puppet/provider/service/windows.rb index 4712dd4b0ba..1ad6ebc2aae 100644 --- a/lib/puppet/provider/service/windows.rb +++ b/lib/puppet/provider/service/windows.rb @@ -92,7 +92,7 @@ def stop end def status - return :stopped unless Puppet::Util::Windows::Service.exists?(@resource[:name]) + return :absent unless Puppet::Util::Windows::Service.exists?(@resource[:name]) current_state = Puppet::Util::Windows::Service.service_state(@resource[:name]) state = case current_state diff --git a/lib/puppet/type/service.rb b/lib/puppet/type/service.rb index fceb66bb863..95bc2970fea 100644 --- a/lib/puppet/type/service.rb +++ b/lib/puppet/type/service.rb @@ -111,6 +111,10 @@ def insync?(current) end newvalue(:absent) + + validate do |val| + fail "Managing absent on a service is not supported" if val.to_s == 'absent' + end aliasvalue(:false, :stopped) aliasvalue(:true, :running) diff --git a/spec/unit/application/resource_spec.rb b/spec/unit/application/resource_spec.rb index b257696eac8..46078f25e23 100644 --- a/spec/unit/application/resource_spec.rb +++ b/spec/unit/application/resource_spec.rb @@ -118,12 +118,19 @@ @resource_app.main end + before :each do + allow(@res).to receive(:ref).and_return("type/name") + end + it "should add given parameters to the object" do allow(@resource_app.command_line).to receive(:args).and_return(['type','name','param=temp']) expect(Puppet::Resource.indirection).to receive(:save).with(@res, 'type/name').and_return([@res, @report]) expect(Puppet::Resource).to receive(:new).with('type', 'name', {:parameters => {'param' => 'temp'}}).and_return(@res) + resource_status = instance_double('Puppet::Resource::Status') + allow(@report).to receive(:resource_statuses).and_return({'type/name' => resource_status}) + allow(resource_status).to receive(:failed?).and_return(false) @resource_app.main end end @@ -140,11 +147,13 @@ def exists? true end + def string=(value) + end + def string Puppet::Util::Execution::ProcessOutput.new('test', 0) end end - it "should not emit puppet class tags when printing yaml when strict mode is off" do Puppet[:strict] = :warning diff --git a/spec/unit/provider/service/systemd_spec.rb b/spec/unit/provider/service/systemd_spec.rb index 3f5d5c5f377..765dfff7105 100644 --- a/spec/unit/provider/service/systemd_spec.rb +++ b/spec/unit/provider/service/systemd_spec.rb @@ -388,19 +388,42 @@ # Note: systemd provider does not care about hasstatus or a custom status # command. I just assume that it does not make sense for systemd. describe "#status" do - it "should return running if the command returns 0" do - provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd.service')) - expect(provider).to receive(:execute) - .with(['/bin/systemctl','is-active', '--', 'sshd.service'], {:failonfail => false, :override_locale => false, :squelch => false, :combine => true}) - .and_return(Puppet::Util::Execution::ProcessOutput.new("active\n", 0)) + it 'when called on a service that does not exist returns absent' do + provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'doesnotexist.service')) + expect(provider).to receive(:exist?).and_return(false) + expect(provider.status).to eq(:absent) + end + + it 'when called on a service that does exist and is running returns running' do + provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'doesexist.service')) + expect(provider).to receive(:execute). + with(['/bin/systemctl','cat', '--', 'doesexist.service'], {:failonfail=>false}). + and_return(Puppet::Util::Execution::ProcessOutput.new("# /lib/systemd/system/doesexist.service\n...", 0)).once + expect(provider).to receive(:execute). + with(['/bin/systemctl','is-active', '--', 'doesexist.service'], {:combine=>true, :failonfail=>false, :override_locale=>false, :squelch=>false}). + and_return(Puppet::Util::Execution::ProcessOutput.new("# /lib/systemd/system/doesexist.service\n...", 0)).once expect(provider.status).to eq(:running) end + it 'when called on a service that does exist and is not running returns stopped' do + provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'doesexist.service')) + expect(provider).to receive(:execute). + with(['/bin/systemctl','cat', '--', 'doesexist.service'], {:failonfail=>false}). + and_return(Puppet::Util::Execution::ProcessOutput.new("# /lib/systemd/system/doesexist.service\n...", 0)).once + expect(provider).to receive(:execute). + with(['/bin/systemctl','is-active', '--', 'doesexist.service'], {:combine=>true, :failonfail=>false, :override_locale=>false, :squelch=>false}). + and_return(Puppet::Util::Execution::ProcessOutput.new("inactive\n", 3)).once + expect(provider.status).to eq(:stopped) + end + [-10,-1,3,10].each { |ec| it "should return stopped if the command returns something non-0" do provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd.service')) + expect(provider).to receive(:execute). + with(['/bin/systemctl','cat', '--', 'sshd.service'], {:failonfail=>false}). + and_return(Puppet::Util::Execution::ProcessOutput.new("# /lib/systemd/system/sshd.service\n...", 0)).once expect(provider).to receive(:execute) - .with(['/bin/systemctl','is-active', '--', 'sshd.service'], {:failonfail => false, :override_locale => false, :squelch => false, :combine => true}) + .with(['/bin/systemctl','is-active', '--', 'sshd.service'], {:failonfail => false, :override_locale => false, :squelch => false, :combine => true}) .and_return(Puppet::Util::Execution::ProcessOutput.new("inactive\n", ec)) expect(provider.status).to eq(:stopped) end @@ -408,6 +431,9 @@ it "should use the supplied status command if specified" do provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd.service', :status => '/bin/foo')) + expect(provider).to receive(:execute). + with(['/bin/systemctl','cat', '--', 'sshd.service'], {:failonfail=>false}). + and_return(Puppet::Util::Execution::ProcessOutput.new("# /lib/systemd/system/sshd.service\n...", 0)).once expect(provider).to receive(:execute) .with(['/bin/foo'], {:failonfail => false, :override_locale => false, :squelch => false, :combine => true}) .and_return(process_output) diff --git a/spec/unit/provider/service/windows_spec.rb b/spec/unit/provider/service/windows_spec.rb index 47a71254696..0a04bd07064 100644 --- a/spec/unit/provider/service/windows_spec.rb +++ b/spec/unit/provider/service/windows_spec.rb @@ -81,10 +81,10 @@ end describe "#status" do - it "should report a nonexistent service as stopped" do + it "should report a nonexistent service as absent" do allow(service_util).to receive(:exists?).with(resource[:name]).and_return(false) - expect(provider.status).to eql(:stopped) + expect(provider.status).to eql(:absent) end it "should report service as stopped when status cannot be retrieved" do diff --git a/spec/unit/type/service_spec.rb b/spec/unit/type/service_spec.rb index 0b55e0997be..c706951aa6a 100644 --- a/spec/unit/type/service_spec.rb +++ b/spec/unit/type/service_spec.rb @@ -67,6 +67,12 @@ def safely_load_service_type expect(svc.should(:ensure)).to eq(:stopped) end + describe 'the absent property' do + it 'should fail validate if it is a service' do + expect { Puppet::Type.type(:service).new(:name => "service_name", :ensure => :absent) }.to raise_error(Puppet::Error, /Managing absent on a service is not supported/) + end + end + describe "the enable property" do before :each do allow(@provider.class).to receive(:supports_parameter?).and_return(true)