Skip to content

Commit

Permalink
[k8s] improve kubernetes_secrets provider secret logging (#6841)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@pkoutsovasilis
pkoutsovasilis authored Feb 25, 2025
1 parent 106a1a2 commit 0ae5d35
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 15 deletions.
32 changes: 32 additions & 0 deletions changelog/fragments/1739437836-Improve-kubernetes_secrets-provider-secret-logging.yaml
View file
Original file line number Diff line number Diff line change
@@ -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
36 changes: 23 additions & 13 deletions internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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)
}
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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
}
74 changes: 72 additions & 2 deletions internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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),
Expand Down

0 comments on commit 0ae5d35

Please # to comment.