Skip to content

Commit

Permalink
Fix OneAgent command line arguments (#4357)
Browse files Browse the repository at this point in the history
  • Loading branch information
@StefanHauth
StefanHauth committed Jan 28, 2025
1 parent 99214d3 commit 8dd230c
Show file tree
Hide file tree
Showing 6 changed files with 189 additions and 15 deletions.
1 change: 1 addition & 0 deletions pkg/controllers/dynakube/logmonitoring/daemonset/volumes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const (

func TestGetVolumeMounts(t *testing.T) {
tenantUUID := "test-uuid"

t.Run("get volume mounts", func(t *testing.T) {
mounts := getVolumeMounts(tenantUUID)

Expand Down
4 changes: 3 additions & 1 deletion pkg/controllers/dynakube/oneagent/daemonset/arguments.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ func (b *builder) arguments() ([]string, error) {
argMap := prioritymap.New(
prioritymap.WithSeparator(prioritymap.DefaultSeparator),
prioritymap.WithPriority(defaultArgumentPriority),
prioritymap.WithAllowDuplicates(),
prioritymap.WithAvoidDuplicates(),
prioritymap.WithAllowDuplicatesFor("--set-host-property"),
prioritymap.WithAllowDuplicatesFor("--set-host-tag"),
)

isProxyAsEnvDeprecated, err := isProxyAsEnvVarDeprecated(b.dk.OneAgent().GetVersion())
Expand Down
33 changes: 25 additions & 8 deletions pkg/controllers/dynakube/oneagent/daemonset/arguments_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,28 +88,46 @@ func TestArguments(t *testing.T) {
}
assert.Equal(t, expectedDefaultArguments, arguments)
})
t.Run("when injected arguments are provided then they come last", func(t *testing.T) {
args := []string{
t.Run("when injected arguments are provided then they come last, only allowed duplicates and no duplicate key/value pairs", func(t *testing.T) {
custArgs := []string{
"--set-app-log-content-access=true",
"--set-host-id-source=lustiglustig",
"--set-host-id-source=fqdn",
"--set-host-group=APP_LUSTIG_PETER",
"--set-server=https://hyper.super.com:9999",
"--set-host-property=prop1",
"--set-host-property=prop1",
"--set-host-property=prop1",
"--set-host-property=prop2",
"--set-host-property=prop2",
"--set-host-property=prop3",
"--set-host-property=prop3",
"--set-host-property=prop3",
"--set-host-tag=tag1",
"--set-host-tag=tag2",
"--set-host-tag=tag2",
"--set-host-tag=tag2",
"--set-host-tag=tag3",
}
builder := builder{
dk: &dynakube.DynaKube{},
hostInjectSpec: &oneagent.HostInjectSpec{Args: args},
dk: &dynakube.DynaKube{Spec: dynakube.DynaKubeSpec{OneAgent: oneagent.Spec{ClassicFullStack: &oneagent.HostInjectSpec{}}}},
hostInjectSpec: &oneagent.HostInjectSpec{Args: custArgs},
}

arguments, _ := builder.arguments()

expectedDefaultArguments := []string{
"--set-app-log-content-access=true",
"--set-host-group=APP_LUSTIG_PETER",
"--set-host-id-source=lustiglustig",
"--set-host-id-source=fqdn",
"--set-host-property=OperatorVersion=$(DT_OPERATOR_VERSION)",
"--set-host-property=prop1",
"--set-host-property=prop2",
"--set-host-property=prop3",
"--set-host-tag=tag1",
"--set-host-tag=tag2",
"--set-host-tag=tag3",
"--set-no-proxy=",
"--set-proxy=",
"--set-server={$(DT_SERVER)}",
"--set-server=https://hyper.super.com:9999",
"--set-tenant=$(DT_TENANT)",
}
Expand Down Expand Up @@ -217,7 +235,6 @@ func TestArguments(t *testing.T) {
"--set-host-property=item2=value2",
"--set-no-proxy=",
"--set-proxy=",
"--set-server={$(DT_SERVER)}",
"--set-server=https://hyper.super.com:9999",
"--set-tenant=$(DT_TENANT)",
}
Expand Down
5 changes: 1 addition & 4 deletions pkg/controllers/dynakube/oneagent/daemonset/daemonset_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@ func TestOneAgentHostGroup(t *testing.T) {

require.NoError(t, err)
require.NotNil(t, daemonset)
assert.Equal(t, "--set-host-group=newgroup", daemonset.Spec.Template.Spec.Containers[0].Args[1])
assert.Equal(t, "--set-host-group=newgroup", daemonset.Spec.Template.Spec.Containers[0].Args[0])
})
}

Expand Down Expand Up @@ -965,12 +965,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-no-proxy=",
"--set-proxy=",
"--set-server={$(DT_SERVER)}",
"--set-server=https://hyper.super.com:9999",
"--set-tenant=$(DT_TENANT)",
}
Expand All @@ -996,7 +994,6 @@ func TestDefaultArguments(t *testing.T) {
"--set-host-property=OperatorVersion=$(DT_OPERATOR_VERSION)",
"--set-no-proxy=",
"--set-proxy=",
"--set-server={$(DT_SERVER)}",
"--set-server=https://hyper.super.com:9999",
"--set-tenant=$(DT_TENANT)",
}
Expand Down
32 changes: 30 additions & 2 deletions pkg/util/prioritymap/map.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package prioritymap

import (
"slices"
"strings"

corev1 "k8s.io/api/core/v1"
Expand All @@ -18,6 +19,7 @@ type Map struct {

type entry struct {
value any
key string
delimiter string
priority int
allowDuplicates bool
Expand All @@ -37,13 +39,34 @@ 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 WithAvoidDuplicates() Option {
return func(a *entry) {
a.allowDuplicates = false
}
}

func WithAvoidDuplicatesFor(key string) Option {
return func(a *entry) {
if a.key == key {
a.allowDuplicates = false
}
}
}

func WithAllowDuplicatesFor(key string) Option {
return func(a *entry) {
if a.key == key {
a.allowDuplicates = true
}
}
}

func New(defaultOptions ...Option) *Map {
m := &Map{
entries: make(map[string][]entry),
Expand All @@ -59,6 +82,7 @@ func (m Map) Append(key string, value any, opts ...Option) {
}

newArg := entry{
key: key,
value: value,
priority: DefaultPriority,
allowDuplicates: false,
Expand All @@ -79,7 +103,11 @@ func (m Map) Append(key string, value any, opts ...Option) {
m.entries[key] = make([]entry, 0)
}

m.entries[key] = append(m.entries[key], newArg)
if !slices.ContainsFunc(m.entries[key], func(e entry) bool {
return e.value == value
}) {
m.entries[key] = append(m.entries[key], newArg)
}
}
}

Expand Down
129 changes: 129 additions & 0 deletions pkg/util/prioritymap/map_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,132 @@ func TestArgumentSlice(t *testing.T) {

assert.Equal(t, expectedArgs, argMap.AsKeyValueStrings())
}

func TestDuplicateArguments(t *testing.T) {
defaultArgs := []string{
"--set-proxy=127.0.0.1",
"--set-host-id-source=auto",
"--set-server=localhost",
"--set-host-property=prop1",
"--set-host-property=prop2",
"--set-host-property=prop3",
"--set-host-property=prop3",
"--set-host-property=prop3",
}

customArgs := []string{
"--set-host-id-source=fqdn",
"--set-server=foobar.com",
"--set-host-property=custom-prop1",
"--set-host-property=custom-prop2",
"--set-host-property=custom-prop3",
"--set-host-property=custom-prop3",
"--set-host-property=custom-prop3",
}

var tests = []struct {
title string
expectedArgs []string
defaultArgPrio int
customArgsPrio int
mapOptions []Option
}{
{
title: "Enter avoided duplicates with higher prio",
expectedArgs: []string{
"--set-host-id-source=fqdn",
"--set-host-property=prop1",
"--set-host-property=prop2",
"--set-host-property=prop3",
"--set-host-property=custom-prop1",
"--set-host-property=custom-prop2",
"--set-host-property=custom-prop3",
"--set-proxy=127.0.0.1",
"--set-server=foobar.com",
},
defaultArgPrio: DefaultPriority,
customArgsPrio: HighPriority,
mapOptions: []Option{
WithSeparator("="),
WithAllowDuplicates(),
WithAvoidDuplicatesFor("--set-host-id-source"),
WithAvoidDuplicatesFor("--set-server"),
},
},
{
title: "Enter avoided duplicates with lower prio",
expectedArgs: []string{
"--set-host-id-source=auto",
"--set-host-property=custom-prop1",
"--set-host-property=custom-prop2",
"--set-host-property=custom-prop3",
"--set-host-property=prop1",
"--set-host-property=prop2",
"--set-host-property=prop3",
"--set-proxy=127.0.0.1",
"--set-server=localhost",
},
defaultArgPrio: HighPriority,
customArgsPrio: DefaultPriority,
mapOptions: []Option{
WithSeparator("="),
WithAllowDuplicates(),
WithAvoidDuplicatesFor("--set-host-id-source"),
WithAvoidDuplicatesFor("--set-server"),
},
},
{
title: "Enter avoided duplicates with higher prio",
expectedArgs: []string{
"--set-host-id-source=fqdn",
"--set-host-property=prop1",
"--set-host-property=prop2",
"--set-host-property=prop3",
"--set-host-property=custom-prop1",
"--set-host-property=custom-prop2",
"--set-host-property=custom-prop3",
"--set-proxy=127.0.0.1",
"--set-server=foobar.com",
},
defaultArgPrio: DefaultPriority,
customArgsPrio: HighPriority,
mapOptions: []Option{
WithSeparator("="),
WithAvoidDuplicates(),
WithAllowDuplicatesFor("--set-host-property"),
},
},
{
title: "Enter avoided duplicates with lower prio",
expectedArgs: []string{
"--set-host-id-source=auto",
"--set-host-property=custom-prop1",
"--set-host-property=custom-prop2",
"--set-host-property=custom-prop3",
"--set-host-property=prop1",
"--set-host-property=prop2",
"--set-host-property=prop3",
"--set-proxy=127.0.0.1",
"--set-server=localhost",
},
defaultArgPrio: HighPriority,
customArgsPrio: DefaultPriority,
mapOptions: []Option{
WithSeparator("="),
WithAvoidDuplicates(),
WithAllowDuplicatesFor("--set-host-property"),
},
},
}

for _, tt := range tests {
t.Run(tt.title, func(t *testing.T) {
argMap := New(tt.mapOptions...)

Append(argMap, defaultArgs, WithPriority(tt.defaultArgPrio))
Append(argMap, customArgs, WithPriority(tt.customArgsPrio))

assert.Equal(t, tt.expectedArgs, argMap.AsKeyValueStrings())
})
}
}

0 comments on commit 8dd230c

Please sign in to comment.