From 7978f89b25f211f65ca113d49f1c7dde2934a810 Mon Sep 17 00:00:00 2001 From: Michael T Lombardi Date: Tue, 20 Apr 2021 22:03:56 -0500 Subject: [PATCH] (GH-145) Add insync? and invoke_test_method to dsc provider Prior to this commit the DSC base provider had no way to handle resource level insync checks and could only rely on the default property-by-property insync checks in Puppet::Property. This fell short in cases where such a check could not work, such as in cases where the DSC Resource does not return the expected values in Get or had a mismatch between its API spec and the implementation. This commit: 1. Adds a class variable, `@@cached_test_results` 2. Adds an accessor method, `cached_test_results`, to retrieve the cache 3. Adds the `insync?` method which can be called by the resource API; for more info, see puppetlabs/puppet-resource_api#225. This method only handles custom insync validation for a resource which has the `validation_mode` specified as `resource` and otherwise defaults to the existing behavior. If the `validation_mode` *is* specified as `resource`, it instead first checks the test result cache for whether or not the resource is in the desired state and, if the resource has not already been cached, calls `invoke_test_method` and returns that result. 4. Adds the `invoke_test_method` method which passes the `should` hash of the resource to `Invoke-DscResource` similarly to the extant `invoke_set_method`; unlike with `set`, however, it caches the name and whether or not the resource is in the desired state and then it returns `true` if in the desired state and otherwise `false` with a descriptive change message. In cases where something goes wrong, it returns `nil` and causes Puppet to fall back on the default property- by-property comparison logic. This commit includes new unit tests for the additional methods. This commit makes progress towards but does not alone implement puppetlabs/Puppet.Dsc#145. --- .../dsc_base_provider/dsc_base_provider.rb | 48 ++++++++++ .../dsc_base_provider_spec.rb | 94 ++++++++++++++++++- 2 files changed, 141 insertions(+), 1 deletion(-) diff --git a/lib/puppet/provider/dsc_base_provider/dsc_base_provider.rb b/lib/puppet/provider/dsc_base_provider/dsc_base_provider.rb index fd3bca45..41b24d07 100644 --- a/lib/puppet/provider/dsc_base_provider/dsc_base_provider.rb +++ b/lib/puppet/provider/dsc_base_provider/dsc_base_provider.rb @@ -13,10 +13,15 @@ class Puppet::Provider::DscBaseProvider def initialize @@cached_canonicalized_resource ||= [] @@cached_query_results ||= [] + @@cached_test_results ||= [] @@logon_failures ||= [] super end + def cached_test_results + @@cached_test_results + end + # Look through a cache to retrieve the hashes specified, if they have been cached. # Does so by seeing if each of the specified hashes is a subset of any of the hashes # in the cache, so {foo: 1, bar: 2} would return if {foo: 1} was the search hash. @@ -270,6 +275,24 @@ def invoke_dsc_resource(context, name_hash, props, method) data end + # Determine if the DSC Resource is in the desired state, invoking the `Test` method unless it's + # already been run for the resource, in which case reuse the result instead of checking for each + # property. This behavior is only triggered if the validation_mode is set to resource; by default + # it is set to property and uses the default property comparison logic in Puppet::Property. + # + # @param context [Object] the Puppet runtime context to operate in and send feedback to + # @param name [String] the name of the resource being tested + # @param is_hash [Hash] the current state of the resource on the system + # @param should_hash [Hash] the desired state of the resource per the manifest + # @return [Boolean, Void] returns true/false if the resource is/isn't in the desired state and + # the validation mode is set to resource, otherwise nil. + def insync?(context, name, _property_name, _is_hash, should_hash) + return nil if should_hash[:validation_mode] != 'resource' + + prior_result = fetch_cached_hashes(@@cached_test_results, [name]) + prior_result.empty? ? invoke_test_method(context, name, should_hash) : prior_result.first[:in_desired_state] + end + # Invokes the `Get` method, passing the name_hash as the properties to use with `Invoke-DscResource` # The PowerShell script returns a JSON representation of the DSC Resource's CIM Instance munged as # best it can be for Ruby. Once that JSON is parsed into a hash this method further munges it to @@ -357,6 +380,31 @@ def invoke_set_method(context, name, should) # notify_reboot_pending if data['rebootrequired'] == true end + # Invokes the `Test` method, passing the should hash as the properties to use with `Invoke-DscResource` + # The PowerShell script returns a JSON hash with key-value pairs indicating whether or not the resource + # is in the desired state and any error messages captured. + # + # @param context [Object] the Puppet runtime context to operate in and send feedback to + # @param should [Hash] the desired state represented definition to pass as properties to Invoke-DscResource + # @return [Boolean] returns true if the resource is in the desired state, otherwise false + def invoke_test_method(context, name, should) + context.debug("Relying on DSC Test method for validating if '#{name}' is in the desired state") + context.debug("Invoking Test Method for '#{name}' with #{should.inspect}") + + test_props = should.select { |k, _v| k.to_s =~ /^dsc_/ } + data = invoke_dsc_resource(context, name, test_props, 'test') + # Something went wrong with Invoke-DscResource; fall back on property state comparisons + return nil if data.nil? + + in_desired_state = data['indesiredstate'] + @@cached_test_results << name.merge({ in_desired_state: in_desired_state }) + + return in_desired_state if in_desired_state + + change_log = 'DSC reported that this resource is not in the desired state; treating all properties as out-of-sync' + [in_desired_state, change_log] + end + # Converts a Puppet resource hash into a hash with the information needed to call Invoke-DscResource, # including the desired state, the path to the PowerShell module containing the resources, the invoke # method, and metadata about the DSC Resource and Puppet Type. diff --git a/spec/unit/puppet/provider/dsc_base_provider/dsc_base_provider_spec.rb b/spec/unit/puppet/provider/dsc_base_provider/dsc_base_provider_spec.rb index ebc35d0f..098ee905 100644 --- a/spec/unit/puppet/provider/dsc_base_provider/dsc_base_provider_spec.rb +++ b/spec/unit/puppet/provider/dsc_base_provider/dsc_base_provider_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' require 'puppet/type' require 'puppet/provider/dsc_base_provider/dsc_base_provider' +require 'json' RSpec.describe Puppet::Provider::DscBaseProvider do subject(:provider) { described_class.new } @@ -10,13 +11,13 @@ let(:context) { instance_double('Puppet::ResourceApi::PuppetContext') } let(:type) { instance_double('Puppet::ResourceApi::TypeDefinition') } let(:ps_manager) { instance_double('Pwsh::Manager') } - let(:command) { 'command' } let(:execute_response) { { stdout: nil, stderr: nil, exitcode: 0 } } # Reset the caches after each run after(:each) do described_class.class_variable_set(:@@cached_canonicalized_resource, nil) # rubocop:disable Style/ClassVars described_class.class_variable_set(:@@cached_query_results, nil) # rubocop:disable Style/ClassVars + described_class.class_variable_set(:@@cached_test_results, nil) # rubocop:disable Style/ClassVars described_class.class_variable_set(:@@logon_failures, nil) # rubocop:disable Style/ClassVars end @@ -32,11 +33,23 @@ it 'initializes the cached_query_results class variable' do expect(described_class.class_variable_get(:@@cached_query_results)).to eq([]) end + it 'initializes the cached_test_results class variable' do + expect(described_class.class_variable_get(:@@cached_test_results)).to eq([]) + end it 'initializes the logon_failures class variable' do expect(described_class.class_variable_get(:@@logon_failures)).to eq([]) end end + context '.cached_test_results' do + let(:cache_value) { %w[foo bar] } + + it 'returns the value of the @@cached_test_results class variable' do + described_class.class_variable_set(:@@cached_test_results, cache_value) # rubocop:disable Style/ClassVars + expect(provider.cached_test_results).to eq(cache_value) + end + end + context '.fetch_cached_hashes' do let(:cached_hashes) { [{ foo: 1, bar: 2, baz: 3 }, { foo: 4, bar: 5, baz: 6 }] } let(:findable_full_hash) { { foo: 1, bar: 2, baz: 3 } } @@ -354,6 +367,35 @@ end end + context '.insync?' do + let(:name) { { name: 'foo' } } + let(:attribute_name) { :foo } + let(:is_hash) { { name: 'foo', foo: 1 } } + let(:cached_test_result) { [{ name: 'foo', in_desired_state: true }] } + let(:should_hash_validate_by_property) { { name: 'foo', foo: 1, validation_mode: 'property' } } + let(:should_hash_validate_by_resource) { { name: 'foo', foo: 1, validation_mode: 'resource' } } + + context 'when the validation_mode is "resource"' do + it 'calls invoke_test_method if the result of a test is not already cached' do + expect(provider).to receive(:fetch_cached_hashes).and_return([]) + expect(provider).to receive(:invoke_test_method).and_return(true) + expect(provider.send(:insync?, context, name, attribute_name, is_hash, should_hash_validate_by_resource)).to be true + end + it 'does not call invoke_test_method if the result of a test is already cached' do + expect(provider).to receive(:fetch_cached_hashes).and_return(cached_test_result) + expect(provider).not_to receive(:invoke_test_method) + expect(provider.send(:insync?, context, name, attribute_name, is_hash, should_hash_validate_by_resource)).to be true + end + end + context 'when the validation_mode is "property"' do + it 'does not call invoke_test_method and returns nil' do + expect(provider).not_to receive(:fetch_cached_hashes) + expect(provider).not_to receive(:invoke_test_method) + expect(provider.send(:insync?, context, name, attribute_name, is_hash, should_hash_validate_by_property)).to be nil + end + end + end + context '.invoke_get_method' do subject(:result) { provider.invoke_get_method(context, name_hash) } @@ -908,6 +950,56 @@ end end + context '.invoke_test_method' do + subject(:result) { provider.invoke_test_method(context, name, should) } + + let(:name) { { name: 'foo', dsc_name: 'bar' } } + let(:should) { name.merge(dsc_ensure: 'present') } + let(:test_properties) { should.reject { |k, _v| k == :name } } + let(:invoke_dsc_resource_data) { nil } + + before(:each) do + allow(context).to receive(:notice) + allow(context).to receive(:debug) + allow(provider).to receive(:invoke_dsc_resource).with(context, name, test_properties, 'test').and_return(invoke_dsc_resource_data) + end + + after(:each) do + described_class.class_variable_set(:@@cached_test_results, []) # rubocop:disable Style/ClassVars + end + + context 'when something went wrong calling Invoke-DscResource' do + it 'falls back on property-by-property state comparison and does not cache anything' do + expect(context).not_to receive(:err) + expect(result).to be(nil) + expect(provider.cached_test_results).to eq([]) + end + end + + context 'when the DSC Resource is in the desired state' do + let(:invoke_dsc_resource_data) { { 'indesiredstate' => true, 'errormessage' => '' } } + + it 'returns true and caches the result' do + expect(context).not_to receive(:err) + expect(result).to eq(true) + expect(provider.cached_test_results).to eq([name.merge(in_desired_state: true)]) + end + end + + context 'when the DSC Resource is not in the desired state' do + let(:invoke_dsc_resource_data) { { 'indesiredstate' => false, 'errormessage' => '' } } + + it 'returns false and caches the result' do + expect(context).not_to receive(:err) + # Resource is not in the desired state + expect(result.first).to eq(false) + # Custom out-of-sync message passed + expect(result.last).to match(/not in the desired state/) + expect(provider.cached_test_results).to eq([name.merge(in_desired_state: false)]) + end + end + end + context '.random_variable_name' do it 'creates random variables' do expect(provider.random_variable_name).not_to be_nil