From 90457e1114d087abf43620133a5c8d3eac75b4da Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 29 Mar 2018 15:01:10 +0100 Subject: [PATCH] (PDK-887) Add checks for read_only values being set or modified --- lib/puppet/resource_api.rb | 116 +++++++++--------- spec/acceptance/validation_spec.rb | 34 +++++ .../test_validation/test_validation.rb | 2 + .../lib/puppet/type/test_validation.rb | 5 + spec/puppet/resource_api_spec.rb | 59 +++++++++ 5 files changed, 160 insertions(+), 56 deletions(-) diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 5486cf7e..581e7b8d 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -142,72 +142,76 @@ def feature_support?(feature) if options.key? :default defaultto options[:default] end + end - type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type]) - validate do |value| - return true if type.instance?(value) + type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type]) + validate do |value| + if options[:behaviour] == :read_only + raise Puppet::ResourceError, "Attempting to set `#{name}` read_only attribute value to `#{value}`" + end + + return true if type.instance?(value) - if value.is_a? String - # when running under `puppet resource`, we need to try to coerce from strings to the real type - case value - when %r{^-?\d+$}, Puppet::Pops::Patterns::NUMERIC - value = Puppet::Pops::Utils.to_n(value) - when %r{\Atrue|false\Z} - value = value == 'true' - end - return true if type.instance?(value) - - inferred_type = Puppet::Pops::Types::TypeCalculator.infer_set(value) - error_msg = Puppet::Pops::Types::TypeMismatchDescriber.new.describe_mismatch("#{definition[:name]}.#{name}", type, inferred_type) - raise Puppet::ResourceError, error_msg + if value.is_a? String + # when running under `puppet resource`, we need to try to coerce from strings to the real type + case value + when %r{^-?\d+$}, Puppet::Pops::Patterns::NUMERIC + value = Puppet::Pops::Utils.to_n(value) + when %r{\Atrue|false\Z} + value = value == 'true' end - end + return true if type.instance?(value) - if type.instance_of? Puppet::Pops::Types::POptionalType - type = type.type + inferred_type = Puppet::Pops::Types::TypeCalculator.infer_set(value) + error_msg = Puppet::Pops::Types::TypeMismatchDescriber.new.describe_mismatch("#{definition[:name]}.#{name}", type, inferred_type) + raise Puppet::ResourceError, error_msg end + end - # provide better handling of the standard types - case type - when Puppet::Pops::Types::PStringType - # require any string value - Puppet::ResourceApi.def_newvalues(self, param_or_property, %r{}) - # rubocop:disable Lint/BooleanSymbol - when Puppet::Pops::Types::PBooleanType - Puppet::ResourceApi.def_newvalues(self, param_or_property, 'true', 'false') - aliasvalue true, 'true' - aliasvalue false, 'false' - aliasvalue :true, 'true' - aliasvalue :false, 'false' - - munge do |v| - case v - when 'true', :true - true - when 'false', :false - false - else - v - end - end - # rubocop:enable Lint/BooleanSymbol - when Puppet::Pops::Types::PIntegerType - Puppet::ResourceApi.def_newvalues(self, param_or_property, %r{^-?\d+$}) - munge do |v| - Puppet::Pops::Utils.to_n(v) - end - when Puppet::Pops::Types::PFloatType, Puppet::Pops::Types::PNumericType - Puppet::ResourceApi.def_newvalues(self, param_or_property, Puppet::Pops::Patterns::NUMERIC) - munge do |v| - Puppet::Pops::Utils.to_n(v) + if type.instance_of? Puppet::Pops::Types::POptionalType + type = type.type + end + + # provide better handling of the standard types + case type + when Puppet::Pops::Types::PStringType + # require any string value + Puppet::ResourceApi.def_newvalues(self, param_or_property, %r{}) + # rubocop:disable Lint/BooleanSymbol + when Puppet::Pops::Types::PBooleanType + Puppet::ResourceApi.def_newvalues(self, param_or_property, 'true', 'false') + aliasvalue true, 'true' + aliasvalue false, 'false' + aliasvalue :true, 'true' + aliasvalue :false, 'false' + + munge do |v| + case v + when 'true', :true + true + when 'false', :false + false + else + v end end - - case options[:type] - when 'Enum[present, absent]' - Puppet::ResourceApi.def_newvalues(self, param_or_property, :absent, :present) + # rubocop:enable Lint/BooleanSymbol + when Puppet::Pops::Types::PIntegerType + Puppet::ResourceApi.def_newvalues(self, param_or_property, %r{^-?\d+$}) + munge do |v| + Puppet::Pops::Utils.to_n(v) + end + when Puppet::Pops::Types::PFloatType, Puppet::Pops::Types::PNumericType + Puppet::ResourceApi.def_newvalues(self, param_or_property, Puppet::Pops::Patterns::NUMERIC) + munge do |v| + Puppet::Pops::Utils.to_n(v) end end + + case options[:type] + when 'Enum[present, absent]' + Puppet::ResourceApi.def_newvalues(self, param_or_property, :absent, :present) + end end end diff --git a/spec/acceptance/validation_spec.rb b/spec/acceptance/validation_spec.rb index 19fd63b4..64447ff3 100644 --- a/spec/acceptance/validation_spec.rb +++ b/spec/acceptance/validation_spec.rb @@ -51,6 +51,23 @@ expect(output.strip).to match %r{Test_validation\[bye\]: test_validation.param expect.* an Integer value, got String} expect(status.exitstatus).to eq 1 end + + context 'when passing a value to a read_only property' do + context 'with an existing resource' do + it 'throws' do + output, status = Open3.capture2e("puppet resource #{common_args} test_validation foo ensure=present prop_ro=3") + expect(output.strip).to match %r{Test_validation\[foo\]: Attempting to set `prop_ro` read_only attribute value to `3`} + expect(status.exitstatus).to eq 1 + end + end + context 'with a resource which should be absent' do + it 'throws' do + output, status = Open3.capture2e("puppet resource #{common_args} test_validation foo ensure=absent prop_ro=3") + expect(output.strip).to match %r{Test_validation\[foo\]: Attempting to set `prop_ro` read_only attribute value to `3`} + expect(status.exitstatus).to eq 1 + end + end + end end describe 'using `puppet apply`' do @@ -91,5 +108,22 @@ expect(output.strip).to match %r{Test_validation\[gone\]: test_validation.param expect.* an Integer value, got String}i expect(status.exitstatus).to eq 1 end + + context 'when passing a value to a read_only property' do + context 'with an existing resource' do + it 'throws' do + output, status = Open3.capture2e("puppet apply #{common_args} -e \"test_validation{ foo: ensure => present, param => 3, prop_ro =>4 }\"") + expect(output.strip).to match %r{Test_validation\[foo\]: Attempting to set `prop_ro` read_only attribute value to `4`} + expect(status.exitstatus).to eq 1 + end + end + context 'with a resource which should be absent' do + it 'throws' do + output, status = Open3.capture2e("puppet apply #{common_args} -e \"test_validation{ foo: ensure => absent, param => 3, prop_ro =>4 }\"") + expect(output.strip).to match %r{Test_validation\[foo\]: Attempting to set `prop_ro` read_only attribute value to `4`} + expect(status.exitstatus).to eq 1 + end + end + end end end diff --git a/spec/fixtures/test_module/lib/puppet/provider/test_validation/test_validation.rb b/spec/fixtures/test_module/lib/puppet/provider/test_validation/test_validation.rb index f451ff01..993a7aac 100644 --- a/spec/fixtures/test_module/lib/puppet/provider/test_validation/test_validation.rb +++ b/spec/fixtures/test_module/lib/puppet/provider/test_validation/test_validation.rb @@ -9,11 +9,13 @@ def get(_context) name: 'foo', ensure: :present, prop: 2, + prop_ro: 8, }, { name: 'bar', ensure: :present, prop: 3, + prop_ro: 9, }, ] end diff --git a/spec/fixtures/test_module/lib/puppet/type/test_validation.rb b/spec/fixtures/test_module/lib/puppet/type/test_validation.rb index 757049ce..fff9bdc2 100644 --- a/spec/fixtures/test_module/lib/puppet/type/test_validation.rb +++ b/spec/fixtures/test_module/lib/puppet/type/test_validation.rb @@ -26,5 +26,10 @@ desc: 'A mandatory parameter, that MUST be validated on deleting.', behaviour: :parameter, }, + prop_ro: { + type: 'Integer', + desc: 'A property that cannot be set by a catalog', + behaviour: :read_only + }, }, ) diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index 5c40e9d0..823257d1 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -509,6 +509,65 @@ def set(_context, _changes); end end end + context 'when registering a type with an `read_only` attribute', agent_test: true do + let(:definition) do + { + name: 'read_only_behaviour', + attributes: { + ensure: { + type: 'Enum[present, absent]', + }, + name: { + type: 'String', + behavior: :namevar, + }, + immutable: { + type: 'String', + behaviour: :read_only, + }, + }, + } + end + + it { expect { described_class.register_type(definition) }.not_to raise_error } + + describe 'the registered type' do + subject(:type) { Puppet::Type.type(:read_only_behaviour) } + + it { is_expected.not_to be_nil } + it { expect(type.parameters[0]).to eq :name } + end + + describe 'an instance of the type' do + let(:provider_class) do + Class.new do + def get(_context) + [{ name: 'foo_ro', ensure: :present, immutable: 'physics' }] + end + + def set(_context, _changes); end + end + end + + before(:each) do + stub_const('Puppet::Provider::ReadOnlyBehaviour', Module.new) + stub_const('Puppet::Provider::ReadOnlyBehaviour::ReadOnlyBehaviour', provider_class) + end + + context 'when a manifest wants to set the value of a read_only attribute' do + let(:instance) { Puppet::Type.type(:read_only_behaviour).new(name: 'new_ro', ensure: :present, immutable: 'new') } + + it { expect { instance.flush }.to raise_error Puppet::ResourceError, %r{Attempting to set `immutable` read_only attribute value to} } + end + + context 'when a manifest wants to change the value of a read_only attribute' do + let(:instance) { Puppet::Type.type(:read_only_behaviour).new(name: 'foo_ro', ensure: :present, immutable: 'change') } + + it { expect { instance.flush }.to raise_error Puppet::ResourceError, %r{Attempting to set `immutable` read_only attribute value to} } + end + end + end + context 'when registering a type with a malformed attributes' do context 'without a type' do let(:definition) do