(#469) Upgrade: Assign correct environment to node groups

This checks if a user configured a environment in pe.conf. If that's the
case, it will be used for the PEADM-specific node groups. Otherwise we
fall back to production.

This fixes a timing issue discovered in #469. In situations where the PE
infra isn't running in production, we cannot assume that a production
environment exists. And a node group can only reference classes from the
environment the node group belongs to.

Author: bastelfreak

REFERENCE.md

@@ -30,6 +30,7 @@
 * [`peadm::file_or_content`](#peadm--file_or_content)
 * [`peadm::flatten_compact`](#peadm--flatten_compact)
 * [`peadm::generate_pe_conf`](#peadm--generate_pe_conf): Generate a pe.conf file in JSON format
+* [`peadm::get_node_group_environment`](#peadm--get_node_group_environment): check if a custom PE environment is set in pe.conf
 * [`peadm::get_pe_conf`](#peadm--get_pe_conf)
 * [`peadm::get_targets`](#peadm--get_targets): Accept undef or a SingleTargetSpec, and return an Array[Target, 1, 0]. This differs from get_target() in that:   - It returns an Array[Target
 * [`peadm::log_plan_parameters`](#peadm--log_plan_parameters)
@@ -740,6 +741,24 @@ Data type: `Hash`
 A hash of settings to set in the config file. Any keys that are set to
 undef will not be included in the config file.
 
+### <a name="peadm--get_node_group_environment"></a>`peadm::get_node_group_environment`
+
+Type: Puppet Language
+
+check if a custom PE environment is set in pe.conf
+
+#### `peadm::get_node_group_environment(Peadm::SingleTargetSpec $primary)`
+
+The peadm::get_node_group_environment function.
+
+Returns: `String` the desired environment for PE specific node groups
+
+##### `primary`
+
+Data type: `Peadm::SingleTargetSpec`
+
+the FQDN for the primary, here we will read the pe.conf from
+
 ### <a name="peadm--get_pe_conf"></a>`peadm::get_pe_conf`
 
 Type: Puppet Language
@@ -1845,7 +1864,7 @@ Data type: `String[1]`
 
 environment for the PEADM specific node groups, if not set it will be gathered from pe.conf or production
 
-Default value: `'production'`
+Default value: `peadm::get_node_group_environment($primary_host)`
 
 ##### <a name="-peadm--add_database--targets"></a>`targets`
 
@@ -1945,7 +1964,7 @@ Data type: `String[1]`
 
 environment for the PEADM specific node groups, if not set it will be gathered from pe.conf or production
 
-Default value: `'production'`
+Default value: `peadm::get_node_group_environment($primary_host)`
 
 ### <a name="peadm--backup"></a>`peadm::backup`
 
@@ -2853,7 +2872,7 @@ Data type: `String[1]`
 
 environment for the PEADM specific node groups, if not set it will be gathered from pe.conf or production
 
-Default value: `'production'`
+Default value: `peadm::get_node_group_environment($primary_host)`
 
 ##### <a name="-peadm--upgrade--primary_host"></a>`primary_host`
 

functions/get_node_group_environment.pp

@@ -0,0 +1,33 @@
+#
+# @summary check if a custom PE environment is set in pe.conf
+#
+# @param primary the FQDN for the primary, here we will read the pe.conf from
+#
+# @return [String] the desired environment for PE specific node groups
+#
+# @see https://www.puppet.com/docs/pe/latest/upgrade_pe#update_environment
+#
+# @author Tim Meusel <tim@bastelfreak.de>
+#
+function peadm::get_node_group_environment(Peadm::SingleTargetSpec $primary) {
+  $peconf = peadm::get_pe_conf(get_target($primary))
+  # if both are set, they need to be set to the same value
+  # if they are not set, we assume that the user runs their infra in production
+  $pe_install = $peconf['pe_install::install::classification::pe_node_group_environment']
+  $puppet_enterprise = $peconf['puppet_enterprise::master::recover_configuration::pe_environment']
+
+  # check if both are equal
+  # This also evaluates to true if both are undef
+  if $pe_install == $puppet_enterprise {
+    # check if the option isn't undef
+    # ToDo: A proper regex for allowed characters in an environment would be nice
+    # https://github.com/puppetlabs/puppet-docs/issues/1158
+    if $pe_install =~ String[1] {
+      return $pe_install
+    } else {
+      return 'production'
+    }
+  } else {
+    fail("pe_install::install::classification::pe_node_group_environment and puppet_enterprise::master::recover_configuration::pe_environment need to be set to the same value, not '${pe_install}' and '${puppet_enterprise}'")
+  }
+}

plans/add_database.pp

@@ -13,7 +13,7 @@
       'cleanup-db',
   'finalize']] $begin_at_step = undef,
   Optional[Boolean] $is_migration = false,
-  String[1] $node_group_environment = 'production',
+  String[1] $node_group_environment = peadm::get_node_group_environment($primary_host),
 ) {
   $primary_target = peadm::get_targets($primary_host, 1)
   $postgresql_target = peadm::get_targets($targets, 1)

plans/add_replica.pp

@@ -18,7 +18,7 @@
 
   # Common Configuration
   Optional[String] $token_file = undef,
-  String[1] $node_group_environment = 'production',
+  String[1] $node_group_environment = peadm::get_node_group_environment($primary_host),
 ) {
   $primary_target             = peadm::get_targets($primary_host, 1)
   $replica_target             = peadm::get_targets($replica_host, 1)

plans/convert_compiler_to_legacy.pp

@@ -3,7 +3,7 @@
   Peadm::SingleTargetSpec $primary_host,
   TargetSpec              $legacy_hosts,
   Optional[Boolean]       $remove_pdb = true,
-  String[1] $node_group_environment = 'production',
+  String[1] $node_group_environment = peadm::get_node_group_environment($primary_host),
 ) {
   $primary_target            = peadm::get_targets($primary_host, 1)
   $convert_legacy_compiler_targets   = peadm::get_targets($legacy_hosts)

plans/upgrade.pp

@@ -63,7 +63,7 @@
   Boolean                    $permit_unsafe_versions = false,
 
   Optional[Peadm::UpgradeSteps] $begin_at_step = undef,
-  String[1] $node_group_environment = 'production',
+  String[1] $node_group_environment = peadm::get_node_group_environment($primary_host),
 ) {
   # Log parameters for debugging
   peadm::log_plan_parameters({

spec/functions/get_node_group_environment_spec.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'peadm::get_node_group_environment' do
+  include BoltSpec::BoltContext
+  around(:each) do |example|
+    in_bolt_context do
+      example.run
+    end
+  end
+  let(:primary) do
+    ['test-vm.puppet.vm']
+  end
+
+  context 'returns production on empty pe.conf' do
+    it do
+      expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => '{}' })
+      is_expected.to run.with_params(primary).and_return('production')
+    end
+  end
+
+  context 'returns production on non-empty pe.conf with unrelated settings' do
+    it do
+      expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => '{"foo": true}' })
+      is_expected.to run.with_params(primary).and_return('production')
+    end
+  end
+  context 'returns environment from pe.conf when set twice correctly' do
+    it do
+      pe = '{"pe_install::install::classification::pe_node_group_environment": "foo", "puppet_enterprise::master::recover_configuration::pe_environment": "foo"}'
+      expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => pe })
+      is_expected.to run.with_params(primary).and_return('foo')
+    end
+  end
+  context 'fails when both PE options are different' do
+    it do
+      pe = '{"pe_install::install::classification::pe_node_group_environment": "foo", "puppet_enterprise::master::recover_configuration::pe_environment": "bar"}'
+      expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => pe })
+      is_expected.to run.with_params(primary).and_raise_error(Puppet::PreformattedError,
+%r{pe_install::install::classification::pe_node_group_environment and puppet_enterprise::master::recover_configuration::pe_environment})
+    end
+  end
+end

spec/plans/convert_spec.rb

@@ -19,7 +19,8 @@
     allow_apply
 
     expect_task('peadm::cert_data').return_for_targets('primary' => trustedjson).be_called_times(2)
-    expect_task('peadm::read_file').always_return({ 'content' => '2021.7.9' })
+    expect_task('peadm::read_file').with_params('path' => '/opt/puppetlabs/server/pe_version').always_return({ 'content' => '2021.7.9' })
+    expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => '{}' })
     expect_task('peadm::get_group_rules').return_for_targets('primary' => { '_output' => '{"rules": []}' })
     expect_task('peadm::node_group_unpin').with_targets('primary').with_params({ 'node_certnames' => ['pe_compiler_legacy'], 'group_name' => 'PE Master' })
     expect_task('peadm::check_legacy_compilers').with_targets('primary').with_params({ 'legacy_compilers' => 'pe_compiler_legacy' }).return_for_targets('primary' => { '_output' => '' })

spec/plans/upgrade_spec.rb

@@ -37,6 +37,7 @@ def allow_standard_non_returning_calls
 
     expect_task('peadm::cert_data').return_for_targets('primary' => trusted_primary).be_called_times(1)
     expect_task('peadm::check_pe_master_rules').always_return(pe_rule_check)
+    expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => '{}' })
 
     expect(run_plan('peadm::upgrade',
                     'primary_host' => 'primary',
@@ -50,7 +51,7 @@ def allow_standard_non_returning_calls
     expect_task('peadm::read_file')
       .with_params('path' => '/opt/puppetlabs/server/pe_build')
       .always_return({ 'content' => '2021.7.3' })
-
+    expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => '{}' })
     expect_task('peadm::cert_data').return_for_targets('primary' => trusted_primary,
                                                        'compiler' => trusted_compiler).be_called_times(1)
     expect_task('peadm::check_pe_master_rules').always_return(pe_rule_check).be_called_times(1)
@@ -109,7 +110,7 @@ def allow_standard_non_returning_calls
     it 'updates pe.conf if r10k_known_hosts is set' do
       expect_task('peadm::read_file')
         .with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf')
-        .always_return({ 'content' => <<~PECONF })
+        .always_return({ 'content' => <<~PECONF }).be_called_times(2)
           # spec pe.conf
           "puppet_enterprise::puppet_master_host": "%{::trusted.certname}"
         PECONF
@@ -132,6 +133,8 @@ def allow_standard_non_returning_calls
       expect_out_message.with_params(r10k_warning)
       expect_task('peadm::check_pe_master_rules').always_return(pe_rule_check)
 
+      expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => '{}' })
+
       expect(run_plan('peadm::upgrade',
                        'primary_host'     => 'primary',
                        'version'          => '2023.3.0',
@@ -145,6 +148,8 @@ def allow_standard_non_returning_calls
         expect_out_message.with_params(r10k_warning).not_be_called
         expect_task('peadm::check_pe_master_rules').always_return(pe_rule_check)
 
+        expect_task('peadm::read_file').with_params('path' => '/etc/puppetlabs/enterprise/conf.d/pe.conf').always_return({ 'content' => '{}' })
+
         expect(run_plan('peadm::upgrade',
                          'primary_host'     => 'primary',
                          'version'          => '2023.4.0',