Add support for repeated arguments (#1434)

* Add support repetitive arguments to operand Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> * Add MapString method to get only strings from Options type Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> * Add options tests for stringMap and Values Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> * Fix upgrde tests Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> * Fix import order, improve comments for new type Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jaegertracing · Aug 5, 2021 · 5ec0aa1 · 5ec0aa1
1 parent 05b0c69
commit 5ec0aa1
Show file tree

Hide file tree

Showing 14 changed files with 129 additions and 43 deletions.
diff --git a/pkg/apis/jaegertracing/v1/options.go b/pkg/apis/jaegertracing/v1/options.go
@@ -7,10 +7,28 @@ import (
 	"strings"
 )
 
+// Values hold a map, with string as the key and either a string or a slice of strings as the value
+type Values map[string]interface{}
+
+// DeepCopy indicate how to do a deep copy of Values type
+func (v *Values) DeepCopy() *Values {
+	out := make(Values, len(*v))
+	for key, val := range *v {
+		switch val.(type) {
+		case string:
+			out[key] = val
+
+		case []string:
+			out[key] = append([]string(nil), val.([]string)...)
+		}
+	}
+	return &out
+}
+
 // Options defines a common options parameter to the different structs
 type Options struct {
-	opts map[string]string `json:"-"`
-	json *[]byte           `json:"-"`
+	opts Values  `json:"-"`
+	json *[]byte `json:"-"`
 }
 
 // NewOptions build a new Options object based on the given map
@@ -23,7 +41,7 @@ func NewOptions(o map[string]interface{}) Options {
 // Filter creates a new Options object with just the elements identified by the supplied prefix
 func (o *Options) Filter(prefix string) Options {
 	options := Options{}
-	options.opts = make(map[string]string)
+	options.opts = make(map[string]interface{})
 
 	archivePrefix := prefix + "-archive."
 	prefix += "."
@@ -45,7 +63,6 @@ func (o *Options) UnmarshalJSON(b []byte) error {
 	if err := d.Decode(&entries); err != nil {
 		return err
 	}
-
 	o.parse(entries)
 	o.json = &b
 	return nil
@@ -69,18 +86,24 @@ func (o Options) MarshalJSON() ([]byte, error) {
 }
 
 func (o *Options) parse(entries map[string]interface{}) {
-	o.opts = make(map[string]string)
+	o.opts = make(map[string]interface{})
 	for k, v := range entries {
 		o.opts = entry(o.opts, k, v)
 	}
 }
 
-func entry(entries map[string]string, key string, value interface{}) map[string]string {
+func entry(entries map[string]interface{}, key string, value interface{}) map[string]interface{} {
 	switch value.(type) {
 	case map[string]interface{}:
 		for k, v := range value.(map[string]interface{}) {
 			entries = entry(entries, fmt.Sprintf("%s.%v", key, k), v)
 		}
+	case []interface{}:
+		values := make([]string, 0, len(value.([]interface{})))
+		for _, v := range value.([]interface{}) {
+			values = append(values, v.(string))
+		}
+		entries[key] = values
 	case interface{}:
 		entries[key] = fmt.Sprintf("%v", value)
 	}
@@ -91,24 +114,41 @@ func entry(entries map[string]string, key string, value interface{}) map[string]
 // ToArgs converts the options to a value suitable for the Container.Args field
 func (o *Options) ToArgs() []string {
 	if len(o.opts) > 0 {
-		i := 0
-		args := make([]string, len(o.opts))
+		args := make([]string, 0, len(o.opts))
 		for k, v := range o.opts {
-			args[i] = fmt.Sprintf("--%s=%v", k, v)
-			i++
+			switch v.(type) {
+			case string:
+				args = append(args, fmt.Sprintf("--%s=%v", k, v))
+			case []string:
+				for _, vv := range v.([]string) {
+					args = append(args, fmt.Sprintf("--%s=%v", k, vv))
+				}
+			}
 		}
 		return args
 	}
-
 	return nil
 }
 
 // Map returns a map representing the option entries. Items are flattened, with dots as separators. For instance
 // an option "cassandra" with a nested "servers" object becomes an entry with the key "cassandra.servers"
-func (o *Options) Map() map[string]string {
+func (o *Options) Map() map[string]interface{} {
 	return o.opts
 }
 
+// StringMap returns a map representing the option entries,excluding entries that have multiple values.
+// Items are flattened, with dots as separators in the same way as Map does.
+func (o *Options) StringMap() map[string]string {
+	smap := make(map[string]string)
+	for k, v := range o.opts {
+		switch v.(type) {
+		case string:
+			smap[k] = v.(string)
+		}
+	}
+	return smap
+}
+
 // GenericMap returns the map representing the option entries as interface{}, suitable for usage with NewOptions()
 func (o *Options) GenericMap() map[string]interface{} {
 	out := make(map[string]interface{})

diff --git a/pkg/apis/jaegertracing/v1/options_test.go b/pkg/apis/jaegertracing/v1/options_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSimpleOption(t *testing.T) {
@@ -148,3 +149,38 @@ func TestUpdate(t *testing.T) {
 	// verify
 	assert.Equal(t, o.opts["key"], "new")
 }
+
+func TestStringMap(t *testing.T) {
+	o := NewOptions(nil)
+	err := o.UnmarshalJSON([]byte(`{"firstsarg":"v1", "additional-headers":["whatever:thing", "access-control-allow-origin:blerg"]}`))
+	require.NoError(t, err)
+	expected := map[string]string{"firstsarg": "v1"}
+	strMap := o.StringMap()
+	assert.Len(t, strMap, 1)
+	assert.Equal(t, expected, strMap)
+}
+
+func TestDeepCopy(t *testing.T) {
+	o1 := NewOptions(nil)
+	err := o1.UnmarshalJSON([]byte(`{"firstsarg":"v1", "additional-headers":["whatever:thing", "access-control-allow-origin:blerg"]}`))
+	require.NoError(t, err)
+	copy := o1.opts.DeepCopy()
+
+	assert.Equal(t, copy, &(o1.opts))
+}
+
+func TestRepetitiveArguments(t *testing.T) {
+	o := NewOptions(nil)
+	err := o.UnmarshalJSON([]byte(`{"firstsarg":"v1", "additional-headers":["whatever:thing", "access-control-allow-origin:blerg"]}`))
+	require.NoError(t, err)
+	expected := []string{"--additional-headers=access-control-allow-origin:blerg", "--additional-headers=whatever:thing", "--firstsarg=v1"}
+
+	args := o.ToArgs()
+	sort.SliceStable(args, func(i, j int) bool {
+		return args[i] < args[j]
+	})
+
+	assert.Len(t, args, 3)
+	assert.Equal(t, expected, args)
+
+}
diff --git a/pkg/apis/jaegertracing/v1/zz_generated.deepcopy.go b/pkg/apis/jaegertracing/v1/zz_generated.deepcopy.go
diff --git a/pkg/cronjob/es_index_cleaner.go b/pkg/cronjob/es_index_cleaner.go
@@ -28,7 +28,7 @@ func CreateEsIndexCleaner(jaeger *v1.Jaeger) *batchv1beta1.CronJob {
 
 	envFromSource := util.CreateEnvsFromSecret(jaeger.Spec.Storage.SecretName)
 	envs := EsScriptEnvVars(jaeger.Spec.Storage.Options)
-	if val, ok := jaeger.Spec.Storage.Options.Map()["es.use-aliases"]; ok && strings.EqualFold(val, "true") {
+	if val, ok := jaeger.Spec.Storage.Options.StringMap()["es.use-aliases"]; ok && strings.EqualFold(val, "true") {
 		envs = append(envs, corev1.EnvVar{Name: "ROLLOVER", Value: "true"})
 	}
 

diff --git a/pkg/cronjob/es_rollover.go b/pkg/cronjob/es_rollover.go
@@ -155,7 +155,7 @@ func EsScriptEnvVars(opts v1.Options) []corev1.EnvVar {
 		{flag: "es.tls.key", envVar: "ES_TLS_KEY"},
 		{flag: "es.tls.skip-host-verify", envVar: "ES_TLS_SKIP_HOST_VERIFY"},
 	}
-	options := opts.Map()
+	options := opts.StringMap()
 	var envs []corev1.EnvVar
 	for _, x := range scriptEnvVars {
 		if val, ok := options[x.flag]; ok {

diff --git a/pkg/cronjob/spark_dependencies.go b/pkg/cronjob/spark_dependencies.go
@@ -119,7 +119,7 @@ func CreateSparkDependencies(jaeger *v1.Jaeger) *batchv1beta1.CronJob {
 }
 
 func getStorageEnvs(s v1.JaegerStorageSpec) []corev1.EnvVar {
-	sFlagsMap := s.Options.Map()
+	sFlagsMap := s.Options.StringMap()
 	switch s.Type {
 	case v1.JaegerCassandraStorage:
 		keyspace := sFlagsMap["cassandra.keyspace"]
@@ -156,7 +156,7 @@ func getStorageEnvs(s v1.JaegerStorageSpec) []corev1.EnvVar {
 }
 
 func logTLSNotSupported(j *v1.Jaeger) {
-	sFlagsMap := j.Spec.Storage.Options.Map()
+	sFlagsMap := j.Spec.Storage.Options.StringMap()
 	if strings.EqualFold(sFlagsMap["es.tls.enabled"], "true") || strings.EqualFold(sFlagsMap["es.tls"], "true") {
 		j.Logger().Warn("Spark dependencies does not support TLS with Elasticsearch, consider disabling dependencies")
 	}

diff --git a/pkg/ingress/query.go b/pkg/ingress/query.go
@@ -77,9 +77,9 @@ func (i *QueryIngress) Get() *networkingv1.Ingress {
 func (i *QueryIngress) addRulesSpec(spec *networkingv1.IngressSpec, backend *networkingv1.IngressBackend) {
 	path := ""
 
-	if allInOneQueryBasePath, ok := i.jaeger.Spec.AllInOne.Options.Map()["query.base-path"]; ok && i.jaeger.Spec.Strategy == v1.DeploymentStrategyAllInOne {
+	if allInOneQueryBasePath, ok := i.jaeger.Spec.AllInOne.Options.StringMap()["query.base-path"]; ok && i.jaeger.Spec.Strategy == v1.DeploymentStrategyAllInOne {
 		path = allInOneQueryBasePath
-	} else if queryBasePath, ok := i.jaeger.Spec.Query.Options.Map()["query.base-path"]; ok && i.jaeger.Spec.Strategy == v1.DeploymentStrategyProduction {
+	} else if queryBasePath, ok := i.jaeger.Spec.Query.Options.StringMap()["query.base-path"]; ok && i.jaeger.Spec.Strategy == v1.DeploymentStrategyProduction {
 		path = queryBasePath
 	}
 

diff --git a/pkg/service/collector.go b/pkg/service/collector.go
@@ -107,7 +107,7 @@ func GetPortNameForGRPC(jaeger *v1.Jaeger) string {
 
 	// perhaps the user has provisioned the certs and configured the CR manually?
 	// for that, we check whether the CLI option `collector.grpc.tls.enabled` was set for the collector
-	if val, ok := jaeger.Spec.Collector.Options.Map()["collector.grpc.tls.enabled"]; ok {
+	if val, ok := jaeger.Spec.Collector.Options.StringMap()["collector.grpc.tls.enabled"]; ok {
 		enabled, err := strconv.ParseBool(val)
 		if err != nil {
 			return "grpc-http" // not "true", defaults to false

diff --git a/pkg/storage/cassandra_dependencies.go b/pkg/storage/cassandra_dependencies.go
@@ -44,7 +44,7 @@ func cassandraDeps(jaeger *v1.Jaeger) []batchv1.Job {
 		Value: jaeger.Spec.Storage.CassandraCreateSchema.Datacenter,
 	}}
 
-	host := jaeger.Spec.Storage.Options.Map()["cassandra.servers"]
+	host := jaeger.Spec.Storage.Options.StringMap()["cassandra.servers"]
 	if host == "" {
 		jaeger.Logger().Info("Cassandra hostname not specified. Using 'cassandra' for the cassandra-create-schema job.")
 		host = "cassandra" // this is the default in the image
@@ -54,7 +54,7 @@ func cassandraDeps(jaeger *v1.Jaeger) []batchv1.Job {
 		Value: host,
 	})
 
-	port := jaeger.Spec.Storage.Options.Map()["cassandra.port"]
+	port := jaeger.Spec.Storage.Options.StringMap()["cassandra.port"]
 	if port == "" {
 		jaeger.Logger().Info("Cassandra port not specified. Using '9042' for the cassandra-create-schema job.")
 		port = "9042" // this is the default in the image
@@ -64,7 +64,7 @@ func cassandraDeps(jaeger *v1.Jaeger) []batchv1.Job {
 		Value: port,
 	})
 
-	keyspace := jaeger.Spec.Storage.Options.Map()["cassandra.keyspace"]
+	keyspace := jaeger.Spec.Storage.Options.StringMap()["cassandra.keyspace"]
 	if keyspace == "" {
 		jaeger.Logger().Info("Cassandra keyspace not specified. Using 'jaeger_v1_test' for the cassandra-create-schema job.")
 		keyspace = "jaeger_v1_test" // this is default in the image
@@ -74,16 +74,18 @@ func cassandraDeps(jaeger *v1.Jaeger) []batchv1.Job {
 		Name:  "KEYSPACE",
 		Value: keyspace,
 	})
+	username := jaeger.Spec.Storage.Options.StringMap()["cassandra.username"]
+	password := jaeger.Spec.Storage.Options.StringMap()["cassandra.password"]
 
 	envFromSource := util.CreateEnvsFromSecret(jaeger.Spec.Storage.SecretName)
 	if len(envFromSource) == 0 {
 		envVars = append(envVars, corev1.EnvVar{
 			Name:  "CASSANDRA_USERNAME",
-			Value: jaeger.Spec.Storage.Options.Map()["cassandra.username"],
+			Value: username,
 		})
 		envVars = append(envVars, corev1.EnvVar{
 			Name:  "CASSANDRA_PASSWORD",
-			Value: jaeger.Spec.Storage.Options.Map()["cassandra.password"],
+			Value: password,
 		})
 	}
 

diff --git a/pkg/storage/elasticsearch_dependencies.go b/pkg/storage/elasticsearch_dependencies.go
@@ -15,7 +15,7 @@ import (
 
 // EnableRollover returns true if rollover should be enabled
 func EnableRollover(spec v1.JaegerStorageSpec) bool {
-	useAliases := spec.Options.Map()["es.use-aliases"]
+	useAliases := spec.Options.StringMap()["es.use-aliases"]
 	return (spec.Type == v1.JaegerESStorage) && strings.EqualFold(useAliases, "true")
 }
 
@@ -78,7 +78,7 @@ func envVars(opts v1.Options) []corev1.EnvVar {
 		{flag: "es.num-shards", envVar: "SHARDS"},
 		{flag: "es.num-replicas", envVar: "REPLICAS"},
 	}
-	options := opts.Map()
+	options := opts.StringMap()
 	for _, x := range scriptEnvVars {
 		if val, ok := options[x.flag]; ok {
 			envs = append(envs, corev1.EnvVar{Name: x.envVar, Value: val})

diff --git a/pkg/strategy/controller.go b/pkg/strategy/controller.go
@@ -129,7 +129,7 @@ func distributedStorage(storage v1.JaegerStorageType) bool {
 }
 
 func normalizeSparkDependencies(spec *v1.JaegerStorageSpec) {
-	sFlagsMap := spec.Options.Map()
+	sFlagsMap := spec.Options.StringMap()
 	tlsEnabled := sFlagsMap["es.tls"]
 	tlsSkipHost := sFlagsMap["es.tls.skip-host-verify"]
 	tlsCa := sFlagsMap["es.tls.ca"]
@@ -201,7 +201,7 @@ func normalizeUI(spec *v1.JaegerSpec) {
 			uiOpts = m
 		}
 	}
-	enableArchiveButton(uiOpts, spec.Storage.Options.Map())
+	enableArchiveButton(uiOpts, spec.Storage.Options.StringMap())
 	disableDependenciesTab(uiOpts, spec.Storage.Type, spec.Storage.Dependencies.Enabled)
 	enableDocumentationLink(uiOpts, spec)
 	enableLogOut(uiOpts, spec)

diff --git a/pkg/upgrade/v1_22_0_test.go b/pkg/upgrade/v1_22_0_test.go
@@ -252,7 +252,7 @@ func TestMigrateQueryHostPortFlagsv1_22_0(t *testing.T) {
 		persisted := &v1.Jaeger{}
 		assert.NoError(t, cl.Get(context.Background(), nsn, persisted))
 		assert.Equal(t, latestVersion, persisted.Status.Version)
-		assert.Equal(t, tt.expectedOps, persisted.Spec.Query.Options.Map())
+		assert.Equal(t, tt.expectedOps, persisted.Spec.Query.Options.StringMap())
 
 	}
 

diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -177,12 +177,16 @@ func Labels(name, component string, jaeger v1.Jaeger) map[string]string {
 }
 
 // GetEsHostname return first ES hostname from options map
-func GetEsHostname(opts map[string]string) string {
+func GetEsHostname(opts map[string]interface{}) string {
 	urls, ok := opts["es.server-urls"]
 	if !ok {
 		return ""
 	}
-	urlArr := strings.Split(urls, ",")
+	urlsString, isString := urls.(string)
+	if !isString {
+		return ""
+	}
+	urlArr := strings.Split(urlsString, ",")
 	return urlArr[0]
 }
 

diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go
@@ -288,14 +288,14 @@ func TestMergeTolerations(t *testing.T) {
 
 func TestGetEsHostname(t *testing.T) {
 	tests := []struct {
-		underTest map[string]string
+		underTest map[string]interface{}
 		hostname  string
 	}{
 		{hostname: ""},
-		{underTest: map[string]string{"": ""}, hostname: ""},
-		{underTest: map[string]string{"es.server-urls": ""}, hostname: ""},
-		{underTest: map[string]string{"es.server-urls": "goo:tar"}, hostname: "goo:tar"},
-		{underTest: map[string]string{"es.server-urls": "http://es:9000,https://es2:9200"}, hostname: "http://es:9000"},
+		{underTest: map[string]interface{}{"": ""}, hostname: ""},
+		{underTest: map[string]interface{}{"es.server-urls": ""}, hostname: ""},
+		{underTest: map[string]interface{}{"es.server-urls": "goo:tar"}, hostname: "goo:tar"},
+		{underTest: map[string]interface{}{"es.server-urls": "http://es:9000,https://es2:9200"}, hostname: "http://es:9000"},
 	}
 	for _, test := range tests {
 		assert.Equal(t, test.hostname, GetEsHostname(test.underTest))