POC hack to parse Puppetfile directly W/O eval

[binford2k]

Ruby 2.6 instroduced the RubyVM::AbstractSyntaxTree, which will parse
arbitrary Ruby code into an AST. This means that instead of eval'ing
code, we can just parse it and take action on the three methods we care
about. In other words, the Puppetfile will no longer be arbitrary and
unchecked Ruby code.

See https://docs.ruby-lang.org/en/trunk/RubyVM/AbstractSyntaxTree.html
for more information.

Note: This implementation is experimental and will almost certainly
change. This should not be merged until upstream commits to a stable
API. But it's worth exploring and validating that we can indeed have
non-executable Puppetfiles.

[dhollinger]

👍

[dhollinger]

Needs rebase

[mdwheele]

In our environment, we have multiple tenants separated into separate control repositories that all depend on shared modules (roles/profiles) we maintain as a community. This means that when we decide to update underlying component modules, we have to communicate those changes to individual tenants and it creates a release bottleneck.

One "hack" that we've been using in the last year was to have our r10k node write the "shared upstream Puppetfile" to disk and then in tenant Puppetfile, they do the following:

# "Include" shared Puppetfile
self.instance_eval(File.read('/usr/local/share/r10k/Puppetfile'))

# 
# Tenant-specific dependencies
#
mod 'camptocamp-kmod', '2.3.0'
mod 'crayfishx-firewalld', '3.4.0'

The fact that Puppetfile are eval()-d as Ruby allows us to solve the problem. If an AST-based approach were used, I imagine this Puppetfile would drop the call the self.instance_eval, but actually work (install 2 modules listed). In our case, this tenant would be "broken" in that modules would not be available even though r10k works as designed.

This works out pretty well, but might be somewhat impacted by this line of development. Is there an alternative solution to this problem you would suggest? If not, is this feature something that could be gated as configuration?

Also, to be clear, definitely amenable to any suggestions here as it's likely not a new problem. Not interested in holding the project back, so if worst comes to worst, we'll either hold back the version of r10k or go back to manually managing the Puppetfile.

/edit

I don't know how interested folks would be in specialized r10k-specific Puppetfile syntax, but an include /path/to/Puppetfile could very easily be built on top of this PR as well. So that may or may not be a reasonable solution. I don't know if there is an "official" standards body / specification for Puppetfile 😀

[jadestorm]

FWIW, I would love to see a formally supported "include" in Puppetfile parsing.

[mdwheele]

I have implemented an include syntax for Puppetfile in #927 that also includes the RubyVM-based AST parser implementation in e744c0c

[glennsarti]

Could also use Ripper to do the AST generation so as to lift the Ruby 2.6 restriction.

[binford2k]

Ruby 2.6 is no longer an onerous requirement since 2.5 goes EOL on 2021-03-31. I would like to make a decision on this. I believe that executable Puppetfiles are dangerous. We can gate this behind a config option and make it default in 4.x.

Can someone put some 👀 on this?

[mwaggett]

The docs say that the RubyVM::AbstractSyntaxTree class is MRI-specific. Is that going to work when we use r10k in code-manager, since we use Jruby? /cc @puppetlabs/puppetserver-maintainers

I agree that just calling instance_eval on the Puppetfile seems dangerous, but I'm hesitant about this implementation. They still say that the API is not stable and this doesn't seem very straightforward to maintain. They suggest using parser or Ripper instead.

[binford2k]

@mwaggett that's a really good point. I think that CM just fork/execs r10k in its own process, but not entirely sure.

Regardless, you make a good point that parser or ripper would be better options. I unfortunately don't have time to refactor that, but I'll open a ticket to do so and reference this PR. Thanks!