TLS support for HTTP API of Query server

[rjs211]

Which problem is this PR solving?

• Adds TLS support for HTTP endpoint of query server
• solves part of issue Support TLS and mTLS in collector and query HTTP servers #2249
• Resolves gRPC with TLS doesn't work with cmux in mixed mode #2338

Short description of the changes

• Similar to PR TLS support for gRPC Query server #2297 .

• 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.

• forces the user to use dedicated HTTP and gRPC ports if TLS is enabled in any of the endpoints.

[jpkrohling]

As @yurishkuro mentioned in Gitter, there's a bug in the current code, causing the flags to not be exposed. You'll need to add the following to the flags.go, #AddFlags method:

	tlsGrpcFlagsConfig.AddFlags(flagSet)
	tlsHttpFlagsConfig.AddFlags(flagSet)

Please, fix the naming of the properties and vars as well: gRPC should be always GRPC and Http should be HTTP. Other than that, looks good to me.

[jpkrohling]

We may want to hold on merging this before #2338 is sorted out.

[yurishkuro]

Please describe if/how you performed the real integration test that uses certificates. Would be nice to add those to the suite of integration tests in the CI.

[tcolgate]

I used mkcert I think.

…

On Mon, 13 Jul 2020, 04:27 Yuri Shkuro, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cmd/query/app/server_test.go <#2337 (comment)>: > @@ -53,7 +53,20 @@ func TestCreateTLSServerError(t *testing.T) { } _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, - &QueryOptions{TLS: tlsCfg}, opentracing.NoopTracer{}) + &QueryOptions{TLSGrpc: tlsCfg}, opentracing.NoopTracer{}) + assert.NotNil(t, err) +} + +func TestCreateTLSHttpServerError(t *testing.T) { + tlsCfg := tlscfg.Options{ + Enabled: true, + CertPath: "invalid/path", + KeyPath: "invalid/path", + ClientCAPath: "invalid/path", + } + + _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, I am not sure. We also have another set of test certificates under ./cmd/agent/app/reporter/grpc/testdata, and they are used for the actual client/server test in TestProxyClientTLS -- ./cmd/agent/app/reporter/grpc/builder_test.go. @tcolgate <https://github.com/tcolgate> do you remember how you generated those certificates? We should have documented it somewhere, either in testdata/README or have a Makefile target. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2337 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAASR47VJOQPZVH6GYAKMPDR3J5IXANCNFSM4OVJWF2A> .

[codecov]

Codecov Report

Merging #2337 (1bee20f) into master (e788e55) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2337      +/-   ##
==========================================
+ Coverage   95.73%   95.81%   +0.08%     
==========================================
  Files         216      216              
  Lines        9593     9615      +22     
==========================================
+ Hits         9184     9213      +29     
+ Misses        336      332       -4     
+ Partials       73       70       -3     

Impacted Files	Coverage Δ
cmd/query/app/flags.go	100.00% <100.00%> (ø)
cmd/query/app/server.go	97.12% <100.00%> (+8.59%)	⬆️
...lugin/sampling/strategystore/adaptive/processor.go	99.07% <0.00%> (-0.93%)	⬇️
plugin/storage/integration/integration.go	77.34% <0.00%> (-0.56%)	⬇️

---

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 e788e55...1bee20f. Read the comment docs.

[jpkrohling]

LGTM. I'll leave it open for a day, to see if other maintainers want to review it as well.

[jpkrohling]

@rjs211 are you still interested in working on this one?

Pull request has been modified.

[davidcmitchell]

@rjs211 Are you still working on this issue?

[rjs211]

I had finished this task and all the test cases had passed. I had received one approval and was waiting for a second one. I see now that in the meantime the branch is our of date with master. I'll start working on this again.

[jpkrohling]

Is this ready for a review?

[rjs211]

Yes. It is. There weren't any merge conflicts. Ready for review.

[jpkrohling]

I'll take another look tomorrow! Thanks for working on this one.

[jpkrohling]

Sorry, didn't have time to look into this yet, but should be able to look at this at most early next week.

[jpkrohling]

Would you like to give a final review, @yurishkuro ? From my perspective, this can be merged already.

[yurishkuro]

@rjs211 thanks! Could you add another PR that describes these changes in the CHANGELOG? This change is not as straightforward as the PR title implies, due to the tricky handling of separate ports.

[rjs211]

@yurishkuro Hello, and apologies for delayed response. My personal laptop was under repair. I was looking at the entries in https://github.com/jaegertracing/jaeger/blob/master/CHANGELOG.md and they seem to be one liners without much detail. Could you please show me an example where some logic is explained?

Would writing a documentation help? If so, could you please direct me to the correct place to write the same? Thanks.

[yurishkuro]

See, for example, the Breaking Changes section in https://github.com/jaegertracing/jaeger/blob/master/CHANGELOG.md#1200-2020-09-29

We don't need to explain the logic, but what is changing and how those changes affect users.