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

Add option to ignore Cosmos emulator certificate. #1668

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

mitchdenny
Copy link
Member

@mitchdenny mitchdenny commented Jan 16, 2024

This PR adds an option to allow developers to ignore the Cosmos emulator ceritficate if it is detected. Usage:

builder.AddAzureCosmosDB("ratingsdb", (settings) =>
{
    settings.IgnoreEmulatorCertificate = true;
});

Also fixes:
#1664

Related:
#1002

Outstanding items:

  • Update README.md
  • Add some test coverage.
Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Jan 16, 2024
@mitchdenny mitchdenny self-assigned this Jan 16, 2024
@mitchdenny mitchdenny requested review from eerhardt, Pilchie and IEvangelist and removed request for eerhardt January 16, 2024 07:38
@mitchdenny mitchdenny added area-integrations Issues pertaining to Aspire Integrations packages area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication labels Jan 16, 2024
@mitchdenny mitchdenny added this to the preview 3 (Feb) milestone Jan 16, 2024
@Pilchie
Copy link
Member

Pilchie commented Jan 16, 2024

Also tagging @sourabh1007

@Pilchie
Copy link
Member

Pilchie commented Jan 16, 2024

See aslo Azure/azure-cosmos-dotnet-v3#4251 where @sourabh1007 is working on building this directly into the SDK.

@josephaw1022
Copy link

can we just set this to true if you use the useEmulator method?

so instead of

builder.AddAzureCosmosDB("ratingsdb", (settings) =>
   {
       settings.IgnoreEmulatorCertificate = true;
   })
      .UseEmulator()
      .AddDatabase("mydb");

It would just be

builder.AddAzureCosmosDB("ratingsdb")
      .UseEmulator()
      .AddDatabase("mydb");


// and useEmulator would then make that setting change. 

// or another approach is 

builder.AddAzureCosmosDB("ratingsdb", useEmulator: true)
      .AddDatabase("mydb");

and it would then do the same thing as the first code block

@mitchdenny
Copy link
Member Author

mitchdenny commented Jan 17, 2024

can we just set this to true if you use the useEmulator method?

@josephaw1022 ... good question. We had a session today where we were reviewing some aspects of the app model. The UseEmulator(...) method will be staying. It will grow into a mechanism to allow customization of the underlying container if required.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks!

src/Components/Aspire.Microsoft.Azure.Cosmos/README.md Outdated Show resolved Hide resolved
src/Shared/Cosmos/CosmosUtils.cs Show resolved Hide resolved
ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator
});
clientOptions.ConnectionMode = ConnectionMode.Gateway;
clientOptions.LimitToEndpoint = true;
Copy link
Member

Choose a reason for hiding this comment

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

This line isn't in

https://learn.microsoft.com/en-us/azure/cosmos-db/how-to-develop-emulator?tabs=windows%2Ccsharp&pivots=api-nosql#connect-to-the-emulator-from-the-sdk

It just has the ServerCertificateCustomValidationCallback and ConnectionMode lines. Why is LimitToEndpoint being set to true here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure of the reason why but without LimitToEndpoint the trick doesn't work. I think it has something to do with the way that the Cosmos client talks to different regional gateways. But I'm not sure how/why that comes into play in the emulator scenario ... but it does seem to make a difference here.

Copy link
Member

Choose a reason for hiding this comment

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

Should the docs be updated then? - cc @Pilchie @sourabh1007

Choose a reason for hiding this comment

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

Looks like document is already updated and LimitToEndpoint is not required.

Copy link
Member

Choose a reason for hiding this comment

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

and LimitToEndpoint is not required.

Then why is @mitchdenny saying it is required above?

but without LimitToEndpoint the trick doesn't work.

Choose a reason for hiding this comment

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

@eerhardt docs was update just a day back or so.
@mitchdenny might have referenced it before that change.

Emulator will only have a single region; functional wise it will not impact but unnecessary and confuses.

Ability to ignore SSLCert through connection string is currently in PR stage and next release might ship it. Post that hopefully all above can be updated accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

@mitchdenny - can you verify that we don't need the line setting LimitToEndpoint = true;? I don't want us doing something that isn't in the docs.

Copy link

Choose a reason for hiding this comment

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

Whoever wrote those docs did not consult with the SDK team. The original version of that doc had it and that might have brought the confusion. We discovered the issue, we corrected the docs (https://github.com/MicrosoftDocs/azure-docs-pr/pull/263406). LimitToEndpoint has nothing to do with emulator or SSL certificates, it's a flag meant to disable cross-region retries on the SDK -> https://learn.microsoft.com/en-us/azure/cosmos-db/nosql/troubleshoot-sdk-availability
image

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@mitchdenny mitchdenny merged commit fc0d681 into main Jan 18, 2024
8 checks passed
@mitchdenny mitchdenny deleted the ignore-cosmos-emulator-cert-option branch January 18, 2024 01:09
var cosmosdb = builder.AddAzureCosmosDB("cosmos").UseEmulator();
```

When the AppHost starts up a local container running the Azure CosmosDB will also be started. Inside the project that uses CosmosDB you also need to specify that you want to ignore the server certificate (so you don't need to manually download and install it):
Copy link
Member

Choose a reason for hiding this comment

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

This should have had the same changes as the other README.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication area-codeflow for labeling automated codeflow. intentionally a different color! area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants