Enable support for Vault secrets in Manifester #38
Conversation
A CI test failure in SatelliteQE/robottelo#14633 revealed that Manifester was unable to interpolate secrets stored in HashiCorp Vault. This PR adds support for Vault secrets based on Robottelo's implementation of Vault integration.
manifester/commands.py
Outdated
| @@ -60,8 +60,13 @@ def delete(allocations, all_, remove_manifest_file): | |||
| uuid=allocation.get("uuid") | |||
| ) | |||
| if remove_manifest_file: | |||
| manifester_directory = ( | |||
| Path(os.environ["MANIFESTER_DIRECTORY"]) | |||
There was a problem hiding this comment.
Might want to slap on a resolve here
| Path(os.environ["MANIFESTER_DIRECTORY"]) | |
| Path(os.environ["MANIFESTER_DIRECTORY"]).resolve() |
| manifester_directory = Path() | ||
|
|
||
| if "MANIFESTER_DIRECTORY" in os.environ: | ||
| envar_location = Path(os.environ["MANIFESTER_DIRECTORY"]) | ||
| if envar_location.is_dir(): | ||
| manifester_directory = envar_location | ||
| self.env_path = manifester_directory.joinpath(env_file) |
There was a problem hiding this comment.
These would likely be best done in a common place, like the settings module
There was a problem hiding this comment.
@JacobCallahan This actually is done in the settings module, but I revisited this today and remembered why I had repeated it here: vault_login.py throws an error when trying to retrieve settings values if the user is not already logged in to Vault. If I'm understanding the situation correctly, vault_login.py imports helpers.py, in which it encounters an assignment that uses settings.get(), then dynaconf attempts to process the settings file, which includes a Vault-formatted setting (offline_token), and dynaconf subsequently tries to retrieve that value from Vault before the login process has completed. This is also why I had hard-coded the log level in this module, which you commented on as well.
There may be another way to work around this race condition, but I have not figured one out yet. Do you have any thoughts on an alternative, or alternatively, do you think my admittedly inelegant solution is acceptable?
There was a problem hiding this comment.
Hmmm not ideal, but if it needs to be repeated anyway for the vault login then it's up to you whether that repetition happens here or in the vault script. I have no objections either way now.
manifester/settings.py
Outdated
| @@ -2,7 +2,7 @@ | |||
| import os | |||
| from pathlib import Path | |||
|
|
|||
| from dynaconf import Dynaconf, Validator | |||
| from dynaconf import LazySettings, Validator | |||
There was a problem hiding this comment.
This change was part of my initial debugging. In trying to determine why my Vault integration was failing, I tried matching the Robottelo implementation as closely as possible. I can revert this.
There was a problem hiding this comment.
Yeah they're equivalent statements since Dynaconf is an alias to LazySettings. However, it's recommended to use Dynaconf here instead. Robottelo is likely just holding on to an old pattern (and should be changed).
https://github.com/dynaconf/dynaconf/blob/master/dynaconf/__init__.py#L28
| from manifester.settings import settings | ||
|
|
||
| setup_logzero(level="info") |
There was a problem hiding this comment.
If switching to using the settings module, you can just use the configured log level
scripts/vault_login.py
Outdated
There was a problem hiding this comment.
Looking at https://hvac.readthedocs.io/en/stable/usage/auth_methods/jwt-oidc.html#oidc-authorization-url-request, it should be possible to use pure Python implementation, using hvac, of vault login without needing to install vault CLI tool. It is something I have wanted to explore some time ago, but I never found time for it.
If you would like to dig into it, I would be happy to assist/help you.
There was a problem hiding this comment.
I'm definitely interested in this as a possibility. I'll miss the next Automation Brainstorming meeting, but maybe we can discuss this at the following one.
|
@synkd What if we would have :
Hence we don't need to rewrite the code in manifester for vault integration. Manifester being a standalone tool and since we are the only users of it, does not need to store/access the secrets from vault for now and its just a one secret. |
|
@jyejare I have been trying to avoid having Manifester depend directly on Robottelo. Although SatQE is currently the only group using Manifester, I think it has potential applications elsewhere as well. However, I'm open to discussion on the topic. Maybe we can discuss this at the Automation Brainstorming meeting in two weeks, along with Ondrej's suggestion above? |
synkd
force-pushed
the
integrate_vault_with_manifester
branch
from
8eac938 to
45588d7
Compare
A CI test failure in SatelliteQE/robottelo#14633 revealed that Manifester was unable to interpolate secrets stored in HashiCorp Vault. This PR adds support for Vault secrets based on Robottelo's implementation of Vault integration.