Syntactically incorrect types cause nil types in Puppet::InfoService::ClassInformationService

[jonathannewman]

Describe the Bug

When a type is incorrectly specified, e.g. Integer[1-3] for a class parameter, Puppet's class_information_service ignores this error, and produces an empty type specification.

puppet/lib/puppet/info_service/class_information_service.rb

Lines 76 to 80 in 298762f

	def extract_type(structure, p)
	return structure if p.type_expr.nil?
	structure[:type] = typeexpr_to_string(p.type_expr)
	structure
	end

Expected Behavior

A warning should be produced, and a default type should be assigned in the nil case to prevent incorrect behavior. Note that this issue does not impact catalog compilation, but does impact the ability to get classes manifest information.

Impacts both 7.x and 8.x

[github-actions]

Migrated issue to PUP-11981

[joshcooper]

We recently saw a similar issue when a class parameter was defined as Optional[[String]] (note the double brackets). Seems like a test should have failed along the way before the code was deployed to puppetserver.

Edit: But none of our tests puppet-lint, unit, beaker) actually failed... I would have expected compilation to fail unless the compiler resolves the type to nil and maybe falls back to Any? More investigation is needed to understand why it's not getting caught earlier.

[hlindberg]

That API does not have a way to report detail errors, it either produces a result (and skips bad input) or returns an error. Code should have been validated before deploying and using the class info service.

[joshcooper]

The type Integer[1-3] is actually valid, because 1-3 = -2 and Integer[-2] matches any integer greater than or equal to -2. And this is valid too, because 4 > -2:

class foo(Integer[1-3] $i) {}
class { 'foo':  i => 4 }

The class info service is getting an error because it only accepts literal expressions:

puppet/lib/puppet/info_service/class_information_service.rb

Lines 94 to 97 in 775592c

	rescue Puppet::ParseError
	# type is to complex - contains expressions that are not literal
	nil
	end

Can we validate that class parameters don't contain non-literal expressions earlier? Or maybe a puppet-lint check?

[hlindberg]

Should be possible to validate.
There is a validator for type, and IIRC one (or a parameter) for static expression validation.

[joshcooper]

Hi @hlindberg I see we validate class definitions here, for example to ensure class parameter names are unique. Would it make sense to add a check that class parameter types are literal too?

diff --git a/lib/puppet/pops/issues.rb b/lib/puppet/pops/issues.rb
index b619636568..23cb80dbde 100644
--- a/lib/puppet/pops/issues.rb
+++ b/lib/puppet/pops/issues.rb
@@ -207,6 +207,9 @@ module Issues
     _("The numeric parameter name '$%{name}' cannot be used (clashes with numeric match result variables)") % { name: name }
   end
 
+  ILLEGAL_NONLITERAL_PARAMETER_TYPE = issue :ILLEGAL_NONLITERAL_PARAMETER_TYPE, :name, :type_class do
+    _("The parameter '$%{name}' must be a literal type, not %{type_class}") % { name: name, type_class: label.a_an(type_class) }
+  end
   # In certain versions of Puppet it may be allowed to assign to a not already assigned key
   # in an array or a hash. This is an optional validation that may be turned on to prevent accidental
   # mutation.

diff --git a/lib/puppet/pops/validation/checker4_0.rb b/lib/puppet/pops/validation/checker4_0.rb
index eb7ac5cc77..66f62d1895 100644
--- a/lib/puppet/pops/validation/checker4_0.rb
+++ b/lib/puppet/pops/validation/checker4_0.rb
@@ -471,6 +471,7 @@ class Checker4_0 < Evaluator::LiteralEvaluator
     internal_check_parameter_name_uniqueness(o)
     internal_check_reserved_params(o)
     internal_check_no_idem_last(o)
+    internal_check_parameter_type_literal(o)
   end
 
   def check_ResourceTypeDefinition(o)
@@ -683,6 +684,17 @@ class Checker4_0 < Evaluator::LiteralEvaluator
     end
   end
 
+  def internal_check_parameter_type_literal(o)
+    o.parameters.each do |p|
+      next unless p.type_expr
+      catch :not_literal do
+        literal(p.type_expr)
+        next
+      end
+      acceptor.accept(Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE, p, {name: p.name, type_class: p.type_expr.class})
+    end
+  end
+

The Checker4_0 inherits from LiteralEvaluator, so I'm just calling the super literal method.

However, we have tests that class parameter types can be QualifiedReferences which would be rejected by the above changes because the LiteralEvaluator says:

puppet/lib/puppet/pops/evaluator/literal_evaluator.rb

Lines 15 to 16 in 775592c

	# Not considered literal
	# QualifiedReference # i.e. File, FooBar

Since the ClassInfoService validates parameter types using the TypeParser, I tried using that instead, but it too rejects QualifiedReference:

(byebug) Puppet::Pops::Types::TypeParser.singleton.interpret_any(p, nil)
*** Puppet::ParseError Exception: The expression <Type[Bar] $x> is not a valid type specification.

Is there a different evaluator I should be using for class parameters? Or should I create a new evaluator and implement the missing literal_QualifiedReference(o)?

class Something < LiteralEvaluator

  def literal_QualifiedReference(o)
    ...
  end

I assume whatever we do, both the ClassInfoService and Checker4_0 should call the same validation code so that the compiler and class info service accept/reject the same types.

[hlindberg]

The one issue that bothers me a bit is that any type of reference that isn't defined as a data type or one of the built-in is considered a reference to a class, defined resource-type, or built-in resource type. Thus I am thinking that when validation takes place it isn't known if it is a reference to an actual thing or not. It probably does not matter as the problem here is that it cannot be turned into a human-readable string for the info-service.

It will probably work to allow qualified references in a specialized evaluator like you suggest and then make sure it gets used in both places. The logic that turns QualifiedReference into a human-readable string probably already exists (i.e. the class where it now fails for the info-service).

I don't remember if the info-service also produces information for defined resources. If so the validator for those must do the same - otherwise it could be allowed (as for functions). In both those case it is still not good for generating documentation as it would need to produce somethng humany-readable. You may thus want to also validate functions to only accept literal type definitions. You should still allow it for lambdas and everywhere else - just not in any puppet language "definition" (i.e. define, class, function).

May have to be adjusted for Bolt as well if they use specialized validators. Also take a look at what Puppet String does if faced with non-literal type in definitions and how it handles if it gets a QualifiedReference.

The much simpler solution would be to consider QualifiedReference to be "literal" - it is after all a literal reference, but it can be faulty (not reference anything). I was very conservative when not allowing it to be a "literal". You could try that and see if it breaks any tests. Or maybe you tried that already? If changed it may affect Bolt and Puppet Strings.

Hope that helps.

[ibuclaw]

Why wasn't this listed as a breaking change? It breaks many downstream modules that expect i.e: Integer[-1] to work.

i.e: https://github.com/voxpupuli/puppet-dhcp/blob/master/manifests/init.pp#L29-L30

Puppet The parameter '$default_lease_time' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: /etc/puppetlabs/code/environments/production/modules/dhcp/manifests/init.pp, line: 29, column: 15)
Puppet The parameter '$max_lease_time' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: /etc/puppetlabs/code/environments/production/modules/dhcp/manifests/init.pp, line: 30, column: 15)

Not even the examples in the documentation work.
https://www.puppet.com/docs/puppet/7/lang_data_number#lang_data_number_numeric_type

class test (
Integer[-3,3] $foo,
Float[-3.0,3.0] $bar,
) {
}

$ puppet apply test.pp
Error: The parameter '$foo' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: test.pp, line: 2, column: 15)
Error: The parameter '$bar' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: test.pp, line: 3, column: 17)

[joshcooper]

@ibuclaw negative numbers were fixed in #9269