From 4976d7a3a4926406f16f951961f5059b9d7354d0 Mon Sep 17 00:00:00 2001 From: StefanHauth <63204425+StefanHauth@users.noreply.github.com> Date: Wed, 10 Apr 2024 10:41:48 +0200 Subject: [PATCH] Bugfix/allow duplicate oneagent arguments (#2968) --- .../dynakube/oneagent/daemonset/arguments.go | 6 ++- .../oneagent/daemonset/arguments_test.go | 34 +++++++++++- .../oneagent/daemonset/daemonset_test.go | 7 ++- pkg/util/prioritymap/cmdline_parser.go | 2 +- pkg/util/prioritymap/conversion.go | 52 +++++++++++-------- pkg/util/prioritymap/map.go | 37 ++++++++----- 6 files changed, 99 insertions(+), 39 deletions(-) diff --git a/pkg/controllers/dynakube/oneagent/daemonset/arguments.go b/pkg/controllers/dynakube/oneagent/daemonset/arguments.go index c5c82e84b7..83c05bb5fc 100644 --- a/pkg/controllers/dynakube/oneagent/daemonset/arguments.go +++ b/pkg/controllers/dynakube/oneagent/daemonset/arguments.go @@ -13,7 +13,11 @@ const customArgumentPriority = 2 const defaultArgumentPriority = 1 func (dsInfo *builderInfo) arguments() ([]string, error) { - argMap := prioritymap.New(prioritymap.WithSeparator(prioritymap.DefaultSeparator), prioritymap.WithPriority(defaultArgumentPriority)) + argMap := prioritymap.New( + prioritymap.WithSeparator(prioritymap.DefaultSeparator), + prioritymap.WithPriority(defaultArgumentPriority), + prioritymap.WithAllowDuplicates(), + ) isProxyAsEnvDeprecated, err := isProxyAsEnvVarDeprecated(dsInfo.dynakube.OneAgentVersion()) if err != nil { diff --git a/pkg/controllers/dynakube/oneagent/daemonset/arguments_test.go b/pkg/controllers/dynakube/oneagent/daemonset/arguments_test.go index 26ce69f0c7..b2ab1939f9 100644 --- a/pkg/controllers/dynakube/oneagent/daemonset/arguments_test.go +++ b/pkg/controllers/dynakube/oneagent/daemonset/arguments_test.go @@ -79,7 +79,7 @@ func TestArguments(t *testing.T) { } assert.Equal(t, expectedDefaultArguments, arguments) }) - t.Run("when injected arguments are provided then they take precedence", func(t *testing.T) { + t.Run("when injected arguments are provided then they come last", func(t *testing.T) { args := []string{ "--set-app-log-content-access=true", "--set-host-id-source=lustiglustig", @@ -98,6 +98,7 @@ func TestArguments(t *testing.T) { "--set-host-group=APP_LUSTIG_PETER", "--set-host-id-source=lustiglustig", "--set-host-property=OperatorVersion=$(DT_OPERATOR_VERSION)", + "--set-server={$(DT_SERVER)}", "--set-server=https://hyper.super.com:9999", "--set-tenant=$(DT_TENANT)", } @@ -124,6 +125,37 @@ func TestArguments(t *testing.T) { } assert.Equal(t, expectedDefaultArguments, arguments) }) + t.Run("multiple set-host-property entries are possible", func(t *testing.T) { + args := []string{ + "--set-app-log-content-access=true", + "--set-host-id-source=lustiglustig", + "--set-host-group=APP_LUSTIG_PETER", + "--set-server=https://hyper.super.com:9999", + "--set-host-property=item0=value0", + "--set-host-property=item1=value1", + "--set-host-property=item2=value2", + } + builder := builderInfo{ + dynakube: &dynatracev1beta1.DynaKube{}, + hostInjectSpec: &dynatracev1beta1.HostInjectSpec{Args: args}, + } + + arguments, _ := builder.arguments() + + expectedDefaultArguments := []string{ + "--set-app-log-content-access=true", + "--set-host-group=APP_LUSTIG_PETER", + "--set-host-id-source=lustiglustig", + "--set-host-property=OperatorVersion=$(DT_OPERATOR_VERSION)", + "--set-host-property=item0=value0", + "--set-host-property=item1=value1", + "--set-host-property=item2=value2", + "--set-server={$(DT_SERVER)}", + "--set-server=https://hyper.super.com:9999", + "--set-tenant=$(DT_TENANT)", + } + assert.Equal(t, expectedDefaultArguments, arguments) + }) } func TestPodSpec_Arguments(t *testing.T) { diff --git a/pkg/controllers/dynakube/oneagent/daemonset/daemonset_test.go b/pkg/controllers/dynakube/oneagent/daemonset/daemonset_test.go index 250c43c6fd..d2cab9c531 100644 --- a/pkg/controllers/dynakube/oneagent/daemonset/daemonset_test.go +++ b/pkg/controllers/dynakube/oneagent/daemonset/daemonset_test.go @@ -836,8 +836,8 @@ func TestOneAgentHostGroup(t *testing.T) { daemonset, err := builder.BuildDaemonSet() require.NoError(t, err) - assert.NotNil(t, daemonset) - assert.Equal(t, "--set-host-group=newgroup", daemonset.Spec.Template.Spec.Containers[0].Args[0]) + require.NotNil(t, daemonset) + assert.Equal(t, "--set-host-group=newgroup", daemonset.Spec.Template.Spec.Containers[0].Args[1]) }) } @@ -881,8 +881,10 @@ func TestDefaultArguments(t *testing.T) { expectedDefaultArguments := []string{ "--set-app-log-content-access=true", "--set-host-group=APP_LUSTIG_PETER", + "--set-host-id-source=auto", "--set-host-id-source=fqdn", "--set-host-property=OperatorVersion=$(DT_OPERATOR_VERSION)", + "--set-server={$(DT_SERVER)}", "--set-server=https://hyper.super.com:9999", "--set-tenant=$(DT_TENANT)", } @@ -906,6 +908,7 @@ func TestDefaultArguments(t *testing.T) { "--set-host-group=APP_LUSTIG_PETER", "--set-host-id-source=auto", "--set-host-property=OperatorVersion=$(DT_OPERATOR_VERSION)", + "--set-server={$(DT_SERVER)}", "--set-server=https://hyper.super.com:9999", "--set-tenant=$(DT_TENANT)", } diff --git a/pkg/util/prioritymap/cmdline_parser.go b/pkg/util/prioritymap/cmdline_parser.go index 76934f24ab..adafea8ee5 100644 --- a/pkg/util/prioritymap/cmdline_parser.go +++ b/pkg/util/prioritymap/cmdline_parser.go @@ -7,7 +7,7 @@ import ( const DefaultSeparator = "=" // ParseCommandLineArgument splits strings in the format of "--param=value" up in its components "--param", "=" and "value". -// The separator is returned because to let the caller know if it was there +// The separator is returned to let the caller know if it was there func ParseCommandLineArgument(arg string) (string, string, string) { arg, value, foundSeparator := strings.Cut(arg, DefaultSeparator) diff --git a/pkg/util/prioritymap/conversion.go b/pkg/util/prioritymap/conversion.go index 3624e81383..f682b62bc7 100644 --- a/pkg/util/prioritymap/conversion.go +++ b/pkg/util/prioritymap/conversion.go @@ -2,6 +2,7 @@ package prioritymap import ( "fmt" + "sort" "golang.org/x/exp/slices" corev1 "k8s.io/api/core/v1" @@ -12,26 +13,28 @@ func (m Map) AsEnvVars() []corev1.EnvVar { envVars := make([]corev1.EnvVar, 0, len(keys)) for _, key := range keys { - switch typedValue := m.entries[key].value.(type) { - case string: - envVars = append(envVars, corev1.EnvVar{ - Name: key, - Value: typedValue, - }) - case corev1.EnvVar: - envVars = append(envVars, typedValue) - case *corev1.EnvVar: - envVars = append(envVars, *typedValue) - case *corev1.EnvVarSource: - envVars = append(envVars, corev1.EnvVar{ - Name: key, - ValueFrom: typedValue, - }) - case corev1.EnvVarSource: - envVars = append(envVars, corev1.EnvVar{ - Name: key, - ValueFrom: &typedValue, - }) + for _, entry := range m.entries[key] { + switch typedValue := entry.value.(type) { + case string: + envVars = append(envVars, corev1.EnvVar{ + Name: key, + Value: typedValue, + }) + case corev1.EnvVar: + envVars = append(envVars, typedValue) + case *corev1.EnvVar: + envVars = append(envVars, *typedValue) + case *corev1.EnvVarSource: + envVars = append(envVars, corev1.EnvVar{ + Name: key, + ValueFrom: typedValue, + }) + case corev1.EnvVarSource: + envVars = append(envVars, corev1.EnvVar{ + Name: key, + ValueFrom: &typedValue, + }) + } } } @@ -43,8 +46,13 @@ func (m Map) AsKeyValueStrings() []string { valStrings := make([]string, 0) for _, key := range keys { - val := m.entries[key] - valStrings = append(valStrings, fmt.Sprintf("%s%s%v", key, val.delimiter, val.value)) + sort.SliceStable(m.entries[key], func(i, j int) bool { + return m.entries[key][i].priority < m.entries[key][j].priority + }) + + for _, entry := range m.entries[key] { + valStrings = append(valStrings, fmt.Sprintf("%s%s%v", key, entry.delimiter, entry.value)) + } } return valStrings diff --git a/pkg/util/prioritymap/map.go b/pkg/util/prioritymap/map.go index 3018010134..484ccbb281 100644 --- a/pkg/util/prioritymap/map.go +++ b/pkg/util/prioritymap/map.go @@ -6,22 +6,23 @@ import ( corev1 "k8s.io/api/core/v1" ) -type entry struct { - value any - delimiter string - priority int -} - const DefaultPriority = LowPriority const LowPriority = 1 const MediumPriority = 5 const HighPriority = 10 type Map struct { - entries map[string]entry + entries map[string][]entry defaultOptions []Option } +type entry struct { + value any + delimiter string + priority int + allowDuplicates bool +} + type Option func(a *entry) func WithPriority(priority int) Option { @@ -36,9 +37,16 @@ func WithSeparator(separator string) Option { } } +// WithAllowDuplicatesForKey allows to add multiple values for the same key (covers all keys) +func WithAllowDuplicates() Option { + return func(a *entry) { + a.allowDuplicates = true + } +} + func New(defaultOptions ...Option) *Map { m := &Map{ - entries: make(map[string]entry), + entries: make(map[string][]entry), defaultOptions: defaultOptions, } @@ -51,8 +59,9 @@ func (m Map) Append(key string, value any, opts ...Option) { } newArg := entry{ - value: value, - priority: DefaultPriority, + value: value, + priority: DefaultPriority, + allowDuplicates: false, } for _, opt := range m.defaultOptions { @@ -65,8 +74,12 @@ func (m Map) Append(key string, value any, opts ...Option) { key, _ = strings.CutSuffix(key, newArg.delimiter) - if existingArg, exists := m.entries[key]; !exists || newArg.priority > existingArg.priority { - m.entries[key] = newArg + if existingArg, exists := m.entries[key]; !exists || newArg.allowDuplicates || newArg.priority > existingArg[0].priority { + if !exists || !newArg.allowDuplicates { + m.entries[key] = make([]entry, 0) + } + + m.entries[key] = append(m.entries[key], newArg) } }