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

feat: Allow MongoDb module configuration without credentials #983

Merged

Conversation

the-avid-engineer
Copy link
Contributor

@the-avid-engineer the-avid-engineer commented Aug 28, 2023

What does this PR do?

  1. Do not require a username/password for mongodb
  2. Allow for disabling default credentials for mongodb

Why is it important?

This addresses a regression on #547 as well as adding the ability to not use default credentials; when using credentials in replica set mode, there are additional set up pieces that are tedious; it's 1000x easier to run in replica set mode without credentials. Replica set mode allows users to use transactions

Related issues


I would love to add a test to prevent another regression, but on my last PR I wasn't able to run the tests locally, and I suspect I still cannot run them

@netlify
Copy link

netlify bot commented Aug 28, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit f470e4b
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/64ef8d53ebdaca0008edc9ed
😎 Deploy Preview https://deploy-preview-983--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Thanks for your PR 👍.

This addresses a regression on #547 as well as adding the ability to not use default credentials;

Oh, looks like I missed that requirement during the migration to dedicated modules, thanks.

src/Testcontainers.MongoDb/MongoDbBuilder.cs Outdated Show resolved Hide resolved
src/Testcontainers.MongoDb/MongoDbBuilder.cs Outdated Show resolved Hide resolved
@HofmeisterAn
Copy link
Collaborator

We must ensure that the tests also pass (_ = new MongoDbBuilder(false).Build()). The container instance is capable of executing a MongoDB shell command that relies on the credentials, too. Perhaps we can replace a null string with an empty one.

[Fact]
[Trait(nameof(DockerCli.DockerPlatform), nameof(DockerCli.DockerPlatform.Linux))]
public async Task ExecScriptReturnsSuccessful()
{
// Given
const string scriptContent = "printjson(db.adminCommand({listDatabases:1,nameOnly:true,filter:{\"name\":/^admin/}}));";
// When
var execResult = await _mongoDbContainer.ExecScriptAsync(scriptContent)
.ConfigureAwait(false);
// When
Assert.True(0L.Equals(execResult.ExitCode), execResult.Stderr);
}

@the-avid-engineer
Copy link
Contributor Author

the-avid-engineer commented Aug 30, 2023

We must ensure that the tests also pass (_ = new MongoDbBuilder(false).Build()). The container instance is capable of executing a MongoDB shell command that relies on the credentials, too. Perhaps we can replace a null string with an empty one.

I don't have a Linux box (I'm assuming it only works on Linux because the test has a Linux trait), so I went with the best bet that not passing those options when username/password are null will certainly work as expected, and I'm not opposed to passing empty strings if that works

@HofmeisterAn
Copy link
Collaborator

HofmeisterAn commented Aug 30, 2023

I extended the tests and executed them against MongoDbBuilder(false), resulting in failures. Afterward, I made some adjustments to the implementation. It appears that we can keep non-nullable while achieving a configuration with no authentication by utilizing empty strings. Kindly share your thoughts on this matter. I am not a MongoDB expert, but curious why MongoDB logs different messages according to the configuration (user vs. w/o user). Probably something you ran into.

We can add something like WithNoAuth() to offer a more explicit API.

@the-avid-engineer
Copy link
Contributor Author

I do know that mongodb://:@localhost:12345 is not a valid connection string, so it won't work there

I will check the other changes against replica set mode

@HofmeisterAn
Copy link
Collaborator

I do know that mongodb://:@localhost:12345 is not a valid connection string, so it won't work there

That's not what GetConnectionString() returns. If you run the MongoDbNoAuthConfiguration test you will see it returns

mongodb://127.0.0.1:56789/

If the username and password is empty it will not be included in the connection string.

@HofmeisterAn
Copy link
Collaborator

I will check the other changes against replica set mode

@the-avid-engineer have you had the chance to test the changes in replica set mode? I would like to prepare a new release, and ideal include the changes from this pull request.

@the-avid-engineer
Copy link
Contributor Author

Sorry, I was preoccupied and not able to test yet. I will do so later tonight

That is good that the connection string is correct, I only assumed it would not handle empty string the same as null

@the-avid-engineer
Copy link
Contributor Author

@HofmeisterAn I can confirm the empty strings work fine with replica set mode :)

@HofmeisterAn HofmeisterAn changed the title feat: MongoDb no-authentication mode feat: Allow MongoDb module configuration without credentials Sep 6, 2023
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time 🙏.

@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Sep 6, 2023
@HofmeisterAn HofmeisterAn merged commit 59991ec into testcontainers:develop Sep 6, 2023
10 checks passed
@ConMD
Copy link

ConMD commented Feb 28, 2024

Hi everyone,

hope this is the right place to ask this question:
When will this PR be merged into main?

Would be highly appreciated, if it happens soon :)

@HofmeisterAn
Copy link
Collaborator

This has already been merged into the main branch and is released.

public sealed class MongoDbNoAuthConfiguration : MongoDbContainerTest
{
public MongoDbNoAuthConfiguration()
: base(new MongoDbBuilder().WithUsername(string.Empty).WithPassword(string.Empty).Build())
{
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Explicit no-authentication mode for MongoDb
3 participants