Enhancement: Add include_insecure_cipher_suites to configtls to enable insecure cipher suites#14043
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14043 +/- ##
=======================================
Coverage 91.27% 91.27%
=======================================
Files 709 709
Lines 46218 46221 +3
=======================================
+ Hits 42186 42189 +3
Misses 2817 2817
Partials 1215 1215 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@TylerHelmuth Waiting for the reviews. Thanks :) |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@TylerHelmuth @codeboten waiting for reviews. thanks :) |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Hey Team, looks like we're super close to the finish line. Hoping we can close this out. @atoulme, how do you feel about the reasoning above for adding the flag? Thanks y'all! |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Hey @vignesh-codes looks like there's a lingering test failing. I'm hopeful if you can get that resolved we can get this across the line. Would love to see this wrapped up. Thanks!! |
|
@dentonk looks like the benchmarks tests are timing out. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
jade-guiton-dd
left a comment
jade-guiton-dd
left a comment
There was a problem hiding this comment.
Nothing has changed since my last review, so I can't approve much more than I already have. You'll have to ping some maintainers to get it merged.
jade-guiton-dd
added
ready-to-merge
Merging this PR will improve performance by 86.43%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | BenchmarkHTTPProtoLogsSequential |
4.7 ms | 2.5 ms | +82.99% |
| ⚡ | BenchmarkGRPCLogsSequential |
5.6 ms | 3 ms | +89.93% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing vignesh-codes:feature/configtls-insecure-cipher-suites (3640768) with main (0093005)
|
I think some of those test failures may come from main, but: seems to be something to fix. |
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Please address the comments from @jade-guiton-dd, we can get this merged asap
codeboten
removed
the
ready-to-merge
Head branch was pushed to by a user without write access
|
@jade-guiton-dd @codeboten I pushed the suggested fixes. Apologies for the delay. Totally forgot about this pr |
jade-guiton-dd
left a comment
jade-guiton-dd
left a comment
There was a problem hiding this comment.
Good to merge.
(The remaining failure in check-links seems to be with an existing link, and the checker seems to be straight up wrong about the fragment not existing?)
jade-guiton-dd
added
the
ready-to-merge
fe2cf23
|
Thank you for your contribution @vignesh-codes! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
6 participants
Description
Enhancement: Add
include_insecure_cipher_suitesto configtls to enable insecure cipher suites. Insecure cipher suites are disabled by default for securityLink to tracking issue
Fixes #13829
Testing
Added TestInsecureCipherSuites
Documentation
Updated config/configtls/README.md