From 78df329a9b5ea91ad0397a34a7fd8f65cc8429e9 Mon Sep 17 00:00:00 2001 From: rjs211 Date: Sun, 7 Feb 2021 11:47:58 +0530 Subject: [PATCH 1/8] Adding TLS flags for collector HTTP endpoint Signed-off-by: rjs211 --- cmd/collector/app/builder_flags.go | 20 +++++++++++++++----- cmd/collector/app/collector.go | 23 +++++++++++++++-------- cmd/collector/app/server/http.go | 17 ++++++++++++++++- 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/cmd/collector/app/builder_flags.go b/cmd/collector/app/builder_flags.go index a02e55c34ab..353184d14bf 100644 --- a/cmd/collector/app/builder_flags.go +++ b/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, ShowClientCA: true, } +var tlsHTTPFlagsConfig = tlscfg.ServerFlagsConfig{ + Prefix: "collector.http", + ShowEnabled: true, + ShowClientCA: true, +} + // CollectorOptions holds configuration for collector type CollectorOptions struct { // DynQueueSizeMemory determines how much memory to use for the queue @@ -58,8 +64,10 @@ type CollectorOptions struct { CollectorHTTPHostPort string // CollectorGRPCHostPort is the host:port address that the collector service listens in on for gRPC requests CollectorGRPCHostPort string - // TLS configures secure transport - TLS tlscfg.Options + // TLSGRPC configures secure transport for gRPC endpoint to collect spans + TLSGRPC tlscfg.Options + // TLSHTTP configures secure transport for HTTP endpoint to collect spans + TLSHTTP tlscfg.Options // CollectorTags is the string representing collector tags to append to each and every span CollectorTags map[string]string // CollectorZipkinHTTPHostPort is the host:port address that the Zipkin collector service listens in on for http requests @@ -86,7 +94,8 @@ func AddFlags(flags *flag.FlagSet) { func AddOTELJaegerFlags(flags *flag.FlagSet) { flags.String(CollectorHTTPHostPort, ports.PortToHostPort(ports.CollectorHTTP), "The host:port (e.g. 127.0.0.1:14268 or :14268) of the collector's HTTP server") flags.String(CollectorGRPCHostPort, ports.PortToHostPort(ports.CollectorGRPC), "The host:port (e.g. 127.0.0.1:14250 or :14250) of the collector's GRPC server") - tlsFlagsConfig.AddFlags(flags) + tlsGRPCFlagsConfig.AddFlags(flags) + tlsHTTPFlagsConfig.AddFlags(flags) } // AddOTELZipkinFlags adds flag that are exposed by OTEL Zipkin receiver @@ -105,7 +114,8 @@ func (cOpts *CollectorOptions) InitFromViper(v *viper.Viper) *CollectorOptions { cOpts.CollectorTags = flags.ParseJaegerTags(v.GetString(collectorTags)) cOpts.CollectorZipkinAllowedOrigins = v.GetString(collectorZipkinAllowedOrigins) cOpts.CollectorZipkinAllowedHeaders = v.GetString(collectorZipkinAllowedHeaders) - cOpts.TLS = tlsFlagsConfig.InitFromViper(v) + cOpts.TLSGRPC = tlsHTTPFlagsConfig.InitFromViper(v) + cOpts.TLSHTTP = tlsGRPCFlagsConfig.InitFromViper(v) return cOpts } diff --git a/cmd/collector/app/collector.go b/cmd/collector/app/collector.go index 4f06e75a427..eb3e174c502 100644 --- a/cmd/collector/app/collector.go +++ b/cmd/collector/app/collector.go @@ -45,10 +45,11 @@ type Collector struct { spanHandlers *SpanHandlers // state, read only - hServer *http.Server - zkServer *http.Server - grpcServer *grpc.Server - tlsCloser io.Closer + hServer *http.Server + zkServer *http.Server + grpcServer *grpc.Server + TLSGRPCCloser io.Closer + TLSHTTPCloser io.Closer } // CollectorParams to construct a new Jaeger Collector. @@ -88,7 +89,7 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error { grpcServer, err := server.StartGRPCServer(&server.GRPCServerParams{ HostPort: builderOpts.CollectorGRPCHostPort, Handler: c.spanHandlers.GRPCHandler, - TLSConfig: builderOpts.TLS, + TLSConfig: builderOpts.TLSGRPC, SamplingStore: c.strategyStore, Logger: c.logger, }) @@ -100,6 +101,7 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error { httpServer, err := server.StartHTTPServer(&server.HTTPServerParams{ HostPort: builderOpts.CollectorHTTPHostPort, Handler: c.spanHandlers.JaegerBatchesHandler, + TLSConfig: builderOpts.TLSHTTP, HealthCheck: c.hCheck, MetricsFactory: c.metricsFactory, SamplingStore: c.strategyStore, @@ -110,7 +112,8 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error { } c.hServer = httpServer - c.tlsCloser = &builderOpts.TLS + c.TLSGRPCCloser = &builderOpts.TLSGRPC + c.TLSHTTPCloser = &builderOpts.TLSHTTP zkServer, err := server.StartZipkinServer(&server.ZipkinServerParams{ HostPort: builderOpts.CollectorZipkinHTTPHostPort, Handler: c.spanHandlers.ZipkinSpansHandler, @@ -165,8 +168,12 @@ func (c *Collector) Close() error { c.logger.Error("failed to close span processor.", zap.Error(err)) } - if err := c.tlsCloser.Close(); err != nil { - c.logger.Error("failed to close TLS certificate watcher", zap.Error(err)) + if err := c.TLSGRPCCloser.Close(); err != nil { + c.logger.Error("failed to close gRPC TLS certificate watcher", zap.Error(err)) + } + + if err := c.TLSHTTPCloser.Close(); err != nil { + c.logger.Error("failed to close HTTP TLS certificate watcher", zap.Error(err)) } return nil diff --git a/cmd/collector/app/server/http.go b/cmd/collector/app/server/http.go index f6523207b6a..de1a54d23a8 100644 --- a/cmd/collector/app/server/http.go +++ b/cmd/collector/app/server/http.go @@ -25,6 +25,7 @@ import ( "github.com/jaegertracing/jaeger/cmd/collector/app/handler" "github.com/jaegertracing/jaeger/cmd/collector/app/sampling/strategystore" clientcfgHandler "github.com/jaegertracing/jaeger/pkg/clientcfg/clientcfghttp" + "github.com/jaegertracing/jaeger/pkg/config/tlscfg" "github.com/jaegertracing/jaeger/pkg/healthcheck" "github.com/jaegertracing/jaeger/pkg/httpmetrics" "github.com/jaegertracing/jaeger/pkg/recoveryhandler" @@ -32,6 +33,7 @@ import ( // HTTPServerParams to construct a new Jaeger Collector HTTP Server type HTTPServerParams struct { + TLSConfig tlscfg.Options HostPort string Handler handler.JaegerBatchesHandler SamplingStore strategystore.StrategyStore @@ -50,6 +52,13 @@ func StartHTTPServer(params *HTTPServerParams) (*http.Server, error) { } server := &http.Server{Addr: params.HostPort} + if params.TLSConfig.Enabled { + tlsCfg, err := params.TLSConfig.Config(params.Logger) // This checks if the certificates are correctly provided + if err != nil { + return nil, err + } + server.TLSConfig = tlsCfg + } serveHTTP(server, listener, params) return server, nil @@ -74,7 +83,13 @@ func serveHTTP(server *http.Server, listener net.Listener, params *HTTPServerPar recoveryHandler := recoveryhandler.NewRecoveryHandler(params.Logger, true) server.Handler = httpmetrics.Wrap(recoveryHandler(r), params.MetricsFactory) go func() { - if err := server.Serve(listener); err != nil { + var err error + if params.TLSConfig.Enabled { + err = server.ServeTLS(listener, "", "") + } else { + err = server.Serve(listener) + } + if err != nil { if err != http.ErrServerClosed { params.Logger.Error("Could not start HTTP collector", zap.Error(err)) } From 62b6d7909a83c0532cfdc4e1cb5db1d99debffd5 Mon Sep 17 00:00:00 2001 From: rjs211 Date: Sun, 7 Feb 2021 15:40:07 +0530 Subject: [PATCH 2/8] correcting config mismatch, failing unit test Signed-off-by: rjs211 --- cmd/collector/app/builder_flags.go | 4 +- .../jaegerreceiver/jaeger_receiver.go | 22 +- .../jaegerreceiver/jaeger_receiver_test.go | 210 +++++++++--------- 3 files changed, 123 insertions(+), 113 deletions(-) diff --git a/cmd/collector/app/builder_flags.go b/cmd/collector/app/builder_flags.go index 353184d14bf..f57ce3254fb 100644 --- a/cmd/collector/app/builder_flags.go +++ b/cmd/collector/app/builder_flags.go @@ -114,8 +114,8 @@ func (cOpts *CollectorOptions) InitFromViper(v *viper.Viper) *CollectorOptions { cOpts.CollectorTags = flags.ParseJaegerTags(v.GetString(collectorTags)) cOpts.CollectorZipkinAllowedOrigins = v.GetString(collectorZipkinAllowedOrigins) cOpts.CollectorZipkinAllowedHeaders = v.GetString(collectorZipkinAllowedHeaders) - cOpts.TLSGRPC = tlsHTTPFlagsConfig.InitFromViper(v) - cOpts.TLSHTTP = tlsGRPCFlagsConfig.InitFromViper(v) + cOpts.TLSGRPC = tlsGRPCFlagsConfig.InitFromViper(v) + cOpts.TLSHTTP = tlsHTTPFlagsConfig.InitFromViper(v) return cOpts } diff --git a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go index 98dc7cf731b..e6801f23787 100644 --- a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go +++ b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go @@ -92,7 +92,7 @@ func configureCollector(v *viper.Viper, cfg *jaegerreceiver.Config) { }, } } - if cOpts.TLS.Enabled { + if cOpts.TLSGRPC.Enabled { if cfg.GRPC == nil { cfg.GRPC = &configgrpc.GRPCServerSettings{ NetAddr: confignet.NetAddr{ @@ -101,10 +101,10 @@ func configureCollector(v *viper.Viper, cfg *jaegerreceiver.Config) { } } cfg.GRPC.TLSSetting = &configtls.TLSServerSetting{ - ClientCAFile: cOpts.TLS.ClientCAPath, + ClientCAFile: cOpts.TLSGRPC.ClientCAPath, TLSSetting: configtls.TLSSetting{ - CertFile: cOpts.TLS.CertPath, - KeyFile: cOpts.TLS.KeyPath, + CertFile: cOpts.TLSGRPC.CertPath, + KeyFile: cOpts.TLSGRPC.KeyPath, }, } } @@ -113,6 +113,20 @@ func configureCollector(v *viper.Viper, cfg *jaegerreceiver.Config) { Endpoint: cOpts.CollectorHTTPHostPort, } } + if cOpts.TLSHTTP.Enabled { + if cfg.ThriftHTTP == nil { + cfg.ThriftHTTP = &confighttp.HTTPServerSettings{ + Endpoint: fmt.Sprintf(":%d", ports.CollectorHTTP), + } + } + cfg.ThriftHTTP.TLSSetting = &configtls.TLSServerSetting{ + ClientCAFile: cOpts.TLSGRPC.ClientCAPath, + TLSSetting: configtls.TLSSetting{ + CertFile: cOpts.TLSHTTP.CertPath, + KeyFile: cOpts.TLSHTTP.KeyPath, + }, + } + } } func createDefaultSamplingConfig(v *viper.Viper) *jaegerreceiver.RemoteSamplingConfig { diff --git a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go index d19ab0e2cc6..77111443ee4 100644 --- a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go +++ b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go @@ -16,7 +16,6 @@ package jaegerreceiver import ( "context" - "fmt" "path" "testing" @@ -26,16 +25,13 @@ import ( "go.opentelemetry.io/collector/component/componenttest" "go.opentelemetry.io/collector/config/configerror" "go.opentelemetry.io/collector/config/configgrpc" - "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/config/configmodels" "go.opentelemetry.io/collector/config/confignet" "go.opentelemetry.io/collector/config/configtest" "go.opentelemetry.io/collector/config/configtls" "go.opentelemetry.io/collector/receiver/jaegerreceiver" - collectorApp "github.com/jaegertracing/jaeger/cmd/collector/app" jConfig "github.com/jaegertracing/jaeger/pkg/config" - "github.com/jaegertracing/jaeger/plugin/sampling/strategystore/static" ) func TestDefaultValues(t *testing.T) { @@ -57,109 +53,109 @@ func TestDefaultValueFromViper(t *testing.T) { flags []string expected *jaegerreceiver.Config }{ - { - name: "samplingStrategyFile", - flags: []string{fmt.Sprintf("--%s=%s", static.SamplingStrategiesFile, "conf.json")}, - expected: &jaegerreceiver.Config{ - RemoteSampling: &jaegerreceiver.RemoteSamplingConfig{ - StrategyFile: "conf.json", - }, - Protocols: jaegerreceiver.Protocols{}, - }, - }, - { - name: "thriftCompact", - flags: []string{fmt.Sprintf("--%s=%s", thriftCompactHostPort, "localhost:9999")}, - expected: &jaegerreceiver.Config{ - Protocols: jaegerreceiver.Protocols{ - ThriftCompact: &jaegerreceiver.ProtocolUDP{ - Endpoint: "localhost:9999", - ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), - }, - }, - }, - }, - { - name: "thriftBinary", - flags: []string{fmt.Sprintf("--%s=%s", thriftBinaryHostPort, "localhost:8888")}, - expected: &jaegerreceiver.Config{ - Protocols: jaegerreceiver.Protocols{ - ThriftBinary: &jaegerreceiver.ProtocolUDP{ - Endpoint: "localhost:8888", - ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), - }, - }, - }, - }, - { - name: "grpc", - flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorGRPCHostPort, "localhost:7894")}, - expected: &jaegerreceiver.Config{ - Protocols: jaegerreceiver.Protocols{ - GRPC: &configgrpc.GRPCServerSettings{ - NetAddr: confignet.NetAddr{ - Endpoint: "localhost:7894", - }, - }, - }, - }, - }, - { - name: "thriftHttp", - flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorHTTPHostPort, "localhost:8080")}, - expected: &jaegerreceiver.Config{ - Protocols: jaegerreceiver.Protocols{ - ThriftHTTP: &confighttp.HTTPServerSettings{ - Endpoint: "localhost:8080", - }, - }, - }, - }, - { - name: "thriftHttpAndThriftBinary", - flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorHTTPHostPort, "localhost:8089"), fmt.Sprintf("--%s=%s", thriftBinaryHostPort, "localhost:2222")}, - expected: &jaegerreceiver.Config{ - Protocols: jaegerreceiver.Protocols{ - ThriftHTTP: &confighttp.HTTPServerSettings{ - Endpoint: "localhost:8089", - }, - ThriftBinary: &jaegerreceiver.ProtocolUDP{ - Endpoint: "localhost:2222", - ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), - }, - }, - }, - }, - { - name: "remoteSampling", - flags: []string{ - "--http-server.host-port=machine:1", - "--sampling.strategies-file=foo", - "--reporter.grpc.host-port=coll:33", - "--reporter.grpc.tls.enabled=true", - "--reporter.grpc.tls.ca=cacert.pem", - "--reporter.grpc.tls.cert=cert.pem", - "--reporter.grpc.tls.key=key.key", - }, - expected: &jaegerreceiver.Config{ - RemoteSampling: &jaegerreceiver.RemoteSamplingConfig{ - StrategyFile: "foo", - HostEndpoint: "machine:1", - GRPCClientSettings: configgrpc.GRPCClientSettings{ - Endpoint: "coll:33", - TLSSetting: configtls.TLSClientSetting{ - Insecure: false, - TLSSetting: configtls.TLSSetting{ - CAFile: "cacert.pem", - CertFile: "cert.pem", - KeyFile: "key.key", - }, - }, - }, - }, - Protocols: jaegerreceiver.Protocols{}, - }, - }, + // { + // name: "samplingStrategyFile", + // flags: []string{fmt.Sprintf("--%s=%s", static.SamplingStrategiesFile, "conf.json")}, + // expected: &jaegerreceiver.Config{ + // RemoteSampling: &jaegerreceiver.RemoteSamplingConfig{ + // StrategyFile: "conf.json", + // }, + // Protocols: jaegerreceiver.Protocols{}, + // }, + // }, + // { + // name: "thriftCompact", + // flags: []string{fmt.Sprintf("--%s=%s", thriftCompactHostPort, "localhost:9999")}, + // expected: &jaegerreceiver.Config{ + // Protocols: jaegerreceiver.Protocols{ + // ThriftCompact: &jaegerreceiver.ProtocolUDP{ + // Endpoint: "localhost:9999", + // ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), + // }, + // }, + // }, + // }, + // { + // name: "thriftBinary", + // flags: []string{fmt.Sprintf("--%s=%s", thriftBinaryHostPort, "localhost:8888")}, + // expected: &jaegerreceiver.Config{ + // Protocols: jaegerreceiver.Protocols{ + // ThriftBinary: &jaegerreceiver.ProtocolUDP{ + // Endpoint: "localhost:8888", + // ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), + // }, + // }, + // }, + // }, + // { + // name: "grpc", + // flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorGRPCHostPort, "localhost:7894")}, + // expected: &jaegerreceiver.Config{ + // Protocols: jaegerreceiver.Protocols{ + // GRPC: &configgrpc.GRPCServerSettings{ + // NetAddr: confignet.NetAddr{ + // Endpoint: "localhost:7894", + // }, + // }, + // }, + // }, + // }, + // { + // name: "thriftHttp", + // flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorHTTPHostPort, "localhost:8080")}, + // expected: &jaegerreceiver.Config{ + // Protocols: jaegerreceiver.Protocols{ + // ThriftHTTP: &confighttp.HTTPServerSettings{ + // Endpoint: "localhost:8080", + // }, + // }, + // }, + // }, + // { + // name: "thriftHttpAndThriftBinary", + // flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorHTTPHostPort, "localhost:8089"), fmt.Sprintf("--%s=%s", thriftBinaryHostPort, "localhost:2222")}, + // expected: &jaegerreceiver.Config{ + // Protocols: jaegerreceiver.Protocols{ + // ThriftHTTP: &confighttp.HTTPServerSettings{ + // Endpoint: "localhost:8089", + // }, + // ThriftBinary: &jaegerreceiver.ProtocolUDP{ + // Endpoint: "localhost:2222", + // ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), + // }, + // }, + // }, + // }, + // { + // name: "remoteSampling", + // flags: []string{ + // "--http-server.host-port=machine:1", + // "--sampling.strategies-file=foo", + // "--reporter.grpc.host-port=coll:33", + // "--reporter.grpc.tls.enabled=true", + // "--reporter.grpc.tls.ca=cacert.pem", + // "--reporter.grpc.tls.cert=cert.pem", + // "--reporter.grpc.tls.key=key.key", + // }, + // expected: &jaegerreceiver.Config{ + // RemoteSampling: &jaegerreceiver.RemoteSamplingConfig{ + // StrategyFile: "foo", + // HostEndpoint: "machine:1", + // GRPCClientSettings: configgrpc.GRPCClientSettings{ + // Endpoint: "coll:33", + // TLSSetting: configtls.TLSClientSetting{ + // Insecure: false, + // TLSSetting: configtls.TLSSetting{ + // CAFile: "cacert.pem", + // CertFile: "cert.pem", + // KeyFile: "key.key", + // }, + // }, + // }, + // }, + // Protocols: jaegerreceiver.Protocols{}, + // }, + // }, { name: "collectorTLS", flags: []string{ From d3c2c8d0bd0269deb999375306aba0cf920e9127 Mon Sep 17 00:00:00 2001 From: rjs211 Date: Sun, 7 Feb 2021 15:45:18 +0530 Subject: [PATCH 3/8] minor refactor Signed-off-by: rjs211 --- cmd/collector/app/collector.go | 18 +- .../jaegerreceiver/jaeger_receiver.go | 2 +- .../jaegerreceiver/jaeger_receiver_test.go | 210 +++++++++--------- 3 files changed, 117 insertions(+), 113 deletions(-) diff --git a/cmd/collector/app/collector.go b/cmd/collector/app/collector.go index eb3e174c502..d90770a7941 100644 --- a/cmd/collector/app/collector.go +++ b/cmd/collector/app/collector.go @@ -45,11 +45,11 @@ type Collector struct { spanHandlers *SpanHandlers // state, read only - hServer *http.Server - zkServer *http.Server - grpcServer *grpc.Server - TLSGRPCCloser io.Closer - TLSHTTPCloser io.Closer + hServer *http.Server + zkServer *http.Server + grpcServer *grpc.Server + tlsGRPCCertWatcherCloser io.Closer + tlsHTTPCertWatcherCloser io.Closer } // CollectorParams to construct a new Jaeger Collector. @@ -112,8 +112,8 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error { } c.hServer = httpServer - c.TLSGRPCCloser = &builderOpts.TLSGRPC - c.TLSHTTPCloser = &builderOpts.TLSHTTP + c.tlsGRPCCertWatcherCloser = &builderOpts.TLSGRPC + c.tlsHTTPCertWatcherCloser = &builderOpts.TLSHTTP zkServer, err := server.StartZipkinServer(&server.ZipkinServerParams{ HostPort: builderOpts.CollectorZipkinHTTPHostPort, Handler: c.spanHandlers.ZipkinSpansHandler, @@ -168,11 +168,11 @@ func (c *Collector) Close() error { c.logger.Error("failed to close span processor.", zap.Error(err)) } - if err := c.TLSGRPCCloser.Close(); err != nil { + if err := c.tlsGRPCCertWatcherCloser.Close(); err != nil { c.logger.Error("failed to close gRPC TLS certificate watcher", zap.Error(err)) } - if err := c.TLSHTTPCloser.Close(); err != nil { + if err := c.tlsHTTPCertWatcherCloser.Close(); err != nil { c.logger.Error("failed to close HTTP TLS certificate watcher", zap.Error(err)) } diff --git a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go index e6801f23787..acee145935b 100644 --- a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go +++ b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go @@ -120,7 +120,7 @@ func configureCollector(v *viper.Viper, cfg *jaegerreceiver.Config) { } } cfg.ThriftHTTP.TLSSetting = &configtls.TLSServerSetting{ - ClientCAFile: cOpts.TLSGRPC.ClientCAPath, + ClientCAFile: cOpts.TLSHTTP.ClientCAPath, TLSSetting: configtls.TLSSetting{ CertFile: cOpts.TLSHTTP.CertPath, KeyFile: cOpts.TLSHTTP.KeyPath, diff --git a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go index 77111443ee4..d19ab0e2cc6 100644 --- a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go +++ b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go @@ -16,6 +16,7 @@ package jaegerreceiver import ( "context" + "fmt" "path" "testing" @@ -25,13 +26,16 @@ import ( "go.opentelemetry.io/collector/component/componenttest" "go.opentelemetry.io/collector/config/configerror" "go.opentelemetry.io/collector/config/configgrpc" + "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/config/configmodels" "go.opentelemetry.io/collector/config/confignet" "go.opentelemetry.io/collector/config/configtest" "go.opentelemetry.io/collector/config/configtls" "go.opentelemetry.io/collector/receiver/jaegerreceiver" + collectorApp "github.com/jaegertracing/jaeger/cmd/collector/app" jConfig "github.com/jaegertracing/jaeger/pkg/config" + "github.com/jaegertracing/jaeger/plugin/sampling/strategystore/static" ) func TestDefaultValues(t *testing.T) { @@ -53,109 +57,109 @@ func TestDefaultValueFromViper(t *testing.T) { flags []string expected *jaegerreceiver.Config }{ - // { - // name: "samplingStrategyFile", - // flags: []string{fmt.Sprintf("--%s=%s", static.SamplingStrategiesFile, "conf.json")}, - // expected: &jaegerreceiver.Config{ - // RemoteSampling: &jaegerreceiver.RemoteSamplingConfig{ - // StrategyFile: "conf.json", - // }, - // Protocols: jaegerreceiver.Protocols{}, - // }, - // }, - // { - // name: "thriftCompact", - // flags: []string{fmt.Sprintf("--%s=%s", thriftCompactHostPort, "localhost:9999")}, - // expected: &jaegerreceiver.Config{ - // Protocols: jaegerreceiver.Protocols{ - // ThriftCompact: &jaegerreceiver.ProtocolUDP{ - // Endpoint: "localhost:9999", - // ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), - // }, - // }, - // }, - // }, - // { - // name: "thriftBinary", - // flags: []string{fmt.Sprintf("--%s=%s", thriftBinaryHostPort, "localhost:8888")}, - // expected: &jaegerreceiver.Config{ - // Protocols: jaegerreceiver.Protocols{ - // ThriftBinary: &jaegerreceiver.ProtocolUDP{ - // Endpoint: "localhost:8888", - // ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), - // }, - // }, - // }, - // }, - // { - // name: "grpc", - // flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorGRPCHostPort, "localhost:7894")}, - // expected: &jaegerreceiver.Config{ - // Protocols: jaegerreceiver.Protocols{ - // GRPC: &configgrpc.GRPCServerSettings{ - // NetAddr: confignet.NetAddr{ - // Endpoint: "localhost:7894", - // }, - // }, - // }, - // }, - // }, - // { - // name: "thriftHttp", - // flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorHTTPHostPort, "localhost:8080")}, - // expected: &jaegerreceiver.Config{ - // Protocols: jaegerreceiver.Protocols{ - // ThriftHTTP: &confighttp.HTTPServerSettings{ - // Endpoint: "localhost:8080", - // }, - // }, - // }, - // }, - // { - // name: "thriftHttpAndThriftBinary", - // flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorHTTPHostPort, "localhost:8089"), fmt.Sprintf("--%s=%s", thriftBinaryHostPort, "localhost:2222")}, - // expected: &jaegerreceiver.Config{ - // Protocols: jaegerreceiver.Protocols{ - // ThriftHTTP: &confighttp.HTTPServerSettings{ - // Endpoint: "localhost:8089", - // }, - // ThriftBinary: &jaegerreceiver.ProtocolUDP{ - // Endpoint: "localhost:2222", - // ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), - // }, - // }, - // }, - // }, - // { - // name: "remoteSampling", - // flags: []string{ - // "--http-server.host-port=machine:1", - // "--sampling.strategies-file=foo", - // "--reporter.grpc.host-port=coll:33", - // "--reporter.grpc.tls.enabled=true", - // "--reporter.grpc.tls.ca=cacert.pem", - // "--reporter.grpc.tls.cert=cert.pem", - // "--reporter.grpc.tls.key=key.key", - // }, - // expected: &jaegerreceiver.Config{ - // RemoteSampling: &jaegerreceiver.RemoteSamplingConfig{ - // StrategyFile: "foo", - // HostEndpoint: "machine:1", - // GRPCClientSettings: configgrpc.GRPCClientSettings{ - // Endpoint: "coll:33", - // TLSSetting: configtls.TLSClientSetting{ - // Insecure: false, - // TLSSetting: configtls.TLSSetting{ - // CAFile: "cacert.pem", - // CertFile: "cert.pem", - // KeyFile: "key.key", - // }, - // }, - // }, - // }, - // Protocols: jaegerreceiver.Protocols{}, - // }, - // }, + { + name: "samplingStrategyFile", + flags: []string{fmt.Sprintf("--%s=%s", static.SamplingStrategiesFile, "conf.json")}, + expected: &jaegerreceiver.Config{ + RemoteSampling: &jaegerreceiver.RemoteSamplingConfig{ + StrategyFile: "conf.json", + }, + Protocols: jaegerreceiver.Protocols{}, + }, + }, + { + name: "thriftCompact", + flags: []string{fmt.Sprintf("--%s=%s", thriftCompactHostPort, "localhost:9999")}, + expected: &jaegerreceiver.Config{ + Protocols: jaegerreceiver.Protocols{ + ThriftCompact: &jaegerreceiver.ProtocolUDP{ + Endpoint: "localhost:9999", + ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), + }, + }, + }, + }, + { + name: "thriftBinary", + flags: []string{fmt.Sprintf("--%s=%s", thriftBinaryHostPort, "localhost:8888")}, + expected: &jaegerreceiver.Config{ + Protocols: jaegerreceiver.Protocols{ + ThriftBinary: &jaegerreceiver.ProtocolUDP{ + Endpoint: "localhost:8888", + ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), + }, + }, + }, + }, + { + name: "grpc", + flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorGRPCHostPort, "localhost:7894")}, + expected: &jaegerreceiver.Config{ + Protocols: jaegerreceiver.Protocols{ + GRPC: &configgrpc.GRPCServerSettings{ + NetAddr: confignet.NetAddr{ + Endpoint: "localhost:7894", + }, + }, + }, + }, + }, + { + name: "thriftHttp", + flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorHTTPHostPort, "localhost:8080")}, + expected: &jaegerreceiver.Config{ + Protocols: jaegerreceiver.Protocols{ + ThriftHTTP: &confighttp.HTTPServerSettings{ + Endpoint: "localhost:8080", + }, + }, + }, + }, + { + name: "thriftHttpAndThriftBinary", + flags: []string{fmt.Sprintf("--%s=%s", collectorApp.CollectorHTTPHostPort, "localhost:8089"), fmt.Sprintf("--%s=%s", thriftBinaryHostPort, "localhost:2222")}, + expected: &jaegerreceiver.Config{ + Protocols: jaegerreceiver.Protocols{ + ThriftHTTP: &confighttp.HTTPServerSettings{ + Endpoint: "localhost:8089", + }, + ThriftBinary: &jaegerreceiver.ProtocolUDP{ + Endpoint: "localhost:2222", + ServerConfigUDP: jaegerreceiver.DefaultServerConfigUDP(), + }, + }, + }, + }, + { + name: "remoteSampling", + flags: []string{ + "--http-server.host-port=machine:1", + "--sampling.strategies-file=foo", + "--reporter.grpc.host-port=coll:33", + "--reporter.grpc.tls.enabled=true", + "--reporter.grpc.tls.ca=cacert.pem", + "--reporter.grpc.tls.cert=cert.pem", + "--reporter.grpc.tls.key=key.key", + }, + expected: &jaegerreceiver.Config{ + RemoteSampling: &jaegerreceiver.RemoteSamplingConfig{ + StrategyFile: "foo", + HostEndpoint: "machine:1", + GRPCClientSettings: configgrpc.GRPCClientSettings{ + Endpoint: "coll:33", + TLSSetting: configtls.TLSClientSetting{ + Insecure: false, + TLSSetting: configtls.TLSSetting{ + CAFile: "cacert.pem", + CertFile: "cert.pem", + KeyFile: "key.key", + }, + }, + }, + }, + Protocols: jaegerreceiver.Protocols{}, + }, + }, { name: "collectorTLS", flags: []string{ From 5ba777b28d2211ba7e1c9b71031c3e8409ee1026 Mon Sep 17 00:00:00 2001 From: rjs211 Date: Sun, 7 Feb 2021 16:16:13 +0530 Subject: [PATCH 4/8] added testcase for collector's HTTP TLS at receiver Signed-off-by: rjs211 --- .../jaegerreceiver/jaeger_receiver_test.go | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go index d19ab0e2cc6..39c0a9f2d38 100644 --- a/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go +++ b/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver_test.go @@ -161,7 +161,7 @@ func TestDefaultValueFromViper(t *testing.T) { }, }, { - name: "collectorTLS", + name: "collectorGRPCTLS", flags: []string{ "--collector.grpc.tls.enabled=true", "--collector.grpc.tls.cert=/cert.pem", @@ -185,6 +185,30 @@ func TestDefaultValueFromViper(t *testing.T) { }, }, }, + { + name: "collectorHTTPTLS", + flags: []string{ + "--collector.http.tls.enabled=true", + "--collector.http.tls.cert=/cert.pem", + "--collector.http.tls.key=/key.pem", + "--collector.http.tls.client-ca=/client-ca.pem", + }, + expected: &jaegerreceiver.Config{ + Protocols: jaegerreceiver.Protocols{ + ThriftHTTP: &confighttp.HTTPServerSettings{ + + Endpoint: ":14268", + TLSSetting: &configtls.TLSServerSetting{ + TLSSetting: configtls.TLSSetting{ + CertFile: "/cert.pem", + KeyFile: "/key.pem", + }, + ClientCAFile: "/client-ca.pem", + }, + }, + }, + }, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { From 0915bca38b70d8aa67734a3a7887a4e092b7f27e Mon Sep 17 00:00:00 2001 From: rjs211 Date: Mon, 8 Feb 2021 21:18:24 +0530 Subject: [PATCH 5/8] rearraging for error logic Signed-off-by: rjs211 --- cmd/collector/app/server/http.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cmd/collector/app/server/http.go b/cmd/collector/app/server/http.go index de1a54d23a8..7860caf0de8 100644 --- a/cmd/collector/app/server/http.go +++ b/cmd/collector/app/server/http.go @@ -46,11 +46,6 @@ type HTTPServerParams struct { func StartHTTPServer(params *HTTPServerParams) (*http.Server, error) { params.Logger.Info("Starting jaeger-collector HTTP server", zap.String("http host-port", params.HostPort)) - listener, err := net.Listen("tcp", params.HostPort) - if err != nil { - return nil, err - } - server := &http.Server{Addr: params.HostPort} if params.TLSConfig.Enabled { tlsCfg, err := params.TLSConfig.Config(params.Logger) // This checks if the certificates are correctly provided @@ -59,6 +54,12 @@ func StartHTTPServer(params *HTTPServerParams) (*http.Server, error) { } server.TLSConfig = tlsCfg } + + listener, err := net.Listen("tcp", params.HostPort) + if err != nil { + return nil, err + } + serveHTTP(server, listener, params) return server, nil From a07970e548104d644238e17e9416b6521d4b2d2e Mon Sep 17 00:00:00 2001 From: rjs211 Date: Mon, 8 Feb 2021 21:18:41 +0530 Subject: [PATCH 6/8] Adding table based test cases Signed-off-by: rjs211 --- cmd/collector/app/server/http_test.go | 199 ++++++++++++++++++++++++++ 1 file changed, 199 insertions(+) diff --git a/cmd/collector/app/server/http_test.go b/cmd/collector/app/server/http_test.go index 9f0643dbfec..20cbd507333 100644 --- a/cmd/collector/app/server/http_test.go +++ b/cmd/collector/app/server/http_test.go @@ -15,19 +15,27 @@ package server import ( + "crypto/tls" + "fmt" + "net" "net/http" "net/http/httptest" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/uber/jaeger-lib/metrics/metricstest" "go.uber.org/zap" "github.com/jaegertracing/jaeger/cmd/collector/app/handler" + "github.com/jaegertracing/jaeger/pkg/config/tlscfg" "github.com/jaegertracing/jaeger/pkg/healthcheck" + "github.com/jaegertracing/jaeger/ports" ) +var testCertKeyLocation = "../../../../pkg/config/tlscfg/testdata" + // test wrong port number func TestFailToListenHTTP(t *testing.T) { logger, _ := zap.NewDevelopment() @@ -39,6 +47,25 @@ func TestFailToListenHTTP(t *testing.T) { assert.EqualError(t, err, "listen tcp: address -1: invalid port") } +func TestCreateTLSHTTPServerError(t *testing.T) { + logger, _ := zap.NewDevelopment() + tlsCfg := tlscfg.Options{ + Enabled: true, + CertPath: "invalid/path", + KeyPath: "invalid/path", + ClientCAPath: "invalid/path", + } + + params := &HTTPServerParams{ + HostPort: fmt.Sprintf(":%d", ports.CollectorHTTP), + HealthCheck: healthcheck.New(), + Logger: logger, + TLSConfig: tlsCfg, + } + _, err := StartHTTPServer(params) + assert.NotNil(t, err) +} + func TestSpanCollectorHTTP(t *testing.T) { logger, _ := zap.NewDevelopment() params := &HTTPServerParams{ @@ -58,3 +85,175 @@ func TestSpanCollectorHTTP(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, response) } + +func TestSpanCollectorHTTPS(t *testing.T) { + + testCases := []struct { + name string + TLS tlscfg.Options + clientTLS tlscfg.Options + expectError bool + expectClientError bool + expectServerFail bool + }{ + { + name: "should fail with TLS client to untrusted TLS server", + TLS: tlscfg.Options{ + Enabled: true, + CertPath: testCertKeyLocation + "/example-server-cert.pem", + KeyPath: testCertKeyLocation + "/example-server-key.pem", + }, + clientTLS: tlscfg.Options{ + Enabled: true, + ServerName: "example.com", + }, + expectError: true, + expectClientError: true, + expectServerFail: false, + }, + { + name: "should fail with TLS client to trusted TLS server with incorrect hostname", + TLS: tlscfg.Options{ + Enabled: true, + CertPath: testCertKeyLocation + "/example-server-cert.pem", + KeyPath: testCertKeyLocation + "/example-server-key.pem", + }, + clientTLS: tlscfg.Options{ + Enabled: true, + CAPath: testCertKeyLocation + "/example-CA-cert.pem", + ServerName: "nonEmpty", + }, + expectError: true, + expectClientError: true, + expectServerFail: false, + }, + { + name: "should pass with TLS client to trusted TLS server with correct hostname", + TLS: tlscfg.Options{ + Enabled: true, + CertPath: testCertKeyLocation + "/example-server-cert.pem", + KeyPath: testCertKeyLocation + "/example-server-key.pem", + }, + clientTLS: tlscfg.Options{ + Enabled: true, + CAPath: testCertKeyLocation + "/example-CA-cert.pem", + ServerName: "example.com", + }, + expectError: false, + expectClientError: false, + expectServerFail: false, + }, + { + name: "should fail with TLS client without cert to trusted TLS server requiring cert", + TLS: tlscfg.Options{ + Enabled: true, + CertPath: testCertKeyLocation + "/example-server-cert.pem", + KeyPath: testCertKeyLocation + "/example-server-key.pem", + ClientCAPath: testCertKeyLocation + "/example-CA-cert.pem", + }, + clientTLS: tlscfg.Options{ + Enabled: true, + CAPath: testCertKeyLocation + "/example-CA-cert.pem", + ServerName: "example.com", + }, + expectError: false, + expectServerFail: false, + expectClientError: true, + }, + { + name: "should pass with TLS client with cert to trusted TLS server requiring cert", + TLS: tlscfg.Options{ + Enabled: true, + CertPath: testCertKeyLocation + "/example-server-cert.pem", + KeyPath: testCertKeyLocation + "/example-server-key.pem", + ClientCAPath: testCertKeyLocation + "/example-CA-cert.pem", + }, + clientTLS: tlscfg.Options{ + Enabled: true, + CAPath: testCertKeyLocation + "/example-CA-cert.pem", + ServerName: "example.com", + CertPath: testCertKeyLocation + "/example-client-cert.pem", + KeyPath: testCertKeyLocation + "/example-client-key.pem", + }, + expectError: false, + expectServerFail: false, + expectClientError: false, + }, + { + name: "should fail with TLS client without cert to trusted TLS server requiring cert from a different CA", + TLS: tlscfg.Options{ + Enabled: true, + CertPath: testCertKeyLocation + "/example-server-cert.pem", + KeyPath: testCertKeyLocation + "/example-server-key.pem", + ClientCAPath: testCertKeyLocation + "/wrong-CA-cert.pem", // NB: wrong CA + }, + clientTLS: tlscfg.Options{ + Enabled: true, + CAPath: testCertKeyLocation + "/example-CA-cert.pem", + ServerName: "example.com", + CertPath: testCertKeyLocation + "/example-client-cert.pem", + KeyPath: testCertKeyLocation + "/example-client-key.pem", + }, + expectError: false, + expectServerFail: false, + expectClientError: true, + }, + } + + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + logger, _ := zap.NewDevelopment() + params := &HTTPServerParams{ + HostPort: fmt.Sprintf(":%d", ports.CollectorHTTP), + Handler: handler.NewJaegerSpanHandler(logger, &mockSpanProcessor{}), + SamplingStore: &mockSamplingStore{}, + MetricsFactory: metricstest.NewFactory(time.Hour), + HealthCheck: healthcheck.New(), + Logger: logger, + TLSConfig: test.TLS, + } + + server, err := StartHTTPServer(params) + if test.expectServerFail { + require.Error(t, err) + } + defer server.Close() + + require.NoError(t, err) + clientTLSCfg, err0 := test.clientTLS.Config(zap.NewNop()) + require.NoError(t, err0) + dialer := &net.Dialer{Timeout: 2 * time.Second} + conn, clientError := tls.DialWithDialer(dialer, "tcp", "localhost:"+fmt.Sprintf("%d", ports.CollectorHTTP), clientTLSCfg) + var clientClose func() error + clientClose = nil + if conn != nil { + clientClose = conn.Close + } + + if test.expectError { + require.Error(t, clientError) + } else { + require.NoError(t, clientError) + } + + if clientClose != nil { + require.Nil(t, clientClose()) + } + + client := &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: clientTLSCfg, + }, + } + + response, requestError := client.Post("https://localhost:"+fmt.Sprintf("%d", ports.CollectorHTTP), "", nil) + + if test.expectClientError { + require.Error(t, requestError) + } else { + require.NoError(t, requestError) + require.NotNil(t, response) + } + }) + } +} From 473c0636300687cb01ac151031543008433a451f Mon Sep 17 00:00:00 2001 From: rjs211 Date: Mon, 8 Feb 2021 21:50:18 +0530 Subject: [PATCH 7/8] dummy commit for git resync Signed-off-by: rjs211 --- cmd/collector/app/server/http_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/collector/app/server/http_test.go b/cmd/collector/app/server/http_test.go index 20cbd507333..c8b4aad33f9 100644 --- a/cmd/collector/app/server/http_test.go +++ b/cmd/collector/app/server/http_test.go @@ -214,6 +214,7 @@ func TestSpanCollectorHTTPS(t *testing.T) { } server, err := StartHTTPServer(params) + if test.expectServerFail { require.Error(t, err) } From cd29744550972b9c5f370f986047406e0073453c Mon Sep 17 00:00:00 2001 From: rjs211 Date: Tue, 9 Feb 2021 04:03:37 +0530 Subject: [PATCH 8/8] removing redundant error capture for TLS certificate watcher close method Signed-off-by: rjs211 --- cmd/collector/app/collector.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/cmd/collector/app/collector.go b/cmd/collector/app/collector.go index d90770a7941..f46f86a2f5d 100644 --- a/cmd/collector/app/collector.go +++ b/cmd/collector/app/collector.go @@ -168,13 +168,9 @@ func (c *Collector) Close() error { c.logger.Error("failed to close span processor.", zap.Error(err)) } - if err := c.tlsGRPCCertWatcherCloser.Close(); err != nil { - c.logger.Error("failed to close gRPC TLS certificate watcher", zap.Error(err)) - } - - if err := c.tlsHTTPCertWatcherCloser.Close(); err != nil { - c.logger.Error("failed to close HTTP TLS certificate watcher", zap.Error(err)) - } + // watchers actually never return errors from Close + _ = c.tlsGRPCCertWatcherCloser.Close() + _ = c.tlsHTTPCertWatcherCloser.Close() return nil }