[PUP-4378] Resource collectors can be assigned to variables by abusing chaining statements Created: 2015/04/06  Updated: 2017/05/15  Resolved: 2015/05/20

Status: Closed
Project: Puppet
Component/s: Language
Affects Version/s: PUP 3.7.5
Fix Version/s: PUP 4.1.0

Type: Bug Priority: Minor
Reporter: Nicholas Fagerlund Assignee: Unassigned
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to PUP-7541 Explore removing export / collect / v... Closed
Template:
Epic Link: 4.x Language
Story Points: 1
Sprint: Language 2015-04-29
Release Notes: Bug Fix
QA Contact: Eric Thompson

 Description   

The 4x parser generally refuses to produce values from resource collectors, and errors if you put one where a value is expected. But since the value of a chaining statement is just the final term of the statement (regardless of arrow direction) and they accept collectors as operands:

notify {"I go last.":}
$myweirdchain = (file {"/tmp/thing": ensure => file,} -> Notify <| |>)
notice( $myweirdchain )

Notice: Scope(Class[main]): [#<Puppet::Pops::Evaluator::Collectors::CatalogCollector:0x007fe33da72c90>]

I don't really have anything to add, and probably no one will ever try to do that. But it does seem unintended.

risk: medium (manual validate only for now)
probability: low
severity: medium (confusion, possible abuse?)
test layer: unit (no unit tests added in PR. Henrik Lindberg can you comment on if you think this needs testing, and at what layer?)



 Comments   
Comment by Nicholas Fagerlund [ 2015/04/06 ]

Also I have a function that goes like this:

Puppet::Parser::Functions::newfunction(:spew, :doc => "Dump an inspect of whatever we get.", :type => :rvalue) do |args|
  #return args.inspect
  return "#{args.inspect}\n#{args.collect {|thing| thing.class}.inspect}"
end

and if you feed $myweirdchain to THAT, it dumps basically the entire program state, afaict.

Comment by Henrik Lindberg [ 2015/04/07 ]

We do need to produce the Collector value and return it and should handle it throughout the system. In this case I think it is basically just the "label provider" that does not have an entry for that runtime object, so the output becomes the class name and its id.

Then, naturally inspecting a Collector, since it has references to almost everything via Catalog you get a lot of output. We could possibly fix this by using a different runtime object that wraps the collector, or modify its inspect method. Not sure that is a real problem though.

Ideally we should be able to do something more intelligent with a Collector, but that is not yet specified.

Nicholas Fagerlund is an acceptable fix for this to just make sure that something intelligent is being displayed for a collector? (In wait for being able to do more with collectors in the future)?

Comment by Nicholas Fagerlund [ 2015/04/07 ]

Henrik Lindberg Seems fine to me. Would this also entail allowing collectors to be used in contexts that require a value?

Comment by Henrik Lindberg [ 2015/04/07 ]

Not really, they have very low precedence, and you have to play tricks (like you did) to get it as a value assigned to something. At the moment there is very little you can do with the collector object, but if you get hold of it and pass it to a ruby function it could be inspected; how much has been collected so far etc. The problem is that it is not fully evaluated until after all puppet logic has been executed (except for possibly some additional overrides). It is thus "beyond being evaluation order dependent".

Giving it a proper label just removes one surprise if you happen to catch one of these otherwise shy values and display it.

We could naturally make them proper first class values, but we need to define the semantics for them; what can you do with them? (I would like to be able to iterate over them, perhaps give it a lambda that is called as a callback, etc. (Have not thought this through yet). There are also discussions about "queries" in general as values. That may be what we do, and collection would be a specialization of such a query object, but again, that is also not designed yet.

Comment by Henrik Lindberg [ 2015/04/08 ]

PR 3798 changes the output to a human readable label of:
"Catalog-Collector[type]", or "Exported-Collector[type]".

This should be more informative. It is unfortunately not possible to output the query as it is is encoded as a Ruby Proc with the predicate logic.

Expect future improvements in later versions where it may be possible to use the collector objects.

Comment by Thomas Hallgren [ 2015/04/20 ]

Merged to master at fcc4e08

Comment by Eric Thompson [ 2015/04/21 ]

validated on ubuntu14 at SHA: ffaffb9

[root@plgf5s85ojlj8m1 ~]# puppet apply off_the_chain.pp
Notice: Scope(Class[main]): [Catalog-Collector[Notify]]
Notice: Compiled catalog for plgf5s85ojlj8m1.delivery.puppetlabs.net in environment production in 0.50 seconds
Notice: /Stage[main]/Main/File[/tmp/thing]/ensure: created
Notice: I go last.
Notice: /Stage[main]/Main/Notify[I go last.]/message: defined 'message' as 'I go last.'
Notice: Applied catalog in 0.01 seconds
[root@plgf5s85ojlj8m1 ~]# vi off_the_chain.pp
[root@plgf5s85ojlj8m1 ~]# puppet apply off_the_chain.pp
Notice: Scope(Class[main]): [Catalog-Collector[Notify]]
Notice: Compiled catalog for plgf5s85ojlj8m1.delivery.puppetlabs.net in environment production in 0.51 seconds
Notice: Applied catalog in 0.01 seconds
[root@plgf5s85ojlj8m1 ~]# cat off_the_chain.pp
$myweirdchain = (file {"/tmp/thing": ensure => file,} -> Notify <| |>)
notice( $myweirdchain )
[root@plgf5s85ojlj8m1 ~]# vi off_the_chain.pp
[root@plgf5s85ojlj8m1 ~]# puppet apply off_the_chain.pp
Warning: You cannot collect exported resources without storeconfigs being set; the collection will be ignored at /root/off_the_chain.pp:1:58
Notice: Scope(Class[main]): [Exported-Collector[Notify]]
Warning: Not collecting exported resources without storeconfigs
Notice: Compiled catalog for plgf5s85ojlj8m1.delivery.puppetlabs.net in environment production in 0.75 seconds
Notice: Applied catalog in 0.01 seconds

Henrik Lindberg, I always thought <| |> was known as a "resource collector" but here we are calling them a "catalog collector", I can see the similarities. Will this cause confusion, or is it an intentional difference? Possibly I don't understand some difference in Type vs. instance.

Comment by Eric Thompson [ 2015/04/21 ]

validated on windows2012r2-rubyx64 at SHA: 39bd7a7

dministrator@f67onhgjleryc6t /opt/puppet-git-repos/puppet
$ cat off_the_chain.pp
$myweirdchain = (file {"/tmp/thing": ensure => file,} -> Notify <| |>)
notice( $myweirdchain )
 
Administrator@f67onhgjleryc6t /opt/puppet-git-repos/puppet
$ cmd /c puppet apply off_the_chain.pp
Notice: Scope(Class[main]): [Catalog-Collector[Notify]]
Notice: Compiled catalog for f67onhgjleryc6t.delivery.puppetlabs.net in environment production in 0.62 seconds

Generated at Sat Aug 24 21:53:05 PDT 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.