From 8995d844a7ce0dd86819537881b09984a5f90e18 Mon Sep 17 00:00:00 2001 From: Balint Molnar Date: Mon, 31 Aug 2026 11:33:04 +0200 Subject: [PATCH] fix(k8ssecret): stop the startup check requiring a key named "token" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit initialValidation looked up secret.Data["token"] while handleEvent read secret.Data[secretRef.Key], so a SecretRef configured with any other key failed the startup check. That was not a spurious warning: the informer replays pre-existing objects as Add events, so by the time the check ran handleEvent had already published a valid token, and credsChan is buffered at 1 — util.SendToChannel discards the oldest entry when full, so publishing ErrMissingData threw the good credential away. The consumer saw only the error and the credential was withdrawn. Nothing republished until the secret was modified, so it never recovered on its own. Replace it with validateSecretExists, which reports only a secret that is absent. That is the single case the handlers cannot cover, since no event fires for an object that was never there. A secret that exists is left entirely to handleEvent, which already publishes either the token or ErrMissingData using the configured key — so there is no longer a second place where "the key" is decided, and nothing overwrites a credential that was just published correctly. Add tests for the package, which had none. They cover the configured key being honoured, a key named "token" not being special, missing key and missing secret, and the delete and filtering paths. --- pkg/k8ssecret/k8ssecret.go | 18 +-- pkg/k8ssecret/k8ssecret_test.go | 214 ++++++++++++++++++++++++++++++++ 2 files changed, 219 insertions(+), 13 deletions(-) create mode 100644 pkg/k8ssecret/k8ssecret_test.go diff --git a/pkg/k8ssecret/k8ssecret.go b/pkg/k8ssecret/k8ssecret.go index a24a853..2fbd7e7 100644 --- a/pkg/k8ssecret/k8ssecret.go +++ b/pkg/k8ssecret/k8ssecret.go @@ -159,8 +159,7 @@ func (cp *credentialsProvider) getCredentials(ctx context.Context, cfg *credenti return credsChan, errors.WithStack(ErrInformerSync) } - // do initial check of the specified secret - if err := cp.initialValidation(ctx, cfg.secretRef); err != nil { + if err := cp.validateSecretExists(ctx, cfg.secretRef); err != nil { cp.publishFetch(ctx, tracer, configAttrs, credsChan, nil, err) } @@ -177,20 +176,13 @@ func (cp *credentialsProvider) getCredentials(ctx context.Context, cfg *credenti return credsChan, nil } -func (cp *credentialsProvider) initialValidation(ctx context.Context, secretRef SecretRef) error { +func (cp *credentialsProvider) validateSecretExists(ctx context.Context, secretRef SecretRef) error { secret := &corev1.Secret{} - if err := cp.cache.Get(ctx, client.ObjectKey{ + + return cp.cache.Get(ctx, client.ObjectKey{ Name: secretRef.Name, Namespace: secretRef.Namespace, - }, secret); err != nil { - return err - } - - if _, ok := secret.Data["token"]; !ok { - return ErrMissingData - } - - return nil + }, secret) } func (cp *credentialsProvider) publishFetch(ctx context.Context, tracer trace.Tracer, configAttrs []attribute.KeyValue, credsChan chan Credential, tok *credential.Token, fetchErr error) { diff --git a/pkg/k8ssecret/k8ssecret_test.go b/pkg/k8ssecret/k8ssecret_test.go new file mode 100644 index 0000000..06d6ef6 --- /dev/null +++ b/pkg/k8ssecret/k8ssecret_test.go @@ -0,0 +1,214 @@ +// Copyright (c) 2026 Riptides Labs, Inc. +// SPDX-License-Identifier: MIT + +package k8ssecret //nolint: testpackage // Tests use unexported functions + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/trace/noop" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + + "go.riptides.io/tokenex/pkg/credential" +) + +var errNotASecret = errors.New("not a secret") + +// stubCache satisfies cache.Cache by embedding it; only Get is implemented, which is +// all validateSecretExists uses. Any other call would panic, which is the intent. +type stubCache struct { + cache.Cache + + secret *corev1.Secret +} + +func (s stubCache) Get(_ context.Context, key client.ObjectKey, obj client.Object, _ ...client.GetOption) error { + if s.secret == nil || key.Name != s.secret.GetName() || key.Namespace != s.secret.GetNamespace() { + return apierrors.NewNotFound(schema.GroupResource{Resource: "secrets"}, key.Name) + } + + out, ok := obj.(*corev1.Secret) + if !ok { + return apierrors.NewInternalError(errNotASecret) + } + + *out = *s.secret + + return nil +} + +func testSecret(data map[string][]byte) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "creds", Namespace: "riptides-system"}, + Data: data, + } +} + +func testSecretRef() SecretRef { + return SecretRef{Name: "creds", Namespace: "riptides-system", Key: "apiKey"} +} + +func TestValidateSecretExists(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + secret *corev1.Secret + ref SecretRef + wantErr bool + }{ + { + name: "secret with a non-token key is fine", + secret: testSecret(map[string][]byte{"apiKey": []byte("sk_live_x")}), + }, + { + name: "secret keyed token is fine", + secret: testSecret(map[string][]byte{"token": []byte("sk_live_x")}), + }, + { + name: "empty secret is fine; the key is handleEvent's business", + secret: testSecret(nil), + }, + { + name: "missing secret is reported", + secret: nil, + wantErr: true, + }, + { + name: "looks the secret up by the configured name", + secret: testSecret(map[string][]byte{"apiKey": []byte("sk_live_x")}), + ref: SecretRef{Name: "other", Namespace: "riptides-system", Key: "apiKey"}, + wantErr: true, + }, + { + name: "looks the secret up by the configured namespace", + secret: testSecret(map[string][]byte{"apiKey": []byte("sk_live_x")}), + ref: SecretRef{Name: "creds", Namespace: "other", Key: "apiKey"}, + wantErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ref := tc.ref + if ref == (SecretRef{}) { + ref = testSecretRef() + } + + cp := &credentialsProvider{cache: stubCache{secret: tc.secret}} + + err := cp.validateSecretExists(context.Background(), ref) + + if tc.wantErr { + require.Error(t, err) + require.True(t, apierrors.IsNotFound(err)) + + return + } + + require.NoError(t, err) + }) + } +} + +func TestHandleEvent(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + secretRef SecretRef + obj any + del bool + wantEvent credential.EventType + wantToken string + wantErr error + wantNone bool + }{ + { + name: "reads the configured key", + secretRef: testSecretRef(), + obj: testSecret(map[string][]byte{"apiKey": []byte("sk_live_x")}), + wantEvent: credential.UpdateEventType, + wantToken: "sk_live_x", + }, + { + name: "a key named token is not special", + secretRef: testSecretRef(), + obj: testSecret(map[string][]byte{"token": []byte("wrong")}), + wantEvent: credential.UpdateEventType, + wantErr: ErrMissingData, + }, + { + name: "missing key reports missing data", + secretRef: testSecretRef(), + obj: testSecret(map[string][]byte{"other": []byte("x")}), + wantEvent: credential.UpdateEventType, + wantErr: ErrMissingData, + }, + { + name: "delete publishes a remove", + secretRef: testSecretRef(), + obj: testSecret(map[string][]byte{"apiKey": []byte("sk_live_x")}), + del: true, + wantEvent: credential.RemoveEventType, + }, + { + name: "a different secret is ignored", + secretRef: SecretRef{Name: "other", Namespace: "riptides-system", Key: "apiKey"}, + obj: testSecret(map[string][]byte{"apiKey": []byte("sk_live_x")}), + wantNone: true, + }, + { + name: "a non-secret object is ignored", + secretRef: testSecretRef(), + obj: &corev1.ConfigMap{}, + wantNone: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + cp := &credentialsProvider{} + credsChan := make(chan Credential, 1) + + cp.handleEvent(context.Background(), noop.NewTracerProvider().Tracer(""), nil, + credsChan, tc.secretRef, tc.obj, tc.del) + + if tc.wantNone { + require.Empty(t, credsChan, "expected nothing to be published") + + return + } + + require.Len(t, credsChan, 1) + got := <-credsChan + require.Equal(t, tc.wantEvent, got.Event) + + if tc.wantErr != nil { + require.ErrorIs(t, got.Err, tc.wantErr) + + return + } + + require.NoError(t, got.Err) + + if tc.wantToken != "" { + tok, ok := got.Credential.(*credential.Token) + require.True(t, ok, "expected a *credential.Token, got %T", got.Credential) + require.Equal(t, tc.wantToken, tok.Token) + } + }) + } +}