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

@@ -16,6 +16,7 @@ package grpc
 
 import (
 	"flag"
+	"fmt"
 	"strings"
 
 	"github.com/spf13/viper"
@@ -44,13 +45,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, fmt.Errorf("failed to process TLS options: %w", err)
+	}
 	b.DiscoveryMinPeers = v.GetInt(discoveryMinPeers)
-	return b
+	return b, nil
 }

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

@@ -22,6 +22,8 @@ import (
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/jaegertracing/jaeger/pkg/config"
 )
 
 func TestBindFlags(t *testing.T) {
@@ -46,7 +48,20 @@ 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, command := config.Viperize(AddFlags)
+	err := command.ParseFlags([]string{
+		"--reporter.grpc.tls.enabled=false",
+		"--reporter.grpc.tls.cert=blah", // invalid unless tls.enabled
+	})
+	require.NoError(t, err)
+	_, err = new(ConnBuilder).InitFromViper(v)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to process TLS options")
+}

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 gRPC connection", zap.Error(err))
+			}
 			builders := map[reporter.Type]app.CollectorProxyBuilder{
 				reporter.GRPC: app.GRPCCollectorProxyBuilder(grpcBuilder),
 			}
@@ -71,15 +74,15 @@ func main() {
 				Metrics: mFactory,
 			}, builders)
 			if err != nil {
-				logger.Fatal("Could not create collector proxy", zap.Error(err))
+				logger.Fatal("Failed to create collector proxy", zap.Error(err))
 			}
 
 			// TODO illustrate discovery service wiring
 
 			builder := new(app.Builder).InitFromViper(v)
 			agent, err := builder.CreateAgent(cp, logger, mFactory)
 			if err != nil {
-				return fmt.Errorf("unable to initialize Jaeger Agent: %w", err)
+				return fmt.Errorf("failed to initialize Jaeger Agent: %w", err)
 			}
 
 			logger.Info("Starting agent")

cmd/all-in-one/main.go

@@ -141,9 +141,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

@@ -17,6 +17,7 @@ package app
 
 import (
 	"flag"
+	"fmt"
 	"time"
 
 	"github.com/spf13/viper"
@@ -103,7 +104,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))
@@ -113,11 +114,19 @@ 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)
 	cOpts.CollectorGRPCMaxReceiveMessageLength = v.GetInt(collectorGRPCMaxReceiveMessageLength)
 	cOpts.CollectorGRPCMaxConnectionAge = v.GetDuration(collectorMaxConnectionAge)
 	cOpts.CollectorGRPCMaxConnectionAgeGrace = v.GetDuration(collectorMaxConnectionAgeGrace)
+	if tlsGrpc, err := tlsGRPCFlagsConfig.InitFromViper(v); err == nil {
+		cOpts.TLSGRPC = tlsGrpc
+	} else {
+		return cOpts, fmt.Errorf("failed to parse gRPC TLS options: %w", err)
+	}
+	if tlsHTTP, err := tlsHTTPFlagsConfig.InitFromViper(v); err == nil {
+		cOpts.TLSHTTP = tlsHTTP
+	} else {
+		return cOpts, fmt.Errorf("failed to parse HTTP TLS options: %w", err)
+	}
 
-	return cOpts
+	return cOpts, nil
 }

cmd/collector/app/builder_flags_test.go

@@ -19,6 +19,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/jaegertracing/jaeger/pkg/config"
 )
@@ -53,6 +54,32 @@ func TestCollectorOptionsWithFlags_CheckFullHostPort(t *testing.T) {
 	assert.Equal(t, "0.0.0.0:3456", c.CollectorZipkinHTTPHostPort)
 }
 
+func TestCollectorOptionsWithFailedHTTPFlags(t *testing.T) {
+	c := &CollectorOptions{}
+	v, command := config.Viperize(AddFlags)
+	err := command.ParseFlags([]string{
+		"--collector.http.tls.enabled=false",
+		"--collector.http.tls.cert=blah", // invalid unless tls.enabled
+	})
+	require.NoError(t, err)
+	_, err = c.InitFromViper(v)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to parse HTTP TLS options")
+}
+
+func TestCollectorOptionsWithFailedGRPCFlags(t *testing.T) {
+	c := &CollectorOptions{}
+	v, command := config.Viperize(AddFlags)
+	err := command.ParseFlags([]string{
+		"--collector.grpc.tls.enabled=false",
+		"--collector.grpc.tls.cert=blah", // invalid unless tls.enabled
+	})
+	require.NoError(t, err)
+	_, err = c.InitFromViper(v)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to parse gRPC TLS options")
+}
+
 func TestCollectorOptionsWithFlags_CheckMaxReceiveMessageLength(t *testing.T) {
 	c := &CollectorOptions{}
 	v, command := config.Viperize(AddFlags)

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

@@ -106,7 +106,10 @@ func main() {
 				Aggregator:     aggregator,
 				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/es-index-cleaner/main.go

@@ -52,7 +52,10 @@ func main() {
 			}
 
 			cfg.InitFromViper(v)
-			tlsOpts := tlsFlags.InitFromViper(v)
+			tlsOpts, err := tlsFlags.InitFromViper(v)
+			if err != nil {
+				return err
+			}
 			tlsCfg, err := tlsOpts.Config(logger)
 			if err != nil {
 				return err

cmd/es-rollover/app/actions.go

@@ -61,7 +61,10 @@ type ActionCreatorFunction func(client.Client, Config) Action
 func ExecuteAction(opts ActionExecuteOptions, createAction ActionCreatorFunction) error {
 	cfg := Config{}
 	cfg.InitFromViper(opts.Viper)
-	tlsOpts := opts.TLSFlags.InitFromViper(opts.Viper)
+	tlsOpts, err := opts.TLSFlags.InitFromViper(opts.Viper)
+	if err != nil {
+		return err
+	}
 	tlsCfg, err := tlsOpts.Config(opts.Logger)
 	if err != nil {
 		return err

cmd/es-rollover/app/actions_test.go

@@ -92,7 +92,8 @@ func TestExecuteAction(t *testing.T) {
 			tlsFlags.AddFlags(flags)
 			command.PersistentFlags().AddGoFlagSet(flags)
 			v.BindPFlags(command.PersistentFlags())
-			err := command.ParseFlags(test.flags)
+			cmdLine := append([]string{"--es.tls.enabled=true"}, test.flags...)
+			err := command.ParseFlags(cmdLine)
 			require.NoError(t, err)
 			executedAction := false
 			err = ExecuteAction(ActionExecuteOptions{

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, fmt.Errorf("failed to process gRPC TLS options: %w", err)
+	}
+	if tlsHTTP, err := tlsHTTPFlagsConfig.InitFromViper(v); err == nil {
+		qOpts.TLSHTTP = tlsHTTP
+	} else {
+		return qOpts, fmt.Errorf("failed to process HTTP TLS options: %w", 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

@@ -17,10 +17,12 @@ package app
 
 import (
 	"net/http"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
 
 	"github.com/jaegertracing/jaeger/pkg/config"
@@ -41,7 +43,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 +62,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 +96,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 +167,29 @@ 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 TestQueryOptions_FailedTLSFlags(t *testing.T) {
+	for _, test := range []string{"gRPC", "HTTP"} {
+		t.Run(test, func(t *testing.T) {
+			proto := strings.ToLower(test)
+			v, command := config.Viperize(AddFlags)
+			err := command.ParseFlags([]string{
+				"--query." + proto + ".tls.enabled=false",
+				"--query." + proto + ".tls.cert=blah", // invalid unless tls.enabled
+			})
+			require.NoError(t, err)
+			_, err = new(QueryOptions).InitFromViper(v, zap.NewNop())
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "failed to process "+test+" TLS options")
+		})
+	}
+}

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(bearertoken.StoragePropagationKey, queryOpts.BearerTokenPropagation)
 			storageFactory.InitFromViper(v, logger)