From 9aea1bfa1e0d9359a3ccf90d0eb1754deb9d52f0 Mon Sep 17 00:00:00 2001 From: Panos Koutsovasilis Date: Tue, 25 Feb 2025 14:03:51 +0200 Subject: [PATCH] [k8s] improve kubernetes_secrets provider secret logging (#6841) * 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 0ae5d358b240258887bdd3dbbe4e8965773ff48b) --- ...netes_secrets-provider-secret-logging.yaml | 32 ++++++++ .../kubernetessecrets/kubernetes_secrets.go | 36 +++++---- .../kubernetes_secrets_test.go | 74 ++++++++++++++++++- 3 files changed, 127 insertions(+), 15 deletions(-) create mode 100644 changelog/fragments/1739437836-Improve-kubernetes_secrets-provider-secret-logging.yaml diff --git a/changelog/fragments/1739437836-Improve-kubernetes_secrets-provider-secret-logging.yaml b/changelog/fragments/1739437836-Improve-kubernetes_secrets-provider-secret-logging.yaml new file mode 100644 index 00000000000..5a364cbcac9 --- /dev/null +++ b/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 diff --git a/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go b/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go index 2db9d65813e..572d89de620 100644 --- a/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go +++ b/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,7 +266,7 @@ 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() @@ -266,7 +274,8 @@ func (p *contextProviderK8SSecrets) fetchFromAPI(ctx context.Context, secretName 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() @@ -274,14 +283,15 @@ func (p *contextProviderK8SSecrets) fetchFromAPI(ctx context.Context, secretName 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 } diff --git a/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets_test.go b/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets_test.go index 262714d425f..0283693fd77 100644 --- a/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets_test.go +++ b/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets_test.go @@ -13,6 +13,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" @@ -935,6 +937,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 @@ -976,14 +1041,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),