puppet catalog download produces 500 error on server

[nabertrand]

Describe the Bug

The puppet catalog download command produces a 500 error on server using the default options:

# puppet catalog download
Notice: Requesting catalog from puppetserver:8140 (xxx.xxx.xxx.xxx)
Error: Could not call 'find' on 'catalog': Error 500 on SERVER: Server Error: Could not intern from json: undefined method `[]' for nil:NilClass
Error: Could not call 'find' on 'catalog': Error 500 on SERVER: Server Error: Could not intern from json: undefined method `[]' for nil:NilClass
Error: Try 'puppet help catalog download' for usage

Expected Behavior

A catalog is downloaded

Steps to Reproduce

Steps to reproduce the behavior:

1. Run puppet catalog download

Environment

• Version puppet-agent-7.29.1-1.el8.x86_64 / puppetserver puppetserver-7.14.0-1.el8.noarch
• Platform Rocky 8 x86_64

Additional Context

The catalog rest terminus expects a facts_for_catalog option to be present in the request:

puppet/lib/puppet/indirector/catalog/rest.rb

Line 30 in 1d188e1

facts: request.options[:facts_for_catalog],

But the face provides no options by default:

puppet/lib/puppet/face/catalog.rb

Line 140 in 1d188e1

catalog = Puppet::Face[:catalog, '0.0.1'].find(Puppet[:certname])

From the API example provided in the puppet catalog download documentation, it seems like the catalog rest terminus supported providing no facts at some point, which would presumably use facts from PuppetDB instead:

puppet/lib/puppet/face/catalog.rb

Lines 130 to 132 in 1d188e1

	Puppet::Face[:plugin, '0.0.1'].download
	Puppet::Face[:facts, '0.0.1'].upload
	Puppet::Face[:catalog, '0.0.1'].download

However, the command now fails when the facts_for_catalog option is nil as shown in the error above. Either the catalog rest terminus should allow providing no facts:

diff --git a/lib/puppet/http/service/compiler.rb b/lib/puppet/http/service/compiler.rb
index 1ead5248e3..9b8731cc86 100644
--- a/lib/puppet/http/service/compiler.rb
+++ b/lib/puppet/http/service/compiler.rb
@@ -79,7 +79,7 @@ class Puppet::HTTP::Service::Compiler < Puppet::HTTP::Service
   #   the server
   #
   # @api public
-  def post_catalog(name, facts:, environment:, configured_environment: nil, check_environment: false, transaction_uuid: nil, job_uuid: nil, static_catalog: true, checksum_type: Puppet[:supported_checksum_types])
+  def post_catalog(name, facts: nil, environment:, configured_environment: nil, check_environment: false, transaction_uuid: nil, job_uuid: nil, static_catalog: true, checksum_type: Puppet[:supported_checksum_types])
     if Puppet[:preferred_serialization_format] == "pson"
       formatter = Puppet::Network::FormatHandler.format_for(:pson)
       # must use 'pson' instead of 'text/pson'
@@ -93,8 +93,6 @@ class Puppet::HTTP::Service::Compiler < Puppet::HTTP::Service
 
     # query parameters are sent in the POST request body
     body = {
-      facts_format: facts_format,
-      facts: Puppet::Util.uri_query_encode(facts_as_string),
       environment: environment,
       configured_environment: configured_environment || environment,
       check_environment: !!check_environment,
@@ -102,7 +100,12 @@ class Puppet::HTTP::Service::Compiler < Puppet::HTTP::Service
       job_uuid: job_uuid,
       static_catalog: static_catalog,
       checksum_type: checksum_type.join('.')
-    }.map do |key, value|
+    }
+    unless facts.nil?
+      body[:facts_format] = facts_format
+      body[:facts] = Puppet::Util.uri_query_encode(facts_as_string)
+    end
+    body = body.map do |key, value|
       "#{key}=#{Puppet::Util.uri_query_encode(value.to_s)}"
     end.join("&")

Or the puppet catalog download face should be modified to include facts in the request, e.g.

diff --git a/lib/puppet/face/catalog.rb b/lib/puppet/face/catalog.rb
index 3a7ea1a975..9ab47bcdf2 100644
--- a/lib/puppet/face/catalog.rb
+++ b/lib/puppet/face/catalog.rb
@@ -137,7 +137,8 @@ Puppet::Indirector::Face.define(:catalog, '0.0.1') do
       Puppet::Resource::Catalog.indirection.cache_class = nil
       catalog = nil
       retrieval_duration = thinmark do
-        catalog = Puppet::Face[:catalog, '0.0.1'].find(Puppet[:certname])
+        facts = Puppet::Face[:facts, '0.0.1'].find(Puppet[:certname])
+        catalog = Puppet::Face[:catalog, '0.0.1'].find(Puppet[:certname], { extra: { facts_for_catalog: facts } })
       end
       catalog.retrieval_duration = retrieval_duration
       catalog.write_class_file

[joshcooper]

@nabertrand has this ever worked correctly? If so, could you git rebase to see when it regressed?

[nabertrand]

I had never used the puppet download catalog interface so was unsure if it ever worked. I confirmed it works and uses the facts from PuppetDB in Puppet 6.13.0 but breaks in Puppet 6.14.0. git bisect indicates it was 8429cb8 that broke this functionality.

[tvpartytonight]

Thanks for tracking down which version this used to work in @nabertrand ; we will add it to our maintenance bucket.

[github-actions]

Migrated issue to PUP-12046