Skip to content

Commit

Permalink
(FM-8092) Fix caching scope of transport schemas
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
DavidS committed Sep 3, 2019
1 parent 7e89026 commit a93ffc2
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 68 deletions.
44 changes: 32 additions & 12 deletions lib/puppet/resource_api/transport.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
68 changes: 12 additions & 56 deletions spec/puppet/resource_api/transport_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -73,64 +73,20 @@ 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
end
context 'when a environment is available' do
before(:each) { change_environment('production') }

context 'when a transports are added to multiple environments' do
let(:transports) { described_class.instance_variable_get(:@transports) }
it 'adds to the transports register' do
expect { described_class.register(schema) }.not_to raise_error
end
end

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)
context 'when no environment is available' do
before(:each) { change_environment(nil) }

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
Expand Down

0 comments on commit a93ffc2

Please sign in to comment.