commit 4e68ddfe501223b1d55edd0b90198d108603f259
Author: Julian Todt <julian.todt@dm.de>
Date:   Thu May 17 13:09:44 2018 +0200

    Fixes #23626 - Move autosign to build queue

diff --git a/app/controllers/unattended_controller.rb b/app/controllers/unattended_controller.rb
index 7fae0c622..f0f6836d3 100644
--- a/app/controllers/unattended_controller.rb
+++ b/app/controllers/unattended_controller.rb
@@ -14,7 +14,6 @@ class UnattendedController < ApplicationController
   before_action :get_host_details, :except => [:hostgroup_template, :built, :failed]
   before_action :get_built_host_details, :only => [:built, :failed]
   before_action :allowed_to_install?, :except => :hostgroup_template
-  before_action :handle_ca, :if => Proc.new { params[:kind] == 'provision' }
   before_action :handle_realm, :if => Proc.new { params[:kind] == 'provision' }
   # load "helper" variables to be available in the templates
   before_action :load_template_vars, :only => :host_template
@@ -207,21 +206,6 @@ class UnattendedController < ApplicationController
     (@host.build || @spoof || Setting[:access_unattended_without_build]) ? true : head(:method_not_allowed)
   end
 
-  # Cleans Certificate and enable autosign. This is run as a before_action for provisioning templates.
-  # The host is requesting its build configuration so I guess we just send them some text so a post mortum can see what happened
-  def handle_ca
-    # The reason we do it here is to minimize the amount of time it is possible to automatically get a certificate
-
-    # We don't do anything if we are in spoof mode.
-    return true if @spoof
-
-    # This should terminate the before_action and the action. We return a HTTP
-    # error so the installer knows something is wrong. This is tested with
-    # Anaconda, but maybe Suninstall will choke on it.
-    render(:plain => _("Failed to clean any old certificates or add the autosign entry. Terminating the build!"), :status => :internal_server_error) unless @host.handle_ca
-    # TODO: Email the user who initiated this build operation.
-  end
-
   # Reset realm OTP. This is run as a before_action for provisioning templates.
   def handle_realm
     # We don't do anything if we are in spoof mode.
diff --git a/app/models/concerns/orchestration/compute.rb b/app/models/concerns/orchestration/compute.rb
index 3352c9dcc..ab4a85822 100644
--- a/app/models/concerns/orchestration/compute.rb
+++ b/app/models/concerns/orchestration/compute.rb
@@ -104,7 +104,6 @@ module Orchestration::Compute
     end
 
     self.compute_attributes[:user_data] = unattended_render(template.template)
-    self.handle_ca
 
     return false if errors.any?
     logger.info "Revoked old certificates and enabled autosign for UserData"
diff --git a/app/models/concerns/orchestration/puppetca.rb b/app/models/concerns/orchestration/puppetca.rb
index d463d1ab6..e813a5336 100644
--- a/app/models/concerns/orchestration/puppetca.rb
+++ b/app/models/concerns/orchestration/puppetca.rb
@@ -30,6 +30,12 @@ module Orchestration::Puppetca
   def setCertificate
   end
 
+  # Reset certname based on whether to use uuids or the hostname
+  def resetCertname
+    logger.info "Resetting certname for #{name}"
+    self.certname = Setting[:use_uuid_for_certificates] ? Foreman.uuid : hostname
+  end
+
   # Adds the host's name to the autosign.conf file
   def setAutosign
     logger.info "Adding autosign entry for #{name}"
@@ -50,28 +56,46 @@ module Orchestration::Puppetca
     new_record? ? queue_puppetca_create : queue_puppetca_update
   end
 
-  # we don't perform any actions upon create
-  # PuppetCA is set only when a provisioning script (such as a kickstart) is being requested.
+  def queue_puppetca_certname_reset
+    queue.create(:name => _("Reset PuppetCA certname for %s") % self, :priority => 49,
+                 :action => [self, :resetCertname])
+  end
+
   def queue_puppetca_create
+    queue.create(:name => _("Cleanup PuppetCA certificates for %s") % self, :priority => 51,
+                 :action => [self, :delCertificate])
+    queue.create(:name => _("Enable PuppetCA autosigning for %s") % self, :priority => 55,
+                 :action => [self, :setAutosign])
   end
 
   def queue_puppetca_update
-    # Host has been built --> remove auto sign
-    queue_puppetca_autosign_destroy if old.build? && !build?
+    if old.build? && !build?
+      # Host has been built --> remove auto sign
+      queue_puppetca_autosign_destroy
+    elsif !old.build? && build?
+      # Host was set to build mode
+      # If use_uuid_for_certificates is true, reuse the certname UUID value.
+      # If false, then reset the certname if it does not match the hostname.
+      if (Setting[:use_uuid_for_certificates] ? !Foreman.is_uuid?(certname) : certname != hostname)
+        queue_puppetca_certname_reset
+      end
+      queue_puppetca_autosign_destroy
+      queue_puppetca_create
+    end
     true
   end
 
   def queue_puppetca_destroy
     return unless puppetca? && errors.empty?
     return unless Setting[:manage_puppetca]
-    queue.create(:name => _("Delete PuppetCA certificates for %s") % self, :priority => 50,
+    queue.create(:name => _("Delete PuppetCA certificates for %s") % self, :priority => 59,
                  :action => [self, :delCertificate])
     queue_puppetca_autosign_destroy
     true
   end
 
   def queue_puppetca_autosign_destroy
-    queue.create(:name => _("Delete PuppetCA autosign entry for %s") % self, :priority => 55,
+    queue.create(:name => _("Disable PuppetCA autosigning for %s") % self, :priority => 50,
                  :action => [self, :delAutosign])
   end
 end
diff --git a/app/models/host/managed.rb b/app/models/host/managed.rb
index 8c837248e..2b04e8bf3 100644
--- a/app/models/host/managed.rb
+++ b/app/models/host/managed.rb
@@ -310,29 +310,6 @@ class Host::Managed < Host::Base
     (puppetmaster == "puppet") ? "puppet.#{domain.name}" : puppetmaster.to_s
   end
 
-  # Cleans Certificate and enable Autosign
-  # Called before a host is given their provisioning template
-  # Returns : Boolean status of the operation
-  def handle_ca
-    # If there's no puppetca, tell the caller that everything is ok
-    return true unless Setting[:manage_puppetca]
-    return true unless puppetca?
-
-    # From here out, we expect things to work and return true
-    return false unless respond_to?(:initialize_puppetca, true)
-    return false unless initialize_puppetca
-    return false unless delCertificate
-
-    # If use_uuid_for_certificates is true, reuse the certname UUID value.
-    # If false, then reset the certname if it does not match the hostname.
-    if (Setting[:use_uuid_for_certificates] ? !Foreman.is_uuid?(certname) : certname != hostname)
-      logger.info "Removing certificate value #{certname} for host #{name}"
-      self.certname = nil
-    end
-
-    setAutosign
-  end
-
   def import_facts(facts, source_proxy = nil)
     # Facts come from 'existing' attributes/infrastructure. We skip triggering
     # the orchestration of this infrastructure when we create a host this way.
diff --git a/test/models/host_test.rb b/test/models/host_test.rb
index a367af95c..b0925e69e 100644
--- a/test/models/host_test.rb
+++ b/test/models/host_test.rb
@@ -1028,85 +1028,6 @@ class HostTest < ActiveSupport::TestCase
       assert_equal domain, host.domain
     end
 
-    test "handle_ca must not perform actions when the manage_puppetca setting is false" do
-      h = FactoryBot.create(:host)
-      Setting[:manage_puppetca] = false
-      h.expects(:initialize_puppetca).never
-      h.expects(:setAutosign).never
-      assert h.handle_ca
-    end
-
-    test "handle_ca must not perform actions when no Puppet CA proxy is associated even if associated with hostgroup" do
-      hostgroup = FactoryBot.create(:hostgroup, :with_puppet_orchestration, :with_domain, :with_os)
-      h = FactoryBot.create(:host, :managed, :with_environment, :hostgroup => hostgroup)
-      Setting[:manage_puppetca] = true
-
-      h.puppet_proxy_id = h.puppet_ca_proxy_id = nil
-      h.save
-
-      refute h.puppetca?
-
-      h.expects(:initialize_puppetca).never
-      assert h.handle_ca
-    end
-
-    test "handle_ca must not perform actions when no Puppet CA proxy is associated" do
-      h = FactoryBot.create(:host)
-      Setting[:manage_puppetca] = true
-      refute h.puppetca?
-      h.expects(:initialize_puppetca).never
-      assert h.handle_ca
-    end
-
-    test "handle_ca must call initialize, delete cert and add autosign methods" do
-      h = FactoryBot.create(:host, :with_puppet_orchestration)
-      Setting[:manage_puppetca] = true
-      assert h.puppetca?
-      h.expects(:initialize_puppetca).returns(true)
-      h.expects(:delCertificate).returns(true)
-      h.expects(:setAutosign).returns(true)
-      assert h.handle_ca
-    end
-
-    test "if the user toggles off the use_uuid_for_certificates option, revoke the UUID and autosign the hostname" do
-      h = FactoryBot.create(:host, :with_puppet_orchestration)
-      Setting[:manage_puppetca] = true
-      assert h.puppetca?
-
-      Setting[:use_uuid_for_certificates] = false
-      some_uuid = Foreman.uuid
-      h.certname = some_uuid
-
-      h.expects(:initialize_puppetca).returns(true)
-      mock_puppetca = Object.new
-      mock_puppetca.expects(:del_certificate).with(some_uuid).returns(true)
-      mock_puppetca.expects(:set_autosign).with(h.name).returns(true)
-      h.instance_variable_set("@puppetca", mock_puppetca)
-
-      assert h.handle_ca
-      assert_equal h.certname, h.name
-    end
-
-    test "if the user changes a hostname in non-use_uuid_for_cetificates mode, revoke the old hostname and autosign the new hostname" do
-      Setting[:use_uuid_for_certificates] = false
-      Setting[:manage_puppetca] = true
-
-      h = FactoryBot.create(:host, :with_puppet_orchestration)
-      assert h.puppetca?
-
-      old_name = 'oldhostname'
-      h.certname = old_name
-
-      h.expects(:initialize_puppetca).returns(true)
-      mock_puppetca = Object.new
-      mock_puppetca.expects(:del_certificate).with(old_name).returns(true)
-      mock_puppetca.expects(:set_autosign).with(h.name).returns(true)
-      h.instance_variable_set("@puppetca", mock_puppetca)
-
-      assert h.handle_ca
-      assert_equal h.certname, h.name
-    end
-
     test "custom_disk_partition_with_erb" do
       h = FactoryBot.create(:host)
       h.disk = "<%= template_name %>"
diff --git a/test/models/orchestration/puppetca_test.rb b/test/models/orchestration/puppetca_test.rb
index 3dc1de5c8..2123a46f6 100644
--- a/test/models/orchestration/puppetca_test.rb
+++ b/test/models/orchestration/puppetca_test.rb
@@ -18,22 +18,130 @@ class PuppetCaOrchestrationTest < ActiveSupport::TestCase
   end
 
   context 'a host with puppetca orchestration' do
-    let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => true) }
-
-    test 'should queue puppetca update' do
-      host.build = false
-      assert_valid host
-      tasks = host.queue.all.map(&:name)
-      assert_includes tasks, "Delete PuppetCA autosign entry for #{host}"
-      assert_equal 1, tasks.size
+    context 'when entering build mode on creation' do
+      let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => true) }
+
+      test 'should queue puppetca autosigning' do
+        assert_valid host
+        tasks = host.queue.all.sort.map(&:name)
+        assert_equal tasks[0], "Cleanup PuppetCA certificates for #{host}"
+        assert_equal tasks[1], "Enable PuppetCA autosigning for #{host}"
+        assert_equal 2, tasks.size
+      end
+
+      test 'should use the hostname for autosigning on setting' do
+        Setting[:use_uuid_for_certificates] = false
+        assert_valid host
+        assert_equal host.certname, host.hostname
+        assert host.send(:initialize_puppetca)
+        host.puppetca.expects(:set_autosign).with(host.hostname).returns(true)
+        assert host.send(:setAutosign)
+      end
+
+      test 'should use a uuid for autosigning on setting' do
+        Setting[:use_uuid_for_certificates] = true
+        assert_valid host
+        assert Foreman.is_uuid?(host.certname)
+        assert host.send(:initialize_puppetca)
+        host.puppetca.expects(:set_autosign).with(host.certname).returns(true)
+        assert host.send(:setAutosign)
+      end
+    end
+
+    context 'when reentering build mode' do
+      let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => false) }
+
+      setup do
+        @host = host
+        @host.queue.clear
+        @host.build = true
+        @host.save!
+      end
+
+      test 'should queue puppetca autosigning' do
+        tasks = @host.queue.all.sort.map(&:name)
+        assert_equal tasks[0], "Disable PuppetCA autosigning for #{host}"
+        assert_equal tasks[1], "Cleanup PuppetCA certificates for #{host}"
+        assert_equal tasks[2], "Enable PuppetCA autosigning for #{host}"
+        assert_equal 3, tasks.size
+      end
     end
 
-    test 'should queue puppetca destroy' do
-      host.send(:queue_puppetca_destroy)
-      tasks = host.queue.all.map(&:name)
-      assert_includes tasks, "Delete PuppetCA autosign entry for #{host}"
-      assert_includes tasks, "Delete PuppetCA certificates for #{host}"
-      assert_equal 2, tasks.size
+    context 'when reentering build mode after certname setting was changed' do
+      let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => false) }
+
+      test 'should reset certname when changing from hostname to uuid' do
+        assert_valid host
+        host.queue.clear
+        Setting[:use_uuid_for_certificates] = true
+        host.build = true
+        host.save!
+        tasks = host.queue.all.sort.map(&:name)
+        assert_equal tasks[0], "Disable PuppetCA autosigning for #{host}"
+        assert_equal tasks[1], "Cleanup PuppetCA certificates for #{host}"
+        assert_equal tasks[2], "Enable PuppetCA autosigning for #{host}"
+        assert_equal 3, tasks.size
+        # Foreman updates the certname automatically in this case
+        assert Foreman.is_uuid?(host.certname)
+      end
+
+      test 'should reset certname when changing from uuid to hostname' do
+        Setting[:use_uuid_for_certificates] = true
+        assert_valid host
+        host.queue.clear
+        Setting[:use_uuid_for_certificates] = false
+        host.build = true
+        host.save!
+        tasks = host.queue.all.sort.map(&:name)
+        assert_equal tasks[0], "Reset PuppetCA certname for #{host}"
+        assert_equal tasks[1], "Disable PuppetCA autosigning for #{host}"
+        assert_equal tasks[2], "Cleanup PuppetCA certificates for #{host}"
+        assert_equal tasks[3], "Enable PuppetCA autosigning for #{host}"
+        assert_equal 4, tasks.size
+      end
+    end
+
+    context 'when host leaves build mode' do
+      let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => true) }
+
+      setup do
+        @host = host
+        @host.queue.clear
+        @host.build = false
+        @host.save!
+      end
+
+      test 'should remove autosign entry for host' do
+        tasks = @host.queue.all.sort.map(&:name)
+        assert_equal tasks[0], "Disable PuppetCA autosigning for #{host}"
+        assert_equal 1, tasks.size
+      end
+    end
+
+    context 'when host is updated' do
+      let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => false) }
+
+      test 'should not queue anything if build mode is not changed' do
+        assert_valid host
+        host.queue.clear
+        host.comment = "updated"
+        host.save!
+        assert_equal 0, host.queue.all.size
+      end
+    end
+
+    context 'when host gets destroyed' do
+      let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => false) }
+
+      test 'should queue puppetca destroy' do
+        assert_valid host
+        host.queue.clear
+        host.send(:queue_puppetca_destroy)
+        tasks = host.queue.all.sort.map(&:name)
+        assert_equal tasks[0], "Disable PuppetCA autosigning for #{host}"
+        assert_equal tasks[1], "Delete PuppetCA certificates for #{host}"
+        assert_equal 2, tasks.size
+      end
     end
   end
 end
