Jira (PUP-9507) Plusignment on defined types ignores default value

2 views
Skip to first unread message

Alexander Olofsson (JIRA)

unread,
Feb 21, 2019, 8:21:03 AM2/21/19
to puppe...@googlegroups.com
Alexander Olofsson created an issue
 
Puppet / Bug PUP-9507
Plusignment on defined types ignores default value
Issue Type: Bug Bug
Assignee: Unassigned
Attachments: broken.pp, working2.pp, working.pp
Created: 2019/02/21 5:20 AM
Priority: Normal Normal
Reporter: Alexander Olofsson

Puppet Version: 4.10.5, 5.5.1, 5.5.8, 6.1.0
OS Name/Version: CentOS 7.6.1810, Ubuntu 18.04, Sabayon 19.01

When creating an instance of a defined type and then modifying it with plusignment at a later point, the first plusignment will overwrite the variable completely if it's using its default value at the time.

Desired Behavior:

Plusignment respects the current value - even if it is the default one

Actual Behavior:

Plusignment ignores any default value, replacing it entirely on the first run.

Example manifests that show this problem have been attached, and the issue is demonstrated easily by attempting to apply them;

$ puppet apply broken.pp
Error: Evaluation Error: Error while evaluating a Resource Statement, Defined_type[broken]: parameter 'values' expects an Array value, got String (file: /tmp/tmp.CaxYtnutGf/broken.pp, line: 6) on node example.local
$ puppet apply working.pp
Notice: Scope(Defined_type[broken]): [This works]
Notice: Compiled catalog for example.local in environment production in 0.02 seconds
Notice: Applied catalog in 0.04 seconds
$ puppet apply working2.pp
Notice: Scope(Defined_type[broken]): [This works, as well]
Notice: Compiled catalog for example.local in environment production in 0.01 seconds
Notice: Applied catalog in 0.04 seconds

Add Comment Add Comment
 
This message was sent by Atlassian JIRA (v7.7.1#77002-sha1:e75ca93)
Atlassian logo

Henrik Lindberg (JIRA)

unread,
Feb 21, 2019, 9:52:03 AM2/21/19
to puppe...@googlegroups.com
Henrik Lindberg commented on Bug PUP-9507
 
Re: Plusignment on defined types ignores default value

To make this easier - this is the broken.pp

define defined_type( Array $values = [] ) {
  notice($values)
}
defined_type { 'broken': ; }
 
Defined_type <||> {
  values +> 'This is broken',
}

Henrik Lindberg (JIRA)

unread,
Feb 21, 2019, 9:54:03 AM2/21/19
to puppe...@googlegroups.com

And this is working.pp

define defined_type( Array $values = [] ) {
  notice($values)
}
defined_type { 'broken':
  values => [],
}
 
Defined_type <||> {
  values +> 'This works',
}

Henrik Lindberg (JIRA)

unread,
Feb 21, 2019, 10:20:02 AM2/21/19
to puppe...@googlegroups.com

It has "worked" this way for a very long time. Unfortunately this is an ambiguity - you are only allowed to override a user defined resource that is not yet evaluated (which is the case in your example), at the same time, the resource is not allowed to set its default values until it is evaluated as other expected behavior depends on this.

The problem is made worse because the collectors are based on the old style that "no value"/undef is signalled as an empty string - so it cannot differentiate between "no value" (in which case it would be reasonable to make append set the given override array as the value), and being given an explicit empty string as value. Since it cannot differentiate between the two, if implementation is changed to accept empty string as undef, some users would potentially be broken - although questionable if this "works" as they probably just wold run into the error you encountered. You would still not be able to get the default value from the default value expression.

One could argue that an executed override should set default values before overriding (i.e. partially evaluate). That could work, but could alter the order of evaluation in a way that would cause other breakage for users.

I recommend that you try to express what you want without the use of the +> operator given its quirky evaluation order behavior.

Ping Eric Sorenson Any opinion on this?

Eric Sorenson (JIRA)

unread,
Feb 21, 2019, 2:13:03 PM2/21/19
to puppe...@googlegroups.com
Eric Sorenson commented on Bug PUP-9507

I don't have any opinion on the correctness of the change, but that's because I don't understand what problem led to the creation of the code that exhibits the bug. Virtual resources + plussignment are two features of the language I've personally never needed to use.

I agree that we wouldn't want to change the existing behavior because it'd almost certainly have unintended consequences for other users.

Alexander Olofsson (JIRA)

unread,
Feb 22, 2019, 6:53:03 AM2/22/19
to puppe...@googlegroups.com

I'd be okay with a solution that involves explicitly documenting the fact that default values are lost when doing plusignment.
Requiring the use of explicit assignments in this - frankly quite specific - case is not a horrible thing to demand.

For more information on the background of the issue; our monitoring system builds dynamic configuration by using PuppetDB queries for a specific type, which means all information about host-specific monitoring needs to be stored as parameters on said type in order to be discovered. Which also means that there needs to be a way for other modules to add data into said parameter in case they want to be monitored.
Plusignment proved to be an actual workable solution, as exposing the data as facts seemed to cause excessive amounts of both false positives and false negatives during the 30 minutes between Puppet runs - when checks were changed.

Jorie Tappa (JIRA)

unread,
Feb 25, 2019, 12:56:03 PM2/25/19
to puppe...@googlegroups.com

Jorie Tappa (JIRA)

unread,
Feb 25, 2019, 12:57:03 PM2/25/19
to puppe...@googlegroups.com

Henrik Lindberg (JIRA)

unread,
May 9, 2019, 10:16:03 AM5/9/19
to puppe...@googlegroups.com
Reply all
Reply to author
Forward
0 new messages