-
Notifications
You must be signed in to change notification settings - Fork 19
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 otlp tls configuration #998
feat: add otlp tls configuration #998
Conversation
Signed-off-by: Fabrizio Sestito <[email protected]>
1fa0050
to
2e6f873
Compare
Signed-off-by: Fabrizio Sestito <[email protected]>
Signed-off-by: Fabrizio Sestito <[email protected]>
Signed-off-by: Fabrizio Sestito <[email protected]>
2e6f873
to
9cccaa1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #998 +/- ##
==========================================
+ Coverage 62.64% 63.58% +0.94%
==========================================
Files 17 17
Lines 1052 1071 +19
==========================================
+ Hits 659 681 +22
+ Misses 393 390 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, modulo what Jose pointed out about the test.
Moreover, as a nitpick, I would not add the certificate CA, keys and certificates to the test directory, but I would generate them before running the tests. This can be easily done with the rcgen crate from the rustls project.
Signed-off-by: Fabrizio Sestito <[email protected]>
cbb96c7
to
218f5db
Compare
@flavio @jvanz I've added a new commit 8a011af which removes the flags entirely and uses the env variables for everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it. The only comment that must be addressed is the one about the mTLS tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo Flavio's suggestions.
Signed-off-by: Fabrizio Sestito <[email protected]>
Signed-off-by: Fabrizio Sestito <[email protected]>
8a011af
to
f0879b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for having implemented the requested changes
👍 |
Description
This PR adds the following flags and env variables:
otlp-certificate
/OTEL_EXPORTER_OTLP_CERTIFICATE
: The trusted certificate to use when verifying a server's TLS credentials.otlp-client-certificate
/OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE
: Client certificate/chain trust for clients private key to use in mTLS communication in PEM format.otlp-client-key
/OTEL_EXPORTER_OTLP_CLIENT_KEY
: Client certificate/chain trust for clients private key to use in mTLS communication in PEM format.Test
Updates otel integration test
Additional Information
Tradeoff
Rust opentelemetry only supports tls by using tonic (grpc).
Closes #993