Merge branch '5.5.x' into 55x_64x_mergeup

* 5.5.x:
  Revert "(PUP-10142) Add white space for #initialize_default_settings!"
  (PUP-10227) Preserve expectation for http.finish
  (PUP-10142) Add white space for #initialize_default_settings!
  (PUP-10142) Refactor settings default initialization
  (PUP-10287) mailalias: comma inside commands fix
  (PUP-10227) Close the HTTP connection

 Conflicts:
	lib/puppet/defaults.rb
	lib/puppet/network/http/pool.rb
	lib/puppet/provider/mailalias/aliases.rb
	spec/fixtures/integration/provider/mailalias/aliases/test1
	spec/unit/network/http/connection_spec.rb

Mailalias was removed in puppet 6

Replaced `Puppet.settings` with `settings` in:

    settings.override_default(:catalog_cache_terminus, :store_configs)

Updates the nocache_pool to finish the connection if it's been started. In
5.5.x, the nocache pool did not explicitly start and finish connections, but
that was modified in d46a3b1.

Author: joshcooper

lib/puppet.rb

@@ -22,6 +22,7 @@
 require 'puppet/external/pson/version'
 require 'puppet/external/pson/pure'
 require 'puppet/gettext/config'
+require 'puppet/defaults'
 
 
 #------------------------------------------------------------
@@ -87,6 +88,7 @@ def self.[](param)
 
   # Store a new default value.
   def self.define_settings(section, hash)
+    Puppet.deprecation_warning('The method Puppet.define_settings is deprecated and will be removed in a future release')
     @@settings.define_settings(section, hash)
   end
 
@@ -121,8 +123,9 @@ def self.run_mode
     Puppet::Util::RunMode[@@settings.preferred_run_mode]
   end
 
-  # Load all of the settings.
-  require 'puppet/defaults'
+  # Modify the settings with defaults defined in `initialize_default_settings` method in puppet/defaults.rb. This can
+  # be used in the initialization of new Puppet::Settings objects in the puppetserver project.
+  Puppet.initialize_default_settings!(settings)
 
   # Now that settings are loaded we have the code loaded to be able to issue
   # deprecation warnings. Warn if we're on a deprecated ruby version.

lib/puppet/defaults.rb

@@ -65,7 +65,11 @@ def self.default_vendormoduledir
 
   AS_DURATION = %q{This setting can be a time interval in seconds (30 or 30s), minutes (30m), hours (6h), days (2d), or years (5y).}
 
-  define_settings(:main,
+  # @api public
+  # @param args [Puppet::Settings] the settings object to define default settings for
+  # @return void
+  def self.initialize_default_settings!(settings)
+  settings.define_settings(:main,
     :confdir  => {
         :default  => nil,
         :type     => :directory,
@@ -102,7 +106,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(:main,
+  settings.define_settings(:main,
     :logdir => {
         :default  => nil,
         :type     => :directory,
@@ -206,7 +210,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(:main,
+  settings.define_settings(:main,
     :priority => {
       :default => nil,
       :type    => :priority,
@@ -523,12 +527,12 @@ def self.default_vendormoduledir
     :hiera_config => {
       :default => lambda do
         config = nil
-        codedir = Puppet.settings[:codedir]
+        codedir = settings[:codedir]
         if codedir.is_a?(String)
           config = File.expand_path(File.join(codedir, 'hiera.yaml'))
           config = nil unless Puppet::FileSystem.exist?(config)
         end
-        config = File.expand_path(File.join(Puppet.settings[:confdir], 'hiera.yaml')) if config.nil?
+        config = File.expand_path(File.join(settings[:confdir], 'hiera.yaml')) if config.nil?
         config
       end,
       :desc    => "The hiera configuration file. Puppet only reads this file on startup, so you must restart the puppet master every time you edit it.",
@@ -582,7 +586,7 @@ def self.default_vendormoduledir
     :http_proxy_password =>{
       :default    => "none",
       :hook       => proc do |value|
-        if Puppet.settings[:http_proxy_password] =~ /[@!# \/]/
+        if settings[:http_proxy_password] =~ /[@!# \/]/
           raise "Passwords set in the http_proxy_password setting must be valid as part of a URL, and any reserved characters must be URL-encoded. We received: #{value}"
         end
       end,
@@ -726,7 +730,7 @@ def self.default_vendormoduledir
     }
   )
 
-  Puppet.define_settings(:module_tool,
+  settings.define_settings(:module_tool,
     :module_repository  => {
       :default  => 'https://forgeapi.puppet.com',
       :desc     => "The module repository",
@@ -745,7 +749,7 @@ def self.default_vendormoduledir
     }
   )
 
-    Puppet.define_settings(
+    settings.define_settings(
     :main,
 
     # We have to downcase the fqdn, because the current ssl stuff (as opposed to in master) doesn't have good facilities for
@@ -1027,7 +1031,7 @@ def self.default_vendormoduledir
     }
   )
 
-    define_settings(
+    settings.define_settings(
     :ca,
     :ca_name => {
       :default => "Puppet CA: $certname",
@@ -1128,7 +1132,7 @@ def self.default_vendormoduledir
 
   # Define the config default.
 
-    define_settings(:application,
+    settings.define_settings(:application,
       :config_file_name => {
           :type     => :string,
           :default  => Puppet::Settings.default_config_file_name,
@@ -1153,7 +1157,7 @@ def self.default_vendormoduledir
       },
   )
 
-  define_settings(:environment,
+  settings.define_settings(:environment,
     :manifest => {
       :default    => nil,
       :type       => :file_or_directory,
@@ -1196,7 +1200,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(:master,
+  settings.define_settings(:master,
     :user => {
       :default    => "puppet",
       :desc       => "The user Puppet Server will run as. Used to ensure
@@ -1389,7 +1393,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(:device,
+  settings.define_settings(:device,
     :devicedir =>  {
         :default  => "$vardir/devices",
         :type     => :directory,
@@ -1404,7 +1408,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(:agent,
+  settings.define_settings(:agent,
     :node_name_value => {
       :default => "$certname",
       :desc => "The explicit value used for the node name for all requests the agent
@@ -1748,7 +1752,7 @@ def self.default_vendormoduledir
 
   # Plugin information.
 
-  define_settings(
+  settings.define_settings(
     :main,
     :plugindest => {
       :type       => :directory,
@@ -1791,7 +1795,7 @@ def self.default_vendormoduledir
 
   # Central fact information.
 
-    define_settings(
+    settings.define_settings(
     :main,
     :factpath => {
       :type     => :path,
@@ -1808,7 +1812,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(
+  settings.define_settings(
     :transaction,
     :tags => {
       :default    => "",
@@ -1836,7 +1840,7 @@ def self.default_vendormoduledir
     }
   )
 
-    define_settings(
+    settings.define_settings(
     :main,
     :external_nodes => {
         :default  => "none",
@@ -1861,7 +1865,7 @@ def self.default_vendormoduledir
     }
     )
 
-        define_settings(
+        settings.define_settings(
         :ldap,
     :ldapssl => {
       :default  => false,
@@ -1930,7 +1934,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(:master,
+  settings.define_settings(:master,
     :storeconfigs => {
       :default  => false,
       :type     => :boolean,
@@ -1948,7 +1952,7 @@ def self.default_vendormoduledir
         require 'puppet/node/facts'
         if value
           Puppet::Resource::Catalog.indirection.cache_class = :store_configs
-          Puppet.settings.override_default(:catalog_cache_terminus, :store_configs)
+          settings.override_default(:catalog_cache_terminus, :store_configs)
           Puppet::Node::Facts.indirection.cache_class = :store_configs
           Puppet::Resource.indirection.terminus_class = :store_configs
         end
@@ -1963,7 +1967,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(:parser,
+  settings.define_settings(:parser,
    :max_errors => {
      :default => 10,
      :desc => <<-'EOT'
@@ -2015,7 +2019,7 @@ def self.default_vendormoduledir
     EOT
     }
   )
-  define_settings(:puppetdoc,
+  settings.define_settings(:puppetdoc,
     :document_all => {
         :default  => false,
         :type     => :boolean,
@@ -2024,7 +2028,7 @@ def self.default_vendormoduledir
     }
   )
 
-  define_settings(
+  settings.define_settings(
     :main,
     :rich_data => {
       :default  => true,
@@ -2041,5 +2045,5 @@ def self.default_vendormoduledir
       EOT
     }
   )
-
+  end
 end

lib/puppet/network/http/connection.rb

@@ -213,6 +213,10 @@ def do_request(request, options)
               current_request[header] = value
             end
           when 429, 503
+            if connection.started?
+              Puppet.debug("Closing connection for #{current_site}")
+              connection.finish
+            end
             response = handle_retry_after(current_response)
           else
             response = current_response

lib/puppet/network/http/nocache_pool.rb

@@ -15,6 +15,7 @@ def with_connection(site, verifier, &block)
     begin
       yield http
     ensure
+      return unless http.started?
       Puppet.debug("Closing connection for #{site}")
       http.finish
     end

lib/puppet/network/http/pool.rb

@@ -33,7 +33,7 @@ def with_connection(site, verifier, &block)
       reuse = false
       raise detail
     ensure
-      if reuse
+      if reuse && http.started?
         release(site, verifier, http)
       else
         close_connection(site, http)
@@ -56,13 +56,17 @@ def pool
   end
 
   # Safely close a persistent connection.
+  # Don't try to close a connection that's already closed.
   #
   # @api private
   def close_connection(site, http)
+    return false unless http.started?
     Puppet.debug("Closing connection for #{site}")
     http.finish
+    true
   rescue => detail
     Puppet.log_exception(detail, _("Failed to close connection for %{site}: %{detail}") % { site: site, detail: detail })
+    nil
   end
 
   # Borrow and take ownership of a persistent connection. If a new

spec/unit/network/http/connection_spec.rb

@@ -160,12 +160,33 @@ def retry_after(datetime)
     end
 
     it "should return a 503 response if Retry-After is not convertible to an Integer or RFC 2822 Date" do
-      stub_request(:get, url).to_return(status: [503, 'Service Unavailable'], headers: {'Retry-After' => 'foo'})
+      retry_after('foo')
 
       result = subject.get('/foo')
       expect(result.code).to eq("503")
     end
 
+    it "should close the connection before sleeping" do
+      retry_after('42')
+
+      http1 = Net::HTTP.new(host, port)
+      http1.use_ssl = true
+      allow(http1).to receive(:started?).and_return(true)
+
+      http2 = Net::HTTP.new(host, port)
+      http2.use_ssl = true
+      allow(http1).to receive(:started?).and_return(true)
+
+      # The "with_connection" method is required to yield started connections
+      pool = Puppet.lookup(:http_pool)
+      allow(pool).to receive(:with_connection).and_yield(http1).and_yield(http2)
+
+      expect(http1).to receive(:finish).ordered
+      expect(::Kernel).to receive(:sleep).with(42).ordered
+
+      subject.get('/foo')
+    end
+
     it "should sleep and retry if Retry-After is an Integer" do
       retry_after('42')
 
@@ -189,6 +210,7 @@ def retry_after(datetime)
 
     it "should sleep for no more than the Puppet runinterval" do
       retry_after('60')
+
       Puppet[:runinterval] = 30
 
       expect(::Kernel).to receive(:sleep).with(30)

spec/unit/network/http/nocache_pool_spec.rb

@@ -8,7 +8,7 @@
   let(:verifier) { double('verifier', :setup_connection => nil) }
 
   it 'yields a started connection' do
-    http  = double('http', start: nil, finish: nil)
+    http  = double('http', start: nil, finish: nil, started?: true)
 
     factory = Puppet::Network::HTTP::Factory.new
     allow(factory).to receive(:create_connection).and_return(http)
@@ -20,8 +20,8 @@
   end
 
   it 'yields a new connection each time' do
-    http1  = double('http1', start: nil, finish: nil)
-    http2  = double('http2', start: nil, finish: nil)
+    http1  = double('http1', start: nil, finish: nil, started?: true)
+    http2  = double('http2', start: nil, finish: nil, started?: true)
 
     factory = Puppet::Network::HTTP::Factory.new
     allow(factory).to receive(:create_connection).and_return(http1, http2)