Merge carlpett/salt-vault #39212
Merge carlpett/salt-vault #39212
Conversation
|
@carlpett This won't merge cleanly. Could you please rebase it? |
|
@carlpett it looks like you removed some references to the master, and SDB is definitely supposed to work on the master too. |
b87cc13 to
3a2749e
Compare
|
@cachedout Ah, missed that there had been an documentation update after I forked. Rebased now. @techhat Hm, I don't understand what you mean? Did I break some use case? |
|
@carlpett Thanks for the rebase. There are two lint errors which also need to be resolved: https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/8371/violations/file/salt/utils/vault.py/ |
3a2749e to
e5ecd8e
Compare
|
@cachedout Fixed those, one with a |
|
If you think it's in the interest of readability, I have no objection at all to the suppress statement. Thanks! |
|
Great! Ok, so as soon as I've understood and fixed @techhat's comment this is good to go, then? |
| raise salt.exceptions.CommandExecutionError(e) | ||
|
|
||
|
|
||
| def write_secret(path, **kwargs): |
There was a problem hiding this comment.
We do not like to have **kwargs accepted in execution modules because we like to document the possible arguments for the user. Is it possible to use explicit keyword arguments instead or is it the number of possible keywords too broad?
There was a problem hiding this comment.
The kwargs are actually used to input the key/values to store in Vault, so they aren't actually arguments to the module itself. It just bundles them up into a dict and sends them on. For example, salt-call vault.write_secret "secret/my/secret" user="foo" password="bar". If I'd use vault.read_secret on the same path, I'd get back a dict {'user':'foo', 'password':'bar'}.
This could be changed to requiring a dict directly, e.g. salt-call vault.write_secret "secret/my/secret" data='{"user":"foo", "password":"bar"}, but it makes it somewhat clunkier, IMO.
There was a problem hiding this comment.
That's fair. We do have a few places where this happens as well but we try to keep them as minimal as possible. Thanks for the explanation.
What does this PR do?
This merges the repo carlpett/salt-vault which contains a separate implementation of Vault integration that was developed concurrently by @techhat with the existing implementation.
The approaches taken in the two implementations are slightly different, but old configuration should still be compatible with the merged code (albeit with deprecation warnings).
Note that this PR has an implicit dependency on #39174 for the sdb module to work, so it cannot be merged before that PR.
Since this is my first larger PR to salt: have I missed something? The linting mostly passes, for instance, do I need to suppress all the things I consider false positives, or are they mainly advisory?
Have I formatted the documentation correctly?
What issues does this PR fix or reference?
#27020
New Behavior
Vault policies are enforced when reading secrets. Supports state management of policies, and an execution module for CRUD operations.
Tests written?
No