Perform log.fatal if TLS flags are used when tls.enabled=false #2893

cmd/agent/app/builder_test.go

@@ -245,7 +245,8 @@ func TestCreateCollectorProxy(t *testing.T) {
 		require.NoError(t, err)
 
 		rOpts := new(reporter.Options).InitFromViper(v, zap.NewNop())
-		grpcBuilder := grpc.NewConnBuilder().InitFromViper(v)
+		grpcBuilder, err := grpc.NewConnBuilder().InitFromViper(v)
+		require.NoError(t, err)
 
 		metricsFactory := metricstest.NewFactory(time.Microsecond)
 

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

@@ -44,13 +44,17 @@ func AddFlags(flags *flag.FlagSet) {
 }
 
 // InitFromViper initializes Options with properties retrieved from Viper.
-func (b *ConnBuilder) InitFromViper(v *viper.Viper) *ConnBuilder {
+func (b *ConnBuilder) InitFromViper(v *viper.Viper) (*ConnBuilder, error) {
 	hostPorts := v.GetString(collectorHostPort)
 	if hostPorts != "" {
 		b.CollectorHostPorts = strings.Split(hostPorts, ",")
 	}
 	b.MaxRetry = uint(v.GetInt(retry))
-	b.TLS = tlsFlagsConfig.InitFromViper(v)
+	if tls, err := tlsFlagsConfig.InitFromViper(v); err == nil {
+		b.TLS = tls
+	} else {
+		return b, err
+	}
 	b.DiscoveryMinPeers = v.GetInt(discoveryMinPeers)
-	return b
+	return b, nil
 }

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

@@ -46,7 +46,26 @@ func TestBindFlags(t *testing.T) {
 
 		err := command.ParseFlags(test.cOpts)
 		require.NoError(t, err)
-		b := new(ConnBuilder).InitFromViper(v)
+		b, err := new(ConnBuilder).InitFromViper(v)
+		require.NoError(t, err)
 		assert.Equal(t, test.expected, b)
 	}
 }
+
+func TestBindTLSFlagFailure(t *testing.T) {
+	v := viper.New()
+	command := cobra.Command{}
+	flags := &flag.FlagSet{}
+	AddFlags(flags)
+	command.ParseFlags([]string{
+		"--reporter.grpc-server.host-port=127.0.0.1:1234",
+	})
+	v.Set("reporter.grpc.tls.enabled", "false")
+	v.Set("reporter.grpc.tls.cert", "abc")
+	v.Set("reporter.grpc.tls.ca", "def")
+	v.Set("reporter.grpc.tls.key", "xyz")
+	v.Set("reporter.grpc.tls.server-name", "xyz")
+	v.Set("reporter.grpc.tls.skip-host-verify", "true")
+	_, err := new(ConnBuilder).InitFromViper(v)
+	require.Error(t, err, "collector.grpc.tls.enabled has been disable")
+}

cmd/agent/main.go

@@ -61,7 +61,10 @@ func main() {
 			version.NewInfoMetrics(mFactory)
 
 			rOpts := new(reporter.Options).InitFromViper(v, logger)
-			grpcBuilder := grpc.NewConnBuilder().InitFromViper(v)
+			grpcBuilder, err := grpc.NewConnBuilder().InitFromViper(v)
+			if err != nil {
+				logger.Fatal("Failed to configure connection for grpc", zap.Error(err))
+			}
 			builders := map[reporter.Type]app.CollectorProxyBuilder{
 				reporter.GRPC: app.GRPCCollectorProxyBuilder(grpcBuilder),
 			}

cmd/all-in-one/main.go

@@ -132,9 +132,18 @@ by default uses only in-memory database.`,
 
 			aOpts := new(agentApp.Builder).InitFromViper(v)
 			repOpts := new(agentRep.Options).InitFromViper(v, logger)
-			grpcBuilder := agentGrpcRep.NewConnBuilder().InitFromViper(v)
-			cOpts := new(collectorApp.CollectorOptions).InitFromViper(v)
-			qOpts := new(queryApp.QueryOptions).InitFromViper(v, logger)
+			grpcBuilder, err := agentGrpcRep.NewConnBuilder().InitFromViper(v)
+			if err != nil {
+				logger.Fatal("Failed to configure connection for grpc", zap.Error(err))
+			}
+			cOpts, err := new(collectorApp.CollectorOptions).InitFromViper(v)
+			if err != nil {
+				logger.Fatal("Failed to initialize collector", zap.Error(err))
+			}
+			qOpts, err := new(queryApp.QueryOptions).InitFromViper(v, logger)
+			if err != nil {
+				logger.Fatal("Failed to configure query service", zap.Error(err))
+			}
 
 			// collector
 			c := collectorApp.New(&collectorApp.CollectorParams{

cmd/collector/app/builder_flags.go

@@ -88,7 +88,7 @@ func AddFlags(flags *flag.FlagSet) {
 }
 
 // InitFromViper initializes CollectorOptions with properties from viper
-func (cOpts *CollectorOptions) InitFromViper(v *viper.Viper) *CollectorOptions {
+func (cOpts *CollectorOptions) InitFromViper(v *viper.Viper) (*CollectorOptions, error) {
 	cOpts.CollectorGRPCHostPort = ports.FormatHostPort(v.GetString(collectorGRPCHostPort))
 	cOpts.CollectorHTTPHostPort = ports.FormatHostPort(v.GetString(collectorHTTPHostPort))
 	cOpts.CollectorTags = flags.ParseJaegerTags(v.GetString(collectorTags))
@@ -98,8 +98,15 @@ func (cOpts *CollectorOptions) InitFromViper(v *viper.Viper) *CollectorOptions {
 	cOpts.DynQueueSizeMemory = v.GetUint(collectorDynQueueSizeMemory) * 1024 * 1024 // we receive in MiB and store in bytes
 	cOpts.NumWorkers = v.GetInt(collectorNumWorkers)
 	cOpts.QueueSize = v.GetInt(collectorQueueSize)
-	cOpts.TLSGRPC = tlsGRPCFlagsConfig.InitFromViper(v)
-	cOpts.TLSHTTP = tlsHTTPFlagsConfig.InitFromViper(v)
-
-	return cOpts
+	if tlsGrpc, err := tlsGRPCFlagsConfig.InitFromViper(v); err == nil {
+		cOpts.TLSGRPC = tlsGrpc
+	} else {
+		return cOpts, err
+	}
+	if tlsHTTP, err := tlsHTTPFlagsConfig.InitFromViper(v); err == nil {
+		cOpts.TLSHTTP = tlsHTTP
+	} else {
+		return cOpts, err
+	}
+	return cOpts, nil
 }

cmd/collector/app/builder_flags_test.go

@@ -18,6 +18,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/jaegertracing/jaeger/pkg/config"
 )
@@ -51,3 +52,33 @@ func TestCollectorOptionsWithFlags_CheckFullHostPort(t *testing.T) {
 	assert.Equal(t, "127.0.0.1:1234", c.CollectorGRPCHostPort)
 	assert.Equal(t, "0.0.0.0:3456", c.CollectorZipkinHTTPHostPort)
 }
+
+func TestCollectorOptionsWithFailedHTTPFlags(t *testing.T) {
+	c := &CollectorOptions{}
+	v, command := config.Viperize(AddFlags)
+	command.ParseFlags([]string{
+		"--collector.http-server.host-port=:5678",
+		"--collector.grpc-server.host-port=127.0.0.1:1234",
+	})
+	v.Set("collector.http.tls.enabled", "false")
+	v.Set("collector.http.tls.cert", "abc")
+	v.Set("collector.http.tls.ca", "def")
+	v.Set("collector.http.tls.key", "xyz")
+	_, err := c.InitFromViper(v)
+	require.Error(t, err, "query.http.tls.enabled has been disable")
+}
+
+func TestCollectorOptionsWithFailedGRPCFlags(t *testing.T) {
+	c := &CollectorOptions{}
+	v, command := config.Viperize(AddFlags)
+	command.ParseFlags([]string{
+		"--collector.http-server.host-port=:5678",
+		"--collector.grpc-server.host-port=127.0.0.1:1234",
+	})
+	v.Set("collector.grpc.tls.enabled", "false")
+	v.Set("collector.grpc.tls.cert", "abc")
+	v.Set("collector.grpc.tls.ca", "def")
+	v.Set("collector.grpc.tls.key", "xyz")
+	_, err := c.InitFromViper(v)
+	require.Error(t, err, "query.grpc.tls.enabled has been disable")
+}

cmd/collector/app/span_handler_builder_test.go

@@ -32,7 +32,8 @@ func TestNewSpanHandlerBuilder(t *testing.T) {
 	v, command := config.Viperize(flags.AddFlags, AddFlags)
 
 	require.NoError(t, command.ParseFlags([]string{}))
-	cOpts := new(CollectorOptions).InitFromViper(v)
+	cOpts, err := new(CollectorOptions).InitFromViper(v)
+	require.NoError(t, err)
 
 	spanWriter := memory.NewStore()
 

cmd/collector/main.go

@@ -97,7 +97,10 @@ func main() {
 				StrategyStore:  strategyStore,
 				HealthCheck:    svc.HC(),
 			})
-			collectorOpts := new(app.CollectorOptions).InitFromViper(v)
+			collectorOpts, err := new(app.CollectorOptions).InitFromViper(v)
+			if err != nil {
+				logger.Fatal("Failed to initialize collector", zap.Error(err))
+			}
 			if err := c.Start(collectorOpts); err != nil {
 				logger.Fatal("Failed to start collector", zap.Error(err))
 			}

cmd/query/app/flags.go

@@ -96,11 +96,19 @@ func AddFlags(flagSet *flag.FlagSet) {
 }
 
 // InitFromViper initializes QueryOptions with properties from viper
-func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *QueryOptions {
+func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) (*QueryOptions, error) {
 	qOpts.HTTPHostPort = v.GetString(queryHTTPHostPort)
 	qOpts.GRPCHostPort = v.GetString(queryGRPCHostPort)
-	qOpts.TLSGRPC = tlsGRPCFlagsConfig.InitFromViper(v)
-	qOpts.TLSHTTP = tlsHTTPFlagsConfig.InitFromViper(v)
+	if tlsGrpc, err := tlsGRPCFlagsConfig.InitFromViper(v); err == nil {
+		qOpts.TLSGRPC = tlsGrpc
+	} else {
+		return qOpts, err
+	}
+	if tlsHTTP, err := tlsHTTPFlagsConfig.InitFromViper(v); err == nil {
+		qOpts.TLSHTTP = tlsHTTP
+	} else {
+		return qOpts, err
+	}
 	qOpts.BasePath = v.GetString(queryBasePath)
 	qOpts.StaticAssets = v.GetString(queryStaticFiles)
 	qOpts.UIConfig = v.GetString(queryUIConfig)
@@ -114,7 +122,7 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *Qu
 	} else {
 		qOpts.AdditionalHeaders = headers
 	}
-	return qOpts
+	return qOpts, nil
 }
 
 // BuildQueryServiceOptions creates a QueryServiceOptions struct with appropriate adjusters and archive config

cmd/query/app/flags_test.go

@@ -21,6 +21,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
 
 	"github.com/jaegertracing/jaeger/pkg/config"
@@ -41,7 +42,8 @@ func TestQueryBuilderFlags(t *testing.T) {
 		"--query.additional-headers=whatever:thing",
 		"--query.max-clock-skew-adjustment=10s",
 	})
-	qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop())
+	qOpts, err := new(QueryOptions).InitFromViper(v, zap.NewNop())
+	require.NoError(t, err)
 	assert.Equal(t, "/dev/null", qOpts.StaticAssets)
 	assert.Equal(t, "some.json", qOpts.UIConfig)
 	assert.Equal(t, "/jaeger", qOpts.BasePath)
@@ -59,7 +61,8 @@ func TestQueryBuilderBadHeadersFlags(t *testing.T) {
 	command.ParseFlags([]string{
 		"--query.additional-headers=malformedheader",
 	})
-	qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop())
+	qOpts, err := new(QueryOptions).InitFromViper(v, zap.NewNop())
+	require.NoError(t, err)
 	assert.Nil(t, qOpts.AdditionalHeaders)
 }
 
@@ -92,7 +95,8 @@ func TestStringSliceAsHeader(t *testing.T) {
 
 func TestBuildQueryServiceOptions(t *testing.T) {
 	v, _ := config.Viperize(AddFlags)
-	qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop())
+	qOpts, err := new(QueryOptions).InitFromViper(v, zap.NewNop())
+	require.NoError(t, err)
 	assert.NotNil(t, qOpts)
 
 	qSvcOpts := qOpts.BuildQueryServiceOptions(&mocks.Factory{}, zap.NewNop())
@@ -162,11 +166,84 @@ func TestQueryOptionsPortAllocationFromFlags(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			v, command := config.Viperize(AddFlags)
 			command.ParseFlags(test.flagsArray)
-			qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop())
+			qOpts, err := new(QueryOptions).InitFromViper(v, zap.NewNop())
+			require.NoError(t, err)
 
 			assert.Equal(t, test.expectedHTTPHostPort, qOpts.HTTPHostPort)
 			assert.Equal(t, test.expectedGRPCHostPort, qOpts.GRPCHostPort)
 
 		})
 	}
 }
+
+func TestQueryOptionsPortAllocationFromFailedGRPCFlags(t *testing.T) {
+	var flagPortCases = []struct {
+		name                 string
+		flagsArray           []string
+		expectedHTTPHostPort string
+		expectedGRPCHostPort string
+		verifyCommonPort     bool
+		expectedHostPort     string
+	}{
+		{
+			// Allows usage of common host-ports.  Flags allow this irrespective of TLS status
+			// The server with TLS enabled with equal HTTP & GRPC host-ports, is still an acceptable flag configuration
+			name: "Common equal host-port specified, TLS enabled in atleast one server",
+			flagsArray: []string{
+				"--query.grpc.tls.enabled=false",
+				"--query.http-server.host-port=127.0.0.1:8081",
+				"--query.grpc-server.host-port=127.0.0.1:8081",
+			},
+			expectedHTTPHostPort: "127.0.0.1:8081",
+			expectedGRPCHostPort: "127.0.0.1:8081",
+		},
+	}
+	for _, test := range flagPortCases {
+		t.Run(test.name, func(t *testing.T) {
+			v, command := config.Viperize(AddFlags)
+			command.ParseFlags(test.flagsArray)
+			v.Set("query.grpc.tls.enabled", "false")
+			v.Set("query.grpc.tls.cert", "abc")
+			v.Set("query.grpc.tls.ca", "def")
+			v.Set("query.grpc.tls.key", "xyz")
+			_, err := new(QueryOptions).InitFromViper(v, zap.NewNop())
+			require.Error(t, err, "query.grpc.tls.enabled has been disable")
+		})
+	}
+}
+
+func TestQueryOptionsPortAllocationFromFailedHTTPFlags(t *testing.T) {
+	var flagPortCases = []struct {
+		name                 string
+		flagsArray           []string
+		expectedHTTPHostPort string
+		expectedGRPCHostPort string
+		verifyCommonPort     bool
+		expectedHostPort     string
+	}{
+		{
+			// Allows usage of common host-ports.  Flags allow this irrespective of TLS status
+			// The server with TLS enabled with equal HTTP & GRPC host-ports, is still an acceptable flag configuration
+			name: "Common equal host-port specified, TLS enabled in atleast one server",
+			flagsArray: []string{
+				"--query.http.tls.enabled=false",
+				"--query.http-server.host-port=127.0.0.1:8081",
+				"--query.grpc-server.host-port=127.0.0.1:8081",
+			},
+			expectedHTTPHostPort: "127.0.0.1:8081",
+			expectedGRPCHostPort: "127.0.0.1:8081",
+		},
+	}
+	for _, test := range flagPortCases {
+		t.Run(test.name, func(t *testing.T) {
+			v, command := config.Viperize(AddFlags)
+			command.ParseFlags(test.flagsArray)
+			v.Set("query.http.tls.enabled", "false")
+			v.Set("query.http.tls.cert", "abc")
+			v.Set("query.http.tls.ca", "def")
+			v.Set("query.http.tls.key", "xyz")
+			_, err := new(QueryOptions).InitFromViper(v, zap.NewNop())
+			require.Error(t, err, "query.http.tls.enabled has been disable")
+		})
+	}
+}

cmd/query/main.go

@@ -93,7 +93,10 @@ func main() {
 			}
 			defer closer.Close()
 			opentracing.SetGlobalTracer(tracer)
-			queryOpts := new(app.QueryOptions).InitFromViper(v, logger)
+			queryOpts, err := new(app.QueryOptions).InitFromViper(v, logger)
+			if err != nil {
+				logger.Fatal("Failed to configure query service", zap.Error(err))
+			}
 			// TODO: Need to figure out set enable/disable propagation on storage plugins.
 			v.Set(spanstore.StoragePropagationKey, queryOpts.BearerTokenPropagation)
 			storageFactory.InitFromViper(v, logger)

pkg/config/tlscfg/flags.go

@@ -16,6 +16,7 @@ package tlscfg
 
 import (
 	"flag"
+	"fmt"
 
 	"github.com/spf13/viper"
 )
@@ -60,23 +61,31 @@ func (c ServerFlagsConfig) AddFlags(flags *flag.FlagSet) {
 }
 
 // InitFromViper creates tls.Config populated with values retrieved from Viper.
-func (c ClientFlagsConfig) InitFromViper(v *viper.Viper) Options {
+func (c ClientFlagsConfig) InitFromViper(v *viper.Viper) (Options, error) {
 	var p Options
 	p.Enabled = v.GetBool(c.Prefix + tlsEnabled)
 	p.CAPath = v.GetString(c.Prefix + tlsCA)
 	p.CertPath = v.GetString(c.Prefix + tlsCert)
 	p.KeyPath = v.GetString(c.Prefix + tlsKey)
 	p.ServerName = v.GetString(c.Prefix + tlsServerName)
 	p.SkipHostVerify = v.GetBool(c.Prefix + tlsSkipHostVerify)
-	return p
+	areTLSFlagsUsed := v.IsSet(c.Prefix+tlsCA) || v.IsSet(c.Prefix+tlsCert) || v.IsSet(c.Prefix+tlsKey) || v.IsSet(c.Prefix+tlsServerName)
+	if (!p.Enabled) && (p.SkipHostVerify && areTLSFlagsUsed) {
+		return p, fmt.Errorf("%s has been disable", c.Prefix+tlsEnabled)
+	}
+	return p, nil
 }
 
 // InitFromViper creates tls.Config populated with values retrieved from Viper.
-func (c ServerFlagsConfig) InitFromViper(v *viper.Viper) Options {
+func (c ServerFlagsConfig) InitFromViper(v *viper.Viper) (Options, error) {
 	var p Options
 	p.Enabled = v.GetBool(c.Prefix + tlsEnabled)
 	p.CertPath = v.GetString(c.Prefix + tlsCert)
 	p.KeyPath = v.GetString(c.Prefix + tlsKey)
 	p.ClientCAPath = v.GetString(c.Prefix + tlsClientCA)
-	return p
+	areTLSFlagsUsed := v.IsSet(c.Prefix+tlsCA) || v.IsSet(c.Prefix+tlsCert) || v.IsSet(c.Prefix+tlsKey)
+	if !p.Enabled && areTLSFlagsUsed {
+		return p, fmt.Errorf("%s has been disable", c.Prefix+tlsEnabled)
+	}
+	return p, nil
 }