Merge pull request #11803 from chef/lcg/policyfile-run-list

3 files changed, 160 insertions, 61 deletions

diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index 11c510f297..3eb8c8475c 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -644,6 +644,16 @@ module ChefConfig
     # effect if `policy_document_native_api` is set to `false`.
     default :deployment_group, nil
 
+    # When using policyfiles you can optionally set it to read the node.run_list
+    # from the server and have that override the policyfile run_list or the
+    # named_run_list set in config.  With policyfiles there is no depsolving done
+    # on the run_list items so every item in the run_list must be in the set of
+    # cookbooks pushed to the node.  This enables flows where the node can change
+    # its run_list and have it persist or to bootstrap nodes with the -j flag.  If
+    # no run_list is set on the server node object then the configured named_run_list
+    # or run_list out of the policy is used.
+    default :policy_persist_run_list, false
+
     # Set these to enable SSL authentication / mutual-authentication
     # with the server
 
diff --git a/lib/chef/policy_builder/policyfile.rb b/lib/chef/policy_builder/policyfile.rb
index 16fdf49256..0603acf958 100644
--- a/lib/chef/policy_builder/policyfile.rb
+++ b/lib/chef/policy_builder/policyfile.rb
@@ -32,14 +32,8 @@ class Chef
     # Policyfile is a policy builder implementation that gets run
     # list and cookbook version information from a single document.
     #
-    # == Unsupported Options:
-    # * override_runlist:: This could potentially be integrated into the
-    # policyfile, or replaced with a similar feature that has different
-    # semantics.
-    # * specific_recipes:: put more design thought into this use case.
-    # * run_list in json_attribs:: would be ignored anyway, so it raises an error.
-    # * chef-solo:: not currently supported. Need more design thought around
-    # how this should work.
+    # Does not support legacy chef-solo or roles/environments.
+    #
     class Policyfile
 
       class UnsupportedFeature < StandardError; end
@@ -96,10 +90,6 @@ class Chef
           raise UnsupportedFeature, "Policyfile does not support chef-solo. Use #{ChefUtils::Dist::Infra::CLIENT} local mode instead."
         end
 
-        if json_attribs && json_attribs.key?("run_list")
-          raise UnsupportedFeature, "Policyfile does not support setting the run_list in json data."
-        end
-
         if Chef::Config[:environment] && !Chef::Config[:environment].chomp.empty?
           raise UnsupportedFeature, "Policyfile does not work with an Environment configured."
         end
@@ -147,6 +137,9 @@ class Chef
         expand_run_list
         apply_policyfile_attributes
 
+        if persistent_run_list_set?
+          Chef::Log.warn("The node.run_list setting is overriding the Policyfile run_list")
+        end
         Chef::Log.info("Run List is [#{run_list}]")
         Chef::Log.info("Run List expands to [#{run_list_with_versions_for_display(run_list).join(", ")}]")
 
@@ -203,7 +196,7 @@ class Chef
       #
       # @return [RunListExpansionIsh] A RunListExpansion duck-type.
       def expand_run_list
-        CookbookCacheCleaner.instance.skip_removal = true if named_run_list_requested?
+        validate_run_list!(run_list)
 
         node.run_list(run_list)
         node.automatic_attrs[:policy_revision] = revision_id
@@ -232,6 +225,18 @@ class Chef
 
       # @api private
       #
+      # Validate run_list against policyfile cookbooks
+      #
+      def validate_run_list!(run_list)
+        run_list.map do |recipe_spec|
+          cookbook, recipe = parse_recipe_spec(recipe_spec)
+          lock_data = cookbook_lock_for(cookbook)
+          raise PolicyfileError, "invalid run_list item '#{recipe_spec}' not in cookbook set of PolicyFile #{policyfile_location}" unless lock_data
+        end
+      end
+
+      # @api private
+      #
       # Generates an array of strings with recipe names including version and
       # identifier info.
       def run_list_with_versions_for_display(run_list)
@@ -278,7 +283,12 @@ class Chef
       def parse_recipe_spec(recipe_spec)
         rmatch = recipe_spec.to_s.match(/recipe\[([^:]+)::([^:]+)\]/)
         if rmatch.nil?
-          raise PolicyfileError, "invalid recipe specification #{recipe_spec} in Policyfile from #{policyfile_location}"
+          rmatch = recipe_spec.to_s.match(/recipe\[([^:]+)\]/)
+          if rmatch.nil?
+            raise PolicyfileError, "invalid recipe specification #{recipe_spec} in Policyfile from #{policyfile_location}"
+          else
+            [rmatch[1], "default"]
+          end
         else
           [rmatch[1], rmatch[2]]
         end
@@ -294,7 +304,11 @@ class Chef
       def run_list
         return override_runlist.map(&:to_s) if override_runlist
 
-        if named_run_list_requested?
+        if json_attribs["run_list"]
+          json_attribs["run_list"]
+        elsif persistent_run_list_set?
+          node.run_list
+        elsif named_run_list_requested?
           named_run_list || raise(ConfigurationError,
             "Policy '#{retrieved_policy_name}' revision '#{revision_id}' does not have named_run_list '#{named_run_list_name}'" +
             "(available named_run_lists: [#{available_named_run_lists.join(", ")}])")
@@ -450,7 +464,7 @@ class Chef
       # should be reduced to a single call.
       def cookbooks_to_sync
         @cookbook_to_sync ||= begin
-          events.cookbook_resolution_start(run_list_with_versions_for_display(policy["run_list"]))
+          events.cookbook_resolution_start(run_list_with_versions_for_display(run_list))
 
           cookbook_versions_by_name = cookbook_locks.inject({}) do |cb_map, (name, lock_data)|
             cb_map[name] = manifest_for(name, lock_data)
@@ -462,7 +476,7 @@ class Chef
         end
       rescue Exception => e
         # TODO: wrap/munge exception to provide helpful error output
-        events.cookbook_resolution_failed(run_list_with_versions_for_display(policy["run_list"]), e)
+        events.cookbook_resolution_failed(run_list_with_versions_for_display(run_list), e)
         raise
       end
 
@@ -532,6 +546,10 @@ class Chef
         (policy["named_run_lists"] || {}).keys
       end
 
+      def persistent_run_list_set?
+        Chef::Config[:policy_persist_run_list] && node.run_list && !node.run_list.empty?
+      end
+
       def named_run_list_requested?
         !!Chef::Config[:named_run_list]
       end
diff --git a/spec/unit/policy_builder/policyfile_spec.rb b/spec/unit/policy_builder/policyfile_spec.rb
index 63dc300f2a..d3ad6154c0 100644
--- a/spec/unit/policy_builder/policyfile_spec.rb
+++ b/spec/unit/policy_builder/policyfile_spec.rb
@@ -72,40 +72,40 @@ describe Chef::PolicyBuilder::Policyfile do
 
   let(:policyfile_default_attributes) do
     {
-        "policyfile_default_attr" => "policyfile_default_value",
-        "top_level_attr" => "hat",
-        "baseline_attr" => {
-          "one" => 1,
-          "two" => 2,
-          "deep" => {
-              "three" => 3,
-              "four" => [4],
-              "five" => [5],
-          },
+      "policyfile_default_attr" => "policyfile_default_value",
+      "top_level_attr" => "hat",
+      "baseline_attr" => {
+        "one" => 1,
+        "two" => 2,
+        "deep" => {
+          "three" => 3,
+          "four" => [4],
+          "five" => [5],
         },
-        "policy_group_value" => {
-          "baseline_attr" => {
-            "one" => 111,
-          },
+      },
+      "policy_group_value" => {
+        "baseline_attr" => {
+          "one" => 111,
         },
-      }
+      },
+    }
   end
 
   let(:policyfile_override_attributes) do
     {
-       "policyfile_override_attr" => "policyfile_override_value",
-       "baseline_attr" => {
-         "deep" => {
-           "three" => 333 },
-       },
-       "policy_group_value" => {
-         "top_level_attr" => "cat",
-         "baseline_attr" => {
-           "deep" => {
-             "four" => [444],
-           },
-         },
-       },
+      "policyfile_override_attr" => "policyfile_override_value",
+      "baseline_attr" => {
+        "deep" => {
+          "three" => 333 },
+      },
+      "policy_group_value" => {
+        "top_level_attr" => "cat",
+        "baseline_attr" => {
+          "deep" => {
+            "four" => [444],
+          },
+        },
+      },
     }
   end
 
@@ -162,14 +162,6 @@ describe Chef::PolicyBuilder::Policyfile do
       end
     end
 
-    context "when json_attribs contains a run_list" do
-      let(:json_attribs) { { "run_list" => [] } }
-
-      it "errors on create" do
-        expect { initialize_pb }.to raise_error(err_namespace::UnsupportedFeature)
-      end
-    end
-
     context "when an environment is configured" do
       before { Chef::Config[:environment] = "blurch" }
 
@@ -347,9 +339,9 @@ describe Chef::PolicyBuilder::Policyfile do
           expect { policy_builder.validate_policyfile }.to raise_error(err_namespace::PolicyfileError)
         end
 
-        it "errors if the policyfile json contains non-fully qualified recipe items" do
+        it "does not error if the policyfile json contains non-fully qualified recipe items" do
           parsed_policyfile_json["run_list"] = ["recipe[foo]"]
-          expect { policy_builder.validate_policyfile }.to raise_error(err_namespace::PolicyfileError)
+          expect { policy_builder.validate_policyfile }.not_to raise_error
         end
 
         it "errors if the policyfile doesn't have a run_list key" do
@@ -397,8 +389,8 @@ describe Chef::PolicyBuilder::Policyfile do
             {
               id: "_policy_node",
               run_list: [
-               { type: "recipe", name: "test::default", skipped: false, version: nil },
-               { type: "recipe", name: "test::other", skipped: false, version: nil },
+                { type: "recipe", name: "test::default", skipped: false, version: nil },
+                { type: "recipe", name: "test::other", skipped: false, version: nil },
               ],
             }
           end
@@ -670,10 +662,6 @@ describe Chef::PolicyBuilder::Policyfile do
               expect(node[:recipes]).to eq( ["example1::default"] )
             end
 
-            it "disables the cookbook cache cleaner" do
-              expect(Chef::CookbookCacheCleaner.instance.skip_removal).to be(true)
-            end
-
           end
         end
 
@@ -884,6 +872,89 @@ describe Chef::PolicyBuilder::Policyfile do
 
         end
       end
+
+      describe "selecting the run_list" do
+        let(:node) do
+          node = Chef::Node.new
+          node.name(node_name)
+          node
+        end
+
+        before do
+          allow(policy_builder).to receive(:node).and_return(node)
+        end
+
+        context "when json_attribs contains a run_list" do
+          let(:json_attribs) { { "run_list" => [ "recipe[something::default]" ] } }
+
+          it "reads the run_list from the json_attribs" do
+            expect(policy_builder.run_list).to eql(json_attribs["run_list"])
+          end
+
+          it "ignores the node.run_list" do
+            node.run_list.reset!("recipe[incorrect::incorrect]")
+            expect(policy_builder.run_list).to eql(json_attribs["run_list"])
+          end
+
+          it "ignores the node.run_list if the Chef::Config value is set" do
+            Chef::Config[:policy_persist_run_list] = true
+            node.run_list.reset!("recipe[incorrect::incorrect]")
+            expect(policy_builder.run_list).to eql(json_attribs["run_list"])
+          end
+        end
+
+        it "reads the run_list from the policyfile" do
+          expect(policy_builder.run_list).to eql(policyfile_run_list)
+        end
+
+        it "ignores the node.run_list by default" do
+          node.run_list.reset!("recipe[incorrect::incorrect]")
+          expect(policy_builder.run_list).to eql(policyfile_run_list)
+        end
+
+        it "uses the node.run_list if the Chef::Config value is set" do
+          Chef::Config[:policy_persist_run_list] = true
+          node.run_list.reset!("recipe[correct::default]")
+          expect(policy_builder.run_list).to eql(node.run_list)
+        end
+
+        it "does not use an empty node.run_list" do
+          Chef::Config[:policy_persist_run_list] = true
+          node.run_list.reset!
+          expect(policy_builder.run_list).to eql(policyfile_run_list)
+        end
+
+        context "with a valid named_run_list" do
+          let(:parsed_policyfile_json) do
+            basic_valid_policy_data.dup.tap do |p|
+              p["named_run_lists"] = {
+                "deploy-app" => [ "recipe[example1::default]" ],
+              }
+            end
+          end
+
+          it "uses the named_run_list over the policyfile" do
+            Chef::Config[:named_run_list] = "deploy-app"
+            expect(policy_builder.run_list).to eq([ "recipe[example1::default]" ])
+          end
+
+          it "is overridden if the run_list is persistent" do
+            Chef::Config[:named_run_list] = "deploy-app"
+            Chef::Config[:policy_persist_run_list] = true
+            node.run_list.reset!("recipe[correct::default]")
+            expect(policy_builder.run_list).to eql(node.run_list)
+          end
+
+          context "when json_attribs contains a run_list" do
+            let(:json_attribs) { { "run_list" => [ "recipe[something::default]" ] } }
+
+            it "overrides the named_run_list" do
+              expect(policy_builder.run_list).to eql(json_attribs["run_list"])
+            end
+          end
+
+        end
+      end
     end
   end
 end