Extract gRPC TLS configuration into a shared package by yurishkuro · Pull Request #1840 · jaegertracing/jaeger

yurishkuro · 2019-10-07T06:00:47Z

Part of #1837

Moves TLS configuration from gRPC client (agent) and server (collector) into a shared package pkg/config/tlscfg. This allows fewer unit tests to be written for the call sites, since all error handling of TLS certificates loading is covered in the new package.

Signed-off-by: Yuri Shkuro <ys@uber.com>

jpkrohling

Is this ready for review? If so, what's the deal with the commented out tests?

cmd/agent/app/reporter/grpc/builder_test.go

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro · 2019-10-08T01:49:42Z

cmd/agent/app/reporter/grpc/flags.go

-	flags.String(collectorTLSCA, "", "Path to a TLS CA file used to verify the remote server. (default use the systems truststore)")
-	flags.String(collectorTLSServerName, "", "Override the TLS server name we expected in the remote certificate")
-	flags.String(agentCert, "", "Path to a TLS client certificate file, used to identify this agent to the collector")
-	flags.String(agentKey, "", "Path to the TLS client key for the client certificate")


Now:

$ go run ./cmd/agent help | grep tls --reporter.grpc.tls Enable TLS when talking to the remote server(s) --reporter.grpc.tls.ca string Path to a TLS CA (Certification Authority) file used to verify the remote server(s) (by default will use the system truststore) --reporter.grpc.tls.cert string Path to a TLS Certificate file, used to identify this process to the remote server(s) --reporter.grpc.tls.key string Path to a TLS Private Key file, used to identify this process to the remote server(s) --reporter.grpc.tls.server-name string Override the TLS server name we expect in the certificate of the remove server(s)

Signed-off-by: Yuri Shkuro <ys@uber.com>

codecov · 2019-10-08T02:01:57Z

Codecov Report

Merging #1840 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1840      +/-   ##
==========================================
+ Coverage   98.21%   98.44%   +0.22%     
==========================================
  Files         195      197       +2     
  Lines        9603     9619      +16     
==========================================
+ Hits         9432     9469      +37     
+ Misses        134      114      -20     
+ Partials       37       36       -1

Impacted Files	Coverage Δ
cmd/collector/app/grpcserver/grpc_server.go	`100% <ø> (+47.5%)`	⬆️
cmd/agent/app/reporter/grpc/collector_proxy.go	`100% <ø> (ø)`	⬆️
cmd/agent/app/reporter/grpc/flags.go	`100% <100%> (ø)`	⬆️
pkg/config/tlscfg/options.go	`100% <100%> (ø)`
pkg/config/tlscfg/flags.go	`100% <100%> (ø)`
cmd/collector/app/builder/builder_flags.go	`100% <100%> (ø)`	⬆️
cmd/agent/app/reporter/grpc/builder.go	`100% <100%> (+3.44%)`	⬆️

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 fcc0adb...207c01b. Read the comment docs.

yurishkuro · 2019-10-08T04:27:04Z

cmd/collector/app/builder/builder_flags.go

-	flags.Bool(collectorGRPCTLS, false, "Enable TLS for the gRPC collector port")
-	flags.String(collectorGRPCCert, "", "Path to TLS certificate for the gRPC collector TLS service")
-	flags.String(collectorGRPCKey, "", "Path to TLS key for the gRPC collector TLS cert")
-	flags.String(collectorGRPCClientCA, "", "Path to a TLS CA to verify certificates presented by clients (if unset, all clients are permitted)")


now:

$ go run ./cmd/collector help | grep grpc.tls --collector.grpc.tls Enable TLS on the server --collector.grpc.tls.cert string Path to a TLS Certificate file, used to identify this server to clients --collector.grpc.tls.client-ca string Path to a TLS CA (Certification Authority) file used to verify certificates presented by clients (if unset, all clients are permitted) --collector.grpc.tls.client.ca string (deprecated) see --collector.grpc.tls.client-ca --collector.grpc.tls.key string Path to a TLS Private Key file, used to identify this server to clients

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro · 2019-10-08T06:19:25Z

Is this ready for review? If so, what's the deal with the commented out tests?

Sorry, I pushed as WIP to show @backjo the approach I was taking. It's now ready for review.

vprithvi · 2019-10-08T17:46:43Z

cmd/agent/app/reporter/grpc/builder_test.go

 				}
+			} else {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), test.expectedError)


https://godoc.org/github.com/stretchr/testify/assert#EqualError is more concise

That function performs exact comparison of the error message. Since the error is wrapped multiple times, the message consists of multiple segments. I don't know or care what the underlying stdlib error was, I only care about the substring introduced by the code under test.

) * Extract TLS flags and cert loading logic Signed-off-by: Yuri Shkuro <ys@uber.com> * Rename package Signed-off-by: Yuri Shkuro <ys@uber.com> * Refactor grpc Signed-off-by: Yuri Shkuro <ys@uber.com> * Repair tests Signed-off-by: Yuri Shkuro <ys@uber.com> * Refactor gRPC server in collector Signed-off-by: Yuri Shkuro <ys@uber.com> * Add ShowCA option Signed-off-by: Yuri Shkuro <ys@uber.com> * Switch options order Signed-off-by: Yuri Shkuro <ys@uber.com> * Separate client and server TLS options Signed-off-by: Yuri Shkuro <ys@uber.com> * Update usage Signed-off-by: Yuri Shkuro <ys@uber.com> * Rename test, add filepath.Clean Signed-off-by: Yuri Shkuro <ys@uber.com> Signed-off-by: Jonah Back <jonah@jonahback.com>

Yuri Shkuro added 3 commits October 6, 2019 21:22

Extract TLS flags and cert loading logic

fbf781b

Signed-off-by: Yuri Shkuro <ys@uber.com>

Rename package

cb41f77

Signed-off-by: Yuri Shkuro <ys@uber.com>

Refactor grpc

10f6f6d

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon and vprithvi as code owners October 7, 2019 06:00

yurishkuro mentioned this pull request Oct 7, 2019

Add common TLS configuration #1838

Merged

jpkrohling reviewed Oct 7, 2019

View reviewed changes

cmd/agent/app/reporter/grpc/builder_test.go Outdated Show resolved Hide resolved

yurishkuro changed the title ~~Refactor tls certs~~ WIP: Refactor tls certs Oct 7, 2019

Yuri Shkuro added 3 commits October 7, 2019 18:21

Repair tests

b3d7bcb

Signed-off-by: Yuri Shkuro <ys@uber.com>

Refactor gRPC server in collector

313cb6b

Signed-off-by: Yuri Shkuro <ys@uber.com>

Add ShowCA option

9bb47db

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro commented Oct 8, 2019

View reviewed changes

Switch options order

e8aae0a

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro commented Oct 8, 2019

View reviewed changes

Yuri Shkuro added 3 commits October 7, 2019 22:33

Separate client and server TLS options

328c2b6

Signed-off-by: Yuri Shkuro <ys@uber.com>

Update usage

4cf2928

Signed-off-by: Yuri Shkuro <ys@uber.com>

Rename test, add filepath.Clean

207c01b

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro changed the title ~~WIP: Refactor tls certs~~ Move TLS logic from gRPC client/server into a shared package Oct 8, 2019

yurishkuro changed the title ~~Move TLS logic from gRPC client/server into a shared package~~ Move TLS configuration from gRPC client/server into a shared package Oct 8, 2019

yurishkuro changed the title ~~Move TLS configuration from gRPC client/server into a shared package~~ Extract gRPC TLS configuration into a shared package Oct 8, 2019

vprithvi reviewed Oct 8, 2019

View reviewed changes

vprithvi approved these changes Oct 8, 2019

View reviewed changes

yurishkuro merged commit d553583 into jaegertracing:master Oct 8, 2019

yurishkuro deleted the refactor-tls-certs branch October 8, 2019 21:33

yurishkuro added this to the Release 1.15 milestone Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract gRPC TLS configuration into a shared package#1840

Extract gRPC TLS configuration into a shared package#1840
yurishkuro merged 10 commits intojaegertracing:masterfrom
yurishkuro:refactor-tls-certs

yurishkuro commented Oct 7, 2019 •

edited

Loading

Uh oh!

jpkrohling left a comment

Uh oh!

Uh oh!

yurishkuro Oct 8, 2019 •

edited

Loading

Uh oh!

codecov bot commented Oct 8, 2019 •

edited

Loading

Uh oh!

yurishkuro Oct 8, 2019 •

edited

Loading

Uh oh!

yurishkuro commented Oct 8, 2019

Uh oh!

vprithvi Oct 8, 2019

Uh oh!

yurishkuro Oct 8, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

yurishkuro commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpkrohling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yurishkuro Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yurishkuro Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurishkuro commented Oct 8, 2019

Uh oh!

vprithvi Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

yurishkuro Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yurishkuro commented Oct 7, 2019 •

edited

Loading

yurishkuro Oct 8, 2019 •

edited

Loading

codecov bot commented Oct 8, 2019 •

edited

Loading

yurishkuro Oct 8, 2019 •

edited

Loading

yurishkuro Oct 8, 2019 •

edited

Loading