testcontainers-dotnet icon indicating copy to clipboard operation
testcontainers-dotnet copied to clipboard

Integrate an option for `TrustServerCertificate` on MsSql options

Open pdevito3 opened this issue 2 years ago • 8 comments

Is your feature request related to a problem? Please describe. As of SqlClient v4, there is a breaking change to have encrypt set to true by default. Currently, the connection string output by the TestcontainerDatabase does not have this option enabled or an ability to toggle it on.

Describe the solution you'd like Given that these dbs are for testing only, i would think adding TrustServerCertificate=true; on all sql server connection strings would be reasonable, but at least adding a toggle for it somehow would be helpful.

Describe alternatives you've considered Currently, i need to do something like this:

    private readonly  TestcontainerDatabase _dbContainer = IsRunningOnMacOsArm64() 
        ? new TestcontainersBuilder<MsSqlTestcontainer>()
            .WithDatabase(new MsSqlTestcontainerConfiguration()
            {
                Password = "#testingDockerPassword#",
            })
            .WithName($"IntegrationTesting_RecipeManagement_{Guid.NewGuid()}")
            .Build()
        : new TestcontainersBuilder<MsSqlTestcontainer>()
            .WithDatabase(new MsSqlTestcontainerConfiguration()
            {
                Password = "#testingDockerPassword#",
            })
            .WithName($"IntegrationTesting_RecipeManagement_{Guid.NewGuid()}")
            .WithImage("mcr.microsoft.com/azure-sql-edge:latest")
            .Build();

    public async Task InitializeAsync()
    {
        await _dbContainer.StartAsync();
        Environment.SetEnvironmentVariable("DB_CONNECTION_STRING", $"{_dbContainer.ConnectionString}TrustServerCertificate=true;");

Open to submitting a PR for this if you're low on bandwidth. Thanks!

pdevito3 avatar Jun 11 '22 18:06 pdevito3

What do you think about removing the sealed modifier from the HostedServiceContainer implementations like MsSqlTestcontainer? This allows to override MsSqlTestcontainer. Then you can do something like:

  private const string MsSqlX86DockerImage = "";

  private const string MsSqlArmDockerImage = "";

  private readonly IDockerContainer container;

  public GitHubIssue474()
  {
    var dockerImage = IsRunningOnM1() ? MsSqlArmDockerImage : MsSqlX86DockerImage;
    this.container = new TestcontainersBuilder<MsSqlRecipeManagement>()
      .WithName($"integration-test-recipe-management-{Guid.NewGuid():D}")
      .WithDatabase(new MsSqlTestcontainerConfiguration(dockerImage)
      {
        Password = "L5hPJzmuE3PGtTxNTw7TzH4k2",
      })
      .Build();
  }

  public Task InitializeAsync()
  {
    return this.container.StartAsync();
  }

  public Task DisposeAsync()
  {
    return this.container.DisposeAsync().AsTask();
  }

  private static bool IsRunningOnM1()
  {
    return true;
  }
}

public sealed class MsSqlRecipeManagement : MsSqlTestcontainer
{
  public MsSqlRecipeManagement(ITestcontainersConfiguration configuration, ILogger logger)
    : base(configuration, logger)
  {
  }

  public override string ConnectionString { get; }
    = string.Empty; // TODO: Set your connection string.
}
diff --git a/src/DotNet.Testcontainers/Containers/Modules/Databases/MsSqlTestcontainer.cs b/src/DotNet.Testcontainers/Containers/Modules/Databases/MsSqlTestcontainer.cs
index 0779950..5b7a361 100644
--- a/src/DotNet.Testcontainers/Containers/Modules/Databases/MsSqlTestcontainer.cs
+++ b/src/DotNet.Testcontainers/Containers/Modules/Databases/MsSqlTestcontainer.cs
@@ -9,14 +9,14 @@ namespace DotNet.Testcontainers.Containers
 
   /// <inheritdoc cref="TestcontainerDatabase" />
   [PublicAPI]
-  public sealed class MsSqlTestcontainer : TestcontainerDatabase
+  public class MsSqlTestcontainer : TestcontainerDatabase
   {
     /// <summary>
     /// Initializes a new instance of the <see cref="MsSqlTestcontainer" /> class.
     /// </summary>
     /// <param name="configuration">The Testcontainers configuration.</param>
     /// <param name="logger">The logger.</param>
-    internal MsSqlTestcontainer(ITestcontainersConfiguration configuration, ILogger logger)
+    protected MsSqlTestcontainer(ITestcontainersConfiguration configuration, ILogger logger)
       : base(configuration, logger)
     {
     }

Probably we need to take a look into the reflection call too (BindingFlags).

HofmeisterAn avatar Jun 12 '22 05:06 HofmeisterAn

hmmm, yeah i can buy that in my use case given the extra stuff i'm doing to use edge to account for M1 chips, but some might not care and just want to have the connection string work by default when using sql client v4+.

so i'd personally lean having TrustServerCertificate=true; included by default for those that only need that much to get it working (sorry that portion of the code example is overshown by my M1 logic), but also having this overriding capability for customization's like the edge override for M1s

pdevito3 avatar Jun 12 '22 17:06 pdevito3

Yes, we can do both, but just adding TrustServerCertificate is not enough IMO. If you need any other option in the future, you'll have the same issue again 😀.

HofmeisterAn avatar Jun 13 '22 18:06 HofmeisterAn

yup, agreed :-)

pdevito3 avatar Jun 13 '22 18:06 pdevito3

I used this workaround:

SqlConnectionStringBuilder sqlConnectionStringBuilder =
                new SqlConnectionStringBuilder(Testcontainers.ConnectionString);

sqlConnectionStringBuilder.TrustServerCertificate = true;
sqlConnectionStringBuilder.InitialCatalog = "my stuff";
//etc
string connectionString = sqlConnectionStringBuilder.ConnectionString;

then use the new connectionString in my tests.

Wondered if it would be an option for the Testcontainers to have properties for eg host, port, etc in addition to a 'generic' connection string.

p1971 avatar Jul 28 '22 18:07 p1971

Wondered if it would be an option for the Testcontainers to have properties for eg host, port, etc in addition to a 'generic' connection string.

What do you mean by that? There is a Hostname, Port, etc. property.

HofmeisterAn avatar Jul 30 '22 17:07 HofmeisterAn

Wondered if it would be an option for the Testcontainers to have properties for eg host, port, etc in addition to a 'generic' connection string.

What do you mean by that? There is a Hostname, Port, etc. property.

Good point - think I saw that originally and forgot when I replied - I was thinking more generically - any property that forms part of the connection string should be available such that the dev can create their own custom connection string if the auto-generated one doesn't quite meet the requirements (eg for adding additional options)

In my example I was able to use the SqlConnectionStringBuilder to parse the autogenerated one and add additional properties, other containers might need more meta-data to re-create a connection string. I can't think of any concrete examples mind!

p1971 avatar Aug 01 '22 15:08 p1971

In case of MsSqlTestcontainerConfiguration some config options, e.g. Database, throw NotImplementedException.

redzimskidev avatar Aug 03 '22 07:08 redzimskidev

In case of MsSqlTestcontainerConfiguration some config options, e.g. Database, throw NotImplementedException.

This is a limitation of the MSSQL Docker image. Not all images support all kinds of configuration. Although we support it now with the upcoming 2.2.0.

I was thinking more generically - any property that forms part of the connection string should be available such that the dev can create their own custom connection string if the auto-generated one doesn't quite meet the requirements

I agree. The new module structure offers more and convention ways to configure containers (accessing properties). I am done by the builder. Right now, I am looking into ways to pass the configurations along the builders. Immutability makes it a bit difficult and cumbersome... I hope I am done with the proposal in the next days.

HofmeisterAn avatar Sep 27 '22 09:09 HofmeisterAn

Any updates here? I came across the same problem (MySql and OldGuids=true) and I know that @paule96 also has it. We thought the best option would be to refactor the Testcontainer to use SqlConnectionStringBuilder under the hood (instead of the custom string concatenation currently e.g: ). https://github.com/testcontainers/testcontainers-dotnet/blob/a5d45f88fbe78c2b831bd5b4c5bdc2ca5a1d56cc/src/Testcontainers/Containers/Modules/Databases/MySqlTestcontainer.cs#L25-L26 This way we could build and modify the ConnectionString as needed.

This would hopefully enable an Extension Method for TestcontainersBuilder like:

_container = new TestcontainersBuilder<MySqlTestcontainer>()
            .WithDatabase(new MySqlTestcontainerConfiguration { Database = "db", Username = "root", Password = "example" })
            .WithImage("mysql:8.0.29")
            // Should only be called after "WithDatabase"
            .ModifyConnectionString(connectionStringBuilder => { // supply current SqlConnectionStringBuilder
                // Modify as needed
                builder["Connect Timeout"] = 1000;
                builder["Trusted_Connection"] = true;
            })
            .Build();
// Modifications are saved and can be retrieved later via the existing interface
connectionString = _container.ConnectionString

DanielHabenicht avatar Oct 24 '22 11:10 DanielHabenicht

SqlConnectionStringBuilder is not part of .NET Standard, but with the new module structure definitely possible and something I am looking forward too. Unfortunately, the PR is on hold due to all the other ongoing topics. It is something I am aware off and hopefully can sort it out soon.

HofmeisterAn avatar Oct 24 '22 11:10 HofmeisterAn

@HofmeisterAn maybe it's better to implement an simple builder by yourself. It's basically only a key value list divided by ';'. The only hard thing todo, is always escaping inside the key values.

That should be doable by the current structure of the project.

paule96 avatar Oct 24 '22 12:10 paule96

If someone would like to work on that, we can certainly add it. Please create an issue if you like. I would favor an implementation that covers multiple database provides.

HofmeisterAn avatar Oct 25 '22 08:10 HofmeisterAn

Okay I looked now a bit to the code, and now I know what @HofmeisterAn mean by different database providers. Providers that use as a connection string an URI schema and not an Connectionstring schema. I will think a bit more about it, the rest of the week.

For everyone who is interested to find a solution too interesting connection strings are the following:

  • normal ADO Connectionstring, Key/Value devided by = and each pair by ;
    • MySql for example: Server={this.Hostname};Port={this.Port};Database={this.Database};Uid={this.Username};Pwd={this.Password};
  • URI based Connectionstrings are using the schema known by SSH or FTP. Wher the user and the password can be part of the URI. Usally divived by @
    • CouchDb for example: http://{this.Username}:{this.Password}@{this.Hostname}:{this.Port}

I don't even know how additional connection options will work with the URI based connection schema. So I will see what I can find out at the next weekend.

First edit:

Based on the java sdk docs, it looks like any additional parameter for uri based schemas are applied via URI parameters: https://docs.couchbase.com/java-sdk/current/howtos/managing-connections.html#connection-strings

paule96 avatar Oct 26 '22 17:10 paule96

@HofmeisterAn would it be possible to split the Database Classes in classes wich support ADO.NET connectionstrings and the others? That would simplify the feature request?

paule96 avatar Oct 26 '22 17:10 paule96

would it be possible to split the Database Classes in classes wich support ADO.NET connectionstrings and the others?

Do you mean connection strings that are key value pairs and delimited by ;? Or databases that are compatible with ADO.NET (have a OLE DB provider)?

HofmeisterAn avatar Oct 27 '22 11:10 HofmeisterAn

@HofmeisterAn yes you are right. I mean more the Syntax of semicolon delimited connection strings.

So at the end to provide two implementations. One for URI based connection strings and one for the semicolon delimited.

And if this is not enough the current fallback to manipulate the builded connection string is still possible.

paule96 avatar Oct 27 '22 16:10 paule96

OC, if it makes the development experience better and more convenient lets add it. We do not need to support both types immediately (URIs usually do not have any options anyway). For those we can probably use the UriBuilder. I just want to point out, that we are more flexible with the dedicated modules in the future. E.g. we can just add System.Data.SqlClient and use SqlConnectionStringBuilder.

HofmeisterAn avatar Oct 27 '22 19:10 HofmeisterAn

Okay finally after some weeks I was able to catch a bit of my free time to implement a first version of it. It would be nice if @HofmeisterAn could review my commit a bit. Just to know if I'm on the right path.

https://github.com/paule96/testcontainers-dotnet-issues-474/commit/7fd7cf045accad02506970be765d5c02ca0b60e7

If this is good I will try to extend the configuation interface with the new options.

paule96 avatar Nov 20 '22 14:11 paule96

With the latest release (3.0.0), we set the mentioned property for the MsSql and SqlEdge module. If you require additional capabilities to configure the connection string more conveniently, please consider creating a follow-up issue or participating in the following discussion. I will close this issue since, as mentioned, the property is now set.

HofmeisterAn avatar Mar 08 '23 10:03 HofmeisterAn