Merge branch 'master' into CBG-3203

Author: bbrks

.github/workflows/ci.yml

@@ -27,7 +27,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: 1.21.3
+          go-version: 1.21.4
       - name: go-build
         run: go build "./..."
 
@@ -38,7 +38,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: 1.21.3
+          go-version: 1.21.4
       - run: go install github.com/google/addlicense@latest
       - run: addlicense -check -f licenses/addlicense.tmpl .
 
@@ -49,13 +49,13 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: 1.21.3
+          go-version: 1.21.4
           cache: false
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.55.0
-          args: --config=.golangci-strict.yml --timeout=3m
+          version: v1.55.2
+          args: --config=.golangci-strict.yml
 
   test:
     runs-on: ${{ matrix.os }}
@@ -77,7 +77,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: 1.21.3
+          go-version: 1.21.4
       - name: Build
         run: go build -v "./..."
       - name: Run Tests
@@ -97,7 +97,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: 1.21.3
+          go-version: 1.21.4
       - name: Run Tests
         run: go test -race -timeout=30m -count=1 -json -v "./..." | tee test.json | jq -s -jr 'sort_by(.Package,.Time) | .[].Output | select (. != null )'
         shell: bash
@@ -137,7 +137,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: 1.21.3
+          go-version: 1.21.4
       - name: Build
         run: go build -v "./tools/stats-definition-exporter"
       - name: Run Tests

.gitignore

@@ -21,3 +21,5 @@ __pycache__
 
 ### Couchbase Plugin ###
 .cbcache/
+
+planPIndexes/

.golangci-strict.yml

@@ -8,6 +8,9 @@
 
 # config file for golangci-lint
 
+run:
+  timeout: 3m
+
 linters:
   enable:
     #- bodyclose # checks whether HTTP response body is closed successfully
@@ -24,7 +27,7 @@ linters:
     #- nakedret # Finds naked returns in functions greater than a specified function length
     #- prealloc # Finds slice declarations that could potentially be preallocated
     #- revive # Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes
-    #- staticcheck # (megacheck) Staticcheck is a go vet on steroids, applying a ton of static analysis checks
+    - staticcheck # (megacheck) Staticcheck is a go vet on steroids, applying a ton of static analysis checks
     - typecheck # Like the front-end of a Go compiler, parses and type-checks Go code
     - unconvert # Remove unnecessary type conversions
     #- unparam # Reports unused function parameters
@@ -67,7 +70,6 @@ linters:
     - nakedret # Finds naked returns in functions greater than a specified function length
     - prealloc # Finds slice declarations that could potentially be preallocated
     - revive # Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes
-    - staticcheck # (megacheck) Staticcheck is a go vet on steroids, applying a ton of static analysis checks
     - structcheck # Finds unused struct fields
     - unparam # Reports unused function parameters
     - varcheck # Finds unused global variables and constants

.golangci.yml

@@ -8,6 +8,9 @@
 
 # config file for golangci-lint
 
+run:
+  timeout: 3m
+
 linters:
   enable:
     - bodyclose # checks whether HTTP response body is closed successfully

Jenkinsfile

@@ -16,7 +16,7 @@ pipeline {
     }
 
     tools {
-        go '1.21.3'
+        go '1.21.4'
     }
 
     stages {

auth/auth_test.go

@@ -721,7 +721,7 @@ func TestConcurrentUserWrites(t *testing.T) {
 	}
 
 	// Retrieve user to trigger initial calculation of roles, channels
-	user, getErr := auth.GetUser(username)
+	_, getErr := auth.GetUser(username)
 	require.NoError(t, getErr, "Error retrieving user")
 
 	require.NoError(t, auth.SetBcryptCost(DefaultBcryptCost))

auth/oidc_test.go

@@ -1205,9 +1205,8 @@ func TestJWTRolesChannels(t *testing.T) {
 
 			for i, login := range tc.logins {
 				var (
-					user           User
-					err            error
-					lastUpdateTime time.Time
+					user User
+					err  error
 				)
 				if i == 0 {
 					user, err = auth.NewUser(testUserPrefix+"_"+testSubject, "test", base.SetFromArray(login.explicitChannels))
@@ -1254,8 +1253,6 @@ func TestJWTRolesChannels(t *testing.T) {
 				}
 				require.Equal(t, base.SetFromArray(login.expectedChannels), user.Channels().AsSet())
 
-				require.Greater(t, user.JWTLastUpdated(), lastUpdateTime)
-				lastUpdateTime = user.JWTLastUpdated()
 			}
 		})
 	}

auth/password_hash_test.go

@@ -35,27 +35,30 @@ func BenchmarkBcryptCostTimes(b *testing.B) {
 
 	for i := minCostToTest; i < maxCostToTest; i++ {
 		b.Run(fmt.Sprintf("cost%d", i), func(bn *testing.B) {
-			bn.N = 1
 			_, err := bcrypt.GenerateFromPassword([]byte("hunter2"), i)
 			assert.NoError(bn, err)
 		})
 	}
 }
 
-// TestBcryptDefaultCostTime will ensure that the default bcrypt cost takes at least a 'reasonable' amount of time
-// If this test fails, it suggests maybe we need to think about increasing the default cost...
-func TestBcryptDefaultCostTime(t *testing.T) {
-	// Modest 2.2GHz macbook i7 takes ~80ms at cost 10
-	// Assume server CPUs are ~2x faster
-	minimumDuration := 40 * time.Millisecond
+// TestBcryptCostTimes will output the time it takes to hash a password with each bcrypt cost value
+func TestBcryptCostTimes(t *testing.T) {
+	// Little value in running this regularly. Might be useful for one-off informational purposes
+	t.Skip("Test disabled")
 
-	startTime := time.Now()
-	_, err := bcrypt.GenerateFromPassword([]byte("hunter2"), DefaultBcryptCost)
-	duration := time.Since(startTime)
+	minCostToTest := bcrypt.DefaultCost
+	maxCostToTest := bcrypt.DefaultCost + 5
 
-	t.Logf("bcrypt.GenerateFromPassword with cost %d took: %v", DefaultBcryptCost, duration)
-	assert.NoError(t, err)
-	assert.True(t, minimumDuration < duration)
+	for i := minCostToTest; i < maxCostToTest; i++ {
+		t.Run(fmt.Sprintf("cost%d", i), func(t *testing.T) {
+			startTime := time.Now()
+			_, err := bcrypt.GenerateFromPassword([]byte("hunter2"), i)
+			duration := time.Since(startTime)
+
+			t.Logf("bcrypt.GenerateFromPassword with cost %d took: %v", i, duration)
+			assert.NoError(t, err)
+		})
+	}
 }
 
 func TestSetBcryptCost(t *testing.T) {

base/collection.go

@@ -63,10 +63,6 @@ func GetGoCBv2Bucket(ctx context.Context, spec BucketSpec) (*GocbV2Bucket, error
 		RetryStrategy:  gocb.NewBestEffortRetryStrategy(nil),
 	}
 
-	if spec.KvPoolSize > 0 {
-		// TODO: Equivalent of kvPoolSize in gocb v2?
-	}
-
 	cluster, err := gocb.Connect(connString, clusterOptions)
 	if err != nil {
 		InfofCtx(ctx, KeyAuth, "Unable to connect to cluster: %v", err)

base/dcp_sharded.go

@@ -28,7 +28,7 @@ const DefaultImportPartitions = 16
 const DefaultImportPartitionsServerless = 6
 
 // firstVersionToSupportCollections represents the earliest Sync Gateway release that supports collections.
-var firstVersionToSupportCollections = &ComparableVersion{
+var firstVersionToSupportCollections = &ComparableBuildVersion{
 	epoch: 0,
 	major: 3,
 	minor: 1,
@@ -38,7 +38,7 @@ var firstVersionToSupportCollections = &ComparableVersion{
 // nodeExtras is the contents of the JSON value of the cbgt.NodeDef.Extras field as used by Sync Gateway.
 type nodeExtras struct {
 	// Version is the node's version.
-	Version *ComparableVersion `json:"v"`
+	Version *ComparableBuildVersion `json:"v"`
 }
 
 // CbgtContext holds the two handles we have for CBGT-related functionality.
@@ -376,7 +376,7 @@ func (c *CbgtContext) StartManager(ctx context.Context, dbName string, configGro
 
 // getNodeVersion returns the version of the node from its Extras field, or nil if none is stored. Returns an error if
 // the extras could not be parsed.
-func getNodeVersion(def *cbgt.NodeDef) (*ComparableVersion, error) {
+func getNodeVersion(def *cbgt.NodeDef) (*ComparableBuildVersion, error) {
 	if len(def.Extras) == 0 {
 		return nil, nil
 	}
@@ -388,7 +388,7 @@ func getNodeVersion(def *cbgt.NodeDef) (*ComparableVersion, error) {
 }
 
 // getMinNodeVersion returns the version of the oldest node currently in the cluster.
-func getMinNodeVersion(cfg cbgt.Cfg) (*ComparableVersion, error) {
+func getMinNodeVersion(cfg cbgt.Cfg) (*ComparableBuildVersion, error) {
 	nodes, _, err := cbgt.CfgGetNodeDefs(cfg, cbgt.NODE_DEFS_KNOWN)
 	if err != nil {
 		return nil, err
@@ -397,14 +397,14 @@ func getMinNodeVersion(cfg cbgt.Cfg) (*ComparableVersion, error) {
 		// If there are no nodes at all, it's likely we're the first node in the cluster.
 		return ProductVersion, nil
 	}
-	var minVersion *ComparableVersion
+	var minVersion *ComparableBuildVersion
 	for _, node := range nodes.NodeDefs {
 		nodeVersion, err := getNodeVersion(node)
 		if err != nil {
 			return nil, fmt.Errorf("failed to get version of node %v: %w", MD(node.HostPort).Redact(), err)
 		}
 		if nodeVersion == nil {
-			nodeVersion = zeroComparableVersion()
+			nodeVersion = zeroComparableBuildVersion()
 		}
 		if minVersion == nil || nodeVersion.Less(minVersion) {
 			minVersion = nodeVersion
@@ -678,11 +678,11 @@ func (meh *sgMgrEventHandlers) OnUnregisterPIndex(pindex *cbgt.PIndex) {
 // OnFeedError is required to trigger reconnection to a feed on a closed connection (EOF).
 // NotifyMgrOnClose will trigger cbgt closing and then attempt to reconnect to the feed, if the manager hasn't
 // been stopped.
-func (meh *sgMgrEventHandlers) OnFeedError(srcType string, r cbgt.Feed, feedErr error) {
+func (meh *sgMgrEventHandlers) OnFeedError(_ string, r cbgt.Feed, feedErr error) {
 
 	// cbgt always passes srcType = SOURCE_GOCBCORE, but we have a wrapped type associated with our indexes - use that instead
 	// for our logging
-	srcType = SOURCE_DCP_SG
+	srcType := SOURCE_DCP_SG
 	var bucketName, bucketUUID string
 	dcpFeed, ok := r.(cbgt.FeedEx)
 	if ok {

base/leaky_bucket.go

@@ -58,9 +58,9 @@ func (b *LeakyBucket) SetIgnoreClose(value bool) {
 	b.config.IgnoreClose = value
 }
 
-func (b *LeakyBucket) CloseAndDelete() error {
+func (b *LeakyBucket) CloseAndDelete(ctx context.Context) error {
 	if bucket, ok := b.bucket.(sgbucket.DeleteableStore); ok {
-		return bucket.CloseAndDelete()
+		return bucket.CloseAndDelete(ctx)
 	}
 	return nil
 }
@@ -358,8 +358,7 @@ func dedupeTapEvents(tapEvents []sgbucket.FeedEvent) []sgbucket.FeedEvent {
 	// sequence order as read off the feed.
 	deduped := []sgbucket.FeedEvent{}
 	for _, tapEvent := range tapEvents {
-		key := string(tapEvent.Key)
-		latestTapEventForKey := latestTapEventPerKey[key]
+		latestTapEventForKey := latestTapEventPerKey[string(tapEvent.Key)]
 		if tapEvent.Cas == latestTapEventForKey.Cas {
 			deduped = append(deduped, tapEvent)
 		}

base/logger_external.go

@@ -46,7 +46,8 @@ func initExternalLoggers() {
 }
 
 func updateExternalLoggers() {
-	if consoleLogger != nil && consoleLogger.shouldLog(nil, LevelDebug, KeyWalrus) {
+	// use context.Background() since this is called from init or to reset test logging
+	if consoleLogger != nil && consoleLogger.shouldLog(context.Background(), LevelDebug, KeyWalrus) {
 		rosmar.SetLogLevel(rosmar.LevelDebug)
 	} else {
 		rosmar.SetLogLevel(rosmar.LevelInfo)

base/logging_test.go

@@ -11,7 +11,6 @@ package base
 import (
 	"bytes"
 	"fmt"
-	"math/rand"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -79,9 +78,7 @@ func BenchmarkLogRotation(b *testing.B) {
 
 	for _, test := range tests {
 		b.Run(fmt.Sprintf("rotate:%t-compress:%t-bytes:%v", test.rotate, test.compress, test.numBytes), func(bm *testing.B) {
-			data := make([]byte, test.numBytes)
-			_, err := rand.Read(data)
-			require.NoError(bm, err)
+			data := FastRandBytes(bm, test.numBytes)
 
 			logPath := b.TempDir()
 			logger := lumberjack.Logger{Filename: filepath.Join(logPath, "output.log"), Compress: test.compress}
@@ -99,7 +96,7 @@ func BenchmarkLogRotation(b *testing.B) {
 			// we can't remove temp dir while the async compression is still writing log files
 			assert.NoError(bm, logger.Close())
 			ctx := TestCtx(bm)
-			err, _ = RetryLoop(ctx, "benchmark-logrotate-teardown",
+			err, _ := RetryLoop(ctx, "benchmark-logrotate-teardown",
 				func() (shouldRetry bool, err error, value interface{}) {
 					err = os.RemoveAll(logPath)
 					return err != nil, err, nil

base/main_test_bucket_pool.go

@@ -198,29 +198,17 @@ func (tbp *TestBucketPool) GetWalrusTestBucket(t testing.TB, url string) (b Buck
 	require.NoError(t, err)
 
 	var walrusBucket *rosmar.Bucket
-	var typeName string
+	const typeName = "rosmar"
 	bucketName := tbpBucketNamePrefix + "rosmar_" + id
 	if url == "walrus:" || url == rosmar.InMemoryURL {
-		walrusBucket, err = rosmar.OpenBucket(url, rosmar.CreateOrOpen)
-		if err == nil {
-			err := walrusBucket.SetName(bucketName)
-			if err != nil {
-				tbp.Fatalf(testCtx, "Could not set name %s for rosmar bucket: %s", bucketName, err)
-			}
-		}
+		walrusBucket, err = rosmar.OpenBucket(url, bucketName, rosmar.CreateOrOpen)
 	} else {
 		walrusBucket, err = rosmar.OpenBucketIn(url, bucketName, rosmar.CreateOrOpen)
 	}
-	typeName = "rosmar"
 	if err != nil {
 		tbp.Fatalf(testCtx, "couldn't get %s bucket from <%s>: %v", typeName, url, err)
 	}
 
-	err = walrusBucket.SetName(bucketName)
-	if err != nil {
-		tbp.Fatalf(testCtx, "Could not set name %s for rosmar bucket: %s", bucketName, err)
-	}
-
 	// Wrap Walrus buckets with a leaky bucket to support vbucket IDs on feed.
 	b = &LeakyBucket{bucket: walrusBucket, config: &LeakyBucketConfig{TapFeedVbuckets: true}}
 
@@ -258,14 +246,10 @@ func (tbp *TestBucketPool) GetWalrusTestBucket(t testing.TB, url string) (b Buck
 		atomic.AddInt32(&tbp.stats.NumBucketsClosed, 1)
 		atomic.AddInt64(&tbp.stats.TotalInuseBucketNano, time.Since(openedStart).Nanoseconds())
 		tbp.markBucketClosed(t, b)
-		if url == kTestWalrusURL {
-			b.Close(ctx)
-		} else {
-			// Persisted buckets should call close and delete
-			closeErr := walrusBucket.CloseAndDelete()
-			if closeErr != nil {
-				tbp.Logf(ctx, "Unexpected error closing persistent %s bucket: %v", typeName, closeErr)
-			}
+		// Persisted buckets should call close and delete
+		closeErr := walrusBucket.CloseAndDelete(ctx)
+		if closeErr != nil {
+			tbp.Logf(ctx, "Unexpected error closing persistent %s bucket: %v", typeName, closeErr)
 		}
 
 	}