(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 puppetlabs/puppet-specifications#142

Author: DavidS

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).
 

Rakefile

@@ -14,7 +14,6 @@ end
 require 'rspec/core/rake_task'
 
 RSpec::Core::RakeTask.new(:spec) do |t|
-  Rake::Task[:spec_prep].invoke
   # thanks to the fixtures/modules/ symlinks this needs to exclude fixture modules explicitely
   excludes = ['fixtures/**/*.rb,fixtures/modules/*/**/*.rb']
   if RUBY_PLATFORM == 'java'
@@ -24,12 +23,20 @@ RSpec::Core::RakeTask.new(:spec) do |t|
   t.exclude_pattern = "spec/{#{excludes.join ','}}"
 end
 
+task :spec => :spec_prep
+
 namespace :spec do
   desc 'Run RSpec code examples with coverage collection'
   task :coverage do
     ENV['COVERAGE'] = 'yes'
     Rake::Task['spec'].execute
   end
+
+  RSpec::Core::RakeTask.new(:unit) do |t|
+    t.pattern = "spec/puppet/**/*_spec.rb,spec/integration/**/*_spec.rb"
+  end
+
+  task :unit => :spec_prep
 end
 
 #### LICENSE_FINDER ####

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

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