From 748564d0ff94336b57e713d01ac3c98dec790c95 Mon Sep 17 00:00:00 2001 From: Henrik Lindberg Date: Thu, 8 May 2014 00:04:18 +0200 Subject: [PATCH 1/4] (PUP-1597) Fix contain functions lookup of class with leading :: When the contain function creates edges it looked up resources without removing the leading :: in the name. The catalog never stores these, and thus an edge was formed between a resource and nil. This commit simply removes the ::. There is no checking that a resource is actually returned since they should all be there due to the call to include them in the catalog (which would fail if it was not possible to include them). A test is also added for the erronous (and now fixed case). --- lib/puppet/parser/functions/contain.rb | 4 +++- spec/unit/parser/functions/contain_spec.rb | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/puppet/parser/functions/contain.rb b/lib/puppet/parser/functions/contain.rb index 8eb514561ad..114fb5079df 100644 --- a/lib/puppet/parser/functions/contain.rb +++ b/lib/puppet/parser/functions/contain.rb @@ -18,7 +18,9 @@ scope.function_include(classes) classes.each do |class_name| - class_resource = scope.catalog.resource("Class", class_name) + # Remove global anchor since it is not part of the resource title and what was just + # included is then not found. + class_resource = scope.catalog.resource("Class", class_name.sub(/^::/, '')) if ! scope.catalog.edge?(scope.resource, class_resource) scope.catalog.add_edge(scope.resource, class_resource) end diff --git a/spec/unit/parser/functions/contain_spec.rb b/spec/unit/parser/functions/contain_spec.rb index 3150e0c8e1a..6db9c4e3d7d 100644 --- a/spec/unit/parser/functions/contain_spec.rb +++ b/spec/unit/parser/functions/contain_spec.rb @@ -25,6 +25,22 @@ class container { expect(catalog.classes).to include("contained") end + it "includes the class when using a fully qualified anchored name" do + catalog = compile_to_catalog(<<-MANIFEST) + class contained { + notify { "contained": } + } + + class container { + contain ::contained + } + + include container + MANIFEST + + expect(catalog.classes).to include("contained") + end + it "makes the class contained in the current class" do catalog = compile_to_catalog(<<-MANIFEST) class contained { From 90f5c314cb10a78c9c03570808b436509bd057a9 Mon Sep 17 00:00:00 2001 From: Andrew Parker Date: Fri, 9 May 2014 12:03:09 -0700 Subject: [PATCH 2/4] (PUP-1597) Handle containment of relative names The contain() function made an error when dealing with a relative name. It assumed that the class that was actually included was always the same as the name that it was given. In many cases this is true, but when the name is found via a relative lookup (e.g. contain(bar) inside a class foo, when a foo::bar exists), the assumption breaks down. The source of the problem is that it tries to look up the included class after the fact. Without all of the extra information about namespaces it isn't able to find the right class. To fix this, the include() function has been updated to return the class resources that it included (including those that were already present). This allows the contain() function to create the appriate edges with the exact resources needed. A secondary change in this was to remove the checks for include not being able to include a class. The compiler method that is involved already makes that check and raises an error, so the include functions code was simply redundant. --- lib/puppet/parser/compiler.rb | 9 +++++- lib/puppet/parser/functions/contain.rb | 10 +++---- lib/puppet/parser/functions/include.rb | 34 +++++----------------- spec/unit/parser/compiler_spec.rb | 4 +-- spec/unit/parser/functions/contain_spec.rb | 21 +++++++++++++ 5 files changed, 44 insertions(+), 34 deletions(-) diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb index 7f4e8229807..2a8c8003020 100644 --- a/lib/puppet/parser/compiler.rb +++ b/lib/puppet/parser/compiler.rb @@ -197,6 +197,7 @@ def evaluate_node_classes # for more detail. --jeffweiss 26 apr 2012 def evaluate_classes(classes, scope, lazy_evaluate = true, fqname = false) raise Puppet::DevError, "No source for scope passed to evaluate_classes" unless scope.source + resources = [] class_parameters = nil # if we are a param class, save the classes hash # and transform classes to be the keys @@ -213,17 +214,23 @@ def evaluate_classes(classes, scope, lazy_evaluate = true, fqname = false) if class_parameters resource = klass.ensure_in_catalog(scope, class_parameters[name] || {}) else - next if scope.class_scope(klass) + if scope.class_scope(klass) + resources << scope.class_scope(klass).resource + next + end resource = klass.ensure_in_catalog(scope) end # If they've disabled lazy evaluation (which the :include function does), # then evaluate our resource immediately. resource.evaluate unless lazy_evaluate + resources << resource else raise Puppet::Error, "Could not find class #{name} for #{node.name}" end end + + resources end def evaluate_relationships diff --git a/lib/puppet/parser/functions/contain.rb b/lib/puppet/parser/functions/contain.rb index 114fb5079df..4ecd2411cdc 100644 --- a/lib/puppet/parser/functions/contain.rb +++ b/lib/puppet/parser/functions/contain.rb @@ -15,14 +15,14 @@ ) do |classes| scope = self - scope.function_include(classes) + included = scope.function_include(classes) - classes.each do |class_name| + included.each do |resource| # Remove global anchor since it is not part of the resource title and what was just # included is then not found. - class_resource = scope.catalog.resource("Class", class_name.sub(/^::/, '')) - if ! scope.catalog.edge?(scope.resource, class_resource) - scope.catalog.add_edge(scope.resource, class_resource) + #class_resource = scope.catalog.resource("Class", class_name.sub(/^::/, '')) + if ! scope.catalog.edge?(scope.resource, resource) + scope.catalog.add_edge(scope.resource, resource) end end end diff --git a/lib/puppet/parser/functions/include.rb b/lib/puppet/parser/functions/include.rb index 29ef45d403c..322d6770b1c 100644 --- a/lib/puppet/parser/functions/include.rb +++ b/lib/puppet/parser/functions/include.rb @@ -18,30 +18,12 @@ where they are declared. For that, see the `contain` function. It also does not create a dependency relationship between the declared class and the surrounding class; for that, see the `require` function.") do |vals| - if vals.is_a?(Array) - # Protect against array inside array - vals = vals.flatten - else - vals = [vals] - end - - # The 'false' disables lazy evaluation. - klasses = compiler.evaluate_classes(vals, self, false) - - missing = vals.find_all do |klass| - ! klasses.include?(klass) - end - - unless missing.empty? - # Throw an error if we didn't evaluate all of the classes. - str = "Could not find class" - str += "es" if missing.length > 1 - - str += " " + missing.join(", ") - - if n = namespaces and ! n.empty? and n != [""] - str += " in namespaces #{@namespaces.join(", ")}" - end - self.fail Puppet::ParseError, str - end + if vals.is_a?(Array) + # Protect against array inside array + vals = vals.flatten + else + vals = [vals] + end + + compiler.evaluate_classes(vals, self, false) end diff --git a/spec/unit/parser/compiler_spec.rb b/spec/unit/parser/compiler_spec.rb index d1e855a2024..4698479d34c 100755 --- a/spec/unit/parser/compiler_spec.rb +++ b/spec/unit/parser/compiler_spec.rb @@ -681,7 +681,7 @@ def compile it "should skip classes that have already been evaluated" do @compiler.catalog.stubs(:tag) - @scope.stubs(:class_scope).with(@class).returns("something") + @scope.stubs(:class_scope).with(@class).returns(@scope) @compiler.expects(:add_resource).never @@ -694,7 +694,7 @@ def compile it "should skip classes previously evaluated with different capitalization" do @compiler.catalog.stubs(:tag) @scope.stubs(:find_hostclass).with("MyClass",{:assume_fqname => false}).returns(@class) - @scope.stubs(:class_scope).with(@class).returns("something") + @scope.stubs(:class_scope).with(@class).returns(@scope) @compiler.expects(:add_resource).never @resource.expects(:evaluate).never Puppet::Parser::Resource.expects(:new).never diff --git a/spec/unit/parser/functions/contain_spec.rb b/spec/unit/parser/functions/contain_spec.rb index 6db9c4e3d7d..d54e55b119f 100644 --- a/spec/unit/parser/functions/contain_spec.rb +++ b/spec/unit/parser/functions/contain_spec.rb @@ -3,11 +3,13 @@ require 'puppet_spec/compiler' require 'puppet/parser/functions' require 'matchers/containment_matchers' +require 'matchers/resource' require 'matchers/include_in_order' describe 'The "contain" function' do include PuppetSpec::Compiler include ContainmentMatchers + include Matchers::Resource it "includes the class" do catalog = compile_to_catalog(<<-MANIFEST) @@ -41,6 +43,25 @@ class container { expect(catalog.classes).to include("contained") end + it "ensures that the edge is with the correct class" do + catalog = compile_to_catalog(<<-MANIFEST) + class outer { + class named { } + contain named + } + + class named { } + + include named + include outer + MANIFEST + + expect(catalog).to have_resource("Class[Named]") + expect(catalog).to have_resource("Class[Outer]") + expect(catalog).to have_resource("Class[Outer::Named]") + expect(catalog).to contain_class("outer::named").in("outer") + end + it "makes the class contained in the current class" do catalog = compile_to_catalog(<<-MANIFEST) class contained { From be990a2f95863b44a2fa2688e67e40c65c5e7642 Mon Sep 17 00:00:00 2001 From: Andrew Parker Date: Fri, 9 May 2014 13:56:46 -0700 Subject: [PATCH 3/4] (PUP-1597) Separate different modes of including The evaluate_classes method has a couple different responsibilities and inputs. This is just an attempt to pull them apart some and make the flows a little more straightforward. --- lib/puppet/parser/compiler.rb | 62 +++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb index 2a8c8003020..367a7524890 100644 --- a/lib/puppet/parser/compiler.rb +++ b/lib/puppet/parser/compiler.rb @@ -197,7 +197,6 @@ def evaluate_node_classes # for more detail. --jeffweiss 26 apr 2012 def evaluate_classes(classes, scope, lazy_evaluate = true, fqname = false) raise Puppet::DevError, "No source for scope passed to evaluate_classes" unless scope.source - resources = [] class_parameters = nil # if we are a param class, save the classes hash # and transform classes to be the keys @@ -205,32 +204,26 @@ def evaluate_classes(classes, scope, lazy_evaluate = true, fqname = false) class_parameters = classes classes = classes.keys end - classes.each do |name| - # If we can find the class, then make a resource that will evaluate it. - if klass = scope.find_hostclass(name, :assume_fqname => fqname) - - # If parameters are passed, then attempt to create a duplicate resource - # so the appropriate error is thrown. - if class_parameters - resource = klass.ensure_in_catalog(scope, class_parameters[name] || {}) - else - if scope.class_scope(klass) - resources << scope.class_scope(klass).resource - next - end - resource = klass.ensure_in_catalog(scope) - end - # If they've disabled lazy evaluation (which the :include function does), - # then evaluate our resource immediately. - resource.evaluate unless lazy_evaluate - resources << resource - else - raise Puppet::Error, "Could not find class #{name} for #{node.name}" - end + hostclasses = classes.collect do |name| + scope.find_hostclass(name, :assume_fqname => fqname) or raise Puppet::Error, "Could not find class #{name} for #{node.name}" end - resources + if class_parameters + resources = ensure_classes_with_parameters(scope, hostclasses, class_parameters) + if !lazy_evaluate + resources.each(&:evaluate) + end + + resources + else + already_included, newly_included = ensure_classes_without_parameters(scope, hostclasses) + if !lazy_evaluate + newly_included.each(&:evaluate) + end + + already_included + newly_included + end end def evaluate_relationships @@ -306,6 +299,27 @@ def is_binder_active? private + def ensure_classes_with_parameters(scope, hostclasses, parameters) + hostclasses.collect do |klass| + klass.ensure_in_catalog(scope, parameters[klass.name] || {}) + end + end + + def ensure_classes_without_parameters(scope, hostclasses) + already_included = [] + newly_included = [] + hostclasses.each do |klass| + class_scope = scope.class_scope(klass) + if class_scope + already_included << class_scope.resource + else + newly_included << klass.ensure_in_catalog(scope) + end + end + + [already_included, newly_included] + end + # If ast nodes are enabled, then see if we can find and evaluate one. def evaluate_ast_node return unless ast_nodes? From e1e446f35542accdf877ba4fe10ad473ce530f77 Mon Sep 17 00:00:00 2001 From: Andrew Parker Date: Mon, 12 May 2014 10:53:32 -0700 Subject: [PATCH 4/4] (maint) Reduce resource lookup scope.resource was called multiple times. Even if this is a simple method, it is still a method call and is slower than just holding onto the result. This also removes some comments that appear to be out of date. --- lib/puppet/parser/functions/contain.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/puppet/parser/functions/contain.rb b/lib/puppet/parser/functions/contain.rb index 4ecd2411cdc..791c7d91f30 100644 --- a/lib/puppet/parser/functions/contain.rb +++ b/lib/puppet/parser/functions/contain.rb @@ -15,14 +15,12 @@ ) do |classes| scope = self - included = scope.function_include(classes) + containing_resource = scope.resource + included = scope.function_include(classes) included.each do |resource| - # Remove global anchor since it is not part of the resource title and what was just - # included is then not found. - #class_resource = scope.catalog.resource("Class", class_name.sub(/^::/, '')) - if ! scope.catalog.edge?(scope.resource, resource) - scope.catalog.add_edge(scope.resource, resource) + if ! scope.catalog.edge?(containing_resource, resource) + scope.catalog.add_edge(containing_resource, resource) end end end