Add Qdrant container #994
Add Qdrant container #994
Conversation
This commit adds a qdrant container to the list of supported Testcontainers. The qdrant container allows configuration of: - an API key to authenticate to Qdrant - an x509 certificate used to secure communication to Qdrant with Transport Layer Security - a custom configuration file. See https://qdrant.tech/documentation/guides/configuration/ Closes testcontainers#992
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Qdrant uses rustls for TLS, which does not accept IPv4 or IPv6 addresses as the hostname in SNI, adhering to RFC 6066: https://www.rfc-editor.org/rfc/rfc6066#page-7 Sending an IPv4 or IPv6 address as the hostname in SNI causes the handshake to be rejected and qdrant to log WARN rustls::msgs::handshake: Illegal SNI hostname received "127.0.0.1" By default, .NET on linux sends the DNS name in the URI in Server Name Indication (SNI), irrespective of whether it's an IP address or not. In order to resolve this, a custom Host request header is added, per dotnet/runtime#20876 (comment)
|
This is ready to review @HofmeisterAn |
There was a problem hiding this comment.
This is ready to review @HofmeisterAn
Thank you for your pull request. I've already seen the PR but haven't had the time to thoroughly review it yet. Spare time is limited 😅.
Most of it looks good! I just have a few minor suggestions and improvements.
In general, we don't expose an API for every configuration (e.g., environment variable) if the developer can configure it themselves through the generic API (it doesn't require in-depth knowledge or isn't crucial for starting a basic container). However, I'm okay with the exposed APIs, but I would like to clean up some parts related to the TLS implementation if needed. I'll need to take a closer look at it (I will look into it by end of this week).
| using System.IO; | ||
| using System.Security.Cryptography.X509Certificates; | ||
| using Org.BouncyCastle.OpenSsl; | ||
| using Org.BouncyCastle.Security; |
There was a problem hiding this comment.
Please move the usings (other classes too) into the global Usings.cs file.
| /// </summary> | ||
| /// <param name="configurationFilePath">The path to the configuration file</param> | ||
| public QdrantBuilder WithConfigFile(string configurationFilePath) => | ||
| Merge(DockerResourceConfiguration, new QdrantConfiguration(configurationFilePath: configurationFilePath)) |
There was a problem hiding this comment.
If we do not require the ConfigurationFilePath property, along with the other configuration properties, at some time later, such as within the Qdrant container instance, then there is no need to keep and store the value. I think we can remove those properties from QdrantConfiguration
| /// <param name="configurationFilePath">The path to the configuration file</param> | ||
| public QdrantBuilder WithConfigFile(string configurationFilePath) => | ||
| Merge(DockerResourceConfiguration, new QdrantConfiguration(configurationFilePath: configurationFilePath)) | ||
| .WithBindMount(configurationFilePath, QdrantLocalConfigurationFilePath); |
There was a problem hiding this comment.
| .WithBindMount(configurationFilePath, QdrantLocalConfigurationFilePath); | |
| .WithResourceMapping(configurationFilePath, QdrantLocalConfigurationFilePath); |
| /// private key. | ||
| /// </summary> | ||
| /// <param name="certificate">A certificate containing a private key</param> | ||
| public QdrantBuilder WithCertificate(X509Certificate2 certificate) |
There was a problem hiding this comment.
Can we not expect the developer to provide valid cert.pem and key.pem files? This way, we can avoid dealing with all the certificate-related issues and Bouncy Castle.
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.4.1"/> | ||
| <PackageReference Include="coverlet.collector" Version="3.2.0"/> | ||
| <PackageReference Include="xunit.runner.visualstudio" Version="2.4.5"/> | ||
| <PackageReference Include="xunit" Version="2.4.2"/> |
There was a problem hiding this comment.
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.4.1"/> | |
| <PackageReference Include="coverlet.collector" Version="3.2.0"/> | |
| <PackageReference Include="xunit.runner.visualstudio" Version="2.4.5"/> | |
| <PackageReference Include="xunit" Version="2.4.2"/> | |
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.7.2"/> | |
| <PackageReference Include="coverlet.collector" Version="6.0.0"/> | |
| <PackageReference Include="xunit.runner.visualstudio" Version="2.5.0"/> | |
| <PackageReference Include="xunit" Version="2.5.0"/> |
|
This pull request has been inactive for some time. I will close it for now. If you wish to continue working on it, please feel free to reopen it. |
|
@HofmeisterAn please can we reopen this PR; I've rebased from |
I am sorry, I cannot reopen it either, it says:
You probably need to create a new PR - sorry. |
|
Superseded by #1149 |
Closes #992
What does this PR do?
This commit adds a qdrant container to the list of supported Testcontainers.
The qdrant container allows configuration of:
Why is it important?
Provides a qdrant test container for folks that need it
Related issues
#992
How to test this PR
Run tests in Testcontainers.Qdrant.Tests