Merge pull request #7224 from coderanger/map-lock

Implement rfc107: NodeMap locking for resource and provider handlers

6 files changed, 192 insertions, 5 deletions

diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index 7218c3bb49..33d625c54e 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -275,6 +275,9 @@ class Chef
 
         do_windows_admin_check
 
+        Chef.resource_handler_map.lock!
+        Chef.provider_handler_map.lock!
+
         run_context = setup_run_context
 
         load_required_recipe(@rest, run_context) unless Chef::Config[:solo_legacy_mode]
diff --git a/lib/chef/node_map.rb b/lib/chef/node_map.rb
index 0406b3c1d6..634786af93 100644
--- a/lib/chef/node_map.rb
+++ b/lib/chef/node_map.rb
@@ -45,12 +45,18 @@ class Chef
     # @param key [Object] Key to store
     # @param value [Object] Value associated with the key
     # @param filters [Hash] Node filter options to apply to key retrieval
+    # @param allow_cookbook_override [Boolean, String] Allow a cookbook to add
+    #   to this key even in locked mode. If a string is given, it should be a
+    #   Gem::Requirement-compatible value indicating for which Chef versions an
+    #   override from cookbooks is allowed.
+    # @param __core_override__ [Boolean] Advanced-mode override to add to a key
+    #   even in locked mode.
     #
     # @yield [node] Arbitrary node filter as a block which takes a node argument
     #
     # @return [NodeMap] Returns self for possible chaining
     #
-    def set(key, klass, platform: nil, platform_version: nil, platform_family: nil, os: nil, canonical: nil, override: nil, &block)
+    def set(key, klass, platform: nil, platform_version: nil, platform_family: nil, os: nil, canonical: nil, override: nil, allow_cookbook_override: false, __core_override__: false, &block) # rubocop:disable Lint/UnderscorePrefixedVariableName
       new_matcher = { klass: klass }
       new_matcher[:platform] = platform if platform
       new_matcher[:platform_version] = platform_version if platform_version
@@ -59,6 +65,31 @@ class Chef
       new_matcher[:block] = block if block
       new_matcher[:canonical] = canonical if canonical
       new_matcher[:override] = override if override
+      new_matcher[:cookbook_override] = allow_cookbook_override
+      new_matcher[:core_override] = __core_override__
+
+      # Check if the key is already present and locked, unless the override is allowed.
+      # The checks to see if we should reject, in order:
+      # 1. Core override mode is not set.
+      # 2. The key exists.
+      # 3. At least one previous `provides` is now locked.
+      # 4. No previous `provides` had `allow_cookbook_override`, either set to
+      #    true or with a string version matcher that still matches Chef::VERSION
+      if !__core_override__ && map[key] && map[key].any? { |matcher| matcher[:locked] } && !map[key].any? { |matcher| matcher[:cookbook_override].is_a?(String) ? Chef::VERSION =~ matcher[:cookbook_override] : matcher[:cookbook_override] }
+        # If we ever use locked mode on things other than the resource and provider handler maps, this probably needs a tweak.
+        type_of_thing = if klass < Chef::Resource
+                          "resource"
+                        elsif klass < Chef::Provider
+                          "provider"
+                        else
+                          klass.superclass.to_s
+                        end
+        # For now, only log the warning.
+        Chef.log_deprecation("Trying to register #{type_of_thing} #{key} on top of existing Chef core #{type_of_thing}. Check if a new version of the cookbook is available.")
+        # In 15.0, uncomment this and remove the log above.
+        # Chef.log_deprecation("Rejecting attempt to register #{type_of_thing} #{key} on top of existing Chef core #{type_of_thing}. Check if a new version of the cookbook is available.")
+        # return
+      end
 
       # The map is sorted in order of preference already; we just need to find
       # our place in it (just before the first value with the same preference level).
@@ -159,6 +190,34 @@ class Chef
       remaining
     end
 
+    # Check if this map has been locked.
+    #
+    # @api internal
+    # @since 14.2
+    # @return [Boolean]
+    def locked?
+      if defined?(@locked)
+        @locked
+      else
+        false
+      end
+    end
+
+    # Set this map to locked mode. This is used to prevent future overwriting
+    # of existing names.
+    #
+    # @api internal
+    # @since 14.2
+    # @return [void]
+    def lock!
+      map.each do |key, matchers|
+        matchers.each do |matcher|
+          matcher[:locked] = true
+        end
+      end
+      @locked = true
+    end
+
     private
 
     #
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index 05349b80e7..478637ff4e 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -1161,6 +1161,22 @@ class Chef
         end
     end
 
+    # Set or return if this resource is in preview mode.
+    #
+    # This is used in Chef core as part of the process of migrating resources
+    # from a cookbook into core. It should be set to `true` when a cookbook
+    # resource is added to core, and then removed (set to `false`) in the next
+    # major release.
+    #
+    # @param value [nil, Boolean] If nil, get the current value. If not nil, set
+    #   the value of the flag.
+    # @return [Boolean]
+    def self.preview_resource(value = nil)
+      @preview_resource = false unless defined?(@preview_resource)
+      @preview_resource = value unless value.nil?
+      @preview_resource
+    end
+
     #
     # Internal Resource Interface (for Chef)
     #
@@ -1305,6 +1321,12 @@ class Chef
         remove_canonical_dsl
       end
 
+      # If a resource is in preview mode, set allow_cookbook_override on all its
+      # mappings by default.
+      if preview_resource && !options.include?(:allow_cookbook_override)
+        options[:allow_cookbook_override] = true
+      end
+
       result = Chef.resource_handler_map.set(name, self, options, &block)
       Chef::DSL::Resources.add_resource_dsl(name)
       result
diff --git a/lib/chef/resource_inspector.rb b/lib/chef/resource_inspector.rb
index bbec03ffd6..10fa42c842 100644
--- a/lib/chef/resource_inspector.rb
+++ b/lib/chef/resource_inspector.rb
@@ -43,6 +43,7 @@ module ResourceInspector
     data[:actions] = resource.allowed_actions
     data[:examples] = resource.examples
     data[:introduced] = resource.introduced
+    data[:preview] = resource.preview_resource
 
     properties = unless complete
                    resource.properties.reject { |_, k| k.options[:declared_in] == Chef::Resource }
diff --git a/spec/unit/node_map_spec.rb b/spec/unit/node_map_spec.rb
index 24f3bebe2a..7e4980219a 100644
--- a/spec/unit/node_map_spec.rb
+++ b/spec/unit/node_map_spec.rb
@@ -22,6 +22,12 @@ require "chef/node_map"
 class Foo; end
 class Bar; end
 
+class FooResource < Chef::Resource; end
+class BarResource < Chef::Resource; end
+
+class FooProvider < Chef::Provider; end
+class BarProvider < Chef::Provider; end
+
 describe Chef::NodeMap do
 
   let(:node_map) { Chef::NodeMap.new }
@@ -139,14 +145,14 @@ describe Chef::NodeMap do
   describe "deleting classes" do
     it "deletes a class and removes the mapping completely" do
       node_map.set(:thing, Bar)
-      expect( node_map.delete_class(Bar) ).to eql({ :thing => [{ :klass => Bar }] })
+      expect( node_map.delete_class(Bar) ).to include({ :thing => [{ :klass => Bar, :cookbook_override => false, :core_override => false }] })
       expect( node_map.get(node, :thing) ).to eql(nil)
     end
 
     it "deletes a class and leaves the mapping that still has an entry" do
       node_map.set(:thing, Bar)
       node_map.set(:thing, Foo)
-      expect( node_map.delete_class(Bar) ).to eql({ :thing => [{ :klass => Bar }] })
+      expect( node_map.delete_class(Bar) ).to eql({ :thing => [{ :klass => Bar, :cookbook_override => false, :core_override => false }] })
       expect( node_map.get(node, :thing) ).to eql(Foo)
     end
 
@@ -154,7 +160,7 @@ describe Chef::NodeMap do
       node_map.set(:thing1, Bar)
       node_map.set(:thing2, Bar)
       node_map.set(:thing2, Foo)
-      expect( node_map.delete_class(Bar) ).to eql({ :thing1 => [{ :klass => Bar }], :thing2 => [{ :klass => Bar }] })
+      expect( node_map.delete_class(Bar) ).to eql({ :thing1 => [{ :klass => Bar, :cookbook_override => false, :core_override => false }], :thing2 => [{ :klass => Bar, :cookbook_override => false, :core_override => false }] })
       expect( node_map.get(node, :thing1) ).to eql(nil)
       expect( node_map.get(node, :thing2) ).to eql(Foo)
     end
@@ -204,4 +210,66 @@ describe Chef::NodeMap do
     end
   end
 
+  describe "locked mode" do
+    context "while unlocked" do
+      it "allows setting the same key twice" do
+        expect(Chef).to_not receive(:log_deprecation)
+        node_map.set(:foo, FooResource)
+        node_map.set(:foo, BarResource)
+        expect(node_map.get(node, :foo)).to eql(BarResource)
+      end
+    end
+
+    context "while locked" do
+      # Uncomment the commented `expect`s in 15.0.
+      it "rejects setting the same key twice" do
+        expect(Chef).to receive(:log_deprecation).with("Trying to register resource foo on top of existing Chef core resource. Check if a new version of the cookbook is available.")
+        node_map.set(:foo, FooResource)
+        node_map.lock!
+        node_map.set(:foo, BarResource)
+        # expect(node_map.get(node, :foo)).to eql(FooResource)
+      end
+
+      it "allows setting the same key twice when the first has allow_cookbook_override" do
+        expect(Chef).to_not receive(:log_deprecation)
+        node_map.set(:foo, FooResource, allow_cookbook_override: true)
+        node_map.lock!
+        node_map.set(:foo, BarResource)
+        expect(node_map.get(node, :foo)).to eql(BarResource)
+      end
+
+      it "allows setting the same key twice when the first has allow_cookbook_override with a future version" do
+        expect(Chef).to_not receive(:log_deprecation)
+        node_map.set(:foo, FooResource, allow_cookbook_override: "< 100")
+        node_map.lock!
+        node_map.set(:foo, BarResource)
+        expect(node_map.get(node, :foo)).to eql(BarResource)
+      end
+
+      it "rejects setting the same key twice when the first has allow_cookbook_override with a past version" do
+        expect(Chef).to receive(:log_deprecation).with("Trying to register resource foo on top of existing Chef core resource. Check if a new version of the cookbook is available.")
+        node_map.set(:foo, FooResource, allow_cookbook_override: "< 1")
+        node_map.lock!
+        node_map.set(:foo, BarResource)
+        # expect(node_map.get(node, :foo)).to eql(FooResource)
+      end
+
+      it "allows setting the same key twice when the second has __core_override__" do
+        expect(Chef).to_not receive(:log_deprecation)
+        node_map.set(:foo, FooResource)
+        node_map.lock!
+        node_map.set(:foo, BarResource, __core_override__: true)
+        expect(node_map.get(node, :foo)).to eql(BarResource)
+      end
+
+      it "rejects setting the same key twice for a provider" do
+        expect(Chef).to receive(:log_deprecation).with("Trying to register provider foo on top of existing Chef core provider. Check if a new version of the cookbook is available.")
+        node_map.set(:foo, FooProvider)
+        node_map.lock!
+        node_map.set(:foo, BarProvider)
+        # expect(node_map.get(node, :foo)).to eql(FooProvider)
+      end
+    end
+  end
+
 end
diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb
index 2866d5439f..26660c9415 100644
--- a/spec/unit/resource_spec.rb
+++ b/spec/unit/resource_spec.rb
@@ -867,7 +867,7 @@ end
       snitch_var1 = snitch_var2 = 0
       runner = Chef::Runner.new(run_context)
 
-      Chef::Provider::SnakeOil.provides :cat
+      Chef::Provider::SnakeOil.provides :cat, __core_override__: true
 
       resource1.only_if { snitch_var1 = 1 }
       resource1.not_if { snitch_var2 = 2 }
@@ -1144,4 +1144,38 @@ end
       it { is_expected.to eq [:two, :one] }
     end
   end
+
+  describe ".preview_resource" do
+    let(:klass) { Class.new(Chef::Resource) }
+
+    before do
+      allow(Chef::DSL::Resources).to receive(:add_resource_dsl).with(:test_resource)
+    end
+
+    it "defaults to false" do
+      expect(klass.preview_resource).to eq false
+    end
+
+    it "can be set to true" do
+      klass.preview_resource(true)
+      expect(klass.preview_resource).to eq true
+    end
+
+    it "does not affect provides by default" do
+      expect(Chef.resource_handler_map).to receive(:set).with(:test_resource, klass, { canonical: true })
+      klass.resource_name(:test_resource)
+    end
+
+    it "adds allow_cookbook_override when true" do
+      expect(Chef.resource_handler_map).to receive(:set).with(:test_resource, klass, { canonical: true, allow_cookbook_override: true })
+      klass.preview_resource(true)
+      klass.resource_name(:test_resource)
+    end
+
+    it "allows manually overriding back to false" do
+      expect(Chef.resource_handler_map).to receive(:set).with(:test_resource, klass, { allow_cookbook_override: false })
+      klass.preview_resource(true)
+      klass.provides(:test_resource, allow_cookbook_override: false)
+    end
+  end
 end