Skip to content
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

DRIVERS-1541 Retry KMS decrypt requests on transient errors #783

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

Conversation

adriandole
Copy link
Contributor

@adriandole adriandole commented Apr 18, 2024

Retry GCP, AWS, and Azure requests that encounter transient errors.
Evergreen testing with the C driver implementation: https://spruce.mongodb.com/version/664bc7d2ebaef90007283d77/

Scope doc: https://docs.google.com/document/d/1JUVC2-Pe8mfpgemHyb4ihsIOzIe_QFQPnYDJV2idVY4/edit

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Leaving quick initial feedback. I suggest git merge master to pull in cb4d4fa. That may resolve many Evergreen task failures. The other task failures (e.g. clang-tidy) may have been introduced in this PR.

src/mongocrypt-kms-ctx.c Outdated Show resolved Hide resolved
src/mongocrypt-kms-ctx.c Outdated Show resolved Hide resolved
src/mongocrypt.h Outdated Show resolved Hide resolved
src/mongocrypt.h Outdated Show resolved Hide resolved
src/mongocrypt.h Outdated
* Retrieve it with @ref mongocrypt_ctx_status
*/
MONGOCRYPT_EXPORT
bool mongocrypt_setopt_retry(mongocrypt_t *crypt, bool enable);
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
bool mongocrypt_setopt_retry(mongocrypt_t *crypt, bool enable);
bool mongocrypt_setopt_retry_kms(mongocrypt_t *crypt, bool enable);

src/mongocrypt-kms-ctx.c Show resolved Hide resolved
@@ -841,6 +893,7 @@ void _mongocrypt_tester_install_ctx_rewrap_many_datakey(_mongocrypt_tester_t *te
INSTALL_TEST(_test_rewrap_many_datakey_init);
INSTALL_TEST(_test_rewrap_many_datakey_need_mongo_keys);
INSTALL_TEST(_test_rewrap_many_datakey_need_kms_decrypt);
INSTALL_TEST(_test_rewrap_many_datakey_need_kms_decrypt_retry);
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 separate tests to isolate retry behavior:

void _test_retry_tcp(_mongocrypt_tester_t *tester) {
    mongocrypt_t *crypt;

    crypt = _mongocrypt_tester_mongocrypt(TESTER_MONGOCRYPT_DEFAULT);

    // Test that an HTTP error is retried.
    {
        mongocrypt_ctx_t *ctx = mongocrypt_ctx_new(crypt);
        ASSERT_OK(mongocrypt_ctx_encrypt_init(ctx, "test", -1, TEST_FILE("./test/example/cmd.json")), ctx);
        _mongocrypt_tester_run_ctx_to(tester, ctx, MONGOCRYPT_CTX_NEED_KMS);
        mongocrypt_kms_ctx_t *kms_ctx = mongocrypt_ctx_next_kms_ctx(ctx);
        ASSERT_OK(kms_ctx, ctx);
        // Expect no sleep is requested before any error.
        ASSERT_CMPINT64(mongocrypt_kms_ctx_usleep(kms_ctx), ==, 0);
        // Feed a retryable HTTP error.
        // TODO: use ASSERT_OK once `kms_ctx` is changed not to set error status.
        ASSERT(mongocrypt_kms_ctx_feed(kms_ctx, TEST_FILE("./test/data/rmd/kms-decrypt-reply-429.txt")));
        // Expect KMS request is returned again for a retry.
        kms_ctx = mongocrypt_ctx_next_kms_ctx(ctx);
        ASSERT_OK(kms_ctx, ctx);
        // Feed a successful response.
        ASSERT_OK(mongocrypt_kms_ctx_feed(kms_ctx, TEST_FILE("./test/data/kms-aws/decrypt-response.txt")), kms_ctx);
        ASSERT_OK(mongocrypt_ctx_kms_done(ctx), ctx);
        _mongocrypt_tester_run_ctx_to(tester, ctx, MONGOCRYPT_CTX_DONE);
        mongocrypt_ctx_destroy(ctx);
    }

    mongocrypt_destroy(crypt); /* recreate crypt because of caching. */
}

@kevinAlbs kevinAlbs self-requested a review May 31, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants