allow use of secure gRPC connections between agent and collector

[ghouscht]

Which problem is this PR solving?

Resolves #1310

Short description of the changes

Implementation for a secure gRPC connection between the agent and the collector.

[yurishkuro]

lgtm

[ghouscht]

Looks like an unrelated test is now failing in CI: TestSaramaConsumerWrapper_start_Messages. Can't reproduce it on my machine, is this test known to be flaky/broken?

[yurishkuro]

possibly, I restarted the build to see.

[yurishkuro]

Confirmed flaky test - #1314

[yurishkuro]

@ghouscht what is your action plan with this PR? Are you going to add the equivalent settings to the collector?

[ghouscht]

Yes, today I wrote some very basic unit tests for the agent part (just flags and ProxyBuilder).
Will have a look at the collector code tomorrow to add those settings there to. The only thing left then, is some sort of e2e test, but don't know at the moment how to do that...

[codecov]

Codecov Report

Merging #1311 into master will decrease coverage by 0.02%.
The diff coverage is 93.1%.

@@            Coverage Diff             @@
##           master    #1311      +/-   ##
==========================================
- Coverage     100%   99.97%   -0.03%     
==========================================
  Files         164      164              
  Lines        7453     7488      +35     
==========================================
+ Hits         7453     7486      +33     
- Misses          0        1       +1     
- Partials        0        1       +1

Impacted Files	Coverage Δ
cmd/collector/app/builder/builder_flags.go	100% <100%> (ø)	⬆️
cmd/agent/app/reporter/grpc/flags.go	100% <100%> (ø)	⬆️
cmd/agent/app/reporter/grpc/collector_proxy.go	95.45% <88.23%> (-4.55%)	⬇️
plugin/storage/es/dependencystore/storage.go	100% <0%> (ø)	⬆️

---

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 d8ab694...37552b4. Read the comment docs.

[ghouscht]

Code for the collector part now also finished. Tested it on my local machine and seems to work well.

things left TODO:

• agent: rewrite unit test TestProxyBuilderwith custom CA (open for ideas) possible idea: write a cert to a tempfile and use this file to test the NewCollectorProxy method with a real CA or is the test like it is now ok?
• collector: do we need a custom error message if the user starts the collector with --collector.grpc.tls but does not provide a cert and key? Or just stay with the current error which is like: "Could not start gRPC collector","error":"failed to load TLS keys: open : no such file or directory" (not ideal in my opinion)?
• E2E test? how?

[yurishkuro]

loading certs from a file in /tmp might be a good way to unit test the initialization paths

[ghouscht]

Added pkg/testutils/certificate.go, maybe a bit overkill at the moment. But this method could be used too for an e2e/integration test where a cert and key is needed.

Do you think there is anything left which needs to be done? E2E test or tests for the initialization paths of the collector in cmd/collector/main.go (currently no tests at all for this part of the code)?

[yurishkuro]

we do have integration tests running binaries, crossdock/docker-compose.yml, but it's hard to extend. In fact, we still haven't switched it to use gRPC.

[ghouscht]

Cleaned up the PR and removed the certificate stuff. TestSaramaConsumerWrapper_start_Messages is failing again... (this time with a different error I think so).

[benley]

You'll probably want to add a ssl trusted roots bundle to the jaeger-agent docker image, similar to what already exists in jaeger-collector's Dockerfile:

--- a/cmd/agent/Dockerfile
+++ b/cmd/agent/Dockerfile
@@ -1,5 +1,10 @@
+FROM alpine:latest as certs
+RUN apk add --update --no-cache ca-certificates
+
 FROM scratch
 
+COPY --from=certs /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
+
 EXPOSE 5775/udp 6831/udp 6832/udp 5778
 COPY agent-linux /go/bin/
-ENTRYPOINT ["/go/bin/agent-linux"]
+ENTRYPOINT ["/go/bin/agent-linux", "--reporter.grpc.tls.ca=/etc/ssl/certs/ca-certificates.crt"]

[ghouscht]

@yurishkuro how to proceed now with this PR?

[ghouscht]

Looks like an unrelated test is failing again? What about coverage, is it ok like this?

[pavolloffay]

The coverage should remain at 100%.

[ghouscht]

Ok, catching the first miss here could be hard, or at least I don't know how to do this, any ideas?

The second miss here shouldn't be a problem, will do this in a few minutes.

[yurishkuro]

@ghouscht I fixed coverage by adding one more test (#1391). I can get it approved and merged tomorrow, or you can merge it into your branch, and then we can merge this PR.

NB: in the future I recommend not committing directly to master in your fork, but creating a branch (as I did in #1391). Had you done that, I would've been able to push these extra commits directly into your branch and update this PR automatically.

[ghouscht]

I'm out of the office today. Will merge the changes into my branch tomorrow and squash the commits together.

[yurishkuro]

merged as #1391