Mongodb plugin #2698
Mongodb plugin #2698
Conversation
| { | ||
| "plugin_name": "mongodb-database-plugin", | ||
| "allowed_roles": "readonly", | ||
| "uri": "mongodb://admin:Password!@mongodb.acme.com:27017/admin?ssl=true" |
There was a problem hiding this comment.
Can we name this connection_url to match the other plugin types?
There was a problem hiding this comment.
Sure, I kept it as uri since that's the parameter name for the current secret backend, but it makes sense to change it to match the other DBs.
|
|
||
| func (cp *MongoDBCredentialsProducer) GenerateExpiration(ttl time.Time) (string, error) { | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
This is essentially the same as the cassandra one, what if we made a generic CredentialsProducer that will work for both?
There was a problem hiding this comment.
GenerateUsername() constructs the username differently though.
| } | ||
|
|
||
| return &info, nil | ||
| } |
There was a problem hiding this comment.
Do you think one-off connection producers like this should live in the helpers folder or in the e.g. database/mongo folder? If they are not going to be used by more than one plugin I'm not sure they need to live in helpers. Open to a discussion and if we decide to move it the cassandra one should probably be moved as well.
There was a problem hiding this comment.
The potential problem I see is that this will produce tighter coupling and a potential dependency cycle since ParseMongoURI is used in both connutil and in plugins/database/mongodb. Even though ParseMongoURI() is only used my one plugin database, connutil has not context (i.e. imports) of the plugin package, which I think is a better tradeoff than creating a bidirectional dependency.
There was a problem hiding this comment.
But there is no reason connutil will need to use ParseMongoURI() if this whole file is moved to the plugins/database/mongodb package, right? If moved, I don't think there will be a circular dependency
There was a problem hiding this comment.
Oh, I see. I thought you meant the one function, ParseMongoURI.
There was a problem hiding this comment.
Ah, yeah sorry. I meant the whole file.
There was a problem hiding this comment.
I wouldn't be opposed to moving it, but then we would have some connection producers that live within the plugin package, and some in the connutil package (e.g. sql.go). I guess the same would apply for credsutil then (moving it to the plugin vs keeping it in credsutil)?
plugins/database/mongodb/mongodb.go
Outdated
| } | ||
| if statements.CreationStatements == "" { | ||
| return "", "", dbutil.ErrEmptyCreationStatement | ||
| } |
There was a problem hiding this comment.
Can you move this check to the top? No need to connect to the database if there is no statement provided.
|
@briankassouf this should be ready for another review. |
plugins/database/mongodb/util.go
Outdated
| // [ "readWrite", { "role": "readWrite", "db": "test" } ] | ||
| // | ||
| // MongoDB's createUser command accepts the latter. | ||
| // |
There was a problem hiding this comment.
Could you move this comment to above the function?
plugins/database/mongodb/util.go
Outdated
| func (roles mongodbRoles) toStandardRolesArray() []interface{} { | ||
| // Convert array of role documents like: | ||
| // | ||
| // [ { "role": "readWrite" }, { "role": "readWrite", "db": "test" } ] |
There was a problem hiding this comment.
If there is no db set here, does it give permissions to the db specified by the role, or does it give permissions to every db?
There was a problem hiding this comment.
In this case the role is applied to the session's database, which will be the "db" string value from the creation_statement's JSON, or "admin" if none is provided.
https://docs.mongodb.com/manual/reference/method/db.createUser/
To specify a role that exists in a different database, specify the role with a document.
| --request POST \ | ||
| --data @payload.json \ | ||
| https://vault.rocks/v1/database/config/mongodb | ||
| ``` |
There was a problem hiding this comment.
Let's add some specifics about the statements that are required down here. Like in #2718
No description provided.