From 91961bcf13db002f8b32fc5b4fafb702ad7ce5c4 Mon Sep 17 00:00:00 2001 From: Justin Stoller Date: Tue, 3 Sep 2024 15:47:56 -0700 Subject: [PATCH 1/5] (PUP-12077) Respect rich_data setting in base context Since Puppet 6.0 "datafication" has inspected the context for the value of rich_data. However, in only some code paths does the value in the context get overridden with a value taken from the settings. This means in some cases the rich_data value will always be true or always be false, regardless of how the user has configured the rich_data setting. And, in the case the simply calling `to_data_hash` on a resource the rich_data value will always be false. This updates the base_context to set rich_data to the settings value, ensuring that the default value for rich_data in the context is the value users have set. Additional changes are primarily where tests still assumed the default value of rich_data was false, with one exception - YAML serialization in the resource application will break if the internal rich_data `__pcore` values are output. This forces rich_data to be false for that code path in the resource application. Fixes GH #9470 --- lib/puppet.rb | 2 +- lib/puppet/application/resource.rb | 4 +++- spec/integration/application/apply_spec.rb | 6 ++++++ spec/unit/application/resource_spec.rb | 8 +++++--- spec/unit/resource/catalog_spec.rb | 7 ++++++- spec/unit/resource_spec.rb | 20 ++++++++++++++++++++ 6 files changed, 41 insertions(+), 6 deletions(-) diff --git a/lib/puppet.rb b/lib/puppet.rb index 484fda4c4ef..d8a0ec00114 100644 --- a/lib/puppet.rb +++ b/lib/puppet.rb @@ -237,7 +237,7 @@ def self.base_context(settings) :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 => Puppet[:rich_data] } end diff --git a/lib/puppet/application/resource.rb b/lib/puppet/application/resource.rb index 0c39a6c1aae..8a7d97bcbe3 100644 --- a/lib/puppet/application/resource.rb +++ b/lib/puppet/application/resource.rb @@ -155,7 +155,9 @@ def main if options[:to_yaml] data = resources.map do |resource| - resource.prune_parameters(:parameters_to_include => @extra_params).to_hiera_hash + Puppet.override(rich_data: false) do + resource.prune_parameters(:parameters_to_include => @extra_params).to_hiera_hash + end end.inject(:merge!) text = YAML.dump(type.downcase => data) else diff --git a/spec/integration/application/apply_spec.rb b/spec/integration/application/apply_spec.rb index 9f159dbec70..adea37ed406 100644 --- a/spec/integration/application/apply_spec.rb +++ b/spec/integration/application/apply_spec.rb @@ -447,6 +447,12 @@ def bogus() Puppet[:strict] = :warning end + around :each do |test| + Puppet.override(rich_data: false) do + test.run + end + end + it 'will notify a string that is the result of Regexp#inspect (from Runtime3xConverter)' do catalog = compile_to_catalog(execute, node) apply.command_line.args = ['--catalog', file_containing('manifest', catalog.to_json)] diff --git a/spec/unit/application/resource_spec.rb b/spec/unit/application/resource_spec.rb index 901fb4a9bb0..ecc82f60075 100644 --- a/spec/unit/application/resource_spec.rb +++ b/spec/unit/application/resource_spec.rb @@ -170,11 +170,13 @@ def string expect { @resource_app.main }.not_to raise_error end - it "should raise an error when printing yaml by default" do + it "should raise an error when printing yaml if rich_data is off" do @resource_app.options[:to_yaml] = true allow(@resource_app.command_line).to receive(:args).and_return(['stringify', 'hello', 'ensure=present', 'string=asd']) - expect { @resource_app.main }.to raise_error( Puppet::PreformattedError, - /Stringify\[hello\]\['string'\] contains a Puppet::Util::Execution::ProcessOutput value. It will be converted to the String 'test'/) + Puppet.override(rich_data: false) do + expect { @resource_app.main }.to raise_error( Puppet::PreformattedError, + /Stringify\[hello\]\['string'\] contains a Puppet::Util::Execution::ProcessOutput value. It will be converted to the String 'test'/) + end end it "should ensure all values to be printed are in the external encoding" do diff --git a/spec/unit/resource/catalog_spec.rb b/spec/unit/resource/catalog_spec.rb index 9c462f72d25..7abca7e7b58 100644 --- a/spec/unit/resource/catalog_spec.rb +++ b/spec/unit/resource/catalog_spec.rb @@ -965,10 +965,15 @@ class {'multi_param_class': context 'and rich_data is disabled' do before(:each) do - Puppet[:rich_data] = false Puppet[:strict] = :warning # do not want to stub out behavior in tests end + around(:each) do |test| + Puppet.override(rich_data: false) do + test.run + end + end + let(:catalog_w_regexp) { compile_to_catalog("notify {'foo': message => /[a-z]+/ }") } it 'should not generate rich value hash for parameter values that are not Data' do diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb index c527358e3f6..28d634991a3 100644 --- a/spec/unit/resource_spec.rb +++ b/spec/unit/resource_spec.rb @@ -906,6 +906,26 @@ def inject_and_set_defaults(resource, scope) # Note: to_stringified_spec.rb has tests for all other data types end + describe 'when serializing resources' do + require 'puppet_spec/compiler' + include PuppetSpec::Compiler + + it 'should do something' do + resource = compile_to_catalog('notify {"foo": message => Deferred("func", ["a", "b", "c"])}') + + # This should be true by default + if Puppet[:rich_data] + expect(resource.to_data_hash.class).to be(Hash) + end + + expect { + Puppet.override(rich_data: false) do + resource.to_data_hash + end + }.to raise_error(Puppet::PreformattedError) + end + end + describe "when converting from json" do before do @data = { From 3ee2564149eb82fb2e3aa5622d01c38a500d9cd5 Mon Sep 17 00:00:00 2001 From: Justin Stoller Date: Tue, 17 Sep 2024 14:39:12 -0700 Subject: [PATCH 2/5] (PUP-12077) Use stringify_rich instead of rich_data in resource application --- lib/puppet/application/resource.rb | 2 +- spec/unit/application/resource_spec.rb | 9 --------- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/lib/puppet/application/resource.rb b/lib/puppet/application/resource.rb index 8a7d97bcbe3..052290da76f 100644 --- a/lib/puppet/application/resource.rb +++ b/lib/puppet/application/resource.rb @@ -155,7 +155,7 @@ def main if options[:to_yaml] data = resources.map do |resource| - Puppet.override(rich_data: false) do + Puppet.override(stringify_rich: true) do resource.prune_parameters(:parameters_to_include => @extra_params).to_hiera_hash end end.inject(:merge!) diff --git a/spec/unit/application/resource_spec.rb b/spec/unit/application/resource_spec.rb index ecc82f60075..55a34a3480c 100644 --- a/spec/unit/application/resource_spec.rb +++ b/spec/unit/application/resource_spec.rb @@ -170,15 +170,6 @@ def string expect { @resource_app.main }.not_to raise_error end - it "should raise an error when printing yaml if rich_data is off" do - @resource_app.options[:to_yaml] = true - allow(@resource_app.command_line).to receive(:args).and_return(['stringify', 'hello', 'ensure=present', 'string=asd']) - Puppet.override(rich_data: false) do - expect { @resource_app.main }.to raise_error( Puppet::PreformattedError, - /Stringify\[hello\]\['string'\] contains a Puppet::Util::Execution::ProcessOutput value. It will be converted to the String 'test'/) - end - end - it "should ensure all values to be printed are in the external encoding" do resources = [ Puppet::Type.type(:user).new(:name => "\u2603".force_encoding(Encoding::UTF_8)).to_resource, From b06a0de6e3d314542943734b260396d57d568983 Mon Sep 17 00:00:00 2001 From: Justin Stoller Date: Tue, 17 Sep 2024 14:52:30 -0700 Subject: [PATCH 3/5] (PUP-12077) Move stringify_rich into the base_context --- lib/puppet.rb | 6 +++++- lib/puppet/resource.rb | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/puppet.rb b/lib/puppet.rb index d8a0ec00114..b717de7e130 100644 --- a/lib/puppet.rb +++ b/lib/puppet.rb @@ -237,7 +237,11 @@ def self.base_context(settings) :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 => Puppet[:rich_data] + :rich_data => Puppet[:rich_data], + # `stringify_rich` controls whether `rich_data` is stringified into a lossy format + # instead of a lossless format. Catalogs should not be stringified, though to_yaml + # and the resource application have uses for a lossy, user friendly format. + :stringify_rich => false } end diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb index 4b51cdc2980..ac3c421e73f 100644 --- a/lib/puppet/resource.rb +++ b/lib/puppet/resource.rb @@ -112,7 +112,7 @@ def to_data_hash # To get stringified parameter values the flag :stringify_rich can be set # in the puppet context. # - stringify = Puppet.lookup(:stringify_rich) { false } + stringify = Puppet.lookup(:stringify_rich) converter = stringify ? Puppet::Pops::Serialization::ToStringifiedConverter.new : nil params = {} From 9f61d16dee732a84e924d836cba442f30b48b575 Mon Sep 17 00:00:00 2001 From: Justin Stoller Date: Tue, 17 Sep 2024 14:57:53 -0700 Subject: [PATCH 4/5] (PUP-12077) Raise stringify_rich higher in the resource application code --- lib/puppet/application/resource.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/puppet/application/resource.rb b/lib/puppet/application/resource.rb index 052290da76f..03361c09d1d 100644 --- a/lib/puppet/application/resource.rb +++ b/lib/puppet/application/resource.rb @@ -146,7 +146,11 @@ def main # If the specified environment does not exist locally, fall back to the default (production) environment env = Puppet.lookup(:environments).get(Puppet[:environment]) || create_default_environment - Puppet.override(:current_environment => env, :loaders => Puppet::Pops::Loaders.new(env)) do + Puppet.override( + current_environment: env, + loaders: Puppet::Pops::Loaders.new(env), + stringify_rich: true + ) do type, name, params = parse_args(command_line.args) raise _("Editing with Yaml output is not supported") if options[:edit] and options[:to_yaml] @@ -155,9 +159,7 @@ def main if options[:to_yaml] data = resources.map do |resource| - Puppet.override(stringify_rich: true) do - resource.prune_parameters(:parameters_to_include => @extra_params).to_hiera_hash - end + resource.prune_parameters(:parameters_to_include => @extra_params).to_hiera_hash end.inject(:merge!) text = YAML.dump(type.downcase => data) else From 8633e947b38191394a7788a713025bf21252af4b Mon Sep 17 00:00:00 2001 From: Justin Stoller Date: Tue, 17 Sep 2024 15:04:29 -0700 Subject: [PATCH 5/5] (PUP-12077) Clean up rich_data resource specs --- spec/unit/resource_spec.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb index 28d634991a3..bf90cd8cb2f 100644 --- a/spec/unit/resource_spec.rb +++ b/spec/unit/resource_spec.rb @@ -910,14 +910,15 @@ def inject_and_set_defaults(resource, scope) require 'puppet_spec/compiler' include PuppetSpec::Compiler - it 'should do something' do + it 'serializes rich data' do resource = compile_to_catalog('notify {"foo": message => Deferred("func", ["a", "b", "c"])}') - # This should be true by default - if Puppet[:rich_data] - expect(resource.to_data_hash.class).to be(Hash) - end + # This assume rich_data is true by default + expect(resource.to_data_hash.class).to be(Hash) + end + it 'raises when rich data is disabled' do + resource = compile_to_catalog('notify {"foo": message => Deferred("func", ["a", "b", "c"])}') expect { Puppet.override(rich_data: false) do resource.to_data_hash