Ensure Passphrase Secret Manager is available to use with new config #5110
Conversation
Fixes: #5104 This reverts a code change that was checking initially for the SecretsProvider of the currentStack being an empty string. When it was an empty string, we were checking the backend type and we were setting a serviceSecretsManager. This wasn't correct, the logic needed to check for an empty SecretsProvider AND an empty EncryptionSalt The important part is that it needed to check for the EncryptionSalt to make sure it wasn't a passphrase secrets manger
stack72
force-pushed
the
fixup-passphrase-secrets-provider
branch
from
3fc687a
to
ca18028
Compare
| @@ -70,6 +64,8 @@ func getStackSecretsManager(s backend.Stack) (secrets.Manager, error) { | |||
| switch s.(type) { | |||
| case filestate.Stack: | |||
| return newPassphraseSecretsManager(s.Ref().Name(), stackConfigFile) | |||
| case httpstate.Stack: | |||
There was a problem hiding this comment.
I think it's okay (a no-op) to keep the check for ps.EncryptionSalt != "" above this case - but the overall structure of this code is a little hard to follow. There are really three cases here - is it possible to structure this to make the checks for each case clearer? For example - is the line above this (return newPassphraseSecretsManager) even reachable currently?
There was a problem hiding this comment.
It is reachable when the code has been called as part of creating a new stack - it has no secretsManager at that point and therefore will have an empty secretsProvider AND an empty EncryptionSalt as they have not been added to the config file
There was a problem hiding this comment.
FWIW, this returns the code in this specific block back to that of what it was before
There was a problem hiding this comment.
FWIW, this returns the code in this specific block back to that of what it was before
Okay - great.
It is reachable when the code has been called as part of creating a new stack - it has no secretsManager at that point and therefore will have an empty secretsProvider AND an empty EncryptionSalt as they have not been added to the config file
I see - definitely would be helpful to note that in a comment here - though recognize this is an issue with the code as it was before you touched it.
There was a problem hiding this comment.
@lukehoban so we have 2 options here:
-
We work out the test coverage now and risk causing more potential issues for passphrase secrets provider users
-
We ship the patch (as this code is now back to what it was before) and we then work out the testing of the code
Thoughts?
There was a problem hiding this comment.
Definitely okay with fixing the serious user-facing bug ASAP - but would love to invest in test coverage here as well as a follow-up.
Fixes: #5104
This reverts a code change that was checking initially for the
SecretsProvider of the currentStack being an empty string. When it
was an empty string, we were checking the backend type and we were
setting a serviceSecretsManager. This wasn't correct, the logic
needed to check for an empty SecretsProvider AND an empty EncryptionSalt
The important part is that it needed to check for the EncryptionSalt
to make sure it wasn't a passphrase secrets manger
I am 99% confident that this bug only affected those users who interacted directly with the configuration of the stack from the CLI i.e ran a command
pulumi config x