-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Return non-retryable errors on transit encrypt and decrypt failures #13111
Conversation
82fe472
to
c5b7092
Compare
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 added a suggestion but it looks good to me 👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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. |
905cc8d
to
7d35b46
Compare
Alright, this should be ready for review again. Thanks y'all! |
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.
The changes are good but I'm finishing the review with "request changes" as I think the non-batch encrypt/decrypt operations need the check for the right response code.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
977aee4
to
56ca935
Compare
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.
Looks good to me!
Thanks y'all! |
Closes #10842 |
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.