Add TLS flags test

This commit adds tls flag test in collector/app/builder_flag_test.go and query/app/flag_test.go Also fixes InitFromViper check in builder_flag.go (#2893) Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>
jaegertracing · Jul 16, 2021 · f8f1cdf · f8f1cdf
1 parent f92ac6a
commit f8f1cdf
Show file tree

Hide file tree

Showing 8 changed files with 130 additions and 24 deletions.
diff --git a/cmd/agent/app/reporter/grpc/flags_test.go b/cmd/agent/app/reporter/grpc/flags_test.go
@@ -51,3 +51,21 @@ func TestBindFlags(t *testing.T) {
 		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")
+}
diff --git a/cmd/collector/app/builder_flags.go b/cmd/collector/app/builder_flags.go
@@ -98,12 +98,12 @@ 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)
-	if tlsGrpc, err := tlsGRPCFlagsConfig.InitFromViper(v); err != nil {
+	if tlsGrpc, err := tlsGRPCFlagsConfig.InitFromViper(v); err == nil {
 		cOpts.TLSGRPC = tlsGrpc
 	} else {
 		return cOpts, err
 	}
-	if tlsHTTP, err := tlsGRPCFlagsConfig.InitFromViper(v); err != nil {
+	if tlsHTTP, err := tlsHTTPFlagsConfig.InitFromViper(v); err == nil {
 		cOpts.TLSHTTP = tlsHTTP
 	} else {
 		return cOpts, err

diff --git a/cmd/collector/app/builder_flags_test.go b/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")
+}
diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go
@@ -104,7 +104,7 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) (*Q
 	} else {
 		return qOpts, err
 	}
-	if tlsHTTP, err := tlsGRPCFlagsConfig.InitFromViper(v); err == nil {
+	if tlsHTTP, err := tlsHTTPFlagsConfig.InitFromViper(v); err == nil {
 		qOpts.TLSHTTP = tlsHTTP
 	} else {
 		return qOpts, err

diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go
@@ -175,3 +175,75 @@ func TestQueryOptionsPortAllocationFromFlags(t *testing.T) {
 		})
 	}
 }
+
+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")
+		})
+	}
+}
diff --git a/plugin/metrics/prometheus/options.go b/plugin/metrics/prometheus/options.go
@@ -68,16 +68,11 @@ func (opt *Options) AddFlags(flagSet *flag.FlagSet) {
 }
 
 // InitFromViper initializes the options struct with values from Viper.
-func (opt *Options) InitFromViper(v *viper.Viper) error {
+func (opt *Options) InitFromViper(v *viper.Viper) {
 	cfg := &opt.Primary
 	cfg.ServerURL = stripWhiteSpace(v.GetString(cfg.namespace + suffixServerURL))
 	cfg.ConnectTimeout = v.GetDuration(cfg.namespace + suffixConnectTimeout)
-	if tls, err := cfg.getTLSFlagsConfig().InitFromViper(v); err == nil {
-		cfg.TLS = tls
-	} else {
-		return err
-	}
-	return nil
+	cfg.TLS, _ = cfg.getTLSFlagsConfig().InitFromViper(v)
 }
 
 func (config *namespaceConfig) getTLSFlagsConfig() tlscfg.ClientFlagsConfig {

diff --git a/plugin/storage/cassandra/options.go b/plugin/storage/cassandra/options.go
@@ -234,7 +234,7 @@ func tlsFlagsConfig(namespace string) tlscfg.ClientFlagsConfig {
 	}
 }
 
-func (cfg *namespaceConfig) initFromViper(v *viper.Viper) error {
+func (cfg *namespaceConfig) initFromViper(v *viper.Viper) {
 	var tlsFlagsConfig = tlsFlagsConfig(cfg.namespace)
 	if cfg.namespace != primaryStorageConfig {
 		cfg.Enabled = v.GetBool(cfg.namespace + suffixEnabled)
@@ -254,12 +254,7 @@ func (cfg *namespaceConfig) initFromViper(v *viper.Viper) error {
 	cfg.Authenticator.Basic.Username = v.GetString(cfg.namespace + suffixUsername)
 	cfg.Authenticator.Basic.Password = v.GetString(cfg.namespace + suffixPassword)
 	cfg.DisableCompression = v.GetBool(cfg.namespace + suffixDisableCompression)
-	if tls, err := tlsFlagsConfig.InitFromViper(v); err == nil {
-		cfg.TLS = tls
-	} else {
-		return err
-	}
-	return nil
+	cfg.TLS, _ = tlsFlagsConfig.InitFromViper(v)
 }
 
 // GetPrimary returns primary configuration.

diff --git a/plugin/storage/es/options.go b/plugin/storage/es/options.go
@@ -290,7 +290,7 @@ func (opt *Options) InitFromViper(v *viper.Viper) {
 	}
 }
 
-func initFromViper(cfg *namespaceConfig, v *viper.Viper) error {
+func initFromViper(cfg *namespaceConfig, v *viper.Viper) {
 	cfg.Username = v.GetString(cfg.namespace + suffixUsername)
 	cfg.Password = v.GetString(cfg.namespace + suffixPassword)
 	cfg.TokenFilePath = v.GetString(cfg.namespace + suffixTokenPath)
@@ -337,12 +337,7 @@ func initFromViper(cfg *namespaceConfig, v *viper.Viper) error {
 	// Dependencies calculation should be daily, and this index size is very small
 	cfg.IndexDateLayoutDependencies = initDateLayout(defaultIndexRolloverFrequency, separator)
 
-	if tls, err := cfg.getTLSFlagsConfig().InitFromViper(v); err == nil {
-		cfg.TLS = tls
-	} else {
-		return err
-	}
-	return nil
+	cfg.TLS, _ = cfg.getTLSFlagsConfig().InitFromViper(v)
 }
 
 // GetPrimary returns primary configuration.