Skip to content

MONGOCRYPT-763 add in-place retry API #1011

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mdb-ad
Copy link
Contributor

@mdb-ad mdb-ad commented May 17, 2025

Add in-place retry API to better support drivers that fan out KMS requests.

@mdb-ad mdb-ad requested a review from a team as a code owner May 17, 2025 23:39
@mdb-ad mdb-ad requested review from a user and kevinAlbs and removed request for a team May 17, 2025 23:39
@@ -1188,6 +1188,23 @@ bool mongocrypt_kms_ctx_feed(mongocrypt_kms_ctx_t *kms, mongocrypt_binary_t *byt
MONGOCRYPT_EXPORT
bool mongocrypt_kms_ctx_fail(mongocrypt_kms_ctx_t *kms);

/**
* Reset a KMS context allowing it to be retried.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment could be expanded a bit regarding the specific situations in which this method should be called and what exactly "reset" means w.r.t. the state of the kms handle?

Comment on lines +498 to +501
// Feed part of a response
ASSERT_OK(mongocrypt_kms_ctx_feed(kms_ctx, TEST_FILE("./test/data/kms-aws/encrypt-response-partial.txt")),
kms_ctx);
// Assume a network error and reset the context.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Feed part of a response
ASSERT_OK(mongocrypt_kms_ctx_feed(kms_ctx, TEST_FILE("./test/data/kms-aws/encrypt-response-partial.txt")),
kms_ctx);
// Assume a network error and reset the context.
// Mark a network error.
ASSERT_OK(mongocrypt_kms_ctx_fail(kms_ctx), kms_ctx);
// Reset the context.

I expect feeding a partial response is not needed. If a driver gets a network error, the protocol expects the driver to call mongocrypt_kms_ctx_fail.

* @return A boolean indicating whether the failed request is complete but should be retried.
*/
MONGOCRYPT_EXPORT
bool mongocrypt_kms_ctx_should_retry_http(mongocrypt_kms_ctx_t *kms);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest removing http from the name and documentation. I expect it may simplify the protocol to always call this function on error to check if a retry is needed. For example:

static bool
handle_kms_with_retry (mongocrypt_kms_ctx_t *kms, _state_machine_t *state_machine, bson_error_t *error)
{
   while (!handle_kms (kms, state_machine, error)) {
      // Error occurred.
      if (mongocrypt_kms_ctx_should_retry (kms)) {
         mongocrypt_kms_ctx_reset (kms);
         continue; // Retry.
      }
      return false; // Return error.
   }
   return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest also documenting the in-place protocol in integrating.md.

Comment on lines +1 to +3
HTTP/1.1 200 OK
x-amzn-RequestId: deeb35e5-4ecb-4bf1-9af5-84a54ff0af0e
Content-Type: appli
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
HTTP/1.1 200 OK
x-amzn-RequestId: deeb35e5-4ecb-4bf1-9af5-84a54ff0af0e
Content-Type: appli
HTTP/1.1 200 OK
x-amzn-RequestId: deeb35e5-4ecb-4bf1-9af5-84a54ff0af0e
Content-Type: application/x-amz-json-1.1
Content-Length: 446
Connection: close
{"KeyId": "arn:aws:k

Suggest including all headers and part of body. Otherwise, this sequence appears to succeed:

ASSERT_OK(mongocrypt_kms_feed(kms, TEST_FILE("./test/data/kms-aws/encrypt-response-partial.txt")), kms);
ASSERT_OK(mongocrypt_kms_feed(kms, TEST_FILE("./test/data/kms-aws/encrypt-response.txt")), kms);

I expect this is due to the extra headers being ignored. With this suggestion, the second call fails.

ASSERT_OK(mongocrypt_kms_ctx_feed(kms_ctx, TEST_FILE("./test/data/rmd/kms-decrypt-reply-429.txt")), kms_ctx);
// In-place retry is indicated.
ASSERT(mongocrypt_kms_ctx_should_retry_http(kms_ctx));
mongocrypt_kms_ctx_reset(kms_ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an error check in mongocrypt_kms_ctx_feed, like:

if (mongocrypt_kms_ctx_should_retry_http(kms)) {
    CLIENT_ERR("Attempting to feed to KMS request requiring retry. Reset first.");
    return false;
}

That may help a driver identify a missing call to mongocrypt_kms_ctx_reset. I expect it may be difficult to diagnose a missing reset call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants