Accept UnaryMinusExpression as class parameter type #9269

joshcooper · 2024-02-28T00:49:33Z

Previously, class parameters of the form Integer[-1] $param would fail
compilation, because the value -1 was lexed as a UnaryMinusExpression
containing a LiteralInteger. And since the LiteralEvaluator didn't implement the
literal_UnaryMinusExpression method, the visitor called literal_XXX for each
ancestor class, until reaching literal_Object, which always raises.

This adds the literal_UnaryMinusExpression method and returns -1 times
the expression it wraps. This is similar to how the TypeParser interprets
UnaryMinusExpressions[1].

If strict is off, the issue will be ignored. If strict is warning, then a
warning will be reported, but compilation will continue:

Warning: The parameter '$i' must be a literal type, not a Puppet::Pops::Model::AccessExpression

If strict is error, then compilation will fail.

Fixes #9268

[1] https://github.com/puppetlabs/puppet/blob/8.5.0/lib/puppet/pops/types/type_parser.rb#L161

spec/unit/pops/evaluator/literal_evaluator_spec.rb

joshcooper · 2024-02-28T20:38:01Z

lib/puppet/pops/validation/validator_factory_4_0.rb

@@ -36,6 +36,8 @@ def severity_producer
    p[Issues::NAME_WITH_HYPHEN]               = :error
    p[Issues::EMPTY_RESOURCE_SPECIALIZATION]  = :ignore
    p[Issues::CLASS_NOT_VIRTUALIZABLE]        = :error
+
+    p[Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE] = Puppet[:strict] == :off ? :ignore : Puppet[:strict]


Since strict defaults to warning in 7.x, this would effectively cause incorrect types to no longer fail compilation for the cases we fixed in adbb02c, like Integer[1-3].

We could instead do the following for both main and 7.x branches

Puppet[:strict] == :off ? :ignore : :error

Seems like the least surprising thing would be to let it default to warning. In the case there is just a warning would the type just not be enforced? That doesn't seem terribly dangerous?

It's a bit subtle. In the case of Integer[1-3], the type is actually valid, because it evaluates to Integer[-2], which in the context of a class parameter Integer[-2] $i means it will accept integers >= -2. However, the manifest author intended to specify the range Integer[1, 3]. So if you pass a value bigger than 3, then it passes validation (surprise)!

donoghuc

Seems good. Double checked ranges work as expected locally

diff --git a/spec/unit/pops/evaluator/literal_evaluator_spec.rb b/spec/unit/pops/evaluator/literal_evaluator_spec.rb
index 63ee699a78..90a0aeb201 100644
--- a/spec/unit/pops/evaluator/literal_evaluator_spec.rb
+++ b/spec/unit/pops/evaluator/literal_evaluator_spec.rb
@@ -18,6 +18,8 @@ describe "Puppet::Pops::Evaluator::LiteralEvaluator" do
     'a::b'    => 'a::b',
     'Integer[1]' => [1],
     'Integer[-1]' => [-1],
+    'Integer[-5, -1]' => [-5, -1],
+    'Integer[-5, 5]' => [-5, 5],
     'File' => "file",

I like that it will follow strict settings. So effectively it will default to warning in 7 and error in 8. 👍

Previously, class parameters of the form `Integer[-1] $param` would fail compilation, because the value `-1` was lexed as a UnaryMinusExpression containing a LiteralInteger. And since the LiteralEvaluator didn't implement the `literal_UnaryMinusExpression` method, the visitor called `literal_XXX` for each ancestor class, until reaching `literal_Object`, which always raises. This adds the `literal_UnaryMinusExpression` method and returns -1 times the expression it wraps. This is similar to how the TypeParser interprets UnaryMinusExpressions[1]. [1] https://github.com/puppetlabs/puppet/blob/8.5.0/lib/puppet/pops/types/type_parser.rb#L161

If strict is off, the issue will be ignored. If strict is warning, then a warning will be reported, but compilation will continue: Warning: The parameter '$i' must be a literal type, not a Puppet::Pops::Model::AccessExpression If strict is error, then compilation will fail.

spec/unit/pops/evaluator/literal_evaluator_spec.rb

The "undef to nil" test is special because otherwise the value nil when interpolated in the name of the test results in empty string.

github-actions · 2024-02-29T18:50:09Z

Successfully created backport PR for 7.x:

[Backport 7.x] Accept UnaryMinusExpression as class parameter type #9272

Backport #9269 to 7.29.1

Backport #9269 to 8.5.1

rembun · 2024-03-02T20:25:51Z

I am getting this error on fresh install foreman running on alma 8 foreman-installer [root@localhost ~]# vi /usr/share/foreman-installer/modules/redis/manifests/init.pp

/usr/share/gems/gems/kafo-7.4.0/lib/kafo/puppet_module.rb:70:in `parse': No Puppet module parser is installed and no cache of the file /usr/share/foreman-installer/modules/foreman/manifests/cli.pp is available. Please check debug logs and install optional dependencies for the parser. (Kafo::ParserError)
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/configuration.rb:133:in `block in modules'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/configuration.rb:133:in `map'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/configuration.rb:133:in `modules'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/kafo_configure.rb:257:in `modules'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/kafo_configure.rb:261:in `module'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/hook_context.rb:113:in `module_present?'
        from /usr/share/foreman-installer/hooks/boot/20-certs_update.rb:2:in `block (4 levels) in load'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/hooking.rb:36:in `instance_eval'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/hooking.rb:36:in `block (4 levels) in load'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/hook_context.rb:19:in `instance_eval'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/hook_context.rb:19:in `execute'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/hooking.rb:67:in `block in execute'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/hooking.rb:65:in `each'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/hooking.rb:65:in `execute'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/kafo_configure.rb:144:in `initialize'
        from /usr/share/gems/gems/clamp-1.3.2/lib/clamp/command.rb:140:in `new'
        from /usr/share/gems/gems/clamp-1.3.2/lib/clamp/command.rb:140:in `run'
        from /usr/share/gems/gems/kafo-7.4.0/lib/kafo/kafo_configure.rb:54:in `run'
        from /usr/sbin/foreman-installer:8:in `<main>'

rembun · 2024-03-02T20:26:47Z

Has the changed already pushed to the repo as it seems that we still pulling wrong code

mhashizume · 2024-03-05T22:45:47Z

@rembun We released Puppet 7.29.1 and 8.5.1 today with this fix included. Please let us know if those versions do not resolve this issue for you.

kjetilho · 2024-03-05T23:17:44Z

This fix is not sufficient (I tested using 7.29.1. gem), it breaks arithmetic in general. Examples from my code:

  Integer[0, 3*3600] $random_delay = 1800,
  Integer[0, 24*60] $downtime_duration = 10,

edit: 8.5.1 gem has same failure:

The parameter '$random_delay' must be a literal type, not a Puppet::Pops::Model::AccessExpression

donoghuc · 2024-03-06T00:09:02Z

@kjetilho The intent of the original commit was to prevent that sort of thing. See the commit message here 1f6e950 type constraints must be literal, not expressions.

ekohl · 2024-03-06T01:44:25Z

@donoghuc isn't that an unusual thing to do in a minor release?

joshcooper · 2024-03-06T05:33:09Z

@kjetilho please file a new GH issue instead of reusing this one. In the new issue, can you explain what you mean by "it breaks arithmetic in general"? How exactly is it broken? What behavior do you see?

kjetilho · 2024-03-06T08:47:26Z

@joshcooper - I included both my code and the error message. What more do you need?

@donoghuc - thanks, I see it was intended to break my code. In which case I must agree with @ekohl, that seems like an unusual thing to do in a minor release, and at least warrants an entry in the release notes.

In any case, the goal of "provid[ing] a descriptive warning if parameter(s) are not literal" was not met IMHO. It is not descriptive for users (no users have any idea what an Puppet::Pops::Model::AccessExpression is), and it is not a warning 😄

joshcooper · 2024-03-06T19:39:01Z

@kjetilho you said, "This fix is not sufficient (I tested using 7.29.1. gem), it breaks arithmetic in general" And you gave the resulting message, "The parameter '$random_delay' must be a literal type, not a Puppet::Pops::Model::AccessExpression" However, isn't it true that in 7.29.1 the message is printed as a warning in the puppetserver logs? If so, that's very different than "it breaks". If not, are you running with strict=error in puppet.conf?

kjetilho · 2024-03-06T20:43:45Z

Ah, I am sorry - my pipeline failed on the first step, which was rake syntax ('puppet-syntax/tasks/puppet-syntax'). That's where it was a fatal error. You are correct, it is just a warning with default settings for puppet apply.

joshcooper · 2024-03-07T00:01:58Z

@kjetilho ah makes sense, thank you! When rake syntax fails does that cause your CI pipelines to fail too?

I would expect puppet to be able to add warnings to the language runtime and evaluation without breaking all module CI, as described in https://github.com/puppetlabs/puppet/blob/7.29.1/lib/puppet/defaults.rb#L192-L204.

It seems puppet-syntax doesn't distinguish between warnings and errors in manifests https://github.com/voxpupuli/puppet-syntax/blob/8114392547895ef352cfa66c3becdc3cf33859fb/lib/puppet-syntax/manifests.rb#L24-L25

but it does for templates https://github.com/voxpupuli/puppet-syntax/blob/8114392547895ef352cfa66c3becdc3cf33859fb/lib/puppet-syntax/templates.rb#L38-L57

Maybe it should for manifests so it's clear the "must be a literal type" message is a warning and not an error? Thoughts @bastelfreak

kjetilho · 2024-03-08T10:16:25Z

Yes, the syntax check is strict, but I think it is good to fix even deprecations early. The puppet-lint in my CI pipeline, is strict, too. It even regards trailing whitespace a fatal error :)

To summarise my position: I'm fine with the change in behaviour - but I would love to see the warning message improved to something more easily human understandable if possible.

The parameter '$random_delay' must be a literal type, not a Puppet::Pops::Model::AccessExpression

This indicates to me that the problem is with my parameter value, not the parameter declaration. How about:

The type for parameter '$random_delay' must be a literal type, not an expression

Of course my intuition is not everyone else's intution, so that's just my 2 cents.

bastelfreak · 2024-03-08T13:10:23Z

I would love to see the warning message improved to something more easily human understandable if possible.

I agree. I think the message isn't very user friendly at the moment.

When rake syntax fails does that cause your CI pipelines to fail too?

Yes, that's the default behaviour

And as stated on slack: I think this is a breaking change and should have been landed in Puppet 9 and not in a minor release. And thinking further about it I argue that puppet 7 and puppet 8 should print a deprecation message, not a warning. and on Puppet 8 it should be an error or warning. And the puppet-syntax behaviour for deprecations is configurable.

joshcooper · 2024-03-08T19:31:26Z

The parameter '$random_delay' must be a literal type, not a Puppet::Pops::Model::AccessExpression

This is a bug, we should be using the LabelProvider to format the model. In the case of Integer[0, 24*60], how about?

The parameter '$i' must be a literal type, not a '*' expression (line: 1, column: 18)

I think this is a breaking change and should have been landed in Puppet 9 and not in a minor release.

In the past, we have introduced new language validations in minor or patch releases in one of two ways:

First, the issue is introduced as a deprecation. It is not under control of strict, so strict=off does nothing. All deprecations can be disabled via disable_warnings=deprecations. Examples of this include:

ILLEGAL_DEFINITION_LOCATION First introduced in 5.5.6 under strict control but converted to a deprecation before 5.5.6 was released. In 6.0, converted to an error
ILLEGAL_TOP_CONSTRUCT_LOCATION First introduced in 5.5.7 as a deprecation. In 6.0, converted to an error

Second, the issue is introduced under control of strict. By default, that is a warning in 7.x and error in 8.x, but can be turned off using strict=off. Examples of this include:

DUPLICATE_KEY First introduced in 4.5.0 as a warning, but converted to be under the control of strict before 4.5.0 was released
DUPLICATE_DEFAULT First introduced in 4.9.0 under the control of strict. In 4.10.5, converted to an error
CLASS_NOT_VIRTUALIZEABLE First introduced in 4.10.2 under the control of strict. In 7.0, converted to an error

The reason we went with the "under control of strict" route is because class parameters were always assumed to be literal and puppet's environment_classes REST API fails when they are non-literal, for example Integer[0, 2*2]. So there is some urgency around fixing puppet code, as opposed to an arbitrary decision on our part.

But given the precedent with how we handled validations in 5.x and feedback on this thread, I agree we should switch the ILLEGAL_NONLITERAL_PARAMETER_TYPE validation to a deprecation. It also means the next releases of 7.x and 8.x will behave consistently. I'll file a new issue for this including the label provider.

bastelfreak · 2024-03-08T20:33:51Z

The parameter '$i' must be a literal type, not a '*' expression (line: 1, column: 18)

sounds good to me!

joshcooper requested a review from a team as a code owner February 28, 2024 00:49

joshcooper added the bug Something isn't working label Feb 28, 2024

joshcooper commented Feb 28, 2024

View reviewed changes

spec/unit/pops/evaluator/literal_evaluator_spec.rb Outdated Show resolved Hide resolved

joshcooper force-pushed the unary_minus_literal branch from 83ed38b to c3054b4 Compare February 28, 2024 20:30

joshcooper commented Feb 28, 2024

View reviewed changes

donoghuc approved these changes Feb 29, 2024

View reviewed changes

joshcooper force-pushed the unary_minus_literal branch from 7138802 to 7f438d2 Compare February 29, 2024 01:35

joshcooper added 2 commits February 28, 2024 17:39

joshcooper force-pushed the unary_minus_literal branch 2 times, most recently from a3380ab to 7eeb236 Compare February 29, 2024 01:49

evgeni reviewed Feb 29, 2024

View reviewed changes

spec/unit/pops/evaluator/literal_evaluator_spec.rb Outdated Show resolved Hide resolved

Add additional literal evaluator tests

17ba937

The "undef to nil" test is special because otherwise the value nil when interpolated in the name of the test results in empty string.

joshcooper force-pushed the unary_minus_literal branch from 7eeb236 to 17ba937 Compare February 29, 2024 14:57

cthorn42 approved these changes Feb 29, 2024

View reviewed changes

cthorn42 merged commit e8d1797 into puppetlabs:main Feb 29, 2024

joshcooper deleted the unary_minus_literal branch February 29, 2024 18:49

joshcooper added the backport 7.x Generate a backport PR to 7.x label Feb 29, 2024

github-actions bot mentioned this pull request Feb 29, 2024

[Backport 7.x] Accept UnaryMinusExpression as class parameter type #9272

Merged

This was referenced Feb 29, 2024

Backport #9269 to 8.5.1 #9273

Merged

Backport #9269 to 7.29.1 #9274

Merged

joshcooper added a commit that referenced this pull request Feb 29, 2024

Merge pull request #9274 from joshcooper/backport_unary_7291

090122a

Backport #9269 to 7.29.1

joshcooper added a commit that referenced this pull request Feb 29, 2024

Merge pull request #9273 from joshcooper/backport_unary_851

939d5d4

Backport #9269 to 8.5.1

joshcooper mentioned this pull request Mar 4, 2024

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

Closed

elfranne mentioned this pull request Mar 8, 2024

Puppet agent 7.29.0 break the module voxpupuli/puppet-keepalived#314

Closed

joshcooper mentioned this pull request Mar 8, 2024

Non-literal class parameter types should be deprecated not a warning #9285

Closed

Accept UnaryMinusExpression as class parameter type #9269

Accept UnaryMinusExpression as class parameter type #9269

Uh oh!

Conversation

joshcooper commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

joshcooper Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

donoghuc Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

joshcooper Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

donoghuc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Feb 29, 2024

Uh oh!

rembun commented Mar 2, 2024 • edited by joshcooper Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rembun commented Mar 2, 2024

Uh oh!

mhashizume commented Mar 5, 2024

Uh oh!

kjetilho commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donoghuc commented Mar 6, 2024

Uh oh!

ekohl commented Mar 6, 2024

Uh oh!

joshcooper commented Mar 6, 2024

Uh oh!

kjetilho commented Mar 6, 2024

Uh oh!

joshcooper commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjetilho commented Mar 6, 2024

Uh oh!

joshcooper commented Mar 7, 2024

Uh oh!

kjetilho commented Mar 8, 2024

Uh oh!

bastelfreak commented Mar 8, 2024

Uh oh!

joshcooper commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bastelfreak commented Mar 8, 2024

Uh oh!

Uh oh!

joshcooper commented Feb 28, 2024 •

edited

Loading

rembun commented Mar 2, 2024 •

edited by joshcooper

Loading

kjetilho commented Mar 5, 2024 •

edited

Loading

joshcooper commented Mar 6, 2024 •

edited

Loading

joshcooper commented Mar 8, 2024 •

edited

Loading