From 9d80178832febd4db86f590e0934470087f59b4b Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 11 Jun 2019 17:08:35 +0100 Subject: [PATCH] (PUP-9747) Relax validation for bolt Bolt always provides all computed connection info attributes to tasks, instead of only the ones the user provided. Therefore we can't be strict when validating the passed in arguments. To limit the impact to bolt users, this change makes the relaxed validation trigger only when a `'remote-transport'` key is specified, as that is always provided by bolt. This also contains the docs changes from https://github.com/puppetlabs/puppet-specifications/pull/142 --- README.md | 45 ++++++++++------------ lib/puppet/resource_api/transport.rb | 37 ++++++++++++++++++ spec/puppet/resource_api/transport_spec.rb | 42 +++++++++++++++++--- 3 files changed, 93 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index c8ddaf58..c4a6719b 100644 --- a/README.md +++ b/README.md @@ -239,31 +239,26 @@ Puppet::ResourceAPI.register_transport( ### Transport Schema keywords -Please note that within the transport schema, the following keywords are reserved words: - -#### Usable within the schema - -The following keywords are encouraged within the Transport schema: - -* `uri` - Use when you need to specify a specific URL to connect to. All of the following keys will be computed from the `uri` if possible. In the future more url parts may be computed from the URI as well. -* `host` - Use to specify and IP or address to connect to. -* `protocol` - Use to specify which protocol the transport should use for example `http`, `https`, `ssh` or `tcp` -* `user` - The user the transport should connect as. -* `port` - The port the transport should connect to. - -#### Non-Usable within the schema - -The following keywords are keywords that must not be used by the transport schema: - -* `name` - transports should use `uri` instead of name. -* `path` - reserved as a uri part -* `query` - reserved as a uri part -* `run-on` - This is used by bolt to determine which target to proxy to. Transports should not rely on this key. -* `remote-transport` - This is used to determine which transport to load. It should always be the transport class name "declassified". -* `remote-*` Any key starting with `remote-` is reserved for future use. -* `implementations`: reserved by bolt - -Note: Currently bolt inventory requires that a name be set for every target and always uses that name as the URI. This means there is no way to specify `host` separately from the host section of the `name` when parsed as a URI. +To align with [Bolt's inventory file](https://puppet.com/docs/bolt/latest/inventory_file.html), a transport schema prefers the following keywords (when relevant): + +* `uri`: use when you need to specify a specific URL to connect to. Bolt will compute the following keys from the `uri` when possible. In the future more url parts may be computed from the URI. +* `protocol`: use to specify which protocol the transport should use for example `http`, `https`, `ssh` or `tcp`. +* `host`: use to specify an IP or address to connect to. +* `port`: the port the transport should connect to. +* `user`: the user the transport should connect as. +* `password`: the password for the specified user. + +Do not use the following keywords when writing a schema: + +* `implementations`: reserved by Bolt. +* `name`: transports should use `uri` instead of name. +* `path`: reserved as a uri part. +* `query`: reserved as a uri part. +* `remote-*`: any key starting with `remote-` is reserved for future use. +* `remote-transport`: determines which transport to load. It is always the transport class named "declassified". +* `run-on`: Bolt uses this keyword to determine which target to proxy to. Transports should not rely on this key. + +> Note: Bolt inventory requires you to set a name for every target and always use it for the URI. This means that there is no way to specify `host` separately from the host section of the `name` when parsed as a URI. After the device class, transport class and transport schema have been implemented, `puppet device` will be able to use the new provider, and supply it (through the device class) with the URL specified in the [`device.conf`](https://puppet.com/docs/puppet/5.3/config_file_device.html). diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 3c22a2c7..adb9df63 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -53,6 +53,11 @@ def self.validate(name, connection_info) environment: current_environment, } end + + if connection_info.key?(:"remote-transport") + clean_bolt_attributes(transport_schema, connection_info) + end + 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) @@ -93,4 +98,36 @@ def self.current_environment end end private_class_method :current_environment + + def self.clean_bolt_attributes(transport_schema, connection_info) + context = get_context(transport_schema.name) + + # Attributes we expect from bolt, but want to ignore if the transport does not expect them + [:uri, :host, :protocol, :user, :port, :password].each do |attribute_name| + if connection_info.key?(attribute_name) && !transport_schema.attributes.key?(attribute_name) + context.info('Discarding superfluous bolt attribute: %{attribute_name}' % { attribute_name: attribute_name }) + connection_info.delete(attribute_name) + end + end + + # Attributes that bolt emits, but we want to ignore if the transport does not expect them + ([:name, :path, :query, :"run-on", :"remote-transport", :implementations] + connection_info.keys.select { |k| k.to_s.start_with? 'remote-' }).each do |attribute_name| + if connection_info.key?(attribute_name) && !transport_schema.attributes.key?(attribute_name) + context.debug('Discarding bolt metaparameter: %{attribute_name}' % { attribute_name: attribute_name }) + connection_info.delete(attribute_name) + end + end + + # remove any other attributes the transport is not prepared to handle + connection_info.keys.each do |attribute_name| + if connection_info.key?(attribute_name) && !transport_schema.attributes.key?(attribute_name) + context.warning('Discarding unknown attribute: %{attribute_name}' % { attribute_name: attribute_name }) + connection_info.delete(attribute_name) + end + end + + # don't return a value as we've already modified the hash + nil + end + private_class_method :clean_bolt_attributes end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index ccb02997..5fedb181 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -294,20 +294,50 @@ class Wibble; end end end - context 'when the transport being validated has been registered' do - let(:schema) { { name: 'validate', desc: 'a minimal connection', connection_info: {} } } + context 'with a registered transport' do + let(:attributes) { {} } + let(:schema) { { name: 'validate', desc: 'a minimal connection', connection_info: attributes } } let(:schema_def) { instance_double('Puppet::ResourceApi::TransportSchemaDef', 'schema_def') } - it 'validates the connection_info' do + before(:each) do allow(Puppet::ResourceApi::TransportSchemaDef).to receive(:new).with(schema).and_return(schema_def) + allow(schema_def).to receive(:attributes).with(no_args).and_return(attributes) + allow(schema_def).to receive(:name).with(no_args).and_return(schema[:name]) described_class.register(schema) + end + it 'validates the connection_info' do expect(described_class).not_to receive(:require).with('puppet/transport/schema/validate') - 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) + expect(schema_def).to receive(:check_schema).with({}, kind_of(String)).and_return(nil) + expect(schema_def).to receive(:validate).with({}).and_return(nil) - described_class.send :validate, 'validate', 'connection_info' + described_class.send :validate, 'validate', {} + end + + context 'when validating bolt _target information' do + let(:attributes) { { host: {}, foo: {} } } + let(:context) { instance_double(Puppet::ResourceApi::PuppetContext, 'context') } + + it 'cleans the connection_info' do + allow(described_class).to receive(:get_context).with('validate').and_return(context) + + expect(schema_def).to receive(:check_schema).with({ host: 'host value', foo: 'foo value' }, kind_of(String)).and_return(nil) + expect(schema_def).to receive(:validate).with(host: 'host value', foo: 'foo value').and_return(nil) + + expect(context).to receive(:debug).with('Discarding bolt metaparameter: query') + expect(context).to receive(:debug).with('Discarding bolt metaparameter: remote-transport') + expect(context).to receive(:debug).with('Discarding bolt metaparameter: remote-reserved') + expect(context).to receive(:info).with('Discarding superfluous bolt attribute: user') + expect(context).to receive(:warning).with('Discarding unknown attribute: bar') + described_class.send :validate, 'validate', :"remote-transport" => 'validate', + :host => 'host value', + :foo => 'foo value', + :user => 'superfluous bolt value', + :query => 'metaparameter value', + :"remote-reserved" => 'reserved value', + :bar => 'unknown attribute' + end end end end