Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PE-28877) Teach ASTCompiler about bolt #2329

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion documentation/puppet-api/v3/compile.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"boltlib": <String> Path to bolt-modules to prepend to modulepath}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an array of strings now right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh, yeah

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
```

Expand Down
9 changes: 7 additions & 2 deletions src/clj/puppetlabs/services/jruby/jruby_puppet_schemas.clj
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@

* :use-legacy-auth-conf - Whether to use the legacy core Puppet auth.conf
(true) or trapperkeeper-authorization (false) to authorize requests
being made to core Puppet endpoints."
being made to core Puppet endpoints.

* :botlib-path - Optional array containng path(s) to bolt modules. This path
will be prepended to AST compilation modulepath. This is required for
compiling AST that contains bolt types"
{:master-conf-dir (schema/maybe schema/Str)
:master-code-dir (schema/maybe schema/Str)
:master-var-dir (schema/maybe schema/Str)
Expand All @@ -67,5 +71,6 @@
:http-client-idle-timeout-milliseconds schema/Int
:http-client-metrics-enabled schema/Bool
:max-requests-per-instance schema/Int
:use-legacy-auth-conf schema/Bool})
:use-legacy-auth-conf schema/Bool
(schema/optional-key :botlib-path) [schema/Str]})

4 changes: 2 additions & 2 deletions src/clj/puppetlabs/services/jruby/jruby_puppet_service.clj
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@
(.compileCatalog jruby-instance request-options))

(compile-ast
[this jruby-instance compile-options]
(.compileAST jruby-instance compile-options))
[this jruby-instance compile-options boltlib-path]
(.compileAST jruby-instance compile-options boltlib-path))

(get-environment-module-info
[this jruby-instance env-name]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
master-ns
config)
jruby-service (tk-services/get-service this :JRubyPuppetService)
boltlib-path (get-in config
[:jruby-puppet :boltlib-path]
[""])
master-routes (comidi/context path
(master-core/root-routes handle-request
(partial identity)
Expand All @@ -45,7 +48,8 @@
(throw (IllegalStateException.
(i18n/trs "Versioned code not supported."))))
(constantly nil)
false))
false
boltlib-path))
master-route-handler (comidi/routes->handler master-routes)
master-mount (master-core/get-master-mount
master-ns
Expand Down
45 changes: 29 additions & 16 deletions src/clj/puppetlabs/services/master/master_core.clj
Original file line number Diff line number Diff line change
Expand Up @@ -890,13 +890,15 @@
{(schema/required-key "certname") schema/Str
(schema/required-key "environment") schema/Str
(schema/required-key "code_ast") schema/Str
(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}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(.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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(schema/required-key "facts") {(schema/required-key "values") schema/Str}
(schema/required-key "variables") {(schema/required-key "values") schema/Str}
(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 "bolt") schema/Bool
(schema/optional-key "boltlib_path") [schema/Str]}})

(defn validated-body
[body schema]
Expand Down Expand Up @@ -929,7 +931,8 @@

(schema/defn ^:always-validate compile-fn :- IFn
[jruby-service :- (schema/protocol jruby-protocol/JRubyPuppetService)
current-code-id-fn :- IFn]
current-code-id-fn :- IFn
boltlib-path :- [schema/Str]]
(fn [request]
(let [env (jruby-request/get-environment-from-request request)
request-options (-> request
Expand All @@ -944,7 +947,8 @@
:body (json/encode
(jruby-protocol/compile-ast jruby-service
(:jruby-instance request)
compile-options))})))
compile-options
boltlib-path))})))

(schema/defn ^:always-validate
v4-catalog-handler :- IFn
Expand All @@ -960,8 +964,9 @@
compile-handler :- IFn
[jruby-service :- (schema/protocol jruby-protocol/JRubyPuppetService)
wrap-with-jruby-queue-limit :- IFn
current-code-id-fn :- IFn]
(-> (compile-fn jruby-service current-code-id-fn)
current-code-id-fn :- IFn
boltlib-path :- [schema/Str]]
(-> (compile-fn jruby-service current-code-id-fn boltlib-path)
(jruby-request/wrap-with-jruby-instance jruby-service)
wrap-with-jruby-queue-limit
jruby-request/wrap-with-error-handling))
Expand Down Expand Up @@ -1023,7 +1028,8 @@
get-code-content-fn :- IFn
current-code-id-fn :- IFn
cache-enabled :- schema/Bool
wrap-with-jruby-queue-limit :- IFn]
wrap-with-jruby-queue-limit :- IFn
boltlib-path :- [schema/Str]]
(let [class-handler (create-cacheable-info-handler-with-middleware
(fn [jruby env]
(some-> jruby-service
Expand Down Expand Up @@ -1057,7 +1063,8 @@
compile-handler' (compile-handler
jruby-service
wrap-with-jruby-queue-limit
current-code-id-fn)]
current-code-id-fn
boltlib-path)]
(comidi/routes
(comidi/POST "/compile" request
(compile-handler' request))
Expand Down Expand Up @@ -1107,15 +1114,17 @@
get-code-content-fn :- IFn
current-code-id-fn :- IFn
environment-class-cache-enabled :- schema/Bool
wrap-with-jruby-queue-limit :- IFn]
wrap-with-jruby-queue-limit :- IFn
boltlib-path :- [schema/Str]]
(comidi/context "/v3"
(v3-ruby-routes ruby-request-handler)
(comidi/wrap-routes
(v3-clojure-routes jruby-service
get-code-content-fn
current-code-id-fn
environment-class-cache-enabled
wrap-with-jruby-queue-limit)
wrap-with-jruby-queue-limit
boltlib-path)
clojure-request-wrapper)))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Expand Down Expand Up @@ -1216,15 +1225,17 @@
wrap-with-jruby-queue-limit :- IFn
get-code-content-fn :- IFn
current-code-id-fn :- IFn
environment-class-cache-enabled :- schema/Bool]
environment-class-cache-enabled :- schema/Bool
boltlib-path :- [schema/Str]]
(comidi/routes
(v3-routes ruby-request-handler
clojure-request-wrapper
jruby-service
get-code-content-fn
current-code-id-fn
environment-class-cache-enabled
wrap-with-jruby-queue-limit)
wrap-with-jruby-queue-limit
boltlib-path)
(v4-routes clojure-request-wrapper
jruby-service
wrap-with-jruby-queue-limit
Expand Down Expand Up @@ -1301,7 +1312,8 @@
handle-request :- IFn
wrap-with-authorization-check :- IFn
wrap-with-jruby-queue-limit :- IFn
environment-class-cache-enabled :- schema/Bool]
environment-class-cache-enabled :- schema/Bool
boltlib-path :- [schema/Str]]
(let [ruby-request-handler (get-wrapped-handler handle-request
wrap-with-authorization-check
puppet-version
Expand All @@ -1317,7 +1329,8 @@
wrap-with-jruby-queue-limit
get-code-content
current-code-id
environment-class-cache-enabled)))
environment-class-cache-enabled
boltlib-path)))

(def MasterStatusV1
{(schema/optional-key :experimental) {:http-metrics [http-metrics/RouteSummary]
Expand Down
7 changes: 6 additions & 1 deletion src/clj/puppetlabs/services/master/master_service.clj
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@
max-retry-delay))
identity)

boltlib-path (get-in config
[:jruby-puppet :boltlib-path]
[""])

ring-app (comidi/routes
(core/construct-root-routes puppet-version
use-legacy-auth-conf
Expand All @@ -131,7 +135,8 @@
handle-request
(get-auth-handler)
wrap-with-jruby-queue-limit
environment-class-cache-enabled))
environment-class-cache-enabled
boltlib-path))
routes (comidi/context path ring-app)
route-metadata (comidi/route-metadata routes)
comidi-handler (comidi/routes->handler routes)
Expand Down
2 changes: 1 addition & 1 deletion src/clj/puppetlabs/services/protocols/jruby_puppet.clj
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@
"Compile the catalog for a given certname")

(compile-ast
[this jruby-instance compile-options]
[this jruby-instance compile-options boltlib-path]
"Compiles arbitrary Puppet ASTs into mini catalogs")

(get-environment-module-info
Expand Down
2 changes: 1 addition & 1 deletion src/java/com/puppetlabs/puppetserver/JRubyPuppet.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public interface JRubyPuppet {
List getTransportInfoForEnvironment(String environment);
List getModuleInfoForEnvironment(String environment);
Map compileCatalog(Map requestBody);
Map compileAST(Map compileOptions);
Map compileAST(Map compileOptions, List boltlibPath);
donoghuc marked this conversation as resolved.
Show resolved Hide resolved
Map getModuleInfoForAllEnvironments();
JRubyPuppetResponse handleRequest(Map request);
Object getSetting(String setting);
Expand Down
121 changes: 97 additions & 24 deletions src/ruby/puppetserver-lib/puppet/server/ast_compiler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,45 +5,118 @@
module Puppet
module Server
class ASTCompiler
def self.compile(compile_options)
def self.compile(compile_options, boltlib_path)
options = compile_options['options'] || {}

log_level = options['log_level']
code = JSON.parse(compile_options['code_ast'])

if options['capture_logs']
catalog, logs = Logging.capture_logs(log_level) do
compile_ast(code, compile_options)
compile_ast(code, compile_options, boltlib_path)
end

{ catalog: catalog, logs: logs }
else
catalog = compile_ast(code, compile_options)
catalog = compile_ast(code, compile_options, boltlib_path)
{ catalog: catalog }
end
end

def self.compile_ast(code, compile_options)
Puppet[:node_name_value] = compile_options['certname']

# Use the existing environment with the requested name
Puppet::Pal.in_environment(compile_options['environment'],
envpath: Puppet[:environmentpath],
facts: compile_options['facts']['values'],
variables: compile_options['variables']['values']) do |pal|
justinstoller marked this conversation as resolved.
Show resolved Hide resolved
Puppet.lookup(:pal_current_node).trusted_data = compile_options['trusted_facts']['values']

# This compiler has been configured with a node containing
# the requested environment, facts, and variables, and is used
# to compile a catalog in that context from the supplied AST.
pal.with_catalog_compiler do |compiler|
# We have to parse the AST inside the compiler block, because it
# initializes the necessary type loaders for us.
ast = Puppet::Pops::Serialization::FromDataConverter.convert(code)

compiler.evaluate(ast)
compiler.compile_additions
compiler.catalog_data_hash
def self.compile_ast(code, compile_options, boltlib_path)
# If the request requires that bolt be loaded we assume we are in a properly
# configured PE environment
if compile_options.dig('options', 'bolt')
require 'bolt'
require 'bolt/target'
require 'bolt/apply_inventory'
require 'bolt/apply_target'
require 'bolt/apply_result'
require 'addressable'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Puppet[:rich_data] = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Puppet[:node_name_value] = compile_options['certname']
json_deserialized_facts = JSON.parse(compile_options['facts']['values'])
json_deserialized_trusted_facts = JSON.parse(compile_options['trusted_facts']['values'])
json_deserialzed_vars = JSON.parse(compile_options['variables']['values'])
Puppet.warning("boltib_path: #{boltlib_path}")
# Use the existing environment with the requested name
Puppet::Pal.in_environment(compile_options['environment'],
pre_modulepath: boltlib_path,
envpath: Puppet[:environmentpath],
facts: json_deserialized_facts) do |pal|
# TODO: We need to decide on an appropriate config for inventory. Given we
# hide this from plan authors this current iteration has only the "required"
# data for now. This is being discussed on https://github.com/puppetlabs/bolt/pull/1770
fake_config = {
'transport' => 'redacted',
'transports' => {
'redacted' => 'redacted'
}
}
bolt_inv = Bolt::ApplyInventory.new(fake_config)
Puppet.override(bolt_inventory: bolt_inv) do
Puppet.lookup(:pal_current_node).trusted_data = json_deserialized_trusted_facts
# This compiler has been configured with a node containing
# the requested environment, facts, and variables, and is used
# to compile a catalog in that context from the supplied AST.
pal.with_catalog_compiler do |compiler|
#TODO: Should we update these in PAL? They are the defaults in Puppet
Puppet[:strict] = :warning
Puppet[:strict_variables] = false
vars = Puppet::Pops::Serialization::FromDataConverter.convert(json_deserialzed_vars)

json_deserialized_facts.keys.each {|fact_name| vars.delete(fact_name)}
# TODO: Refactor PAL api such that we do not need to use private methods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo, this should have a PUP ticket associated with it

pal.send(:add_variables, compiler.send(:topscope), vars)

# We have to parse the AST inside the compiler block, because it
# initializes the necessary type loaders for us.
ast = Puppet::Pops::Serialization::FromDataConverter.convert(code)
# Node definitions must be at the top level of the apply block.
# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ast.statements.select { |st| st.is_a?(Puppet::Pops::Model::NodeDefinition) }
elsif ast.is_a?(Puppet::Pops::Model::NodeDefinition)
[ast]
else
[]
end
ast = Puppet::Pops::Model::Factory.PROGRAM(ast, definitions, ast.locator).model

compiler.evaluate(ast)
compiler.instance_variable_get(:@internal_compiler).send(:evaluate_ast_node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

compiler.compile_additions
compiler.catalog_data_hash
end
end
end
else
# When we do not need to load bolt we assume there are no Bolt types to resolve in
# AST compilation.
Puppet[:node_name_value] = compile_options['certname']

# Use the existing environment with the requested name
Puppet::Pal.in_environment(compile_options['environment'],
envpath: Puppet[:environmentpath],
facts: compile_options['facts']['values'],
variables: compile_options['variables']['values']) do |pal|
Puppet.lookup(:pal_current_node).trusted_data = compile_options['trusted_facts']['values']

# This compiler has been configured with a node containing
# the requested environment, facts, and variables, and is used
# to compile a catalog in that context from the supplied AST.
pal.with_catalog_compiler do |compiler|
# We have to parse the AST inside the compiler block, because it
# initializes the necessary type loaders for us.
ast = Puppet::Pops::Serialization::FromDataConverter.convert(code)

compiler.evaluate(ast)
compiler.compile_additions
compiler.catalog_data_hash
end
end
end
end
Expand Down
10 changes: 8 additions & 2 deletions src/ruby/puppetserver-lib/puppet/server/master.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,14 @@ def compileCatalog(request_data)
end
end

def compileAST(compile_options)
Puppet::Server::ASTCompiler.compile(convert_java_args_to_ruby(compile_options))
def compileAST(compile_options, boltlib_path)
translated_compile_options = convert_java_args_to_ruby(compile_options)
translated_boltlib_path = if boltlib_path.java_kind_of?(Java::JavaUtil::List)
boltlib_path.to_a
else
Array("")
end
Puppet::Server::ASTCompiler.compile(translated_compile_options, translated_boltlib_path)
end

def create_recorder
Expand Down