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: Replace CosmosDb module #833

Merged
merged 13 commits into from
Mar 20, 2023
Merged

feat: Replace CosmosDb module #833

merged 13 commits into from
Mar 20, 2023

Conversation

AButler
Copy link
Contributor

@AButler AButler commented Mar 13, 2023

What does this PR do?

Replaces the old CosmosDb module with the new module format.

Why is it important?

Migrates one of the obsolete modules to the new format.

Related issues

All the CosmosDb tests should pass

@netlify
Copy link

netlify bot commented Mar 13, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 08ef6d5
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/64188f84f4aabf0008806128
😎 Deploy Preview https://deploy-preview-833--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 settings.

@AButler AButler changed the title Feature/replace cosmosdb module feat: Introduce new CosmosDb module Mar 13, 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.

Thank you for submitting the pull request! I was wondering if you know if any changes were made to the Cosmos DB image or the GH-hosted runners? Cosmos DB did not run properly or was flaky running on Ubuntu-22.04.

I will review your pull request tomorrow. Thank you once again for your contribution!

@AButler
Copy link
Contributor Author

AButler commented Mar 13, 2023

Hi, unfortunately the emulator is still flaky on some runners due to certain CPUs. Hopefully MS will fix this soon. I still find this useful for Testcontainers since we use Testcontainers when running the tests locally and use a real cosmos container in the pipelines (at least until the issue is fixed).

Really hoping MS will fix it soon!

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.

The PR looks good. I just have a question about PartitionCount and IpAddressOverride.

src/Testcontainers.CosmosDb/CosmosDbBuilder.cs Outdated Show resolved Hide resolved
@Melethainiel
Copy link

Could it be possible to integrate the /Port configuration from last comment in #421 to avoid HttpClient exception ? Maybe something in the builder .WithPort() ?

@AButler
Copy link
Contributor Author

AButler commented Mar 14, 2023

Could it be possible to integrate the /Port configuration from last comment in #421 to avoid HttpClient exception ? Maybe something in the builder .WithPort() ?

If you use the HttpClient from the CosmosDbBuilder (see example in the test: https://github.com/testcontainers/testcontainers-dotnet/pull/833/files#diff-7c5e50ab4a8d9fd82823a579b5c9d3d61d52e6d13f706b54a8b3877acec2a45dR30) then it will automatically rewrite the host/port and accept the dev certificate.

Example:

var cosmosDbContainer = new CosmosDbBuilder().Build();

var cosmosClientOptions = new CosmosClientOptions {
  ConnectionMode = ConnectionMode.Gateway,
  HttpClientFactory = () => cosmosDbContainer.HttpClient
};

var cosmosClient = new CosmosClient(cosmosDbContainer.GetConnectionString(), cosmosClientOptions);

@Melethainiel
Copy link

Melethainiel commented Mar 14, 2023

Could it be possible to integrate the /Port configuration from last comment in #421 to avoid HttpClient exception ? Maybe something in the builder .WithPort() ?

If you use the HttpClient from the CosmosDbBuilder (see example in the test: https://github.com/testcontainers/testcontainers-dotnet/pull/833/files#diff-7c5e50ab4a8d9fd82823a579b5c9d3d61d52e6d13f706b54a8b3877acec2a45dR30) then it will automatically rewrite the host/port and accept the dev certificate.

Example:

var cosmosDbContainer = new CosmosDbBuilder().Build();

var cosmosClientOptions = new CosmosClientOptions {
  ConnectionMode = ConnectionMode.Gateway,
  HttpClientFactory = () => cosmosDbContainer.HttpClient
};

var cosmosClient = new CosmosClient(cosmosDbContainer.GetConnectionString(), cosmosClientOptions);

only if this is the 8081 port used. If not you have to rewrite the url with an HttpMessageHandler or use the /Port argument

But the random port will likely not be 8081.

@AButler
Copy link
Contributor Author

AButler commented Mar 14, 2023

only if this is the 8081 port used. If not you have to rewrite the url with an HttpMessageHandler or use the /Port argument

Yes, the CosmosDbBuilder.HttpClient already has the HttpMessageHandler to do the rewrite built in. Even if the port was randomly generated and passed through to the Cosmos emulator, you'd still need a message handler to rewrite the host and disable the certificate validation so not sure passing the port through adds anything.

@Melethainiel
Copy link

only if this is the 8081 port used. If not you have to rewrite the url with an HttpMessageHandler or use the /Port argument

Yes, the CosmosDbBuilder.HttpClient already has the HttpMessageHandler to do the rewrite built in. Even if the port was randomly generated and passed through to the Cosmos emulator, you'd still need a message handler to rewrite the host and disable the certificate validation so not sure passing the port through adds anything.

I don't know the magic behind :/ The only thing I know is that all the errors disappear when you set /Port=xxx and bind the docker port xxx to the host port xxx as in the second link I provided.

It's just a question, because with the last implementation I had the issue and ended by creating my own implementation with the /Port arg

@HofmeisterAn
Copy link
Collaborator

only if this is the 8081 port used. If not you have to rewrite the url with an HttpMessageHandler or use the /Port argument

But the random port will likely not be 8081.

The container port will always be 8081. We resolve it to the random assigned host port her:

properties.Add("AccountEndpoint", new UriBuilder("https", Hostname, GetMappedPublicPort(CosmosDbBuilder.CosmosDbPort)).ToString());

I think the configuration is fine.

@Melethainiel
Copy link

Melethainiel commented Mar 15, 2023

The container port will always be 8081. We resolve it to the random assigned host port her:

properties.Add("AccountEndpoint", new UriBuilder("https", Hostname, GetMappedPublicPort(CosmosDbBuilder.CosmosDbPort)).ToString());

I think the configuration is fine.

The problem is not the connection string. It's from having a random port assign to 8081.
Let me show what I mean with docker-compose example :

Here is the equivalent of what the builder will produce with a random port mapped to the 8081

version: '2'
services:
  cosmosdb:
    image: mcr.microsoft.com/cosmosdb/linux/azure-cosmos-emulator
    container_name: cosmosdb
    ports:
      - 14559:8081 

And here is what it should be to avoid error

version: '2'
services:
  cosmosdb:
    image: mcr.microsoft.com/cosmosdb/linux/azure-cosmos-emulator
    container_name: cosmosdb
    ports:
      - 14559:14559 
    environment:
      AZURE_COSMOS_EMULATOR_ARGS: /Port=14559

From Microsoft doc and the issue when both port are not the same.

And here with some code :

The cosmosClientBuilder which doesn't change

  cosmosClientBuilder.WithHttpClientFactory(
                    () =>
                    {
                        HttpMessageHandler httpMessageHandler = new HttpClientHandler
                        {
                            ServerCertificateCustomValidationCallback =
                                HttpClientHandler.DangerousAcceptAnyServerCertificateValidator
                        };

                        return new HttpClient(httpMessageHandler);
                    })
                .WithConnectionModeGateway();

The exception I got when the port is mapped 8082:8081

System.IO.IOException:  Received an unexpected EOF or 0 bytes from the transport stream.

with port mapped 8082:8082 with the /Port argument all work smoothly

@AButler
Copy link
Contributor Author

AButler commented Mar 15, 2023

In your above example, you are not using the HttpClient provided by the Testcontainers module. If you did then the code would be:

// Create test container
var cosmosDbContainer = new CosmosDbBuilder().Build();

cosmosClientBuilder
    .WithHttpClientFactory(() => cosmosDbContainer.HttpClient) // Use http client from Testcontainers module
    .WithConnectionModeGateway();

@Melethainiel
Copy link

Melethainiel commented Mar 15, 2023

In your above example, you are not using the HttpClient provided by the Testcontainers module. If you did then the code would be:

// Create test container
var cosmosDbContainer = new CosmosDbBuilder().Build();

cosmosClientBuilder
    .WithHttpClientFactory(() => cosmosDbContainer.HttpClient) // Use http client from Testcontainers module
    .WithConnectionModeGateway();

It would imply to add the library as reference to the main project or pass down the httpclient which is not needed with the fix I suggest.

@AButler
Copy link
Contributor Author

AButler commented Mar 15, 2023

It would imply to add the library as reference to the main project and pass down the httpclient which is not needed with the fix I suggest.

No, as this is a test library then I would normally use dependency injection to inject the CosmosClient. It is not as though you would ever want the certificate validation disabled in the main project, nor for it to be forced to use gateway mode.

@Melethainiel
Copy link

Melethainiel commented Mar 15, 2023

No, as this is a test library then I would normally use dependency injection to inject the CosmosClient. It is not as though you would ever want the certificate validation disabled in the main project, nor for it to be forced to use gateway mode.

Nevermind, It was just a suggestion

HofmeisterAn
HofmeisterAn previously approved these changes Mar 15, 2023
HofmeisterAn
HofmeisterAn previously approved these changes Mar 19, 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.

Sorry for the back and forth, I was trying to get the tests running on the Windows agent, but that is even more a mess. I think we need to skip the tests for now. I will merge it later in the day.

@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Mar 20, 2023
@HofmeisterAn HofmeisterAn changed the title feat: Introduce new CosmosDb module feat: Replace CosmosDb module Mar 20, 2023
@HofmeisterAn HofmeisterAn merged commit e31b13c into testcontainers:develop Mar 20, 2023
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.

None yet

3 participants