From fb3467634ae1ed0dc409970bfd0d8a263909a574 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 8 Oct 2026 23:02:49 +0545 Subject: [PATCH] fix: skip duplicate key-auth credentials at reconcile time The Consumer webhook checks key-auth keys only at admission, and skips a credential whose Secret does not exist yet. A Secret created after its Consumer therefore published a key another Consumer on the same Gateway already used. The reconciler now drops a key-auth credential whose key an older Consumer on the same Gateway publishes, and reports it on the status. Keys are found through hashed field indexes on Consumers and Secrets, and Consumer and Secret changes requeue Consumers sharing a key so a skipped credential returns once the key is free. Signed-off-by: Abhishek Choudhary --- internal/controller/consumer_controller.go | 189 +++++++++++++++++- .../controller/consumer_controller_test.go | 141 ++++++++++++- internal/controller/indexer/indexer.go | 62 ++++++ 3 files changed, 389 insertions(+), 3 deletions(-) diff --git a/internal/controller/consumer_controller.go b/internal/controller/consumer_controller.go index 230255a2..a50eb50a 100644 --- a/internal/controller/consumer_controller.go +++ b/internal/controller/consumer_controller.go @@ -20,6 +20,7 @@ package controller import ( "context" "fmt" + "slices" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" @@ -47,6 +48,8 @@ import ( pkgutils "github.com/apache/apisix-ingress-controller/pkg/utils" ) +const keyAuthType = "key-auth" + // ConsumerReconciler reconciles a Gateway object. type ConsumerReconciler struct { //nolint:revive client.Client @@ -103,6 +106,9 @@ func (r *ConsumerReconciler) SetupWithManager(mgr ctrl.Manager) error { }, ), ). + Watches(&v1alpha1.Consumer{}, + handler.EnqueueRequestsFromMapFunc(r.listConsumersSharingKeyAuthKeys), + ). Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.listConsumersForSecret), ). @@ -118,7 +124,7 @@ func (r *ConsumerReconciler) listConsumersForSecret(ctx context.Context, obj cli r.Log.Error(nil, "failed to convert to Secret", "object", obj) return nil } - return ListRequests( + requests := ListRequests( ctx, r.Client, r.Log, @@ -127,6 +133,51 @@ func (r *ConsumerReconciler) listConsumersForSecret(ctx context.Context, obj cli indexer.SecretIndexRef: indexer.GenIndexKey(secret.GetNamespace(), secret.GetName()), }, ) + return append(requests, r.listKeyAuthKeyOwners(ctx, string(secret.Data["key"]))...) +} + +// listConsumersSharingKeyAuthKeys requeues Consumers sharing a key-auth key with +// obj, so a credential skipped as a duplicate returns once the key is free. +func (r *ConsumerReconciler) listConsumersSharingKeyAuthKeys(ctx context.Context, obj client.Object) []reconcile.Request { + consumer, ok := obj.(*v1alpha1.Consumer) + if !ok { + r.Log.Error(nil, "failed to convert to Consumer", "object", obj) + return nil + } + keys := make([]string, 0, len(consumer.Spec.Credentials)) + for _, credential := range consumer.Spec.Credentials { + if credential.Type != keyAuthType { + continue + } + if credential.SecretRef == nil { + keys = append(keys, indexer.InlineKeyAuthKey(credential)) + continue + } + var secret corev1.Secret + if err := r.Get(ctx, credentialSecretNN(consumer, credential), &secret); err != nil { + continue + } + keys = append(keys, string(secret.Data["key"])) + } + return r.listKeyAuthKeyOwners(ctx, keys...) +} + +func (r *ConsumerReconciler) listKeyAuthKeyOwners(ctx context.Context, keys ...string) []reconcile.Request { + var requests []reconcile.Request + for _, key := range keys { + if key == "" { + continue + } + owners, err := r.keyAuthKeyOwners(ctx, key) + if err != nil { + r.Log.Error(err, "failed to list consumers sharing a key-auth key") + continue + } + for i := range owners { + requests = append(requests, reconcile.Request{NamespacedName: utils.NamespacedName(&owners[i])}) + } + } + return requests } func (r *ConsumerReconciler) listConsumersForGateway(ctx context.Context, obj client.Object) []reconcile.Request { @@ -241,7 +292,15 @@ func (r *ConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c statusErr = err } - if err := r.Provider.Update(ctx, tctx, consumer); err != nil { + published, skipped, err := r.skipDuplicateKeyAuthCredentials(ctx, tctx, consumer) + if err != nil { + return ctrl.Result{}, err + } + if skipped > 0 { + statusErr = fmt.Errorf("%d key-auth credential(s) skipped: key already used by another Consumer", skipped) + } + + if err := r.Provider.Update(ctx, tctx, published); err != nil { r.Log.Error(err, "failed to update consumer", "consumer", utils.NamespacedName(consumer)) statusErr = err } @@ -293,6 +352,132 @@ func (r *ConsumerReconciler) processSpec(ctx context.Context, tctx *provider.Tra return loadPluginSecrets(ctx, r.Client, tctx, consumer.GetNamespace(), consumer.Spec.Plugins) } +// skipDuplicateKeyAuthCredentials drops key-auth credentials whose key an older +// Consumer on the same Gateway already uses. Admission alone misses a duplicate +// that arrives later, e.g. via a Secret created after its Consumer. +func (r *ConsumerReconciler) skipDuplicateKeyAuthCredentials(ctx context.Context, tctx *provider.TranslateContext, consumer *v1alpha1.Consumer) (*v1alpha1.Consumer, int, error) { + kept := make([]v1alpha1.Credential, 0, len(consumer.Spec.Credentials)) + for _, credential := range consumer.Spec.Credentials { + key := publishedKeyAuthKey(tctx, consumer, credential) + if key != "" { + taken, err := r.keyAuthKeyTaken(ctx, consumer, key) + if err != nil { + return nil, 0, err + } + if taken { + continue + } + } + kept = append(kept, credential) + } + skipped := len(consumer.Spec.Credentials) - len(kept) + if skipped == 0 { + return consumer, 0, nil + } + published := consumer.DeepCopy() + published.Spec.Credentials = kept + return published, skipped, nil +} + +// keyAuthKeyTaken reports whether an older Consumer on the same Gateway uses key. +func (r *ConsumerReconciler) keyAuthKeyTaken(ctx context.Context, consumer *v1alpha1.Consumer, key string) (bool, error) { + owners, err := r.keyAuthKeyOwners(ctx, key) + if err != nil { + return false, err + } + gatewayRef := indexer.ConsumerGatewayRefIndexFunc(consumer) + for i := range owners { + owner := &owners[i] + if owner.Namespace == consumer.Namespace && owner.Name == consumer.Name { + continue + } + if !slices.Equal(indexer.ConsumerGatewayRefIndexFunc(owner), gatewayRef) { + continue + } + if olderConsumer(owner, consumer) { + return true, nil + } + } + return false, nil +} + +// keyAuthKeyOwners lists the Consumers whose key-auth credentials use key. +func (r *ConsumerReconciler) keyAuthKeyOwners(ctx context.Context, key string) ([]v1alpha1.Consumer, error) { + index := client.MatchingFields{indexer.KeyAuthKey: indexer.GenKeyAuthKeyIndex(key)} + + var owners v1alpha1.ConsumerList + if err := r.List(ctx, &owners, index); err != nil { + return nil, err + } + var secrets corev1.SecretList + if err := r.List(ctx, &secrets, index); err != nil { + return nil, err + } + for _, secret := range secrets.Items { + var refs v1alpha1.ConsumerList + if err := r.List(ctx, &refs, client.MatchingFields{ + indexer.SecretIndexRef: indexer.GenIndexKey(secret.Namespace, secret.Name), + }); err != nil { + return nil, err + } + for i := range refs.Items { + uses, err := usesKeyAuthSecret(ctx, r.Client, &refs.Items[i], utils.NamespacedName(&secret)) + if err != nil { + return nil, err + } + if uses { + owners.Items = append(owners.Items, refs.Items[i]) + } + } + } + return owners.Items, nil +} + +// usesKeyAuthSecret reports whether consumer loads secretNN as a key-auth credential. +func usesKeyAuthSecret(ctx context.Context, c client.Client, consumer *v1alpha1.Consumer, secretNN types.NamespacedName) (bool, error) { + for _, credential := range consumer.Spec.Credentials { + if credential.Type != keyAuthType || credential.SecretRef == nil || credentialSecretNN(consumer, credential) != secretNN { + continue + } + if secretNN.Namespace == consumer.Namespace { + return true, nil + } + return CheckConsumerSecretRef(ctx, c, consumer.Namespace, secretNN) + } + return false, nil +} + +// publishedKeyAuthKey returns the key a key-auth credential publishes, or "". +func publishedKeyAuthKey(tctx *provider.TranslateContext, consumer *v1alpha1.Consumer, credential v1alpha1.Credential) string { + if credential.Type != keyAuthType { + return "" + } + if credential.SecretRef == nil { + return indexer.InlineKeyAuthKey(credential) + } + secret := tctx.Secrets[credentialSecretNN(consumer, credential)] + if secret == nil { + return "" + } + return string(secret.Data["key"]) +} + +func credentialSecretNN(consumer *v1alpha1.Consumer, credential v1alpha1.Credential) types.NamespacedName { + ns := consumer.Namespace + if credential.SecretRef.Namespace != nil { + ns = *credential.SecretRef.Namespace + } + return types.NamespacedName{Namespace: ns, Name: credential.SecretRef.Name} +} + +// olderConsumer orders by creation time, then by namespace/name for a stable tie-break. +func olderConsumer(a, b *v1alpha1.Consumer) bool { + if !a.CreationTimestamp.Equal(&b.CreationTimestamp) { + return a.CreationTimestamp.Before(&b.CreationTimestamp) + } + return utils.NamespacedName(a).String() < utils.NamespacedName(b).String() +} + func (r *ConsumerReconciler) updateStatus(consumer *v1alpha1.Consumer, err error) { condition := NewCondition(consumer.Generation, true, "Successfully") if err != nil { diff --git a/internal/controller/consumer_controller_test.go b/internal/controller/consumer_controller_test.go index 92278611..5290f21b 100644 --- a/internal/controller/consumer_controller_test.go +++ b/internal/controller/consumer_controller_test.go @@ -20,18 +20,22 @@ package controller import ( "context" "testing" + "time" "github.com/go-logr/logr" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/apache/apisix-ingress-controller/api/v1alpha1" + "github.com/apache/apisix-ingress-controller/internal/controller/indexer" "github.com/apache/apisix-ingress-controller/internal/provider" ) @@ -48,7 +52,11 @@ func buildConsumerReconciler(t *testing.T, objs ...runtime.Object) *ConsumerReco require.NoError(t, v1alpha1.AddToScheme(scheme)) require.NoError(t, gatewayv1.Install(scheme)) - cli := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objs...).Build() + cli := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objs...). + WithIndex(&v1alpha1.Consumer{}, indexer.SecretIndexRef, indexer.ConsumerSecretIndexFunc). + WithIndex(&v1alpha1.Consumer{}, indexer.KeyAuthKey, indexer.ConsumerKeyAuthKeyIndexFunc). + WithIndex(&corev1.Secret{}, indexer.KeyAuthKey, indexer.SecretKeyAuthKeyIndexFunc). + Build() return &ConsumerReconciler{Client: cli, Log: logr.Discard()} } @@ -144,3 +152,134 @@ func TestProcessSpec_SameNamespaceSecretRef_Allowed(t *testing.T) { require.NoError(t, err) require.Contains(t, tctx.Secrets, types.NamespacedName{Namespace: consumerNS, Name: "local-secret"}) } + +const sharedKey = "shared-key" + +func keyAuthConsumer(name, gateway string, created time.Time, credentials ...v1alpha1.Credential) *v1alpha1.Consumer { + return &v1alpha1.Consumer{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: consumerNS, + CreationTimestamp: metav1.NewTime(created.Truncate(time.Second)), + }, + Spec: v1alpha1.ConsumerSpec{ + GatewayRef: v1alpha1.GatewayRef{Name: gateway}, + Credentials: credentials, + }, + } +} + +func inlineKeyAuth(name, key string) v1alpha1.Credential { + return v1alpha1.Credential{ + Name: name, + Type: "key-auth", + Config: apiextensionsv1.JSON{Raw: []byte(`{"key":"` + key + `"}`)}, + } +} + +func secretKeyAuth(name, secret string, namespace *string) v1alpha1.Credential { + return v1alpha1.Credential{ + Name: name, + Type: "key-auth", + SecretRef: &v1alpha1.SecretReference{Name: secret, Namespace: namespace}, + } +} + +func keyAuthSecret(name, namespace string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Data: map[string][]byte{"key": []byte(sharedKey)}, + } +} + +func skipDuplicates(t *testing.T, r *ConsumerReconciler, consumer *v1alpha1.Consumer) (*v1alpha1.Consumer, int) { + t.Helper() + tctx := provider.NewDefaultTranslateContext(context.Background()) + require.NoError(t, r.processSpec(context.Background(), tctx, consumer)) + published, skipped, err := r.skipDuplicateKeyAuthCredentials(context.Background(), tctx, consumer) + require.NoError(t, err) + return published, skipped +} + +// A Secret created after its Consumer must not publish a key an older Consumer already uses. +func TestSkipDuplicateKeyAuth_SecretCreatedLater(t *testing.T) { + now := time.Now() + victim := keyAuthConsumer("victim", "gw", now.Add(-time.Hour), inlineKeyAuth("cred", sharedKey)) + attacker := keyAuthConsumer("attacker", "gw", now, + secretKeyAuth("dup", "late-secret", nil), + inlineKeyAuth("own", "attacker-key"), + ) + r := buildConsumerReconciler(t, victim, attacker, keyAuthSecret("late-secret", consumerNS)) + + published, skipped := skipDuplicates(t, r, attacker) + require.Equal(t, 1, skipped) + require.Len(t, published.Spec.Credentials, 1) + require.Equal(t, "own", published.Spec.Credentials[0].Name) + require.Len(t, attacker.Spec.Credentials, 2, "the cached Consumer must stay untouched") +} + +// The older Consumer keeps its key. +func TestSkipDuplicateKeyAuth_OlderConsumerKeepsKey(t *testing.T) { + now := time.Now() + victim := keyAuthConsumer("victim", "gw", now.Add(-time.Hour), inlineKeyAuth("cred", sharedKey)) + attacker := keyAuthConsumer("attacker", "gw", now, secretKeyAuth("dup", "late-secret", nil)) + r := buildConsumerReconciler(t, victim, attacker, keyAuthSecret("late-secret", consumerNS)) + + published, skipped := skipDuplicates(t, r, victim) + require.Zero(t, skipped) + require.Same(t, victim, published) +} + +// Equal creation times fall back to namespace/name, so exactly one side wins. +func TestSkipDuplicateKeyAuth_TieBreak(t *testing.T) { + now := time.Now() + a := keyAuthConsumer("a", "gw", now, inlineKeyAuth("cred", sharedKey)) + b := keyAuthConsumer("b", "gw", now, inlineKeyAuth("cred", sharedKey)) + r := buildConsumerReconciler(t, a, b) + + _, skippedA := skipDuplicates(t, r, a) + _, skippedB := skipDuplicates(t, r, b) + require.Zero(t, skippedA) + require.Equal(t, 1, skippedB) +} + +// Consumers on different Gateways may share a key. +func TestSkipDuplicateKeyAuth_OtherGatewayIgnored(t *testing.T) { + now := time.Now() + other := keyAuthConsumer("other", "other-gw", now.Add(-time.Hour), inlineKeyAuth("cred", sharedKey)) + consumer := keyAuthConsumer("consumer", "gw", now, inlineKeyAuth("cred", sharedKey)) + r := buildConsumerReconciler(t, other, consumer) + + _, skipped := skipDuplicates(t, r, consumer) + require.Zero(t, skipped) +} + +// A cross-namespace Secret that no ReferenceGrant permits is never published, so it holds no key. +func TestSkipDuplicateKeyAuth_UngrantedSecretOwnsNoKey(t *testing.T) { + SetEnableReferenceGrant(true) + defer SetEnableReferenceGrant(false) + + now := time.Now() + target := secretNS + ungranted := keyAuthConsumer("ungranted", "gw", now.Add(-time.Hour), secretKeyAuth("cred", "foreign", &target)) + consumer := keyAuthConsumer("consumer", "gw", now, inlineKeyAuth("cred", sharedKey)) + r := buildConsumerReconciler(t, ungranted, consumer, keyAuthSecret("foreign", secretNS)) + + _, skipped := skipDuplicates(t, r, consumer) + require.Zero(t, skipped) +} + +// Changes that free a key requeue the Consumer that was skipped for it. +func TestKeyAuthKeyOwnersRequeued(t *testing.T) { + now := time.Now() + victim := keyAuthConsumer("victim", "gw", now.Add(-time.Hour), inlineKeyAuth("cred", sharedKey)) + attacker := keyAuthConsumer("attacker", "gw", now, secretKeyAuth("dup", "late-secret", nil)) + secret := keyAuthSecret("late-secret", consumerNS) + r := buildConsumerReconciler(t, victim, attacker, secret) + + attackerReq := reconcile.Request{NamespacedName: types.NamespacedName{Namespace: consumerNS, Name: "attacker"}} + victimReq := reconcile.Request{NamespacedName: types.NamespacedName{Namespace: consumerNS, Name: "victim"}} + + require.Contains(t, r.listConsumersSharingKeyAuthKeys(context.Background(), victim), attackerReq) + require.Contains(t, r.listConsumersForSecret(context.Background(), secret), victimReq) +} diff --git a/internal/controller/indexer/indexer.go b/internal/controller/indexer/indexer.go index 0e3480cf..ab4944d1 100644 --- a/internal/controller/indexer/indexer.go +++ b/internal/controller/indexer/indexer.go @@ -20,6 +20,9 @@ package indexer import ( "cmp" "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" @@ -56,6 +59,7 @@ const ( ApisixUpstreamRef = "apisixUpstreamRef" PluginConfigIndexRef = "pluginConfigRefs" ControllerName = "controllerName" + KeyAuthKey = "keyAuthKey" ) func SetupIndexer(mgr ctrl.Manager) error { @@ -193,6 +197,22 @@ func setupConsumerIndexer(mgr ctrl.Manager) error { ); err != nil { return err } + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), + &v1alpha1.Consumer{}, + KeyAuthKey, + ConsumerKeyAuthKeyIndexFunc, + ); err != nil { + return err + } + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), + &corev1.Secret{}, + KeyAuthKey, + SecretKeyAuthKeyIndexFunc, + ); err != nil { + return err + } return nil } @@ -255,6 +275,48 @@ func ConsumerSecretIndexFunc(rawObj client.Object) []string { return secretKeys } +// GenKeyAuthKeyIndex hashes a key-auth key so the index never holds plaintext. +func GenKeyAuthKeyIndex(key string) string { + sum := sha256.Sum256([]byte(key)) + return hex.EncodeToString(sum[:]) +} + +// InlineKeyAuthKey returns the key of an inline key-auth credential, or "" if none. +func InlineKeyAuthKey(credential v1alpha1.Credential) string { + if credential.Type != "key-auth" || credential.SecretRef != nil || len(credential.Config.Raw) == 0 { + return "" + } + var cfg struct { + Key string `json:"key"` + } + if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil { + return "" + } + return cfg.Key +} + +// ConsumerKeyAuthKeyIndexFunc indexes Consumers by their inline key-auth keys. +func ConsumerKeyAuthKeyIndexFunc(rawObj client.Object) []string { + consumer := rawObj.(*v1alpha1.Consumer) + var keys []string + for _, credential := range consumer.Spec.Credentials { + if key := InlineKeyAuthKey(credential); key != "" { + keys = append(keys, GenKeyAuthKeyIndex(key)) + } + } + return keys +} + +// SecretKeyAuthKeyIndexFunc indexes Secrets by the key-auth key they hold. +func SecretKeyAuthKeyIndexFunc(rawObj client.Object) []string { + secret := rawObj.(*corev1.Secret) + key := secret.Data["key"] + if len(key) == 0 { + return nil + } + return []string{GenKeyAuthKeyIndex(string(key))} +} + func ConsumerGatewayRefIndexFunc(rawObj client.Object) []string { consumer := rawObj.(*v1alpha1.Consumer)