-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add auth to mongo db container #5595
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
Closed
silaev
wants to merge
68
commits into
testcontainers:main
from
silaev:add_auth_to_mongo_db_container
+213
−11
Closed
Changes from all commits
Commits
Show all changes
68 commits
Select commit
Hold shift + click to select a range
3c07308
Implement MongoDbContainer
silaev a2c5c69
change oraclejdk8 with openjdk11, delete obsolete ITTest
silaev 8ddd6b4
Small fixes
silaev 1795235
Fix docs
silaev ee5e4fa
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev e06749d
fix mongodb.md
silaev 1a2b1dc
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev 8f68d71
Code review remarks adjusted
silaev 58ab3f2
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev 957cd51
Small fix mongodb.md
silaev 991a5f8
Small fix mongodb.md
silaev 4d9f360
Merge branch 'master' into master
rnorth 72d2b30
Merge branch 'master' into master
rnorth c0fe7ba
Merge branch 'master' into master
rnorth 509a454
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev d1d0367
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev 184d4e2
Fix container exit code check
silaev fc45144
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev cfe5177
Adjust tests and build.gradle to follow Testcontainers module test co…
silaev a74d766
Add Javadoc explaining the use of LOCALHOST
silaev 7620cb8
Refactor to use a default replica set configuration
silaev 4ca7c7b
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev aad0e76
Remove extra tests
silaev b92d690
Remove extra unit tests
silaev 9a746df
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev 5c3182e
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev 5f3145b
Remove MockMaker
silaev 2ea01ce
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev 8b727f0
Move initReplicaSet to containerIsStarted, set timeout delay to 100ms…
silaev e714b26
Make initReplicaSet private
silaev 5b5531d
Fix docs
silaev d892006
Fix docs
silaev 7adc9f8
Fix docs
silaev c136592
Make mongoDbContainer final in test
silaev a773cc5
Fix docs
silaev fb325d4
Fix docs, change MongoDb with MongoDB, remove logReplicaSetStatus
silaev 649dae2
Reuse secondary constructor
silaev cd23ddb
Adjust heading
silaev 47c39e7
Remove configureMongoDBContainer
silaev 8e57d07
Merge branch 'master' into master
rnorth 78ad147
Update docs/modules/databases/mongodb.md
rnorth 7cdbad4
Merge branch 'master' into master
rnorth 6ba608e
Merge branch 'master' of https://github.com/testcontainers/testcontai…
silaev 7a27fd1
Merge remote-tracking branch 'upstream/master'
silaev 49682c2
Add Authentication to MongoDBContainer
silaev e97ed90
Adjust to code style
silaev 0972312
Adjust to code style
silaev dac9070
Enable logs
silaev ae49077
Downgrade mongodb version for shouldTestAuthentication test
silaev 79267b0
Show logs
silaev a498102
Rename vars for shouldTestAuthentication test
silaev 42ef9e7
Clean up logs
silaev b61763c
Merge remote-tracking branch 'upstream/master'
silaev 9ff0da8
Merge branch 'master' into add_auth_to_mongo_db_container
silaev e5fda5e
Merge remote-tracking branch 'upstream/master'
silaev 69fb8cc
Merge branch 'master' into add_auth_to_mongo_db_container
silaev 4e2ebc5
Fix keyFile permissions
silaev 4441a2e
Fix CheckStyle
silaev 97c0512
Change getDefaultConnectionString name with constructConnectionString…
silaev eb9eb2b
Merge remote-tracking branch 'upstream/master'
silaev 779f5fc
Merge branch 'master' into add_auth_to_mongo_db_container
silaev bdd7e82
Merge upstream
silaev 827a6a4
Prettify shouldTestAuthentication
silaev 1c01a11
Merge remote-tracking branch 'upstream/main'
silaev 6e54adb
Merge branch 'master' into add_auth_to_mongo_db_container
silaev 47138ac
Resovle conflicts
silaev c76ddd1
Code review remarks adjusted
silaev 72bd52b
Code review remarks adjusted
silaev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| privateOldE8+8pPloNORolrRpGU6E6Ps8rh6GAcThtIfnh0Nfiy+fGvROwqlEtFmAY4Xb6 | ||
| aDjb/cFoYSNIxh5DZaBD4hmllCkoAAl15WDKWEv+ELxj124KiyuzJUbu50iXYG+/ | ||
| g6oWzElAdnckXCj+9CVhuw1dq9LgaIOd3n7NsrKK4rG7BgrdRl3HSpexBgd4WPva | ||
| jtIcvg+eKBvUysACGSpCubFQa1VoRiU7G0h5TYPXVBpmvN7cFHKANNKyggDPKlih | ||
| SfnMuXPGaecBm9UkmpHJoPUuzrE5wXStbho6SQzlbSBoxxgOCAHaAPtn7d3scP1i | ||
| lk8hoIyNjbq37D2b15VP9+JxBgkqywjcm3Z7D5m+NSI22xYD44kNBxvAIFUtE1RS | ||
| qgTFizA2ORb73TGfhhy2vuIJdsn97dZAMFOayiJvdzyIQJ9027d5eAVUE/U9UQjP | ||
| 7BrHrJ+iV+PwggmIvwXDjFP7n0gs6tGmghfG/13y3lwpD+Xs84hcEbXitdns+8dE | ||
| lpnTkqUpGMexeuEuL4O5yfX46mVyT6+qvD+jb6y5oB1ydP/n3dmuWfoE3hv2rvVn | ||
| pFbPzuTF2mvIj0HTTmkNBCBh8Rq7McZ2vNW5nx3jdf8A+ICw6O9KlkemhHORIsIY | ||
| /HbsL1xjPs+gizMOddFwgfLovkQ9Oap7fAed+yl8JxqTWe5OMHZCDxyssZEOscnZ | ||
| xSWEXKWsWv2LLqtIdc++ZqrkvMWHNVqILcpe2upb7DbCVMjrsv5htXrYL7lgaItm | ||
| MpyP20q4ut7ja0YRwPITJyHNacJygAE/TViTL3K1JNXKHXCLWfIHGkVhYzc/9uv7 | ||
| 0nu5el6crurO57rQFC3T14huEQvouZl9SmHflkBFF7/kQeAJj10bmZWYae8mhdhb | ||
| zaJ2tUcsEgxEaZGrVK4f1NRAwBBY5t5AZMwLcYyIY1F1YZxw2BmrnPqr3GgSCcQ5 | ||
| eMo3u9jKXXYJ6Eb3xwNpFXaGFPS1TmTK9y1CU8fXfDOsr7ln |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,8 @@ | ||||||||||||||||||||||
| package org.testcontainers.containers; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import com.mongodb.BasicDBObject; | ||||||||||||||||||||||
| import com.mongodb.ConnectionString; | ||||||||||||||||||||||
| import com.mongodb.MongoCommandException; | ||||||||||||||||||||||
| import com.mongodb.ReadConcern; | ||||||||||||||||||||||
| import com.mongodb.ReadPreference; | ||||||||||||||||||||||
| import com.mongodb.TransactionOptions; | ||||||||||||||||||||||
|
|
@@ -8,12 +11,17 @@ | |||||||||||||||||||||
| import com.mongodb.client.MongoClient; | ||||||||||||||||||||||
| import com.mongodb.client.MongoClients; | ||||||||||||||||||||||
| import com.mongodb.client.MongoCollection; | ||||||||||||||||||||||
| import com.mongodb.client.MongoDatabase; | ||||||||||||||||||||||
| import com.mongodb.client.TransactionBody; | ||||||||||||||||||||||
| import org.bson.Document; | ||||||||||||||||||||||
| import org.junit.Test; | ||||||||||||||||||||||
| import org.testcontainers.utility.DockerImageName; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import java.util.Collections; | ||||||||||||||||||||||
| import java.util.Objects; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import static org.assertj.core.api.Assertions.assertThat; | ||||||||||||||||||||||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public class MongoDBContainerTest { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -98,7 +106,8 @@ public void shouldTestDatabaseName() { | |||||||||||||||||||||
| try (final MongoDBContainer mongoDBContainer = new MongoDBContainer(DockerImageName.parse("mongo:4.0.10"))) { | ||||||||||||||||||||||
| mongoDBContainer.start(); | ||||||||||||||||||||||
| final String databaseName = "my-db"; | ||||||||||||||||||||||
| assertThat(mongoDBContainer.getReplicaSetUrl(databaseName)).endsWith(databaseName); | ||||||||||||||||||||||
| assertThat(databaseName) | ||||||||||||||||||||||
| .isEqualTo(new ConnectionString(mongoDBContainer.getReplicaSetUrl(databaseName)).getDatabase()); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -109,4 +118,73 @@ public void supportsMongoDB_6() { | |||||||||||||||||||||
| executeTx(mongoDBContainer); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @Test | ||||||||||||||||||||||
| public void shouldTestAuthenticationAccessControl() { | ||||||||||||||||||||||
| final String usernameFullAccess = "my-name"; | ||||||||||||||||||||||
| final String passwordFullAccess = "my-pass"; | ||||||||||||||||||||||
| try ( | ||||||||||||||||||||||
| final MongoDBContainer mongoDBContainer = new MongoDBContainer(DockerImageName.parse("mongo:4.4")) | ||||||||||||||||||||||
| .withUsername(usernameFullAccess) | ||||||||||||||||||||||
| .withPassword(passwordFullAccess) | ||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
| mongoDBContainer.start(); | ||||||||||||||||||||||
| final ConnectionString connectionStringFullAccess = new ConnectionString( | ||||||||||||||||||||||
| mongoDBContainer.getReplicaSetUrl() | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| try (final MongoClient mongoSyncClientFullAccess = MongoClients.create(connectionStringFullAccess)) { | ||||||||||||||||||||||
| final MongoDatabase adminDatabase = mongoSyncClientFullAccess.getDatabase( | ||||||||||||||||||||||
| MongoDBContainer.DEFAULT_AUTHENTICATION_DATABASE_NAME | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| final MongoDatabase testDatabaseFullAccess = mongoSyncClientFullAccess.getDatabase( | ||||||||||||||||||||||
| MongoDBContainer.DEFAULT_DATABASE_NAME | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| final String collectionName = "my-collection"; | ||||||||||||||||||||||
| final Document document1 = new Document("abc", 1); | ||||||||||||||||||||||
| testDatabaseFullAccess.getCollection(collectionName).insertOne(document1); | ||||||||||||||||||||||
| final String usernameRestrictedAccess = usernameFullAccess + "-restricted"; | ||||||||||||||||||||||
| final String passwordRestrictedAccess = passwordFullAccess + "-restricted"; | ||||||||||||||||||||||
| runCommand( | ||||||||||||||||||||||
| adminDatabase, | ||||||||||||||||||||||
| new BasicDBObject("createUser", usernameRestrictedAccess).append("pwd", passwordRestrictedAccess), | ||||||||||||||||||||||
| "read" | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| try ( | ||||||||||||||||||||||
| final MongoClient mongoSyncRestrictedAccess = MongoClients.create( | ||||||||||||||||||||||
| mongoDBContainer.getReplicaSetUrl( | ||||||||||||||||||||||
| MongoDBContainer.ConnectionString | ||||||||||||||||||||||
| .builder() | ||||||||||||||||||||||
| .username(usernameRestrictedAccess) | ||||||||||||||||||||||
| .password(passwordRestrictedAccess) | ||||||||||||||||||||||
| .build() | ||||||||||||||||||||||
|
Comment on lines
+154
to
+159
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pls, see the above mentioned discussion about |
||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
| final MongoCollection<Document> collection = mongoSyncRestrictedAccess | ||||||||||||||||||||||
| .getDatabase(MongoDBContainer.DEFAULT_DATABASE_NAME) | ||||||||||||||||||||||
| .getCollection(collectionName); | ||||||||||||||||||||||
| assertThat(collection.find().first()).isEqualTo(document1); | ||||||||||||||||||||||
| final Document document2 = new Document("abc", 2); | ||||||||||||||||||||||
| assertThatThrownBy(() -> collection.insertOne(document2)).isInstanceOf(MongoCommandException.class); | ||||||||||||||||||||||
| runCommand(adminDatabase, new BasicDBObject("updateUser", usernameRestrictedAccess), "readWrite"); | ||||||||||||||||||||||
| collection.insertOne(document2); | ||||||||||||||||||||||
| assertThat(collection.countDocuments()).isEqualTo(2); | ||||||||||||||||||||||
| assertThat(connectionStringFullAccess.getUsername()).isEqualTo(usernameFullAccess); | ||||||||||||||||||||||
| assertThat(new String(Objects.requireNonNull(connectionStringFullAccess.getPassword()))) | ||||||||||||||||||||||
| .isEqualTo(passwordFullAccess); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| private void runCommand(MongoDatabase adminDatabase, BasicDBObject command, String role) { | ||||||||||||||||||||||
| adminDatabase.runCommand( | ||||||||||||||||||||||
| command.append( | ||||||||||||||||||||||
| "roles", | ||||||||||||||||||||||
| Collections.singletonList( | ||||||||||||||||||||||
| new BasicDBObject("role", role).append("db", MongoDBContainer.DEFAULT_DATABASE_NAME) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mongo already provides a convinient way to create a client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that it's a good idea here because:
Exposing
MongoClientSettingsto the public API seems to be dangerous because a user might set a SRV DNS or other settings thatMongoDBContainerdoes not support. I'd rather not allow a user to construct a URL on their side, it's safer to do delegate it toMongoDBContainerproviding a URL string. Otherwise, we might receive some unexpected bug tickets here;ConnectionStringis for the public API purpose to expose onlydatabaseName,usernameandpasswordfor now. As apposed toMongoClientSettings, it doesn't require setting auth db source, database etc. which simplifies the public API and takes control of itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not proposing to use Mongo API in MongoDBContainer. Just sharing how it can be done from the outside as in this suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I do not expect users to do that but who knows.
I think our docs can be improved in order to make it clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We provide a URL string for a user via clear API that we take control of. I reckon it's not a good idea to constantly update docs mentioning "it does not support srv etc."? In my opionin, it's better not to provide a way to set srv.
I see your point, but it's not a part of our public API. This way a user can construct everything, but having an issue with it is on their hands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated Java doc to make it clear
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @silaev, but it is still unclear with me what is the issue with the approach suggested by @eddumelendez, since it is very close to how we do it for other modules.
So you think users will struggle doing it this way and construct invalid URLs? Especially since users would normally use
getConnectionString(). Or maybe I am missing the point of the discussion?I thought @eddumelendez's point was about not exposing
MongoDBContainer.ConnectionStringas a public API.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kiview The idea of this builder is to provide 3 params for now (might be more in the future): username, pass and db name for MongoDBContainer to generate a URL as simple as this:
It’s similar to withUsername(...), withPassword(…) and withDatabaseName(...) etc. builder methods in the PostgreSQLContainer to generate a URL via getJdbcUrl(). As I understand, generating URL inside a container is
close to how we do it for other modules. There is alreadypublic String getReplicaSetUrl(final String databaseName) {}, so I followed the same approach with a param herepublic String getReplicaSetUrl(final ConnectionString connectionString).@eddumelendez proposed a solution when a URL is generated outside of MongoDBContainer via MongoClientSettings which we do not control. Is it
close to how we do it for other modules? This way a user has to construct a URL on their own and know some additional data:The second variant seems to me as quite complex and error prone, so I'm more comfortable with the first one. Correct me if I'm wrong, but the whole idea of Testcontainers is to simplify testing experience which, as I see it, the first variant follows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kiview , @eddumelendez
I haven’t heard back from you after my last comment. Did I have a change to convince you to go with the simple builder approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for ping us, I was thinking about it last week. There is a difference with the builders in database modules due to those are used by
JdbcDatabaseContainerfor the specific wait strategy and, well, JDBC API is in the JDK itself.With latest changes in the
MongoDbContainer, I would suggest the following:withAuthenticationEnable(), instead of making it the default. Unless, a specific MongoDB version is forcing it, we can check on that version.withUsernameandwithPasswordas initially provided in the PRkeyfile. So, the user is aware of what's involved to run MongoDB with authentication enabled.Testcontainers responsibility is to provide a ready to use service running in a container. There could be many configuration in the service itself which we do not master and we prefer a generic approach, meanwhile we are learning from uses cases, rather than do something at the beginning that can change later just because we are not aware of it.
I know the answer doesn't answer the last question but I think it is our best effort, meanwhile we also learn about MongoDB and how to use it. So, the builder can be revisited in the future. I'm also aware that this will involve more changes than initially expected but it is due to the evolution of the module itself.