[13.1] Use WithHttpsCertificateConfiguration to configure tls cert for Otel Collector#1058
[13.1] Use WithHttpsCertificateConfiguration to configure tls cert for Otel Collector#1058afscrome wants to merge 2 commits into
WithHttpsCertificateConfiguration to configure tls cert for Otel Collector#1058Conversation
WithHttpsCertificateConfiguration to configure tls cert for the…WithHttpsCertificateConfiguration to configure tls cert for Otel Collector
martinjt
left a comment
There was a problem hiding this comment.
I don't think we need the TelemetryGen as part of this change.
|
I'll see what I can do tomorrow - reason I added telemetry generator is that I needed something that generated container from a container as I wanted to make sure the cert was being trusted both in |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the OpenTelemetry Collector integration by adopting Aspire 13.1's new WithHttpsCertificateConfiguration API for TLS certificate management, replacing a custom dev certificate export implementation. The change simplifies the codebase by leveraging built-in Aspire functionality.
- Replaced custom dev certificate export logic with
WithHttpsCertificateConfigurationAPI - Removed
DevCertHostingExtensions.csshared utility file and associated tests - Enhanced the example app with telemetry generators to demonstrate container-to-container communication
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/OpenTelemetryCollectorExtensions.cs | Refactored certificate configuration to use new Aspire 13.1 API instead of custom implementation |
| src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector.csproj | Removed reference to shared DevCertHostingExtensions.cs file |
| src/Shared/DevCertHostingExtensions.cs | Deleted entire custom dev certificate hosting extensions utility |
| tests/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector.Tests/ResourceCreationTests.cs | Removed tests specific to custom dev certificate export implementation |
| examples/opentelemetry-collector/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector.AppHost/AppHost.cs | Added telemetry generator containers to demonstrate collector functionality |
| var scheme = useHttpsForRecievers ? "https" : "http"; | ||
| resourceBuilder.WithEndpoint(targetPort: port, name: protocol, scheme: scheme); | ||
|
|
||
| if (!useHttpsForRecievers) |
There was a problem hiding this comment.
The variable name has a spelling error: "Recievers" should be "Receivers" (the 'i' and 'e' are transposed).
| .WithArgs(signal, | ||
| "--duration", "inf", | ||
| "--otlp-endpoint", collector.GetEndpoint("grpc").Property(EndpointProperty.HostAndPort)) | ||
| .WithArgs(args) |
There was a problem hiding this comment.
The variable args is not defined in the scope of the AddTelemetryGenerator method. This appears to be a bug as it references the command-line arguments from the Main method which are not accessible here. This line should likely be removed since the necessary arguments are already added on lines 21-23.
| .WithArgs(args) |
| if (!settings.ForceNonSecureReceiver && isHttpsEnabled && builder.ExecutionContext.IsRunMode) | ||
| { | ||
| resourceBuilder.RunWithHttpsDevCertificate(); | ||
| var useHttpsForRecievers = !settings.ForceNonSecureReceiver && url.StartsWith("https", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The variable name has a spelling error: "Recievers" should be "Receivers" (the 'i' and 'e' are transposed).
|
|
||
| void ConfigureReceiver(int port, string protocol) | ||
| { | ||
| var scheme = useHttpsForRecievers ? "https" : "http"; |
There was a problem hiding this comment.
The variable name has a spelling error: "Recievers" should be "Receivers" (the 'i' and 'e' are transposed).
| ctx.Arguments.Add(ReferenceExpression.Create($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::cert_file: ""{ctx.CertificatePath}""")); | ||
| ctx.Arguments.Add(ReferenceExpression.Create($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::key_file: ""{ctx.KeyPath}""")); |
There was a problem hiding this comment.
Default 'ToString()': ReferenceExpression inherits 'ToString()' from 'Object', and so is not suitable for printing.
| ctx.Arguments.Add(ReferenceExpression.Create($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::cert_file: ""{ctx.CertificatePath}""")); | |
| ctx.Arguments.Add(ReferenceExpression.Create($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::key_file: ""{ctx.KeyPath}""")); | |
| ctx.Arguments.Add($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::cert_file: ""{ctx.CertificatePath}"""); | |
| ctx.Arguments.Add($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::key_file: ""{ctx.KeyPath}"""); |
There was a problem hiding this comment.
ReferenceExpression is absolutely required here as certificate and key paths cannot be resolve until after the app host starts.
| ctx.Arguments.Add(ReferenceExpression.Create($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::cert_file: ""{ctx.CertificatePath}""")); | ||
| ctx.Arguments.Add(ReferenceExpression.Create($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::key_file: ""{ctx.KeyPath}""")); |
There was a problem hiding this comment.
Default 'ToString()': ReferenceExpression inherits 'ToString()' from 'Object', and so is not suitable for printing.
| ctx.Arguments.Add(ReferenceExpression.Create($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::cert_file: ""{ctx.CertificatePath}""")); | |
| ctx.Arguments.Add(ReferenceExpression.Create($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::key_file: ""{ctx.KeyPath}""")); | |
| ctx.Arguments.Add($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::cert_file: ""{ctx.CertificatePath}"""); | |
| ctx.Arguments.Add($@"--config=yaml:receivers::otlp::protocols::{protocol}::tls::key_file: ""{ctx.KeyPath}"""); |
There was a problem hiding this comment.
ReferenceExpression is absolutely required here as certificate and key paths cannot be resolve until after the app host starts.
Use the new `WithHttpsCertificateConfiguration` apis in 13.1 rather than the previous custom implementation of exporting the dev cert.
04e47c4 to
163148e
Compare
|
@aaronpowell Can you reopen this. @martinjt Do you mind re-reviewing? |
|
The only question I have is how this will work for people using the latest CommunityToolkit against not .NET 10, and whether that will actually break things now. |
…nTelemetryCollectorExtensions.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Everything should work the same way. dotnet runtime version doesn't matter. All that matters is whether you have a dev cert available. If one is available, it will will be injected. (And as the dashboard defaults to https, you likely do have a dev cert avaialble)
But these two scenarios aren't really any different from the existing implementation which would also have injected the dev cert, but apps may not have ended up trusting it. |
Closes #981
Use the new
WithHttpsCertificateConfigurationapis in 13.1 rather than the previous custom implementation of exporting the dev cert.Added some telemetry generator resources to the otel collector's playground app to generate some dummy otel, as well as to test telemetry for container --> container.
PR Checklist
Other information