Skip to content

(GH-1350) Configure plugins using other plugins #1381

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

Merged

Conversation

nicklewis
Copy link
Contributor

@nicklewis nicklewis commented Nov 5, 2019

Previously, the plugins section of bolt.yaml was required to be
completely static. Now, plugin can use the same _plugin hashes as in
inventory.yaml to define their configuration. For example, this allows
secrets for plugins to be encrypted or read from vault.

We now instantiate (and thus validate) all plugins that are configured in
bolt.yaml at the time the plugin system is initialized.

We also now support _plugin lookups under the plugin_hooks key for
similar reasons.
Closes GH-1350

@nicklewis nicklewis requested review from K8med and a team as code owners November 5, 2019 21:31
@nicklewis
Copy link
Contributor Author

This currently depends on the resolve_references refactor from #1374. I'll rebase once that's merged.

@nicklewis nicklewis added Do Not Merge Work that should not be merged yet. Blocked Work blocked by other issues or PRs. labels Nov 5, 2019
@puppetcla
Copy link

CLA signed by all contributors.

@nicklewis nicklewis force-pushed the GH-1350-resolve-references-in-config branch 4 times, most recently from 4652708 to 8b3d2bc Compare November 8, 2019 18:23
@nicklewis nicklewis removed Blocked Work blocked by other issues or PRs. Do Not Merge Work that should not be merged yet. labels Nov 8, 2019
@donoghuc donoghuc self-requested a review November 8, 2019 21:57
Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

I think we need to define how we would expect populating the entire section for plugins or plugin_hooks should behave. Currently it seems like for plugins the settings are ignored, and for plugin_hooks parameters retrieved from the plugin_hook in the yaml are attempted to be passed again to the task that implements the plugin.

plugins:
  _plugin: yaml
  filepath: /home/cas/working_dir/bolt/Boltdir/pkcs.yaml
plugin_hooks:
  _plugin: yaml
  filepath: /home/cas/working_dir/bolt/Boltdir/puppet_install.yaml

puppet_install.yaml

puppet_library:
  plugin: puppet_agent
  stop_service: false
  _run_as:
    _plugin: prompt
    message: run as user

pkcs7.yaml

pkcs7:
  private-key: ~/working_dir/dynamic-inventory-demo/keys/public.pkcs7.pem
  public-key: ~/working_dir/dynamic-inventory-demo/keys/private.pkcs7.pem
  keysize: 4096
cas@cas-ThinkPad-T460p:~/working_dir/bolt$ bolt apply -e "notice('hji')" -t damp-quartz.delivery.puppetlabs.net
Task yaml::resolve_reference:
 has no parameter named 'puppet_library'

@nicklewis nicklewis force-pushed the GH-1350-resolve-references-in-config branch 2 times, most recently from af35b3a to 5882d19 Compare November 11, 2019 21:26
@nicklewis
Copy link
Contributor Author

Pushed a new commit to handle setting plugin_hooks with a top-level plugin reference. We now evaluate the plugins first and then merge them over top of the defaults rather than vice versa.

Also squashed in a change to check whether plugins is set with a top-level plugin reference and fail.

Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

This looks good. In testing this out I found a couple places where a stacktrace would surface when assuming keys are in a incorrectly formed plugin reference. The simplest example is with a bolt.yaml simply have a key for plugins:

plugins:

But validating user input there does not seem like part of this work.

@donoghuc
Copy link
Contributor

@nicklewis Can you add a CHANGELOG update for this issue?

Previously, the update_from_inventory method was implemented in terms of
update_from_file and relied on the fact that most of the behavior of
update_from_file would be a noop as it operated on data disallowed in
the inventory. This commit extracts the common behavior, which is only
related to configuring transports, and calls that method from both
places. This allows the behavior for the config file to be separate from
behavior for config from inventory.
Previously, the `plugins` section of `bolt.yaml` was required to be
completely static. Now, plugin can use the same `_plugin` hashes as in
`inventory.yaml` to define their configuration. For example, this allows
secrets for plugins to be encrypted or read from vault.

We now instantiate (and thus validate) all plugins that are configured
in `bolt.yaml` at the time the plugin system is initialized.
Previously, the value of `plugin_hooks` had to be static with all
hooks and parameters being defined in the config file. We now allow
these hooks and parameters to be looked up using plugins, which is
helpful if an external source is responsible for controlling the Puppet
version to use or if the Puppet install method requires the knowledge of
some secret.
…rence

Previously, setting `plugin_hooks` to a `_plugin` reference would cause
a failure because we would merge the default `plugin_hooks` with the
reference before evaluating it. That caused the reference to receive
arguments it didn't expect. We now evaluate the configured
`plugin_hooks` key and then merge it over top of the defaults afterward.
@nicklewis nicklewis force-pushed the GH-1350-resolve-references-in-config branch from 5882d19 to c257b05 Compare November 14, 2019 20:03
@nicklewis
Copy link
Contributor Author

Rebased and updated the changelog

@nicklewis nicklewis force-pushed the GH-1350-resolve-references-in-config branch from c257b05 to 0408fbd Compare November 14, 2019 20:23
@donoghuc donoghuc merged commit 7709c9e into puppetlabs:master Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use resolve_reference in bolt.yaml
4 participants