-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Show a better error message when decrypting fails #1815
Conversation
It is most often the case that failing to decrypt a secret implies that the secret was transferred from one stack to another via copying the configuration. This commit introduces a better error message for this case and instructs users to explicitly re-encrypt their encrypted keys in the context of the new stack.
Obviously it's still not good that the service 500s here, but this at least makes the CLI present the best error message it can with the information it has. |
Gometalinter is teaching me to spell, one failed travis job at at time. |
Why can't we just fix the service? @chrsmith |
I mean, we should absolutely do that too, but it was a CLI bug that the error experience here is as terrible as it is. Even if the service did respond with a reasonable response, I think this PR is still critical in presenting the message in a reasonable way. There's nothing in this PR that assumes that the service isn't capable of producing a reasonable error message here. The reality is that it doesn't until we fix the bug. |
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 - with a suggested error message text tweak.
cmd/errors.go
Outdated
fprintf(writer, ` | ||
This most commonly occurs when using a secret encrypted by a stack and then using the encrypted | ||
configuration in another stack. Configuration encryption is done per-stack and it is not possible | ||
for another stack to decrypt a stack's encrypted configuration. |
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.
Grammar is a little awkward here - perhaps: "This can occur when a secret is copied from one stack to another. Encryption of secrets is done per-stack, and it is not possible to share an encrypted configuration value across stacks."
Great - agreed - we should take the fix in the PR here as well as fix the service. |
configMap[key] = config.NewSecureValue("hunter2") | ||
p := &TestPlan{ | ||
Options: UpdateOptions{host: host}, | ||
Decrypter: brokenDecrypter{ErrorMessage: msg}, |
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.
...this is awesome 🙌
thanks! |
It is most often the case that failing to decrypt a secret implies that
the secret was transferred from one stack to another via copying the
configuration. This commit introduces a better error message for this
case and instructs users to explicitly re-encrypt their encrypted keys
in the context of the new stack.
Fixes #1763.
![screen shot 2018-08-22 at 11 39 22 am](https://user-images.githubusercontent.com/1871912/44483461-093cf200-a600-11e8-958e-5b134f143765.png)