From 419f74b0040564807f890314dfc556abfd429b0a Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Sun, 21 Jun 2020 19:04:35 +0200 Subject: [PATCH 1/6] Add TLS options for query service Signed-off-by: Abhilash Gnan --- cmd/query/app/flags.go | 10 ++++++++++ cmd/query/app/server.go | 35 ++++++++++++++++++++++++++++------- cmd/query/main.go | 6 +++++- 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index b34a1b70306..80f4e2ea152 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -31,6 +31,7 @@ import ( "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/model/adjuster" "github.com/jaegertracing/jaeger/pkg/config" + "github.com/jaegertracing/jaeger/pkg/config/tlscfg" "github.com/jaegertracing/jaeger/ports" "github.com/jaegertracing/jaeger/storage" ) @@ -47,6 +48,12 @@ const ( queryMaxClockSkewAdjust = "query.max-clock-skew-adjustment" ) +var tlsFlagsConfig = tlscfg.ServerFlagsConfig{ + Prefix: "query.grpc", + ShowEnabled: true, + ShowClientCA: true, +} + // QueryOptions holds configuration for query service type QueryOptions struct { // HostPort is the host:port address that the query service listens o n @@ -59,6 +66,8 @@ type QueryOptions struct { UIConfig string // BearerTokenPropagation activate/deactivate bearer token propagation to storage BearerTokenPropagation bool + // TLS configures secure transport + TLS tlscfg.Options // AdditionalHeaders AdditionalHeaders http.Header // MaxClockSkewAdjust is the maximum duration by which jaeger-query will adjust a span @@ -84,6 +93,7 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *Qu qOpts.StaticAssets = v.GetString(queryStaticFiles) qOpts.UIConfig = v.GetString(queryUIConfig) qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation) + qOpts.TLS = tlsFlagsConfig.InitFromViper(v) qOpts.MaxClockSkewAdjust = v.GetDuration(queryMaxClockSkewAdjust) stringSlice := v.GetStringSlice(queryAdditionalHeaders) diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 462575b4553..952f16be912 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -24,6 +24,7 @@ import ( "github.com/soheilhy/cmux" "go.uber.org/zap" "google.golang.org/grpc" + "google.golang.org/grpc/credentials" "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/pkg/healthcheck" @@ -47,16 +48,21 @@ type Server struct { } // NewServer creates and initializes Server -func NewServer(logger *zap.Logger, querySvc *querysvc.QueryService, options *QueryOptions, tracer opentracing.Tracer) *Server { +func NewServer(logger *zap.Logger, querySvc *querysvc.QueryService, options *QueryOptions, tracer opentracing.Tracer) (*Server, error) { + grpcServer, err := createGRPCServer(querySvc, options, logger, tracer) + if err != nil { + return nil, err + } + return &Server{ logger: logger, querySvc: querySvc, queryOptions: options, tracer: tracer, - grpcServer: createGRPCServer(querySvc, logger, tracer), + grpcServer: grpcServer, httpServer: createHTTPServer(querySvc, options, tracer, logger), unavailableChannel: make(chan healthcheck.Status), - } + }, nil } // HealthCheckStatus returns health check status channel a client can subscribe to @@ -64,11 +70,26 @@ func (s Server) HealthCheckStatus() chan healthcheck.Status { return s.unavailableChannel } -func createGRPCServer(querySvc *querysvc.QueryService, logger *zap.Logger, tracer opentracing.Tracer) *grpc.Server { - srv := grpc.NewServer() +func createGRPCServer(querySvc *querysvc.QueryService, options *QueryOptions, logger *zap.Logger, tracer opentracing.Tracer) (*grpc.Server, error) { + var server *grpc.Server + + if options.TLS.Enabled { + // user requested a server with TLS, setup creds + tlsCfg, err := options.TLS.Config() + if err != nil { + return nil, err + } + + creds := credentials.NewTLS(tlsCfg) + server = grpc.NewServer(grpc.Creds(creds)) + } else { + // server without TLS + server = grpc.NewServer() + } + handler := NewGRPCHandler(querySvc, logger, tracer) - api_v2.RegisterQueryServiceServer(srv, handler) - return srv + api_v2.RegisterQueryServiceServer(server, handler) + return server, nil } func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) *http.Server { diff --git a/cmd/query/main.go b/cmd/query/main.go index c2d94d3e044..b7e4987ce1b 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -106,7 +106,11 @@ func main() { dependencyReader, *queryServiceOptions) - server := app.NewServer(svc.Logger, queryService, queryOpts, tracer) + server, err := app.NewServer(svc.Logger, queryService, queryOpts, tracer) + if err != nil { + logger.Fatal("Failed to create server", zap.Error(err)) + } + go func() { for s := range server.HealthCheckStatus() { svc.SetHealthCheckStatus(s) From 7781d1a6e57dd8ed80a36fe50f6c2a6d77402642 Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Sun, 21 Jun 2020 19:09:24 +0200 Subject: [PATCH 2/6] Fix tests Signed-off-by: Abhilash Gnan --- cmd/query/app/server_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 520d96ceb17..fcc53001846 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -55,9 +55,10 @@ func TestServer(t *testing.T) { querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - server := NewServer(flagsSvc.Logger, querySvc, + server, err := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{HostPort: hostPort, BearerTokenPropagation: true}, opentracing.NoopTracer{}) + assert.Nil(t, err) assert.NoError(t, server.Start()) go func() { for s := range server.HealthCheckStatus() { @@ -95,7 +96,8 @@ func TestServerGracefulExit(t *testing.T) { querySvc := &querysvc.QueryService{} tracer := opentracing.NoopTracer{} - server := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{HostPort: ports.PortToHostPort(ports.QueryAdminHTTP)}, tracer) + server, err := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{HostPort: ports.PortToHostPort(ports.QueryAdminHTTP)}, tracer) + assert.Nil(t, err) assert.NoError(t, server.Start()) go func() { for s := range server.HealthCheckStatus() { @@ -121,7 +123,8 @@ func TestServerHandlesPortZero(t *testing.T) { querySvc := &querysvc.QueryService{} tracer := opentracing.NoopTracer{} - server := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{HostPort: ":0"}, tracer) + server, err := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{HostPort: ":0"}, tracer) + assert.Nil(t, err) assert.NoError(t, server.Start()) server.Close() From 941a56b9507e2b135ee086a4cf86534365643a0c Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Sun, 21 Jun 2020 20:24:11 +0200 Subject: [PATCH 3/6] Fix all-in-one Signed-off-by: Abhilash Gnan --- cmd/all-in-one/main.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index e4b8bef63aa..1794f3bb80d 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -224,7 +224,10 @@ func startQuery( ) *queryApp.Server { spanReader = storageMetrics.NewReadMetricsDecorator(spanReader, baseFactory.Namespace(metrics.NSOptions{Name: "query"})) qs := querysvc.NewQueryService(spanReader, depReader, *queryOpts) - server := queryApp.NewServer(svc.Logger, qs, qOpts, opentracing.GlobalTracer()) + server, err := queryApp.NewServer(svc.Logger, qs, qOpts, opentracing.GlobalTracer()) + if err != nil { + svc.Logger.Fatal("Could not start jaeger-query service", zap.Error(err)) + } go func() { for s := range server.HealthCheckStatus() { svc.SetHealthCheckStatus(s) From 9ad2bc62ee1f5b861437603e2910af21bbbad505 Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Mon, 22 Jun 2020 07:14:10 +0200 Subject: [PATCH 4/6] Fix otel all-in-one use of query-svc Signed-off-by: Abhilash Gnan --- cmd/opentelemetry/cmd/all-in-one/main.go | 5 ++++- cmd/query/app/server.go | 11 +++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/cmd/opentelemetry/cmd/all-in-one/main.go b/cmd/opentelemetry/cmd/all-in-one/main.go index 3ec0259aaf5..dc3818c5254 100644 --- a/cmd/opentelemetry/cmd/all-in-one/main.go +++ b/cmd/opentelemetry/cmd/all-in-one/main.go @@ -198,7 +198,10 @@ func startQuery(v *viper.Viper, logger *zap.Logger, exporter configmodels.Export *queryServiceOptions) tracerCloser := initTracer(logger) - server := queryApp.NewServer(logger, queryService, queryOpts, opentracing.GlobalTracer()) + server, err := queryApp.NewServer(logger, queryService, queryOpts, opentracing.GlobalTracer()) + if err != nil { + logger.Fatal("Could not create jaeger-query service", zap.Error(err)) + } if err := server.Start(); err != nil { return nil, nil, err } diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 952f16be912..1dac3d71abb 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -71,7 +71,7 @@ func (s Server) HealthCheckStatus() chan healthcheck.Status { } func createGRPCServer(querySvc *querysvc.QueryService, options *QueryOptions, logger *zap.Logger, tracer opentracing.Tracer) (*grpc.Server, error) { - var server *grpc.Server + var grpcOpts []grpc.ServerOption if options.TLS.Enabled { // user requested a server with TLS, setup creds @@ -79,14 +79,13 @@ func createGRPCServer(querySvc *querysvc.QueryService, options *QueryOptions, lo if err != nil { return nil, err } - creds := credentials.NewTLS(tlsCfg) - server = grpc.NewServer(grpc.Creds(creds)) - } else { - // server without TLS - server = grpc.NewServer() + + grpcOpts = append(grpcOpts, grpc.Creds(creds)) } + server := grpc.NewServer(grpcOpts...) + handler := NewGRPCHandler(querySvc, logger, tracer) api_v2.RegisterQueryServiceServer(server, handler) return server, nil From 472213cb67b2bd321a84b73165eac319178258bc Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Mon, 22 Jun 2020 18:21:19 +0200 Subject: [PATCH 5/6] Remove redundant code Signed-off-by: Abhilash Gnan --- cmd/opentelemetry/cmd/all-in-one/main.go | 2 +- cmd/query/app/server.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/opentelemetry/cmd/all-in-one/main.go b/cmd/opentelemetry/cmd/all-in-one/main.go index dc3818c5254..b3c76ac8a6b 100644 --- a/cmd/opentelemetry/cmd/all-in-one/main.go +++ b/cmd/opentelemetry/cmd/all-in-one/main.go @@ -200,7 +200,7 @@ func startQuery(v *viper.Viper, logger *zap.Logger, exporter configmodels.Export tracerCloser := initTracer(logger) server, err := queryApp.NewServer(logger, queryService, queryOpts, opentracing.GlobalTracer()) if err != nil { - logger.Fatal("Could not create jaeger-query service", zap.Error(err)) + return nil, nil, fmt.Errorf("Could not create jaeger-query service: %w", err) } if err := server.Start(); err != nil { return nil, nil, err diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 1dac3d71abb..3e4b6756462 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -74,7 +74,6 @@ func createGRPCServer(querySvc *querysvc.QueryService, options *QueryOptions, lo var grpcOpts []grpc.ServerOption if options.TLS.Enabled { - // user requested a server with TLS, setup creds tlsCfg, err := options.TLS.Config() if err != nil { return nil, err From d00015d2bd3f48b4d6424f3741fa647d0d5ef6f2 Mon Sep 17 00:00:00 2001 From: Abhilash Gnan Date: Tue, 23 Jun 2020 22:19:31 +0200 Subject: [PATCH 6/6] Test error on create tls query server Signed-off-by: Abhilash Gnan --- cmd/query/app/server_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index fcc53001846..33f8a36b802 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -27,6 +27,7 @@ import ( "github.com/jaegertracing/jaeger/cmd/flags" "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" + "github.com/jaegertracing/jaeger/pkg/config/tlscfg" "github.com/jaegertracing/jaeger/pkg/healthcheck" "github.com/jaegertracing/jaeger/ports" "github.com/jaegertracing/jaeger/proto-gen/api_v2" @@ -43,6 +44,19 @@ func TestServerError(t *testing.T) { assert.Error(t, srv.Start()) } +func TestCreateTLSServerError(t *testing.T) { + tlsCfg := tlscfg.Options{ + Enabled: true, + CertPath: "invalid/path", + KeyPath: "invalid/path", + ClientCAPath: "invalid/path", + } + + _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, + &QueryOptions{TLS: tlsCfg}, opentracing.NoopTracer{}) + assert.NotNil(t, err) +} + func TestServer(t *testing.T) { flagsSvc := flags.NewService(ports.QueryAdminHTTP) flagsSvc.Logger = zap.NewNop()