From 4539b1cccbc5bf6287e322d7296c4b7cfc47e4ce Mon Sep 17 00:00:00 2001 From: Andrew Shafer Date: Fri, 13 Jun 2008 18:11:20 -0600 Subject: Issue 1215 Removed logic to reuse parser and log on server when there is a parsing error. Now we just make a new parser and if there is an error, raise it up, which will result in errors on the client logs. --- lib/puppet/parser/interpreter.rb | 19 ++----------------- spec/unit/parser/interpreter.rb | 27 +++------------------------ 2 files changed, 5 insertions(+), 41 deletions(-) diff --git a/lib/puppet/parser/interpreter.rb b/lib/puppet/parser/interpreter.rb index 04ca41494..423c34a4e 100644 --- a/lib/puppet/parser/interpreter.rb +++ b/lib/puppet/parser/interpreter.rb @@ -50,23 +50,8 @@ class Puppet::Parser::Interpreter # Return the parser for a specific environment. def parser(environment) if ! @parsers[environment] or @parsers[environment].reparse? - # This will throw an exception if it does not succeed. We only - # want to get rid of the old parser if we successfully create a new - # one. - begin - tmp = create_parser(environment) - @parsers[environment].clear if @parsers[environment] - @parsers[environment] = tmp - rescue => detail - # If a parser already exists, than assume that we logged the - # exception elsewhere and reuse the parser. If one doesn't - # exist, then reraise. - if @parsers[environment] - Puppet.err(detail.to_s + "; using previously parsed manifests") - else - raise detail - end - end + # This will throw an exception if it does not succeed. + @parsers[environment] = create_parser(environment) end @parsers[environment] end diff --git a/spec/unit/parser/interpreter.rb b/spec/unit/parser/interpreter.rb index f2526c73d..27cf7056d 100755 --- a/spec/unit/parser/interpreter.rb +++ b/spec/unit/parser/interpreter.rb @@ -84,7 +84,6 @@ describe Puppet::Parser::Interpreter do oldparser = mock('oldparser') newparser = mock('newparser') oldparser.expects(:reparse?).returns(true) - oldparser.expects(:clear) @interp.expects(:create_parser).with(:myenv).returns(oldparser) @interp.send(:parser, :myenv).should equal(oldparser) @@ -92,36 +91,16 @@ describe Puppet::Parser::Interpreter do @interp.send(:parser, :myenv).should equal(newparser) end - it "should keep the old parser if a new parser cannot be created" do + it "should raise an exception if a new parser cannot be created" do # Get the first parser in the hash. @interp.expects(:create_parser).with(:myenv).returns(@parser) @interp.send(:parser, :myenv).should equal(@parser) - # Have it indicate something has changed @parser.expects(:reparse?).returns(true) - # But fail to create a new parser - @interp.expects(:create_parser).with(:myenv).raises(ArgumentError) + @interp.expects(:create_parser).with(:myenv).raises(Puppet::Error, "Could not parse") - # And make sure we still get the old valid parser - @interp.send(:parser, :myenv).should equal(@parser) - end - - it "should log syntax errors when using the old parser" do - # Get the first parser in the hash. - @interp.stubs(:create_parser).with(:myenv).returns(@parser) - @interp.send(:parser, :myenv) - - # Have it indicate something has changed - @parser.stubs(:reparse?).returns(true) - - # But fail to create a new parser - @interp.stubs(:create_parser).with(:myenv).raises(ArgumentError) - - Puppet.expects(:err) - - # And make sure we still get the old valid parser - @interp.send(:parser, :myenv) + lambda { @interp.parser(:myenv) }.should raise_error(Puppet::Error) end end end -- cgit From 4d70449a61b2e1ed198b38c8e75a44410c6adcaf Mon Sep 17 00:00:00 2001 From: Andrew Shafer Date: Sat, 14 Jun 2008 12:37:58 -0600 Subject: Fix bug in test, add more specs and small refactor The tests were failing when run on a machine with certs on the file system Stub out failure to read where appropriate Worked fine at my desk :( --- lib/puppet/executables/client/certhandler.rb | 15 +++- spec/unit/executables/client/certhandler.rb | 106 +++++++++++++++++++-------- 2 files changed, 86 insertions(+), 35 deletions(-) diff --git a/lib/puppet/executables/client/certhandler.rb b/lib/puppet/executables/client/certhandler.rb index d2ead3950..b041397ae 100644 --- a/lib/puppet/executables/client/certhandler.rb +++ b/lib/puppet/executables/client/certhandler.rb @@ -4,13 +4,21 @@ module Puppet module Client class CertHandler attr_writer :wait_for_cert, :one_time + attr_reader :new_cert def initialize(wait_time, is_one_time) @wait_for_cert = wait_time @one_time = is_one_time @new_cert = false end + + # Did we just read a cert? + def new_cert? + new_cert + end + # Read, or retrieve if necessary, our certificate. Returns true if we retrieved + # a new cert, false if the cert already exists. def read_retrieve #NOTE: ACS this is checking that a file exists, maybe next time just do that? unless read_cert @@ -19,7 +27,7 @@ module Puppet retrieve_cert end - !@new_cert + ! new_cert? end def retrieve_cert @@ -46,13 +54,14 @@ module Puppet end def read_cert - Puppet::Network::HttpPool.read_cert + Puppet::Network::HttpPool.read_cert end def read_new_cert if Puppet::Network::HttpPool.read_cert # If we read it in, then we need to get rid of our existing http connection. - # The @new_cert flag will help us do that + # The @new_cert flag will help us do that, in that it provides a way + # to notify that the cert status has changed. @new_cert = true Puppet.notice "Got signed certificate" else diff --git a/spec/unit/executables/client/certhandler.rb b/spec/unit/executables/client/certhandler.rb index 0a7b77f15..4f8f8139c 100755 --- a/spec/unit/executables/client/certhandler.rb +++ b/spec/unit/executables/client/certhandler.rb @@ -14,67 +14,109 @@ describe cert_handler, "when handling certificates" do Puppet::Network::Client.stubs(:ca).returns(caclient_class) end - it "should return true if the certificate exists" do - Puppet::Network::HttpPool.expects(:read_cert).returns(true) - cert_handler.new(1,true).read_retrieve.should be_true - end - - it "should return false when getting a new cert" do - Puppet::Network::HttpPool.expects(:read_cert).returns(true) - @caclient.stubs(:request_cert).returns(true) - ch = cert_handler.new(1,true) - ch.stubs(:read_cert).returns(false) - ch.read_retrieve.should be_false + describe "when reading or retrieving the certificate" do + before do + @handler = cert_handler.new(1,true) + end + + it "should attempt to read the certificate" do + @handler.expects(:read_cert).returns true + @handler.read_retrieve + end + + it "should delegate to Puppet::Network::HttpPool to read the certificate" do + Puppet::Network::HttpPool.expects(:read_cert).returns(true) + @handler.read_retrieve + end + + it "should not attempt to retrieve a certificate if one can be read" do + @handler.stubs(:read_cert).returns true + @handler.expects(:retrieve_cert).never + @handler.read_retrieve + end + + it "should attempt to retrieve a certificate if none can be read" do + @handler.stubs(:read_cert).returns false + @handler.expects(:retrieve_cert) + @handler.read_retrieve + end + + it "should delegate to caclient to retrieve a certificate" do + @handler.stubs(:read_cert).returns false + @caclient.expects(:request_cert).returns(true) + @handler.stubs(:read_new_cert).returns(true) + @handler.read_retrieve + end + + it "should return true if the certificate exists" do + @handler.stubs(:read_cert).returns true + @handler.read_retrieve.should be_true + end + + it "should return false when getting a new cert" do + #This is the second call to httppool that happens in 'read_new_cert' + Puppet::Network::HttpPool.expects(:read_cert).returns(true) + @caclient.stubs(:request_cert).returns(true) + @handler.stubs(:read_cert).returns(false) + @handler.read_retrieve.should be_false + end end describe "when waiting for cert" do + before do + @handler = cert_handler.new(1,false) + @handler.stubs(:read_cert).returns false + #all waiting for cert tests should loop, which will always happen if sleep is called + #yeah, I put the expectation in the setup, deal with it + @handler.expects(:sleep).with(1) + + #This is needed to get out of the loop + @handler.stubs(:read_new_cert).returns(true) + end + it "should loop when the cert request does not return a certificate" do @caclient.stubs(:request_cert).times(2).returns(false).then.returns(true) - ch = cert_handler.new(1,false) - ch.expects(:sleep) - ch.expects(:read_new_cert).returns(true) - ch.read_retrieve + @handler.retrieve_cert end it "should loop when the cert request raises an Error" do @caclient.stubs(:request_cert).times(2).raises(StandardError, 'Testing').then.returns(true) - ch = cert_handler.new(1,false) - ch.expects(:sleep) - ch.expects(:read_new_cert).returns(true) - ch.read_retrieve + @handler.retrieve_cert end it "should loop when the new cert can't be read" do @caclient.stubs(:request_cert).returns(true) - ch = cert_handler.new(1,false) - ch.expects(:sleep) - ch.expects(:read_new_cert).times(2).returns(false).then.returns(true) - ch.read_retrieve + @handler.stubs(:read_new_cert).times(2).returns(false).then.returns(true) + @handler.retrieve_cert end end describe "when in one time mode" do + before do + #true puts us in onetime mode + @handler = cert_handler.new(1,true) + @handler.stubs(:read_cert).returns false + end + it "should exit if the cert request does not return a certificate" do @caclient.stubs(:request_cert).returns(false) - ch = cert_handler.new(1,true) - ch.expects(:exit).with(1).raises(SystemExit) - lambda { ch.read_retrieve }.should raise_error(SystemExit) + @handler.expects(:exit).with(1).raises(SystemExit) + lambda { @handler.retrieve_cert }.should raise_error(SystemExit) end it "should exit if the cert request raises an exception" do @caclient.stubs(:request_cert).raises(StandardError, 'Testing') - ch = cert_handler.new(1,true) - ch.expects(:exit).with(23).raises(SystemExit) - lambda { ch.read_retrieve }.should raise_error(SystemExit) + @handler.expects(:exit).with(23).raises(SystemExit) + lambda { @handler.retrieve_cert }.should raise_error(SystemExit) end it "should exit if the new cert can't be read" do @caclient.stubs(:request_cert).returns(true) + #this is the second, call to httppool inside read_new_cert Puppet::Network::HttpPool.stubs(:read_cert).returns(false) - ch = cert_handler.new(1,true) - ch.expects(:exit).with(34).raises(SystemExit) - lambda { ch.read_retrieve }.should raise_error(SystemExit) + @handler.expects(:exit).with(34).raises(SystemExit) + lambda { @handler.retrieve_cert }.should raise_error(SystemExit) end end end -- cgit