Add puppetcore macos support #769

cthorn42 · 2025-04-01T21:30:53Z

This is a work in progress, currently based on Josh's WIP: #766

cthorn42 · 2025-04-01T21:34:11Z

manifests/osfamily/darwin.pp

@@ -20,12 +20,20 @@
    } else {
      $source = "puppet:///pe_packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"
    }
-  } else {
+  } elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ {
+    if $puppet_agent::prepare::package_version =~ /^\d+\.\d+\.\d+\.\d+\.g([a-f0-9]+)+$/ {


Is there a better way to determine if we need to set dev=true parameter?

This worked for me, splitting the version and checking for strictly more than 3 components:

$dev = count(split($puppet_agent::prepare::package_version, '\.')) > 3

cthorn42 · 2025-04-01T21:38:26Z

manifests/prepare/package.pp

+      onlyif  => "test -f '${response_file}'",
+      logoutput => true,
+      require => Exec['Download Puppet Agent for Darwin'],
+    }


This is what the output looks like for setting up the netrc file, reading the output file and then executing the install script:

Started on jovial-duck.delivery.puppetlabs.net... Finished on jovial-duck.delivery.puppetlabs.net: Notice: /Stage[main]/Puppet_agent::Prepare::Package/File[/var/folders/zz/zyxvpxvq6csfxvn_n0000000000000/T/.netrc]/ensure: defined content as '{sha256}4f3d0ec5c7fa14041867e7009e7924c13c15c54cc2fbfc52362708a88964bf86' Notice: /Stage[main]/Puppet_agent::Prepare::Package/Exec[Remove .netrc file]/returns: executed successfully Notice: /Stage[main]/Puppet_agent::Prepare::Package/Exec[Read HTTP Response Code]/returns: 200 Notice: /Stage[main]/Puppet_agent::Prepare::Package/Exec[Read HTTP Response Code]/returns: executed successfully Notice: /Stage[main]/Puppet_agent::Install::Darwin/Exec[osx_install script]/returns: executed successfully changed: 4, failed: 0, unchanged: 26 skipped: 0, noop: 0 Finished: apply catalog with 0 failures in 15.54 sec

For now while working on this I'm mainly looking to get quick feedback on the curl HTTP response. But this is only a work in progress, and I do plan on spending time to improve this.

joshcooper · 2025-04-02T16:21:31Z

manifests/prepare/package.pp

+      mode    => '0600',
+    }
+
+    $curl_command = "curl -1 -sL --netrc-file '${netrc_file}' -w '%{http_code}' -o '${local_package_file_path}' '${source}' > '${response_file}'"


suggest adding --fail so it returns non-zero exit on HTTP error

joshcooper · 2025-04-02T16:22:22Z

manifests/prepare/package.pp

+      path    => ['/usr/bin', '/bin'],
+      onlyif  => "test -f '${netrc_file}'",
+      require => Exec['Download Puppet Agent for Darwin'],
+    }


may produce a "changed" event each time the agent runs?

Isn't that the kind of issue https://forge.puppet.com/modules/puppetlabs/transition/readme exists for?

manifests/prepare/package.pp

tasks/install_powershell.json

bastelfreak · 2025-04-02T16:42:15Z

manifests/prepare/package.pp

+    file { $netrc_file:
+      ensure  => file,
+      content => "machine artifacts-puppetcore.puppet.com\nlogin ${download_username}\npassword ${download_password}\n",
+      mode    => '0600',


please add show_diff => false,, to ensure password won't be leaked

👍 Good catch.

When using the puppetcore collection on Windows, if we detect the installed version does not match, then upgrade the MSI. Due to a puppet bug, we cannot pass credentials in the `source` parameter. And `curl.exe` is not present in our puppet-agent packages. So use powershell to download. Co-authored-by: Kevin <[email protected]>

bastelfreak · 2025-04-22T19:03:02Z

manifests/osfamily/darwin.pp

@@ -8,7 +8,7 @@
    $productversion_array = split($facts['os']['macosx']['version']['major'], '[.]')
    $productversion_major = $productversion_array[0]
  }
-
+  $destination_name = undef


Why do you set the variable here?

The variable needs to get set for the scenario if puppetcore isn't being used. The way I had it set here is a work in progress, I'm about to push up changes that reflect how Josh fixed this with his Windows commit. I think it will be a bit cleaner in what is going on. Thanks for the comment I had forgotten to update this.

bastelfreak · 2025-04-22T20:46:53Z

manifests/osfamily/darwin.pp

@@ -20,12 +19,25 @@
    } else {
      $source = "puppet:///pe_packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"
    }
+  } elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ {


Suggested change

} elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ {

} elsif $puppet_agent::collection =~ /core/ {

$puppet_agent::collection cannot be undef, so we don't need to check if it's undef

bastelfreak · 2025-04-22T20:48:47Z

manifests/osfamily/darwin.pp

@@ -8,7 +8,6 @@
    $productversion_array = split($facts['os']['macosx']['version']['major'], '[.]')
    $productversion_major = $productversion_array[0]
  }
-
  if $puppet_agent::absolute_source {


I think this whole block becomes a bit more readable and shorter, if you do something like:

$source = if $puppet_agent::absolute_source { ...

Which enables you to get rid of all the following $source = in the logic block.

bastelfreak · 2025-04-22T20:49:44Z

manifests/prepare/package.pp

@@ -56,6 +56,45 @@
      creates => $local_package_file_path,
      require => File[$puppet_agent::params::local_packages_dir],
    }
+  } elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ and $facts['os']['family'] =~ /Darwin/ {


Suggested change

} elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ and $facts['os']['family'] =~ /Darwin/ {

} elsif $puppet_agent::collection =~ /core/ and $facts['os']['family'] =~ /Darwin/ {

This updates the download of puppet-agent when puppetcore packages are used. The new 'puppetcore7' and 'puppetcore8' collections when used for MacOS will now download puppetcore packages. Due to a bug in Puppet for now we're going to depend on Curl to download the package.

joshcooper · 2025-04-24T17:32:16Z

manifests/osfamily/darwin.pp

  } elsif ($puppet_agent::is_pe and (!$puppet_agent::use_alternate_sources)) {
    $pe_server_version = pe_build_version()
    if $puppet_agent::alternate_pe_source {
-      $source = "${puppet_agent::alternate_pe_source}/packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"
+      "${puppet_agent::alternate_pe_source}/packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${productversion_major}.dmg"


We lost the inner '$'

Suggested change

"${puppet_agent::alternate_pe_source}/packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${productversion_major}.dmg"

"${puppet_agent::alternate_pe_source}/packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"

But that's not required, is it? We already have $ before the {

joshcooper · 2025-04-24T17:33:00Z

manifests/osfamily/darwin.pp

    } elsif $puppet_agent::source {
-      $source = "${puppet_agent::source}/packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"
+      "${puppet_agent::source}/packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${productversion_major}.dmg"


Same

Suggested change

"${puppet_agent::source}/packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${productversion_major}.dmg"

"${puppet_agent::source}/packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"

joshcooper · 2025-04-24T17:34:07Z

manifests/osfamily/darwin.pp

    }
  } else {
-    $source = "${puppet_agent::mac_source}/mac/${puppet_agent::collection}/${productversion_major}/${puppet_agent::arch}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"
+    "${puppet_agent::mac_source}/mac/${puppet_agent::collection}/${productversion_major}/${puppet_agent::arch}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${productversion_major}.dmg"


Same here, also indentation is off

Suggested change

"${puppet_agent::mac_source}/mac/${puppet_agent::collection}/${productversion_major}/${puppet_agent::arch}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${productversion_major}.dmg"

"${puppet_agent::mac_source}/mac/${puppet_agent::collection}/${productversion_major}/${puppet_agent::arch}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"

joshcooper · 2025-04-24T17:35:54Z

manifests/osfamily/darwin.pp

    } else {
-      $source = "puppet:///pe_packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"


I think we want to keep puppet:///pe_packages/${pe_server_version}/.... as a valid source.

joshcooper · 2025-04-24T17:47:26Z

tasks/install_shell.sh

+    if [[ "$collection" =~ core ]]; then
+      if [ -z "$password" ]; then
+      echo "A password parameter is required to install"
+      exit 1


Since we check for an empty password earlier, is the if [ -z "$password ] check needed?

joshcooper · 2025-04-24T17:56:10Z

tasks/install_shell.sh

@@ -431,6 +451,12 @@ do_curl() {
    unable_to_retrieve_package
  fi

+  grep "HTTP/2 401" $tmp_stderr 2>&1 >/dev/null


I don't know that we can safely assume the server replied with HTTP 2. Probably better to just check for:

Suggested change

grep "HTTP/2 401" $tmp_stderr 2>&1 >/dev/null

grep "401 Unauthorized" $tmp_stderr 2>&1 >/dev/null

cthorn42 added the don't merge label Apr 1, 2025

cthorn42 requested review from bastelfreak and a team as code owners April 1, 2025 21:30

cthorn42 commented Apr 1, 2025

View reviewed changes

cthorn42 force-pushed the add_puppetcore_macos_support branch from 191a2cc to 4a3c3e5 Compare April 2, 2025 15:46

joshcooper reviewed Apr 2, 2025

View reviewed changes