[k8s] improve kubernetes_secrets provider secret logging (#6841) (#7006)

* feat: improve kubernetes_secrets provider secret logging

* feat: improve readability by replacing Info with Infof

* fix: categorise properly the type of change

* feat: more logging readability improvements

* feat: improve readability by replacing "%s" with %q

(cherry picked from commit 0ae5d35)

Co-authored-by: Panos Koutsovasilis <panos.koutsovasilis@elastic.co>

Author: mergify[bot]

changelog/fragments/1739437836-Improve-kubernetes_secrets-provider-secret-logging.yaml

@@ -0,0 +1,32 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: enhancement
+
+# Change summary; a 80ish characters long description of the change.
+summary: Improve kubernetes_secrets provider secret logging
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+#description:
+
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+component: "elastic-agent"
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+pr: https://github.com/elastic/elastic-agent/pull/6841
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+issue: https://github.com/elastic/elastic-agent/issues/6187

internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go

@@ -136,7 +136,7 @@ func (p *contextProviderK8SSecrets) Fetch(key string) (string, bool) {
 		return "", false
 	}
 	if len(tokens) != 4 {
-		p.logger.Warn("Invalid secret key format: ", key, ". Secrets should be of the format kubernetes_secrets.namespace.secret_name.value")
+		p.logger.Warnf(`Invalid secret key format: %q. Secrets should be of the format kubernetes_secrets.namespace.secret_name.value`, key)
 		return "", false
 	}
 
@@ -151,7 +151,8 @@ func (p *contextProviderK8SSecrets) Fetch(key string) (string, bool) {
 
 	if p.config.DisableCache {
 		// cache disabled - fetch secret from the API
-		return p.fetchFromAPI(ctx, secretName, secretNamespace, secretKey)
+		val, _, ok := p.fetchFromAPI(ctx, secretName, secretNamespace, secretKey)
+		return val, ok
 	}
 
 	// cache enabled
@@ -162,7 +163,7 @@ func (p *contextProviderK8SSecrets) Fetch(key string) (string, bool) {
 	}
 
 	// cache miss - fetch secret from the API
-	apiSecretValue, apiExists := p.fetchFromAPI(ctx, secretName, secretNamespace, secretKey)
+	apiSecretValue, apiSecretResourceVersion, apiExists := p.fetchFromAPI(ctx, secretName, secretNamespace, secretKey)
 	now := time.Now()
 	sd = secret{
 		name:         secretName,
@@ -175,11 +176,13 @@ func (p *contextProviderK8SSecrets) Fetch(key string) (string, bool) {
 	p.store.AddConditionally(key, sd, true, func(existing secret, exists bool) bool {
 		if !exists {
 			// no existing secret in the cache thus add it
+			p.logger.Infof(`Fetch: %q inserted. Resource Version of secret: %q`, key, apiSecretResourceVersion)
 			return true
 		}
 		if existing.value != apiSecretValue && !existing.apiFetchTime.After(now) {
 			// there is an existing secret in the cache but its value has changed since the last time
 			// it was fetched from the API thus update it
+			p.logger.Infof(`Fetch: %q updated. Resource Version of secret: %q`, key, apiSecretResourceVersion)
 			return true
 		}
 		// there is an existing secret in the cache, and it points already to the latest value
@@ -199,10 +202,13 @@ func (p *contextProviderK8SSecrets) refreshCache(ctx context.Context, comm corec
 		case <-ctx.Done():
 			return
 		case <-timer.C:
+			p.logger.Info("Cache: refresh started")
 			hasUpdate := p.updateSecrets(ctx)
 			if hasUpdate {
-				p.logger.Info("Secrets cache was updated, the agent will be notified.")
+				p.logger.Info("Cache: refresh ended with updates, agent will be notified")
 				comm.Signal()
+			} else {
+				p.logger.Info("Cache: refresh ended without updates")
 			}
 			timer.Reset(p.config.RefreshInterval)
 		}
@@ -220,11 +226,12 @@ func (p *contextProviderK8SSecrets) updateSecrets(ctx context.Context) bool {
 		sd, exists := p.store.Get(key, false)
 		if !exists {
 			// this item has expired thus mark that the cache has updates and continue
+			p.logger.Infof(`Cache: %q expired`, key)
 			hasUpdates = true
 			continue
 		}
 
-		apiSecretValue, apiExists := p.fetchFromAPI(ctx, sd.name, sd.namespace, sd.key)
+		apiSecretValue, apiResourceVersion, apiExists := p.fetchFromAPI(ctx, sd.name, sd.namespace, sd.key)
 		now := time.Now()
 		sd = secret{
 			name:         sd.name,
@@ -247,6 +254,7 @@ func (p *contextProviderK8SSecrets) updateSecrets(ctx context.Context) bool {
 				// the secret value has changed and the above fetchFromAPI is more recent thus
 				// add it and mark that the cache has updates
 				hasUpdates = true
+				p.logger.Infof(`Cache: %q updated. Resource Version of secret: %q`, key, apiResourceVersion)
 				return true
 			}
 			// the secret value has not changed
@@ -258,30 +266,32 @@ func (p *contextProviderK8SSecrets) updateSecrets(ctx context.Context) bool {
 }
 
 // fetchFromAPI fetches the secret value from the API
-func (p *contextProviderK8SSecrets) fetchFromAPI(ctx context.Context, secretName string, secretNamespace string, secretKey string) (string, bool) {
+func (p *contextProviderK8SSecrets) fetchFromAPI(ctx context.Context, secretName string, secretNamespace string, secretKey string) (string, string, bool) {
 	ctx, cancel := context.WithTimeout(ctx, p.config.RequestTimeout)
 	defer cancel()
 
 	p.clientMtx.RLock()
 	if p.client == nil {
 		// k8s client is nil most probably due to an error at p.Run
 		p.clientMtx.RUnlock()
-		return "", false
+		p.logger.Warnf(`Could not retrieve secret %q at namespace %q because k8s client is nil`, secretName, secretNamespace)
+		return "", "", false
 	}
 	c := p.client
 	p.clientMtx.RUnlock()
 
 	si := c.CoreV1().Secrets(secretNamespace)
 	secret, err := si.Get(ctx, secretName, metav1.GetOptions{})
 	if err != nil {
-		p.logger.Warn("Could not retrieve secret ", secretName, " at namespace ", secretNamespace, ": ", err.Error())
-		return "", false
+		p.logger.Warnf(`Could not retrieve secret %q at namespace %q: %s`, secretName, secretNamespace, err.Error())
+		return "", "", false
 	}
 
-	if _, ok := secret.Data[secretKey]; !ok {
-		p.logger.Warn("Could not retrieve value of key ", secretKey, " for secret ", secretName, " at namespace ", secretNamespace)
-		return "", false
+	secretData, ok := secret.Data[secretKey]
+	if !ok {
+		p.logger.Warnf(`Could not retrieve value of key %q for secret %q at namespace %q because it does not exist`, secretKey, secretName, secretNamespace)
+		return "", "", false
 	}
 
-	return string(secret.Data[secretKey]), true
+	return string(secretData), secret.GetResourceVersion(), true
 }

internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets_test.go

@@ -12,6 +12,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
@@ -936,6 +938,69 @@ func Test_Config(t *testing.T) {
 	}
 }
 
+func Test_FetchFromAPI(t *testing.T) {
+	for _, tc := range []struct {
+		name                    string
+		k8sClient               k8sclient.Interface
+		secretName              string
+		secretNamespace         string
+		secretKey               string
+		expectedValue           string
+		expectedResourceVersion string
+		expectedOK              bool
+	}{
+		{
+			name:      "k8s client is nil",
+			k8sClient: nil,
+		},
+		{
+			name:            "secret not found",
+			k8sClient:       k8sfake.NewClientset(),
+			secretName:      "secret_name",
+			secretNamespace: "secret_namespace",
+			secretKey:       "secret_key",
+		},
+		{
+			name: "key in secret not found",
+			k8sClient: k8sfake.NewClientset(
+				buildK8SSecret("secret_namespace", "secret_name", "secret_key", "secret_value"),
+			),
+			secretName:      "secret_name",
+			secretNamespace: "secret_namespace",
+			secretKey:       "secret_key_not_found",
+		},
+		{
+			name: "key in secret not found",
+			k8sClient: k8sfake.NewClientset(
+				buildK8SSecretWithResourceVersion("secret_namespace", "secret_name", "secret_key", "secret_value", "100000"),
+			),
+			secretName:              "secret_name",
+			secretNamespace:         "secret_namespace",
+			secretKey:               "secret_key",
+			expectedValue:           "secret_value",
+			expectedResourceVersion: "100000",
+			expectedOK:              true,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			provider := &contextProviderK8SSecrets{
+				logger:    logp.NewLogger("test_k8s_secrets"),
+				config:    defaultConfig(),
+				client:    tc.k8sClient,
+				clientMtx: sync.RWMutex{},
+			}
+
+			val, resourceVersion, ok := provider.fetchFromAPI(ctx, tc.secretName, tc.secretNamespace, tc.secretKey)
+			assert.Equal(t, tc.expectedValue, val)
+			assert.Equal(t, tc.expectedOK, ok)
+			assert.Equal(t, tc.expectedResourceVersion, resourceVersion)
+		})
+	}
+}
+
 type secretTestDataBuilder struct {
 	namespace string
 	name      string
@@ -977,14 +1042,19 @@ func buildCacheEntryKey(e *cacheEntry) string {
 }
 
 func buildK8SSecret(namespace string, name string, key string, value string) *v1.Secret {
+	return buildK8SSecretWithResourceVersion(namespace, name, key, value, "1")
+}
+
+func buildK8SSecretWithResourceVersion(namespace string, name string, key string, value string, resourceVersion string) *v1.Secret {
 	return &v1.Secret{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Secret",
 			APIVersion: "apps/v1beta1",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      name,
-			Namespace: namespace,
+			Name:            name,
+			Namespace:       namespace,
+			ResourceVersion: resourceVersion,
 		},
 		Data: map[string][]byte{
 			key: []byte(value),