GODRIVER-3168 Retry KMS requests on transient errors. #1887

qingyang-hu · 2024-11-11T15:40:45Z

Summary

Retry KMS requests on transient errors.

Background & Motivation

Changes described here: https://github.com/mongodb/libmongocrypt/blob/master/integrating.md#state-mongocrypt_ctx_need_kms
New tests: https://github.com/mongodb/specifications/blob/master/source/client-side-encryption/tests/README.md#24-kms-retry-tests

mongodb-drivers-pr-bot · 2024-11-11T15:49:37Z

API Change Report

./v2/mongo

incompatible changes

##Connect: changed from func(...*./v2/mongo/options.ClientOptions) (*Client, error) to func(..../v2/mongo/options.Lister[./v2/mongo/options.ClientOptions]) (*Client, error)

./v2/mongo/options

incompatible changes

(*AutoEncryptionOptions).SetBypassAutoEncryption: removed
(*AutoEncryptionOptions).SetBypassQueryAnalysis: removed
(*AutoEncryptionOptions).SetEncryptedFieldsMap: removed
(*AutoEncryptionOptions).SetExtraOptions: removed
(*AutoEncryptionOptions).SetKeyVaultClientOptions: removed
(*AutoEncryptionOptions).SetKeyVaultNamespace: removed
(*AutoEncryptionOptions).SetKmsProviders: removed
(*AutoEncryptionOptions).SetSchemaMap: removed
(*AutoEncryptionOptions).SetTLSConfig: removed
(*ClientOptions).ApplyURI: removed
(*ClientOptions).SetAppName: removed
(*ClientOptions).SetAuth: removed
(*ClientOptions).SetAutoEncryptionOptions: removed
(*ClientOptions).SetBSONOptions: removed
(*ClientOptions).SetCompressors: removed
(*ClientOptions).SetConnectTimeout: removed
(*ClientOptions).SetDialer: removed
(*ClientOptions).SetDirect: removed
(*ClientOptions).SetDisableOCSPEndpointCheck: removed
(*ClientOptions).SetDriverInfo: removed
(*ClientOptions).SetHTTPClient: removed
(*ClientOptions).SetHeartbeatInterval: removed
(*ClientOptions).SetHosts: removed
(*ClientOptions).SetLoadBalanced: removed
(*ClientOptions).SetLocalThreshold: removed
(*ClientOptions).SetLoggerOptions: removed
(*ClientOptions).SetMaxConnIdleTime: removed
(*ClientOptions).SetMaxConnecting: removed
(*ClientOptions).SetMaxPoolSize: removed
(*ClientOptions).SetMinPoolSize: removed
(*ClientOptions).SetMonitor: removed
(*ClientOptions).SetPoolMonitor: removed
(*ClientOptions).SetReadConcern: removed
(*ClientOptions).SetReadPreference: removed
(*ClientOptions).SetRegistry: removed
(*ClientOptions).SetReplicaSet: removed
(*ClientOptions).SetRetryReads: removed
(*ClientOptions).SetRetryWrites: removed
(*ClientOptions).SetSRVMaxHosts: removed
(*ClientOptions).SetSRVServiceName: removed
(*ClientOptions).SetServerAPIOptions: removed
(*ClientOptions).SetServerMonitor: removed
(*ClientOptions).SetServerMonitoringMode: removed
(*ClientOptions).SetServerSelectionTimeout: removed
(*ClientOptions).SetTLSConfig: removed
(*ClientOptions).SetTimeout: removed
(*ClientOptions).SetWriteConcern: removed
(*ClientOptions).SetZlibLevel: removed
(*ClientOptions).SetZstdLevel: removed
(*ClientOptions).Validate: removed
(*DistinctOptionsBuilder).SetHint: removed
(*FindOneOptionsBuilder).SetOplogReplay: removed
(*FindOptionsBuilder).SetOplogReplay: removed
(*LoggerOptions).SetComponentLevel: removed
(*LoggerOptions).SetMaxDocumentLength: removed
(*LoggerOptions).SetSink: removed
(*ServerAPIOptions).SetDeprecationErrors: removed
(*ServerAPIOptions).SetStrict: removed
AutoEncryption: changed from func() *AutoEncryptionOptions to func() *AutoEncryptionOptionsBuilder
AutoEncryptionOptions.KeyVaultClientOptions: changed from *ClientOptions to Lister[ClientOptions]
Client: changed from func() *ClientOptions to func() *ClientOptionsBuilder
ClientOptions.AutoEncryptionOptions: changed from *AutoEncryptionOptions to Lister[AutoEncryptionOptions]
ClientOptions.DriverInfo: removed
ClientOptions.LoggerOptions: changed from *LoggerOptions to Lister[LoggerOptions]
ClientOptions.ServerAPIOptions: changed from *ServerAPIOptions to Lister[ServerAPIOptions]
DistinctOptions.Hint: removed
DriverInfo: removed
FindOneOptions.OplogReplay: removed
FindOptions.OplogReplay: removed
Logger: changed from func() *LoggerOptions to func() *LoggerOptionsBuilder
MergeClientOptions: removed
ServerAPI: changed from func(ServerAPIVersion) *ServerAPIOptions to func(ServerAPIVersion) *ServerAPIOptionsBuilder

compatible changes

AutoEncryptionOptionsBuilder: added
ClientOptionsBuilder: added
LoggerOptionsBuilder: added
ServerAPIOptionsBuilder: added

./v2/x/mongo/driver/auth

incompatible changes

HandshakeOptions.OuterLibraryName: removed
HandshakeOptions.OuterLibraryPlatform: removed
HandshakeOptions.OuterLibraryVersion: removed

./v2/x/mongo/driver/mongocrypt

compatible changes

(*KmsContext).RequestError: added

./v2/x/mongo/driver/operation

incompatible changes

(*Distinct).Hint: removed
(*Hello).OuterLibraryName: removed
(*Hello).OuterLibraryPlatform: removed
(*Hello).OuterLibraryVersion: removed

./v2/x/mongo/driver/topology

incompatible changes

##ConvertToDriverAPIOptions: changed from func(*./v2/mongo/options.ServerAPIOptions) *./v2/x/mongo/driver.ServerAPIOptions to func(./v2/mongo/options.Lister[./v2/mongo/options.ServerAPIOptions]) ./v2/x/mongo/driver.ServerAPIOptions
##NewConfig: changed from func(./v2/mongo/options.ClientOptions, *./v2/x/mongo/driver/session.ClusterClock) (Config, error) to func(./v2/mongo/options.ClientOptionsBuilder, *./v2/x/mongo/driver/session.ClusterClock) (*Config, error)
WithOuterLibraryName: removed
WithOuterLibraryPlatform: removed
WithOuterLibraryVersion: removed

compatible changes

NewConfigFromOptions: added

kevinAlbs · 2024-12-04T21:15:46Z

x/mongo/driver/crypt.go

@@ -399,7 +397,10 @@ func (c *crypt) decryptKey(kmsCtx *mongocrypt.KmsContext) error {

 		res := make([]byte, bytesNeeded)
 		bytesRead, err := conn.Read(res)
-		if err != nil && !errors.Is(err, io.EOF) {
+		if err != nil {
+			if kmsCtx.Fail() {


See MONGOCRYPT-752. When mongocrypt_kms_ctx_fail returns false, consider wrapping the error message from mongocrypt_ctx_kms_status. The error from mongocrypt_ctx_kms_status may help identify a retry occurred (e.g. "KMS request failed after 3 retries due to a network error: last attempt failed with: ")

prestonvasquez · 2025-01-08T23:31:17Z

internal/integration/client_side_encryption_prose_test.go

+		if tlsCAFile := os.Getenv("KMS_FAILPOINT_CA_FILE"); tlsCAFile == "" {
+			require.Fail(mt, "failed to load CA file")
+		} else {
+			var err error
+			clientAndCATlsMap := map[string]interface{}{
+				"tlsCAFile": tlsCAFile,
+			}
+			tlsCfg, err = options.BuildTLSConfig(clientAndCATlsMap)
+			require.Nil(mt, err, "BuildTLSConfig error: %v", err)
+		}


Since require.* stops test execution when a test fails, an if/else here isn't required.

Suggested change

if tlsCAFile := os.Getenv("KMS_FAILPOINT_CA_FILE"); tlsCAFile == "" {

require.Fail(mt, "failed to load CA file")

} else {

var err error

clientAndCATlsMap := map[string]interface{}{

"tlsCAFile": tlsCAFile,

}

tlsCfg, err = options.BuildTLSConfig(clientAndCATlsMap)

require.Nil(mt, err, "BuildTLSConfig error: %v", err)

}

tlsCAFile := os.Getenv("KMS_FAILPOINT_CA_FILE")

require.NotEmpty(mt, tlsCAFile, "failed to load CA file")

tlsCfg, err = options.BuildTLSConfig(map[string]interface{}{"tlsCAFile": tlsCAFile})

require.Nil(mt, err, "BuildTLSConfig error: %v", err)

prestonvasquez

LGTM with minor edit

internal/integration/client_side_encryption_prose_test.go

Co-authored-by: Preston Vasquez <[email protected]>

matthewdale · 2025-01-17T04:06:41Z

.evergreen/config.yml

+    - command: subprocess.exec
+      params:
+        binary: python3
+        background: true
+        args: ["-u", "${DRIVERS_TOOLS}/.evergreen/csfle/kms_failpoint_server.py", "--port", "9003"]


This step is already done by start-servers.sh.

matthewdale · 2025-01-17T04:07:17Z

.evergreen/config.yml

@@ -553,6 +553,39 @@ functions:
          KMS_MOCK_SERVERS_RUNNING: "true"
        args: [*task-runner, evg-test-kmip]

+  start-kms-failpoint-server:


This function seems like mostly a duplicate of start-cse-servers. Can we use that existing function instead of this one?

matthewdale · 2025-01-17T04:12:57Z

internal/integration/client_side_encryption_prose_test.go

@@ -2979,6 +2981,147 @@ func TestClientSideEncryptionProse(t *testing.T) {
 			assert.Greater(t, len(payload.Data), len(payloadDefaults.Data), "the returned payload size is expected to be greater than %d", len(payloadDefaults.Data))
 		})
 	})
+
+	mt.RunOpts("24. kms retry tests", noClientOpts, func(mt *mtest.T) {


Optional: Consider separating this into its own test function.

matthewdale · 2025-01-17T04:14:27Z

.evergreen/config.yml

+  - matrix_name: "retry-kms-requests-test"
+    matrix_spec: { version: ["7.0"], os-ssl-40: ["rhel87-64"] }
+    display_name: "Retry KMS Requests ${os-ssl-40}"
+    tasks:
+      - name: ".retry-kms-requests"


Instead of making a new matrix, can we roll this up into a general "KMS Test" that contains all ".kms-tls" and ".retry-kms-requests" tests?

mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Nov 11, 2024

qingyang-hu force-pushed the godriver3168 branch 5 times, most recently from b11c2a5 to 93ebd31 Compare November 11, 2024 19:25

GODRIVER-3168 Retry KMS requests on transient errors.

02a60a5

qingyang-hu force-pushed the godriver3168 branch 6 times, most recently from 4d94342 to fd682b7 Compare November 13, 2024 16:38

add task

12a1530

qingyang-hu force-pushed the godriver3168 branch 4 times, most recently from 6ecb7c2 to ddc0900 Compare November 20, 2024 17:02

qingyang-hu force-pushed the godriver3168 branch 5 times, most recently from 77cf4a1 to d4a0765 Compare November 20, 2024 21:49

update NeedKms logic

00e69f5

qingyang-hu force-pushed the godriver3168 branch 5 times, most recently from 074d596 to 50549f6 Compare November 25, 2024 23:59

qingyang-hu force-pushed the godriver3168 branch 12 times, most recently from 58a6a49 to db03cdf Compare December 2, 2024 21:43

fix tests

701bd46

qingyang-hu force-pushed the godriver3168 branch from db03cdf to 701bd46 Compare December 2, 2024 22:22

qingyang-hu marked this pull request as ready for review December 2, 2024 22:41

mongodb-drivers-pr-bot bot requested a review from prestonvasquez December 2, 2024 22:47

qingyang-hu requested a review from matthewdale December 2, 2024 22:47

kevinAlbs reviewed Dec 4, 2024

View reviewed changes

qingyang-hu marked this pull request as draft December 4, 2024 21:17

qingyang-hu marked this pull request as ready for review December 5, 2024 21:11

return a wrapped error

614aba5

qingyang-hu force-pushed the godriver3168 branch from 4c20d77 to 614aba5 Compare December 5, 2024 21:16

prestonvasquez requested changes Jan 14, 2025

View reviewed changes

qingyang-hu added 2 commits January 14, 2025 14:59

Clarify test logic.

893731f

Clarify test logic.

f567dbf

qingyang-hu requested a review from prestonvasquez January 14, 2025 21:50

prestonvasquez reviewed Jan 15, 2025

View reviewed changes

internal/integration/client_side_encryption_prose_test.go Outdated Show resolved Hide resolved

Update internal/integration/client_side_encryption_prose_test.go

12650cd

Co-authored-by: Preston Vasquez <[email protected]>

prestonvasquez approved these changes Jan 15, 2025

View reviewed changes

matthewdale reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-3168 Retry KMS requests on transient errors. #1887

GODRIVER-3168 Retry KMS requests on transient errors. #1887

qingyang-hu commented Nov 11, 2024 •

edited

Loading

mongodb-drivers-pr-bot bot commented Nov 11, 2024 •

edited

Loading

kevinAlbs Dec 4, 2024

prestonvasquez Jan 8, 2025

prestonvasquez left a comment

matthewdale Jan 17, 2025

matthewdale Jan 17, 2025

matthewdale Jan 17, 2025

matthewdale Jan 17, 2025

GODRIVER-3168 Retry KMS requests on transient errors. #1887

Are you sure you want to change the base?

GODRIVER-3168 Retry KMS requests on transient errors. #1887

Conversation

qingyang-hu commented Nov 11, 2024 • edited Loading

Summary

Background & Motivation

mongodb-drivers-pr-bot bot commented Nov 11, 2024 • edited Loading

API Change Report

./v2/mongo

incompatible changes

./v2/mongo/options

incompatible changes

compatible changes

./v2/x/mongo/driver/auth

incompatible changes

./v2/x/mongo/driver/mongocrypt

compatible changes

./v2/x/mongo/driver/operation

incompatible changes

./v2/x/mongo/driver/topology

incompatible changes

compatible changes

kevinAlbs Dec 4, 2024

Choose a reason for hiding this comment

prestonvasquez Jan 8, 2025

Choose a reason for hiding this comment

prestonvasquez left a comment

Choose a reason for hiding this comment

matthewdale Jan 17, 2025

Choose a reason for hiding this comment

matthewdale Jan 17, 2025

Choose a reason for hiding this comment

matthewdale Jan 17, 2025

Choose a reason for hiding this comment

matthewdale Jan 17, 2025

Choose a reason for hiding this comment

qingyang-hu commented Nov 11, 2024 •

edited

Loading

mongodb-drivers-pr-bot bot commented Nov 11, 2024 •

edited

Loading