From d3a1821bc95bdf6e7b623f4ba809dc9a8a1333b3 Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Mon, 4 Nov 2024 15:37:15 -0500 Subject: [PATCH 1/4] Require children map for innerNode constuctor, dont recurse --- pkg/config/nodetreemodel/config.go | 4 +- pkg/config/nodetreemodel/inner_node.go | 59 ++++++++----------- pkg/config/nodetreemodel/inner_node_test.go | 14 ++--- pkg/config/nodetreemodel/node.go | 6 +- pkg/config/nodetreemodel/read_config_file.go | 4 +- .../nodetreemodel/read_config_file_test.go | 22 +++---- pkg/config/nodetreemodel/reflection_node.go | 3 +- 7 files changed, 52 insertions(+), 60 deletions(-) diff --git a/pkg/config/nodetreemodel/config.go b/pkg/config/nodetreemodel/config.go index 58c9f3cce7b6f..6e05855178960 100644 --- a/pkg/config/nodetreemodel/config.go +++ b/pkg/config/nodetreemodel/config.go @@ -795,8 +795,8 @@ func NewConfig(name string, envPrefix string, envKeyReplacer *strings.Replacer) configEnvVars: map[string]struct{}{}, knownKeys: map[string]struct{}{}, unknownKeys: map[string]struct{}{}, - defaults: newInnerNodeImpl(), - file: newInnerNodeImpl(), + defaults: newInnerNode(nil), + file: newInnerNode(nil), } config.SetTypeByDefaultValue(true) diff --git a/pkg/config/nodetreemodel/inner_node.go b/pkg/config/nodetreemodel/inner_node.go index b4048eb5c1fe3..0dda43050b2e6 100644 --- a/pkg/config/nodetreemodel/inner_node.go +++ b/pkg/config/nodetreemodel/inner_node.go @@ -16,47 +16,36 @@ import ( // innerNode represents an non-leaf node of the config type innerNode struct { - val map[string]Node + children map[string]Node // remapCase maps each lower-case key to the original case. This // enables GetChild to retrieve values using case-insensitive keys remapCase map[string]string } -func newInnerNodeImpl() *innerNode { - return &innerNode{val: map[string]Node{}, remapCase: map[string]string{}} -} - -func newInnerNodeImplWithData(v map[string]interface{}, source model.Source) (*innerNode, error) { - children := map[string]Node{} - for name, value := range v { - n, err := NewNode(value, source) - if err != nil { - return nil, err - } - children[name] = n +func newInnerNode(children map[string]Node) *innerNode { + if children == nil { + children = map[string]Node{} } - in := &innerNode{val: children} - in.makeRemapCase() - return in, nil + node := &innerNode{children: children, remapCase: map[string]string{}} + node.makeRemapCase() + return node } var _ Node = (*innerNode)(nil) // Clone clones a InnerNode and all its children func (n *innerNode) Clone() Node { - clone := newInnerNodeImpl() - - for k, node := range n.val { - clone.val[k] = node.Clone() + children := make(map[string]Node) + for k, node := range n.children { + children[k] = node.Clone() } - clone.makeRemapCase() - return clone + return newInnerNode(children) } // makeRemapCase creates a map that converts keys from their lower-cased version to their original case func (n *innerNode) makeRemapCase() { remap := make(map[string]string) - for k := range n.val { + for k := range n.children { remap[strings.ToLower(k)] = k } n.remapCase = remap @@ -65,7 +54,7 @@ func (n *innerNode) makeRemapCase() { // GetChild returns the child node at the given case-insensitive key, or an error if not found func (n *innerNode) GetChild(key string) (Node, error) { mkey := n.remapCase[strings.ToLower(key)] - child, found := n.val[mkey] + child, found := n.children[mkey] if !found { return nil, ErrNotFound } @@ -74,7 +63,7 @@ func (n *innerNode) GetChild(key string) (Node, error) { // HasChild returns true if the node has a child for that given key func (n *innerNode) HasChild(key string) bool { - _, ok := n.val[key] + _, ok := n.children[key] return ok } @@ -86,7 +75,7 @@ func (n *innerNode) Merge(src InnerNode) error { srcChild, _ := src.GetChild(name) if !n.HasChild(name) { - n.val[name] = srcChild.Clone() + n.children[name] = srcChild.Clone() } else { // We alredy have child with the same name @@ -100,7 +89,7 @@ func (n *innerNode) Merge(src InnerNode) error { if srcIsLeaf { if srcLeaf.SourceGreaterOrEqual(dstLeaf.Source()) { - n.val[name] = srcLeaf.Clone() + n.children[name] = srcLeaf.Clone() } } else { dstInner, _ := dstChild.(InnerNode) @@ -116,7 +105,7 @@ func (n *innerNode) Merge(src InnerNode) error { // ChildrenKeys returns the list of keys of the children of the given node, if it is a map func (n *innerNode) ChildrenKeys() []string { - mapkeys := maps.Keys(n.val) + mapkeys := maps.Keys(n.children) // map keys are iterated non-deterministically, sort them slices.Sort(mapkeys) return mapkeys @@ -136,15 +125,15 @@ func (n *innerNode) SetAt(key []string, value interface{}, source model.Source) part := key[0] if keyLen == 1 { - node, ok := n.val[part] + node, ok := n.children[part] if !ok { - n.val[part] = newLeafNodeImpl(value, source) + n.children[part] = newLeafNodeImpl(value, source) return true, nil } if leaf, ok := node.(LeafNode); ok { if leaf.Source().IsGreaterOrEqualThan(source) { - n.val[part] = newLeafNodeImpl(value, source) + n.children[part] = newLeafNodeImpl(value, source) return true, nil } return false, nil @@ -153,9 +142,9 @@ func (n *innerNode) SetAt(key []string, value interface{}, source model.Source) } // new node case - if _, ok := n.val[part]; !ok { - newNode := newInnerNodeImpl() - n.val[part] = newNode + if _, ok := n.children[part]; !ok { + newNode := newInnerNode(nil) + n.children[part] = newNode return newNode.SetAt(key[1:keyLen], value, source) } @@ -170,6 +159,6 @@ func (n *innerNode) SetAt(key []string, value interface{}, source model.Source) // InsertChildNode sets a node in the current node func (n *innerNode) InsertChildNode(name string, node Node) { - n.val[name] = node + n.children[name] = node n.makeRemapCase() } diff --git a/pkg/config/nodetreemodel/inner_node_test.go b/pkg/config/nodetreemodel/inner_node_test.go index 5e87583712fc1..df9a1f45081b9 100644 --- a/pkg/config/nodetreemodel/inner_node_test.go +++ b/pkg/config/nodetreemodel/inner_node_test.go @@ -30,23 +30,23 @@ func TestMergeToEmpty(t *testing.T) { src, ok := node.(InnerNode) require.True(t, ok) - dst := newInnerNodeImpl() + dst := newInnerNode(nil) err = dst.Merge(src) require.NoError(t, err) expected := &innerNode{ remapCase: map[string]string{"a": "a", "b": "b", "c": "c"}, - val: map[string]Node{ + children: map[string]Node{ "a": &leafNodeImpl{val: "apple", source: model.SourceFile}, "b": &leafNodeImpl{val: 123, source: model.SourceFile}, "c": &innerNode{ remapCase: map[string]string{"d": "d", "e": "e"}, - val: map[string]Node{ + children: map[string]Node{ "d": &leafNodeImpl{val: true, source: model.SourceFile}, "e": &innerNode{ remapCase: map[string]string{"f": "f"}, - val: map[string]Node{ + children: map[string]Node{ "f": &leafNodeImpl{val: 456, source: model.SourceFile}, }, }, @@ -96,17 +96,17 @@ func TestMergeTwoTree(t *testing.T) { expected := &innerNode{ remapCase: map[string]string{"a": "a", "b": "b", "z": "z", "c": "c"}, - val: map[string]Node{ + children: map[string]Node{ "a": &leafNodeImpl{val: "orange", source: model.SourceEnvVar}, "b": &leafNodeImpl{val: 123, source: model.SourceFile}, "z": &leafNodeImpl{val: 987, source: model.SourceEnvVar}, "c": &innerNode{ remapCase: map[string]string{"d": "d", "e": "e"}, - val: map[string]Node{ + children: map[string]Node{ "d": &leafNodeImpl{val: false, source: model.SourceEnvVar}, "e": &innerNode{ remapCase: map[string]string{"f": "f", "g": "g"}, - val: map[string]Node{ + children: map[string]Node{ "f": &leafNodeImpl{val: 456, source: model.SourceEnvVar}, "g": &leafNodeImpl{val: "kiwi", source: model.SourceEnvVar}, }, diff --git a/pkg/config/nodetreemodel/node.go b/pkg/config/nodetreemodel/node.go index 3e73528bf0f01..b469b334a66f2 100644 --- a/pkg/config/nodetreemodel/node.go +++ b/pkg/config/nodetreemodel/node.go @@ -19,9 +19,11 @@ var ErrNotFound = fmt.Errorf("not found") func NewNode(v interface{}, source model.Source) (Node, error) { switch it := v.(type) { case map[interface{}]interface{}: - return newInnerNodeImplWithData(mapInterfaceToMapString(it), source) + return nil, fmt.Errorf("not implemented") + //return newInnerNodeImplWithData(mapInterfaceToMapString(it), source) case map[string]interface{}: - return newInnerNodeImplWithData(it, source) + return nil, fmt.Errorf("not implemented") + //return newInnerNodeImplWithData(it, source) case []interface{}: return newArrayNodeImpl(it, source) } diff --git a/pkg/config/nodetreemodel/read_config_file.go b/pkg/config/nodetreemodel/read_config_file.go index 942062c9fb536..1d6a4e22cff9e 100644 --- a/pkg/config/nodetreemodel/read_config_file.go +++ b/pkg/config/nodetreemodel/read_config_file.go @@ -17,7 +17,7 @@ import ( ) func (c *ntmConfig) mergeAllLayers() error { - root := newInnerNodeImpl() + root := newInnerNode(nil) treeList := []InnerNode{ c.defaults, @@ -165,7 +165,7 @@ func loadYamlInto(defaults InnerNode, dest InnerNode, data map[string]interface{ defaultNext, _ := defaultNode.(InnerNode) if !dest.HasChild(key) { - destInner := newInnerNodeImpl() + destInner := newInnerNode(nil) warnings = append(warnings, loadYamlInto(defaultNext, destInner, mapString, curPath)...) dest.InsertChildNode(key, destInner) continue diff --git a/pkg/config/nodetreemodel/read_config_file_test.go b/pkg/config/nodetreemodel/read_config_file_test.go index ac8288e7b904f..ccf51e658e1d4 100644 --- a/pkg/config/nodetreemodel/read_config_file_test.go +++ b/pkg/config/nodetreemodel/read_config_file_test.go @@ -179,7 +179,7 @@ c: defaults, ok := newNode.(InnerNode) require.True(t, ok) - tree := newInnerNodeImpl() + tree := newInnerNode(nil) warnings := loadYamlInto(defaults, tree, yamlData, "") @@ -187,11 +187,11 @@ c: expected := &innerNode{ remapCase: map[string]string{"a": "a", "c": "c"}, - val: map[string]Node{ + children: map[string]Node{ "a": &leafNodeImpl{val: "orange", source: model.SourceFile}, "c": &innerNode{ remapCase: map[string]string{"d": "d"}, - val: map[string]Node{ + children: map[string]Node{ "d": &leafNodeImpl{val: 1234, source: model.SourceFile}, }, }, @@ -223,7 +223,7 @@ c: defaults, ok := newNode.(InnerNode) require.True(t, ok) - tree := newInnerNodeImpl() + tree := newInnerNode(nil) warnings := loadYamlInto(defaults, tree, yamlData, "") @@ -232,11 +232,11 @@ c: expected := &innerNode{ remapCase: map[string]string{"a": "a", "c": "c"}, - val: map[string]Node{ + children: map[string]Node{ "a": &leafNodeImpl{val: "orange", source: model.SourceFile}, "c": &innerNode{ remapCase: map[string]string{"d": "d"}, - val: map[string]Node{ + children: map[string]Node{ "d": &leafNodeImpl{val: 1234, source: model.SourceFile}, }, }, @@ -266,7 +266,7 @@ c: 1234 defaults, ok := newNode.(InnerNode) require.True(t, ok) - tree := newInnerNodeImpl() + tree := newInnerNode(nil) warnings := loadYamlInto(defaults, tree, yamlData, "") @@ -275,11 +275,11 @@ c: 1234 expected := &innerNode{ remapCase: map[string]string{"a": "a", "c": "c"}, - val: map[string]Node{ + children: map[string]Node{ "a": &leafNodeImpl{val: "orange", source: model.SourceFile}, "c": &innerNode{ remapCase: map[string]string{}, - val: map[string]Node{}, + children: map[string]Node{}, }, }, } @@ -307,7 +307,7 @@ a: defaults, ok := newNode.(InnerNode) require.True(t, ok) - tree := newInnerNodeImpl() + tree := newInnerNode(nil) tree.SetAt([]string{"a", "b", "c"}, 9876, model.SourceFile) warnings := loadYamlInto(defaults, tree, yamlData, "") @@ -339,7 +339,7 @@ a: defaults, ok := newNode.(InnerNode) require.True(t, ok) - tree := newInnerNodeImpl() + tree := newInnerNode(nil) tree.SetAt([]string{"a", "b"}, 9876, model.SourceFile) warnings := loadYamlInto(defaults, tree, yamlData, "") diff --git a/pkg/config/nodetreemodel/reflection_node.go b/pkg/config/nodetreemodel/reflection_node.go index aeb25e5dd6e1b..8f6a563914547 100644 --- a/pkg/config/nodetreemodel/reflection_node.go +++ b/pkg/config/nodetreemodel/reflection_node.go @@ -52,7 +52,8 @@ func asReflectionNode(v interface{}) (Node, error) { } res[kstr] = rv.MapIndex(mk).Interface() } - return newInnerNodeImplWithData(res, model.SourceDefault) + // TODO: not correct, res is not being used + return newInnerNode(nil), nil } return nil, errUnknownConversion } From 8c98b573a339c5241260c01b0ef77692177fb21c Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Mon, 4 Nov 2024 16:14:51 -0500 Subject: [PATCH 2/4] Fix up remaining usages of NewNode --- pkg/config/nodetreemodel/array_node.go | 3 +- pkg/config/nodetreemodel/config_test.go | 5 +-- pkg/config/nodetreemodel/inner_node_test.go | 10 +++--- pkg/config/nodetreemodel/node.go | 35 +++++++++++++++---- pkg/config/nodetreemodel/node_test.go | 2 +- .../nodetreemodel/read_config_file_test.go | 12 +++---- pkg/config/nodetreemodel/reflection_node.go | 12 +++---- .../nodetreemodel/reflection_node_test.go | 5 ++- pkg/config/nodetreemodel/struct_node.go | 2 +- pkg/config/structure/unmarshal.go | 2 +- pkg/config/structure/unmarshal_test.go | 2 +- 11 files changed, 53 insertions(+), 37 deletions(-) diff --git a/pkg/config/nodetreemodel/array_node.go b/pkg/config/nodetreemodel/array_node.go index 41f2791aafc3a..48e0aafbd3417 100644 --- a/pkg/config/nodetreemodel/array_node.go +++ b/pkg/config/nodetreemodel/array_node.go @@ -32,7 +32,8 @@ func newArrayNodeImpl(v []interface{}, source model.Source) (Node, error) { nodes = append(nodes, n) continue } - n, err := NewNode(it, source) + // TODO: Not correct, going to delete this + n, err := NewNodeTree(it, source) if err != nil { return nil, err } diff --git a/pkg/config/nodetreemodel/config_test.go b/pkg/config/nodetreemodel/config_test.go index caff3e69eef4b..b3a030a4fd4ed 100644 --- a/pkg/config/nodetreemodel/config_test.go +++ b/pkg/config/nodetreemodel/config_test.go @@ -11,11 +11,8 @@ import ( "github.com/stretchr/testify/require" ) -// The current implementation uses NewNode to build the tree, but it treats leafs with -// map data as though those maps should also become nodes. +// NOTE: Fix this test in this PR func TestBuildDefaultMakesTooManyNodes(t *testing.T) { - t.Skip("test fails because the tree builder is too aggressive in making nodes") - cfg := NewConfig("test", "", nil) cfg.BindEnvAndSetDefault("kubernetes_node_annotations_as_tags", map[string]string{"cluster.k8s.io/machine": "kube_machine"}) cfg.BuildSchema() diff --git a/pkg/config/nodetreemodel/inner_node_test.go b/pkg/config/nodetreemodel/inner_node_test.go index df9a1f45081b9..1d5ab2daa181d 100644 --- a/pkg/config/nodetreemodel/inner_node_test.go +++ b/pkg/config/nodetreemodel/inner_node_test.go @@ -25,7 +25,7 @@ func TestMergeToEmpty(t *testing.T) { }, } - node, err := NewNode(obj, model.SourceFile) + node, err := NewNodeTree(obj, model.SourceFile) require.NoError(t, err) src, ok := node.(InnerNode) require.True(t, ok) @@ -81,12 +81,12 @@ func TestMergeTwoTree(t *testing.T) { }, } - node, err := NewNode(obj, model.SourceFile) + node, err := NewNodeTree(obj, model.SourceFile) require.NoError(t, err) base, ok := node.(InnerNode) require.True(t, ok) - node, err = NewNode(obj2, model.SourceEnvVar) + node, err = NewNodeTree(obj2, model.SourceEnvVar) require.NoError(t, err) overwrite, ok := node.(InnerNode) require.True(t, ok) @@ -127,12 +127,12 @@ func TestMergeErrorLeafToNode(t *testing.T) { "a": map[string]interface{}{}, } - node, err := NewNode(obj, model.SourceFile) + node, err := NewNodeTree(obj, model.SourceFile) require.NoError(t, err) base, ok := node.(InnerNode) require.True(t, ok) - node, err = NewNode(obj2, model.SourceEnvVar) + node, err = NewNodeTree(obj2, model.SourceEnvVar) require.NoError(t, err) overwrite, ok := node.(InnerNode) require.True(t, ok) diff --git a/pkg/config/nodetreemodel/node.go b/pkg/config/nodetreemodel/node.go index b469b334a66f2..a52cfa8119c33 100644 --- a/pkg/config/nodetreemodel/node.go +++ b/pkg/config/nodetreemodel/node.go @@ -15,15 +15,21 @@ import ( // ErrNotFound is an error for when a key is not found var ErrNotFound = fmt.Errorf("not found") -// NewNode constructs a Node from either a map, a slice, or a scalar value -func NewNode(v interface{}, source model.Source) (Node, error) { +// NewNodeTree will recursively create nodes from the input value to construct a tree +func NewNodeTree(v interface{}, source model.Source) (Node, error) { switch it := v.(type) { case map[interface{}]interface{}: - return nil, fmt.Errorf("not implemented") - //return newInnerNodeImplWithData(mapInterfaceToMapString(it), source) + children, err := makeChildNodeTrees(mapInterfaceToMapString(it), source) + if err != nil { + return nil, err + } + return newInnerNode(children), nil case map[string]interface{}: - return nil, fmt.Errorf("not implemented") - //return newInnerNodeImplWithData(it, source) + children, err := makeChildNodeTrees(it, source) + if err != nil { + return nil, err + } + return newInnerNode(children), nil case []interface{}: return newArrayNodeImpl(it, source) } @@ -39,6 +45,23 @@ func NewNode(v interface{}, source model.Source) (Node, error) { return node, err } +// NewLeafTree creates a new leaf node +func NewLeafNode(v interface{}, source model.Source) (Node, error) { + return newLeafNodeImpl(v, source), nil +} + +func makeChildNodeTrees(input map[string]interface{}, source model.Source) (map[string]Node, error) { + children := make(map[string]Node) + for k, v := range input { + node, err := NewNodeTree(v, source) + if err != nil { + return nil, err + } + children[k] = node + } + return children, nil +} + // NodeType represents node types in the tree (ie: inner or leaf) type NodeType int diff --git a/pkg/config/nodetreemodel/node_test.go b/pkg/config/nodetreemodel/node_test.go index 5588795f3d96e..2088434204cf4 100644 --- a/pkg/config/nodetreemodel/node_test.go +++ b/pkg/config/nodetreemodel/node_test.go @@ -22,7 +22,7 @@ func TestNewNodeAndNodeMethods(t *testing.T) { }, } - node, err := NewNode(obj, model.SourceDefault) + node, err := NewNodeTree(obj, model.SourceDefault) assert.NoError(t, err) n, ok := node.(InnerNode) diff --git a/pkg/config/nodetreemodel/read_config_file_test.go b/pkg/config/nodetreemodel/read_config_file_test.go index ccf51e658e1d4..0e89d316c454c 100644 --- a/pkg/config/nodetreemodel/read_config_file_test.go +++ b/pkg/config/nodetreemodel/read_config_file_test.go @@ -48,7 +48,7 @@ func setupDefault(t *testing.T, cfg model.Config) *ntmConfig { }, }, } - newNode, err := NewNode(obj, model.SourceDefault) + newNode, err := NewNodeTree(obj, model.SourceDefault) require.NoError(t, err) defaults, ok := newNode.(InnerNode) require.True(t, ok) @@ -174,7 +174,7 @@ c: }, }, } - newNode, err := NewNode(obj, model.SourceDefault) + newNode, err := NewNodeTree(obj, model.SourceDefault) require.NoError(t, err) defaults, ok := newNode.(InnerNode) require.True(t, ok) @@ -218,7 +218,7 @@ c: "d": true, }, } - newNode, err := NewNode(obj, model.SourceDefault) + newNode, err := NewNodeTree(obj, model.SourceDefault) require.NoError(t, err) defaults, ok := newNode.(InnerNode) require.True(t, ok) @@ -261,7 +261,7 @@ c: 1234 "d": true, }, } - newNode, err := NewNode(obj, model.SourceDefault) + newNode, err := NewNodeTree(obj, model.SourceDefault) require.NoError(t, err) defaults, ok := newNode.(InnerNode) require.True(t, ok) @@ -302,7 +302,7 @@ a: "b": true, }, } - newNode, err := NewNode(obj, model.SourceDefault) + newNode, err := NewNodeTree(obj, model.SourceDefault) require.NoError(t, err) defaults, ok := newNode.(InnerNode) require.True(t, ok) @@ -334,7 +334,7 @@ a: }, }, } - newNode, err := NewNode(obj, model.SourceDefault) + newNode, err := NewNodeTree(obj, model.SourceDefault) require.NoError(t, err) defaults, ok := newNode.(InnerNode) require.True(t, ok) diff --git a/pkg/config/nodetreemodel/reflection_node.go b/pkg/config/nodetreemodel/reflection_node.go index 8f6a563914547..9fab0ae3f02e7 100644 --- a/pkg/config/nodetreemodel/reflection_node.go +++ b/pkg/config/nodetreemodel/reflection_node.go @@ -33,13 +33,10 @@ func asReflectionNode(v interface{}) (Node, error) { } else if rv.Kind() == reflect.Slice { elems := make([]interface{}, 0, rv.Len()) for i := 0; i < rv.Len(); i++ { - node, err := NewNode(rv.Index(i).Interface(), model.SourceDefault) - if err != nil { - return nil, err - } - elems = append(elems, node) + item := rv.Index(i).Interface() + elems = append(elems, item) } - return newArrayNodeImpl(elems, model.SourceDefault) + return NewLeafNode(elems, model.SourceUnknown) } else if rv.Kind() == reflect.Map { res := make(map[string]interface{}, rv.Len()) mapkeys := rv.MapKeys() @@ -52,8 +49,7 @@ func asReflectionNode(v interface{}) (Node, error) { } res[kstr] = rv.MapIndex(mk).Interface() } - // TODO: not correct, res is not being used - return newInnerNode(nil), nil + return NewLeafNode(res, model.SourceUnknown) } return nil, errUnknownConversion } diff --git a/pkg/config/nodetreemodel/reflection_node_test.go b/pkg/config/nodetreemodel/reflection_node_test.go index eb3a40392ff68..3a4b6e124ddf2 100644 --- a/pkg/config/nodetreemodel/reflection_node_test.go +++ b/pkg/config/nodetreemodel/reflection_node_test.go @@ -8,7 +8,6 @@ package nodetreemodel import ( "testing" - "github.com/DataDog/datadog-agent/pkg/config/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -19,10 +18,10 @@ type Object struct { } func TestNewReflectionNode(t *testing.T) { - node, err := NewNode(Object{ + node, err := asReflectionNode(Object{ Name: "test", Num: 7, - }, model.SourceDefault) + }) assert.NoError(t, err) n, ok := node.(InnerNode) diff --git a/pkg/config/nodetreemodel/struct_node.go b/pkg/config/nodetreemodel/struct_node.go index 1b9eea58961ef..b1a14895a5cee 100644 --- a/pkg/config/nodetreemodel/struct_node.go +++ b/pkg/config/nodetreemodel/struct_node.go @@ -33,7 +33,7 @@ func (n *structNodeImpl) GetChild(key string) (Node, error) { if inner.Kind() == reflect.Interface { inner = inner.Elem() } - return NewNode(inner.Interface(), model.SourceDefault) + return NewNodeTree(inner.Interface(), model.SourceDefault) } func (n *structNodeImpl) HasChild(name string) bool { diff --git a/pkg/config/structure/unmarshal.go b/pkg/config/structure/unmarshal.go index 460ed9b973bc6..00eb366b05428 100644 --- a/pkg/config/structure/unmarshal.go +++ b/pkg/config/structure/unmarshal.go @@ -63,7 +63,7 @@ func unmarshalKeyReflection(cfg model.Reader, key string, target interface{}, op if rawval == nil { return nil } - source, err := nodetreemodel.NewNode(rawval, cfg.GetSource(key)) + source, err := nodetreemodel.NewNodeTree(rawval, cfg.GetSource(key)) if err != nil { return err } diff --git a/pkg/config/structure/unmarshal_test.go b/pkg/config/structure/unmarshal_test.go index 255638d0d4ecf..a137e80826503 100644 --- a/pkg/config/structure/unmarshal_test.go +++ b/pkg/config/structure/unmarshal_test.go @@ -1261,7 +1261,7 @@ service: func TestMapGetChildNotFound(t *testing.T) { m := map[string]interface{}{"a": "apple", "b": "banana"} - n, err := nodetreemodel.NewNode(m, model.SourceDefault) + n, err := nodetreemodel.NewNodeTree(m, model.SourceDefault) assert.NoError(t, err) val, err := n.GetChild("a") From d1750f4dedc0dec4af206590c712362dc5e57a2a Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Mon, 4 Nov 2024 16:34:09 -0500 Subject: [PATCH 3/4] Remove ArrayNode, allow leaf to have complex values Test in config_test.go now passes since a leaf can be a slice or map --- pkg/config/nodetreemodel/array_node.go | 91 ------------------------- pkg/config/nodetreemodel/config_test.go | 2 +- pkg/config/nodetreemodel/node.go | 2 +- pkg/config/structure/unmarshal.go | 36 ++++++---- 4 files changed, 26 insertions(+), 105 deletions(-) delete mode 100644 pkg/config/nodetreemodel/array_node.go diff --git a/pkg/config/nodetreemodel/array_node.go b/pkg/config/nodetreemodel/array_node.go deleted file mode 100644 index 48e0aafbd3417..0000000000000 --- a/pkg/config/nodetreemodel/array_node.go +++ /dev/null @@ -1,91 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2016-present Datadog, Inc. - -package nodetreemodel - -import ( - "fmt" - - "github.com/DataDog/datadog-agent/pkg/config/model" -) - -// ArrayNode represents a node with ordered, numerically indexed set of children -type ArrayNode interface { - Size() int - Index(int) (Node, error) -} - -type arrayNodeImpl struct { - nodes []Node - source model.Source -} - -var _ ArrayNode = (*arrayNodeImpl)(nil) -var _ Node = (*arrayNodeImpl)(nil) - -func newArrayNodeImpl(v []interface{}, source model.Source) (Node, error) { - nodes := make([]Node, 0, len(v)) - for _, it := range v { - if n, ok := it.(Node); ok { - nodes = append(nodes, n) - continue - } - // TODO: Not correct, going to delete this - n, err := NewNodeTree(it, source) - if err != nil { - return nil, err - } - nodes = append(nodes, n) - } - return &arrayNodeImpl{nodes: nodes}, nil -} - -// GetChild returns an error because array node does not have children accessible by name -func (n *arrayNodeImpl) GetChild(string) (Node, error) { - return nil, fmt.Errorf("arrayNodeImpl.GetChild not implemented") -} - -// Clone clones a LeafNode -func (n *arrayNodeImpl) Clone() Node { - clone := &arrayNodeImpl{nodes: make([]Node, len(n.nodes)), source: n.source} - for idx, n := range n.nodes { - clone.nodes[idx] = n.Clone() - } - return clone -} - -// SourceGreaterOrEqual returns true if the source of the current node is greater or equal to the one given as a -// parameter -func (n *arrayNodeImpl) SourceGreaterOrEqual(source model.Source) bool { - return n.source.IsGreaterOrEqualThan(source) -} - -// ChildrenKeys returns an error because array node does not have children accessible by name -func (n *arrayNodeImpl) ChildrenKeys() ([]string, error) { - return nil, fmt.Errorf("arrayNodeImpl.ChildrenKeys not implemented") -} - -// Size returns number of children in the list -func (n *arrayNodeImpl) Size() int { - return len(n.nodes) -} - -// Index returns the kth element of the list -func (n *arrayNodeImpl) Index(k int) (Node, error) { - if k < 0 || k >= len(n.nodes) { - return nil, ErrNotFound - } - return n.nodes[k], nil -} - -// Source returns the source for this leaf -func (n *arrayNodeImpl) Source() model.Source { - return n.source -} - -// Set is not implemented for an array node -func (n *arrayNodeImpl) Set([]string, interface{}, model.Source) (bool, error) { - return false, fmt.Errorf("not implemented") -} diff --git a/pkg/config/nodetreemodel/config_test.go b/pkg/config/nodetreemodel/config_test.go index b3a030a4fd4ed..225ff00a83f84 100644 --- a/pkg/config/nodetreemodel/config_test.go +++ b/pkg/config/nodetreemodel/config_test.go @@ -11,7 +11,7 @@ import ( "github.com/stretchr/testify/require" ) -// NOTE: Fix this test in this PR +// Test that a setting with a map value is seen as a leaf by the nodetreemodel config func TestBuildDefaultMakesTooManyNodes(t *testing.T) { cfg := NewConfig("test", "", nil) cfg.BindEnvAndSetDefault("kubernetes_node_annotations_as_tags", map[string]string{"cluster.k8s.io/machine": "kube_machine"}) diff --git a/pkg/config/nodetreemodel/node.go b/pkg/config/nodetreemodel/node.go index a52cfa8119c33..79507e4fd8c9e 100644 --- a/pkg/config/nodetreemodel/node.go +++ b/pkg/config/nodetreemodel/node.go @@ -31,7 +31,7 @@ func NewNodeTree(v interface{}, source model.Source) (Node, error) { } return newInnerNode(children), nil case []interface{}: - return newArrayNodeImpl(it, source) + return newLeafNodeImpl(it, source), nil } if isScalar(v) { return newLeafNodeImpl(v, source), nil diff --git a/pkg/config/structure/unmarshal.go b/pkg/config/structure/unmarshal.go index 00eb366b05428..a1a54c35ec72e 100644 --- a/pkg/config/structure/unmarshal.go +++ b/pkg/config/structure/unmarshal.go @@ -77,8 +77,11 @@ func unmarshalKeyReflection(cfg model.Reader, key string, target interface{}, op case reflect.Struct: return copyStruct(outValue, source, fs) case reflect.Slice: - if arr, ok := source.(nodetreemodel.ArrayNode); ok { - return copyList(outValue, arr, fs) + if leaf, ok := source.(nodetreemodel.LeafNode); ok { + thing, _ := leaf.GetAny() + if arr, ok := thing.([]interface{}); ok { + return copyList(outValue, makeNodeArray(arr), fs) + } } if isEmptyString(source) { if fs.convertEmptyStrNil { @@ -234,22 +237,19 @@ func copyLeaf(target reflect.Value, source nodetreemodel.LeafNode, _ *featureSet return fmt.Errorf("unsupported scalar type %v", target.Kind()) } -func copyList(target reflect.Value, source nodetreemodel.ArrayNode, fs *featureSet) error { - if source == nil { +func copyList(target reflect.Value, sourceList []nodetreemodel.Node, fs *featureSet) error { + if sourceList == nil { return fmt.Errorf("source value is not a list") } elemType := target.Type() elemType = elemType.Elem() - numElems := source.Size() + numElems := len(sourceList) results := reflect.MakeSlice(reflect.SliceOf(elemType), numElems, numElems) for k := 0; k < numElems; k++ { - elemSource, err := source.Index(k) - if err != nil { - return err - } + elemSource := sourceList[k] ptrOut := reflect.New(elemType) outTarget := ptrOut.Elem() - err = copyAny(outTarget, elemSource, fs) + err := copyAny(outTarget, elemSource, fs) if err != nil { return err } @@ -275,8 +275,11 @@ func copyAny(target reflect.Value, source nodetreemodel.Node, fs *featureSet) er } else if target.Kind() == reflect.Struct { return copyStruct(target, source, fs) } else if target.Kind() == reflect.Slice { - if arr, ok := source.(nodetreemodel.ArrayNode); ok { - return copyList(target, arr, fs) + if leaf, ok := source.(nodetreemodel.LeafNode); ok { + thing, _ := leaf.GetAny() + if arr, ok := thing.([]interface{}); ok { + return copyList(target, makeNodeArray(arr), fs) + } } return fmt.Errorf("can't copy into target: []T required, but source is not an array") } else if target.Kind() == reflect.Invalid { @@ -285,6 +288,15 @@ func copyAny(target reflect.Value, source nodetreemodel.Node, fs *featureSet) er return fmt.Errorf("unknown value to copy: %v", target.Type()) } +func makeNodeArray(vals []interface{}) []nodetreemodel.Node { + res := make([]nodetreemodel.Node, 0, len(vals)) + for _, v := range vals { + node, _ := nodetreemodel.NewNodeTree(v, model.SourceUnknown) + res = append(res, node) + } + return res +} + func isEmptyString(source nodetreemodel.Node) bool { if leaf, ok := source.(nodetreemodel.LeafNode); ok { if str, err := leaf.GetString(); err == nil { From f52239f3c3affcd0839fde5caf5ef13c76f8e7dd Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Tue, 5 Nov 2024 09:23:34 -0500 Subject: [PATCH 4/4] Remove public constructor for leafNode --- pkg/config/nodetreemodel/inner_node.go | 4 ++-- pkg/config/nodetreemodel/leaf_node.go | 4 ++-- pkg/config/nodetreemodel/node.go | 9 ++------- pkg/config/nodetreemodel/read_config_file.go | 2 +- pkg/config/nodetreemodel/reflection_node.go | 4 ++-- 5 files changed, 9 insertions(+), 14 deletions(-) diff --git a/pkg/config/nodetreemodel/inner_node.go b/pkg/config/nodetreemodel/inner_node.go index 0dda43050b2e6..5fd25ea345eb3 100644 --- a/pkg/config/nodetreemodel/inner_node.go +++ b/pkg/config/nodetreemodel/inner_node.go @@ -127,13 +127,13 @@ func (n *innerNode) SetAt(key []string, value interface{}, source model.Source) if keyLen == 1 { node, ok := n.children[part] if !ok { - n.children[part] = newLeafNodeImpl(value, source) + n.children[part] = newLeafNode(value, source) return true, nil } if leaf, ok := node.(LeafNode); ok { if leaf.Source().IsGreaterOrEqualThan(source) { - n.children[part] = newLeafNodeImpl(value, source) + n.children[part] = newLeafNode(value, source) return true, nil } return false, nil diff --git a/pkg/config/nodetreemodel/leaf_node.go b/pkg/config/nodetreemodel/leaf_node.go index 5e8943cba5a94..fda4b382e90d6 100644 --- a/pkg/config/nodetreemodel/leaf_node.go +++ b/pkg/config/nodetreemodel/leaf_node.go @@ -34,13 +34,13 @@ func isScalar(v interface{}) bool { } } -func newLeafNodeImpl(v interface{}, source model.Source) Node { +func newLeafNode(v interface{}, source model.Source) Node { return &leafNodeImpl{val: v, source: source} } // Clone clones a LeafNode func (n *leafNodeImpl) Clone() Node { - return newLeafNodeImpl(n.val, n.source) + return newLeafNode(n.val, n.source) } // SourceGreaterOrEqual returns true if the source of the current node is greater or equal to the one given as a diff --git a/pkg/config/nodetreemodel/node.go b/pkg/config/nodetreemodel/node.go index 79507e4fd8c9e..07977809b0017 100644 --- a/pkg/config/nodetreemodel/node.go +++ b/pkg/config/nodetreemodel/node.go @@ -31,10 +31,10 @@ func NewNodeTree(v interface{}, source model.Source) (Node, error) { } return newInnerNode(children), nil case []interface{}: - return newLeafNodeImpl(it, source), nil + return newLeafNode(it, source), nil } if isScalar(v) { - return newLeafNodeImpl(v, source), nil + return newLeafNode(v, source), nil } // Finally, try determining node type using reflection, should only be needed for unit tests that // supply data that isn't one of the "plain" types produced by parsing json, yaml, etc @@ -45,11 +45,6 @@ func NewNodeTree(v interface{}, source model.Source) (Node, error) { return node, err } -// NewLeafTree creates a new leaf node -func NewLeafNode(v interface{}, source model.Source) (Node, error) { - return newLeafNodeImpl(v, source), nil -} - func makeChildNodeTrees(input map[string]interface{}, source model.Source) (map[string]Node, error) { children := make(map[string]Node) for k, v := range input { diff --git a/pkg/config/nodetreemodel/read_config_file.go b/pkg/config/nodetreemodel/read_config_file.go index 1d6a4e22cff9e..55c7db391b835 100644 --- a/pkg/config/nodetreemodel/read_config_file.go +++ b/pkg/config/nodetreemodel/read_config_file.go @@ -151,7 +151,7 @@ func loadYamlInto(defaults InnerNode, dest InnerNode, data map[string]interface{ // Both default and dest have a child but they conflict in type. This should never happen. warnings = append(warnings, "invalid tree: default and dest tree don't have the same layout") } else { - dest.InsertChildNode(key, newLeafNodeImpl(value, model.SourceFile)) + dest.InsertChildNode(key, newLeafNode(value, model.SourceFile)) } continue } diff --git a/pkg/config/nodetreemodel/reflection_node.go b/pkg/config/nodetreemodel/reflection_node.go index 9fab0ae3f02e7..9c70aa47c1d31 100644 --- a/pkg/config/nodetreemodel/reflection_node.go +++ b/pkg/config/nodetreemodel/reflection_node.go @@ -36,7 +36,7 @@ func asReflectionNode(v interface{}) (Node, error) { item := rv.Index(i).Interface() elems = append(elems, item) } - return NewLeafNode(elems, model.SourceUnknown) + return newLeafNode(elems, model.SourceUnknown), nil } else if rv.Kind() == reflect.Map { res := make(map[string]interface{}, rv.Len()) mapkeys := rv.MapKeys() @@ -49,7 +49,7 @@ func asReflectionNode(v interface{}) (Node, error) { } res[kstr] = rv.MapIndex(mk).Interface() } - return NewLeafNode(res, model.SourceUnknown) + return newLeafNode(res, model.SourceUnknown), nil } return nil, errUnknownConversion }