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.

Sign up for GitHub

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

Mongodb driver switch to mongo-driver #8140

Merged
merged 9 commits into from
Jan 24, 2020
Merged

Mongodb driver switch to mongo-driver #8140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
127aaa3
Switch mongodb driver to mongo-driver
michelvocks Jan 13, 2020
d62affa
Tidy mod
michelvocks Jan 13, 2020
1682794
Make writeConcern private
michelvocks Jan 14, 2020
6e5f64b
Implement review feedback
michelvocks Jan 15, 2020
d14105f
Add retry functionality
michelvocks Jan 16, 2020
7286eb0
Added backoff time
michelvocks Jan 22, 2020
3d0cb6b
go mod vendor
michelvocks Jan 23, 2020
5d9fe4a
Fix failing test
michelvocks Jan 23, 2020
a74f924
goimport
michelvocks Jan 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
33 changes: 31 additions & 2 deletions builtin/logical/database/rotation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ import (
"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/helper/dbtxn"
"github.com/hashicorp/vault/sdk/logical"

"github.com/lib/pq"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"
)

const (
Expand Down Expand Up @@ -843,6 +844,11 @@ func TestBackend_StaticRole_Rotations_MongoDB(t *testing.T) {
// configure backend, add item and confirm length
cleanup, connURL := mongodb.PrepareTestContainerWithDatabase(t, "latest", "vaulttestdb")
defer cleanup()
testCases := []string{"65", "130", "5400"}
// Create database users ahead
for _, tc := range testCases {
testCreateDBUser(t, connURL, "vaulttestdb", "statictestMongo"+tc, "test")
}

// Configure a connection
data := map[string]interface{}{
Expand All @@ -865,7 +871,6 @@ func TestBackend_StaticRole_Rotations_MongoDB(t *testing.T) {
}

// create three static roles with different rotation periods
testCases := []string{"65", "130", "5400"}
for _, tc := range testCases {
roleName := "plugin-static-role-" + tc
data = map[string]interface{}{
Expand Down Expand Up @@ -956,6 +961,30 @@ func TestBackend_StaticRole_Rotations_MongoDB(t *testing.T) {
}
}

func testCreateDBUser(t testing.TB, connURL, db, username, password string) {
ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
client, err := mongo.Connect(ctx, options.Client().ApplyURI(connURL))
if err != nil {
t.Fatal(err)
}

createUserCmd := &createUserCommand{
Username: username,
Password: password,
Roles: []interface{}{},
}
result := client.Database(db).RunCommand(ctx, createUserCmd, nil)
if result.Err() != nil {
t.Fatal(result.Err())
}
}

type createUserCommand struct {
Username string `bson:"createUser"`
Password string `bson:"pwd"`
Roles []interface{} `bson:"roles"`
}

// Demonstrates a bug fix for the credential rotation not releasing locks
func TestBackend_StaticRole_LockRegression(t *testing.T) {
cluster, sys := getCluster(t)
Expand Down
5 changes: 5 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ require (
cloud.google.com/go v0.39.0
github.com/Azure/azure-sdk-for-go v36.2.0+incompatible
github.com/Azure/go-autorest/autorest v0.9.2
github.com/DataDog/zstd v1.4.4 // indirect
github.com/NYTimes/gziphandler v1.1.1
github.com/SAP/go-hdb v0.14.1
github.com/StackExchange/wmi v0.0.0-20180116203802-5d049714c4a6 // indirect
Expand Down Expand Up @@ -124,9 +125,13 @@ require (
github.com/shirou/w32 v0.0.0-20160930032740-bb4de0191aa4 // indirect
github.com/shopspring/decimal v0.0.0-20180709203117-cd690d0c9e24 // indirect
github.com/stretchr/testify v1.4.0
github.com/tidwall/pretty v1.0.0 // indirect
github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c // indirect
github.com/xdg/stringprep v1.0.0 // indirect
github.com/xi2/xz v0.0.0-20171230120015-48954b6210f8 // indirect
go.etcd.io/bbolt v1.3.2
go.etcd.io/etcd v0.0.0-20190412021913-f29b1ada1971
go.mongodb.org/mongo-driver v1.2.1
go.uber.org/atomic v1.4.0
golang.org/x/crypto v0.0.0-20191106202628-ed6320f186d4
golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7
Expand Down
11 changes: 11 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03
github.com/DataDog/datadog-go v2.2.0+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ=
github.com/DataDog/datadog-go v3.2.0+incompatible h1:qSG2N4FghB1He/r2mFrWKCaL7dXCilEuNEeAn20fdD4=
github.com/DataDog/datadog-go v3.2.0+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ=
github.com/DataDog/zstd v1.4.4 h1:+IawcoXhCBylN7ccwdwf8LOH2jKq7NavGpEPanrlTzE=
github.com/DataDog/zstd v1.4.4/go.mod h1:1jcaCB/ufaK+sKp1NBhlGmpz41jOoPQ35bpF36t7BBo=
github.com/Jeffail/gabs v1.1.1 h1:V0uzR08Hj22EX8+8QMhyI9sX2hwRu+/RJhJUmnwda/E=
github.com/Jeffail/gabs v1.1.1/go.mod h1:6xMvQMK4k33lb7GUUpaAPh6nKMmemQeg5d4gn7/bOXc=
github.com/Masterminds/semver v1.4.2 h1:WBLTQ37jOCzSLtXNdoo8bNM8876KhNqOKvrlGITgsTc=
Expand Down Expand Up @@ -202,6 +204,7 @@ github.com/go-ole/go-ole v1.2.1 h1:2lOsA72HgjxAuMlKpFiCbHTvu44PIVkZ5hqm3RSdI/E=
github.com/go-ole/go-ole v1.2.1/go.mod h1:7FAglXiTm7HKlQRDeOQ6ZNUHidzCWXuZWq/1dTyBNF8=
github.com/go-sql-driver/mysql v1.4.1 h1:g24URVg0OFbNUTx9qqY1IRZ9D9z3iPyi5zKhQZpNwpA=
github.com/go-sql-driver/mysql v1.4.1/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w=
github.com/go-stack/stack v1.8.0 h1:5SgMzNM5HxrEjV0ww2lTmX6E2Izsfxas4+YHWRs3Lsk=
github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
github.com/go-test/deep v1.0.2-0.20181118220953-042da051cf31/go.mod h1:wGDj63lr65AM2AQyKZd/NYHGb0R+1RLqB8NKt3aSFNA=
github.com/go-test/deep v1.0.2 h1:onZX1rnHT3Wv6cqNgYyFOOlgVKJrksuCMCRvJStbMYw=
Expand Down Expand Up @@ -629,6 +632,8 @@ github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UV
github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
github.com/tarm/serial v0.0.0-20180830185346-98f6abe2eb07/go.mod h1:kDXzergiv9cbyO7IOYJZWg1U88JhDg3PB6klq9Hg2pA=
github.com/tidwall/pretty v1.0.0 h1:HsD+QiTn7sK6flMKIvNmpqz1qrpP3Ps6jOKIKMooyg4=
github.com/tidwall/pretty v1.0.0/go.mod h1:XNkn88O1ChpSDQmQeStsy+sBenx6DDtFZJxhVysOjyk=
github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8 h1:ndzgwNDnKIqyCvHTXaCqh9KlOWKvBry6nuXMJmonVsE=
github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926 h1:G3dpKMzFDjgEh2q1Z7zUUtKa8ViPtH+ocF0bE0g00O8=
Expand All @@ -640,6 +645,10 @@ github.com/ugorji/go/codec v0.0.0-20190204201341-e444a5086c43/go.mod h1:iT03XoTw
github.com/ulikunitz/xz v0.5.6 h1:jGHAfXawEGZQ3blwU5wnWKQJvAraT7Ftq9EXjnXYgt8=
github.com/ulikunitz/xz v0.5.6/go.mod h1:2bypXElzHzzJZwzH67Y6wb67pO62Rzfn7BSiF4ABRW8=
github.com/urfave/cli v1.20.0/go.mod h1:70zkFmudgCuE/ngEzBv17Jvp/497gISqfk5gWijbERA=
github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c h1:u40Z8hqBAAQyv+vATcGgV0YCnDjqSL7/q/JyPhhJSPk=
github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c/go.mod h1:lB8K/P019DLNhemzwFU4jHLhdvlE6uDZjXFejJXr49I=
github.com/xdg/stringprep v1.0.0 h1:d9X0esnoa3dFsV0FG35rAT0RIhYFlPq7MiP+DW89La0=
github.com/xdg/stringprep v1.0.0/go.mod h1:Jhud4/sHMO4oL310DaZAKk9ZaJ08SJfe+sJh0HrGL1Y=
github.com/xi2/xz v0.0.0-20171230120015-48954b6210f8 h1:nIPpBwaJSVYIxUFsDv3M8ofmx9yWTog9BfvIu0q41lo=
github.com/xi2/xz v0.0.0-20171230120015-48954b6210f8/go.mod h1:HUYIGzjTL3rfEspMxjDjgmT5uz5wzYJKVo23qUhYTos=
github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2 h1:eY9dn8+vbi4tKz5Qo6v2eYzo7kUS51QINcR5jNpbZS8=
Expand All @@ -648,6 +657,8 @@ go.etcd.io/bbolt v1.3.2 h1:Z/90sZLPOeCy2PwprqkFa25PdkusRzaj9P8zm/KNyvk=
go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU=
go.etcd.io/etcd v0.0.0-20190412021913-f29b1ada1971 h1:C+ye4QyWT3rbVj8As5DUc+Dsp067xJxCC6aa9+UnCmU=
go.etcd.io/etcd v0.0.0-20190412021913-f29b1ada1971/go.mod h1:KSGwdbiFchh5KIC9My2+ZVl5/3ANcwohw50dpPwa2cw=
go.mongodb.org/mongo-driver v1.2.1 h1:ANAlYXXM5XmOdW/Nc38jOr+wS5nlk7YihT24U1imiWM=
go.mongodb.org/mongo-driver v1.2.1/go.mod h1:u7ryQJ+DOzQmeO7zB6MHyr8jkEQvC8vH7qLUO4lqsUM=
go.opencensus.io v0.18.0/go.mod h1:vKdFvxhtzZ9onBp9VKHK8z/sRpBMnKAsufL7wlDrCOA=
go.opencensus.io v0.19.1/go.mod h1:gug0GbSHa8Pafr0d2urOSgoXHZ6x/RUlaiT0d9pqb4A=
go.opencensus.io v0.19.2/go.mod h1:NO/8qkisMZLZ1FCsKNqtJPwc8/TaclWyY0B6wcYNg9M=
Expand Down
180 changes: 71 additions & 109 deletions plugins/database/mongodb/connection_producer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,20 @@ package mongodb

import (
"context"
"crypto/tls"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"net"
"net/url"
"strconv"
"strings"
"sync"
"time"

"github.com/hashicorp/errwrap"
"github.com/hashicorp/vault/sdk/database/helper/connutil"
"github.com/hashicorp/vault/sdk/database/helper/dbutil"
"github.com/mitchellh/mapstructure"
mgo "gopkg.in/mgo.v2"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/mongo/readpref"
"go.mongodb.org/mongo-driver/mongo/writeconcern"
)

// mongoDBConnectionProducer implements ConnectionProducer and provides an
Expand All @@ -29,14 +26,23 @@ type mongoDBConnectionProducer struct {
Username string `json:"username" structs:"username" mapstructure:"username"`
Password string `json:"password" structs:"password" mapstructure:"password"`

Initialized bool
RawConfig map[string]interface{}
Type string
session *mgo.Session
safe *mgo.Safe
Initialized bool
RawConfig map[string]interface{}
Type string
clientOptions *options.ClientOptions
client *mongo.Client
sync.Mutex
}

// writeConcern defines the write concern options
type writeConcern struct {
W int // Min # of servers to ack before success
WMode string // Write mode for MongoDB 2.0+ (e.g. "majority")
WTimeout int // Milliseconds to wait for W before timing out
FSync bool // DEPRECATED: Is now handled by J. See: https://jira.mongodb.org/browse/CXX-910
J bool // Sync via the journal if present
}

func (c *mongoDBConnectionProducer) Initialize(ctx context.Context, conf map[string]interface{}, verifyConnection bool) error {
_, err := c.Init(ctx, conf, verifyConnection)
return err
Expand Down Expand Up @@ -73,18 +79,41 @@ func (c *mongoDBConnectionProducer) Init(ctx context.Context, conf map[string]in
input = string(inputBytes)
}

concern := &mgo.Safe{}
concern := &writeConcern{}
err = json.Unmarshal([]byte(input), concern)
if err != nil {
return nil, errwrap.Wrapf("error mashalling write_concern: {{err}}", err)
return nil, errwrap.Wrapf("error unmarshalling write_concern: {{err}}", err)
}

// Translate write concern to mongo options
var w writeconcern.Option
switch {
case concern.W != 0:
w = writeconcern.W(concern.W)
case concern.WMode != "":
w = writeconcern.WTagSet(concern.WMode)
default:
w = writeconcern.WMajority()
}

var j writeconcern.Option
switch {
case concern.FSync:
j = writeconcern.J(concern.FSync)
case concern.J:
j = writeconcern.J(concern.J)
default:
j = writeconcern.J(false)
}

// Guard against empty, non-nil mgo.Safe object; we don't want to pass that
// into mgo.SetSafe in Connection().
if (mgo.Safe{} == *concern) {
return nil, fmt.Errorf("provided write_concern values did not map to any mgo.Safe fields")
writeConcern := writeconcern.New(
w,
j,
writeconcern.WTimeout(time.Duration(concern.WTimeout)*time.Millisecond))

c.clientOptions = &options.ClientOptions{
WriteConcern: writeConcern,
}
c.safe = concern
}

// Set initialized to true at this point since all fields are set,
Expand All @@ -96,7 +125,7 @@ func (c *mongoDBConnectionProducer) Init(ctx context.Context, conf map[string]in
return nil, errwrap.Wrapf("error verifying connection: {{err}}", err)
}

if err := c.session.Ping(); err != nil {
if err := c.client.Ping(ctx, readpref.Primary()); err != nil {
return nil, errwrap.Wrapf("error verifying connection: {{err}}", err)
}
}
Expand All @@ -106,120 +135,53 @@ func (c *mongoDBConnectionProducer) Init(ctx context.Context, conf map[string]in

// Connection creates or returns an existing a database connection. If the session fails
// on a ping check, the session will be closed and then re-created.
func (c *mongoDBConnectionProducer) Connection(_ context.Context) (interface{}, error) {
// This method does not lock the mutex and it is intended that this is the callers
// responsibility.
func (c *mongoDBConnectionProducer) Connection(ctx context.Context) (interface{}, error) {
if !c.Initialized {
kalafut marked this conversation as resolved.
Show resolved Hide resolved
return nil, connutil.ErrNotInitialized
}

if c.session != nil {
if err := c.session.Ping(); err == nil {
return c.session, nil
if c.client != nil {
if err := c.client.Ping(ctx, readpref.Primary()); err == nil {
return c.client, nil
}
c.session.Close()
// Ignore error on purpose since we want to re-create a session
Copy link

Choose a reason for hiding this comment

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

This code seems a little weird to me. The mongo.Client type is resilient to failovers, so a failed Ping does not necessarily indicate that the client should be restarted. If the server is in the cluster of failing over during the Ping, the Client will re-calibrate its view of the cluster and figure out what changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the server is in the cluster of failing over during the Ping, the Client will re-calibrate its view of the cluster and figure out what changed.

I assume this happens automatically while we ping the server, correct? If that is the case I think this is just for safety reasons in case something bad happens. I agree that mostly the next connection attempt will fail too but I also think it doesn't hurt to try it again?

Copy link

Choose a reason for hiding this comment

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

Instead of creating a completely new Client, maybe we can just try the Ping again? This is the biggest downfall of using the Ping method in general: it's good for testing server connectivity but if the server is undergoing a transient state change, it will report a failure even though it's not really a failure.

If you want to avoid risk, though, perhaps it is best to keep the existing behavior and just recreate the Client. I'll defer that to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally think that creating a fresh client which adds a minimal overhead compared to reusing the old client is the better choice here.

_ = c.client.Disconnect(ctx)
kalafut marked this conversation as resolved.
Show resolved Hide resolved
}

dialInfo, err := parseMongoURL(c.ConnectionURL)
if err != nil {
return nil, err
if c.clientOptions == nil {
c.clientOptions = options.Client()
}
c.clientOptions.SetSocketTimeout(1 * time.Minute)
c.clientOptions.SetConnectTimeout(1 * time.Minute)

c.session, err = mgo.DialWithInfo(dialInfo)
var err error
c.client, err = mongo.Connect(ctx, c.clientOptions.ApplyURI(c.ConnectionURL))
if err != nil {
return nil, err
}

if c.safe != nil {
c.session.SetSafe(c.safe)
}

c.session.SetSyncTimeout(1 * time.Minute)
c.session.SetSocketTimeout(1 * time.Minute)

return c.session, nil
return c.client, nil
}

// Close terminates the database connection.
func (c *mongoDBConnectionProducer) Close() error {
c.Lock()
defer c.Unlock()

if c.session != nil {
c.session.Close()
if c.client != nil {
ctx, cancel := context.WithTimeout(context.Background(), 1 * time.Minute)
defer cancel()
if err := c.client.Disconnect(ctx); err != nil {
return err
}
}

c.session = nil
c.client = nil

return nil
}

func parseMongoURL(rawURL string) (*mgo.DialInfo, error) {
url, err := url.Parse(rawURL)
if err != nil {
return nil, err
}

info := mgo.DialInfo{
Addrs: strings.Split(url.Host, ","),
Database: strings.TrimPrefix(url.Path, "/"),
Timeout: 10 * time.Second,
}

if url.User != nil {
info.Username = url.User.Username()
info.Password, _ = url.User.Password()
}

query := url.Query()
for key, values := range query {
var value string
if len(values) > 0 {
value = values[0]
}

switch key {
case "authSource":
info.Source = value
case "authMechanism":
info.Mechanism = value
case "gssapiServiceName":
info.Service = value
case "replicaSet":
info.ReplicaSetName = value
case "maxPoolSize":
poolLimit, err := strconv.Atoi(value)
if err != nil {
return nil, errors.New("bad value for maxPoolSize: " + value)
}
info.PoolLimit = poolLimit
case "ssl":
// Unfortunately, mgo doesn't support the ssl parameter in its MongoDB URI parsing logic, so we have to handle that
// ourselves. See https://github.com/go-mgo/mgo/issues/84
ssl, err := strconv.ParseBool(value)
if err != nil {
return nil, errors.New("bad value for ssl: " + value)
}
if ssl {
info.DialServer = func(addr *mgo.ServerAddr) (net.Conn, error) {
return tls.Dial("tcp", addr.String(), &tls.Config{})
}
}
case "connect":
if value == "direct" {
info.Direct = true
break
}
if value == "replicaSet" {
break
}
fallthrough
default:
return nil, errors.New("unsupported connection URL option: " + key + "=" + value)
}
}

return &info, nil
}

func (c *mongoDBConnectionProducer) secretValues() map[string]interface{} {
return map[string]interface{}{
c.Password: "[password]",
Expand Down
Loading