Fix diff around default and known settings (#31658)

pkg/config/model/viper.go

@@ -852,7 +852,7 @@ func (c *safeConfig) GetProxies() *Proxy {
 	if c.Viper.GetBool("fips.enabled") {
 		return nil
 	}
-	if !c.Viper.IsSet("proxy.http") && !c.Viper.IsSet("proxy.https") && !c.Viper.IsSet("proxy.no_proxy") {
+	if c.Viper.GetString("proxy.http") == "" && c.Viper.GetString("proxy.https") == "" && len(c.Viper.GetStringSlice("proxy.no_proxy")) == 0 {
 		return nil
 	}
 	p := &Proxy{

pkg/config/nodetreemodel/config.go

@@ -51,6 +51,18 @@ type ntmConfig struct {
 	// Bellow are all the different configuration layers. Each layers represents a source for our configuration.
 	// They are merge into the 'root' tree following order of importance (see pkg/model/viper.go:sourcesPriority).
 
+	// schema holds all the settings with or without value. Settings are added to the schema through BindEnv and
+	// SetDefault.
+	//
+	// This solved the difference between 'AllKeysLowercased' which returns the configuration schema and
+	// 'AllSettings' which only returns settings with a value.
+	//
+	// A setting register with BindEnv without default might not have a value depending on the environment. Such
+	// settings are part of the schema but won't appear in the configuration (through Get, AllSettings, ...). This
+	// mimic the behavior from Viper. Once we enfore a default value for all settings we will be able to merge
+	// 'schema' and 'defaults' fields.
+	schema InnerNode
+
 	// defaults contains the settings with a default value
 	defaults InnerNode
 	// unknown contains the settings set at runtime from unknown source. This should only evey be used by tests.
@@ -131,11 +143,11 @@ func (c *ntmConfig) OnUpdate(callback model.NotificationReceiver) {
 	c.notificationReceivers = append(c.notificationReceivers, callback)
 }
 
-func (c *ntmConfig) setDefault(key string, value interface{}) {
+func (c *ntmConfig) addToSchema(key string, source model.Source) {
 	parts := splitKey(key)
-	// TODO: Ensure that for default tree, setting nil to a node will not override
-	// an existing value
-	_, _ = c.defaults.SetAt(parts, value, model.SourceDefault)
+	_, _ = c.schema.SetAt(parts, nil, source)
+
+	c.addToKnownKeys(key)
 }
 
 func (c *ntmConfig) getTreeBySource(source model.Source) (InnerNode, error) {
@@ -179,7 +191,7 @@ func (c *ntmConfig) Set(key string, newValue interface{}, source model.Source) {
 	key = strings.ToLower(key)
 
 	c.Lock()
-	previousValue := c.leafAtPath(key).Get()
+	previousValue := c.leafAtPathFromNode(key, c.root).Get()
 
 	parts := splitKey(key)
 
@@ -221,8 +233,12 @@ func (c *ntmConfig) SetDefault(key string, value interface{}) {
 		panic("cannot SetDefault() once the config has been marked as ready for use")
 	}
 	key = strings.ToLower(key)
-	c.knownKeys[key] = struct{}{}
-	c.setDefault(key, value)
+	c.addToSchema(key, model.SourceDefault)
+
+	parts := splitKey(key)
+	// TODO: Ensure that for default tree, setting nil to a node will not override
+	// an existing value
+	_, _ = c.defaults.SetAt(parts, value, model.SourceDefault)
 }
 
 // UnsetForSource unsets a config entry for a given source
@@ -232,16 +248,27 @@ func (c *ntmConfig) UnsetForSource(_key string, _source model.Source) {
 	c.Unlock()
 }
 
-// SetKnown adds a key to the set of known valid config keys
+func (c *ntmConfig) addToKnownKeys(key string) {
+	base := ""
+	keyParts := splitKey(key)
+	for _, part := range keyParts {
+		base = joinKey(base, part)
+		c.knownKeys[base] = struct{}{}
+	}
+}
+
+// SetKnown adds a key to the set of known valid config keys.
+//
+// Important: this doesn't add the key to the schema. The "known keys" are a legacy feature we inherited from our Viper
+// wrapper. Once all settings have a default we'll be able to remove this concept entirely.
 func (c *ntmConfig) SetKnown(key string) {
 	c.Lock()
 	defer c.Unlock()
 	if c.isReady() {
 		panic("cannot SetKnown() once the config has been marked as ready for use")
 	}
 
-	key = strings.ToLower(key)
-	c.knownKeys[key] = struct{}{}
+	c.addToKnownKeys(key)
 }
 
 // IsKnown returns whether a key is known
@@ -273,6 +300,7 @@ func (c *ntmConfig) checkKnownKey(key string) {
 }
 
 func (c *ntmConfig) mergeAllLayers() error {
+	// We intentionally don't merge the schema layer as it hold no values
 	treeList := []InnerNode{
 		c.defaults,
 		c.unknown,
@@ -324,7 +352,7 @@ func (c *ntmConfig) BuildSchema() {
 	if err := c.mergeAllLayers(); err != nil {
 		c.warnings = append(c.warnings, err.Error())
 	}
-	c.allSettings = computeAllSettings(c.defaults, "")
+	c.allSettings = computeAllSettings(c.schema, "")
 }
 
 // Stringify stringifies the config, but only with the test build tag
@@ -409,7 +437,21 @@ func (c *ntmConfig) IsSet(key string) bool {
 	c.RLock()
 	defer c.RUnlock()
 
-	return c.IsKnown(key)
+	if !c.isReady() {
+		log.Errorf("attempt to read key before config is constructed: %s", key)
+		return false
+	}
+
+	pathParts := splitKey(key)
+	var curr Node = c.root
+	for _, part := range pathParts {
+		next, err := curr.GetChild(part)
+		if err != nil {
+			return false
+		}
+		curr = next
+	}
+	return true
 }
 
 // AllKeysLowercased returns all keys lower-cased from the default tree, but not keys that are merely marked as known
@@ -420,16 +462,7 @@ func (c *ntmConfig) AllKeysLowercased() []string {
 	return slices.Clone(c.allSettings)
 }
 
-func (c *ntmConfig) leafAtPath(key string) LeafNode {
-	return c.leafAtPathFromNode(key, c.root)
-}
-
 func (c *ntmConfig) leafAtPathFromNode(key string, curr Node) LeafNode {
-	if !c.isReady() {
-		log.Errorf("attempt to read key before config is constructed: %s", key)
-		return missingLeaf
-	}
-
 	pathParts := splitKey(key)
 	for _, part := range pathParts {
 		next, err := curr.GetChild(part)
@@ -496,8 +529,7 @@ func (c *ntmConfig) BindEnv(key string, envvars ...string) {
 		c.configEnvVars[envvar] = key
 	}
 
-	c.knownKeys[key] = struct{}{}
-	c.setDefault(key, nil)
+	c.addToSchema(key, model.SourceEnvVar)
 }
 
 // SetEnvKeyReplacer binds a replacer function for keys
@@ -706,6 +738,7 @@ func NewConfig(name string, envPrefix string, envKeyReplacer *strings.Replacer)
 		knownKeys:          map[string]struct{}{},
 		allSettings:        []string{},
 		unknownKeys:        map[string]struct{}{},
+		schema:             newInnerNode(nil),
 		defaults:           newInnerNode(nil),
 		file:               newInnerNode(nil),
 		unknown:            newInnerNode(nil),
@@ -716,6 +749,7 @@ func NewConfig(name string, envPrefix string, envKeyReplacer *strings.Replacer)
 		fleetPolicies:      newInnerNode(nil),
 		cli:                newInnerNode(nil),
 		envTransform:       make(map[string]func(string) interface{}),
+		configName:         "datadog",
 	}
 
 	config.SetConfigName(name)

pkg/config/nodetreemodel/config_test.go

@@ -64,13 +64,13 @@ secret_backend_command: ./my_secret_fetcher.sh
 		{
 			description:  "nested setting from env var works",
 			setting:      "network_path.collector.input_chan_size",
-			expectValue:  "23456",
+			expectValue:  23456,
 			expectSource: model.SourceEnvVar,
 		},
 		{
 			description:  "top-level setting from env var works",
 			setting:      "secret_backend_timeout",
-			expectValue:  "60", // TODO: cfg.Get returns string because this is an env var
+			expectValue:  60,
 			expectSource: model.SourceEnvVar,
 		},
 		{
@@ -326,13 +326,21 @@ func TestIsSet(t *testing.T) {
 	cfg := NewConfig("test", "TEST", nil)
 	cfg.SetDefault("a", 0)
 	cfg.SetDefault("b", 0)
+	cfg.SetKnown("c")
 	cfg.BuildSchema()
 
 	cfg.Set("b", 123, model.SourceAgentRuntime)
 
-	assert.True(t, cfg.IsSet("b"))
 	assert.True(t, cfg.IsSet("a"))
+	assert.True(t, cfg.IsSet("b"))
+	assert.False(t, cfg.IsSet("c"))
+
+	assert.True(t, cfg.IsKnown("a"))
+	assert.True(t, cfg.IsKnown("b"))
+	assert.True(t, cfg.IsKnown("c"))
+
 	assert.False(t, cfg.IsSet("unknown"))
+	assert.False(t, cfg.IsKnown("unknown"))
 }
 
 func TestAllKeysLowercased(t *testing.T) {

pkg/config/nodetreemodel/getter.go

@@ -16,6 +16,15 @@ import (
 	"golang.org/x/exp/slices"
 )
 
+func (c *ntmConfig) leafAtPath(key string) LeafNode {
+	if !c.isReady() {
+		log.Errorf("attempt to read key before config is constructed: %s", key)
+		return missingLeaf
+	}
+
+	return c.leafAtPathFromNode(key, c.root)
+}
+
 // GetKnownKeysLowercased returns all the keys that meet at least one of these criteria:
 // 1) have a default, 2) have an environment variable binded or 3) have been SetKnown()
 // Note that it returns the keys lowercased.
@@ -71,16 +80,50 @@ func (c *ntmConfig) GetProxies() *model.Proxy {
 	return c.proxies
 }
 
+func (c *ntmConfig) inferTypeFromDefault(key string, value interface{}) (interface{}, error) {
+	// Viper infer the type from the default value for Get. This reproduce the same behavior.
+	// Once all settings have a default value we could move this logic where we load data into the config rather
+	// than out.
+	defaultNode := c.leafAtPathFromNode(key, c.defaults)
+	if defaultNode != missingLeaf {
+		switch defaultNode.Get().(type) {
+		case bool:
+			return cast.ToBoolE(value)
+		case string:
+			return cast.ToStringE(value)
+		case int32, int16, int8, int:
+			return cast.ToIntE(value)
+		case int64:
+			return cast.ToInt64E(value)
+		case float64, float32:
+			return cast.ToFloat64E(value)
+		case time.Time:
+			return cast.ToTimeE(value)
+		case time.Duration:
+			return cast.ToDurationE(value)
+		case []string:
+			return cast.ToStringSliceE(value)
+		}
+	}
+
+	// NOTE: should only need to deepcopy for `Get`, because it can be an arbitrary value,
+	// and we shouldn't ever return complex types like maps and slices that could be modified
+	// by callers accidentally or on purpose. By copying, the caller may modify the result safetly
+	return deepcopy.Copy(value), nil
+}
+
 // Get returns a copy of the value for the given key
 func (c *ntmConfig) Get(key string) interface{} {
 	c.RLock()
 	defer c.RUnlock()
 	c.checkKnownKey(key)
 	val := c.leafAtPath(key).Get()
-	// NOTE: should only need to deepcopy for `Get`, because it can be an arbitrary value,
-	// and we shouldn't ever return complex types like maps and slices that could be modified
-	// by callers accidentally or on purpose. By copying, the caller may modify the result safetly
-	return deepcopy.Copy(val)
+
+	val, err := c.inferTypeFromDefault(key, val)
+	if err != nil {
+		log.Warnf("failed to get configuration value for key %q: %s", key, err)
+	}
+	return val
 }
 
 // GetAllSources returns all values for a key for each source in sorted from lower to higher priority

pkg/config/nodetreemodel/getter_test.go

@@ -24,7 +24,10 @@ func TestGetKnownKeysLowercased(t *testing.T) {
 	assert.Equal(t,
 		map[string]interface{}{
 			"a":     struct{}{},
+			"b":     struct{}{},
 			"b.c":   struct{}{},
+			"d":     struct{}{},
+			"d.e":   struct{}{},
 			"d.e.f": struct{}{},
 		},
 		cfg.GetKnownKeysLowercased())
@@ -37,8 +40,25 @@ func TestGet(t *testing.T) {
 
 	assert.Equal(t, 1234, cfg.Get("a"))
 
-	cfg.Set("a", "test", model.SourceAgentRuntime)
-	assert.Equal(t, "test", cfg.Get("a"))
+	cfg.Set("a", 9876, model.SourceAgentRuntime)
+	assert.Equal(t, 9876, cfg.Get("a"))
+
+	assert.Equal(t, nil, cfg.Get("does_not_exists"))
+}
+
+func TestGetCastToDefault(t *testing.T) {
+	cfg := NewConfig("test", "", nil)
+	cfg.SetDefault("a", []string{})
+	cfg.BuildSchema()
+
+	// This test that we mimic viper's behavior on Get where we convert the value from the config to the same type
+	// from the default.
+
+	cfg.Set("a", 9876, model.SourceAgentRuntime)
+	assert.Equal(t, []string{"9876"}, cfg.Get("a"))
+
+	cfg.Set("a", "a b c", model.SourceAgentRuntime)
+	assert.Equal(t, []string{"a", "b", "c"}, cfg.Get("a"))
 
 	assert.Equal(t, nil, cfg.Get("does_not_exists"))
 }