(PUP-11209) Puppet agent silently skips unknown resources.

BobosilaVictor · BobosilaVictor · commit da8b73edca17 · 2021-09-30T09:44:11.000+03:00
Previously all unknown resources were converted into a component
(Puppet::Type::Component) and silently skipped.

This commit adds a new resource attribute (:kind) that specifies the resource
kind, which is used to differentiate between built-in types and user defined
types.

When converting the catalog to a RAL catalog, this new attribute is used to to
check whether a resource should use the resource type's :new method, or create
a component, and in the case where the server considers a resource to be
compilable, but the agent can't resolve it, then we raise an error
diff --git a/acceptance/tests/agent/agent_fails_with_unknown_resource.rb b/acceptance/tests/agent/agent_fails_with_unknown_resource.rb
@@ -0,0 +1,77 @@
+test_name "agent run should fail if it finds an unknown resource type" do
+  tag 'audit:high',
+      'audit:integration'
+
+  require 'puppet/acceptance/common_utils'
+
+  require 'puppet/acceptance/environment_utils'
+  extend Puppet::Acceptance::EnvironmentUtils
+
+  require 'puppet/acceptance/temp_file_utils'
+  extend Puppet::Acceptance::TempFileUtils
+  
+  step "agent should fail when it can't find a resource" do
+    vendor_modules_path = master.tmpdir('vendor_modules')
+    tmp_environment = mk_tmp_environment_with_teardown(master, 'tmp')
+
+    site_pp_content = <<-SITEPP
+      define foocreateresource($one) {
+        $msg = 'hello'
+        notify { $name: message => $msg }
+      }
+      class example($x) {
+        if $x == undef or $x == [] or $x == '' {
+          notice 'foo'
+          return()
+        }
+        notice 'bar'
+      }
+      node default {
+        class { example: x => [] }
+        create_resources('foocreateresource', {'blah'=>{'one'=>'two'}})
+        mycustomtype{'foobar':}
+      }
+    SITEPP
+    manifests_path = "/tmp/#{tmp_environment}/manifests"
+    on(master, "mkdir -p '#{manifests_path}'")
+    create_remote_file(master, "#{manifests_path}/site.pp", site_pp_content)
+
+    custom_type_content = <<-CUSTOMTYPE
+      Puppet::Type.newtype(:mycustomtype) do
+        @doc = "Create a new mycustomtype thing."
+
+        newparam(:name, :namevar => true) do
+          desc "Name of mycustomtype instance"
+        end
+
+        def refresh
+        end
+      end
+    CUSTOMTYPE
+    type_path = "#{vendor_modules_path}/foo/lib/puppet/type"
+    on(master, "mkdir -p '#{type_path}'")
+    create_remote_file(master, "#{type_path}/mycustomtype.rb", custom_type_content)
+
+    on(master, "chmod -R 750 '#{vendor_modules_path}' '/tmp/#{tmp_environment}'")
+    on(master, "chown -R #{master.puppet['user']}:#{master.puppet['group']} '#{vendor_modules_path}' '/tmp/#{tmp_environment}'")
+
+    master_opts = {
+      'main' => {
+        'environment' => tmp_environment,
+        'vendormoduledir' => vendor_modules_path
+       }
+    }
+
+    with_puppet_running_on(master, master_opts) do
+      agents.each do |agent|
+        teardown do
+          agent.rm_rf(vendor_modules_path)
+        end
+
+        on(agent, puppet('agent', '-t', '--environment', tmp_environment), acceptable_exit_codes: [1]) do |result|  
+          assert_match(/Error: Failed to apply catalog: Resource type 'Mycustomtype' was not found/, result.stderr)
+        end
+      end
+    end
+  end
+end
diff --git a/api/schemas/catalog.json b/api/schemas/catalog.json
@@ -45,6 +45,9 @@
                     "line": {
                         "type": "integer"
                     },
+                    "kind": {
+                        "type": "string"
+                    },
                     "file": {
                         "type": "string"
                     },
diff --git a/benchmarks/serialization/catalog.json b/benchmarks/serialization/catalog.json
@@ -118,7 +118,7 @@
   "version": 1492108311,
   "code_id": null,
   "catalog_uuid": "c85cdf7e-f56d-4fc7-b513-3a00532cee91",
-  "catalog_format": 1,
+  "catalog_format": 2,
   "environment": "production",
   "resources": [
     {
diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
@@ -13,7 +13,7 @@ class Puppet::Parser::Resource < Puppet::Resource
 
   attr_accessor :source, :scope, :collector_id
   attr_accessor :virtual, :override, :translated, :catalog, :evaluated
-  attr_accessor :file, :line
+  attr_accessor :file, :line, :kind
 
   attr_reader :exported, :parameters
 
diff --git a/lib/puppet/pops/evaluator/runtime3_resource_support.rb b/lib/puppet/pops/evaluator/runtime3_resource_support.rb
@@ -40,6 +40,7 @@ def self.create_resources(file, line, scope, virtual, exported, type_name, resou
           :parameters => evaluated_parameters,
           :file => file,
           :line => line,
+          :kind => Puppet::Resource.to_kind(resolved_type),
           :exported => exported,
           :virtual => virtual,
           # WTF is this? Which source is this? The file? The name of the context ?
diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb
@@ -11,7 +11,7 @@ class Puppet::Resource
   include Puppet::Util::PsychSupport
 
   include Enumerable
-  attr_accessor :file, :line, :catalog, :exported, :virtual, :strict
+  attr_accessor :file, :line, :catalog, :exported, :virtual, :strict, :kind
   attr_reader :type, :title, :parameters
 
   # @!attribute [rw] sensitive_parameters
@@ -29,11 +29,16 @@ class Puppet::Resource
   EMPTY_ARRAY = [].freeze
   EMPTY_HASH = {}.freeze
 
-  ATTRIBUTES = [:file, :line, :exported].freeze
+  ATTRIBUTES = [:file, :line, :exported, :kind].freeze
   TYPE_CLASS = 'Class'.freeze
   TYPE_NODE  = 'Node'.freeze
   TYPE_SITE  = 'Site'.freeze
 
+  CLASS_STRING = 'class'.freeze
+  DEFINED_TYPE_STRING = 'defined_type'.freeze
+  COMPILABLE_TYPE_STRING = 'compilable_type'.freeze
+  UNKNOWN_TYPE_STRING  = 'unknown'.freeze
+
   PCORE_TYPE_KEY = '__ptype'.freeze
   VALUE_KEY = 'value'.freeze
 
@@ -194,6 +199,18 @@ def builtin_type?
     resource_type.is_a?(Puppet::CompilableResourceType)
   end
 
+  def self.to_kind(resource_type)
+    if resource_type == CLASS_STRING
+      CLASS_STRING
+    elsif resource_type.is_a?(Puppet::Resource::Type) && resource_type.type == :definition
+      DEFINED_TYPE_STRING
+    elsif resource_type.is_a?(Puppet::CompilableResourceType)
+      COMPILABLE_TYPE_STRING
+    else
+      UNKNOWN_TYPE_STRING
+    end
+  end
+
   # Iterate over each param/value pair, as required for Enumerable.
   def each
     parameters.each { |p,v| yield p, v }
@@ -248,6 +265,7 @@ def initialize(type, title = nil, attributes = EMPTY_HASH)
       src = type
       self.file = src.file
       self.line = src.line
+      self.kind = src.kind
       self.exported = src.exported
       self.virtual = src.virtual
       self.set_tags(src)
@@ -310,6 +328,7 @@ def initialize(type, title = nil, attributes = EMPTY_HASH)
 
       rt = resource_type
 
+      self.kind = self.class.to_kind(rt) unless kind
       if strict? && rt.nil?
         if self.class?
           raise ArgumentError, _("Could not find declared class %{title}") % { title: title }
@@ -493,10 +512,24 @@ def to_ref
     ref
   end
 
-  # Convert our resource to a RAL resource instance.  Creates component
-  # instances for resource types that don't exist.
+  # Convert our resource to a RAL resource instance. Creates component
+  # instances for resource types that are not of a compilable_type kind. In case
+  # the resource doesn’t exist and it’s compilable_type kind, raise an error.
+  # There are certain cases where a resource won't be in a catalog, such as 
+  # when we create a resource directly by using Puppet::Resource.new(...), so we 
+  # must check its kind before deciding whether the catalog format is of an older
+  # version or not.
   def to_ral
-    typeklass = Puppet::Type.type(self.type) || Puppet::Type.type(:component)
+    if self.kind == COMPILABLE_TYPE_STRING
+      typeklass = Puppet::Type.type(self.type)
+    elsif self.catalog && self.catalog.catalog_format >= 2
+      typeklass = Puppet::Type.type(:component)
+    else
+      typeklass =  Puppet::Type.type(self.type) || Puppet::Type.type(:component)
+    end
+
+    raise(Puppet::Error, "Resource type '#{self.type}' was not found") unless typeklass
+
     typeklass.new(self)
   end
 
diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
@@ -315,7 +315,7 @@ def initialize(name = nil, environment = Puppet::Node::Environment::NONE, code_i
     super()
     @name = name
     @catalog_uuid = SecureRandom.uuid
-    @catalog_format = 1
+    @catalog_format = 2
     @metadata = {}
     @recursive_metadata = {}
     @classes = []
diff --git a/spec/fixtures/integration/application/agent/cached_deferred_catalog.json b/spec/fixtures/integration/application/agent/cached_deferred_catalog.json
@@ -6,7 +6,7 @@
   "version": 1607629733,
   "code_id": null,
   "catalog_uuid": "afc8472a-306b-4b24-b060-e956dffb79b8",
-  "catalog_format": 1,
+  "catalog_format": 2,
   "environment": "production",
   "resources": [
     {
@@ -50,6 +50,7 @@
       ],
       "file": "",
       "line": 1,
+      "kind": "compilable_type",
       "exported": false,
       "parameters": {
         "message": {
diff --git a/spec/integration/parser/pcore_resource_spec.rb b/spec/integration/parser/pcore_resource_spec.rb
@@ -136,6 +136,16 @@ module Puppet
       expect(catalog.resource(:cap, "c")['message']).to eq('c works')
     end
 
+    it 'considers Pcore types to be builtin ' do
+      genface.types
+      catalog = compile_to_catalog(<<-MANIFEST)
+        test1 { 'a':
+          message => 'a works'
+        }
+      MANIFEST
+      expect(catalog.resource(:test1, "a").kind).to eq('compilable_type')
+    end
+
     it 'the validity of attribute names are checked' do
       genface.types
       expect do
diff --git a/spec/unit/configurer_spec.rb b/spec/unit/configurer_spec.rb
@@ -742,7 +742,7 @@ def expects_neither_new_or_cached_catalog
           expect(configurer.run).to be_nil
         end
 
-        it "should proceed with the cached catalog if its environment matchs the local environment" do
+        it "should proceed with the cached catalog if its environment matches the local environment" do
           expects_cached_catalog_only(catalog)
 
           expect(configurer.run).to eq(0)
diff --git a/spec/unit/resource/catalog_spec.rb b/spec/unit/resource/catalog_spec.rb
@@ -104,7 +104,7 @@
 
   it "should include the current catalog_format" do
     catalog = Puppet::Resource::Catalog.new("host")
-    expect(catalog.catalog_format).to eq(1)
+    expect(catalog.catalog_format).to eq(2)
   end
 
   describe "when compiling" do
@@ -178,6 +178,7 @@
       @original.add_edge(@middle, @bottom)
       @original.add_edge(@bottom, @bottomobject)
 
+      @original.catalog_format = 1
       @catalog = @original.to_ral
     end
 
@@ -190,6 +191,18 @@
       end
     end
 
+    it "should raise if an unknown resource is being converted" do
+      @new_res = Puppet::Resource.new "Unknown", "type", :kind => 'compilable_type'
+      @resource_array = [@new_res]
+
+      @original.add_resource(*@resource_array)
+      @original.add_edge(@bottomobject, @new_res)
+
+      @original.catalog_format = 2
+
+      expect { @original.to_ral }.to raise_error(Puppet::Error, "Resource type 'Unknown' was not found")
+    end
+
     it "should copy the tag list to the new catalog" do
       expect(@catalog.tags.sort).to eq(@original.tags.sort)
     end
diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb
@@ -694,19 +694,68 @@ def inject_and_set_defaults(resource, scope)
     it "should use the resource type's :new method to create the resource if the resource is of a builtin type" do
       resource = Puppet::Resource.new("file", basepath+"/my/file")
       result = resource.to_ral
+
       expect(result).to be_instance_of(Puppet::Type.type(:file))
       expect(result[:path]).to eq(basepath+"/my/file")
     end
 
-    it "should convert to a component instance if the resource type is not of a builtin type" do
+    it "should convert to a component instance if the resource is not a compilable_type" do
       resource = Puppet::Resource.new("foobar", "somename")
       result = resource.to_ral
 
       expect(result).to be_instance_of(Puppet::Type.type(:component))
       expect(result.title).to eq("Foobar[somename]")
     end
-  end
 
+    it "should convert to a component instance if the resource is a class" do
+      resource = Puppet::Resource.new("Class", "somename")
+      result = resource.to_ral
+
+      expect(result).to be_instance_of(Puppet::Type.type(:component))
+      expect(result.title).to eq("Class[Somename]")
+    end
+
+    it "should convert to component when the resource is a defined_type" do
+      resource = Puppet::Resource.new("Unknown", "type", :kind => 'defined_type')
+
+      result = resource.to_ral
+      expect(result).to be_instance_of(Puppet::Type.type(:component))
+    end
+
+    it "should raise if a resource type is a compilable_type and it wasn't found" do
+      resource = Puppet::Resource.new("Unknown", "type", :kind => 'compilable_type')
+
+      expect { resource.to_ral }.to raise_error(Puppet::Error, "Resource type 'Unknown' was not found")
+    end
+
+    it "should use the old behaviour when the catalog_format is equal to 1" do
+      resource = Puppet::Resource.new("Unknown", "type")
+      catalog = Puppet::Resource::Catalog.new("mynode")
+
+      resource.catalog = catalog
+      resource.catalog.catalog_format = 1
+
+      result = resource.to_ral
+      expect(result).to be_instance_of(Puppet::Type.type(:component))
+    end
+
+    it "should use the new behaviour and fail when the catalog_format is greater than 1" do
+      resource = Puppet::Resource.new("Unknown", "type", :kind => 'compilable_type')
+      catalog = Puppet::Resource::Catalog.new("mynode")
+
+      resource.catalog = catalog
+      resource.catalog.catalog_format = 2
+
+      expect { resource.to_ral }.to raise_error(Puppet::Error, "Resource type 'Unknown' was not found")
+    end
+
+    it "should use the resource type when the resource doesn't respond to kind and the resource type can be found" do
+      resource = Puppet::Resource.new("file", basepath+"/my/file")
+
+      result = resource.to_ral
+      expect(result).to be_instance_of(Puppet::Type.type(:file))
+    end
+  end
   describe "when converting to puppet code" do
     before do
       @resource = Puppet::Resource.new("one::two", "/my/file",
@@ -822,6 +871,13 @@ def inject_and_set_defaults(resource, scope)
       expect(Puppet::Resource.from_data_hash(JSON.parse(resource.to_json)).line).to eq(50)
     end
 
+    it "should include the kind if one is set" do
+      resource = Puppet::Resource.new("File", "/foo")
+      resource.kind = 'im_a_file'
+
+      expect(Puppet::Resource.from_data_hash(JSON.parse(resource.to_json)).kind).to eq('im_a_file')
+    end
+
     it "should include the 'exported' value if one is set" do
       resource = Puppet::Resource.new("File", "/foo")
       resource.exported = true

Original file line number	Diff line number	Diff line change
`@@ -118,7 +118,7 @@`
`118`	`118`	`"version": 1492108311,`
`119`	`119`	`"code_id": null,`
`120`	`120`	`"catalog_uuid": "c85cdf7e-f56d-4fc7-b513-3a00532cee91",`
`121`		`- "catalog_format": 1,`
	`121`	`+ "catalog_format": 2,`
`122`	`122`	`"environment": "production",`
`123`	`123`	`"resources": [`
`124`	`124`	`{`