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

Fixes #12865: Vault plugin : Add a variable_from_vault generic method. #65

Conversation

victorqrt
Copy link
Contributor

@victorqrt victorqrt force-pushed the ust_12865/vault_plugin_add_a_variable_from_vault_generic_method branch from be14573 to 6d53dcd Compare July 12, 2018 12:56
@@ -0,0 +1,3 @@
#!/bin/sh
cp -a /opt/rudder/share/plugins/vault/usr/share/ncf/tree /usr/share/ncf/
Copy link
Member

Choose a reason for hiding this comment

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

@ncharles do you think we should put it with CFEgine stubs for dsc methods instead?

Copy link
Member

Choose a reason for hiding this comment

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

yes, because if we put it in /usr/share/ncf, it'll be erased at ncf upgrade


pass2.vault_reachable::
"vault.auth_token" string => "${data_output[auth][client_token]}",
ifvarclass => isvariable("data_output[auth][client_token]");
Copy link
Member

Choose a reason for hiding this comment

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

We should display an error message if the config is not defined

Copy link
Member

Choose a reason for hiding this comment

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

👍

# @description Gets a key-value dictionnary from Vault given the secret path
#
# @documentation
# Vault
Copy link
Member

Choose a reason for hiding this comment

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

you'll need to improve a bit the documentation

@victorqrt victorqrt force-pushed the ust_12865/vault_plugin_add_a_variable_from_vault_generic_method branch from 6d53dcd to 0fc63ae Compare July 13, 2018 09:01
@victorqrt
Copy link
Contributor Author

Commit modified

Copy link
Member

@ncharles ncharles left a comment

Choose a reason for hiding this comment

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

This is great ! I made some really minor remarks, otherwise this is a great addition to Rudder

cp -a /opt/rudder/share/plugins/vault/* /var/rudder/configuration-repository/ncf/
cd /var/rudder/configuration-repository/ncf/
rm sample_vault.json
git add . && git commit -m "Vault plugin installation"
Copy link
Member

Choose a reason for hiding this comment

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

You should probably mention somewhere in the output that there is a sample_vault.json

#
#####################################################################################

bundle agent vault_fetch_config()
Copy link
Member

Choose a reason for hiding this comment

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

could you document here the format of the vault.json file ?

# @documentation To use the generated variable, you must use the form `${variable_prefix.variable_name}` with each name replaced with the parameters of this method.
#
# Access to the vault has to be configured on each agent in /var/rudder/plugin-resources/vault.json. A sample config file is provided in /opt/rudder/share/plugins/vault/sample_vault.json
#
Copy link
Member

Choose a reason for hiding this comment

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

can you specify the success & error case ? that fetching a key is a repair, failing is an error?


pass3.variable_defined::
"success" usebundle => _classes_repaired("${old_class_prefix}");
"success" usebundle => _classes_repaired("${class_prefix}");
Copy link
Member

Choose a reason for hiding this comment

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

for the sanity of the person doing maintenance in the futur, could you change the promiser to "repair" ?

@victorqrt
Copy link
Contributor Author

Commit modified

@victorqrt victorqrt force-pushed the ust_12865/vault_plugin_add_a_variable_from_vault_generic_method branch from 0fc63ae to f7a08c0 Compare July 13, 2018 12:42
#####################################################################################

# Fetches the configuration to be used for this run.
# The config is located at "config_path". It is a JSON file containing two fields : a server_addr and an auth dictionary.
Copy link
Member

Choose a reason for hiding this comment

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

can you paste the sample, so that it'll be easier for users ?

@victorqrt
Copy link
Contributor Author

Commit modified

@victorqrt victorqrt force-pushed the ust_12865/vault_plugin_add_a_variable_from_vault_generic_method branch from f7a08c0 to f81db13 Compare July 13, 2018 12:54
@victorqrt victorqrt force-pushed the ust_12865/vault_plugin_add_a_variable_from_vault_generic_method branch from f81db13 to 2f0eaa2 Compare July 16, 2018 13:31
@Normation-Quality-Assistant
Copy link
Contributor

This PR breaks qa-test

-- stdout -- 
Your branch is behind 'NRM/branches/rudder/4.3' by 4 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)
Updating 31e02fa..57aefb8
Fast-forward
[...]
-- stderr --
From github.com:VictorQrt/rudder-plugins
 * [new branch]      ust_12865/vault_plugin_add_a_variable_from_vault_generic_method -> ust_12865/vault_plugin_add_a_variable_from_vault_generic_method_pr
From github.com:Normation/rudder-plugins
   31e02fa..57aefb8  branches/rudder/4.3 -> NRM/branches/rudder/4.3
   a2edb83..9a5356d  master              -> NRM/master
Switched to branch 'branches/rudder/4.3'
Switched to a new branch '4.3_test'
No config file found, using default configuration

You should run ./qa-test in your repository to make sure it works.
You can also run rudder-dev merge https://github.com/Normation/rudder-plugins/pull/65 --test to test with upmerging.
After this, you can remove the qa: Can't merge ta
-- Your faithful QA

Copy link
Member

@peckpeck peckpeck left a comment

Choose a reason for hiding this comment

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

Don't merge this

Copy link
Member

@peckpeck peckpeck left a comment

Choose a reason for hiding this comment

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

Ready now

@peckpeck
Copy link
Member

OK, merging this PR

@peckpeck peckpeck merged commit 2f0eaa2 into Normation:branches/rudder/4.3 Jul 26, 2018
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants