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.

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

Support MongoDB session-wide write concern #3646

Merged
merged 6 commits into from
Dec 5, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions plugins/database/mongodb/connection_producer.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package mongodb

import (
"crypto/tls"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"net"
Expand All @@ -21,6 +23,7 @@ import (
// interface for databases to make connections.
type mongoDBConnectionProducer struct {
ConnectionURL string `json:"connection_url" structs:"connection_url" mapstructure:"connection_url"`
WriteConcern string `json:"write_concern" structs:"write_concern" mapstructure:"write_concern"`

Initialized bool
Type string
Expand Down Expand Up @@ -78,6 +81,32 @@ func (c *mongoDBConnectionProducer) Connection() (interface{}, error) {
if err != nil {
return nil, err
}

if c.WriteConcern != "" {
input := c.WriteConcern

// Try to base64 decode the input. If successful, consider the decoded
// value as input.
inputBytes, err := base64.StdEncoding.DecodeString(input)
if err == nil {
input = string(inputBytes)
}

var concern writeConcern
err = json.Unmarshal([]byte(input), &concern)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be better to do in the Initialize function and cache the writeConcern object in the struct. Will save on marshaling it on every call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only gets through if Connection() creates a new session, otherwise it will return pretty early up top. Since other session settings were set here, I thought calling SetSafe here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also probably better to return any unmarshaling errors during Init too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say decode and unmarshal in Init, store the mgo.Safe pointer in the struct and call SetSafe in Connection()

if err != nil {
return nil, err
}

c.session.SetSafe(&mgo.Safe{
W: concern.W,
WMode: concern.WMode,
WTimeout: concern.WTimeout,
FSync: concern.FSync,
J: concern.J,
})
}

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

Expand Down
40 changes: 40 additions & 0 deletions plugins/database/mongodb/mongodb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (

const testMongoDBRole = `{ "db": "admin", "roles": [ { "role": "readWrite" } ] }`

const testMongoDBWriteConcern = `{ "wmode": "majority", "wtimeout": 5000 }`

func prepareMongoDBTestContainer(t *testing.T) (cleanup func(), retURL string) {
if os.Getenv("MONGODB_URL") != "" {
return func() {}, os.Getenv("MONGODB_URL")
Expand Down Expand Up @@ -129,6 +131,44 @@ func TestMongoDB_CreateUser(t *testing.T) {
}
}

func TestMongoDB_CreateUser_writeConcern(t *testing.T) {
cleanup, connURL := prepareMongoDBTestContainer(t)
defer cleanup()

connectionDetails := map[string]interface{}{
"connection_url": connURL,
"write_concern": testMongoDBWriteConcern,
}

dbRaw, err := New()
if err != nil {
t.Fatalf("err: %s", err)
}
db := dbRaw.(*MongoDB)
err = db.Initialize(connectionDetails, true)
if err != nil {
t.Fatalf("err: %s", err)
}

statements := dbplugin.Statements{
CreationStatements: testMongoDBRole,
}

usernameConfig := dbplugin.UsernameConfig{
DisplayName: "test",
RoleName: "test",
}

username, password, err := db.CreateUser(statements, usernameConfig, time.Now().Add(time.Minute))
if err != nil {
t.Fatalf("err: %s", err)
}

if err := testCredsExist(t, connURL, username, password); err != nil {
t.Fatalf("Could not connect with new credentials: %s", err)
}
}

func TestMongoDB_RevokeUser(t *testing.T) {
cleanup, connURL := prepareMongoDBTestContainer(t)
defer cleanup()
Expand Down
10 changes: 10 additions & 0 deletions plugins/database/mongodb/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ type mongoDBStatement struct {
Roles mongodbRoles `json:"roles"`
}

// writeConcern is the mgo.Safe struct with JSON tags
// More info: https://godoc.org/gopkg.in/mgo.v2#Safe
type writeConcern struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this struct or can we just use the mgo.Safe version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mgo.Safe struct doesn't have json tags for the fields :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we might not need the JSON tags if there's no snake-casing on the fields so this could probably go away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, in that case i'd say it's fine to leave this struct as is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We agreed offline that this is okay to remove since we are following mongoDB's naming convention (e.g. wtimeout is not snake-cased). In this case, we can use mgo.Safe's struct directly.

W int `json:"w"` // Min # of servers to ack before success
WMode string `json:"w_mode"` // Write mode for MongoDB 2.0+ (e.g. "majority")
WTimeout int `json:"wtimeout"` // Milliseconds to wait for W before timing out
FSync bool `json:"fsync"` // Sync via the journal if present, or via data files sync otherwise
J bool `json:"j"` // Sync via the journal if present
}

// Convert array of role documents like:
//
// [ { "role": "readWrite" }, { "role": "readWrite", "db": "test" } ]
Expand Down
21 changes: 16 additions & 5 deletions website/source/api/secret/databases/mongodb.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,26 @@ has a number of parameters to further configure a connection.

| Method | Path | Produces |
| :------- | :--------------------------- | :--------------------- |
| `POST` | `/database/config/:name` | `204 (empty body)` |
| `POST` | `/database/config/:name` | `204 (empty body)` |

### Parameters
- `connection_url` `(string: <required>)` – Specifies the MongoDB standard connection string (URI).

- `connection_url` `(string: <required>)` – Specifies the MongoDB standard
connection string (URI).
- `write_concern` `(string: "")` - Specifies the MongoDB [write
concern][mongodb-write-concern]. This is set for the entirety of the session,
maintained for the lifecycle of the plugin process. Must be a serialized JSON
object, or a base64-encoded serialized JSON object. The JSON payload values
map to the values in the [Safe][mgo-safe] struct from the mgo driver.

### Sample Payload

```json
{
"plugin_name": "mongodb-database-plugin",
"allowed_roles": "readonly",
"connection_url": "mongodb://admin:[email protected]:27017/admin?ssl=true"
"connection_url": "mongodb://admin:[email protected]:27017/admin?ssl=true",
"write_concern": "{ \"wmode\": \"majority\", \"wtimeout\": 5000 }"
}
```

Expand Down Expand Up @@ -68,7 +76,7 @@ list the plugin does not support that statement type.
[MongoDB's documentation](https://docs.mongodb.com/manual/reference/method/db.createUser/).

- `revocation_statements` `(string: "")` – Specifies the database statements to
be executed to revoke a user. Must be a serialized JSON object, or a base64-encoded
be executed to revoke a user. Must be a serialized JSON object, or a base64-encoded
serialized JSON object. The object can optionally contain a "db" string. If no
"db" value is provided, it defaults to the "admin" database.

Expand All @@ -84,4 +92,7 @@ list the plugin does not support that statement type.
}
]
}
```
```

[mongodb-write-concern]: https://docs.mongodb.com/manual/reference/write-concern/
[mgo-safe]: https://godoc.org/gopkg.in/mgo.v2#Safe