Skip to content

Conversation

@danish9039
Copy link
Contributor

@danish9039 danish9039 commented Jul 14, 2025

Which problem is this PR solving?

part of

Description of the changes

It is part of the planned work to add support for API authentication, as discussed in #7230 (comment)

  1. Improved GetHTTPRoundTripper

    Previous Problems :

    • Auth Bypass: Secure TLS paths could bypass bearer token authentication.
    • Nil Return Risk: Could return a nil RoundTripper without error, causing panics.
    • Confusing TLS: Insecure mode with CAFile led to ambiguous TLS configuration.

    Refactored Solutions:

    • Consistent Auth: Always applies TLS first, then wraps with authentication.
    • Guaranteed Return: Ensures a valid, non-nil RoundTripper is always returned.
    • Clearer TLS: Streamlines TLS application for predictable behavior.
  2. Refactored getConfigOptions

    • getESOptions: Extracted basic ES client options into a dedicated helper for modularity.
    • Health Check: Refined logic to disable health checks only when explicitly requested or when context-only bearer tokens are used, preventing startup issues.
    • HTTP Client: Integrated GetHTTPRoundTripper output directly into http.Client setup, ensuring full transport configuration from the start.

How was this change tested?

Checklist

Signed-off-by: danish9039 <[email protected]>
@danish9039 danish9039 requested a review from a team as a code owner July 14, 2025 21:59
@danish9039 danish9039 requested a review from joe-elliott July 14, 2025 21:59
@codecov
Copy link

codecov bot commented Jul 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.27%. Comparing base (5b0b5a1) to head (8c9152d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7313      +/-   ##
==========================================
+ Coverage   96.19%   96.27%   +0.08%     
==========================================
  Files         377      377              
  Lines       22898    22899       +1     
==========================================
+ Hits        22026    22046      +20     
+ Misses        657      645      -12     
+ Partials      215      208       -7     
Flag Coverage Δ
badger_v1 9.17% <0.00%> (-0.01%) ⬇️
badger_v2 1.75% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1-manual 11.91% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-auto 1.74% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-manual 1.74% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1-manual 11.91% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-auto 1.74% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-manual 1.74% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 16.77% <58.18%> (+0.10%) ⬆️
elasticsearch-7.x-v1 16.81% <58.18%> (+0.10%) ⬆️
elasticsearch-8.x-v1 16.97% <58.18%> (+0.10%) ⬆️
elasticsearch-8.x-v2 1.75% <0.00%> (-0.01%) ⬇️
grpc_v1 10.42% <0.00%> (-0.01%) ⬇️
grpc_v2 1.75% <0.00%> (-0.01%) ⬇️
kafka-3.x-v1 9.34% <0.00%> (-0.01%) ⬇️
kafka-3.x-v2 1.75% <0.00%> (-0.01%) ⬇️
memory_v2 1.75% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 16.86% <58.18%> (+0.10%) ⬆️
opensearch-2.x-v1 16.86% <58.18%> (+0.10%) ⬆️
opensearch-2.x-v2 1.75% <0.00%> (-0.01%) ⬇️
query 1.75% <0.00%> (-0.01%) ⬇️
tailsampling-processor 0.48% <0.00%> (-0.01%) ⬇️
unittests 95.19% <100.00%> (+0.08%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yurishkuro yurishkuro added the changelog:refactoring Internal code refactoring without functional changes label Jul 14, 2025
@yurishkuro
Copy link
Member

Is this change purely a refactor or does it change the behavior? It seems to me that the TLS handling is different now. If so, please update the description/title to reflect what changed.

@yurishkuro yurishkuro removed the changelog:refactoring Internal code refactoring without functional changes label Jul 14, 2025
@danish9039 danish9039 changed the title [refactor] config.go [refactor] Improved GetHTTPRoundTripper Jul 14, 2025
@danish9039
Copy link
Contributor Author

Is this change purely a refactor or does it change the behavior? It seems to me that the TLS handling is different now. If so, please update the description/title to reflect what changed.

done

@yurishkuro yurishkuro enabled auto-merge July 15, 2025 15:28
@yurishkuro yurishkuro added this pull request to the merge queue Jul 15, 2025
Merged via the queue into jaegertracing:main with commit 2eb85e1 Jul 15, 2025
63 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants