Return non-retryable errors on transit encrypt and decrypt failures #13111
Conversation
schultz-is
force-pushed
the
schultz/vault-4153
branch
from
82fe472
to
c5b7092
Compare
| map[string]interface{}{"plaintext": "dGhlIHF1aWNrIGJyb3duIGZveA==", "context": "dGVzdGNvbnRleHQ="}, | ||
| map[string]interface{}{"plaintext": "dGhlIHF1aWNrIGJyb3duIGZveA==", "context": "dGVzdGNvbnRleHQ="}, | ||
| // Encrypt some values for use in test cases. | ||
| plaintextItems := []BatchRequestItem{ |
There was a problem hiding this comment.
Not that it matters a whole lot, but instead of using BatchRequestItem couldn't you simply use an anonymous struct here, as the only fields that really matter for the test are the plaintext an the context?
There was a problem hiding this comment.
Ah, so the reason I did this is mainly for convenience. In order for the encrypt request to work, the batch_input field has to be specifically a []interface{}, which precludes map indexing without type assertions to a map[string]interface{}. I just made this as kind of a source of truth lookup table for easy test definitions. It lets me define test input and output without duplicating the strings inline in each test.
There was a problem hiding this comment.
I agree that using a struct is a good idea. I was suggesting using an anonymous struct rather than BatchRequestItem since the latter has many more fields that are not really relevant to the tests.
Something like:
plaintextItems := []struct{
plaintext, context string
}{
...
There was a problem hiding this comment.
Gotcha, I see what you mean now. I can certainly use an inline struct here to make the the data more directly relevant to tests. Thanks for explaining that!
schultz-is
force-pushed
the
schultz/vault-4153
branch
from
c5b7092
to
d30100e
Compare
|
After a discussion about partial batch error handling, there are some more changes coming with this PR before it is fully ready. Namely, we've decided to return HTTP errors on partial failures with non-retryable HTTP 400's taking precedence over retryable HTTP 500's. Some of the tests will be slightly reworked to accommodate this behavior. |
schultz-is
force-pushed
the
schultz/vault-4153
branch
from
905cc8d
to
7d35b46
Compare
|
Alright, this should be ready for review again. Thanks y'all! |
| map[string]interface{}{"plaintext": "dGhlIHF1aWNrIGJyb3duIGZveA==", "context": "dGVzdGNvbnRleHQ="}, | ||
| map[string]interface{}{"plaintext": "dGhlIHF1aWNrIGJyb3duIGZveA==", "context": "dGVzdGNvbnRleHQ="}, | ||
| // Encrypt some values for use in test cases. | ||
| plaintextItems := []BatchRequestItem{ |
There was a problem hiding this comment.
I agree that using a struct is a good idea. I was suggesting using an anonymous struct rather than BatchRequestItem since the latter has many more fields that are not really relevant to the tests.
Something like:
plaintextItems := []struct{
plaintext, context string
}{
...
7d35b46
to
977aee4
Compare
…plify transit batch decryption test data. Ensure HTTP status codes are expected values on batch transit batch decryption partial failure.
…ually return error HTTP status code on transit batch encryption failure (partial or full).
schultz-is
force-pushed
the
schultz/vault-4153
branch
from
977aee4
to
56ca935
Compare
|
Thanks y'all! |
|
Closes #10842 |
…ashicorp#13111) * Return HTTP 400s on transit decrypt requests where decryption fails. (hashicorp#10842) * Don't abort transit batch encryption when a single batch item fails. * Add unit tests for updated transit batch decryption behavior. * Add changelog entry for transit encrypt/decrypt batch abort fix. * Simplify transit batch error message generation when ciphertext is empty. * Return error HTTP status codes in transit on partial batch decrypt failure. * Return error HTTP status codes in transit on partial batch encrypt failure. * Properly account for non-batch transit decryption failure return. Simplify transit batch decryption test data. Ensure HTTP status codes are expected values on batch transit batch decryption partial failure. * Properly account for non-batch transit encryption failure return. Actually return error HTTP status code on transit batch encryption failure (partial or full).
This also address an issue found with transit encrypt and decrypt paths where a single batch item failure would abort the whole batch and simply return a top-level error.