Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove TLS loading and replace with OTEL configtls #6345

Merged
merged 9 commits into from
Dec 13, 2024

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Dec 12, 2024

Which problem is this PR solving?

Description of the changes

  • Remove our custom logic for loading certificates
  • Fully rely on configtls module from otel-collector

How was this change tested?

  • CI

@yurishkuro yurishkuro requested a review from a team as a code owner December 12, 2024 03:33
@yurishkuro yurishkuro marked this pull request as draft December 12, 2024 03:34
Signed-off-by: Yuri Shkuro <github@ysh.us>
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.02%. Comparing base (737be78) to head (935f76a).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6345      +/-   ##
==========================================
- Coverage   96.07%   96.02%   -0.05%     
==========================================
  Files         358      356       -2     
  Lines       20477    20313     -164     
==========================================
- Hits        19673    19506     -167     
- Misses        613      615       +2     
- Partials      191      192       +1     
Flag Coverage Δ
badger_v1 9.01% <ø> (+0.16%) ⬆️
badger_v2 1.64% <ø> (+0.03%) ⬆️
cassandra-4.x-v1-manual 15.01% <ø> (+0.27%) ⬆️
cassandra-4.x-v2-auto 1.58% <ø> (+0.02%) ⬆️
cassandra-4.x-v2-manual 1.58% <ø> (+0.02%) ⬆️
cassandra-5.x-v1-manual 15.01% <ø> (+0.27%) ⬆️
cassandra-5.x-v2-auto 1.58% <ø> (+0.02%) ⬆️
cassandra-5.x-v2-manual 1.58% <ø> (+0.02%) ⬆️
elasticsearch-6.x-v1 18.70% <ø> (+0.33%) ⬆️
elasticsearch-7.x-v1 18.79% <ø> (+0.34%) ⬆️
elasticsearch-8.x-v1 18.95% <ø> (+0.35%) ⬆️
elasticsearch-8.x-v2 1.64% <ø> (+0.03%) ⬆️
grpc_v1 10.53% <ø> (+0.19%) ⬆️
grpc_v2 7.95% <ø> (+0.14%) ⬆️
kafka-v1 9.33% <ø> (+0.16%) ⬆️
kafka-v2 1.64% <ø> (+0.03%) ⬆️
memory_v2 1.64% <ø> (+0.03%) ⬆️
opensearch-1.x-v1 18.84% <ø> (+0.35%) ⬆️
opensearch-2.x-v1 18.84% <ø> (+0.34%) ⬆️
opensearch-2.x-v2 1.63% <ø> (+0.02%) ⬆️
tailsampling-processor 0.45% <ø> (+<0.01%) ⬆️
unittests 94.89% <100.00%> (-0.06%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro changed the title Remove watcher Remove TLS loading and replace with OTEL configtls Dec 13, 2024
@yurishkuro yurishkuro requested review from mahadzaryab1 and removed request for joe-elliott December 13, 2024 02:38
@yurishkuro yurishkuro marked this pull request as ready for review December 13, 2024 02:39
@yurishkuro
Copy link
Member Author

@chahatsagarmain thanks to your changes we can now remove this tls watcher that was causing race conditions.

Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 left a comment

Choose a reason for hiding this comment

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

just a couple of nits

Signed-off-by: Yuri Shkuro <github@ysh.us>
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro merged commit 989dcb0 into jaegertracing:main Dec 13, 2024
54 checks passed
@yurishkuro yurishkuro deleted the remove-watcher branch December 13, 2024 16:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: race condition when reloading TLS certificates
2 participants