puppetlabs / puppet

Server automation framework and application
https://puppet.com/open-source/#osp
Apache License 2.0
7.46k stars 2.19k forks source link

Rich Data setting not respected when converting catalogs to data #9470

Closed justinstoller closed 2 months ago

justinstoller commented 2 months ago

Describe the Bug

When calling to_data_hash on a catalog (and consequently on resources within that catalog) we do not respect the rich_data setting, instead of we lookup the value in the Context here, which defaults to false here.

Typically, catalogs go through our networking code, which will correctly chooses json vs application/vnd.puppet.rich+json mime types based on the request and local configuration (see here & here). So this is not a problem during most catalog compilations.

However, for CD4PE we go through an alternate compilation process which does not go through the networking serialization code, and expects the setting value rich_data to be respected. This has become an issue as Puppet 8 sets strict mode to error, and rich data types (like Deferreds) are becoming more popular.

Expected Behavior

The environment.conf / puppet.conf setting respected when calling Catalog.to_data_hash()

Steps to Reproduce

Do a CD4PE Impact Analysis on an environment with Deferred resources.

With a Puppet Server installed (I used the latest PE release): Update the file /etc/puppetlabs/code/environments/production/manifests/site.pp to have a default node group of

node default {
  $message = Deferred('inline_epp', ['This value is <%= $val %>', {'val' => 'deferred'}])
  notify { "deferred test":
    message => $message,
  }
}

And then request a CD4PE catalog with:

curl -k \
  --cacert /etc/puppetlabs/puppet/ssl/certs/ca.pem \
  --cert /etc/puppetlabs/puppet/ssl/certs/$(hostname -f).pem \
  --key /etc/puppetlabs/puppet/ssl/private_keys/$(hostname -f).pem \
  -H 'content-type: application/json' \
  -d "{\"certname\":\"$(hostname -f)\", \"environment\":\"production\", \"persistence\": { \"facts\": true, \"catalog\": true }}" \
  https://$(hostname -f):8140/puppet/v4/catalog

I also have a unit test reproducer in Puppet Server here: https://github.com/puppetlabs/puppetserver/pull/2875

Environment

Additional Context

I would expect something like this to fix the issue:

diff --git a/lib/puppet.rb b/lib/puppet.rb
index 484fda4c4e..e80248b86c 100644
--- a/lib/puppet.rb
+++ b/lib/puppet.rb
@@ -237,7 +237,7 @@ module Puppet
       :ssl_context => proc { Puppet.runtime[:http].default_ssl_context },
       :http_session => proc { Puppet.runtime[:http].create_session },
       :plugins => proc { Puppet::Plugins::Configuration.load_plugins },
-      :rich_data => false
+      :rich_data => proc { Puppet.lookup(:current_environment).rich_data? }
     }
   end

But I have no idea if that would create more subtle problems than it solves.

github-actions[bot] commented 2 months ago

Migrated issue to PUP-12077