(PE-28877) Teach `ASTCompiler` about bolt #2329

donoghuc · 2020-04-27T17:32:10Z

Previously when the compile endpoint was asked to compile AST with references to Bolt objects the compile would fail. This commit updates the compiler to accept serialized pcore and to deserialize it for compilation. This adds the requirement of bolt library code (and some supporting gems [logging, addressable, publis_suffix]) as well as the boltlib module. Part of making the serialized types perform as expected the compiler now creates specialized bolt inventory to back operations on bolt Target options.

donoghuc · 2020-04-27T17:38:31Z

Orchestrator change: https://github.com/puppetlabs/orchestrator/pull/1338

I'm wondering if we should just make a new endpoint or a new version of the existing one? Otherwise there will be some work to do to decide if bolt is available. In order to avoid branching behavior based on existence of bolt I think it might be cleaner to make a new/updated endpoint.

donoghuc · 2020-04-27T17:42:11Z

I've got this master with all the code needed infant-sub.delivery.puppetlabs.net

That includes at least:

pe-puppetserver with the code in this PR
pe-orchestration-services with the code in https://github.com/puppetlabs/orchestrator/pull/1338
puppetserver gem install {logging, addressable} --no-document
update in pe-puppet-server.conf like:

[root@infant-sub ~]# cat /etc/puppetlabs/puppetserver/conf.d/pe-puppet-server.conf 
# configuration for the JRuby interpreters
jruby-puppet: {
    # Where the puppet-agent dependency places puppet, facter, etc...
    # Puppet server expects to load Puppet from this location
    ruby-load-path: [
        "/opt/puppetlabs/puppet/lib/ruby/vendor_ruby",
        "/opt/puppetlabs/puppet/cache/lib",
        "/opt/puppetlabs/server/apps/bolt-server/lib/ruby/gems/bolt-2.6.0/lib"
    ]

donoghuc · 2020-04-27T17:43:46Z

src/clj/puppetlabs/services/master/master_core.clj

@@ -896,7 +896,8 @@
   (schema/optional-key "job_id") schema/Str
   (schema/optional-key "transaction_uuid") schema/Str
   (schema/optional-key "options") {(schema/optional-key "capture_logs") schema/Bool
-                                    (schema/optional-key "log_level") (schema/enum "debug" "info" "warning" "error")}})
+                                    (schema/optional-key "log_level") (schema/enum "debug" "info" "warning" "error")
+                                    (schema/optional-key "boltlib") schema/Str}})


Should perhaps allow array as well.

What does the Orchestrator send?

https://github.com/puppetlabs/orchestrator/pull/1338/files#diff-e3cb1255de6ee2d1bf0cf60785425dd0R310

I updated orchestrator to send two options, the first is whether or not to load bold (we could probably just interpret the presence of boltlib_path for that) and the path to the boltlib module code on disk. Allowing only a list type seems best even if we only ever have a single element.

donoghuc · 2020-04-27T17:45:34Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+              # initializes the necessary type loaders for us.
+              ast = Puppet::Pops::Serialization::FromDataConverter.convert(code)
+              unless ast.is_a?(Puppet::Pops::Model::Program)
+                Puppet.warning("AST IS A: Puppet::Pops::Model::Program")


TODO: delete this debug logging

also this comment is wrong.. right? since unless == if !

Yeah, this was just frustrated debugging... I was not thinking. It needs to be deleted anyway.

Was this going to come out?

donoghuc · 2020-04-27T17:46:09Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+        require 'bolt/apply_inventory'
+        require 'bolt/apply_target'
+        require 'bolt/apply_result'
+        require 'addressable'


Again, this wont be required if puppetlabs/bolt#1770 is accepted

justinstoller · 2020-04-27T17:59:17Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+                ast = Puppet::Pops::Model::Factory.PROGRAM(ast, definitions, ast.locator).model
+              end
+              compiler.evaluate(ast)
+              compiler.instance_variable_get(:@internal_compiler).send(:evaluate_ast_node)


Why are we doing this? afaict, PAL only exists to support Bolt. Seems weird to have an interface's only consumer have to meta-program around the API.

I did not consider that. I just used what was already in bolt. https://github.com/puppetlabs/bolt/blob/bafdcb788c5227244991ac721917732a4008ed76/lib/bolt/catalog.rb#L130

puppetlabs/bolt@09aed28 Looks like it supports node definitions in apply blocks.

justinstoller · 2020-04-27T18:00:38Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+              # silently overwrite?
+              compile_options['facts']['values'].keys.each {|fact_name| vars.delete(fact_name)}
+              Puppet.warning("FACTS KEYS #{compile_options['facts']['values'].keys}")
+              pal.send(:add_variables, compiler.send(:topscope), vars)


What's going on here? Same comment wrt metaprograming around a thing built for this use case.

https://github.com/puppetlabs/puppetserver/pull/2329/files#r416044603 I will see if i can get around this.

I dont think we can get around this. We need to avoid collisions between top level fact names and variables in plan scope. We need to be in the compiler to parse the variables.

*short of changes in Puppet.

It looks like this likely should have been updated in puppet, not bolt: https://github.com/puppetlabs/puppet/blob/master/lib/puppet/pal/pal_impl.rb#L427 so that we

aren't metaprogramming around an existing API (since variables is already part of the in_environment functionality of PAL)

and

We wouldn't have to update this in multiple places.

I would prefer if we didn't block on this for the impending release unless there's concern around safety of the change right now. I don't mind opening tickets and/or doing work to shore up PAL to fit the growing use cases required from bolt so we can improve maintainability, but if we can merge this now and refactor after the release I think we can provide some real user value in PE with the apply feature.

Hey @adreyer (I'm unsure who else to ping here?) - am I way off base with my two concerns above?

I'm with Justin, now that we're no longer under such a time crunch, if PAL isn't working for us, we should update it and not hack around it. If other people out in the world are using it, they should realize it's far from the beaten path and experimental, and therefore subject to change.

Also +1 to a module. That's the official extension point for stuff like this, and we should set a good example by using it.

@justinstoller fwiw on your 2nd concern, we are not loading all of the types/modules that bolt packages internally, just the ones in this module: https://github.com/puppetlabs/bolt/tree/master/bolt-modules/boltlib

Additionally, when we do load the types in this code, they are not loaded via the require 'bolt' call we make: the types are loaded when we include that linked module at the beginning of the modulepath when calling pal.in_environment. This should make it so that only the AST compiler in this new bolt_types mode would ever load any of the bolt types/functions

It looks like Cas has updated the new PR to include changes that only require the specific files from within bolt that the module needs to operate: https://github.com/puppetlabs/puppetserver/pull/2332/files#diff-73104a90aa2b4d7f02119c3f544b30c6R35 rather than the whole project.

To take a step back, personally overall I agree with thoughts towards modularizing the parts of bolt we need in PE, so that we could potentially just load a module instead of adding libpaths etc. but these changes completely block the apply block feature. I support refactoring for maintainability/supportability but I would also like to be able to start testing and exercising apply blocks. It seems to me like making the refactors we are talking about could potentially be rather large, and could potentially take up a significant amount of time.

Apologies for splitting these PRs... Looks like Alex replied in the new one. Can we move the discussion there? #2332 (comment)

justinstoller · 2020-04-27T18:03:29Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+        require 'bolt/apply_result'
+        require 'addressable'
+
+        Puppet[:rich_data] = true
        Puppet[:node_name_value] = compile_options['certname']


Did we miss this when reviewing our threadsafety work? I think the implementation we landed on makes this safe, but I'm pretty sure only by an accident of the implementation.

I dont have context here on this question. I'm guessing its a question to Froyo?

Yeah, sorry that was for Froyo

mwaggett · 2020-04-27T17:52:05Z

src/clj/puppetlabs/services/master/master_core.clj

@@ -896,7 +896,8 @@
   (schema/optional-key "job_id") schema/Str
   (schema/optional-key "transaction_uuid") schema/Str
   (schema/optional-key "options") {(schema/optional-key "capture_logs") schema/Bool
-                                    (schema/optional-key "log_level") (schema/enum "debug" "info" "warning" "error")}})
+                                    (schema/optional-key "log_level") (schema/enum "debug" "info" "warning" "error")
+                                    (schema/optional-key "boltlib") schema/Str}})


(nit) would maybe be clearer if this were boltlib_path?

yeah, i can update that.

mwaggett · 2020-04-27T17:52:18Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

        Puppet[:node_name_value] = compile_options['certname']

+        # TODO: again, error handling/warning here when this is not supplied
+        boltlib_path = Array(compile_options.dig('options', 'boltlib') || '')


Why turn this into an array?

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/pal/pal_impl.rb#L248

mwaggett · 2020-04-27T17:53:46Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+          # No conn info for PE targets can just use empty hash if 
+          # https://github.com/puppetlabs/bolt/pull/1770 is accepted


I don't know what this means. Could this use full words and sentences perhaps?

Yeah, sorry i dint intend the comments here to be finalized. I was asked to put up the rough draft of all the pieces I need to make this work ASAP. It is meant to point to that PR i opened in bolt, there is more details there. Basically the api seems to sugges that config is optional, but we assume the config has a nested key ['transports']['transport'] This just adds the "minimal" config to not make bolt blow up when constructing an ApplyTarget. I'll make sure to update it once i get some feedback from the bolt team.

mwaggett · 2020-04-27T18:00:44Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+              # initializes the necessary type loaders for us.
+              ast = Puppet::Pops::Serialization::FromDataConverter.convert(code)
+              unless ast.is_a?(Puppet::Pops::Model::Program)
+                Puppet.warning("AST IS A: Puppet::Pops::Model::Program")


also this comment is wrong.. right? since unless == if !

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

justinstoller · 2020-04-27T18:09:11Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+            }
+          }
+          bolt_inv = Bolt::ApplyInventory.new(fake_config)
+          Puppet.override(bolt_inventory: bolt_inv) do


What would happen if we didn't override this? iiuc, this will be nil in Puppet Server, does nil not work for a "fake_config".

We need the bolt inventory to support methods on Target objects. A bolt Target is a wrapper around a bolt inventory. Without an inventory backing a target, things like $target.facts will not work.

justinstoller · 2020-04-27T18:10:51Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+            pal.with_catalog_compiler do |compiler|
+              #CODEREVIEW: Do we need to make these configurable in the request?
+              Puppet[:strict] = :warning
+              Puppet[:strict_variables] = false


These are the defaults in Puppet, but the opposite of PAL, right? Should the PAL defaults just change?

I cant find these set anywhere except https://github.com/puppetlabs/puppet/blob/bf06b6258b10c3669e32f670fcd4052ce02e1953/lib/puppet/parser/catalog_compiler.rb#L15-L16 in Puppet.

That catalog compiler is the compiler used by PAL (eg, its setting defaults for PAL there, a normal Catalog compilation goes through a different entrypoint).

The default value for strict is here: https://github.com/puppetlabs/puppet/blob/master/lib/puppet/defaults.rb#L197
and strict_variables here: https://github.com/puppetlabs/puppet/blob/master/lib/puppet/defaults.rb#L2131

justinstoller · 2020-04-27T18:37:05Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

@@ -24,26 +24,79 @@ def self.compile(compile_options)
      end

      def self.compile_ast(code, compile_options)
+        # TODO: move requires to optimal place and guard against FOSS not having bolt lib code
+        # CODEREVIEW: where should these requires be, and what should happen if they are not found?
+        require 'bolt'


If we rescue any load errors when running Bolt so that it is optional, is this endpoint still usable?

My understanding when this endpoint was originally implemented was that Bolt was the only current consumer (or soon to be consumer), but it wasn't necessarily only for Bolt consumption (ie in the future we may open it up to non-Bolt use cases).

I think so, it would roughly be:

if (using bolt) require 'bolt' ... use the code in this PR... else ..use the code that was here before

I can easily do that, i just didnt know if there was a usecase outside of bolt. I do know that there is very little value for plans in PE without supporting bolt types.

SQUASH BEFORE MERGE.

justinstoller · 2020-04-27T22:48:16Z

documentation/puppet-api/v3/compile.markdown

-               "log_level": <one of debug/info/warning/err>}
+               "log_level": <one of debug/info/warning/err>
+               "bolt": <true/false> Whether or not to attempt to load bolt
+               "boltlib": <String> Path to bolt-modules to prepend to modulepath}


This is an array of strings now right?

justinstoller · 2020-04-27T22:50:04Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+          require 'bolt/apply_result'
+          require 'addressable'
+
+          Puppet[:rich_data] = true


Is this needed since the compiler for PAL defaults to having rich_data true: https://github.com/puppetlabs/puppet/blob/master/lib/puppet/parser/catalog_compiler.rb#L18

justinstoller · 2020-04-27T22:51:37Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+                vars = Puppet::Pops::Serialization::FromDataConverter.convert(compile_options['variables']['values'])
+
+                compile_options['facts']['values'].keys.each {|fact_name| vars.delete(fact_name)}
+                # TODO: Refactor PAL api such that we do not need to use private methods


imo, this should have a PUP ticket associated with it

justinstoller · 2020-04-27T22:52:28Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+                ast = Puppet::Pops::Model::Factory.PROGRAM(ast, definitions, ast.locator).model
+
+                compiler.evaluate(ast)
+                compiler.instance_variable_get(:@internal_compiler).send(:evaluate_ast_node)


There should be a PUP ticket associated with this, I think.

justinstoller · 2020-04-27T22:53:49Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+                # That means the apply body either a) consists of just a
+                # NodeDefinition, b) consists of a BlockExpression which may
+                # contain NodeDefinitions, or c) doesn't contain NodeDefinitions.
+                definitions = if ast.is_a?(Puppet::Pops::Model::BlockExpression)


It might be valuable, besides adding PUP tickets to the bits that need to be upstreamed into Puppet, adding links to the tickets/issues/code in Bolt that these are copying.

justinstoller · 2020-04-27T22:57:43Z

src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb

+          require 'bolt/apply_inventory'
+          require 'bolt/apply_target'
+          require 'bolt/apply_result'
+          require 'addressable'


Just to make sure I understand. We need 'bolt/apply_inventory', 'bolt/apply_target', and 'bolt/apply_result' to be loaded so that PCore types that may be in apply block ASTs are defined. But bolt/apply_target requires bolt/target to be loaded. And all of them require 'bolt' to have been loaded previously, but bolt doesn't require addressable? Is that correct?

This seems largely correct. I did not attempt to not whole sale require bolt. I feel like bolt/apply_target should be requiring addressable (but currently does not https://github.com/puppetlabs/bolt/pull/1770/files#diff-452723efd86a7b4a811e7856a524a1d7R3). I think i could spend some more time investigating this.

justinstoller · 2020-04-27T22:59:33Z

This largely looks like what I've been told it should look like. And from what I can glean looking at the Bolt code what it needs to look like. However, I don't think my review has been much more than a rubber ducky exercise. I would love to see @puppetlabs/bolt approve this and if they do, I'll get this merged and promoted.

adreyer · 2020-04-27T23:17:49Z

documentation/puppet-api/v3/compile.markdown

-               "log_level": <one of debug/info/warning/err>}
+               "log_level": <one of debug/info/warning/err>
+               "bolt": <true/false> Whether or not to attempt to load bolt
+               "boltlib": <String> Path to bolt-modules to prepend to modulepath}


Why is this settable by the endpoint it feels like puppet-server configuration? It seems wrong to pass a path to a file on disk over http but I suppose when you're literally accepting arbitrary code it's at least less of a security issue.

Fundamentally managing this is going to be kind of a pain. I explicitly asked skeletor team about where to manage it and the thought was we would manage it in orchestrator. If we can read pe-puppetserver.conf values easily here it probably makes sense to get it from there.

In order to avoid serialization bugs passing data from clojure to ruby pass through facts, trusted_facts, and plan vars as a JSON encoded string in the request, then deserialize them in the compile_ast method.

donoghuc · 2020-04-27T23:33:25Z

src/clj/puppetlabs/services/master/master_core.clj

-   (schema/required-key "trusted_facts") {(schema/required-key "values") {schema/Str schema/Any}}
-   (schema/required-key "facts") {(schema/required-key "values") {schema/Str schema/Any}}
-   (schema/required-key "variables") {(schema/required-key "values") {schema/Str schema/Any}}
+   (schema/required-key "trusted_facts") {(schema/required-key "values") schema/Str}


I realize this will now break the old compile endpoint and we need to figure out a real solution. This was mainly just to see if this will even work.

The serialization bug we are trying to avoid is that it appears that some complex objects are not passed to ruby properly

puppetserver/src/clj/puppetlabs/services/jruby/jruby_puppet_service.clj

Line 89 in 89f3d86

(.compileAST jruby-instance compile-options))

Specifically we will see for example when a key in a hash points to a value that is a list of hashes ruby sees it as something like "missing_update_kbs"=>#<Java::ClojureLang::PersistentVector:0x61cfd20e>,. My quick workaround (https://github.com/puppetlabs/orchestrator/pull/1338/commits/39ff2e5b592ff742ae16d62af49580240b3800f2) was to serialize as a JSON string in so its just a string when clojure passes it through to ruby. This allows ruby to just deserialize the JSON.

That makes sense, I actually wonder why we haven't seen that in the endpoint that CD4PE uses. That's this code fwiw.

It looks like in normal catalog compilation we pass the facts in as a string so they can be deserialized by Puppet here.

fwiw, I think you want to use Puppet::Util::Json here.

TBH I think I might advocate to just make the breaking API change. Nothing else has used this, since no-one ever opened tickets or anything to fix the serialization problem. Since the endpoint is broken without this change I think breaking the API should be alright in this case.

mcdonaldseanp · 2020-04-28T14:13:01Z

documentation/puppet-api/v3/compile.markdown

@@ -36,7 +36,9 @@ The request body must look like:
  "transaction_uuid": "<uuid string>",
  "job_id": "<id string>",
  "options": { "capture_logs": <true/false>,
-               "log_level": <one of debug/info/warning/err>}
+               "log_level": <one of debug/info/warning/err>
+               "bolt": <true/false> Whether or not to attempt to load bolt


nit: change to bolt_types to identify what we are doing with bolt (loading bolt types)

src/java/com/puppetlabs/puppetserver/JRubyPuppet.java

SQUASH BEFORE MERGE>

donoghuc · 2020-04-28T18:18:16Z

Thanks for the feedback everyone. I'm going to close this in favor of a fresh one that takes in to consideration everything we decided on for this.

donoghuc requested a review from a team April 27, 2020 17:32

donoghuc commented Apr 27, 2020

View reviewed changes

justinstoller reviewed Apr 27, 2020

View reviewed changes

mwaggett reviewed Apr 27, 2020

View reviewed changes

justinstoller reviewed Apr 27, 2020

View reviewed changes

(SQUASH-ME) Updates based on review

b43cc28

SQUASH BEFORE MERGE.

donoghuc requested a review from a team April 27, 2020 22:10

justinstoller reviewed Apr 27, 2020

View reviewed changes

adreyer reviewed Apr 27, 2020

View reviewed changes

(PE-28941) Expect plan vars and facts as JSON encoded string

08ae2b6

In order to avoid serialization bugs passing data from clojure to ruby pass through facts, trusted_facts, and plan vars as a JSON encoded string in the request, then deserialize them in the compile_ast method.

donoghuc commented Apr 27, 2020

View reviewed changes

donoghuc added the DO NOT MERGE label Apr 27, 2020

mcdonaldseanp reviewed Apr 28, 2020

View reviewed changes

donoghuc commented Apr 28, 2020

View reviewed changes

src/java/com/puppetlabs/puppetserver/JRubyPuppet.java Show resolved Hide resolved

(TMP-SQUASH) Temporarly commit for debugging help

983b31a

SQUASH BEFORE MERGE>

donoghuc force-pushed the pops-deserialization branch from 5d82019 to 983b31a Compare April 28, 2020 17:04

donoghuc closed this Apr 28, 2020

justinstoller mentioned this pull request Apr 29, 2020

(PE-28877, PE-28941) Add option to compile-ast endpoint to load bolt types #2332

Merged

		# No conn info for PE targets can just use empty hash if
		# https://github.com/puppetlabs/bolt/pull/1770 is accepted

(PE-28877) Teach ASTCompiler about bolt #2329

(PE-28877) Teach ASTCompiler about bolt #2329

Conversation

donoghuc commented Apr 27, 2020

donoghuc commented Apr 27, 2020

donoghuc commented Apr 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdonaldseanp Apr 29, 2020 • edited

Choose a reason for hiding this comment

mcdonaldseanp Apr 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinstoller Apr 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinstoller commented Apr 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdonaldseanp Apr 28, 2020 • edited

Choose a reason for hiding this comment

donoghuc commented Apr 28, 2020

(PE-28877) Teach `ASTCompiler` about bolt #2329

(PE-28877) Teach `ASTCompiler` about bolt #2329

mcdonaldseanp Apr 29, 2020 •

edited

mcdonaldseanp Apr 29, 2020 •

edited

justinstoller Apr 27, 2020 •

edited

mcdonaldseanp Apr 28, 2020 •

edited