Merge pull request #317 from Sharpie/fix-upgrades-with-pcp

Fail with guidance if peadm::util::retrieve_and_upload is used on PE XL with the PCP transport

Author: timidri

REFERENCE.md

@@ -531,7 +531,7 @@ Fails if any nodes have the chosen transport.
 
 Useful for excluding PCP when it's not appopriate
 
-#### `peadm::fail_on_transport(TargetSpec $nodes, String $transport)`
+#### `peadm::fail_on_transport(TargetSpec $nodes, String $transport, String $message)`
 
 Fails if any nodes have the chosen transport.
 

functions/fail_on_transport.pp

@@ -5,12 +5,13 @@
 function peadm::fail_on_transport (
   TargetSpec $nodes,
   String     $transport,
+  String     $message = 'This is not supported.',
 ) {
   $targets = get_targets($nodes)
   $targets.each |$target| {
     if $target.protocol == $transport {
       fail_plan(
-        "${target.name} uses ${transport} transport. This is not supported",
+        "${target.name} uses ${transport} transport: ${message}",
         'unexpected-transport',
         {
           'target'    => $target,

plans/subplans/modify_certificate.pp

@@ -11,10 +11,20 @@
   $target = get_target($targets)
   $primary_target = get_target($primary_host)
 
-  # This plan doesn't work to reissue the master cert over the orchestrator due
-  # to pe-puppetserver needing to restart
   if ($primary_target == $target) {
-    $primary_target.peadm::fail_on_transport('pcp')
+    $primary_target.peadm::fail_on_transport('pcp', @(HEREDOC/n))
+      \nThe "pcp" transport is not available for use with the Primary
+      as peadm::subplans::modify_certificate will cause a restart of the
+      PE Orchestration service.
+
+      Use the "local" transport if running this plan directly from
+      the Primary node, or the "ssh" transport if running this
+      plan from an external Bolt host.
+
+      For information on configuring transports, see:
+
+          https://www.puppet.com/docs/bolt/latest/bolt_transports_reference.html
+      |-HEREDOC
   }
 
   # Figure out some information from the existing certificate

plans/upgrade.pp

@@ -82,7 +82,19 @@
 
   out::message('# Gathering information')
 
-  $primary_target.peadm::fail_on_transport('pcp')
+  $primary_target.peadm::fail_on_transport('pcp', @(HEREDOC/n))
+    \nThe "pcp" transport is not available for use with the Primary
+    as peadm::upgrade will cause a restart of the
+    PE Orchestration service.
+
+    Use the "local" transport if running this plan directly from
+    the Primary node, or the "ssh" transport if running this
+    plan from an external Bolt host.
+
+    For information on configuring transports, see:
+
+        https://www.puppet.com/docs/bolt/latest/bolt_transports_reference.html
+    |-HEREDOC
 
   $platform = run_task('peadm::precheck', $primary_target).first['platform']
 
@@ -142,9 +154,6 @@
   }
 
   peadm::plan_step('preparation') || {
-    # Support for running over the orchestrator transport relies on Bolt being
-    # executed from the primary using the local transport. For now, fail the plan
-    # if the orchestrator is being used for the primary.
     if $download_mode == 'bolthost' {
       # Download the PE tarball on the nodes that need it
       run_plan('peadm::util::retrieve_and_upload', $pe_installer_targets,

plans/util/retrieve_and_upload.pp

@@ -5,6 +5,28 @@
   String[1]  $local_path,
   String[1]  $upload_path,
 ) {
+  $nodes.peadm::fail_on_transport('pcp', @(HEREDOC/n))
+    \nThe "pcp" transport is not available for uploading PE installers as
+    the ".tar.gz" file is too large to send over the PE Orchestrator
+    as an argument to the "bolt_shim::upload" task.
+
+    To upgrade PE XL database nodes via PCP, use "download_mode = direct".
+    If Puppet download servers are not reachable over the internet,
+    upload the ".tar.gz" to an internal fileserver and use the
+    "pe_installer_source" parameter to retrieve it.
+
+    For information on configuring plan parameters, see:
+
+        https://forge.puppet.com/modules/puppetlabs/peadm/plans
+
+    Or, use the "ssh" transport for database nodes so that the
+    installer can be transferred via SCP.
+
+    For information on configuring transports, see:
+
+        https://www.puppet.com/docs/bolt/latest/bolt_transports_reference.html
+    |-HEREDOC
+
   $exists = without_default_logging() || {
     run_command("test -e '${local_path}'", 'local://localhost',
       _catch_errors => true,

spec/functions/fail_on_transport_spec.rb

@@ -3,12 +3,39 @@
 require 'spec_helper'
 
 describe 'peadm::fail_on_transport' do
+  include BoltSpec::BoltContext
+
+  around :each do |example|
+    in_bolt_context do
+      example.run
+    end
+  end
+
+  # NOTE: If https://github.com/puppetlabs/bolt/issues/3184
+  #       is fixed, this will start causing a duplicate declaration
+  #       error. If that happens, delete this pre_condition.
+  let(:pre_condition) do
+    'type TargetSpec = Boltlib::TargetSpec'
+  end
+
   let(:nodes) do
-    'some_value_goes_here'
+    'pcp://target.example'
   end
-  let(:transport) do
-    'some_value_goes_here'
+
+  # Function testing depends on rspec-puppet magic in the opening describe
+  # statement. Re-defining the subject just to give it a different name
+  # would require duplicating rspec-puppet code, and that's a far worse sin.
+  # rubocop:disable Rspec/NamedSubject
+  it 'raises an error when nodes use the specified transport' do
+    expect { subject.execute(nodes, 'pcp') }.to raise_error(Puppet::PreformattedError, %r{target\.example uses pcp transport: This is not supported\.})
+  end
+
+  it 'raises an error with a custom explanation if one is provided' do
+    expect { subject.execute(nodes, 'pcp', 'It would be bad.') }.to raise_error(Puppet::PreformattedError, %r{target\.example uses pcp transport: It would be bad\.})
   end
 
-  xit { is_expected.to run.with_params(nodes, transport).and_return('some_value') }
+  it 'raises no error when nodes do not use the specified transport' do
+    expect { subject.execute(nodes, 'ssh') }.not_to raise_error
+  end
+  # rubocop:enable Rspec/NamedSubject
 end

spec/plans/subplans/modify_certificate_spec.rb

@@ -39,5 +39,18 @@
         expect(run_plan('peadm::subplans::modify_certificate', params)).to be_ok
       end
     end
+
+    context 'modifying the primary certificate' do
+      it 'fails if the primary is using the PCP transport' do
+        result = run_plan('peadm::subplans::modify_certificate',
+                          { 'targets'          => 'pcp://primary.example',
+                            'primary_host'     => 'pcp://primary.example',
+                            'primary_certname' => 'primary.example' })
+
+        expect(result).not_to be_ok
+        expect(result.value.kind).to eq('unexpected-transport')
+        expect(result.value.msg).to match(%r{The "pcp" transport is not available for use with the Primary})
+      end
+    end
   end
 end

spec/plans/upgrade_spec.rb

@@ -43,4 +43,16 @@ def allow_standard_non_returning_calls
                     'compiler_hosts' => 'compiler',
                     'version' => '2021.7.2')).to be_ok
   end
+
+  it 'fails if the primary uses the pcp transport' do
+    allow_standard_non_returning_calls
+
+    result = run_plan('peadm::upgrade',
+                      'primary_host' => 'pcp://primary.example',
+                      'version' => '2021.7.1')
+
+    expect(result).not_to be_ok
+    expect(result.value.kind).to eq('unexpected-transport')
+    expect(result.value.msg).to match(%r{The "pcp" transport is not available for use with the Primary})
+  end
 end

spec/plans/util/retrieve_and_upload_spec.rb

@@ -41,4 +41,16 @@
 
     expect(run_plan('peadm::util::retrieve_and_upload', 'nodes' => 'primary', 'source' => '/tmp/source', 'upload_path' => '/tmp/upload', 'local_path' => '/tmp/download')).to be_ok
   end
+
+  it 'fails when nodes are configured to use the pcp transport' do
+    result = run_plan('peadm::util::retrieve_and_upload',
+                      { 'nodes'       => ['pcp://node.example'],
+                        'source'      => '/tmp/source',
+                        'upload_path' => '/tmp/upload',
+                        'local_path'  => '/tmp/download' })
+
+    expect(result).not_to be_ok
+    expect(result.value.kind).to eq('unexpected-transport')
+    expect(result.value.msg).to match(%r{The "pcp" transport is not available for uploading PE installers})
+  end
 end