Ensure locking patterns don't prevent parallel requests (#24)

* update dependencies

* Ensure locking patterns don't prevent parallel requests

* update comment on lock

Author: fairclothjm

connection_producer.go

@@ -30,7 +30,7 @@ type couchbaseDBConnectionProducer struct {
 	rawConfig   map[string]interface{}
 	Type        string
 	cluster     *gocb.Cluster
-	sync.Mutex
+	sync.RWMutex
 }
 
 func (c *couchbaseDBConnectionProducer) secretValues() map[string]string {
@@ -41,7 +41,7 @@ func (c *couchbaseDBConnectionProducer) secretValues() map[string]string {
 }
 
 func (c *couchbaseDBConnectionProducer) Init(ctx context.Context, initConfig map[string]interface{}, verifyConnection bool) (saveConfig map[string]interface{}, err error) {
-
+	// Don't let anyone read or write the config while we're using it
 	c.Lock()
 	defer c.Unlock()
 
@@ -98,9 +98,10 @@ func (c *couchbaseDBConnectionProducer) Initialize(ctx context.Context, config m
 	_, err := c.Init(ctx, config, verifyConnection)
 	return err
 }
+
 func (c *couchbaseDBConnectionProducer) Connection(ctx context.Context) (interface{}, error) {
-	// This is intentionally not grabbing the lock since the calling functions (e.g. CreateUser)
-	// are claiming it. (The locking patterns could be refactored to be more consistent/clear.)
+	// This is intentionally not grabbing the lock since the calling functions
+	// (e.g. CreateUser) are claiming it.
 
 	if !c.Initialized {
 		return nil, connutil.ErrNotInitialized
@@ -163,7 +164,6 @@ func (c *couchbaseDBConnectionProducer) Connection(ctx context.Context) (interfa
 
 // close terminates the database connection without locking
 func (c *couchbaseDBConnectionProducer) close() error {
-
 	if c.cluster != nil {
 		if err := c.cluster.Close(&gocb.ClusterCloseOptions{}); err != nil {
 			return err
@@ -176,6 +176,7 @@ func (c *couchbaseDBConnectionProducer) close() error {
 
 // Close terminates the database connection with locking
 func (c *couchbaseDBConnectionProducer) Close() error {
+	// Don't let anyone read or write the config while we're using it
 	c.Lock()
 	defer c.Unlock()
 

couchbase.go

@@ -9,7 +9,6 @@ import (
 
 	"github.com/couchbase/gocb/v2"
 	"github.com/hashicorp/errwrap"
-	hclog "github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-secure-stdlib/strutil"
 	dbplugin "github.com/hashicorp/vault/sdk/database/dbplugin/v5"
 	"github.com/hashicorp/vault/sdk/database/helper/credsutil"
@@ -87,9 +86,9 @@ func (c *CouchbaseDB) Initialize(ctx context.Context, req dbplugin.InitializeReq
 }
 
 func (c *CouchbaseDB) NewUser(ctx context.Context, req dbplugin.NewUserRequest) (dbplugin.NewUserResponse, error) {
-	// Grab the lock
-	c.Lock()
-	defer c.Unlock()
+	// Don't let anyone write the config while we're using it
+	c.RLock()
+	defer c.RUnlock()
 
 	username, err := c.usernameProducer.Generate(req.UsernameConfig)
 	if err != nil {
@@ -123,22 +122,15 @@ func (c *CouchbaseDB) UpdateUser(ctx context.Context, req dbplugin.UpdateUserReq
 }
 
 func (c *CouchbaseDB) DeleteUser(ctx context.Context, req dbplugin.DeleteUserRequest) (dbplugin.DeleteUserResponse, error) {
-	c.Lock()
-	defer c.Unlock()
+	// Don't let anyone write the config while we're using it
+	c.RLock()
+	defer c.RUnlock()
 
 	db, err := c.getConnection(ctx)
 	if err != nil {
 		return dbplugin.DeleteUserResponse{}, fmt.Errorf("failed to make connection: %w", err)
 	}
 
-	// Close the database connection to ensure no new connections come in
-	defer func() {
-		if err := c.close(); err != nil {
-			logger := hclog.New(&hclog.LoggerOptions{})
-			logger.Error("defer close failed", "error", err)
-		}
-	}()
-
 	// Get the UserManager
 	mgr := db.Users()
 
@@ -191,22 +183,15 @@ func newUser(ctx context.Context, db *gocb.Cluster, username string, req dbplugi
 }
 
 func (c *CouchbaseDB) changeUserPassword(ctx context.Context, username, password string) error {
-	c.Lock()
-	defer c.Unlock()
+	// Don't let anyone write the config while we're using it
+	c.RLock()
+	defer c.RUnlock()
 
 	db, err := c.getConnection(ctx)
 	if err != nil {
 		return err
 	}
 
-	// Close the database connection to ensure no new connections come in
-	defer func() {
-		if err := c.close(); err != nil {
-			logger := hclog.New(&hclog.LoggerOptions{})
-			logger.Error("defer close failed", "error", err)
-		}
-	}()
-
 	// Get the UserManager
 	mgr := db.Users()
 	user, err := mgr.GetUser(username, nil)

couchbase_test.go

@@ -11,8 +11,8 @@ import (
 
 	dbplugin "github.com/hashicorp/vault/sdk/database/dbplugin/v5"
 	dbtesting "github.com/hashicorp/vault/sdk/database/dbplugin/v5/testing"
-	"github.com/ory/dockertest"
-	dc "github.com/ory/dockertest/docker"
+	"github.com/ory/dockertest/v3"
+	dc "github.com/ory/dockertest/v3/docker"
 	"github.com/stretchr/testify/require"
 )
 
@@ -181,7 +181,6 @@ func TestDriver(t *testing.T) {
 }
 
 func testGetCouchbaseVersion(t *testing.T, address string) {
-
 	var err error
 	pre6dot5, err = CheckForOldCouchbaseVersion(address, adminUsername, adminPassword)
 	if err != nil {
@@ -191,7 +190,6 @@ func testGetCouchbaseVersion(t *testing.T, address string) {
 }
 
 func setupCouchbaseDBInitialize(t *testing.T, connectionDetails map[string]interface{}) (err error) {
-
 	initReq := dbplugin.InitializeRequest{
 		Config:           connectionDetails,
 		VerifyConnection: true,
@@ -213,6 +211,7 @@ func setupCouchbaseDBInitialize(t *testing.T, connectionDetails map[string]inter
 	}
 	return nil
 }
+
 func testCouchbaseDBInitialize_TLS(t *testing.T, address string, port int) {
 	t.Log("Testing TLS Init()")
 
@@ -242,6 +241,7 @@ func testCouchbaseDBInitialize_TLS(t *testing.T, address string, port int) {
 		t.Log("Testing TLS Init() failed as expected (no BucketName set)")
 	}
 }
+
 func testCouchbaseDBInitialize_NoTLS(t *testing.T, address string, port int) {
 	t.Log("Testing plain text Init()")
 
@@ -258,8 +258,8 @@ func testCouchbaseDBInitialize_NoTLS(t *testing.T, address string, port int) {
 	if err != nil && pre6dot5 {
 		t.Log("Testing TLS Init() failed as expected (no BucketName set)")
 	}
-
 }
+
 func testCouchbaseDBInitialize_Pre6dot5TLS(t *testing.T, address string, port int) {
 	t.Log("Testing TLS Pre 6.5 Init()")
 
@@ -287,6 +287,7 @@ func testCouchbaseDBInitialize_Pre6dot5TLS(t *testing.T, address string, port in
 	}
 	setupCouchbaseDBInitialize(t, connectionDetails)
 }
+
 func testCouchbaseDBInitialize_Pre6dot5NoTLS(t *testing.T, address string, port int) {
 	t.Log("Testing Pre 6.5 Init()")
 
@@ -613,6 +614,7 @@ func testCouchbaseDBCreateUser_groupOnly(t *testing.T, address string, port int)
 		t.Fatalf("Could not revoke user: %s", userResp.Username)
 	}
 }
+
 func testCouchbaseDBCreateUser_roleAndGroup(t *testing.T, address string, port int) {
 	if pre6dot5 {
 		t.Log("Skipping as groups are not supported pre6.5.0")
@@ -676,6 +678,7 @@ func testCouchbaseDBCreateUser_roleAndGroup(t *testing.T, address string, port i
 		t.Fatalf("Could not revoke user: %s", userResp.Username)
 	}
 }
+
 func testCouchbaseDBRotateRootCredentials(t *testing.T, address string, port int) {
 	t.Log("Testing RotateRootCredentials()")
 
@@ -727,7 +730,6 @@ func testCouchbaseDBRotateRootCredentials(t *testing.T, address string, port int
 }
 
 func doCouchbaseDBSetCredentials(t *testing.T, username, password, address string, port int) {
-
 	t.Log("Testing SetCredentials()")
 
 	connectionDetails := map[string]interface{}{

go.mod

@@ -3,20 +3,69 @@ module github.com/hashicorp/vault-plugin-database-couchbase
 go 1.14
 
 require (
-	github.com/Nvveen/Gotty v0.0.0-20120604004816-cd527374f1e5 // indirect
 	github.com/cenkalti/backoff v2.2.1+incompatible
-	github.com/containerd/continuity v0.0.0-20200710164510-efbc4488d8fe // indirect
+	github.com/containerd/continuity v0.2.0 // indirect
 	github.com/couchbase/gocb/v2 v2.3.1
-	github.com/gotestyourself/gotestyourself v2.2.0+incompatible // indirect
+	github.com/docker/cli v20.10.9+incompatible // indirect
+	github.com/docker/docker v20.10.9+incompatible // indirect
 	github.com/hashicorp/errwrap v1.0.0
 	github.com/hashicorp/go-hclog v0.14.1
 	github.com/hashicorp/go-secure-stdlib/strutil v0.1.1
 	github.com/hashicorp/go-version v1.2.1
 	github.com/hashicorp/vault/sdk v0.1.14-0.20210204230556-cf85a862b7c6
 	github.com/hashicorp/yamux v0.0.0-20181012175058-2f1d1f20f75d // indirect
 	github.com/lib/pq v1.8.0 // indirect
-	github.com/mitchellh/mapstructure v1.3.3
-	github.com/ory/dockertest v3.3.5+incompatible
-	github.com/sirupsen/logrus v1.6.0 // indirect
+	github.com/mitchellh/mapstructure v1.4.2
+	github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6 // indirect
+	github.com/ory/dockertest/v3 v3.8.0
 	github.com/stretchr/testify v1.7.0
+	github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
+	golang.org/x/net v0.0.0-20211008194852-3b03d305991f // indirect
+	golang.org/x/sys v0.0.0-20211007075335-d3039528d8ac // indirect
+	gopkg.in/yaml.v2 v2.4.0 // indirect
+)
+
+require (
+	github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
+	github.com/Microsoft/go-winio v0.5.0 // indirect
+	github.com/Nvveen/Gotty v0.0.0-20120604004816-cd527374f1e5 // indirect
+	github.com/armon/go-metrics v0.3.3 // indirect
+	github.com/cenkalti/backoff/v4 v4.1.1 // indirect
+	github.com/couchbase/gocbcore/v10 v10.0.2 // indirect
+	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/docker/go-connections v0.4.0 // indirect
+	github.com/docker/go-units v0.4.0 // indirect
+	github.com/fatih/color v1.7.0 // indirect
+	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/golang/protobuf v1.5.0 // indirect
+	github.com/golang/snappy v0.0.1 // indirect
+	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
+	github.com/google/uuid v1.1.1 // indirect
+	github.com/hashicorp/go-immutable-radix v1.1.0 // indirect
+	github.com/hashicorp/go-multierror v1.1.0 // indirect
+	github.com/hashicorp/go-plugin v1.0.1 // indirect
+	github.com/hashicorp/go-sockaddr v1.0.2 // indirect
+	github.com/hashicorp/go-uuid v1.0.2 // indirect
+	github.com/hashicorp/golang-lru v0.5.3 // indirect
+	github.com/imdario/mergo v0.3.12 // indirect
+	github.com/mattn/go-colorable v0.1.6 // indirect
+	github.com/mattn/go-isatty v0.0.12 // indirect
+	github.com/mitchellh/go-testing-interface v1.0.0 // indirect
+	github.com/oklog/run v1.0.0 // indirect
+	github.com/opencontainers/go-digest v1.0.0 // indirect
+	github.com/opencontainers/image-spec v1.0.1 // indirect
+	github.com/opencontainers/runc v1.0.2 // indirect
+	github.com/pierrec/lz4 v2.5.2+incompatible // indirect
+	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
+	github.com/ryanuber/go-glob v1.0.0 // indirect
+	github.com/sirupsen/logrus v1.8.1 // indirect
+	github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect
+	github.com/xeipuuv/gojsonschema v1.2.0 // indirect
+	golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 // indirect
+	golang.org/x/text v0.3.6 // indirect
+	google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013 // indirect
+	google.golang.org/grpc v1.29.1 // indirect
+	google.golang.org/protobuf v1.26.0 // indirect
+	gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c // indirect
 )