-
Notifications
You must be signed in to change notification settings - Fork 47
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
Include rotated secrets for cloud vault secrets VAULT-22307 #850
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, nothing is blocking, LGTM!
getParams := secret_service.NewOpenAppSecretParamsWithContext(ctx). | ||
WithAppName(appName). | ||
WithSecretName(secretName). | ||
WithOrganizationID(loc.OrganizationID). | ||
WithProjectID(loc.ProjectID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
@@ -106,8 +106,20 @@ func (d *DataSourceVaultSecretsApp) Read(ctx context.Context, req datasource.Rea | |||
|
|||
openAppSecrets := map[string]string{} | |||
for _, appSecret := range appSecrets { | |||
secretName := appSecret.Name | |||
openAppSecrets[secretName] = appSecret.Version.Value | |||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be cleaner to switch on secret.Type instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaner: yes
was concerned about the string type, are we going to switch to enum at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe 🤷 It's OK to leave as is for now
internal/provider/vaultsecrets/data_source_vault_secrets_secret.go
Outdated
Show resolved
Hide resolved
// NOTE: for backwards compatibility purposes, if the secret is not a static secret (aka a string) | ||
// encode the complex secret as a json string | ||
var secretValue string | ||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, switching on secret.Type might be cleaner
resp.Diagnostics.AddWarning( | ||
"HCP Vault Secrets mismatched type", | ||
"Attempted to get a rotating secret in a KV secret data source, encoding the secret values as JSON", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit unsure about this warning. We may want to keep this data source as an equivalent of vault_generic_secret data source. In this case, it should be perfectly valid to use it to fetch a rotating secret.
I think it's OK to leave it here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets keep, was in the original discussion when we planned this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🛠️ Description
Allows users to fetch rotating secrets using the
hcp_vault_secrets_app
andhcp_vault_secrets_secret
data sourceshcp_vault_secrets_app
hcp_vault_secrets_app
: keys from complex (rotating) secrets are concatenated in the same pattern we've been usingGiven these secrets
with
gives (secret values print out to stdout successfully, am not including here)
hcp_vault_secrets_secret
For backwards compatibility in the
hcp_vault_secrets_secret
, any rotating secret will be JSON encodedusing the same secrets setup above in the portal
gives
Because the user may not expect a json payload we emit a warning
🏗️ Acceptance tests
Output from acceptance testing:
$ make testacc TESTARGS='-run=TestAccXXX' ...