From 7b0ab297217d07f8bb4991d9a92ec18128c1254d Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 13 Dec 2018 11:49:37 +0000 Subject: [PATCH 01/37] (FM-7597) RSAPI Transport register function --- lib/puppet/resource_api.rb | 7 + lib/puppet/resource_api/transport.rb | 15 ++ lib/puppet/resource_api/type_definition.rb | 250 ++++++++++-------- .../resource_api/base_type_definition_spec.rb | 220 +++++++++++++++ .../resource_api/transport_schema_def_spec.rb | 28 ++ spec/puppet/resource_api/transport_spec.rb | 79 ++++++ .../resource_api/type_definition_spec.rb | 203 +------------- spec/puppet/resource_api_spec.rb | 20 ++ 8 files changed, 512 insertions(+), 310 deletions(-) create mode 100644 lib/puppet/resource_api/transport.rb create mode 100644 spec/puppet/resource_api/base_type_definition_spec.rb create mode 100644 spec/puppet/resource_api/transport_schema_def_spec.rb create mode 100644 spec/puppet/resource_api/transport_spec.rb diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 07b97edf..1d69ac0a 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -5,6 +5,7 @@ require 'puppet/resource_api/property' require 'puppet/resource_api/puppet_context' unless RUBY_PLATFORM == 'java' require 'puppet/resource_api/read_only_parameter' +require 'puppet/resource_api/transport' require 'puppet/resource_api/type_definition' require 'puppet/resource_api/value_creator' require 'puppet/resource_api/version' @@ -451,6 +452,12 @@ def load_device_provider(class_name, type_name_sym, device_class_name, device_na end module_function :load_device_provider # rubocop:disable Style/AccessModifierDeclarations + # keeps the existing register API format. e.g. Puppet::ResourceApi.register_type + def register_transport(schema) + Puppet::ResourceApi::Transport.register(schema) + end + module_function :register_transport # rubocop:disable Style/AccessModifierDeclarations + def self.class_name_from_type_name(type_name) type_name.to_s.split('_').map(&:capitalize).join end diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb new file mode 100644 index 00000000..8e4c0935 --- /dev/null +++ b/lib/puppet/resource_api/transport.rb @@ -0,0 +1,15 @@ +# Remote target transport API +module Puppet::ResourceApi::Transport + def register(schema) + raise Puppet::DevError, 'requires a hash as schema, not `%{other_type}`' % { other_type: schema.class } unless schema.is_a? Hash + raise Puppet::DevError, 'requires a `:name`' unless schema.key? :name + raise Puppet::DevError, 'requires `:desc`' unless schema.key? :desc + raise Puppet::DevError, 'requires `:connection_info`' unless schema.key? :connection_info + raise Puppet::DevError, '`:connection_info` must be a hash, not `%{other_type}`' % { other_type: schema[:connection_info].class } unless schema[:connection_info].is_a?(Hash) + + @transports ||= {} + raise Puppet::DevError, 'Transport `%{name}` is already registered.' % { name: schema[:name] } unless @transports[schema[:name]].nil? + @transports[schema[:name]] = Puppet::ResourceApi::TransportSchemaDef.new(schema) + end + module_function :register # rubocop:disable Style/AccessModifierDeclarations +end diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb index e23a077a..a5bf2ccb 100644 --- a/lib/puppet/resource_api/type_definition.rb +++ b/lib/puppet/resource_api/type_definition.rb @@ -1,144 +1,168 @@ # Provides accessor methods for the type being provided -class Puppet::ResourceApi::TypeDefinition - attr_reader :definition +module Puppet::ResourceApi + # pre-declare class + class BaseTypeDefinition; end + + # RSAPI Resource Type + class TypeDefinition < BaseTypeDefinition + def initialize(definition) + super(definition, :attributes) + end - def initialize(definition) - @data_type_cache = {} - validate_schema(definition) - end + def ensurable? + attributes.key?(:ensure) + end - def name - @definition[:name] - end + # rubocop complains when this is named has_feature? + def feature?(feature) + supported = (definition[:features] && definition[:features].include?(feature)) + if supported + Puppet.debug("#{definition[:name]} supports `#{feature}`") + else + Puppet.debug("#{definition[:name]} does not support `#{feature}`") + end + supported + end - def attributes - @definition[:attributes] - end + def validate_schema(definition, attr_key) + super(definition, attr_key) + [:title, :provider, :alias, :audit, :before, :consume, :export, :loglevel, :noop, :notify, :require, :schedule, :stage, :subscribe, :tag].each do |name| + raise Puppet::DevError, 'must not define an attribute called `%{name}`' % { name: name.inspect } if definition[attr_key].key? name + end + if definition.key?(:title_patterns) && !definition[:title_patterns].is_a?(Array) + raise Puppet::DevError, '`:title_patterns` must be an array, not `%{other_type}`' % { other_type: definition[:title_patterns].class } + end - def ensurable? - @definition[:attributes].key?(:ensure) - end + Puppet::ResourceApi::DataTypeHandling.validate_ensure(definition) - def namevars - @namevars ||= @definition[:attributes].select { |_name, options| - options.key?(:behaviour) && options[:behaviour] == :namevar - }.keys - end + definition[:features] ||= [] + supported_features = %w[supports_noop canonicalize remote_resource simple_get_filter].freeze + unknown_features = definition[:features] - supported_features + Puppet.warning("Unknown feature detected: #{unknown_features.inspect}") unless unknown_features.empty? - # rubocop complains when this is named has_feature? - def feature?(feature) - supported = (definition[:features] && definition[:features].include?(feature)) - if supported - Puppet.debug("#{definition[:name]} supports `#{feature}`") - else - Puppet.debug("#{definition[:name]} does not support `#{feature}`") + # store the validated definition + @definition = definition end - supported end - def validate_schema(definition) - raise Puppet::DevError, 'Type definition must be a Hash, not `%{other_type}`' % { other_type: definition.class } unless definition.is_a?(Hash) - raise Puppet::DevError, 'Type definition must have a name' unless definition.key? :name - raise Puppet::DevError, 'Type definition must have `:attributes`' unless definition.key? :attributes - unless definition[:attributes].is_a?(Hash) - raise Puppet::DevError, '`%{name}.attributes` must be a hash, not `%{other_type}`' % { - name: definition[:name], other_type: definition[:attributes].class - } - end - [:title, :provider, :alias, :audit, :before, :consume, :export, :loglevel, :noop, :notify, :require, :schedule, :stage, :subscribe, :tag].each do |name| - raise Puppet::DevError, 'must not define an attribute called `%{name}`' % { name: name.inspect } if definition[:attributes].key? name - end - if definition.key?(:title_patterns) && !definition[:title_patterns].is_a?(Array) - raise Puppet::DevError, '`:title_patterns` must be an array, not `%{other_type}`' % { other_type: definition[:title_patterns].class } + # RSAPI Transport schema + class TransportSchemaDef < BaseTypeDefinition + def initialize(definition) + super(definition, :connection_info) end + end - Puppet::ResourceApi::DataTypeHandling.validate_ensure(definition) + # Base RSAPI schema Object + class BaseTypeDefinition + attr_reader :definition, :attributes + + def initialize(definition, attr_key) + @data_type_cache = {} + validate_schema(definition, attr_key) + end - definition[:features] ||= [] - supported_features = %w[supports_noop canonicalize remote_resource simple_get_filter].freeze - unknown_features = definition[:features] - supported_features - Puppet.warning("Unknown feature detected: #{unknown_features.inspect}") unless unknown_features.empty? + def name + @definition[:name] + end - definition[:attributes].each do |key, attr| - raise Puppet::DevError, "`#{definition[:name]}.#{key}` must be a Hash, not a #{attr.class}" unless attr.is_a? Hash - raise Puppet::DevError, "`#{definition[:name]}.#{key}` has no type" unless attr.key? :type - Puppet.warning("`#{definition[:name]}.#{key}` has no docs") unless attr.key? :desc + def namevars + @namevars ||= attributes.select { |_name, options| + options.key?(:behaviour) && options[:behaviour] == :namevar + }.keys + end - # validate the type by attempting to parse into a puppet type - @data_type_cache[definition[:attributes][key][:type]] ||= - Puppet::ResourceApi::DataTypeHandling.parse_puppet_type( - key, - definition[:attributes][key][:type], - ) + def validate_schema(definition, attr_key) + raise Puppet::DevError, '%{type_class} must be a Hash, not `%{other_type}`' % { type_class: self.class.name, other_type: definition.class } unless definition.is_a?(Hash) + @attributes = definition[attr_key] + raise Puppet::DevError, '%{type_class} must have a name' % { type_class: self.class.name } unless definition.key? :name + raise Puppet::DevError, '%{type_class} must have `%{attr_key}`' % { type_class: self.class.name, attrs: attr_key } unless definition.key? attr_key + unless attributes.is_a?(Hash) + raise Puppet::DevError, '`%{name}.%{attrs}` must be a hash, not `%{other_type}`' % { + name: definition[:name], attrs: attr_key, other_type: attributes.class + } + end - # fixup any weird behavior ;-) - next unless attr[:behavior] - if attr[:behaviour] - raise Puppet::DevError, "the '#{key}' attribute has both a `behavior` and a `behaviour`, only use one" + attributes.each do |key, attr| + raise Puppet::DevError, "`#{definition[:name]}.#{key}` must be a Hash, not a #{attr.class}" unless attr.is_a? Hash + raise Puppet::DevError, "`#{definition[:name]}.#{key}` has no type" unless attr.key? :type + Puppet.warning("`#{definition[:name]}.#{key}` has no docs") unless attr.key? :desc + + # validate the type by attempting to parse into a puppet type + @data_type_cache[attributes[key][:type]] ||= + Puppet::ResourceApi::DataTypeHandling.parse_puppet_type( + key, + attributes[key][:type], + ) + + # fixup any weird behavior ;-) + next unless attr[:behavior] + if attr[:behaviour] + raise Puppet::DevError, "the '#{key}' attribute has both a `behavior` and a `behaviour`, only use one" + end + attr[:behaviour] = attr[:behavior] + attr.delete(:behavior) end - attr[:behaviour] = attr[:behavior] - attr.delete(:behavior) + # store the validated definition + @definition = definition end - # store the validated definition - @definition = definition - end - # validates a resource hash against its type schema - def check_schema(resource) - namevars.each do |namevar| - if resource[namevar].nil? - raise Puppet::ResourceError, "`#{name}.get` did not return a value for the `#{namevar}` namevar attribute" + # validates a resource hash against its type schema + def check_schema(resource) + namevars.each do |namevar| + if resource[namevar].nil? + raise Puppet::ResourceError, "`#{name}.get` did not return a value for the `#{namevar}` namevar attribute" + end end - end - message = "Provider returned data that does not match the Type Schema for `#{name}[#{resource[namevars.first]}]`" + message = "Provider returned data that does not match the Type Schema for `#{name}[#{resource[namevars.first]}]`" - rejected_keys = check_schema_keys(resource) # removes bad keys - bad_values = check_schema_values(resource) + rejected_keys = check_schema_keys(resource) # removes bad keys + bad_values = check_schema_values(resource) - unless rejected_keys.empty? - message += "\n Unknown attribute:\n" - rejected_keys.each { |key, _value| message += " * #{key}\n" } - end - unless bad_values.empty? - message += "\n Value type mismatch:\n" - bad_values.each { |key, value| message += " * #{key}: #{value}\n" } - end + unless rejected_keys.empty? + message += "\n Unknown attribute:\n" + rejected_keys.each { |key, _value| message += " * #{key}\n" } + end + unless bad_values.empty? + message += "\n Value type mismatch:\n" + bad_values.each { |key, value| message += " * #{key}: #{value}\n" } + end - return if rejected_keys.empty? && bad_values.empty? + return if rejected_keys.empty? && bad_values.empty? - if Puppet.settings[:strict] == :off - Puppet.debug(message) - elsif Puppet.settings[:strict] == :warning - Puppet::ResourceApi.warning_count += 1 - Puppet.warning(message) if Puppet::ResourceApi.warning_count <= 100 # maximum number of schema warnings to display in a run - elsif Puppet.settings[:strict] == :error - raise Puppet::DevError, message + if Puppet.settings[:strict] == :off + Puppet.debug(message) + elsif Puppet.settings[:strict] == :warning + Puppet::ResourceApi.warning_count += 1 + Puppet.warning(message) if Puppet::ResourceApi.warning_count <= 100 # maximum number of schema warnings to display in a run + elsif Puppet.settings[:strict] == :error + raise Puppet::DevError, message + end end - end - # Returns an array of keys that where not found in the type schema - # Modifies the resource passed in, leaving only valid attributes - def check_schema_keys(resource) - rejected = [] - resource.reject! { |key| rejected << key if key != :title && attributes.key?(key) == false } - rejected - end + # Returns an array of keys that where not found in the type schema + # Modifies the resource passed in, leaving only valid attributes + def check_schema_keys(resource) + rejected = [] + resource.reject! { |key| rejected << key if key != :title && attributes.key?(key) == false } + rejected + end - # Returns a hash of keys and values that are not valid - # does not modify the resource passed in - def check_schema_values(resource) - bad_vals = {} - resource.each do |key, value| - next unless attributes[key] - type = @data_type_cache[attributes[key][:type]] - error_message = Puppet::ResourceApi::DataTypeHandling.try_validate( - type, - value, - '', - ) - bad_vals[key] = value unless error_message.nil? + # Returns a hash of keys and values that are not valid + # does not modify the resource passed in + def check_schema_values(resource) + bad_vals = {} + resource.each do |key, value| + next unless attributes[key] + type = @data_type_cache[attributes[key][:type]] + error_message = Puppet::ResourceApi::DataTypeHandling.try_validate( + type, + value, + '', + ) + bad_vals[key] = value unless error_message.nil? + end + bad_vals end - bad_vals end end diff --git a/spec/puppet/resource_api/base_type_definition_spec.rb b/spec/puppet/resource_api/base_type_definition_spec.rb new file mode 100644 index 00000000..2c432dc0 --- /dev/null +++ b/spec/puppet/resource_api/base_type_definition_spec.rb @@ -0,0 +1,220 @@ +require 'spec_helper' + +RSpec.describe Puppet::ResourceApi::BaseTypeDefinition do + subject(:type) { described_class.new(definition, :attributes) } + + let(:definition) do + { name: 'some_resource', attributes: { + ensure: { + type: 'Enum[present, absent]', + desc: 'Whether this resource should be present or absent on the target system.', + default: 'present', + }, + name: { + type: 'String', + desc: 'The name of the resource you want to manage.', + behaviour: :namevar, + }, + prop: { + type: 'Integer', + desc: 'A mandatory property, that MUST NOT be validated on deleting.', + }, + }, features: feature_support } + end + let(:feature_support) { [] } + + it { expect { described_class.new(nil, :attributes) }.to raise_error Puppet::DevError, %r{BaseTypeDefinition must be a Hash} } + + describe '.name' do + it { expect(type.name).to eq 'some_resource' } + end + + describe '#check_schema_keys' do + context 'when resource contains only valid keys' do + it 'returns an empty array' do + expect(type.check_schema_keys(definition[:attributes])).to eq([]) + end + end + + context 'when resource contains invalid keys' do + let(:resource) { { name: 'test_string', wibble: '1', foo: '2' } } + + it 'returns an array containing the bad keys' do + expect(type.check_schema_keys(resource)).to eq([:wibble, :foo]) + end + end + end + + describe '#check_schema_values' do + context 'when resource contains only valid values' do + let(:resource) { { name: 'some_resource', prop: 1, ensure: 'present' } } + + it 'returns an empty array' do + expect(type.check_schema_values(resource)).to eq({}) + end + end + + context 'when resource contains invalid values' do + let(:resource) { { name: 'test_string', prop: 'foo', ensure: 1 } } + + it 'returns a hash of the keys that have invalid values' do + expect(type.check_schema_values(resource)).to eq(prop: 'foo', ensure: 1) + end + end + end + + describe '#check_schema' do + context 'when resource does not contain its namevar' do + let(:resource) { { nom: 'some_resource', prop: 1, ensure: 'present' } } + + it { expect { type.check_schema(resource) }.to raise_error Puppet::ResourceError, %r{`some_resource.get` did not return a value for the `name` namevar attribute} } + end + + context 'when a resource contains unknown attributes' do + let(:resource) { { name: 'wibble', prop: 1, ensure: 'present', foo: 'bar' } } + let(:message) { %r{Provider returned data that does not match the Type Schema for `some_resource\[wibble\]`\n\s*Unknown attribute:\n\s*\* foo} } + let(:strict_level) { :warning } + + before(:each) do + Puppet::ResourceApi.warning_count = 0 + Puppet.settings[:strict] = strict_level + end + + context 'when puppet strict is set to default (warning)' do + it 'displays up to 100 warnings' do + expect(Puppet).to receive(:warning).with(message).exactly(100).times + 110.times do + type.check_schema(resource.dup) + end + end + end + + context 'when puppet strict is set to error' do + let(:strict_level) { :error } + + it 'raises a DevError' do + expect { type.check_schema(resource) }.to raise_error Puppet::DevError, message + end + end + + context 'when puppet strict is set to off' do + let(:strict_level) { :off } + + it 'logs to Debug console' do + expect(Puppet).to receive(:debug).with(message) + type.check_schema(resource) + end + end + end + + context 'when a resource contains invalid value' do + let(:resource) { { name: 'wibble', prop: 'foo', ensure: 'present' } } + let(:message) { %r{Provider returned data that does not match the Type Schema for `some_resource\[wibble\]`\n\s*Value type mismatch:\n\s*\* prop: foo} } + let(:strict_level) { :warning } + + before(:each) do + Puppet::ResourceApi.warning_count = 0 + Puppet.settings[:strict] = strict_level + end + + context 'when puppet strict is set to default (warning)' do + it 'displays up to 100 warnings' do + expect(Puppet).to receive(:warning).with(message).exactly(100).times + 110.times do + type.check_schema(resource.dup) + end + end + end + + context 'when puppet strict is set to error' do + let(:strict_level) { :error } + + it 'raises a DevError' do + expect { type.check_schema(resource) }.to raise_error Puppet::DevError, message + end + end + + context 'when puppet strict is set to off' do + let(:strict_level) { :off } + + it 'logs to Debug console' do + expect(Puppet).to receive(:debug).with(message) + type.check_schema(resource) + end + end + end + end + + describe '#validate_schema' do + context 'when the type definition does not have a name' do + let(:definition) { { attributes: 'some_string' } } + + it { expect { type }.to raise_error Puppet::DevError, %r{must have a name} } + end + + context 'when attributes is not a hash' do + let(:definition) { { name: 'some_resource', attributes: 'some_string' } } + + it { expect { type }.to raise_error Puppet::DevError, %r{`some_resource.attributes` must be a hash} } + end + + context 'when an attribute is not a hash' do + let(:definition) { { name: 'some_resource', attributes: { name: 'some_string' } } } + + it { expect { type }.to raise_error Puppet::DevError, %r{`some_resource.name` must be a Hash} } + end + + context 'when an attribute has no type' do + let(:definition) { { name: 'some_resource', attributes: { name: { desc: 'message' } } } } + + it { expect { type }.to raise_error Puppet::DevError, %r{has no type} } + end + + context 'when an attribute has no descrption' do + let(:definition) { { name: 'some_resource', attributes: { name: { type: 'String' } } } } + + it 'Raises a warning message' do + expect(Puppet).to receive(:warning).with('`some_resource.name` has no docs') + type + end + end + + context 'when an attribute has an unsupported type' do + let(:definition) { { name: 'some_resource', attributes: { name: { type: 'basic' } } } } + + it { expect { type }.to raise_error %r{ is not a valid type specification} } + end + + context 'with both behavior and behaviour' do + let(:definition) do + { + name: 'bad_behaviour', + attributes: { + name: { + type: 'String', + behaviour: :namevar, + behavior: :namevar, + }, + }, + } + end + + it { expect { type }.to raise_error Puppet::DevError, %r{name.*attribute has both} } + end + + context 'when registering a type with badly formed attribute type' do + let(:definition) do + { + name: 'bad_syntax', + attributes: { + name: { + type: 'Optional[String', + }, + }, + } + end + + it { expect { type }.to raise_error Puppet::DevError, %r{The type of the `name` attribute `Optional\[String` could not be parsed:} } + end + end +end diff --git a/spec/puppet/resource_api/transport_schema_def_spec.rb b/spec/puppet/resource_api/transport_schema_def_spec.rb new file mode 100644 index 00000000..e88bb0e8 --- /dev/null +++ b/spec/puppet/resource_api/transport_schema_def_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +RSpec.describe Puppet::ResourceApi::TransportSchemaDef do + subject(:type) { described_class.new(definition) } + + let(:definition) do + { name: 'some_target', + connection_info: { + host: { + type: 'String', + desc: 'The IP address or hostname', + }, + user: { + type: 'String', + desc: 'The user to connect as', + }, + } } + end + + it { expect { described_class.new(nil) }.to raise_error Puppet::DevError, %r{TransportSchemaDef must be a Hash} } + + describe '#attributes' do + context 'when type has attributes' do + it { expect(type.attributes).to be_key(:host) } + it { expect(type.attributes).to be_key(:user) } + end + end +end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb new file mode 100644 index 00000000..7bbeca6a --- /dev/null +++ b/spec/puppet/resource_api/transport_spec.rb @@ -0,0 +1,79 @@ +require 'spec_helper' + +RSpec.describe Puppet::ResourceApi::Transport do + let(:strict_level) { :error } + + before(:each) do + # set default to strictest setting + # by default Puppet runs at warning level + Puppet.settings[:strict] = strict_level + # Enable debug logging + Puppet.debug = true + end + + context 'when registering a schema with missing keys' do + it { expect { described_class.register([]) }.to raise_error(Puppet::DevError, %r{requires a hash as schema}) } + it { expect { described_class.register({}) }.to raise_error(Puppet::DevError, %r{requires a `:name`}) } + it { expect { described_class.register(name: 'no connection info', desc: 'some description') }.to raise_error(Puppet::DevError, %r{requires `:connection_info`}) } + it { expect { described_class.register(name: 'no description') }.to raise_error(Puppet::DevError, %r{requires `:desc`}) } + it { expect { described_class.register(name: 'no hash attributes', desc: 'some description', connection_info: []) }.to raise_error(Puppet::DevError, %r{`:connection_info` must be a hash, not}) } + end + + context 'when registering a minimal transport' do + let(:schema) { { name: 'minimal', desc: 'a minimal connection', connection_info: {} } } + + it { expect { described_class.register(schema) }.not_to raise_error } + + context 'when re-registering a transport' do + it { expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) } + end + end + + context 'when registering a transport' do + let(:schema) do + { + name: 'a_remote_thing', + desc: 'basic transport', + connection_info: { + host: { + type: 'String', + desc: 'the host ip address or hostname', + }, + user: { + type: 'String', + desc: 'the user to connect as', + }, + password: { + type: 'Sensitive[String]', + 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 + end + + context 'when registering a transport with a bad type' do + let(:schema) do + { + name: 'a_bad_thing', + desc: 'basic transport', + connection_info: { + host: { + type: 'garbage', + desc: 'the host ip address or hostname', + }, + }, + } + end + + it { + expect { described_class.register(schema) }.to raise_error( + Puppet::DevError, %r{ is not a valid type specification} + ) + } + end +end diff --git a/spec/puppet/resource_api/type_definition_spec.rb b/spec/puppet/resource_api/type_definition_spec.rb index a2df5d99..121745a6 100644 --- a/spec/puppet/resource_api/type_definition_spec.rb +++ b/spec/puppet/resource_api/type_definition_spec.rb @@ -23,22 +23,18 @@ end let(:feature_support) { [] } - it { expect { described_class.new(nil) }.to raise_error Puppet::DevError, %r{Type definition must be a Hash} } - - describe '.name' do - it { expect(type.name).to eq 'some_resource' } - end + it { expect { described_class.new(nil) }.to raise_error Puppet::DevError, %r{TypeDefinition must be a Hash} } describe '#ensurable?' do context 'when type is ensurable' do - let(:definition) { { name: 'some_resource', attributes: { ensure: { type: 'Enum[absent, present]' } } } } + let(:definition) { { name: 'ensurable', attributes: { ensure: { type: 'Enum[absent, present]' } } } } it { expect(type).to be_ensurable } it { expect(type.attributes).to be_key(:ensure) } end context 'when type is not ensurable' do - let(:definition) { { name: 'some_resource', attributes: { name: { type: 'String' } } } } + let(:definition) { { name: 'ensurable', attributes: { name: { type: 'String' } } } } it { expect(type).not_to be_ensurable } it { expect(type.attributes).to be_key(:name) } @@ -61,204 +57,17 @@ describe '#attributes' do context 'when type has attributes' do - let(:definition) { { name: 'some_resource', attributes: { wibble: { type: 'String' } } } } - - it { expect(type.attributes).to be_key(:wibble) } - end - end - - describe '#check_schema_keys' do - context 'when resource contains only valid keys' do - it 'returns an empty array' do - expect(type.check_schema_keys(definition[:attributes])).to eq([]) - end - end - - context 'when resource contains invalid keys' do - let(:resource) { { name: 'test_string', wibble: '1', foo: '2' } } - - it 'returns an array containing the bad keys' do - expect(type.check_schema_keys(resource)).to eq([:wibble, :foo]) - end - end - end - - describe '#check_schema_values' do - context 'when resource contains only valid values' do - let(:resource) { { name: 'some_resource', prop: 1, ensure: 'present' } } - - it 'returns an empty array' do - expect(type.check_schema_values(resource)).to eq({}) - end - end - - context 'when resource contains invalid values' do - let(:resource) { { name: 'test_string', prop: 'foo', ensure: 1 } } - - it 'returns a hash of the keys that have invalid values' do - expect(type.check_schema_values(resource)).to eq(prop: 'foo', ensure: 1) - end - end - end - - describe '#check_schema' do - context 'when resource does not contain its namevar' do - let(:resource) { { nom: 'some_resource', prop: 1, ensure: 'present' } } - - it { expect { type.check_schema(resource) }.to raise_error Puppet::ResourceError, %r{`some_resource.get` did not return a value for the `name` namevar attribute} } - end - - context 'when a resource contains unknown attributes' do - let(:resource) { { name: 'wibble', prop: 1, ensure: 'present', foo: 'bar' } } - let(:message) { %r{Provider returned data that does not match the Type Schema for `some_resource\[wibble\]`\n\s*Unknown attribute:\n\s*\* foo} } - let(:strict_level) { :warning } - - before(:each) do - Puppet::ResourceApi.warning_count = 0 - Puppet.settings[:strict] = strict_level - end - - context 'when puppet strict is set to default (warning)' do - it 'displays up to 100 warnings' do - expect(Puppet).to receive(:warning).with(message).exactly(100).times - 110.times do - type.check_schema(resource.dup) - end - end - end - - context 'when puppet strict is set to error' do - let(:strict_level) { :error } - - it 'raises a DevError' do - expect { type.check_schema(resource) }.to raise_error Puppet::DevError, message - end - end - - context 'when puppet strict is set to off' do - let(:strict_level) { :off } - - it 'logs to Debug console' do - expect(Puppet).to receive(:debug).with(message) - type.check_schema(resource) - end - end - end - - context 'when a resource contains invalid value' do - let(:resource) { { name: 'wibble', prop: 'foo', ensure: 'present' } } - let(:message) { %r{Provider returned data that does not match the Type Schema for `some_resource\[wibble\]`\n\s*Value type mismatch:\n\s*\* prop: foo} } - let(:strict_level) { :warning } - - before(:each) do - Puppet::ResourceApi.warning_count = 0 - Puppet.settings[:strict] = strict_level - end - - context 'when puppet strict is set to default (warning)' do - it 'displays up to 100 warnings' do - expect(Puppet).to receive(:warning).with(message).exactly(100).times - 110.times do - type.check_schema(resource.dup) - end - end - end - - context 'when puppet strict is set to error' do - let(:strict_level) { :error } - - it 'raises a DevError' do - expect { type.check_schema(resource) }.to raise_error Puppet::DevError, message - end - end - - context 'when puppet strict is set to off' do - let(:strict_level) { :off } - - it 'logs to Debug console' do - expect(Puppet).to receive(:debug).with(message) - type.check_schema(resource) - end - end + it { expect(type.attributes).to be_key(:ensure) } + it { expect(type.attributes).to be_key(:name) } + it { expect(type.attributes).to be_key(:prop) } end end describe '#validate_schema' do - context 'when the type definition does not have a name' do - let(:definition) { { attributes: 'some_string' } } - - it { expect { type }.to raise_error Puppet::DevError, %r{Type definition must have a name} } - end - - context 'when attributes is not a hash' do - let(:definition) { { name: 'some_resource', attributes: 'some_string' } } - - it { expect { type }.to raise_error Puppet::DevError, %r{`some_resource.attributes` must be a hash} } - end - context 'when the schema contains title_patterns and it is not an array' do let(:definition) { { name: 'some_resource', title_patterns: {}, attributes: {} } } it { expect { type }.to raise_error Puppet::DevError, %r{`:title_patterns` must be an array} } end - - context 'when an attribute is not a hash' do - let(:definition) { { name: 'some_resource', attributes: { name: 'some_string' } } } - - it { expect { type }.to raise_error Puppet::DevError, %r{`some_resource.name` must be a Hash} } - end - - context 'when an attribute has no type' do - let(:definition) { { name: 'some_resource', attributes: { name: { desc: 'message' } } } } - - it { expect { type }.to raise_error Puppet::DevError, %r{has no type} } - end - - context 'when an attribute has no descrption' do - let(:definition) { { name: 'some_resource', attributes: { name: { type: 'String' } } } } - - it 'Raises a warning message' do - expect(Puppet).to receive(:warning).with('`some_resource.name` has no docs') - type - end - end - - context 'when an attribute has an unsupported type' do - let(:definition) { { name: 'some_resource', attributes: { name: { type: 'basic' } } } } - - it { expect { type }.to raise_error %r{ is not a valid type specification} } - end - - context 'with both behavior and behaviour' do - let(:definition) do - { - name: 'bad_behaviour', - attributes: { - name: { - type: 'String', - behaviour: :namevar, - behavior: :namevar, - }, - }, - } - end - - it { expect { type }.to raise_error Puppet::DevError, %r{name.*attribute has both} } - end - - context 'when registering a type with badly formed attribute type' do - let(:definition) do - { - name: 'bad_syntax', - attributes: { - name: { - type: 'Optional[String', - }, - }, - } - end - - it { expect { type }.to raise_error Puppet::DevError, %r{The type of the `name` attribute `Optional\[String` could not be parsed:} } - end end end diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index 83fd72c3..7daa44e6 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -1825,4 +1825,24 @@ def set(_context, changes) end it { expect { described_class.register_type(definition) }.to raise_error Puppet::ResourceError, %r{^`bad` is not a valid behaviour value$} } end end + + describe '#register_transport' do + let(:schema) do + { + name: 'test_transport', + desc: 'a demo transport', + connection_info: { + host: { + type: 'String', + desc: 'hostname', + }, + }, + } + end + + it 'calls Puppet::ResourceApi::Transport.register' do + expect(Puppet::ResourceApi::Transport).to receive(:register).with(schema) + described_class.register_transport(schema) + end + end end From 4a3e0bb994a0862d9eef6e12fc883cc4133f55a7 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Tue, 8 Jan 2019 11:09:31 +0000 Subject: [PATCH 02/37] (FM-7600) RSAPI Transport connect method --- lib/puppet/resource_api/transport.rb | 21 ++++++ lib/puppet/resource_api/type_definition.rb | 16 +++++ .../resource_api/transport_schema_def_spec.rb | 17 +++++ spec/puppet/resource_api/transport_spec.rb | 71 ++++++++++++++++++- 4 files changed, 124 insertions(+), 1 deletion(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 8e4c0935..ac66a8ac 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -12,4 +12,25 @@ def register(schema) @transports[schema[:name]] = Puppet::ResourceApi::TransportSchemaDef.new(schema) end module_function :register # rubocop:disable Style/AccessModifierDeclarations + + def connect(name, connection_info) + validate(name, connection_info) + begin + require "puppet/transport/#{name}" + class_name = name.split('_').map { |e| e.capitalize }.join + Puppet::Transport.const_get(class_name).new(connection_info) + rescue LoadError, NameError => detail + raise Puppet::DevError, 'Cannot load transport for `%{target}`: %{detail}' % { target: name, detail: detail } + end + end + module_function :connect # rubocop:disable Style/AccessModifierDeclarations + + def self.validate(name, connection_info) + @transports ||= {} + transport_schema = @transports[name] + raise Puppet::DevError, 'Transport for `%{target}` not registered' % { target: name } if transport_schema.nil? + + transport_schema.check_schema(connection_info) + transport_schema.validate(connection_info) + end end diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb index a5bf2ccb..130bf524 100644 --- a/lib/puppet/resource_api/type_definition.rb +++ b/lib/puppet/resource_api/type_definition.rb @@ -50,6 +50,22 @@ class TransportSchemaDef < BaseTypeDefinition def initialize(definition) super(definition, :connection_info) end + + def validate(resource) + # enforce mandatory attributes + missing_attrs = [] + + attributes.each do |name, _options| + type = @data_type_cache[attributes[name][:type]] + + if resource[name].nil? && !(type.instance_of? Puppet::Pops::Types::POptionalType) + missing_attrs << name + end + end + + error_msg = "The following mandatory attributes were not provided:\n * " + missing_attrs.join(", \n * ") + raise Puppet::ResourceError, error_msg if missing_attrs.any? + end end # Base RSAPI schema Object diff --git a/spec/puppet/resource_api/transport_schema_def_spec.rb b/spec/puppet/resource_api/transport_schema_def_spec.rb index e88bb0e8..92cac1fa 100644 --- a/spec/puppet/resource_api/transport_schema_def_spec.rb +++ b/spec/puppet/resource_api/transport_schema_def_spec.rb @@ -25,4 +25,21 @@ it { expect(type.attributes).to be_key(:user) } end end + + describe '#validate' do + context 'when resource is missing attributes' do + let(:resource) { {} } + + it 'raises an error listing the missing attributes' do + expect { type.validate(resource) }.to raise_error Puppet::ResourceError, %r{host} + expect { type.validate(resource) }.to raise_error Puppet::ResourceError, %r{user} + end + end + + context 'when resource has all its attributes' do + let(:resource) { { host: '1234', user: '4321' } } + + it { expect { type.validate(resource) }.not_to raise_error } + end + end end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 7bbeca6a..f1f27d5a 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -44,7 +44,8 @@ desc: 'the user to connect as', }, password: { - type: 'Sensitive[String]', + type: 'String', + sensitive: true, desc: 'the password to make the connection', }, }, @@ -76,4 +77,72 @@ ) } end + + context 'when connecting to a transport' do + let(:name) { 'test_target' } + let(:connection_info) do + { + name: 'test_target', + desc: 'a basic transport', + connection_info: { + host: { + type: 'String', + desc: 'the host ip address or hostname', + }, + }, + } + end + + context 'when the transport file does not exist' do + it 'throws a DevError' do + expect(described_class).to receive(:validate).with(name, connection_info) + expect { described_class.connect(name, connection_info) }.to raise_error Puppet::DevError, + %r{Cannot load transport for `test_target`} + end + end + + context 'when the transport file does exist' do + context 'with and incorrectly defined transport' do + it 'throws a DevError' do + expect(described_class).to receive(:validate).with(name, connection_info) + expect(described_class).to receive(:require).with('puppet/transport/test_target') + expect { described_class.connect(name, connection_info) }.to raise_error Puppet::DevError, + %r{uninitialized constant Puppet::Transport} + end + end + + context 'with a correctly defined transport' do + let(:test_target) { double('Puppet::Transport::TestTarget') } # rubocop:disable RSpec/VerifiedDoubles + + it 'loads initiates the class successfully' do + expect(described_class).to receive(:require).with('puppet/transport/test_target') + expect(described_class).to receive(:validate).with(name, connection_info) + + stub_const('Puppet::Transport::TestTarget', test_target) + expect(test_target).to receive(:new).with(connection_info) + + described_class.connect(name, connection_info) + end + end + end + end + + describe '#self.validate' do + context 'when the transport being validated has not be registered' do + it { expect { described_class.validate('wibble', {}) }.to raise_error Puppet::DevError, %r{Transport for `wibble` not registered} } + end + + context 'when the transport being validated has been registered' do + let(:schema) { { name: 'validate', desc: 'a minimal connection', connection_info: {} } } + + it 'continues to validate the connection_info' do + # rubocop:disable RSpec/AnyInstance + expect_any_instance_of(Puppet::ResourceApi::TransportSchemaDef).to receive(:check_schema).with({}) + expect_any_instance_of(Puppet::ResourceApi::TransportSchemaDef).to receive(:validate).with({}) + # rubocop:enable RSpec/AnyInstance + described_class.register(schema) + described_class.validate('validate', {}) + end + end + end end From e7ef6ac598d09f59f728fa0a385a227db6ae3a20 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Mon, 14 Jan 2019 13:07:53 +0000 Subject: [PATCH 03/37] (FM-7674) Add a Transport wrapper class Emulate a puppet Device-style class using a Resource API Transport. Can be used to provide backwards compatibility like this: ``` require 'puppet/resource_api/transport/wrapper' # force registering the transport require 'puppet/transport/schema/panos' module Puppet::Util::NetworkDevice::Panos class Device < Puppet::ResourceApi::Transport::Wrapper def initialize(url_or_config, _options = {}) puts url_or_config.inspect super('panos', url_or_config) end end end ``` --- lib/puppet/resource_api/transport.rb | 6 + lib/puppet/resource_api/transport/wrapper.rb | 43 +++++++ spec/acceptance/device_spec.rb | 8 +- .../puppet/transport/schema/test_device.rb | 8 ++ .../lib/puppet/transport/test_device.rb | 21 ++++ .../resource_api/transport/wrapper_spec.rb | 116 ++++++++++++++++++ spec/puppet/resource_api/transport_spec.rb | 4 +- 7 files changed, 203 insertions(+), 3 deletions(-) create mode 100644 lib/puppet/resource_api/transport/wrapper.rb create mode 100644 spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb create mode 100644 spec/fixtures/test_module/lib/puppet/transport/test_device.rb create mode 100644 spec/puppet/resource_api/transport/wrapper_spec.rb diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index ac66a8ac..5858cd91 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -1,3 +1,9 @@ +# rubocop:disable Style/Documentation +module Puppet; end +module Puppet::ResourceApi; end +module Puppet::ResourceApi::Transport; end +# rubocop:enable Style/Documentation + # Remote target transport API module Puppet::ResourceApi::Transport def register(schema) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb new file mode 100644 index 00000000..97fe4fe0 --- /dev/null +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -0,0 +1,43 @@ +require 'puppet/resource_api/transport' +require 'hocon' +require 'hocon/config_syntax' + +# Puppet::ResourceApi::Transport::Wrapper` to interface between the Util::NetworkDevice +class Puppet::ResourceApi::Transport::Wrapper + attr_reader :transport + + def initialize(name, url_or_config) + if url_or_config.is_a? String + @url = URI.parse(url_or_config) + raise "Unexpected url '#{url_or_config}' found. Only file:/// URLs for configuration supported at the moment." unless @url.scheme == 'file' + else + @config = url_or_config + end + + @transport = Puppet::ResourceApi::Transport.connect(name, config) + end + + def config + raise "Trying to load config from '#{@url.path}, but file does not exist." if @url && !File.exist?(@url.path) + # symbolize top-level keys to match up with expected provider/resource data conventions + @config ||= (Hocon.load(@url.path, syntax: Hocon::ConfigSyntax::HOCON) || {}).map { |k, v| [k.to_sym, v] }.to_h + end + + def facts + # @transport.facts + custom_facts # look into custom facts work by TP + @transport.facts + end + + def respond_to_missing?(name, _include_private) + (@transport.respond_to? name) || super + end + + def method_missing(method_name, *args, &block) + if @transport.respond_to? method_name + puts "Delegating #{method_name}" + @transport.send(method_name, *args, &block) + else + super + end + end +end diff --git a/spec/acceptance/device_spec.rb b/spec/acceptance/device_spec.rb index 69302007..909313c9 100644 --- a/spec/acceptance/device_spec.rb +++ b/spec/acceptance/device_spec.rb @@ -74,9 +74,11 @@ <= Gem::Version.new('5.3.6') && Gem::Version.new(Puppet::PUPPETVERSION) != Gem::Version.new('5.4.0') @@ -86,10 +88,14 @@ def is_device_apply_supported? skip "No device --apply in puppet before v5.3.6 nor in v5.4.0 (v#{Puppet::PUPPETVERSION} is installed)" unless is_device_apply_supported? device_conf.write(device_conf_content) device_conf.close + + device_credentials.write(device_credentials_content) + device_credentials.close end after(:each) do device_conf.unlink + device_credentials.unlink end context 'with no config specified' do diff --git a/spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb b/spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb new file mode 100644 index 00000000..5bd6ee45 --- /dev/null +++ b/spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb @@ -0,0 +1,8 @@ +require 'puppet/resource_api' + +Puppet::ResourceApi.register_transport( + name: 'test_device', # points at class Puppet::Transport::TestDevice + desc: 'Connects to a device', + connection_info: { + }, +) diff --git a/spec/fixtures/test_module/lib/puppet/transport/test_device.rb b/spec/fixtures/test_module/lib/puppet/transport/test_device.rb new file mode 100644 index 00000000..ad3a04f6 --- /dev/null +++ b/spec/fixtures/test_module/lib/puppet/transport/test_device.rb @@ -0,0 +1,21 @@ +module Puppet::Transport +# a transport for a test_device +class TestDevice + def initialize(connection_info); + puts connection_info + end + + def facts + { 'foo' => 'bar' } + end + + def verify + return true + end + + def close + return + end +end + +end diff --git a/spec/puppet/resource_api/transport/wrapper_spec.rb b/spec/puppet/resource_api/transport/wrapper_spec.rb new file mode 100644 index 00000000..f2692b19 --- /dev/null +++ b/spec/puppet/resource_api/transport/wrapper_spec.rb @@ -0,0 +1,116 @@ +require 'spec_helper' +require 'puppet/resource_api/transport/wrapper' +require_relative '../../../fixtures/test_module/lib/puppet/transport/test_device' + +RSpec.describe Puppet::ResourceApi::Transport::Wrapper, agent_test: true do + describe '#initialize(name, url_or_config)' do + context 'when called with a url' do + context 'with a file:// prefix' do + let(:url) { 'file:///etc/credentials' } + + it 'will not throw an error' do + allow(File).to receive(:exist?).and_return(true) + expect(Puppet::ResourceApi::Transport).to receive(:connect) + expect(Hocon).to receive(:load) + expect { described_class.new('wibble', url) }.not_to raise_error + end + end + + context 'with an http:// prefix' do + let(:url) { 'http://www.puppet.com' } + + it { expect { described_class.new('wibble', url) }.to raise_error RuntimeError, %r{Only file:/// URLs for configuration supported} } + end + end + + context 'when called with a config hash' do + let(:config) { {} } + + it 'will use the configuration directly' do + expect(Hocon).not_to receive(:load) + expect(Puppet::ResourceApi::Transport).to receive(:connect) + described_class.new('wibble', config) + end + end + end + + describe '#facts' do + context 'when called' do + let(:instance) { described_class.new('wibble', {}) } + let(:facts) { { 'foo' => 'bar' } } + let(:transport) { instance_double(Puppet::Transport::TestDevice, 'transport') } + + it 'will return the facts provided by the transport' do + allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) + allow(transport).to receive(:facts).and_return(facts) + + expect(instance.facts).to eq(facts) + end + end + end + + context 'when an unsupported method is called' do + context 'when the transport can handle the method' do + let(:instance) { described_class.new('wibble', {}) } + let(:transport) { instance_double(Puppet::Transport::TestDevice, 'transport') } + + it 'will return the facts provided by the transport' do + allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) + expect(transport).to receive(:close) + + instance.close + end + end + + context 'when the transport cannot handle the method' do + let(:instance) { described_class.new('wibble', {}) } + let(:transport) { instance_double(Puppet::Transport::TestDevice, 'transport') } + + it 'will raise a NoMethodError' do + allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) + expect { instance.wibble }.to raise_error NoMethodError + end + end + end + + context 'when a method is checked for' do + let(:instance) { described_class.new('wibble', {}) } + let(:transport) { instance_double(Puppet::Transport::TestDevice, 'transport') } + + before(:each) do + allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) + end + + context 'when the transport does not support the function' do + context 'when using respond_to?' do + it 'will return false' do + expect(instance.respond_to?(:wibble)).to eq(false) + end + end + + context 'when using method?' do + it 'will return false' do + expect { instance.method :wibble }.to raise_error + end + end + end + + context 'when the transport does support the function' do + before(:each) do + allow(transport).to receive(:close) + end + + context 'when using respond_to?' do + it 'will return true' do + expect(instance.respond_to?(:close)).to eq(true) + end + end + + context 'when using method?' do + it 'will return the method' do + expect(instance.method(:close)).to be_a(Method) + end + end + end + end +end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index f1f27d5a..3208900d 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -102,12 +102,12 @@ end context 'when the transport file does exist' do - context 'with and incorrectly defined transport' do + context 'with an incorrectly defined transport' do it 'throws a DevError' do expect(described_class).to receive(:validate).with(name, connection_info) expect(described_class).to receive(:require).with('puppet/transport/test_target') expect { described_class.connect(name, connection_info) }.to raise_error Puppet::DevError, - %r{uninitialized constant Puppet::Transport} + %r{uninitialized constant (Puppet::Transport|TestTarget)} end end From bc54be78eb76bf453873be93f0ee565286bdbd5b Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 22 Jan 2019 16:14:51 +0000 Subject: [PATCH 04/37] (FM-7691) start refactoring definition handling in contexts --- lib/puppet/resource_api/base_context.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/puppet/resource_api/base_context.rb b/lib/puppet/resource_api/base_context.rb index bf877680..220ee8f6 100644 --- a/lib/puppet/resource_api/base_context.rb +++ b/lib/puppet/resource_api/base_context.rb @@ -7,7 +7,6 @@ class Puppet::ResourceApi::BaseContext def initialize(definition) raise ArgumentError, 'BaseContext requires definition to be a Hash' unless definition.is_a?(Hash) - @typename = definition[:name] @type = Puppet::ResourceApi::TypeDefinition.new(definition) end @@ -27,7 +26,7 @@ def feature_support?(feature) [:debug, :info, :notice, :warning, :err].each do |level| define_method(level) do |*args| if args.length == 1 - message = "#{@context || @typename}: #{args.last}" + message = "#{@context || @type.name}: #{args.last}" elsif args.length == 2 resources = format_titles(args.first) message = "#{resources}: #{args.last}" @@ -137,9 +136,9 @@ def send_log(_level, _message) def format_titles(titles) if titles.length.zero? && !titles.is_a?(String) - @typename + @type.name else - "#{@typename}[#{[titles].flatten.compact.join(', ')}]" + "#{@type.name}[#{[titles].flatten.compact.join(', ')}]" end end From 5ecffccc3382a790cc4bdbbe000f22ee4b2b4796 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 22 Jan 2019 16:47:12 +0000 Subject: [PATCH 05/37] (FM-7691) Allow pre-fabbed BaseTypeDefinition as argument to BaseContext --- lib/puppet/resource_api/base_context.rb | 10 ++++++++-- spec/puppet/resource_api/base_context_spec.rb | 11 +++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/puppet/resource_api/base_context.rb b/lib/puppet/resource_api/base_context.rb index 220ee8f6..fa995648 100644 --- a/lib/puppet/resource_api/base_context.rb +++ b/lib/puppet/resource_api/base_context.rb @@ -6,8 +6,14 @@ class Puppet::ResourceApi::BaseContext attr_reader :type def initialize(definition) - raise ArgumentError, 'BaseContext requires definition to be a Hash' unless definition.is_a?(Hash) - @type = Puppet::ResourceApi::TypeDefinition.new(definition) + if definition.is_a?(Hash) + # this is only for backwards compatibility + @type = Puppet::ResourceApi::TypeDefinition.new(definition) + elsif definition.is_a? Puppet::ResourceApi::BaseTypeDefinition + @type = definition + else + raise ArgumentError, 'BaseContext requires definition to be a child of Puppet::ResourceApi::BaseTypeDefinition, not <%{actual_type}>' % { actual_type: definition.class } + end end def device diff --git a/spec/puppet/resource_api/base_context_spec.rb b/spec/puppet/resource_api/base_context_spec.rb index 34869f75..1c85129d 100644 --- a/spec/puppet/resource_api/base_context_spec.rb +++ b/spec/puppet/resource_api/base_context_spec.rb @@ -13,10 +13,17 @@ def send_log(log, msg) TestContext.new(definition) end - let(:definition) { { name: 'some_resource', attributes: { name: { type: 'String', desc: 'message' } }, features: feature_support } } + let(:definition_hash) { { name: 'some_resource', attributes: { name: { type: 'String', desc: 'message' } }, features: feature_support } } + let(:definition) { Puppet::ResourceApi::TypeDefinition.new(definition_hash) } let(:feature_support) { [] } - it { expect { described_class.new(nil) }.to raise_error ArgumentError, %r{BaseContext requires definition to be a Hash} } + it { expect { described_class.new(nil) }.to raise_error ArgumentError, %r{BaseContext requires definition to be a child of Puppet::ResourceApi::BaseTypeDefinition} } + describe 'legacy hash definition support' do + let(:definition) { definition_hash } + + it { expect { context }.not_to raise_error } + it { expect(context.type.name).to eq 'some_resource' } + end describe '#failed?' do it('defaults to false') { is_expected.not_to be_failed } From 9d549c05a109b121e123211e8e71fe411da70e16 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 22 Jan 2019 17:16:23 +0000 Subject: [PATCH 06/37] (FM-7691) use the new capability in resource_api.rb --- lib/puppet/resource_api.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 1d69ac0a..16623f6d 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -353,7 +353,7 @@ def cache_current_state(resource_hash) end define_singleton_method(:context) do - @context ||= PuppetContext.new(definition) + @context ||= PuppetContext.new(TypeDefinition.new(definition)) end def context From 76c6eeb6b936ca413d81ab57ee8471b9566b746a Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Wed, 23 Jan 2019 17:34:29 +0000 Subject: [PATCH 07/37] (FM-7691) Load the schema before trying to validate connection_info --- lib/puppet/resource_api/transport.rb | 1 + spec/puppet/resource_api/transport_spec.rb | 17 ++++++++++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 5858cd91..4586ed14 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -33,6 +33,7 @@ def connect(name, connection_info) def self.validate(name, connection_info) @transports ||= {} + require "puppet/transport/schema/#{name}" unless @transports.key? name transport_schema = @transports[name] raise Puppet::DevError, 'Transport for `%{target}` not registered' % { target: name } if transport_schema.nil? diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 3208900d..bc6fe0c2 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -129,19 +129,22 @@ describe '#self.validate' do context 'when the transport being validated has not be registered' do - it { expect { described_class.validate('wibble', {}) }.to raise_error Puppet::DevError, %r{Transport for `wibble` not registered} } + it { expect { described_class.validate('wibble', {}) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/schema/wibble} } end context 'when the transport being validated has been registered' do let(:schema) { { name: 'validate', desc: 'a minimal connection', connection_info: {} } } + let(:schema_def) { instance_double('Puppet::ResourceApi::TransportSchemaDef', 'schema_def') } + + it 'validates the connection_info' do + allow(Puppet::ResourceApi::TransportSchemaDef).to receive(:new).with(schema).and_return(schema_def) - it 'continues to validate the connection_info' do - # rubocop:disable RSpec/AnyInstance - expect_any_instance_of(Puppet::ResourceApi::TransportSchemaDef).to receive(:check_schema).with({}) - expect_any_instance_of(Puppet::ResourceApi::TransportSchemaDef).to receive(:validate).with({}) - # rubocop:enable RSpec/AnyInstance described_class.register(schema) - described_class.validate('validate', {}) + + expect(schema_def).to receive(:check_schema).with('connection_info').and_return(nil) + expect(schema_def).to receive(:validate).with('connection_info').and_return(nil) + + described_class.validate('validate', 'connection_info') end end end From b6cbdcf57f9be727543300c9ade3afa9815eb505 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Wed, 23 Jan 2019 17:38:02 +0000 Subject: [PATCH 08/37] (FM-7691) Fix tests to not rely on previously registered transports --- spec/puppet/resource_api/transport_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index bc6fe0c2..d79f9016 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -11,6 +11,11 @@ Puppet.debug = true end + after(:each) do + # reset registered transports between tests to reduce cross-test poisoning + described_class.instance_variable_set(:@transports, nil) + end + context 'when registering a schema with missing keys' do it { expect { described_class.register([]) }.to raise_error(Puppet::DevError, %r{requires a hash as schema}) } it { expect { described_class.register({}) }.to raise_error(Puppet::DevError, %r{requires a `:name`}) } @@ -25,7 +30,10 @@ it { expect { described_class.register(schema) }.not_to raise_error } context 'when re-registering a transport' do - it { expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) } + it { + described_class.register(schema) + expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) + } end end From c7f320374a3da2b20b3335f853c7c05c55e41008 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 10:50:30 +0000 Subject: [PATCH 09/37] (FM-7696) remove overzealous exception handling --- lib/puppet/resource_api/transport.rb | 10 +++------- spec/puppet/resource_api/transport_spec.rb | 9 ++++----- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 4586ed14..e1225d1b 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -21,13 +21,9 @@ def register(schema) def connect(name, connection_info) validate(name, connection_info) - begin - require "puppet/transport/#{name}" - class_name = name.split('_').map { |e| e.capitalize }.join - Puppet::Transport.const_get(class_name).new(connection_info) - rescue LoadError, NameError => detail - raise Puppet::DevError, 'Cannot load transport for `%{target}`: %{detail}' % { target: name, detail: detail } - end + require "puppet/transport/#{name}" + class_name = name.split('_').map { |e| e.capitalize }.join + Puppet::Transport.const_get(class_name).new(connection_info) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index d79f9016..ba1c8e82 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -102,19 +102,18 @@ end context 'when the transport file does not exist' do - it 'throws a DevError' do + it 'throws a LoadError' do expect(described_class).to receive(:validate).with(name, connection_info) - expect { described_class.connect(name, connection_info) }.to raise_error Puppet::DevError, - %r{Cannot load transport for `test_target`} + expect { described_class.connect(name, connection_info) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/test_target} end end context 'when the transport file does exist' do context 'with an incorrectly defined transport' do - it 'throws a DevError' do + it 'throws a NameError' do expect(described_class).to receive(:validate).with(name, connection_info) expect(described_class).to receive(:require).with('puppet/transport/test_target') - expect { described_class.connect(name, connection_info) }.to raise_error Puppet::DevError, + expect { described_class.connect(name, connection_info) }.to raise_error NameError, %r{uninitialized constant (Puppet::Transport|TestTarget)} end end From c46f50bb3a4b2e8de39aebac52c83cbfdd58fe5f Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 11:03:31 +0000 Subject: [PATCH 10/37] (FM-7691) fix connection_info/schema misnomer in tests --- spec/puppet/resource_api/transport_spec.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index ba1c8e82..0bd2fafa 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -88,7 +88,7 @@ context 'when connecting to a transport' do let(:name) { 'test_target' } - let(:connection_info) do + let(:schema) do { name: 'test_target', desc: 'a basic transport', @@ -103,18 +103,18 @@ context 'when the transport file does not exist' do it 'throws a LoadError' do - expect(described_class).to receive(:validate).with(name, connection_info) - expect { described_class.connect(name, connection_info) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/test_target} + expect(described_class).to receive(:validate).with(name, host: 'example.com') + expect { described_class.connect(name, host: 'example.com') }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/test_target} end end context 'when the transport file does exist' do context 'with an incorrectly defined transport' do it 'throws a NameError' do - expect(described_class).to receive(:validate).with(name, connection_info) + expect(described_class).to receive(:validate).with(name, host: 'example.com') expect(described_class).to receive(:require).with('puppet/transport/test_target') - expect { described_class.connect(name, connection_info) }.to raise_error NameError, - %r{uninitialized constant (Puppet::Transport|TestTarget)} + expect { described_class.connect(name, host: 'example.com') }.to raise_error NameError, + %r{uninitialized constant (Puppet::Transport|TestTarget)} end end @@ -123,12 +123,12 @@ it 'loads initiates the class successfully' do expect(described_class).to receive(:require).with('puppet/transport/test_target') - expect(described_class).to receive(:validate).with(name, connection_info) + expect(described_class).to receive(:validate).with(name, host: 'example.com') stub_const('Puppet::Transport::TestTarget', test_target) - expect(test_target).to receive(:new).with(connection_info) + expect(test_target).to receive(:new).with(host: 'example.com') - described_class.connect(name, connection_info) + described_class.connect(name, host: 'example.com') end end end From 9535fe36c58afdef0d7428a60339a85fceeb09e6 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 11:08:52 +0000 Subject: [PATCH 11/37] (FM-7691) create and pass through context when connecting a transport --- lib/puppet/resource_api/transport.rb | 9 +++------ spec/puppet/resource_api/transport_spec.rb | 8 +++++++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index e1225d1b..d31038a4 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -1,8 +1,4 @@ -# rubocop:disable Style/Documentation -module Puppet; end -module Puppet::ResourceApi; end -module Puppet::ResourceApi::Transport; end -# rubocop:enable Style/Documentation +require 'puppet/resource_api/puppet_context' # Remote target transport API module Puppet::ResourceApi::Transport @@ -23,7 +19,8 @@ def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" class_name = name.split('_').map { |e| e.capitalize }.join - Puppet::Transport.const_get(class_name).new(connection_info) + context = Puppet::ResourceApi::PuppetContext.new(@transports[name]) + Puppet::Transport.const_get(class_name).new(context, connection_info) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 0bd2fafa..ed7d3953 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -111,6 +111,8 @@ context 'when the transport file does exist' do context 'with an incorrectly defined transport' do it 'throws a NameError' do + described_class.register(schema) + expect(described_class).to receive(:validate).with(name, host: 'example.com') expect(described_class).to receive(:require).with('puppet/transport/test_target') expect { described_class.connect(name, host: 'example.com') }.to raise_error NameError, @@ -120,13 +122,17 @@ context 'with a correctly defined transport' do let(:test_target) { double('Puppet::Transport::TestTarget') } # rubocop:disable RSpec/VerifiedDoubles + let(:context) { instance_double(Puppet::ResourceApi::PuppetContext, 'context') } it 'loads initiates the class successfully' do + described_class.register(schema) + expect(described_class).to receive(:require).with('puppet/transport/test_target') expect(described_class).to receive(:validate).with(name, host: 'example.com') + expect(Puppet::ResourceApi::PuppetContext).to receive(:new).with(kind_of(Puppet::ResourceApi::TransportSchemaDef)).and_return(context) stub_const('Puppet::Transport::TestTarget', test_target) - expect(test_target).to receive(:new).with(host: 'example.com') + expect(test_target).to receive(:new).with(context, host: 'example.com') described_class.connect(name, host: 'example.com') end From f8282d215cf6e8dfda940b2d9ca2d820405264ab Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 11:27:12 +0000 Subject: [PATCH 12/37] (FM-7691) clean up wrapper to not expose or remember config Neither the Device API, nor the wrapper need access to the config, so we can immediately load the config, pass it on and never store it ourselves here. --- lib/puppet/resource_api/transport/wrapper.rb | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index 97fe4fe0..df4f7ba3 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -8,21 +8,17 @@ class Puppet::ResourceApi::Transport::Wrapper def initialize(name, url_or_config) if url_or_config.is_a? String - @url = URI.parse(url_or_config) - raise "Unexpected url '#{url_or_config}' found. Only file:/// URLs for configuration supported at the moment." unless @url.scheme == 'file' + url = URI.parse(url_or_config) + raise "Unexpected url '#{url_or_config}' found. Only file:/// URLs for configuration supported at the moment." unless url.scheme == 'file' + raise "Trying to load config from '#{url.path}, but file does not exist." if url && !File.exist?(url.path) + config = (Hocon.load(url.path, syntax: Hocon::ConfigSyntax::HOCON) || {}).map { |k, v| [k.to_sym, v] }.to_h else - @config = url_or_config + config = url_or_config end @transport = Puppet::ResourceApi::Transport.connect(name, config) end - def config - raise "Trying to load config from '#{@url.path}, but file does not exist." if @url && !File.exist?(@url.path) - # symbolize top-level keys to match up with expected provider/resource data conventions - @config ||= (Hocon.load(@url.path, syntax: Hocon::ConfigSyntax::HOCON) || {}).map { |k, v| [k.to_sym, v] }.to_h - end - def facts # @transport.facts + custom_facts # look into custom facts work by TP @transport.facts From f8fcf793dc06cc50d12c2e86905c4f019e59c065 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 13:15:23 +0000 Subject: [PATCH 13/37] (FM-7691) hide loading puppet_context from jruby the JRuby 1.7 only implements ruby language 1.9 which chokes on the logging named arguments. Hiding the require as shown here will avoid the file being loaded for the server-side tests. --- lib/puppet/resource_api/transport.rb | 10 ++++++---- spec/puppet/resource_api/transport_spec.rb | 1 + 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index d31038a4..88aa44e5 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -1,5 +1,3 @@ -require 'puppet/resource_api/puppet_context' - # Remote target transport API module Puppet::ResourceApi::Transport def register(schema) @@ -19,8 +17,7 @@ def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" class_name = name.split('_').map { |e| e.capitalize }.join - context = Puppet::ResourceApi::PuppetContext.new(@transports[name]) - Puppet::Transport.const_get(class_name).new(context, connection_info) + Puppet::Transport.const_get(class_name).new(get_context(name), connection_info) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations @@ -33,4 +30,9 @@ def self.validate(name, connection_info) transport_schema.check_schema(connection_info) transport_schema.validate(connection_info) end + + def self.get_context(name) + require 'puppet/resource_api/puppet_context' + Puppet::ResourceApi::PuppetContext.new(@transports[name]) + end end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index ed7d3953..aca163ed 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -127,6 +127,7 @@ it 'loads initiates the class successfully' do described_class.register(schema) + allow(described_class).to receive(:require).with('puppet/resource_api/puppet_context').and_call_original expect(described_class).to receive(:require).with('puppet/transport/test_target') expect(described_class).to receive(:validate).with(name, host: 'example.com') expect(Puppet::ResourceApi::PuppetContext).to receive(:new).with(kind_of(Puppet::ResourceApi::TransportSchemaDef)).and_return(context) From fc4f585c4fef64d186bd578f17a995534753044a Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 13:25:56 +0000 Subject: [PATCH 14/37] (maint) update notifications to talk to slack --- .travis.yml | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/.travis.yml b/.travis.yml index b2619621..978c4106 100644 --- a/.travis.yml +++ b/.travis.yml @@ -65,10 +65,5 @@ matrix: - rvm: 2.5.1 env: PUPPET_GEM_VERSION='https://github.com/puppetlabs/puppet.git#6.0.x' notifications: - hipchat: - rooms: - secure: 10a49kkZcghKHNnef8x7eBG+KjScL3i1VpygFg6DPAOK2YNbEoyEx1Kv9KLC7GSRYov/SQZOsZrvHZtDhEtFSKhhiAjOwxl1jV1t6aAMGMnN1IoZBOvdAJKrZsm54/bBeYp+je2wqnnoFNtLVFSoOX0LkFaDEWT+zGZ5xKJIH25GpeQEZf1eDxs/d8YX/m+RwbGXHVA//hOpvZo0ntvznh2EbW5OPODKSeUXbWZ+W4ndODTsKWFc/WLMSSgFDzW/Y2/9V40D4IC8lvSx6eKFryMfAQy6pO/d1aTB468awzyVcdYAMMCOITm7hlKGRKxNgq6NkOsXs5KLg6ifpn+a/Rhapbz6Qxbpjjho/7Wxngl4B3T+i35ap/mFrS/fOfKCq3gEQlYn29its9bEFArNGbr+/sXKABb+sRpgW4RTPWYDHJyHJendbevd5tZ+fd0JUBOi0Cb4PcXxQxM8IQrbuu2zso0K5MV05kL0S1DE/VsuUrPaK0RsF+b1+i6NfvtN8kgbYs1eiVku+guIG2ec3xIefQ1hsEOFPFNqSDfHp7nANnRVIbBCt8qw8DhmNEczsfN5Dp21euJUsO9qpau++NzD3jRhkE5Zki5cwsakU7hIQzw82BIb0eSQJCHygieExeEboWRqtDgy/IKIWPgIvEuU68ppR2bl2reKCHLCnWc= - template: - - '%{repository}#%{build_number} (%{branch} - %{commit} : %{author}): %{message} - (Details | PR/Diff)' - format: html + slack: + secure: aPXZYNow8LsmmlS8PQU3FjL0bc7FqUUA95d++wZfIu7YAjGboIUiekxYouQ0XnY+Aig8InvbTOIgBHgGNheyr/YFbFS90/jtulbF8oW7BitW+imgjeAHDCwlQZTCc4FFYde/2pI7QTT8H5NpLR9mKxlTU77Sqr8gFAIybuPdHcKMYQZdEZS07ma2pUp7+GyKS6PDQpzW2+mDCz/wfi3/JdsUvc0mclCZ8vxySc66j5P1E6nFDMzuakBOjwJHpgeDpreapbmSUQLAX0a3ZsFP+N+SNduLotlV2BWnJK2gcO6rGFP4Fz1D0bGXuBnYYdIiB+9OgI3wtXg9y1SifNHUG3IrOBAA8CGNyrebTGKtH0TS2O+HZLbaNX2g6udD5e3156vys9wScmJuQ/rSkVtQfXf1qUm5eijvlXI+DIbssbZHqm6QQGyM4p3NoULmNmF1C85bQoZ4GF7b1P/8mstsVE/HUfnzRPNbwD0r6j1aE/ck3PKMi7ZAhIi0Ja9RnAgP3wi0t62uERYcJGGYEycWohMWnrf2w6GFwGeuoiwAkASdHOLX0/AOMPc4mBOjlc621o8uYMrrZqfF5CrOAvJ151USSsWn2AhXaibIvnHo6X91paNvvNpU/GYu3CUAl6q8OhYovvjtRVPVnhs2DrpgoRB+6NWHnzjRG/wr6Z9U+vA= From 5d89caf5c1f1e1fbca9e80366116b7bc88d04fbf Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 13:31:12 +0000 Subject: [PATCH 15/37] (FM-7691) organise tests, and mark agent tests as such Loading a PuppetContext is not necessary on (the JRuby 1.7) puppet server, so stop trying to run the tests there. --- spec/puppet/resource_api/transport_spec.rb | 118 +++++++++++---------- 1 file changed, 60 insertions(+), 58 deletions(-) diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index aca163ed..7ad393ef 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -16,77 +16,79 @@ described_class.instance_variable_set(:@transports, nil) end - context 'when registering a schema with missing keys' do - it { expect { described_class.register([]) }.to raise_error(Puppet::DevError, %r{requires a hash as schema}) } - it { expect { described_class.register({}) }.to raise_error(Puppet::DevError, %r{requires a `:name`}) } - it { expect { described_class.register(name: 'no connection info', desc: 'some description') }.to raise_error(Puppet::DevError, %r{requires `:connection_info`}) } - it { expect { described_class.register(name: 'no description') }.to raise_error(Puppet::DevError, %r{requires `:desc`}) } - it { expect { described_class.register(name: 'no hash attributes', desc: 'some description', connection_info: []) }.to raise_error(Puppet::DevError, %r{`:connection_info` must be a hash, not}) } - end + describe '#register(schema)' do + context 'when registering a schema with missing keys' do + it { expect { described_class.register([]) }.to raise_error(Puppet::DevError, %r{requires a hash as schema}) } + it { expect { described_class.register({}) }.to raise_error(Puppet::DevError, %r{requires a `:name`}) } + it { expect { described_class.register(name: 'no connection info', desc: 'some description') }.to raise_error(Puppet::DevError, %r{requires `:connection_info`}) } + it { expect { described_class.register(name: 'no description') }.to raise_error(Puppet::DevError, %r{requires `:desc`}) } + it { expect { described_class.register(name: 'no hash attributes', desc: 'some description', connection_info: []) }.to raise_error(Puppet::DevError, %r{`:connection_info` must be a hash, not}) } + end - context 'when registering a minimal transport' do - let(:schema) { { name: 'minimal', desc: 'a minimal connection', connection_info: {} } } + context 'when registering a minimal transport' do + let(:schema) { { name: 'minimal', desc: 'a minimal connection', connection_info: {} } } - it { expect { described_class.register(schema) }.not_to raise_error } + it { expect { described_class.register(schema) }.not_to raise_error } - context 'when re-registering a transport' do - it { - described_class.register(schema) - expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) - } + context 'when re-registering a transport' do + it { + described_class.register(schema) + expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) + } + end end - end - context 'when registering a transport' do - let(:schema) do - { - name: 'a_remote_thing', - desc: 'basic transport', - connection_info: { - host: { - type: 'String', - desc: 'the host ip address or hostname', - }, - user: { - type: 'String', - desc: 'the user to connect as', - }, - password: { - type: 'String', - sensitive: true, - desc: 'the password to make the connection', + context 'when registering a transport' do + let(:schema) do + { + name: 'a_remote_thing', + desc: 'basic transport', + connection_info: { + host: { + type: 'String', + desc: 'the host ip address or hostname', + }, + user: { + type: 'String', + desc: 'the user to connect as', + }, + password: { + type: 'String', + sensitive: true, + desc: 'the password to make the connection', + }, }, - }, - } - end + } + end - it 'adds to the transports register' do - expect { described_class.register(schema) }.not_to raise_error + it 'adds to the transports register' do + expect { described_class.register(schema) }.not_to raise_error + end end - end - context 'when registering a transport with a bad type' do - let(:schema) do - { - name: 'a_bad_thing', - desc: 'basic transport', - connection_info: { - host: { - type: 'garbage', - desc: 'the host ip address or hostname', + context 'when registering a transport with a bad type' do + let(:schema) do + { + name: 'a_bad_thing', + desc: 'basic transport', + connection_info: { + host: { + type: 'garbage', + desc: 'the host ip address or hostname', + }, }, - }, + } + end + + it { + expect { described_class.register(schema) }.to raise_error( + Puppet::DevError, %r{ is not a valid type specification} + ) } end - - it { - expect { described_class.register(schema) }.to raise_error( - Puppet::DevError, %r{ is not a valid type specification} - ) - } end - context 'when connecting to a transport' do + describe '#connect(name, connection_info)', agent_test: true do let(:name) { 'test_target' } let(:schema) do { @@ -141,7 +143,7 @@ end end - describe '#self.validate' do + describe '#validate(name, connection_info)', agent_test: true do context 'when the transport being validated has not be registered' do it { expect { described_class.validate('wibble', {}) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/schema/wibble} } end From 8aaedab16babca938b9016bf7e5416a98eedbc91 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 15:56:17 +0000 Subject: [PATCH 16/37] (maint) Add some docs and remove the Style/Documentation exclusions --- .rubocop.yml | 3 +-- .rubocop_todo.yml | 17 ----------------- lib/puppet/resource_api.rb | 1 + lib/puppet/resource_api/base_context.rb | 5 +++++ lib/puppet/resource_api/io_context.rb | 2 ++ lib/puppet/resource_api/puppet_context.rb | 2 ++ lib/puppet/resource_api/transport.rb | 5 +++++ 7 files changed, 16 insertions(+), 19 deletions(-) delete mode 100644 .rubocop_todo.yml diff --git a/.rubocop.yml b/.rubocop.yml index 6d831a74..5457a9ea 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,6 +1,5 @@ --- require: rubocop-rspec -inherit_from: .rubocop_todo.yml AllCops: TargetRubyVersion: '2.1' Include: @@ -157,4 +156,4 @@ Naming/UncommunicativeMethodParamName: # This cop breaks syntax highlighting in VSCode Layout/ClosingHeredocIndentation: - Enabled: false \ No newline at end of file + Enabled: false diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml deleted file mode 100644 index 63295575..00000000 --- a/.rubocop_todo.yml +++ /dev/null @@ -1,17 +0,0 @@ -# This configuration was generated by -# `rubocop --auto-gen-config` -# on 2017-09-05 11:31:11 +0100 using RuboCop version 0.49.1. -# The point is for the user to remove these configuration records -# one by one as the offenses are removed from the code base. -# Note that changes in the inspected code, or installation of new -# versions of RuboCop, may require this file to be generated again. - -# Offense count: 6 -Style/Documentation: - Exclude: - - 'spec/**/*' - - 'test/**/*' - - 'lib/puppet/resource_api.rb' - - 'lib/puppet/resource_api/base_context.rb' - - 'lib/puppet/resource_api/io_context.rb' - - 'lib/puppet/resource_api/puppet_context.rb' diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 16623f6d..870eb952 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -12,6 +12,7 @@ require 'puppet/type' require 'puppet/util/network_device' +# This module contains the main API to register and access types, providers and transports. module Puppet::ResourceApi @warning_count = 0 diff --git a/lib/puppet/resource_api/base_context.rb b/lib/puppet/resource_api/base_context.rb index fa995648..910a7d81 100644 --- a/lib/puppet/resource_api/base_context.rb +++ b/lib/puppet/resource_api/base_context.rb @@ -1,7 +1,12 @@ require 'puppet/resource_api/type_definition' +# rubocop:disable Style/Documentation module Puppet; end module Puppet::ResourceApi; end +# rubocop:enable Style/Documentation + +# This class provides access to all common external dependencies of providers and transports. +# The runtime environment will inject an appropriate implementation. class Puppet::ResourceApi::BaseContext attr_reader :type diff --git a/lib/puppet/resource_api/io_context.rb b/lib/puppet/resource_api/io_context.rb index 3cfb4afb..d31143c4 100644 --- a/lib/puppet/resource_api/io_context.rb +++ b/lib/puppet/resource_api/io_context.rb @@ -1,5 +1,7 @@ require 'puppet/resource_api/base_context' +# Implement Resource API Conext to log through an IO object, defaulting to `$stderr`. +# There is no access to a device here. class Puppet::ResourceApi::IOContext < Puppet::ResourceApi::BaseContext def initialize(definition, target = $stderr) super(definition) diff --git a/lib/puppet/resource_api/puppet_context.rb b/lib/puppet/resource_api/puppet_context.rb index 770c7692..6dd23a51 100644 --- a/lib/puppet/resource_api/puppet_context.rb +++ b/lib/puppet/resource_api/puppet_context.rb @@ -1,6 +1,8 @@ require 'puppet/resource_api/base_context' require 'puppet/util/logging' +# Implement Resource API Context to log through Puppet facilities +# and access/expose the puppet process' current device class Puppet::ResourceApi::PuppetContext < Puppet::ResourceApi::BaseContext def device # TODO: evaluate facter_url setting for loading config if there is no `current` NetworkDevice diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 88aa44e5..f50412e9 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -1,3 +1,8 @@ +# rubocop:disable Style/Documentation +module Puppet; end +module Puppet::ResourceApi; end +# rubocop:enable Style/Documentation + # Remote target transport API module Puppet::ResourceApi::Transport def register(schema) From bd3a21e0abd825f55ef928189d5ebb8a256b7c6c Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 16:32:34 +0000 Subject: [PATCH 17/37] (FM-7700) Add a `list` operation returning registered transports --- lib/puppet/resource_api/transport.rb | 10 +++++++ spec/puppet/resource_api/transport_spec.rb | 32 ++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index f50412e9..107d002a 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -18,6 +18,16 @@ def register(schema) end module_function :register # rubocop:disable Style/AccessModifierDeclarations + # retrieve a Hash of transport schemas, keyed by their name. + def list + if @transports + Marshal.load(Marshal.dump(@transports)) + else + {} + end + end + module_function :list # rubocop:disable Style/AccessModifierDeclarations + def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 7ad393ef..c9a748a9 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -88,6 +88,38 @@ end end + describe '#list' do + subject { described_class.list } + + context 'with no transports registered' do + it { is_expected.to eq({}) } + end + + context 'with a transport registered' do + let(:schema) do + { + name: 'test_target', + desc: 'a basic transport', + connection_info: { + host: { + type: 'String', + desc: 'the host ip address or hostname', + }, + }, + } + end + + before(:each) do + described_class.register(schema) + end + + it { expect(described_class.list['test_target'].definition).to eq schema } + it 'returns a new object' do + expect(described_class.list['test_target'].definition.object_id).not_to eq schema.object_id + end + end + end + describe '#connect(name, connection_info)', agent_test: true do let(:name) { 'test_target' } let(:schema) do From ac037b2945ded3837229a386685428c32939b62b Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Fri, 25 Jan 2019 09:10:33 +0000 Subject: [PATCH 18/37] (FM-7691) Add `context` parameter for test transport This find other places where we need to handle passing a context --- lib/puppet/resource_api/transport/wrapper.rb | 2 +- .../test_module/lib/puppet/transport/test_device.rb | 8 ++++---- spec/puppet/resource_api/transport/wrapper_spec.rb | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index df4f7ba3..caabc5af 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -21,7 +21,7 @@ def initialize(name, url_or_config) def facts # @transport.facts + custom_facts # look into custom facts work by TP - @transport.facts + @transport.facts(nil) end def respond_to_missing?(name, _include_private) diff --git a/spec/fixtures/test_module/lib/puppet/transport/test_device.rb b/spec/fixtures/test_module/lib/puppet/transport/test_device.rb index ad3a04f6..7974f1d4 100644 --- a/spec/fixtures/test_module/lib/puppet/transport/test_device.rb +++ b/spec/fixtures/test_module/lib/puppet/transport/test_device.rb @@ -1,19 +1,19 @@ module Puppet::Transport # a transport for a test_device class TestDevice - def initialize(connection_info); + def initialize(_context, connection_info); puts connection_info end - def facts + def facts(_context) { 'foo' => 'bar' } end - def verify + def verify(_context) return true end - def close + def close(_context) return end end diff --git a/spec/puppet/resource_api/transport/wrapper_spec.rb b/spec/puppet/resource_api/transport/wrapper_spec.rb index f2692b19..b5757a7a 100644 --- a/spec/puppet/resource_api/transport/wrapper_spec.rb +++ b/spec/puppet/resource_api/transport/wrapper_spec.rb @@ -42,7 +42,7 @@ it 'will return the facts provided by the transport' do allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) - allow(transport).to receive(:facts).and_return(facts) + allow(transport).to receive(:facts).with(nil).and_return(facts) expect(instance.facts).to eq(facts) end @@ -53,12 +53,13 @@ context 'when the transport can handle the method' do let(:instance) { described_class.new('wibble', {}) } let(:transport) { instance_double(Puppet::Transport::TestDevice, 'transport') } + let(:context) { instance_double(Puppet::ResourceApi::PuppetContext, 'context') } it 'will return the facts provided by the transport' do allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) expect(transport).to receive(:close) - instance.close + instance.close(context) end end From e4e8cbef7b52648d48d707916d4ee1ee2fe8ff00 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Fri, 25 Jan 2019 09:25:33 +0000 Subject: [PATCH 19/37] (FM-7691) pass a context to transport.facts This uses the initial `list` implementation from the previous commit to access the schema to create a context to pass around. --- lib/puppet/resource_api/transport/wrapper.rb | 4 +++- spec/puppet/resource_api/transport/wrapper_spec.rb | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index caabc5af..81febbcb 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -17,11 +17,13 @@ def initialize(name, url_or_config) end @transport = Puppet::ResourceApi::Transport.connect(name, config) + @schema = Puppet::ResourceApi::Transport.list[name] end def facts + context = Puppet::ResourceApi::PuppetContext.new(@schema) # @transport.facts + custom_facts # look into custom facts work by TP - @transport.facts(nil) + @transport.facts(context) end def respond_to_missing?(name, _include_private) diff --git a/spec/puppet/resource_api/transport/wrapper_spec.rb b/spec/puppet/resource_api/transport/wrapper_spec.rb index b5757a7a..b6652424 100644 --- a/spec/puppet/resource_api/transport/wrapper_spec.rb +++ b/spec/puppet/resource_api/transport/wrapper_spec.rb @@ -37,12 +37,15 @@ describe '#facts' do context 'when called' do let(:instance) { described_class.new('wibble', {}) } + let(:context) { instance_double(Puppet::ResourceApi::PuppetContext, 'context') } let(:facts) { { 'foo' => 'bar' } } let(:transport) { instance_double(Puppet::Transport::TestDevice, 'transport') } it 'will return the facts provided by the transport' do allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) - allow(transport).to receive(:facts).with(nil).and_return(facts) + allow(Puppet::ResourceApi::Transport).to receive(:list).and_return(schema: :dummy) + allow(Puppet::ResourceApi::PuppetContext).to receive(:new).and_return(context) + allow(transport).to receive(:facts).with(context).and_return(facts) expect(instance.facts).to eq(facts) end From 09fc8566a0a0ddca41cafde639f00f0543ee11a1 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Fri, 25 Jan 2019 12:24:58 +0000 Subject: [PATCH 20/37] (FM-7690) Update transports cache to be grouped by environment --- lib/puppet/resource_api/transport.rb | 50 ++++++---- spec/puppet/resource_api/transport_spec.rb | 106 ++++++++++++++++++++- 2 files changed, 138 insertions(+), 18 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 107d002a..eb0d7efc 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -1,7 +1,4 @@ -# rubocop:disable Style/Documentation -module Puppet; end -module Puppet::ResourceApi; end -# rubocop:enable Style/Documentation +module Puppet::ResourceApi; end # rubocop:disable Style/Documentation # Remote target transport API module Puppet::ResourceApi::Transport @@ -12,19 +9,21 @@ def register(schema) raise Puppet::DevError, 'requires `:connection_info`' unless schema.key? :connection_info raise Puppet::DevError, '`:connection_info` must be a hash, not `%{other_type}`' % { other_type: schema[:connection_info].class } unless schema[:connection_info].is_a?(Hash) - @transports ||= {} - raise Puppet::DevError, 'Transport `%{name}` is already registered.' % { name: schema[:name] } unless @transports[schema[:name]].nil? - @transports[schema[:name]] = Puppet::ResourceApi::TransportSchemaDef.new(schema) + init_transports + unless @transports[@environment][schema[:name]].nil? + raise Puppet::DevError, 'Transport `%{name}` is already registered for `%{environment}`' % { + name: schema[:name], + environment: @environment, + } + end + @transports[@environment][schema[:name]] = Puppet::ResourceApi::TransportSchemaDef.new(schema) end module_function :register # rubocop:disable Style/AccessModifierDeclarations # retrieve a Hash of transport schemas, keyed by their name. def list - if @transports - Marshal.load(Marshal.dump(@transports)) - else - {} - end + init_transports + Marshal.load(Marshal.dump(@transports[@environment])) end module_function :list # rubocop:disable Style/AccessModifierDeclarations @@ -37,17 +36,36 @@ def connect(name, connection_info) module_function :connect # rubocop:disable Style/AccessModifierDeclarations def self.validate(name, connection_info) - @transports ||= {} + init_transports require "puppet/transport/schema/#{name}" unless @transports.key? name - transport_schema = @transports[name] - raise Puppet::DevError, 'Transport for `%{target}` not registered' % { target: name } if transport_schema.nil? + transport_schema = @transports[@environment][name] + if transport_schema.nil? + raise Puppet::DevError, 'Transport for `%{target}` not registered with `%{environment}`' % { + target: name, + environment: @environment, + } + end transport_schema.check_schema(connection_info) transport_schema.validate(connection_info) end + private_class_method :validate def self.get_context(name) require 'puppet/resource_api/puppet_context' - Puppet::ResourceApi::PuppetContext.new(@transports[name]) + Puppet::ResourceApi::PuppetContext.new(@transports[@environment][name]) + end + private_class_method :get_context + + def self.init_transports + lookup = Puppet.lookup(:current_environment) if Puppet.respond_to? :lookup + @environment = if lookup.nil? + :transports_default + else + lookup.name + end + @transports ||= {} + @transports[@environment] ||= {} end + private_class_method :init_transports end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index c9a748a9..be019e97 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -1,6 +1,24 @@ require 'spec_helper' RSpec.describe Puppet::ResourceApi::Transport do + def change_environment(name = nil) + environment = class_double(Puppet::Node::Environment) + + if name.nil? + allow(Puppet).to receive(:respond_to?).and_return(false) + else + allow(Puppet).to receive(:respond_to?).and_return(true) + end + + allow(Puppet).to receive(:lookup).with(:current_environment).and_return(environment) + + # allow clean up scripts to run unhindered + allow(Puppet).to receive(:lookup).with(:root_environment).and_call_original + allow(Puppet).to receive(:lookup).with(:environments).and_call_original + + allow(environment).to receive(:name).and_return(name) + end + let(:strict_level) { :error } before(:each) do @@ -60,10 +78,64 @@ }, } 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 transports are added to multiple environments' do + it 'will record the schemas in the correct structure' do + change_environment(:wibble) + described_class.register(schema) + expect(described_class.instance_variable_get(:@transports)).to be_key(:wibble) + expect(described_class.instance_variable_get(:@transports)[:wibble][schema[:name]]).to be_a_kind_of(Puppet::ResourceApi::TransportSchemaDef) + expect(described_class.instance_variable_get(:@transports)[:wibble][schema[:name]].definition).to eq(schema) + + change_environment(:foo) + described_class.register(schema) + described_class.register(schema2) + expect(described_class.instance_variable_get(:@transports)).to be_key(:foo) + expect(described_class.instance_variable_get(:@transports)[:foo][schema[:name]]).to be_a_kind_of(Puppet::ResourceApi::TransportSchemaDef) + expect(described_class.instance_variable_get(:@transports)[:foo][schema[:name]].definition).to eq(schema) + expect(described_class.instance_variable_get(:@transports)[:foo][schema2[:name]]).to be_a_kind_of(Puppet::ResourceApi::TransportSchemaDef) + expect(described_class.instance_variable_get(:@transports)[:foo][schema2[:name]].definition).to eq(schema2) + + change_environment(:bar) + described_class.register(schema3) + expect(described_class.instance_variable_get(:@transports)).to be_key(:bar) + expect(described_class.instance_variable_get(:@transports)[:bar][schema3[:name]]).to be_a_kind_of(Puppet::ResourceApi::TransportSchemaDef) + expect(described_class.instance_variable_get(:@transports)[:bar][schema3[:name]].definition).to eq(schema3) + end + end end context 'when registering a transport with a bad type' do @@ -176,8 +248,15 @@ end describe '#validate(name, connection_info)', agent_test: true do + context 'when the transport does not exist' do + it { expect { described_class.send(:validate, 'wibble', {}) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/schema/wibble} } + end + context 'when the transport being validated has not be registered' do - it { expect { described_class.validate('wibble', {}) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/schema/wibble} } + it 'will throw an unregistered error message' do + expect(described_class).to receive(:require).with('puppet/transport/schema/wibble') + expect { described_class.send(:validate, 'wibble', {}) }.to raise_error Puppet::DevError, %r{ not registered with } + end end context 'when the transport being validated has been registered' do @@ -189,10 +268,33 @@ described_class.register(schema) + expect(described_class).to receive(:require).with('puppet/transport/schema/validate') expect(schema_def).to receive(:check_schema).with('connection_info').and_return(nil) expect(schema_def).to receive(:validate).with('connection_info').and_return(nil) - described_class.validate('validate', 'connection_info') + described_class.send :validate, 'validate', 'connection_info' + end + end + end + + describe '#init_transports' do + context 'when there is not a current_environment' do + it 'will return the default transport environment name' do + change_environment + + described_class.send :init_transports + + expect(described_class.instance_variable_get(:@environment)).to eq(:transports_default) + end + end + + context 'when there is a current_environment' do + it 'will return the set environment name' do + change_environment(:something) + + described_class.send :init_transports + + expect(described_class.instance_variable_get(:@environment)).to eq(:something) end end end From bd042cf576c6f8defb796a16a191df32b09f1280 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 29 Jan 2019 14:49:19 +0000 Subject: [PATCH 21/37] (FM-7726) implement `context.transport` to provide access Using this accessor providers can talk directly to the transport without having to go through the `Device` at all. --- lib/puppet/resource_api/base_context.rb | 4 ++++ lib/puppet/resource_api/io_context.rb | 7 +++++-- lib/puppet/resource_api/puppet_context.rb | 6 +++++- lib/puppet/resource_api/transport/wrapper.rb | 2 +- .../util/network_device/test_device/device.rb | 10 +++++----- spec/puppet/resource_api/base_context_spec.rb | 4 ++++ spec/puppet/resource_api/io_context_spec.rb | 12 +++++++++++- spec/puppet/resource_api/puppet_context_spec.rb | 16 +++++++++++++++- 8 files changed, 50 insertions(+), 11 deletions(-) diff --git a/lib/puppet/resource_api/base_context.rb b/lib/puppet/resource_api/base_context.rb index 910a7d81..1c44d1f0 100644 --- a/lib/puppet/resource_api/base_context.rb +++ b/lib/puppet/resource_api/base_context.rb @@ -25,6 +25,10 @@ def device raise 'Received device() on an unprepared BaseContext. Use a PuppetContext instead.' end + def transport + raise 'No transport available.' + end + def failed? @failed end diff --git a/lib/puppet/resource_api/io_context.rb b/lib/puppet/resource_api/io_context.rb index d31143c4..3efdde63 100644 --- a/lib/puppet/resource_api/io_context.rb +++ b/lib/puppet/resource_api/io_context.rb @@ -1,11 +1,14 @@ require 'puppet/resource_api/base_context' # Implement Resource API Conext to log through an IO object, defaulting to `$stderr`. -# There is no access to a device here. +# There is no access to a device here. You can supply a transport if necessary. class Puppet::ResourceApi::IOContext < Puppet::ResourceApi::BaseContext - def initialize(definition, target = $stderr) + attr_reader :transport + + def initialize(definition, target = $stderr, transport = nil) super(definition) @target = target + @transport = transport end protected diff --git a/lib/puppet/resource_api/puppet_context.rb b/lib/puppet/resource_api/puppet_context.rb index 6dd23a51..be057bcc 100644 --- a/lib/puppet/resource_api/puppet_context.rb +++ b/lib/puppet/resource_api/puppet_context.rb @@ -2,7 +2,7 @@ require 'puppet/util/logging' # Implement Resource API Context to log through Puppet facilities -# and access/expose the puppet process' current device +# and access/expose the puppet process' current device/transport class Puppet::ResourceApi::PuppetContext < Puppet::ResourceApi::BaseContext def device # TODO: evaluate facter_url setting for loading config if there is no `current` NetworkDevice @@ -10,6 +10,10 @@ def device Puppet::Util::NetworkDevice.current end + def transport + device.transport + end + def log_exception(exception, message: 'Error encountered', trace: false) super(exception, message: message, trace: trace || Puppet[:trace]) end diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index 81febbcb..a8b102d2 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -4,7 +4,7 @@ # Puppet::ResourceApi::Transport::Wrapper` to interface between the Util::NetworkDevice class Puppet::ResourceApi::Transport::Wrapper - attr_reader :transport + attr_reader :transport, :schema def initialize(name, url_or_config) if url_or_config.is_a? String diff --git a/spec/fixtures/test_module/lib/puppet/util/network_device/test_device/device.rb b/spec/fixtures/test_module/lib/puppet/util/network_device/test_device/device.rb index 7fb6ebd6..505209a5 100644 --- a/spec/fixtures/test_module/lib/puppet/util/network_device/test_device/device.rb +++ b/spec/fixtures/test_module/lib/puppet/util/network_device/test_device/device.rb @@ -1,10 +1,10 @@ -require 'puppet/util/network_device/simple/device' +require 'puppet/resource_api/transport/wrapper' module Puppet::Util::NetworkDevice::Test_device # rubocop:disable Style/ClassAndModuleCamelCase - # A simple test device returning hardcoded facts - class Device < Puppet::Util::NetworkDevice::Simple::Device - def facts - { 'foo' => 'bar' } + class Device < Puppet::ResourceApi::Transport::Wrapper + def initialize(url_or_config, _options = {}) + puts url_or_config.inspect + super('test_device', url_or_config) end end end diff --git a/spec/puppet/resource_api/base_context_spec.rb b/spec/puppet/resource_api/base_context_spec.rb index 1c85129d..9569faf3 100644 --- a/spec/puppet/resource_api/base_context_spec.rb +++ b/spec/puppet/resource_api/base_context_spec.rb @@ -341,6 +341,10 @@ def send_log(log, msg) it { expect { described_class.new(definition).device }.to raise_error RuntimeError, %r{Received device\(\) on an unprepared BaseContext\. Use a PuppetContext instead} } end + describe '#transport' do + it { expect { described_class.new(definition).transport }.to raise_error RuntimeError, %r{No transport available\.} } + end + describe '#send_log' do it { expect { described_class.new(definition).send_log(nil, nil) }.to raise_error RuntimeError, %r{Received send_log\(\) on an unprepared BaseContext\. Use IOContext, or PuppetContext instead} } end diff --git a/spec/puppet/resource_api/io_context_spec.rb b/spec/puppet/resource_api/io_context_spec.rb index 266095d0..a4cb8aa5 100644 --- a/spec/puppet/resource_api/io_context_spec.rb +++ b/spec/puppet/resource_api/io_context_spec.rb @@ -2,9 +2,10 @@ require 'puppet/resource_api/io_context' RSpec.describe Puppet::ResourceApi::IOContext do - subject(:context) { described_class.new(definition, io) } + subject(:context) { described_class.new(definition, io, transport) } let(:definition) { { name: 'some_resource', attributes: {} } } + let(:transport) { nil } let(:io) { StringIO.new('', 'w') } @@ -18,4 +19,13 @@ expect(io.string).to match %r{warning}i end end + + describe '#transport' do + it { expect(context.transport).to be_nil } + context 'when passing in a transport' do + let(:transport) { instance_double(Object, 'transport') } + + it { expect(context.transport).to eq transport } + end + end end diff --git a/spec/puppet/resource_api/puppet_context_spec.rb b/spec/puppet/resource_api/puppet_context_spec.rb index 96a3683e..dbc70c8b 100644 --- a/spec/puppet/resource_api/puppet_context_spec.rb +++ b/spec/puppet/resource_api/puppet_context_spec.rb @@ -18,7 +18,21 @@ end end - context 'with no NetworkDevice configured' do + context 'when a Transport::Wrapper device is configured' do + let(:device) { instance_double('Puppet::Util::NetworkDevice::Test_device::Device', 'device') } + let(:transport) { instance_double('Puppet::Transport::TestDevice', 'transport') } + + before(:each) do + allow(Puppet::Util::NetworkDevice).to receive(:current).and_return(device) + allow(device).to receive(:transport).and_return(transport) + end + + it 'returns the transport' do + expect(context.transport).to eq(transport) + end + end + + context 'with nothing configured' do before(:each) do allow(Puppet::Util::NetworkDevice).to receive(:current).and_return(nil) end From 4122f08454f0c493543c9d94b5fbfa8a2b0dc727 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 29 Jan 2019 19:39:23 +0000 Subject: [PATCH 22/37] (maint) lift duplicate assignment --- lib/puppet/resource_api/type_definition.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb index 130bf524..3e515aed 100644 --- a/lib/puppet/resource_api/type_definition.rb +++ b/lib/puppet/resource_api/type_definition.rb @@ -39,9 +39,6 @@ def validate_schema(definition, attr_key) supported_features = %w[supports_noop canonicalize remote_resource simple_get_filter].freeze unknown_features = definition[:features] - supported_features Puppet.warning("Unknown feature detected: #{unknown_features.inspect}") unless unknown_features.empty? - - # store the validated definition - @definition = definition end end @@ -75,10 +72,12 @@ class BaseTypeDefinition def initialize(definition, attr_key) @data_type_cache = {} validate_schema(definition, attr_key) + # store the validated definition + @definition = definition end def name - @definition[:name] + definition[:name] end def namevars @@ -118,8 +117,6 @@ def validate_schema(definition, attr_key) attr[:behaviour] = attr[:behavior] attr.delete(:behavior) end - # store the validated definition - @definition = definition end # validates a resource hash against its type schema From 3a642760158cf1645494a7ce48a0479a55677a34 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Fri, 1 Feb 2019 15:34:32 +0000 Subject: [PATCH 23/37] (FM-7726) deep symbolize all keys when loading credentials --- lib/puppet/resource_api/transport/wrapper.rb | 9 ++++++++- spec/puppet/resource_api/transport/wrapper_spec.rb | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index a8b102d2..c1ac50a0 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -11,7 +11,7 @@ def initialize(name, url_or_config) url = URI.parse(url_or_config) raise "Unexpected url '#{url_or_config}' found. Only file:/// URLs for configuration supported at the moment." unless url.scheme == 'file' raise "Trying to load config from '#{url.path}, but file does not exist." if url && !File.exist?(url.path) - config = (Hocon.load(url.path, syntax: Hocon::ConfigSyntax::HOCON) || {}).map { |k, v| [k.to_sym, v] }.to_h + config = self.class.deep_symbolize(Hocon.load(url.path, syntax: Hocon::ConfigSyntax::HOCON) || {}) else config = url_or_config end @@ -38,4 +38,11 @@ def method_missing(method_name, *args, &block) super end end + + # From https://stackoverflow.com/a/11788082/4918 + def self.deep_symbolize(obj) + return obj.each_with_object({}) { |(k, v), memo| memo[k.to_sym] = deep_symbolize(v); } if obj.is_a? Hash + return obj.each_with_object([]) { |v, memo| memo << deep_symbolize(v); } if obj.is_a? Array + obj + end end diff --git a/spec/puppet/resource_api/transport/wrapper_spec.rb b/spec/puppet/resource_api/transport/wrapper_spec.rb index b6652424..ccb7c093 100644 --- a/spec/puppet/resource_api/transport/wrapper_spec.rb +++ b/spec/puppet/resource_api/transport/wrapper_spec.rb @@ -11,7 +11,7 @@ it 'will not throw an error' do allow(File).to receive(:exist?).and_return(true) expect(Puppet::ResourceApi::Transport).to receive(:connect) - expect(Hocon).to receive(:load) + expect(Hocon).to receive(:load).with('/etc/credentials', any_args).and_return('foo' => %w[a b], 'bar' => 2) expect { described_class.new('wibble', url) }.not_to raise_error end end From f052a621185a7af4a8ab7ea52b78ef55e2ecab0b Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 31 Jan 2019 09:47:37 +0000 Subject: [PATCH 24/37] Fix missing environment check in validate --- lib/puppet/resource_api/transport.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index eb0d7efc..9ac38e44 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -37,7 +37,7 @@ def connect(name, connection_info) def self.validate(name, connection_info) init_transports - require "puppet/transport/schema/#{name}" unless @transports.key? name + require "puppet/transport/schema/#{name}" unless @transports[@environment].key? name transport_schema = @transports[@environment][name] if transport_schema.nil? raise Puppet::DevError, 'Transport for `%{target}` not registered with `%{environment}`' % { From f3b32bcb1e1e16cb853de86fc057fe7a65aa8e03 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 31 Jan 2019 09:49:31 +0000 Subject: [PATCH 25/37] (FM-7701) Support device providers when using Transport Wrapper --- lib/puppet/resource_api.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 870eb952..1c426f26 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -40,7 +40,12 @@ def register_type(definition) # Keeps a copy of the provider around. Weird naming to avoid clashes with puppet's own `provider` member define_singleton_method(:my_provider) do @my_provider ||= Hash.new { |hash, key| hash[key] = Puppet::ResourceApi.load_provider(definition[:name]).new } - @my_provider[Puppet::Util::NetworkDevice.current.class] + + if Puppet::Util::NetworkDevice.current.is_a? Puppet::ResourceApi::Transport::Wrapper + @my_provider[Puppet::Util::NetworkDevice.current.transport.class] + else + @my_provider[Puppet::Util::NetworkDevice.current.class] + end end # make the provider available in the instance's namespace @@ -414,7 +419,11 @@ def load_provider(type_name) nil else # extract the device type from the currently loaded device's class - Puppet::Util::NetworkDevice.current.class.name.split('::')[-2].downcase + if Puppet::Util::NetworkDevice.current.is_a? Puppet::ResourceApi::Transport::Wrapper + Puppet::Util::NetworkDevice.current.schema.name + else + Puppet::Util::NetworkDevice.current.class.name.split('::')[-2].downcase + end end device_class_name = class_name_from_type_name(device_name) From 968a148e5c3f0e379aa2a4f42b4ac71a72ebbfb8 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Tue, 5 Feb 2019 11:27:23 +0000 Subject: [PATCH 26/37] (maint) Remove message due to breakages in existing modules --- lib/puppet/resource_api/transport/wrapper.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index c1ac50a0..9934ce07 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -32,7 +32,6 @@ def respond_to_missing?(name, _include_private) def method_missing(method_name, *args, &block) if @transport.respond_to? method_name - puts "Delegating #{method_name}" @transport.send(method_name, *args, &block) else super From 203f6336ea151bbfa8e84bc1722c4e1bc45ca47d Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Tue, 5 Feb 2019 18:46:35 +0000 Subject: [PATCH 27/37] (maint) Update unit tests for maximum coverage --- lib/puppet/resource_api.rb | 11 ++-- spec/puppet/resource_api/transport_spec.rb | 2 +- spec/puppet/resource_api_spec.rb | 59 ++++++++++++++++++++++ 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 1c426f26..5e84b628 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -6,6 +6,7 @@ require 'puppet/resource_api/puppet_context' unless RUBY_PLATFORM == 'java' require 'puppet/resource_api/read_only_parameter' require 'puppet/resource_api/transport' +require 'puppet/resource_api/transport/wrapper' require 'puppet/resource_api/type_definition' require 'puppet/resource_api/value_creator' require 'puppet/resource_api/version' @@ -417,13 +418,11 @@ def load_provider(type_name) type_name_sym = type_name.to_sym device_name = if Puppet::Util::NetworkDevice.current.nil? nil - else + elsif Puppet::Util::NetworkDevice.current.is_a? Puppet::ResourceApi::Transport::Wrapper # extract the device type from the currently loaded device's class - if Puppet::Util::NetworkDevice.current.is_a? Puppet::ResourceApi::Transport::Wrapper - Puppet::Util::NetworkDevice.current.schema.name - else - Puppet::Util::NetworkDevice.current.class.name.split('::')[-2].downcase - end + Puppet::Util::NetworkDevice.current.schema.name + else + Puppet::Util::NetworkDevice.current.class.name.split('::')[-2].downcase end device_class_name = class_name_from_type_name(device_name) diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index be019e97..0e6d8aca 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -268,7 +268,7 @@ def change_environment(name = nil) described_class.register(schema) - expect(described_class).to receive(:require).with('puppet/transport/schema/validate') + expect(described_class).not_to receive(:require).with('puppet/transport/schema/validate') expect(schema_def).to receive(:check_schema).with('connection_info').and_return(nil) expect(schema_def).to receive(:validate).with('connection_info').and_return(nil) diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index 7daa44e6..d8db9afc 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -1216,6 +1216,37 @@ class OtherDevice; end it('loads the device provider') { expect(described_class.load_provider('multi_provider').name).to eq 'Puppet::Provider::MultiProvider::SomeDevice' } end end + + context 'with a transport configured' do + let(:definition) { { name: 'multi_provider', attributes: {} } } + let(:transport) { instance_double('Puppet::ResourceApi::Transport::Wrapper', 'transport') } + let(:schema_def) { instance_double('Puppet::ResourceApi::TransportSchemaDef', 'schema_def') } + + before(:each) do + allow(Puppet::Util::NetworkDevice).to receive(:current).with(no_args).and_return(transport) + allow(transport).to receive(:is_a?).with(Puppet::ResourceApi::Transport::Wrapper).and_return(true) + allow(transport).to receive(:schema).and_return(schema_def) + allow(schema_def).to receive(:name).and_return(schema_name) + + module ::Puppet::Provider::MultiProvider + class MultiProvider; end + class SomeDevice; end + class OtherDevice; end + end + end + + context 'with no device-specific provider' do + let(:schema_name) { 'multi_provider' } + + it('loads the default provider') { expect(described_class.load_provider('multi_provider').name).to eq 'Puppet::Provider::MultiProvider::MultiProvider' } + end + + context 'with a device-specific provider' do + let(:schema_name) { 'some_device' } + + it('loads the device provider') { expect(described_class.load_provider('multi_provider').name).to eq 'Puppet::Provider::MultiProvider::SomeDevice' } + end + end end context 'with a provider that does canonicalization', agent_test: true do @@ -1601,6 +1632,10 @@ def set(_context, changes) stub_const('Puppet::Provider::Remoter::Remoter', provider_class) allow(provider_class).to receive(:new).and_return(provider) Puppet.settings[:strict] = :warning + + module ::Puppet::Transport + class Wibble; end + end end it 'is seen as a supported feature' do @@ -1618,6 +1653,30 @@ def set(_context, changes) expect(type.context.type).to be_feature('remote_resource') end end + + describe '#self.my_provider' do + subject(:type) { Puppet::Type.type(:remoter) } + + let(:instance) { type.new(name: 'remote_thing', test_string: 'wibble') } + let(:wrapper) { instance_double('Puppet::ResourceApi::Transport::Wrapper', 'wrapper') } + let(:transport) { instance_double('Puppet::Transport::Wibble', 'transport') } + + before(:each) do + allow(described_class).to receive(:load_provider).and_return(provider) + allow(provider).to receive(:new).and_return(provider) + end + + context 'when a transport is returned by NetworkDevice.current' do + it 'stores the provider with the the name of the transport' do + allow(Puppet::Util::NetworkDevice).to receive(:current).and_return(wrapper) + allow(wrapper).to receive(:is_a?).with(Puppet::ResourceApi::Transport::Wrapper).and_return(true) + allow(wrapper).to receive(:transport).and_return(transport) + allow(transport).to receive(:class).and_return(Puppet::Transport::Wibble) + + expect(instance.my_provider).to eq provider + end + end + end end context 'with a `supports_noop` provider', agent_test: true do From 2330c30e0363dde4c4bd24a393d6f846edfb93a6 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 7 Feb 2019 16:51:55 +0000 Subject: [PATCH 28/37] (PDK-1271) Allow a transport to be wrapped and used like a device --- lib/puppet/resource_api/transport.rb | 12 +++++++ lib/puppet/resource_api/transport/wrapper.rb | 22 +++++++++---- .../resource_api/transport/wrapper_spec.rb | 15 +++++++++ spec/puppet/resource_api/transport_spec.rb | 33 +++++++++++++++++++ 4 files changed, 75 insertions(+), 7 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 9ac38e44..df6177ee 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -31,10 +31,22 @@ def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" class_name = name.split('_').map { |e| e.capitalize }.join + # passing the copy as it may have been stripped on invalid key/values by validate Puppet::Transport.const_get(class_name).new(get_context(name), connection_info) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations + def inject_device(name, transport) + transport_wrapper = Puppet::ResourceApi::Transport::Wrapper.new(name, transport) + + if Puppet::Util::NetworkDevice.respond_to?(:set_device) + Puppet::Util::NetworkDevice.set_device(name, transport_wrapper) + else + Puppet::Util::NetworkDevice.instance_variable_set(:@current, transport_wrapper) + end + end + module_function :inject_device # rubocop:disable Style/AccessModifierDeclarations + def self.validate(name, connection_info) init_transports require "puppet/transport/schema/#{name}" unless @transports[@environment].key? name diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index 9934ce07..fe4a41e2 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -6,20 +6,28 @@ class Puppet::ResourceApi::Transport::Wrapper attr_reader :transport, :schema - def initialize(name, url_or_config) - if url_or_config.is_a? String - url = URI.parse(url_or_config) - raise "Unexpected url '#{url_or_config}' found. Only file:/// URLs for configuration supported at the moment." unless url.scheme == 'file' + def initialize(name, url_or_config_or_transport) + if url_or_config_or_transport.is_a? String + url = URI.parse(url_or_config_or_transport) + raise "Unexpected url '#{url_or_config_or_transport}' found. Only file:/// URLs for configuration supported at the moment." unless url.scheme == 'file' raise "Trying to load config from '#{url.path}, but file does not exist." if url && !File.exist?(url.path) config = self.class.deep_symbolize(Hocon.load(url.path, syntax: Hocon::ConfigSyntax::HOCON) || {}) - else - config = url_or_config + elsif url_or_config_or_transport.is_a? Hash + config = url_or_config_or_transport + elsif transport_class?(name, url_or_config_or_transport) + @transport = url_or_config_or_transport end - @transport = Puppet::ResourceApi::Transport.connect(name, config) + @transport ||= Puppet::ResourceApi::Transport.connect(name, config) @schema = Puppet::ResourceApi::Transport.list[name] end + def transport_class?(name, transport) + class_name = name.split('_').map { |e| e.capitalize }.join + expected = Puppet::Transport.const_get(class_name).to_s + expected == transport.class.to_s + end + def facts context = Puppet::ResourceApi::PuppetContext.new(@schema) # @transport.facts + custom_facts # look into custom facts work by TP diff --git a/spec/puppet/resource_api/transport/wrapper_spec.rb b/spec/puppet/resource_api/transport/wrapper_spec.rb index ccb7c093..1b56c20d 100644 --- a/spec/puppet/resource_api/transport/wrapper_spec.rb +++ b/spec/puppet/resource_api/transport/wrapper_spec.rb @@ -32,6 +32,21 @@ described_class.new('wibble', config) end end + + before(:each) do + module Puppet::Transport + class SomethingSomethingDarkside; end + end + end + + context 'when called with a transport class' do + let(:transport) { Puppet::Transport::SomethingSomethingDarkside.new } + let(:instance) { described_class.new('something_something_darkside', transport) } + + it 'will set the @transport class variable' do + expect(instance.instance_variable_get(:@transport)).to eq(transport) + end + end end describe '#facts' do diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 0e6d8aca..31529c2d 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -247,6 +247,39 @@ def change_environment(name = nil) end end + describe '#inject_device(name, transport)' do + let(:device_name) { 'wibble' } + let(:transport) { instance_double(Puppet::Transport::Wibble, 'transport') } + let(:wrapper) { instance_double(Puppet::ResourceApi::Transport::Wrapper, 'wrapper') } + + before(:each) do + module Puppet::Transport + class Wibble; end + end + end + + context 'when puppet has set_device' do + it 'wraps the transport and calls set_device within NetworkDevice' do + expect(Puppet::ResourceApi::Transport::Wrapper).to receive(:new).with(device_name, transport).and_return(wrapper) + allow(Puppet::Util::NetworkDevice).to receive(:respond_to?).with(:set_device).and_return(true) + expect(Puppet::Util::NetworkDevice).to receive(:set_device).with(device_name, wrapper) + + described_class.inject_device(device_name, transport) + end + end + + context 'when puppet does not have set_device' do + it 'wraps the transport and sets it as current in NetworkDevice' do + expect(Puppet::ResourceApi::Transport::Wrapper).to receive(:new).with(device_name, transport).and_return(wrapper) + expect(Puppet::Util::NetworkDevice).to receive(:respond_to?).with(:set_device).and_return(false) + + described_class.inject_device(device_name, transport) + + expect(Puppet::Util::NetworkDevice.current).to eq(wrapper) + end + end + end + describe '#validate(name, connection_info)', agent_test: true do context 'when the transport does not exist' do it { expect { described_class.send(:validate, 'wibble', {}) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/schema/wibble} } From 9baf1c80a31554f4a7082301f03bc65bfd9c1075 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 14 Feb 2019 12:15:20 +0000 Subject: [PATCH 29/37] Change behaviour of check_schema_keys to no longer remove unknown keys This commit prevents validation from changing resource and connection_info hashes passed into RSAPI from removing keys that are not in their schema. --- lib/puppet/resource_api/type_definition.rb | 4 ++-- spec/puppet/resource_api/base_type_definition_spec.rb | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb index 3e515aed..be8ad135 100644 --- a/lib/puppet/resource_api/type_definition.rb +++ b/lib/puppet/resource_api/type_definition.rb @@ -154,10 +154,10 @@ def check_schema(resource) end # Returns an array of keys that where not found in the type schema - # Modifies the resource passed in, leaving only valid attributes + # No longer modifies the resource passed in def check_schema_keys(resource) rejected = [] - resource.reject! { |key| rejected << key if key != :title && attributes.key?(key) == false } + resource.reject { |key| rejected << key if key != :title && attributes.key?(key) == false } rejected end diff --git a/spec/puppet/resource_api/base_type_definition_spec.rb b/spec/puppet/resource_api/base_type_definition_spec.rb index 2c432dc0..64ab1cc5 100644 --- a/spec/puppet/resource_api/base_type_definition_spec.rb +++ b/spec/puppet/resource_api/base_type_definition_spec.rb @@ -38,10 +38,16 @@ context 'when resource contains invalid keys' do let(:resource) { { name: 'test_string', wibble: '1', foo: '2' } } + let(:resource_copy) { { name: 'test_string', wibble: '1', foo: '2' } } it 'returns an array containing the bad keys' do expect(type.check_schema_keys(resource)).to eq([:wibble, :foo]) end + + it 'does not modify the resource passed in' do + type.check_schema_keys(resource) + expect(resource).to eq(resource_copy) + end end end @@ -84,7 +90,7 @@ it 'displays up to 100 warnings' do expect(Puppet).to receive(:warning).with(message).exactly(100).times 110.times do - type.check_schema(resource.dup) + type.check_schema(resource) end end end @@ -121,7 +127,7 @@ it 'displays up to 100 warnings' do expect(Puppet).to receive(:warning).with(message).exactly(100).times 110.times do - type.check_schema(resource.dup) + type.check_schema(resource) end end end From 1f9ede5af4ec5316b003596e4142e7d5a7b6a646 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Mon, 18 Feb 2019 11:30:18 +0000 Subject: [PATCH 30/37] (FM-7698) Ensure that sensitive values are handled correctly --- spec/acceptance/sensitive_spec.rb | 16 +++++++++++++ .../puppet/transport/schema/test_device.rb | 24 +++++++++++++++++++ .../lib/puppet/type/test_sensitive.rb | 6 ++++- 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/spec/acceptance/sensitive_spec.rb b/spec/acceptance/sensitive_spec.rb index f48341fb..e23c4088 100644 --- a/spec/acceptance/sensitive_spec.rb +++ b/spec/acceptance/sensitive_spec.rb @@ -21,6 +21,22 @@ expect(stdout_str).not_to match %r{foo} expect(stdout_str).not_to match %r{warn|error}i end + + context 'when a sensitive value is not the top level type' do + it 'is not exposed by a provider' do + stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_sensitive { bar: secret => Sensitive('foo'), "\ + "optional_secret => Sensitive('optional foo'), variant_secret => [Sensitive('variant foo')] }\"") + expect(stdout_str).to match %r{redacted} + expect(stdout_str).not_to match %r{variant foo} + expect(stdout_str).not_to match %r{warn|error}i + end + it 'properly validates the sensitive type value' do + stdout_str, _status = Open3.capture2e("puppet apply #{common_args} -e \"test_sensitive { bar: secret => Sensitive('foo'), "\ + "optional_secret => Sensitive('optional foo'), variant_secret => [Sensitive(134679)] }\"") + expect(stdout_str).to match %r{Sensitive\[String\]( value)?, got Sensitive\[Integer\]} + expect(stdout_str).not_to match %r{134679} + end + end end describe 'using `puppet resource`' do diff --git a/spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb b/spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb index 5bd6ee45..df2f7a9b 100644 --- a/spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb +++ b/spec/fixtures/test_module/lib/puppet/transport/schema/test_device.rb @@ -4,5 +4,29 @@ name: 'test_device', # points at class Puppet::Transport::TestDevice desc: 'Connects to a device', connection_info: { + username: { + type: 'String', + desc: 'The name of the resource you want to manage.', + }, + secret: { + type: 'String', + desc: 'A secret to protect.', + sensitive: true, + }, + optional_secret: { + type: 'Optional[String]', + desc: 'An optional secret to protect.', + sensitive: true, + }, + array_secret: { + type: 'Optional[Array[String]]', + desc: 'An array secret to protect.', + sensitive: true, + }, + variant_secret: { + type: 'Optional[Variant[Array[String], Integer]]', + desc: 'An array secret to protect.', + sensitive: true, + }, }, ) diff --git a/spec/fixtures/test_module/lib/puppet/type/test_sensitive.rb b/spec/fixtures/test_module/lib/puppet/type/test_sensitive.rb index ce74363d..f42f89c9 100644 --- a/spec/fixtures/test_module/lib/puppet/type/test_sensitive.rb +++ b/spec/fixtures/test_module/lib/puppet/type/test_sensitive.rb @@ -26,7 +26,11 @@ desc: 'An optional secret to protect.', }, array_secret: { - type: 'Array[Sensitive[String]]', + type: 'Optional[Array[Sensitive[String]]]', + desc: 'An array secret to protect.', + }, + variant_secret: { + type: 'Optional[Variant[Array[Sensitive[String]], Integer]]', desc: 'An array secret to protect.', }, }, From 2916e593d36646cb660b4fcdd36404e8e4d6bed7 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Tue, 19 Feb 2019 16:31:35 +0000 Subject: [PATCH 31/37] (maint) Fix schema error message returned when checking a transport --- lib/puppet/resource_api/transport.rb | 4 ++-- lib/puppet/resource_api/type_definition.rb | 7 ++++--- spec/puppet/resource_api/transport_spec.rb | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index df6177ee..8d96d925 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -57,8 +57,8 @@ def self.validate(name, connection_info) environment: @environment, } end - - transport_schema.check_schema(connection_info) + message_prefix = 'The connection info provided does not match the Transport Schema' + transport_schema.check_schema(connection_info, message_prefix) transport_schema.validate(connection_info) end private_class_method :validate diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb index be8ad135..0b92da8f 100644 --- a/lib/puppet/resource_api/type_definition.rb +++ b/lib/puppet/resource_api/type_definition.rb @@ -120,16 +120,17 @@ def validate_schema(definition, attr_key) end # validates a resource hash against its type schema - def check_schema(resource) + def check_schema(resource, message_prefix = nil) namevars.each do |namevar| if resource[namevar].nil? raise Puppet::ResourceError, "`#{name}.get` did not return a value for the `#{namevar}` namevar attribute" end end - message = "Provider returned data that does not match the Type Schema for `#{name}[#{resource[namevars.first]}]`" + message_prefix = 'Provider returned data that does not match the Type Schema' if message_prefix.nil? + message = "#{message_prefix} for `#{name}[#{resource[namevars.first]}]`" - rejected_keys = check_schema_keys(resource) # removes bad keys + rejected_keys = check_schema_keys(resource) bad_values = check_schema_values(resource) unless rejected_keys.empty? diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 31529c2d..d2e10ce4 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -302,7 +302,7 @@ class Wibble; end described_class.register(schema) expect(described_class).not_to receive(:require).with('puppet/transport/schema/validate') - expect(schema_def).to receive(:check_schema).with('connection_info').and_return(nil) + expect(schema_def).to receive(:check_schema).with('connection_info', kind_of(String)).and_return(nil) expect(schema_def).to receive(:validate).with('connection_info').and_return(nil) described_class.send :validate, 'validate', 'connection_info' From 88c636517703278548bd2290d8536392fcf4b9e3 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Tue, 19 Feb 2019 16:32:45 +0000 Subject: [PATCH 32/37] (FM-7698) Handle a transport schema type with `sensitive: true` set --- lib/puppet/resource_api/type_definition.rb | 7 +- spec/acceptance/device_spec.rb | 9 +- spec/acceptance/transport/transport_spec.rb | 196 ++++++++++++++++++ .../transport/schema/test_device_sensitive.rb | 32 +++ .../puppet/transport/test_device_sensitive.rb | 21 ++ .../resource_api/base_type_definition_spec.rb | 55 ++++- 6 files changed, 310 insertions(+), 10 deletions(-) create mode 100644 spec/acceptance/transport/transport_spec.rb create mode 100644 spec/fixtures/test_module/lib/puppet/transport/schema/test_device_sensitive.rb create mode 100644 spec/fixtures/test_module/lib/puppet/transport/test_device_sensitive.rb diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb index 0b92da8f..40bd3713 100644 --- a/lib/puppet/resource_api/type_definition.rb +++ b/lib/puppet/resource_api/type_definition.rb @@ -169,12 +169,17 @@ def check_schema_values(resource) resource.each do |key, value| next unless attributes[key] type = @data_type_cache[attributes[key][:type]] + is_sensitive = (attributes[key].key?(:sensitive) && (attributes[key][:sensitive] == true)) error_message = Puppet::ResourceApi::DataTypeHandling.try_validate( type, value, '', ) - bad_vals[key] = value unless error_message.nil? + if is_sensitive + bad_vals[key] = '<< redacted value >> ' + error_message unless error_message.nil? + else + bad_vals[key] = value unless error_message.nil? + end end bad_vals end diff --git a/spec/acceptance/device_spec.rb b/spec/acceptance/device_spec.rb index 909313c9..788244dd 100644 --- a/spec/acceptance/device_spec.rb +++ b/spec/acceptance/device_spec.rb @@ -78,7 +78,14 @@ DEVICE_CONF end let(:device_credentials) { Tempfile.new('credentials.txt') } - let(:device_credentials_content) { {} } + let(:device_credentials_content) do + <= Gem::Version.new('5.3.6') && Gem::Version.new(Puppet::PUPPETVERSION) != Gem::Version.new('5.4.0') diff --git a/spec/acceptance/transport/transport_spec.rb b/spec/acceptance/transport/transport_spec.rb new file mode 100644 index 00000000..3ab93bec --- /dev/null +++ b/spec/acceptance/transport/transport_spec.rb @@ -0,0 +1,196 @@ +require 'spec_helper' +require 'tempfile' +require 'open3' + +RSpec.describe 'a transport' do + let(:common_args) { '--verbose --trace --debug --strict=error --modulepath spec/fixtures' } + + before(:all) do + FileUtils.mkdir_p(File.expand_path('~/.puppetlabs/opt/puppet/cache/devices/the_node/state')) + end + + describe 'using `puppet device`' do + let(:common_args) { super() + ' --target the_node' } + let(:device_conf) { Tempfile.new('device.conf') } + let(:device_conf_content) do + <= Gem::Version.new('5.3.6') && Gem::Version.new(Puppet::PUPPETVERSION) != Gem::Version.new('5.4.0') + end + + before(:each) do + skip "No device --apply in puppet before v5.3.6 nor in v5.4.0 (v#{Puppet::PUPPETVERSION} is installed)" unless is_device_apply_supported? + device_conf.write(device_conf_content) + device_conf.close + + device_credentials.write(device_credentials_content) + device_credentials.close + end + + after(:each) do + device_conf.unlink + device_credentials.unlink + end + + context 'when all sensitive values are valid' do + let(:device_credentials_content) do + <"foo"} + expect(stdout_str).to match %r{:secret_string=>"wibble"} + expect(stdout_str).to match %r{:optional_secret=>"bar"} + expect(stdout_str).to match %r{:array_secret=>\["meep"\]} + expect(stdout_str).to match %r{:variant_secret=>21} + end + end + end + + context 'with a sensitive string value that is invalid' do + let(:device_credentials_content) do + <> } + expect(stdout_str).not_to match %r{optional_secret} + expect(stdout_str).not_to match %r{array_secret} + expect(stdout_str).not_to match %r{variant_secret} + end + end + end + + context 'with an optional sensitive string value that is invalid' do + let(:device_credentials_content) do + <>} + expect(stdout_str).not_to match %r{array_secret} + expect(stdout_str).not_to match %r{variant_secret} + end + end + end + + context 'with an array of sensitive strings that is invalid' do + let(:device_credentials_content) do + <>} + expect(stdout_str).not_to match %r{variant_secret} + end + end + end + + context 'with an variant containing a sensitive value that is invalid' do + let(:device_credentials_content) do + <>} + end + end + end + end +end diff --git a/spec/fixtures/test_module/lib/puppet/transport/schema/test_device_sensitive.rb b/spec/fixtures/test_module/lib/puppet/transport/schema/test_device_sensitive.rb new file mode 100644 index 00000000..a22c43aa --- /dev/null +++ b/spec/fixtures/test_module/lib/puppet/transport/schema/test_device_sensitive.rb @@ -0,0 +1,32 @@ +require 'puppet/resource_api' + +Puppet::ResourceApi.register_transport( + name: 'test_device_sensitive', # points at class Puppet::Transport::TestDevice + desc: 'Connects to a device', + connection_info: { + username: { + type: 'String', + desc: 'The name of the resource you want to manage.', + }, + secret_string: { + type: 'String', + desc: 'A secret to protect.', + sensitive: true, + }, + optional_secret: { + type: 'Optional[String]', + desc: 'An optional secret to protect.', + sensitive: true, + }, + array_secret: { + type: 'Optional[Array[String]]', + desc: 'An array secret to protect.', + sensitive: true, + }, + variant_secret: { + type: 'Optional[Variant[Array[String], Integer]]', + desc: 'An array secret to protect.', + sensitive: true, + }, + }, +) diff --git a/spec/fixtures/test_module/lib/puppet/transport/test_device_sensitive.rb b/spec/fixtures/test_module/lib/puppet/transport/test_device_sensitive.rb new file mode 100644 index 00000000..c2a898a2 --- /dev/null +++ b/spec/fixtures/test_module/lib/puppet/transport/test_device_sensitive.rb @@ -0,0 +1,21 @@ +module Puppet::Transport +# a transport for a test_device +class TestDeviceSensitive + def initialize(_context, connection_info); + puts "transport connection_info: #{connection_info}" + end + + def facts(_context) + { 'foo' => 'bar' } + end + + def verify(_context) + return true + end + + def close(_context) + return + end +end + +end diff --git a/spec/puppet/resource_api/base_type_definition_spec.rb b/spec/puppet/resource_api/base_type_definition_spec.rb index 64ab1cc5..62a918fd 100644 --- a/spec/puppet/resource_api/base_type_definition_spec.rb +++ b/spec/puppet/resource_api/base_type_definition_spec.rb @@ -52,19 +52,58 @@ end describe '#check_schema_values' do - context 'when resource contains only valid values' do - let(:resource) { { name: 'some_resource', prop: 1, ensure: 'present' } } + context 'when the definition is a type' do + context 'when resource contains only valid values' do + let(:resource) { { name: 'some_resource', prop: 1, ensure: 'present' } } - it 'returns an empty array' do - expect(type.check_schema_values(resource)).to eq({}) + it 'returns an empty array' do + expect(type.check_schema_values(resource)).to eq({}) + end + end + + context 'when resource contains invalid values' do + let(:resource) { { name: 'test_string', prop: 'foo', ensure: 1 } } + + it 'returns a hash of the keys that have invalid values' do + expect(type.check_schema_values(resource)).to eq(prop: 'foo', ensure: 1) + end end end - context 'when resource contains invalid values' do - let(:resource) { { name: 'test_string', prop: 'foo', ensure: 1 } } + context 'when the definition is a transport' do + subject(:type) { described_class.new(definition, :connection_info) } + + let(:definition) do + { + name: 'some_transport', + connection_info: { + username: { + type: 'String', + desc: 'The username to connect with', + }, + secret: { + type: 'String', + desc: 'A sensitive value', + sensitive: true, + }, + }, + } + end + + context 'when resource contains only valid values' do + let(:resource) { { username: 'wibble', secret: 'foo' } } - it 'returns a hash of the keys that have invalid values' do - expect(type.check_schema_values(resource)).to eq(prop: 'foo', ensure: 1) + it 'returns an empty array' do + expect(type.check_schema_values(resource)).to eq({}) + end + end + + context 'when resource contains invalid values' do + let(:resource) { { username: 'wibble', secret: 12_345 } } + + it 'returns a hash of the keys that have invalid values' do + expect(type.check_schema_values(resource)).to match(secret: %r{<< redacted value >> expect(s|ed) a String value, got Integer}) + end end end end From 19f9139b0c9fb91cdb78957ae39d0765b0d7a8c0 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 21 Feb 2019 16:11:06 +0000 Subject: [PATCH 33/37] (maint) remove invalid comment --- lib/puppet/resource_api/transport.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 8d96d925..9ac98297 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -31,7 +31,6 @@ def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" class_name = name.split('_').map { |e| e.capitalize }.join - # passing the copy as it may have been stripped on invalid key/values by validate Puppet::Transport.const_get(class_name).new(get_context(name), connection_info) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations From 7466c20ac37ee8226de13a8b7c9645821ebe009d Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 21 Feb 2019 19:56:45 +0000 Subject: [PATCH 34/37] (maint) use device wrapper to support demo transport --- spec/acceptance/transport/transport_spec.rb | 10 ---------- .../network_device/test_device_sensitive/device.rb | 12 ++++++++++++ 2 files changed, 12 insertions(+), 10 deletions(-) create mode 100644 spec/fixtures/test_module/lib/puppet/util/network_device/test_device_sensitive/device.rb diff --git a/spec/acceptance/transport/transport_spec.rb b/spec/acceptance/transport/transport_spec.rb index 3ab93bec..d7678952 100644 --- a/spec/acceptance/transport/transport_spec.rb +++ b/spec/acceptance/transport/transport_spec.rb @@ -53,8 +53,6 @@ def is_device_apply_supported? end it 'does not throw' do - pending('Requires Puppet with NetworkDevice.rb support for Transports') - Tempfile.create('apply_success') do |f| f.write 'notify { "foo": }' f.close @@ -87,8 +85,6 @@ def is_device_apply_supported? end it 'Value type mismatch' do - pending('Requires Puppet with NetworkDevice.rb support for Transports') - Tempfile.create('apply_success') do |f| f.write 'notify { "foo": }' f.close @@ -117,8 +113,6 @@ def is_device_apply_supported? end it 'Value type mismatch' do - pending('Requires Puppet with NetworkDevice.rb support for Transports') - Tempfile.create('apply_success') do |f| f.write 'notify { "foo": }' f.close @@ -147,8 +141,6 @@ def is_device_apply_supported? end it 'Value type mismatch' do - pending('Requires Puppet with NetworkDevice.rb support for Transports') - Tempfile.create('apply_success') do |f| f.write 'notify { "foo": }' f.close @@ -177,8 +169,6 @@ def is_device_apply_supported? end it 'Value type mismatch' do - pending('Requires Puppet with NetworkDevice.rb support for Transports') - Tempfile.create('apply_success') do |f| f.write 'notify { "foo": }' f.close diff --git a/spec/fixtures/test_module/lib/puppet/util/network_device/test_device_sensitive/device.rb b/spec/fixtures/test_module/lib/puppet/util/network_device/test_device_sensitive/device.rb new file mode 100644 index 00000000..4c04b7cf --- /dev/null +++ b/spec/fixtures/test_module/lib/puppet/util/network_device/test_device_sensitive/device.rb @@ -0,0 +1,12 @@ +require 'puppet/resource_api/transport/wrapper' + +class Puppet::Util::NetworkDevice; end + +module Puppet::Util::NetworkDevice::Test_device_sensitive # rubocop:disable Style/ClassAndModuleCamelCase + # The main class for handling the connection and command parsing to the IOS Catalyst device + class Device < Puppet::ResourceApi::Transport::Wrapper + def initialize(url_or_config, _options = {}) + super('test_device_sensitive', url_or_config) + end + end +end \ No newline at end of file From d3e2e0bff88746638d5ad64a1a6684bc92de2a39 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Thu, 21 Feb 2019 20:43:46 +0000 Subject: [PATCH 35/37] (PDK-7698) Wrap raw values flagged as sensitive in Puppet Sensitive type Before passing the connection_info to the transport, wrap them in a PSensitiveType to prevent unexpected leaking of data. --- lib/puppet/resource_api/transport.rb | 15 ++++++++- spec/acceptance/transport/transport_spec.rb | 13 ++++---- spec/puppet/resource_api/transport_spec.rb | 36 +++++++++++++++++++++ 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 9ac98297..dc6d65a5 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -31,7 +31,7 @@ def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" class_name = name.split('_').map { |e| e.capitalize }.join - Puppet::Transport.const_get(class_name).new(get_context(name), connection_info) + Puppet::Transport.const_get(class_name).new(get_context(name), wrap_sensitive(name, connection_info)) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations @@ -79,4 +79,17 @@ def self.init_transports @transports[@environment] ||= {} end private_class_method :init_transports + + def self.wrap_sensitive(name, connection_info) + transport_schema = @transports[@environment][name] + if transport_schema + transport_schema.definition[:connection_info].each do |attr_name, options| + if options.key?(:sensitive) && (options[:sensitive] == true) + connection_info[attr_name] = Puppet::Pops::Types::PSensitiveType::Sensitive.new(connection_info[:name]) + end + end + end + connection_info + end + private_class_method :wrap_sensitive end diff --git a/spec/acceptance/transport/transport_spec.rb b/spec/acceptance/transport/transport_spec.rb index d7678952..3fe7c0b5 100644 --- a/spec/acceptance/transport/transport_spec.rb +++ b/spec/acceptance/transport/transport_spec.rb @@ -47,7 +47,7 @@ def is_device_apply_supported? secret_string: wibble optional_secret: bar array_secret: [meep] - variant_secret: 21 + variant_secret: 1234567890 } DEVICE_CREDS end @@ -59,14 +59,13 @@ def is_device_apply_supported? stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply #{f.path}") expect(stdout_str).not_to match %r{Value type mismatch} - # These are only matched because the test transport prints them - # This however shows that the transport can view the raw values + expect(stdout_str).to match %r{transport connection_info:} expect(stdout_str).to match %r{:username=>"foo"} - expect(stdout_str).to match %r{:secret_string=>"wibble"} - expect(stdout_str).to match %r{:optional_secret=>"bar"} - expect(stdout_str).to match %r{:array_secret=>\["meep"\]} - expect(stdout_str).to match %r{:variant_secret=>21} + expect(stdout_str).not_to match %r{wibble} + expect(stdout_str).not_to match %r{bar} + expect(stdout_str).not_to match %r{meep} + expect(stdout_str).not_to match %r{1234567890} end end end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index d2e10ce4..6bd10a61 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -331,4 +331,40 @@ class Wibble; end end end end + + describe '#wrap_sensitive(name, connection_info)' do + context 'when the connection info contains a `Sensitive` type' do + let(:schema) do + { + name: 'sensitive_transport', + desc: 'a secret', + connection_info: { + secret: { + type: 'String', + desc: 'A secret to protect.', + sensitive: true, + }, + }, + } + end + let(:schema_def) { instance_double('Puppet::ResourceApi::TransportSchemaDef', 'schema_def') } + let(:connection_info) do + { + secret: 'sup3r_secret_str1ng', + } + end + + before(:each) do + allow(Puppet::ResourceApi::TransportSchemaDef).to receive(:new).with(schema).and_return(schema_def) + described_class.register(schema) + end + + it 'wraps the value in a PSensitiveType' do + allow(schema_def).to receive(:definition).and_return(schema) + + conn_info = described_class.send :wrap_sensitive, 'sensitive_transport', connection_info + expect(conn_info[:secret]).to be_a(Puppet::Pops::Types::PSensitiveType::Sensitive) + end + end + end end From 75aae39ae20d77d10db1278bcfd89b2bc2ab5ef6 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Fri, 22 Feb 2019 14:03:37 +0000 Subject: [PATCH 36/37] (maint) Fixes travis failure concerning jar-dependencies --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 978c4106..7b214550 100644 --- a/.travis.yml +++ b/.travis.yml @@ -25,7 +25,7 @@ matrix: - env: RVM="jruby-9.1.9.0" PUPPET_GEM_VERSION='~> 5' JRUBY_OPTS="--debug" before_cache: pushd ~/.rvm && rm -rf archives rubies/ruby-2.2.7 rubies/ruby-2.3.4 && popd cache: - bundler: true + bundler: false directories: ~/.rvm before_install: rvm use jruby-9.1.9.0 --install --binary --fuzzy - rvm: 2.4.3 From 95d65c030169af1a13b7ffc6351c891b52c25a9a Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Mon, 25 Feb 2019 16:25:56 +0000 Subject: [PATCH 37/37] (PDK-7698) Ensure that Sensitive values get unwrapped correctly --- lib/puppet/resource_api/transport.rb | 2 +- spec/puppet/resource_api/transport_spec.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index dc6d65a5..438333f1 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -85,7 +85,7 @@ def self.wrap_sensitive(name, connection_info) if transport_schema transport_schema.definition[:connection_info].each do |attr_name, options| if options.key?(:sensitive) && (options[:sensitive] == true) - connection_info[attr_name] = Puppet::Pops::Types::PSensitiveType::Sensitive.new(connection_info[:name]) + connection_info[attr_name] = Puppet::Pops::Types::PSensitiveType::Sensitive.new(connection_info[attr_name]) end end end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 6bd10a61..ad59697d 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -364,6 +364,7 @@ class Wibble; end conn_info = described_class.send :wrap_sensitive, 'sensitive_transport', connection_info expect(conn_info[:secret]).to be_a(Puppet::Pops::Types::PSensitiveType::Sensitive) + expect(conn_info[:secret].unwrap).to eq('sup3r_secret_str1ng') end end end