TLS support for HTTP Endpoint of Collector server #2798

rjs211 · 2021-02-07T06:31:23Z

Which problem is this PR solving?

solves part of Support TLS and mTLS in collector and query HTTP servers #2249

Short description of the changes

independent TLS flags are exposed for gRPC and HTTP endpoints, enabling the user to provide different set of key, cert, CA-Cert , etc for each communication channal.
provides the option of enabling TLS/mTLS in none, either one or both of HTTP and gRPC endpoints.

Todo

CHANGELOG.md entry

Signed-off-by: rjs211 <[email protected]>

… dev-TLSColctr

yurishkuro

Overall lgtm. The unit tests are failing to compile though.

cmd/collector/app/collector.go

yurishkuro · 2021-02-07T06:41:13Z

cmd/collector/app/builder_flags.go

@@ -40,12 +40,18 @@ const (
 	collectorZipkinAllowedHeaders = "collector.zipkin.allowed-headers"
 )

-var tlsFlagsConfig = tlscfg.ServerFlagsConfig{
+var tlsGRPCFlagsConfig = tlscfg.ServerFlagsConfig{
 	Prefix:       "collector.grpc",
 	ShowEnabled:  true,


Perhaps we should remove this option, it is never false

$ grx 'ShowEnabled: ' . ./cmd/collector/app/builder_flags.go:45: ShowEnabled: true, ./cmd/agent/app/reporter/grpc/flags.go:36: ShowEnabled: true, ./cmd/query/app/flags.go:52: ShowEnabled: true, ./cmd/query/app/flags.go:58: ShowEnabled: true, ./plugin/storage/cassandra/options.go:250: ShowEnabled: true, ./plugin/storage/es/options.go:139: ShowEnabled: true, ./pkg/config/tlscfg/flags_test.go:51: ShowEnabled: true, ./pkg/config/tlscfg/flags_test.go:98: ShowEnabled: true, ./pkg/kafka/auth/config.go:94: ShowEnabled: true, ./pkg/kafka/auth/options.go:113: ShowEnabled: true,

Do you want me to do this as a part of the same PR?

Signed-off-by: rjs211 <[email protected]>

codecov · 2021-02-07T10:26:20Z

Codecov Report

Merging #2798 (03107be) into master (169322f) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2798      +/-   ##
==========================================
+ Coverage   95.88%   95.93%   +0.04%     
==========================================
  Files         218      218              
  Lines        9606     9620      +14     
==========================================
+ Hits         9211     9229      +18     
+ Misses        327      323       -4     
  Partials       68       68

Impacted Files	Coverage Δ
cmd/collector/app/builder_flags.go	`100.00% <100.00%> (ø)`
cmd/collector/app/collector.go	`78.08% <100.00%> (+3.43%)`	⬆️
cmd/collector/app/server/http.go	`100.00% <100.00%> (+12.00%)`	⬆️
cmd/query/app/static_handler.go	`95.16% <0.00%> (-1.62%)`	⬇️
cmd/collector/app/server/zipkin.go	`76.92% <0.00%> (+3.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 169322f...cd29744. Read the comment docs.

Signed-off-by: rjs211 <[email protected]>

yurishkuro

Code lgtm, but coverage will decrease by 0.10%. Can you add tests to exercise the new paths?

Signed-off-by: rjs211 <[email protected]>

cmd/collector/app/server/http_test.go

Signed-off-by: rjs211 <[email protected]>

yurishkuro · 2021-02-08T17:03:36Z

any other tests could be added to increase the PR coverage above 95%?

cmd/collector/app/collector.go

cmd/collector/app/server/http_test.go

…thod Signed-off-by: rjs211 <[email protected]>

yurishkuro · 2021-02-08T22:57:11Z

codeov is stuck again, but the report shows +0.04%

https://codecov.io/github/jaegertracing/jaeger/commit/03107bed1254f3fc1b5fa2c6ae6d833856412c64

rjs211 · 2021-02-08T23:02:52Z

@yurishkuro Actually im yet to make CHANGELOG.md entry. Is it possible to revert the commit? I dont want to waste another PR for that..

UPDATE: never mind. Just learnt that reverting creates a new PR as well!! sorry to bother you.

yurishkuro · 2021-02-08T23:18:29Z

No need, as this is not a breaking change but a new feature it will be added as part of the release process. Also, we have an abundant number of available PR numbers, so it's ok to waste them 😛

rjs211 added 2 commits February 7, 2021 11:47

Adding TLS flags for collector HTTP endpoint

78df329

Signed-off-by: rjs211 <[email protected]>

Merge branch 'master' of https://github.com/jaegertracing/jaeger into…

de296c5

… dev-TLSColctr

rjs211 requested a review from a team as a code owner February 7, 2021 06:31

rjs211 requested a review from vprithvi February 7, 2021 06:31

mergify bot assigned jpkrohling Feb 7, 2021

mergify bot requested a review from jpkrohling February 7, 2021 06:31

rjs211 marked this pull request as draft February 7, 2021 06:41

yurishkuro reviewed Feb 7, 2021

View reviewed changes

correcting config mismatch, failing unit test

62b6d79

Signed-off-by: rjs211 <[email protected]>

minor refactor

d3c2c8d

Signed-off-by: rjs211 <[email protected]>

rjs211 force-pushed the dev-TLSColctr branch from 6ea977c to f40c12b Compare February 7, 2021 10:47

added testcase for collector's HTTP TLS at receiver

5ba777b

Signed-off-by: rjs211 <[email protected]>

rjs211 force-pushed the dev-TLSColctr branch from f40c12b to 5ba777b Compare February 7, 2021 11:29

yurishkuro reviewed Feb 7, 2021

View reviewed changes

rjs211 added 2 commits February 8, 2021 21:18

rearraging for error logic

0915bca

Signed-off-by: rjs211 <[email protected]>

Adding table based test cases

a07970e

Signed-off-by: rjs211 <[email protected]>

yurishkuro reviewed Feb 8, 2021

View reviewed changes

cmd/collector/app/server/http_test.go Outdated Show resolved Hide resolved

dummy commit for git resync

473c063

Signed-off-by: rjs211 <[email protected]>

rjs211 marked this pull request as ready for review February 8, 2021 16:44

rjs211 commented Feb 8, 2021

View reviewed changes

cmd/collector/app/collector.go Outdated Show resolved Hide resolved

davidcmitchell mentioned this pull request Feb 8, 2021

Support TLS and mTLS in collector and query HTTP servers #2249

Closed

2 tasks

rjs211 commented Feb 8, 2021

View reviewed changes

cmd/collector/app/server/http_test.go Show resolved Hide resolved

removing redundant error capture for TLS certificate watcher close me…

cd29744

…thod Signed-off-by: rjs211 <[email protected]>

yurishkuro merged commit d23b3e2 into jaegertracing:master Feb 8, 2021

albertteoh mentioned this pull request Feb 23, 2021

Preparing release v1.22.0 #2826

Merged

dependabot bot mentioned this pull request Mar 8, 2021

Bump github.com/jaegertracing/jaeger from 1.21.0 to 1.22.0 httpsgithu/opentelemetry-collector#22

Closed

jannyHou mentioned this pull request May 20, 2021

Support specifying cipher suites in tls connection #3019

Closed

Saad-Hussain1 mentioned this pull request Sep 28, 2021

Outdated info in docs? jaegertracing/documentation#525

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS support for HTTP Endpoint of Collector server #2798

TLS support for HTTP Endpoint of Collector server #2798

rjs211 commented Feb 7, 2021 •

edited

Loading

yurishkuro left a comment

yurishkuro Feb 7, 2021

rjs211 Feb 7, 2021

yurishkuro Feb 7, 2021

codecov bot commented Feb 7, 2021 •

edited

Loading

yurishkuro left a comment

yurishkuro commented Feb 8, 2021

yurishkuro commented Feb 8, 2021

rjs211 commented Feb 8, 2021 •

edited

Loading

yurishkuro commented Feb 8, 2021

TLS support for HTTP Endpoint of Collector server #2798

TLS support for HTTP Endpoint of Collector server #2798

Conversation

rjs211 commented Feb 7, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

Todo

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Feb 7, 2021

Choose a reason for hiding this comment

rjs211 Feb 7, 2021

Choose a reason for hiding this comment

yurishkuro Feb 7, 2021

Choose a reason for hiding this comment

codecov bot commented Feb 7, 2021 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Feb 8, 2021

yurishkuro commented Feb 8, 2021

rjs211 commented Feb 8, 2021 • edited Loading

yurishkuro commented Feb 8, 2021

rjs211 commented Feb 7, 2021 •

edited

Loading

codecov bot commented Feb 7, 2021 •

edited

Loading

rjs211 commented Feb 8, 2021 •

edited

Loading