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

[otlp] Add mTLS Support for OTLP Exporter #5918

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sandy2008
Copy link
Contributor

@sandy2008 sandy2008 commented Oct 23, 2024

Fixes #2009
Based on PR

Changes

This pull request introduces support for mutual TLS (mTLS) in the OpenTelemetry Protocol (OTLP) Exporter, allowing secure gRPC connections over HTTPS and mTLS supports for HTTP Protocol.

Key Changes:

  • Certificate file. OTEL_EXPORTER_OTLP_CERTIFICATE
  • Client key file. OTEL_EXPORTER_OTLP_CLIENT_KEY
  • Client certificate file. OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@github-actions github-actions bot added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Oct 23, 2024
@sandy2008 sandy2008 changed the title feat(): Add mTLS Support for OTLP Exporter [otlp] Add mTLS Support for OTLP Exporter Oct 23, 2024
@@ -28,6 +28,26 @@ protected override HttpContent CreateHttpContent(OtlpCollector.ExportTraceServic
return new ExportRequestContent(exportRequest);
}

private static HttpClient ModifyHttpClient(OtlpExporterOptions options, HttpClient httpClient)
Copy link
Member

Choose a reason for hiding this comment

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

We can't just throw away the HttpClient and make a new one. Users are able to configure SSL/TLS today using factory:

This change will break any user doing that or doing anything else to the HttpClient they are intending to use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified a little bit, does it look fine? @CodeBlanch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the design looks ok, I will start to write tests :)

Copy link

codecov bot commented Oct 27, 2024

Codecov Report

Attention: Patch coverage is 61.11111% with 35 lines in your changes missing coverage. Please review.

Project coverage is 86.36%. Comparing base (6250307) to head (3733dee).
Report is 375 commits behind head on main.

Files with missing lines Patch % Lines
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 68.65% 21 Missing ⚠️
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 39.13% 14 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5918      +/-   ##
==========================================
+ Coverage   83.38%   86.36%   +2.97%     
==========================================
  Files         297      262      -35     
  Lines       12531    11893     -638     
==========================================
- Hits        10449    10271     -178     
+ Misses       2082     1622     -460     
Flag Coverage Δ
unittests ?
unittests-Project-Experimental 86.32% <61.11%> (?)
unittests-Project-Stable 86.34% <61.11%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 83.33% <39.13%> (-10.29%) ⬇️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 87.50% <68.65%> (-8.87%) ⬇️

... and 242 files with indirect coverage changes

@sandy2008
Copy link
Contributor Author

Just for your info, AddCertificatesToHttpClient is not needed, so will delete it in the next commit.
@alanwest @CodeBlanch @sokoide Does other changes look ok?

@rajkumar-rangaraj
Copy link
Contributor

Just for your info, AddCertificatesToHttpClient is not needed, so will delete it in the next commit. @alanwest @CodeBlanch @sokoide Does other changes look ok?

@sandy2008 I recommend putting this on hold for a month. We are working on a custom serialization (#5730) and also we plan to handle GRPC without relying on Grpc.Net packages and removing dependencies on Google Protobuf and GRPC libraries. This will impact the part where you are adding support for mTLS.

@sandy2008
Copy link
Contributor Author

Just for your info, AddCertificatesToHttpClient is not needed, so will delete it in the next commit. @alanwest @CodeBlanch @sokoide Does other changes look ok?

@sandy2008 I recommend putting this on hold for a month. We are working on a custom serialization (#5730) and also we plan to handle GRPC without relying on Grpc.Net packages and removing dependencies on Google Protobuf and GRPC libraries. This will impact the part where you are adding support for mTLS.

Hi :) @rajkumar-rangaraj
Since there is also http part as well, shall I change this PR to support HTTP first, and add GRPC support afterwards?

@rajkumar-rangaraj
Copy link
Contributor

Since there is also http part as well, shall I change this PR to support HTTP first, and add GRPC support afterwards?

Today, we had a discussion on this topic in the SIG meeting. We all feel it is better to hold off on this work as this space is expected to change drastically in the coming weeks. We thought it would be beneficial if you could join the next SIG meeting to discuss it further.

@sandy2008
Copy link
Contributor Author

Since there is also http part as well, shall I change this PR to support HTTP first, and add GRPC support afterwards?

Today, we had a discussion on this topic in the SIG meeting. We all feel it is better to hold off on this work as this space is expected to change drastically in the coming weeks. We thought it would be beneficial if you could join the next SIG meeting to discuss it further.

Thank you for the update. @rajkumar-rangaraj

Could you please let me know the time and participation details for the next SIG meeting? I think it’s important for @sokoide , as a co-author, to attend as well, since I might not be able to cover everything alone.

@rajkumar-rangaraj
Copy link
Contributor

Could you please let me know the time and participation details for the next SIG meeting? I think it’s important for @sokoide , as a co-author, to attend as well, since I might not be able to cover everything alone.

Please check the details here - https://github.com/open-telemetry/opentelemetry-dotnet?tab=readme-ov-file#contributing

@sandy2008
Copy link
Contributor Author

Could you please let me know the time and participation details for the next SIG meeting? I think it’s important for @sokoide , as a co-author, to attend as well, since I might not be able to cover everything alone.

Please check the details here - https://github.com/open-telemetry/opentelemetry-dotnet?tab=readme-ov-file#contributing

Thank you for the details! We’ll join the SIG meeting scheduled for November 5 at 16:00 PT (November 6, 11:00 JST). Looking forward to it!

{
var trustedCertificate = X509Certificate2.CreateFromPemFile(this.CertificateFile);

handler.ServerCertificateCustomValidationCallback = (message, cert, chain, errors) =>

Choose a reason for hiding this comment

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

Do you want users of this API being able to add their custom validation callbacks next to this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OTLP Exporter TLS/mTLS configuration options
4 participants