(PUP-1963) allow dependencies on/for generated resources

Previously, generated resources could not have edge metaprams or
participate in autorequire, since they were created AFTER those
transformations had occured on the catalog. This changes resource
generation to occur before those transformations.

Because of this, the containment edges for generated resources now end
up in the final relationship graph. This caused issues with purged
resources, since the purge check now sees that a whit depends on the
purged resource. This was resolved by excluding whits from the purge
safety check. Since whits can only be generated by containment edges,
and generated resources previously could not have containment edges,
this is not a breaking change.

In order to maintain manifest ordering, it is necessary to add the
generated resources into the catalog adjacent to the generating
resource. This is achieved with new helpers on the catalog class to
handle ordered insertions.

Existing spec tests handle verifying manifest ordering works correctly
for generated resources, and integration tests for things like
resource purging handle reasonably complex generation cases. New tests
have been added only for the new edges that are being generated after
this commit.

Author: Branan Riley

Diff for: lib/puppet/resource/catalog.rb

@@ -73,6 +73,25 @@ def title_key_for_ref( ref )
     return a
   end
 
+  def add_resource_before(other, *resources)
+    resources.each do |resource|
+      other_title_key = title_key_for_ref(other.ref)
+      idx = @resources.index(other_title_key)
+      raise ArgumentError, "Cannot add resource #{resource.ref} before #{other.ref} because #{other.ref} is not yet in the catalog" if idx.nil?
+      add_one_resource(resource, idx)
+    end
+  end
+
+  def add_resource_after(other, *resources)
+    resources.each do |resource|
+      other_title_key = title_key_for_ref(other.ref)
+      idx = @resources.index(other_title_key)
+      raise ArgumentError, "Cannot add resource #{resource.ref} after #{other.ref} because #{other.ref} is not yet in the catalog" if idx.nil?
+      add_one_resource(resource, idx+1)
+    end
+  end
+
+
   def add_resource(*resources)
     resources.each do |resource|
       add_one_resource(resource)
@@ -86,23 +105,23 @@ def container_of(resource)
     adjacent(resource, :direction => :in)[0]
   end
 
-  def add_one_resource(resource)
+  def add_one_resource(resource, idx=-1)
     title_key = title_key_for_ref(resource.ref)
     if @resource_table[title_key]
       fail_on_duplicate_type_and_title(resource, title_key)
     end
 
-    add_resource_to_table(resource, title_key)
+    add_resource_to_table(resource, title_key, idx)
     create_resource_aliases(resource)
 
     resource.catalog = self if resource.respond_to?(:catalog=)
     add_resource_to_graph(resource)
   end
   private :add_one_resource
 
-  def add_resource_to_table(resource, title_key)
+  def add_resource_to_table(resource, title_key, idx)
     @resource_table[title_key] = resource
-    @resources << title_key
+    @resources.insert(idx, title_key)
   end
   private :add_resource_to_table
 

Diff for: lib/puppet/transaction.rb

@@ -79,7 +79,7 @@ def perform_pre_run_checks
   # necessary events.
   def evaluate(&block)
     block ||= method(:eval_resource)
-    generator = AdditionalResourceGenerator.new(@catalog, relationship_graph, @prioritizer)
+    generator = AdditionalResourceGenerator.new(@catalog, nil, @prioritizer)
     @catalog.vertices.each { |resource| generator.generate_additional_resources(resource) }
 
     perform_pre_run_checks
@@ -135,6 +135,9 @@ def evaluate(&block)
       end
     end
 
+    # Generate the relationship graph, set up our generator to use it
+    # for eval_generate, then kick off our traversal.
+    generator.relationship_graph = relationship_graph
     relationship_graph.traverse(:while => continue_while,
                                 :pre_process => pre_process,
                                 :overly_deferred_resource_handler => overly_deferred_resource_handler,

Diff for: lib/puppet/transaction/additional_resource_generator.rb

@@ -5,6 +5,8 @@
 #
 # @api private
 class Puppet::Transaction::AdditionalResourceGenerator
+  attr_writer :relationship_graph
+
   def initialize(catalog, relationship_graph, prioritizer)
     @catalog = catalog
     @relationship_graph = relationship_graph
@@ -22,11 +24,19 @@ def generate_additional_resources(resource)
     generated = [generated] unless generated.is_a?(Array)
     generated.collect do |res|
       @catalog.resource(res.ref) || res
-    end.each do |res|
-      priority = @prioritizer.generate_priority_contained_in(resource, res)
-      add_resource(res, resource, priority)
-
-      add_conditional_directed_dependency(resource, res)
+    end.reverse_each do |res|
+      # This is reversed becuase PUP-1963 changed how generated
+      # resources were added to the catalog. It exists for backwards
+      # compatibility only, and can probably be removed in Puppet 5
+      #
+      # Previously, resources were given sequential priorities in the
+      # relationship graph. Post-1963, resources are added to the
+      # catalog one by one adjacent to the parent resource. This
+      # causes an implicit reversal of their application order from
+      # the old code. The reverse makes it all work like it did.
+      add_resource(res, resource)
+
+      add_generated_directed_dependency(resource, res)
       generate_additional_resources(res)
     end
   end
@@ -102,16 +112,69 @@ def add_resources(generated, resource)
     end
   end
 
-  def add_resource(res, parent_resource, priority)
+  def add_resource(res, parent_resource, priority=nil)
     if @catalog.resource(res.ref).nil?
       res.tag(*parent_resource.tags)
-      @catalog.add_resource(res)
-      @relationship_graph.add_vertex(res, priority)
+      if parent_resource.depthfirst?
+        @catalog.add_resource_before(parent_resource, res)
+      else
+        @catalog.add_resource_after(parent_resource, res)
+      end
       @catalog.add_edge(@catalog.container_of(parent_resource), res)
+      if @relationship_graph && priority
+        # If we have a relationship_graph we should add the resource
+        # to it (this is an eval_generate). If we don't, then the
+        # relationship_graph has not yet been created and we can skip
+        # adding it.
+        @relationship_graph.add_vertex(res, priority)
+      end
       res.finish
     end
   end
 
+  # add correct edge for depth- or breadth- first traversal of
+  # generated resource. Skip generating the edge if there is already
+  # some sort of edge between the two resources.
+  def add_generated_directed_dependency(parent, child, label=nil)
+    if parent.depthfirst?
+      source = child
+      target = parent.to_resource
+    else
+      source = parent
+      target = child.to_resource
+    end
+
+    # For each potential relationship metaparam, check if parent or
+    # child references the other. If there are none, we should add our
+    # edge.
+    edge_exists = Puppet::Type.relationship_params.any? { |param|
+      param_sym = param.name.to_sym
+
+      if parent[param_sym]
+        parent_contains = parent[param_sym].any? { |res|
+          res.ref == child.ref
+        }
+      else
+        parent_contains = false
+      end
+
+      if child[param_sym]
+        child_contains = child[param_sym].any? { |res|
+          res.ref == parent.ref
+        }
+      else
+        child_contains = false
+      end
+
+      parent_contains || child_contains
+    }
+
+    if not edge_exists
+      source[:before] ||= []
+      source[:before] << target
+    end
+  end
+
   # Copy an important relationships from the parent to the newly-generated
   # child resource.
   def add_conditional_directed_dependency(parent, child, label=nil)

Diff for: lib/puppet/transaction/resource_harness.rb

@@ -99,10 +99,15 @@ def perform_changes(resource, context)
   def allow_changes?(resource)
     if resource.purging? and resource.deleting? and deps = relationship_graph.dependents(resource) \
             and ! deps.empty? and deps.detect { |d| ! d.deleting? }
-      deplabel = deps.collect { |r| r.ref }.join(",")
-      plurality = deps.length > 1 ? "":"s"
-      resource.warning "#{deplabel} still depend#{plurality} on me -- not purging"
-      false
+      real_deps = deps.reject { |r| r.class.name == :whit }
+      if real_deps.empty?
+        true
+      else
+        deplabel = real_deps.collect { |r| r.ref }.join(",")
+        plurality = real_deps.length > 1 ? "":"s"
+        resource.warning "#{deplabel} still depend#{plurality} on me -- not purging"
+        false
+      end
     else
       true
     end

Diff for: spec/lib/matchers/include_in_order.rb

@@ -11,10 +11,10 @@
   end
 
   def failure_message
-    "expected #{@actual.inspect} to include#{expected_to_sentence} in order"
+    "expected #{@actual.inspect} to include#{expected} in order"
   end
 
   def failure_message_when_negated
-    "expected #{@actual.inspect} not to include#{expected_to_sentence} in order"
+    "expected #{@actual.inspect} not to include#{expected} in order"
   end
 end

Diff for: spec/unit/transaction/additional_resource_generator_spec.rb

@@ -52,6 +52,48 @@ def eval_code
     end
   end
 
+  Puppet::Type.newtype(:autorequire) do
+    newparam(:name) do
+      isnamevar
+    end
+
+    autorequire(:notify) do
+      self[:name]
+    end
+  end
+
+  Puppet::Type.newtype(:gen_auto) do
+    newparam(:name) do
+      isnamevar
+    end
+
+    newparam(:eval_after) do
+    end
+
+    def generate()
+      [ Puppet::Type.type(:autorequire).new(:name => self[:eval_after]) ]
+    end
+  end
+
+  Puppet::Type.newtype(:empty) do
+    newparam(:name) do
+      isnamevar
+    end
+  end
+
+  Puppet::Type.newtype(:gen_empty) do
+    newparam(:name) do
+      isnamevar
+    end
+
+    newparam(:eval_after) do
+    end
+
+    def generate()
+      [ Puppet::Type.type(:empty).new(:name => self[:eval_after], :require => "Notify[#{self[:eval_after]}]") ]
+    end
+  end
+
   context "when applying eval_generate" do
     it "should add the generated resources to the catalog" do
       catalog = compile_to_ral(<<-MANIFEST)
@@ -368,13 +410,44 @@ class container {
                          "Notify[after]"))
     end
 
-    def relationships_after_generating(manifest, resource_to_generate)
-      catalog = compile_to_ral(manifest)
-      relationship_graph = relationship_graph_for(catalog)
+    it "runs autorequire on the generated resource" do
+      graph = relationships_after_generating(<<-MANIFEST, 'Gen_auto[thing]')
+        gen_auto { thing:
+          eval_after => hello,
+        }
 
-      generate_resources_in(catalog, relationship_graph, resource_to_generate)
+        notify { hello: }
+        notify { goodbye: }
+      MANIFEST
 
-      relationship_graph
+      expect(order_resources_traversed_in(graph)).to(
+        include_in_order("Gen_auto[thing]",
+                         "Notify[hello]",
+                         "Autorequire[hello]",
+                         "Notify[goodbye]"))
+    end
+
+    it "evaluates metaparameters on the generated resource" do
+      graph = relationships_after_generating(<<-MANIFEST, 'Gen_empty[thing]')
+        gen_empty { thing:
+          eval_after => hello,
+        }
+
+        notify { hello: }
+        notify { goodbye: }
+      MANIFEST
+
+      expect(order_resources_traversed_in(graph)).to(
+        include_in_order("Gen_empty[thing]",
+                         "Notify[hello]",
+                         "Empty[hello]",
+                         "Notify[goodbye]"))
+    end
+
+    def relationships_after_generating(manifest, resource_to_generate)
+      catalog = compile_to_ral(manifest)
+      generate_resources_in(catalog, nil, resource_to_generate)
+      relationship_graph_for(catalog)
     end
 
     def generate_resources_in(catalog, relationship_graph, resource_to_generate)