Update logging around configsync and fix false notification when the …

…configuration was not updated (#32243)

comp/core/configsync/configsyncimpl/sync.go

@@ -9,7 +9,6 @@ import (
 	"context"
 	"encoding/json"
 	"net/http"
-	"reflect"
 	"strconv"
 	"time"
 
@@ -43,7 +42,7 @@ func (cs *configSync) updater() error {
 			valueMap, ok := value.(map[string]string)
 			if !ok {
 				// this would be unexpected - but deal with it
-				updateConfig(cs.Config, key, value)
+				cs.Config.Set(key, value, pkgconfigmodel.SourceLocalConfigProcess)
 				continue
 			}
 
@@ -56,11 +55,10 @@ func (cs *configSync) updater() error {
 						typedValues[cfgkey] = cfgval
 					}
 				}
-				updateConfig(cs.Config, key, typedValues)
+				cs.Config.Set(key, typedValues, pkgconfigmodel.SourceLocalConfigProcess)
 			}
-
 		} else {
-			updateConfig(cs.Config, key, value)
+			cs.Config.Set(key, value, pkgconfigmodel.SourceLocalConfigProcess)
 		}
 	}
 	return nil
@@ -74,7 +72,6 @@ func (cs *configSync) runWithInterval(refreshInterval time.Duration) {
 }
 
 func (cs *configSync) runWithChan(ch <-chan time.Time) {
-
 	cs.Log.Infof("Starting to sync config with core agent at %s", cs.url)
 
 	for {
@@ -106,11 +103,3 @@ func fetchConfig(ctx context.Context, client *http.Client, authtoken, url string
 
 	return config, nil
 }
-
-func updateConfig(cfg pkgconfigmodel.ReaderWriter, key string, value interface{}) bool {
-	// check if the value changed to only log if it effectively changed the value
-	oldvalue := cfg.Get(key)
-	cfg.Set(key, value, pkgconfigmodel.SourceLocalConfigProcess)
-
-	return !reflect.DeepEqual(oldvalue, cfg.Get(key))
-}

comp/core/configsync/configsyncimpl/sync_test.go

@@ -13,8 +13,11 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/DataDog/datadog-agent/comp/api/authtoken"
+	"github.com/DataDog/datadog-agent/comp/api/authtoken/fetchonlyimpl"
+	logmock "github.com/DataDog/datadog-agent/comp/core/log/mock"
 	configmock "github.com/DataDog/datadog-agent/pkg/config/mock"
-	pkgconfigmodel "github.com/DataDog/datadog-agent/pkg/config/model"
+	"github.com/DataDog/datadog-agent/pkg/config/model"
 )
 
 func TestFetchConfig(t *testing.T) {
@@ -63,12 +66,33 @@ func TestFetchConfig(t *testing.T) {
 	})
 }
 
-func TestUpdateConfig(t *testing.T) {
-	cfg := configmock.New(t)
-	cfg.Set("key1", "value1", pkgconfigmodel.SourceFile)
-	cfg.Set("key3", "set-with-cli", pkgconfigmodel.SourceCLI)
+func TestUpdater(t *testing.T) {
+	callbackCalled := 0
+	handler := func(w http.ResponseWriter, _ *http.Request) {
+		callbackCalled++
+		w.Write([]byte(`{"key1": "value1"}`))
+	}
+	_, client, url := makeServer(t, handler)
 
-	assert.False(t, updateConfig(cfg, "key1", "value1"))
-	assert.True(t, updateConfig(cfg, "key2", "value2"))
-	assert.False(t, updateConfig(cfg, "key3", "value3"))
+	cfg := configmock.New(t)
+	cfg.Set("key1", "base_value", model.SourceDefault)
+
+	cs := configSync{
+		Config:    cfg,
+		Log:       logmock.New(t),
+		Authtoken: authtoken.Component(&fetchonlyimpl.MockFetchOnly{}),
+		url:       url,
+		client:    client,
+		ctx:       context.Background(),
+	}
+
+	cs.updater()
+	assert.Equal(t, "value1", cfg.Get("key1"))
+	assert.Equal(t, 1, callbackCalled)
+
+	cfg.Set("key1", "cli_value", model.SourceCLI)
+
+	cs.updater()
+	assert.Equal(t, "cli_value", cfg.Get("key1"))
+	assert.Equal(t, 2, callbackCalled)
 }

comp/forwarder/defaultforwarder/default_forwarder.go

@@ -27,6 +27,7 @@ import (
 	"github.com/DataDog/datadog-agent/pkg/config/utils"
 	"github.com/DataDog/datadog-agent/pkg/telemetry"
 	"github.com/DataDog/datadog-agent/pkg/util/filesystem"
+	"github.com/DataDog/datadog-agent/pkg/util/scrubber"
 	"github.com/DataDog/datadog-agent/pkg/version"
 )
 
@@ -360,6 +361,7 @@ func NewDefaultForwarder(config config.Component, log log.Component, options *Op
 		oldAPIKey, ok1 := oldValue.(string)
 		newAPIKey, ok2 := newValue.(string)
 		if ok1 && ok2 {
+			log.Infof("Updating API key: %s -> %s", scrubber.HideKeyExceptLastFiveChars(oldAPIKey), scrubber.HideKeyExceptLastFiveChars(newAPIKey))
 			for _, dr := range f.domainResolvers {
 				dr.UpdateAPIKey(oldAPIKey, newAPIKey)
 			}

comp/forwarder/defaultforwarder/forwarder.go

@@ -7,6 +7,7 @@ package defaultforwarder
 
 import (
 	"context"
+	"strings"
 
 	"go.uber.org/fx"
 
@@ -15,6 +16,7 @@ import (
 	"github.com/DataDog/datadog-agent/comp/core/status"
 	"github.com/DataDog/datadog-agent/comp/forwarder/defaultforwarder/resolver"
 	"github.com/DataDog/datadog-agent/pkg/config/utils"
+	"github.com/DataDog/datadog-agent/pkg/util/scrubber"
 )
 
 type dependencies struct {
@@ -62,6 +64,14 @@ func createOptions(params Params, config config.Component, log log.Component) *O
 	}
 	options.SetEnabledFeatures(params.features)
 
+	log.Infof("starting forwarder with %d endpoints", len(options.DomainResolvers))
+	for _, resolver := range options.DomainResolvers {
+		scrubbedKeys := []string{}
+		for _, k := range resolver.GetAPIKeys() {
+			scrubbedKeys = append(scrubbedKeys, scrubber.HideKeyExceptLastFiveChars(k))
+		}
+		log.Infof("domain '%s' has %d keys: %s", resolver.GetBaseDomain(), len(scrubbedKeys), strings.Join(scrubbedKeys, ", "))
+	}
 	return options
 }
 

pkg/config/model/viper.go

@@ -14,6 +14,7 @@ import (
 	"path"
 	"path/filepath"
 	"reflect"
+	"runtime"
 	"slices"
 	"strconv"
 	"strings"
@@ -149,6 +150,12 @@ func (c *safeConfig) OnUpdate(callback NotificationReceiver) {
 	c.notificationReceivers = append(c.notificationReceivers, callback)
 }
 
+func getCallerLocation(nbStack int) string {
+	_, file, line, _ := runtime.Caller(nbStack + 1)
+	fileParts := strings.Split(file, "DataDog/datadog-agent/")
+	return fmt.Sprintf("%s:%d", fileParts[len(fileParts)-1], line)
+}
+
 // Set wraps Viper for concurrent access
 func (c *safeConfig) Set(key string, newValue interface{}, source Source) {
 	if source == SourceDefault {
@@ -159,18 +166,37 @@ func (c *safeConfig) Set(key string, newValue interface{}, source Source) {
 	// modify the config then release the lock to avoid deadlocks while notifying
 	var receivers []NotificationReceiver
 	c.Lock()
-	previousValue := c.Viper.Get(key)
-	c.configSources[source].Set(key, newValue)
-	c.mergeViperInstances(key)
-	if !reflect.DeepEqual(previousValue, newValue) {
+
+	oldValue := c.Viper.Get(key)
+
+	// First we check if the layer changed
+	previousValueFromLayer := c.configSources[source].Get(key)
+	if !reflect.DeepEqual(previousValueFromLayer, newValue) {
+		c.configSources[source].Set(key, newValue)
+		c.mergeViperInstances(key)
+	} else {
+		// nothing changed:w
+		log.Debugf("Updating setting '%s' for source '%s' with the same value, skipping notification", key, source)
+		c.Unlock()
+		return
+	}
+
+	// We might have updated a layer that is itself overridden by another (ex: updating a setting a the 'file' level
+	// already overridden at the 'cli' level. If it the case we do nothing.
+	latestValue := c.Viper.Get(key)
+	if !reflect.DeepEqual(oldValue, latestValue) {
+		log.Infof("Updating setting '%s' for source '%s' with new value. notifying %d listeners", key, source, len(c.notificationReceivers))
 		// if the value has not changed, do not duplicate the slice so that no callback is called
 		receivers = slices.Clone(c.notificationReceivers)
+	} else {
+		log.Debugf("Updating setting '%s' for source '%s' with the same value, skipping notification", key, source)
 	}
 	c.Unlock()
 
 	// notifying all receiver about the updated setting
 	for _, receiver := range receivers {
-		receiver(key, previousValue, newValue)
+		log.Debugf("notifying %s about configuration change for '%s'", getCallerLocation(1), key)
+		receiver(key, oldValue, latestValue)
 	}
 }
 

pkg/config/model/viper_test.go

@@ -225,13 +225,19 @@ func TestNotificationNoChange(t *testing.T) {
 
 	updatedKeyCB1 := []string{}
 
-	config.OnUpdate(func(key string, _, _ any) { updatedKeyCB1 = append(updatedKeyCB1, key) })
+	config.OnUpdate(func(key string, _, newValue any) { updatedKeyCB1 = append(updatedKeyCB1, key+":"+newValue.(string)) })
 
 	config.Set("foo", "bar", SourceFile)
-	assert.Equal(t, []string{"foo"}, updatedKeyCB1)
+	assert.Equal(t, []string{"foo:bar"}, updatedKeyCB1)
 
 	config.Set("foo", "bar", SourceFile)
-	assert.Equal(t, []string{"foo"}, updatedKeyCB1)
+	assert.Equal(t, []string{"foo:bar"}, updatedKeyCB1)
+
+	config.Set("foo", "baz", SourceAgentRuntime)
+	assert.Equal(t, []string{"foo:bar", "foo:baz"}, updatedKeyCB1)
+
+	config.Set("foo", "bar2", SourceFile)
+	assert.Equal(t, []string{"foo:bar", "foo:baz"}, updatedKeyCB1)
 }
 
 func TestCheckKnownKey(t *testing.T) {