Merge pull request #182 from DavidS/pup-9747-bolt-attribute-filtering

(PUP-9747) Relax validation for bolt

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

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