S154 secrets updated too much #451
Conversation
…-secrets_updated_too_much
| @@ -160,7 +143,7 @@ module "api_connector" { | |||
|
|
|||
| secret_bindings = merge( | |||
| # bc some of these are later filled directly, bind to 'latest' | |||
| { for k, v in module.secrets[each.key].secret_bindings : k => merge(v, { version_number : "latest" }) }, | |||
There was a problem hiding this comment.
Here all secrets created were added as part of security bindings, being available as it were env vars
| @@ -248,15 +238,17 @@ private AccessToken refreshAccessToken(int attempt) throws IOException { | |||
| } | |||
There was a problem hiding this comment.
I think you have this from cherry-picking my commit - so this still has bug @jlorper caught w #449 - and will conflict as that has been merged to rc-v0.4.31 already; please be mindful to preserve it when you update this branch and resolve the conflicts OR start a clean branch w/o cherry-picking this
| @@ -160,7 +143,7 @@ module "api_connector" { | |||
|
|
|||
| secret_bindings = merge( | |||
| # bc some of these are later filled directly, bind to 'latest' | |||
| { for k, v in module.secrets[each.key].secret_bindings : k => merge(v, { version_number : "latest" }) }, | |||
| {for k, secrets in local.secrets_to_provision[each.key] : k => merge({ secret_id : module.secrets[each.key].secret_ids_within_project[k], version_number : "latest" }) if (try(!secrets.lockable, true) && try(!secrets.writable, true))}, | |||
There was a problem hiding this comment.
yeah, I agree with this - good change, if not very readable to do it this way 😓 - it's correct via the interface we've defined.
|
|
||
| writable_secrets = flatten([ |
There was a problem hiding this comment.
I think writable is fine; in practice, lockable should be subset of writable.
| versionName = "latest"; | ||
| } | ||
|
|
||
| log.severe("Reading secret version: " + versionName); |
| .build()) | ||
| .build()); | ||
| } catch (Exception e) { | ||
| log.severe(String.format("Cannot put the label of the version on the secret %s due exception: %s", secretName.toString(), e)); |
There was a problem hiding this comment.
don't serialize exception like this; leverage log.log(Level.SEVERE, ..., e) to get it via the native support for that.
There was a problem hiding this comment.
also, imho you shouldn't catch this here, or should at least re-throw. you're supressing it, so caller won't detect failure and will just continue. That's probably OK in the version labeling case, but not in the lock case - it will believe lock was acquired, right?
| } | ||
| }); | ||
| } catch (Exception e) { | ||
| log.severe(String.format("Cannot disable old versions from secret %s due exception: %s", secretName.toString(), e)); |
There was a problem hiding this comment.
i think leave suppressing the exception to the caller
java/impl/gcp/src/main/java/co/worklytics/psoxy/SecretManagerConfigService.java
Show resolved
Hide resolved
| } catch (Exception e) { | ||
| log.log(Level.SEVERE, String.format("Cannot disable old versions from secret %s", secretName.toString()), e); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
again, disabling the secrets is a secondary action that in my option shouldn't jeopardize the main one putConfigProperty
access errors like not enough permissions should fail as it clearly states misconfiguration, but transient errors should be ignored. Next run will attempt to disable the secrets again.
| @@ -160,7 +143,7 @@ module "api_connector" { | |||
|
|
|||
| secret_bindings = merge( | |||
| # bc some of these are later filled directly, bind to 'latest' | |||
| { for k, v in module.secrets[each.key].secret_bindings : k => merge(v, { version_number : "latest" }) }, | |||
| {for k, secrets in local.secrets_to_provision[each.key] : k => merge({ secret_id : module.secrets[each.key].secret_ids_within_project[k], version_number : "latest" }) if (try(!secrets.lockable, true) && try(!secrets.writable, true))}, | |||
There was a problem hiding this comment.
I think the if could be
if !contains(keys(local.writable_secrets), secret_id), more succinct
NOTE #449 has been merged to my branch.
Only for GCP.
latestSecretManagerConfigService, adding support of update the label everytime a new version is added.Fixes
GCP: More than one new token version when refreshing
Change implications
CHANGELOG.md?terraform plan/applythat isn't obviously a no-op? noalpha, requires major version change no