[query] Change HTTP and TLS server configurations to use OTEL configurations #6023

mahadzaryab1 · 2024-09-27T02:12:01Z

Which problem is this PR solving?

Part of [jaeger-v2] Align Query Service Config With OTEL #5996

Description of the changes

Removed the custom TLS and HTTP server configurations and replaced them with OTEL's configurations
🛑 Breaking change: the hot-reload of certificates will not happen at reload_intervals rather than immediately on file changes. This makes Jaeger's behavior similar to OTEL Collector's behavior.

How was this change tested?

Unit tests / CI

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

codecov · 2024-09-27T02:19:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.90%. Comparing base (3cd1f59) to head (f32737b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6023      +/-   ##
==========================================
+ Coverage   88.60%   96.90%   +8.30%     
==========================================
  Files         349      349              
  Lines       18158    16599    -1559     
==========================================
- Hits        16088    16086       -2     
+ Misses       1888      329    -1559     
- Partials      182      184       +2

Flag	Coverage Δ
badger_v1	`7.98% <0.00%> (-0.02%)`	⬇️
badger_v2	`1.81% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v1	`15.75% <0.00%> (-0.04%)`	⬇️
cassandra-4.x-v2	`1.74% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v1	`15.75% <0.00%> (-0.04%)`	⬇️
cassandra-5.x-v2	`1.74% <0.00%> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`18.69% <0.00%> (-0.04%)`	⬇️
elasticsearch-7.x-v1	`18.74% <0.00%> (-0.04%)`	⬇️
elasticsearch-8.x-v1	`18.94% <0.00%> (-0.04%)`	⬇️
elasticsearch-8.x-v2	`1.80% <0.00%> (+0.12%)`	⬆️
grpc_v1	`9.51% <0.00%> (-0.01%)`	⬇️
grpc_v2	`7.10% <0.00%> (-0.03%)`	⬇️
kafka-v1	`9.69% <0.00%> (-0.02%)`	⬇️
kafka-v2	`1.81% <0.00%> (-0.01%)`	⬇️
memory_v2	`1.81% <0.00%> (-0.01%)`	⬇️
opensearch-1.x-v1	`18.80% <0.00%> (-0.03%)`	⬇️
opensearch-2.x-v1	`18.80% <0.00%> (-0.04%)`	⬇️
opensearch-2.x-v2	`1.80% <0.00%> (-0.02%)`	⬇️
tailsampling-processor	`0.46% <0.00%> (-0.01%)`	⬇️
unittests	`95.70% <100.00%> (-0.02%)`	⬇️

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: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 · 2024-09-27T02:58:58Z

@yurishkuro would you be able to re-run the CI? getting some docker failures Error response from daemon: Head "https://registry-1.docker.io/v2/tonistiigi/binfmt/manifests/latest": unauthorized: incorrect username or password

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

cmd/jaeger/internal/extension/jaegerquery/server.go

cmd/query/app/additional_headers_handler.go

cmd/query/app/additional_headers_test.go

cmd/query/app/additional_headers_handler.go

cmd/query/app/server.go

yurishkuro · 2024-09-27T03:46:44Z

pkg/config/tlscfg/options.go

@@ -142,6 +142,28 @@ func (o *Options) ToOtelClientConfig() configtls.ClientConfig {
 	}
 }

+// ToOtelServerConfig provides a mapping between from Options to OTEL's TLS Server Configuration.
+func (o *Options) ToOtelServerConfig() configtls.ServerConfig {
+	cfg := configtls.ServerConfig{


what about translating Enabled to Insecure?

@yurishkuro The Insecure field doesn't actually exist on configtls.ServerConfig, it only exists on configtls.ClientConfig (see https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/configtls.go#L112-L126). The TLSSetting field in configgrpc.TLSSetting and confighttp.TLSSetting is a pointer type so setting it to nil implies no TLS/ insecure (see https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configgrpc/configgrpc.go#L168-L171).

I changed this method to return a pointer and return nil when !o.Enabled.

pkg/config/tlscfg/options_test.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

cmd/jaeger/internal/extension/jaegerquery/server.go

cmd/query/app/flags.go

yurishkuro · 2024-09-28T15:31:41Z

Please update https://docs.google.com/document/d/1ydTrEtIqJZxGb0RBkUAW-ZIaHNijo1pZSrn1TDSK-LU/edit#heading=h.qbckwiyz30oe

cmd/query/app/flags.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 · 2024-09-28T15:38:14Z

Please update https://docs.google.com/document/d/1ydTrEtIqJZxGb0RBkUAW-ZIaHNijo1pZSrn1TDSK-LU/edit#heading=h.qbckwiyz30oe

@yurishkuro what did we want to update here?

cmd/jaeger/internal/extension/jaegerquery/server.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

cmd/query/app/server.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2024-09-28T16:14:44Z

what did we want to update here?

if it's still accurate then nothing, I thought maybe with this refactoring there might be changes

yurishkuro

🎉

mahadzaryab1 · 2024-09-28T16:15:44Z

what did we want to update here?

if it's still accurate then nothing, I thought maybe with this refactoring there might be changes

Oh I see. Yeah no changes required here because we didn't touch the CLI interface - just the internals of where the configs were being held.

yurishkuro · 2024-09-28T16:18:49Z

I think this might be a breaking change if we are switching from internal TLS management to OTEL's because our internal tlscfg has a hot-reload enabled by watching the file system for changes (and therefore using a goroutine and forcing callers to call tls.Close()), whereas OTEL reloads certs every reload_interval when the servers ask for the certs.

mahadzaryab1 added 4 commits September 26, 2024 20:38

Add Function To Map To OTEL's TLS Server Config

09adac6

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Migrate To OTEL's Server Configurations

cc7acf6

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Redundant QueryOptionsBase Type

fd54aa6

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Run Formatter

4cbb74d

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Method

eb9d713

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Simplify Config

fcf5967

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro reviewed Sep 27, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerquery/server.go Outdated Show resolved Hide resolved

yurishkuro reviewed Sep 27, 2024

View reviewed changes

cmd/query/app/additional_headers_handler.go Outdated Show resolved Hide resolved

yurishkuro reviewed Sep 27, 2024

View reviewed changes

cmd/query/app/additional_headers_handler.go Show resolved Hide resolved

yurishkuro reviewed Sep 27, 2024

View reviewed changes

cmd/query/app/additional_headers_test.go Show resolved Hide resolved

yurishkuro reviewed Sep 27, 2024

View reviewed changes

cmd/query/app/additional_headers_handler.go Outdated Show resolved Hide resolved

yurishkuro reviewed Sep 27, 2024

View reviewed changes

cmd/query/app/server.go Show resolved Hide resolved

yurishkuro reviewed Sep 27, 2024

View reviewed changes

pkg/config/tlscfg/options_test.go Outdated Show resolved Hide resolved

Address Feedback From PR Review

e62e8ea

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro added the changelog:exprimental Change to an experimental part of the code label Sep 28, 2024

yurishkuro reviewed Sep 28, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerquery/server.go Outdated Show resolved Hide resolved

yurishkuro reviewed Sep 28, 2024

View reviewed changes

cmd/query/app/flags.go Show resolved Hide resolved

yurishkuro reviewed Sep 28, 2024

View reviewed changes

cmd/query/app/flags.go Outdated Show resolved Hide resolved

mahadzaryab1 marked this pull request as ready for review September 28, 2024 15:35

mahadzaryab1 requested a review from a team as a code owner September 28, 2024 15:35

mahadzaryab1 requested a review from joe-elliott September 28, 2024 15:35

dosubot bot added area/otel v2 labels Sep 28, 2024

Address Feedback From PR Review

ac8b608

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro September 28, 2024 15:39

yurishkuro reviewed Sep 28, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerquery/server.go Outdated Show resolved Hide resolved

Remove Unnecessary Function

c1e57dd

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro reviewed Sep 28, 2024

View reviewed changes

cmd/query/app/server.go Outdated Show resolved Hide resolved

mahadzaryab1 added 2 commits September 28, 2024 12:08

Remove Allocation

c8c66da

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Test Name

f32737b

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro approved these changes Sep 28, 2024

View reviewed changes

yurishkuro added changelog:breaking-change Change that is breaking public APIs or established behavior and removed changelog:exprimental Change to an experimental part of the code labels Sep 28, 2024

yurishkuro changed the title ~~[jaeger-v2] Consolidate HTTP and TLS Server Configurations For Query Service To Use OTEL Configurations~~ [query] Change HTTP and TLS server configurations to use OTEL configurations Sep 28, 2024

yurishkuro enabled auto-merge (squash) September 28, 2024 16:21

yurishkuro disabled auto-merge September 28, 2024 16:21

yurishkuro enabled auto-merge (squash) September 28, 2024 16:22

yurishkuro merged commit 731f50e into jaegertracing:main Sep 28, 2024
51 checks passed

yurishkuro mentioned this pull request Sep 28, 2024

[query] Switch to use OTEL's http/grpc servers #6026

Closed

mahadzaryab1 deleted the consolidate-query-http branch September 28, 2024 16:33

crystalgreen mentioned this pull request Nov 20, 2024

[Bug]: query: TLS handshake error #6230

Closed

[query] Change HTTP and TLS server configurations to use OTEL configurations #6023

[query] Change HTTP and TLS server configurations to use OTEL configurations #6023

Uh oh!

Conversation

mahadzaryab1 commented Sep 27, 2024 • edited by yurishkuro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

codecov bot commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mahadzaryab1 commented Sep 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yurishkuro Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

mahadzaryab1 Sep 28, 2024

Choose a reason for hiding this comment

Uh oh!

mahadzaryab1 Sep 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yurishkuro commented Sep 28, 2024

Uh oh!

Uh oh!

mahadzaryab1 commented Sep 28, 2024

Uh oh!

Uh oh!

Uh oh!

yurishkuro commented Sep 28, 2024

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

mahadzaryab1 commented Sep 28, 2024

Uh oh!

yurishkuro commented Sep 28, 2024

Uh oh!

Uh oh!

Uh oh!

mahadzaryab1 commented Sep 27, 2024 •

edited by yurishkuro

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading