Hi, Only a few nitpicking comments. Feel free to ignore them.
On Tue, 2009-08-11 at 23:06 -0700, Markus Roberts wrote: > This was a "There's A Hole In The Bucket" problem caused when trying > to establish a connection to get a certificate before there was > a certificate with which to establish the connection, ad > infinitum. The solution was to test for the presence of the > CA cert as well as the host cert before attempting to use them. > > This patch modifies existing tests to pass with the new code (by > stubbing out the additional FileTests) and adds a new test which > catches the original problem. Why are your paragraphs indented like this? > Signed-off-by: Markus Roberts <[email protected]> > --- > lib/puppet/network/http_pool.rb | 10 +++------- > spec/unit/network/http_pool.rb | 11 ++++++++++- > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/lib/puppet/network/http_pool.rb b/lib/puppet/network/http_pool.rb > index b1206f7..83a0820 100644 > --- a/lib/puppet/network/http_pool.rb > +++ b/lib/puppet/network/http_pool.rb > @@ -51,7 +51,7 @@ module Puppet::Network::HttpPool > # Use cert information from a Puppet client to set up the http object. > def self.cert_setup(http) > # Just no-op if we don't have certs. > - return false unless FileTest.exist?(Puppet[:hostcert]) # > ssl_host.certificate > + return false unless FileTest.exist?(Puppet[:hostcert]) and > FileTest.exist?(Puppet[:localcacert]) Looks like there is a double-space or a tab in "and FileTest". > http.cert_store = ssl_host.ssl_store > http.ca_file = Puppet[:localcacert] > @@ -60,7 +60,7 @@ module Puppet::Network::HttpPool > http.key = ssl_host.key.content > end > > - # Retrieve a cached http instance of caching is enabled, else return > + # Retrieve a cached http instance if caching is enabled, else return > # a new one. > def self.http_instance(host, port, reset = false) > # We overwrite the uninitialized @http here with a cached one. > @@ -95,11 +95,7 @@ module Puppet::Network::HttpPool > http.read_timeout = Puppet[:configtimeout] > http.open_timeout = Puppet[:configtimeout] > # JJM Configurable fix for #896. > - if Puppet[:http_enable_post_connection_check] > - http.enable_post_connection_check = true > - else > - http.enable_post_connection_check = false > - end > + http.enable_post_connection_check = > Puppet[:http_enable_post_connection_check] I'm really nitpicking here (I like to nitpick when it's not one of my patch, mouahahah :-)), but this is isn't strictly equivalent. What if Puppet[:http_enable_post_connection_check] contains something that evaluates as being true but is not "True" (in the sense of the boolean object instance true). It might not matter of course, because this setting might always be strictly true or false, what matters is that we're giving this to someone else that might not have the same view on what's true or false... > cert_setup(http) > > diff --git a/spec/unit/network/http_pool.rb b/spec/unit/network/http_pool.rb > index ce76309..65f91ef 100755 > --- a/spec/unit/network/http_pool.rb > +++ b/spec/unit/network/http_pool.rb > @@ -147,8 +147,10 @@ describe Puppet::Network::HttpPool do > Puppet[:confdir] = "/sometthing/else" > Puppet.settings.stubs(:value).returns "/some/file" > Puppet.settings.stubs(:value).with(:hostcert).returns > "/host/cert" > + Puppet.settings.stubs(:value).with(:localcacert).returns > "/local/ca/cert" > > FileTest.stubs(:exist?).with("/host/cert").returns true > + FileTest.stubs(:exist?).with("/local/ca/cert").returns true > > Puppet::Network::HttpPool.stubs(:ssl_host).returns @host > end > @@ -157,12 +159,18 @@ describe Puppet::Network::HttpPool do > Puppet.settings.clear > end > > - it "should do nothing if no certificate is on disk" do > + it "should do nothing if no host certificate is on disk" do > FileTest.expects(:exist?).with("/host/cert").returns false > @http.expects(:cert=).never > Puppet::Network::HttpPool.cert_setup(@http) > end > > + it "should do nothing if no local certificate is on disk" do > + FileTest.expects(:exist?).with("/local/ca/cert").returns false > + @http.expects(:cert=).never > + Puppet::Network::HttpPool.cert_setup(@http) > + end > + > it "should add a certificate store from the ssl host" do > @http.expects(:cert_store=).with(@store) > > @@ -192,6 +200,7 @@ describe Puppet::Network::HttpPool do > FileTest.stubs(:exist?).with(Puppet[:hostcert]).returns true > > Puppet.settings.stubs(:value).with(:localcacert).returns > "/ca/cert/file" > + FileTest.stubs(:exist?).with("/ca/cert/file").returns true > @http.expects(:ca_file=).with("/ca/cert/file") > > Puppet::Network::HttpPool.cert_setup(@http) -- Brice Figureau My Blog: http://www.masterzen.fr/ --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
