From 50b2f21ae07fb26479338316e9c8a29274d03bcc Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Wed, 28 Aug 2019 18:24:38 +0100 Subject: [PATCH] (FM-8092) Fix caching scope of transport schemas When running in puppetserver for the [environment_transports](https://github.com/puppetlabs/puppetserver/blob/master/documentation/puppet-api/v3/environment_transports.markdown) API, the Resource API would cache registered transport schemas forever. This meant that deploying a new module version and running the usual environment cache cleaning scripts would not update the transport schema. With this change, we push the transport cache into an ObjectIdCacheAdapter around the environment, which will be cleaned up by the internal utilities. --- lib/puppet/resource_api/transport.rb | 44 ++++++++++---- spec/puppet/resource_api/transport_spec.rb | 67 ++++------------------ 2 files changed, 43 insertions(+), 68 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 7365ee57..80489825 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -12,7 +12,7 @@ def register(schema) unless transports[schema[:name]].nil? raise Puppet::DevError, 'Transport `%{name}` is already registered for `%{environment}`' % { name: schema[:name], - environment: current_environment, + environment: current_environment_name, } end transports[schema[:name]] = Puppet::ResourceApi::TransportSchemaDef.new(schema) @@ -27,7 +27,7 @@ def list module_function :list # rubocop:disable Style/AccessModifierDeclarations # retrieve a Hash of transport schemas, keyed by their name. - # This uses the Puppet autoloader, provide a environment name as `force_environment` + # This uses the Puppet autoloader, provide an environment name as `force_environment` # to choose where to load from. # @api private def list_all_transports(force_environment) @@ -45,7 +45,7 @@ def self.load_all_schemas require 'puppet/settings' require 'puppet/util/autoload' @autoloader ||= Puppet::Util::Autoload.new(self, 'puppet/transport/schema') - @autoloader.loadall(Puppet.lookup(:current_environment)) + @autoloader.loadall(current_environment) end private_class_method :load_all_schemas @@ -74,7 +74,7 @@ def self.validate(name, connection_info) if transport_schema.nil? raise Puppet::DevError, 'Transport for `%{target}` not registered with `%{environment}`' % { target: name, - environment: current_environment, + environment: current_environment_name, } end @@ -108,21 +108,26 @@ def self.wrap_sensitive(name, connection_info) private_class_method :wrap_sensitive def self.transports - @transports ||= {} - @transports[current_environment] ||= {} + env = current_environment + if env + ObjectIdCacheAdapter.adapt(env).retrieve(:rsapi_transport_cache) + else + @transports_default ||= {} + end end private_class_method :transports def self.current_environment - if Puppet.respond_to? :lookup - env = Puppet.lookup(:current_environment) - env.nil? ? :transports_default : env.name - else - :transports_default - end + Puppet.lookup(:current_environment) if Puppet.respond_to? :lookup end private_class_method :current_environment + def self.current_environment_name + env = current_environment + env.nil? ? :transports_default : env.name + end + private_class_method :current_environment_name + def self.clean_bolt_attributes(transport_schema, connection_info) context = get_context(transport_schema.name) @@ -154,4 +159,19 @@ def self.clean_bolt_attributes(transport_schema, connection_info) nil end private_class_method :clean_bolt_attributes + + # copy from https://github.com/puppetlabs/puppet/blob/8cae8a17dbac08d2db0238d5bce2f1e4d1898d65/lib/puppet/pops/adapters.rb#L6-L17 + # to keep backwards compatibility with puppet4 and 5, which don't have this yet. + class ObjectIdCacheAdapter < Puppet::Pops::Adaptable::Adapter + attr_accessor :cache + + def initialize + @cache = {} + end + + # Retrieves a mutable hash with all stored values + def retrieve(obj) + @cache[obj.__id__] ||= {} + end + end end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 13267ab7..e3eee92e 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -5,9 +5,9 @@ def change_environment(name = nil) environment = class_double(Puppet::Node::Environment) if name.nil? - allow(Puppet).to receive(:respond_to?).and_return(false) + allow(Puppet).to receive(:respond_to?).with(:lookup).and_return(false) else - allow(Puppet).to receive(:respond_to?).and_return(true) + allow(Puppet).to receive(:respond_to?).with(:lookup).and_return(true) end allow(Puppet).to receive(:lookup).with(:current_environment).and_return(environment) @@ -73,64 +73,19 @@ def change_environment(name = nil) }, } end - let(:schema2) do - { - name: 'schema2', - desc: 'basic transport', - connection_info: { - host: { - type: 'String', - desc: 'the host ip address or hostname', - }, - }, - } - end - let(:schema3) do - { - name: 'schema3', - desc: 'basic transport', - connection_info: { - user: { - type: 'String', - desc: 'the user to connect as', - }, - password: { - type: 'String', - sensitive: true, - desc: 'the password to make the connection', - }, - }, - } - end - it 'adds to the transports register' do - expect { described_class.register(schema) }.not_to raise_error + context 'when a environment is available' do + before(:each) { change_environment('production') } + it 'adds to the transports register' do + expect { described_class.register(schema) }.not_to raise_error + end end - context 'when a transports are added to multiple environments' do - let(:transports) { described_class.instance_variable_get(:@transports) } + context 'when no environment is available' do + before(:each) { change_environment(nil) } - it 'will record the schemas in the correct structure' do - change_environment(nil) - described_class.register(schema) - expect(transports).to have_key(:transports_default) - expect(transports[:transports_default][schema[:name]]).to be_a_kind_of(Puppet::ResourceApi::TransportSchemaDef) - expect(transports[:transports_default][schema[:name]].definition).to eq(schema) - - change_environment('foo') - described_class.register(schema) - described_class.register(schema2) - expect(transports).to have_key('foo') - expect(transports['foo'][schema[:name]]).to be_a_kind_of(Puppet::ResourceApi::TransportSchemaDef) - expect(transports['foo'][schema[:name]].definition).to eq(schema) - expect(transports['foo'][schema2[:name]]).to be_a_kind_of(Puppet::ResourceApi::TransportSchemaDef) - expect(transports['foo'][schema2[:name]].definition).to eq(schema2) - - change_environment(:bar) - described_class.register(schema3) - expect(transports).to have_key(:bar) - expect(transports[:bar][schema3[:name]]).to be_a_kind_of(Puppet::ResourceApi::TransportSchemaDef) - expect(transports[:bar][schema3[:name]].definition).to eq(schema3) + it 'adds to the transports register' do + expect { described_class.register(schema) }.not_to raise_error end end end