From 10a0db1baa0f274d051507d94dde99fc22299ad5 Mon Sep 17 00:00:00 2001 From: Steven Silvester Date: Tue, 22 Aug 2023 12:45:21 -0500 Subject: [PATCH 1/6] GODRIVER-2951 Improper handling of env vars in Load Balancer Test (#1361) --- .evergreen/config.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.evergreen/config.yml b/.evergreen/config.yml index 4171d6f5c3..db6b51f680 100644 --- a/.evergreen/config.yml +++ b/.evergreen/config.yml @@ -1696,6 +1696,9 @@ tasks: LOAD_BALANCER: "true" - func: run-load-balancer - func: run-load-balancer-tests + vars: + AUTH: "noauth" + SSL: "nossl" - name: test-load-balancer-auth-ssl tags: ["load-balancer"] @@ -1708,6 +1711,9 @@ tasks: LOAD_BALANCER: "true" - func: run-load-balancer - func: run-load-balancer-tests + vars: + AUTH: "auth" + SSL: "ssl" - name: test-race tags: ["race"] From 99d53d6348c0e7dce87255dce18b1f6660717dec Mon Sep 17 00:00:00 2001 From: Steven Silvester Date: Tue, 22 Aug 2023 16:04:57 -0500 Subject: [PATCH 2/6] GODRIVER-2923 Run workflow in an environment (#1360) --- .github/workflows/comment.yml | 41 +++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 .github/workflows/comment.yml diff --git a/.github/workflows/comment.yml b/.github/workflows/comment.yml new file mode 100644 index 0000000000..3d96238c9c --- /dev/null +++ b/.github/workflows/comment.yml @@ -0,0 +1,41 @@ +name: PR API Report +on: + pull_request_target: + +jobs: + comment: + runs-on: ubuntu-latest + permissions: + pull-requests: write + issues: write + environment: + api-report + steps: + - uses: actions/setup-go@v4 + - name: Find Comment + uses: peter-evans/find-comment@v2 + id: fc + with: + issue-number: ${{ github.event.pull_request.number }} + comment-author: 'github-actions[bot]' + body-includes: "## API Change Report" + + - name: Create the comment body + run: | + set -eux + git clone https://github.com/mongodb/mongo-go-driver + cd mongo-go-driver + git remote add source https://github.com/$GITHUB_ACTOR/mongo-go-driver + git fetch origin $GITHUB_BASE_REF + git fetch source $GITHUB_HEAD_REF + git checkout $GITHUB_HEAD_REF + make api-report + cat api-report.md + + - name: Create or update comment + uses: peter-evans/create-or-update-comment@v3 + with: + comment-id: ${{ steps.fc.outputs.comment-id }} + issue-number: ${{ github.event.pull_request.number }} + body-path: 'mongo-go-driver/api-report.md' + edit-mode: replace \ No newline at end of file From 67f257ac1d970452575fa24d7394e1ed46d1d725 Mon Sep 17 00:00:00 2001 From: Matt Dale <9760375+matthewdale@users.noreply.github.com> Date: Wed, 23 Aug 2023 18:33:26 -0700 Subject: [PATCH 3/6] GODRIVER-2852 Use a mock connection for all CMAP spec tests. (#1321) --- x/mongo/driver/topology/CMAP_spec_test.go | 57 ++++++++++------------- 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/x/mongo/driver/topology/CMAP_spec_test.go b/x/mongo/driver/topology/CMAP_spec_test.go index 44ccb8841a..d752af71e2 100644 --- a/x/mongo/driver/topology/CMAP_spec_test.go +++ b/x/mongo/driver/topology/CMAP_spec_test.go @@ -22,7 +22,6 @@ import ( "go.mongodb.org/mongo-driver/event" "go.mongodb.org/mongo-driver/internal/require" "go.mongodb.org/mongo-driver/internal/spectest" - "go.mongodb.org/mongo-driver/mongo/address" "go.mongodb.org/mongo-driver/x/mongo/driver/operation" ) @@ -59,10 +58,6 @@ var skippedTestDescriptions = map[string]string{ // that request a new connection cannot be satisfied by a check-in. // TODO(DRIVERS-2223): Re-enable this test once the spec test is updated to support the Go pool check-in behavior. "threads blocked by maxConnecting check out returned connections": "test requires a checked-in connections cannot satisfy a check-out waiting on a new connection (DRIVERS-2223)", - // TODO(GODRIVER-2852): Fix and unskip this test case. - "must be able to start a pool with minPoolSize connections": "test fails frequently, skipping; see GODRIVER-2852", - // TODO(GODRIVER-2852): Fix and unskip this test case. - "pool clear halts background minPoolSize establishments": "test fails frequently, skipping; see GODRIVER-2852", } type cmapEvent struct { @@ -142,9 +137,6 @@ func runCMAPTest(t *testing.T, testFileName string) { t.Skip(msg) } - l, err := net.Listen("tcp", "localhost:0") - require.NoError(t, err, "unable to create listener") - testInfo := &testInfo{ objects: make(map[string]interface{}), originalEventChan: make(chan *event.PoolEvent, 200), @@ -177,43 +169,44 @@ func runCMAPTest(t *testing.T, testFileName string) { }), } - // If there's a failpoint configured in the test, use a dialer that returns connections that - // mock the configured failpoint. If "blockConnection" is true and "blockTimeMS" is specified, - // use a mock connection that delays reads by the configured amount. If "closeConnection" is - // true, close the connection so it always returns an error on read and write. + var delay time.Duration + var closeConnection bool + if test.FailPoint != nil { data, ok := test.FailPoint["data"].(map[string]interface{}) if !ok { t.Fatalf("expected to find \"data\" map in failPoint (%v)", test.FailPoint) } - var delay time.Duration blockConnection, _ := data["blockConnection"].(bool) if blockTimeMS, ok := data["blockTimeMS"].(float64); ok && blockConnection { delay = time.Duration(blockTimeMS) * time.Millisecond } - closeConnection, _ := data["closeConnection"].(bool) - - sOpts = append(sOpts, WithConnectionOptions(func(...ConnectionOption) []ConnectionOption { - return []ConnectionOption{ - WithDialer(func(Dialer) Dialer { - return DialerFunc(func(_ context.Context, _, _ string) (net.Conn, error) { - msc := newMockSlowConn(makeHelloReply(), delay) - if closeConnection { - msc.Close() - } - return msc, nil - }) - }), - WithHandshaker(func(h Handshaker) Handshaker { - return operation.NewHello() - }), - } - })) + closeConnection, _ = data["closeConnection"].(bool) } - s := NewServer(address.Address(l.Addr().String()), primitive.NewObjectID(), sOpts...) + // Use a dialer that returns mock connections that always respond with a + // "hello" reply. If there's a failpoint configured in the test, use a + // dialer that returns connections that mock the configured failpoint. + sOpts = append(sOpts, WithConnectionOptions(func(...ConnectionOption) []ConnectionOption { + return []ConnectionOption{ + WithDialer(func(Dialer) Dialer { + return DialerFunc(func(_ context.Context, _, _ string) (net.Conn, error) { + msc := newMockSlowConn(makeHelloReply(), delay) + if closeConnection { + msc.Close() + } + return msc, nil + }) + }), + WithHandshaker(func(h Handshaker) Handshaker { + return operation.NewHello() + }), + } + })) + + s := NewServer("mongodb://fake", primitive.NewObjectID(), sOpts...) s.state = serverConnected require.NoError(t, err, "error connecting connection pool") defer s.pool.close(context.Background()) From d71481e770066f86bf7ee4fb5baf0b1bae7d1a4d Mon Sep 17 00:00:00 2001 From: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com> Date: Tue, 29 Aug 2023 18:07:18 -0400 Subject: [PATCH 4/6] GODRIVER-2423 Include pinned connections in WaitQueueTimeoutError only for load balanced clusters. (#1353) --- x/mongo/driver/topology/errors.go | 38 ++++++++++++++++++++----------- x/mongo/driver/topology/pool.go | 25 +++++++++++++++----- x/mongo/driver/topology/server.go | 1 + 3 files changed, 45 insertions(+), 19 deletions(-) diff --git a/x/mongo/driver/topology/errors.go b/x/mongo/driver/topology/errors.go index 4f7b485405..7ce41864e6 100644 --- a/x/mongo/driver/topology/errors.go +++ b/x/mongo/driver/topology/errors.go @@ -9,6 +9,7 @@ package topology import ( "context" "fmt" + "time" "go.mongodb.org/mongo-driver/mongo/description" ) @@ -69,11 +70,17 @@ func (e ServerSelectionError) Unwrap() error { // WaitQueueTimeoutError represents a timeout when requesting a connection from the pool type WaitQueueTimeoutError struct { - Wrapped error - PinnedCursorConnections uint64 - PinnedTransactionConnections uint64 - maxPoolSize uint64 - totalConnectionCount int + Wrapped error + pinnedConnections *pinnedConnections + maxPoolSize uint64 + totalConnections int + availableConnections int + waitDuration time.Duration +} + +type pinnedConnections struct { + cursorConnections uint64 + transactionConnections uint64 } // Error implements the error interface. @@ -95,14 +102,19 @@ func (w WaitQueueTimeoutError) Error() string { ) } - return fmt.Sprintf( - "%s; maxPoolSize: %d, connections in use by cursors: %d"+ - ", connections in use by transactions: %d, connections in use by other operations: %d", - errorMsg, - w.maxPoolSize, - w.PinnedCursorConnections, - w.PinnedTransactionConnections, - uint64(w.totalConnectionCount)-w.PinnedCursorConnections-w.PinnedTransactionConnections) + msg := fmt.Sprintf("%s; total connections: %d, maxPoolSize: %d, ", errorMsg, w.totalConnections, w.maxPoolSize) + if pinnedConnections := w.pinnedConnections; pinnedConnections != nil { + openConnectionCount := uint64(w.totalConnections) - + pinnedConnections.cursorConnections - + pinnedConnections.transactionConnections + msg += fmt.Sprintf("connections in use by cursors: %d, connections in use by transactions: %d, connections in use by other operations: %d, ", + pinnedConnections.cursorConnections, + pinnedConnections.transactionConnections, + openConnectionCount, + ) + } + msg += fmt.Sprintf("idle connections: %d, wait duration: %s", w.availableConnections, w.waitDuration.String()) + return msg } // Unwrap returns the underlying error. diff --git a/x/mongo/driver/topology/pool.go b/x/mongo/driver/topology/pool.go index 5d2369352e..6e150344db 100644 --- a/x/mongo/driver/topology/pool.go +++ b/x/mongo/driver/topology/pool.go @@ -74,6 +74,7 @@ type poolConfig struct { MaxConnecting uint64 MaxIdleTime time.Duration MaintainInterval time.Duration + LoadBalanced bool PoolMonitor *event.PoolMonitor Logger *logger.Logger handshakeErrFn func(error, uint64, *primitive.ObjectID) @@ -93,6 +94,7 @@ type pool struct { minSize uint64 maxSize uint64 maxConnecting uint64 + loadBalanced bool monitor *event.PoolMonitor logger *logger.Logger @@ -206,6 +208,7 @@ func newPool(config poolConfig, connOpts ...ConnectionOption) *pool { minSize: config.MinPoolSize, maxSize: config.MaxPoolSize, maxConnecting: maxConnecting, + loadBalanced: config.LoadBalanced, monitor: config.PoolMonitor, logger: config.Logger, handshakeErrFn: config.handshakeErrFn, @@ -574,6 +577,7 @@ func (p *pool) checkOut(ctx context.Context) (conn *connection, err error) { p.stateMu.RUnlock() // Wait for either the wantConn to be ready or for the Context to time out. + start := time.Now() select { case <-w.ready: if w.err != nil { @@ -615,6 +619,8 @@ func (p *pool) checkOut(ctx context.Context) (conn *connection, err error) { } return w.conn, nil case <-ctx.Done(): + duration := time.Since(start) + if mustLogPoolMessage(p) { keysAndValues := logger.KeyValues{ logger.KeyReason, logger.ReasonConnCheckoutFailedTimout, @@ -632,13 +638,20 @@ func (p *pool) checkOut(ctx context.Context) (conn *connection, err error) { }) } - return nil, WaitQueueTimeoutError{ - Wrapped: ctx.Err(), - PinnedCursorConnections: atomic.LoadUint64(&p.pinnedCursorConnections), - PinnedTransactionConnections: atomic.LoadUint64(&p.pinnedTransactionConnections), - maxPoolSize: p.maxSize, - totalConnectionCount: p.totalConnectionCount(), + err := WaitQueueTimeoutError{ + Wrapped: ctx.Err(), + maxPoolSize: p.maxSize, + totalConnections: p.totalConnectionCount(), + availableConnections: p.availableConnectionCount(), + waitDuration: duration, + } + if p.loadBalanced { + err.pinnedConnections = &pinnedConnections{ + cursorConnections: atomic.LoadUint64(&p.pinnedCursorConnections), + transactionConnections: atomic.LoadUint64(&p.pinnedTransactionConnections), + } } + return nil, err } } diff --git a/x/mongo/driver/topology/server.go b/x/mongo/driver/topology/server.go index 600797df40..88b93b15e6 100644 --- a/x/mongo/driver/topology/server.go +++ b/x/mongo/driver/topology/server.go @@ -177,6 +177,7 @@ func NewServer(addr address.Address, topologyID primitive.ObjectID, opts ...Serv MaxConnecting: cfg.maxConnecting, MaxIdleTime: cfg.poolMaxIdleTime, MaintainInterval: cfg.poolMaintainInterval, + LoadBalanced: cfg.loadBalanced, PoolMonitor: cfg.poolMonitor, Logger: cfg.logger, handshakeErrFn: s.ProcessHandshakeError, From 9c094db39fc530672d8b1ab52372852e46270651 Mon Sep 17 00:00:00 2001 From: Kevin Albertson Date: Wed, 30 Aug 2023 15:05:35 -0400 Subject: [PATCH 5/6] GODRIVER-2963 Use more environment variables in Azure KMS test (#1367) * use environment variables for key_name and key_vault_endpoint This may prevent future code changes if Azure test data changes. --- .evergreen/config.yml | 4 ++-- cmd/testkms/main.go | 20 ++++++++++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/.evergreen/config.yml b/.evergreen/config.yml index db6b51f680..81738eb513 100644 --- a/.evergreen/config.yml +++ b/.evergreen/config.yml @@ -2228,7 +2228,7 @@ tasks: export AZUREKMS_VMNAME=${AZUREKMS_VMNAME} echo '${testazurekms_privatekey}' > /tmp/testazurekms.prikey export AZUREKMS_PRIVATEKEYPATH=/tmp/testazurekms.prikey - AZUREKMS_CMD="LD_LIBRARY_PATH=./install/libmongocrypt/lib MONGODB_URI='mongodb://localhost:27017' PROVIDER='azure' ./testkms" $DRIVERS_TOOLS/.evergreen/csfle/azurekms/run-command.sh + AZUREKMS_CMD="LD_LIBRARY_PATH=./install/libmongocrypt/lib MONGODB_URI='mongodb://localhost:27017' PROVIDER='azure' AZUREKMS_KEY_NAME='${AZUREKMS_KEY_NAME}' AZUREKMS_KEY_VAULT_ENDPOINT='${AZUREKMS_KEY_VAULT_ENDPOINT}' ./testkms" $DRIVERS_TOOLS/.evergreen/csfle/azurekms/run-command.sh - name: "testazurekms-fail-task" # testazurekms-fail-task runs without environment variables. @@ -2250,7 +2250,7 @@ tasks: LD_LIBRARY_PATH=./install/libmongocrypt/lib \ MONGODB_URI='mongodb://localhost:27017' \ EXPECT_ERROR='unable to retrieve azure credentials' \ - PROVIDER='azure' \ + PROVIDER='azure' AZUREKMS_KEY_NAME='${AZUREKMS_KEY_NAME}' AZUREKMS_KEY_VAULT_ENDPOINT='${AZUREKMS_KEY_VAULT_ENDPOINT}' \ ./testkms - name: "test-fuzz" diff --git a/cmd/testkms/main.go b/cmd/testkms/main.go index 5d7532c4b2..af86eca523 100644 --- a/cmd/testkms/main.go +++ b/cmd/testkms/main.go @@ -24,8 +24,8 @@ var datakeyopts = map[string]primitive.M{ "key": "arn:aws:kms:us-east-1:579766882180:key/89fcc2c4-08b0-4bd9-9f25-e30687b580d0", }, "azure": bson.M{ - "keyVaultEndpoint": "https://keyvault-drivers-2411.vault.azure.net/keys/", - "keyName": "KEY-NAME", + "keyVaultEndpoint": "", + "keyName": "", }, "gcp": bson.M{ "projectId": "devprod-drivers", @@ -53,6 +53,20 @@ func main() { default: ok = true } + if provider == "azure" { + azureKmsKeyName := os.Getenv("AZUREKMS_KEY_NAME") + azureKmsKeyVaultEndpoint := os.Getenv("AZUREKMS_KEY_VAULT_ENDPOINT") + if azureKmsKeyName == "" { + fmt.Println("ERROR: Please set required AZUREKMS_KEY_NAME environment variable.") + ok = false + } + if azureKmsKeyVaultEndpoint == "" { + fmt.Println("ERROR: Please set required AZUREKMS_KEY_VAULT_ENDPOINT environment variable.") + ok = false + } + datakeyopts["azure"]["keyName"] = azureKmsKeyName + datakeyopts["azure"]["keyVaultEndpoint"] = azureKmsKeyVaultEndpoint + } if !ok { providers := make([]string, 0, len(datakeyopts)) for p := range datakeyopts { @@ -63,6 +77,8 @@ func main() { fmt.Println("- MONGODB_URI as a MongoDB URI. Example: 'mongodb://localhost:27017'") fmt.Println("- EXPECT_ERROR as an optional expected error substring.") fmt.Println("- PROVIDER as a KMS provider, which supports:", strings.Join(providers, ", ")) + fmt.Println("- AZUREKMS_KEY_NAME as the Azure key name. Required if PROVIDER=azure.") + fmt.Println("- AZUREKMS_KEY_VAULT_ENDPOINT as the Azure key name. Required if PROVIDER=azure.") os.Exit(1) } From 2f372fdae551b566fa65e6400e6bb52d745f9f2a Mon Sep 17 00:00:00 2001 From: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com> Date: Wed, 30 Aug 2023 18:15:18 -0400 Subject: [PATCH 6/6] GODRIVER-2872 Duplicate slice passed to mongocrypt.newBinaryFromBytes(). (#1359) --- .../client_side_encryption_prose_test.go | 3 --- x/mongo/driver/mongocrypt/binary.go | 17 ++++++++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/mongo/integration/client_side_encryption_prose_test.go b/mongo/integration/client_side_encryption_prose_test.go index f650aa6b47..4eac5364ca 100644 --- a/mongo/integration/client_side_encryption_prose_test.go +++ b/mongo/integration/client_side_encryption_prose_test.go @@ -378,9 +378,6 @@ func TestClientSideEncryptionProse(t *testing.T) { } }) mt.Run("4. bson size limits", func(mt *mtest.T) { - // TODO(GODRIVER-2872): Fix and unskip this test case. - mt.Skip("Test fails frequently, skipping. See GODRIVER-2872") - kmsProviders := map[string]map[string]interface{}{ "local": { "key": localMasterKey, diff --git a/x/mongo/driver/mongocrypt/binary.go b/x/mongo/driver/mongocrypt/binary.go index 9e887375a9..4e4b51d74b 100644 --- a/x/mongo/driver/mongocrypt/binary.go +++ b/x/mongo/driver/mongocrypt/binary.go @@ -9,7 +9,10 @@ package mongocrypt -// #include +/* +#include +#include +*/ import "C" import ( "unsafe" @@ -17,6 +20,7 @@ import ( // binary is a wrapper type around a mongocrypt_binary_t* type binary struct { + p *C.uint8_t wrapped *C.mongocrypt_binary_t } @@ -33,11 +37,11 @@ func newBinaryFromBytes(data []byte) *binary { return newBinary() } - // We don't need C.CBytes here because data cannot go out of scope. Any mongocrypt function that takes a - // mongocrypt_binary_t will make a copy of the data so the data can be garbage collected after calling. - addr := (*C.uint8_t)(unsafe.Pointer(&data[0])) // uint8_t* - dataLen := C.uint32_t(len(data)) // uint32_t + // TODO: Consider using runtime.Pinner to replace the C.CBytes after using go1.21.0. + addr := (*C.uint8_t)(C.CBytes(data)) // uint8_t* + dataLen := C.uint32_t(len(data)) // uint32_t return &binary{ + p: addr, wrapped: C.mongocrypt_binary_new_from_data(addr, dataLen), } } @@ -52,5 +56,8 @@ func (b *binary) toBytes() []byte { // close cleans up any resources associated with the given binary instance. func (b *binary) close() { + if b.p != nil { + C.free(unsafe.Pointer(b.p)) + } C.mongocrypt_binary_destroy(b.wrapped) }