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: add Qdrant module #1149

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

russcam
Copy link

@russcam russcam commented Apr 2, 2024

Closes #992

What does this PR do?

This commit adds a qdrant container to the list of supported Testcontainers, for use with the Qdrant vector database.

The qdrant container allows configuration of:

  • an API key to authenticate to Qdrant
  • a certificate and private key in PEM format used to secure communication to Qdrant with Transport Layer Security

Why is it important?

Provides a qdrant test container for folks that need it

Related issues

#992
Supersedes #994

How to test this PR

Run tests in Testcontainers.Qdrant.Tests

russcam added 5 commits March 30, 2024 09:17
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
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)
Copy link

netlify bot commented Apr 2, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit f31250e
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/6613d2a9f9b1cb000875ac68
😎 Deploy Preview https://deploy-preview-1149--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.

@russcam russcam mentioned this pull request Apr 2, 2024
@russcam russcam changed the title Qdrant container feat: add Qdrant module Apr 4, 2024
@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>net6.0;net8.0;netstandard2.0;netstandard2.1</TargetFrameworks>
Copy link

Choose a reason for hiding this comment

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

Are all these really necessary? I would think the following would be enough:

Suggested change
<TargetFrameworks>net6.0;net8.0;netstandard2.0;netstandard2.1</TargetFrameworks>
<TargetFrameworks>net6.0;netstandard2.0</TargetFrameworks>

Copy link
Author

Choose a reason for hiding this comment

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

Every other TestContainer module defines these target frameworks, so was sticking to the project convention. I've also added net462 for same reason Milvus module does: #1131 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should stick to the repo convention too 👍. In the end, it does not really matter, right (except for the legacy framework)? We can consider dropping support for all projects. I just kept it because there were some preprocessor directives we used in the past, but I think we have already changed them. I need to double-check.

russcam and others added 6 commits April 6, 2024 16:28
This commit moves the wait for strategy to
use the readyz endpoint. If a certificate has
been specified, perform the check with TLS and
allow any certificate to pass validation.
The qdrant gRPC client uses .NET Framework build
of Grpc.Net.Client, which uses WinHttpHandler. The
wiring up is easier on net462 if net462 is targeted
specifically. Same reason why the Milvus container
also targets net462 (testcontainers#1131 (comment)).
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.

Thank you for the PR 🙏. I have reviewed it and made some minor changes to align with the repository standards. I have a some suggestions regarding the certificates. I am happy to merge the PR after we have sorted them out.

/// </summary>
public QdrantConfiguration(string apiKey = null, string certificate = null, string privateKey = null)
{
ApiKey = apiKey;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like that the API key is never used anywhere afterward, you do not need to store it in the configuration.

Comment on lines 55 to 70
var waitStrategy = Wait.ForUnixContainer().UntilHttpRequestIsSucceeded(request =>
{
var httpWaitStrategy = request.ForPort(QdrantHttpPort).ForPath("/readyz");

// allow any certificate defined to pass validation
if (DockerResourceConfiguration.Certificate is not null)
{
httpWaitStrategy.UsingTls()
.UsingHttpMessageHandler(new HttpClientHandler
{
ServerCertificateCustomValidationCallback = (_, _, _, _) => true
});
}

return httpWaitStrategy;
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we know the certificate, shouldn't we then check for the certificate? Or do we ignore it out of simplicity?

Copy link
Author

Choose a reason for hiding this comment

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

Was ignoring it out of simplicity for the waiting strategy, but it could be checked

@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>net6.0;net8.0;netstandard2.0;netstandard2.1</TargetFrameworks>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should stick to the repo convention too 👍. In the end, it does not really matter, right (except for the legacy framework)? We can consider dropping support for all projects. I just kept it because there were some preprocessor directives we used in the past, but I think we have already changed them. I need to double-check.

@russcam russcam requested a review from HofmeisterAn April 21, 2024 08:03
@HofmeisterAn HofmeisterAn added enhancement New feature or request module An official Testcontainers module labels May 17, 2024
@eddumelendez
Copy link
Member

It will be nice to add some docs for the module and how to use it.

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

Successfully merging this pull request may close these issues.

[Feature]: Add Qdrant test container
4 participants