From 13a8e18f809f24c86d998e7fc320e6f00b7ab581 Mon Sep 17 00:00:00 2001 From: Ali Asghar <98263017+alliasgher@users.noreply.github.com> Date: Tue, 1 Sep 2026 00:48:06 -0700 Subject: [PATCH 1/5] fix: stop requeueing ApisixConsumer forever after a successful delete (#2850) --- .../controller/apisixconsumer_controller.go | 1 + .../apisixconsumer_controller_test.go | 139 ++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 internal/controller/apisixconsumer_controller_test.go diff --git a/internal/controller/apisixconsumer_controller.go b/internal/controller/apisixconsumer_controller.go index dbcfd024..1fc5f0da 100644 --- a/internal/controller/apisixconsumer_controller.go +++ b/internal/controller/apisixconsumer_controller.go @@ -78,6 +78,7 @@ func (r *ApisixConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Reque r.Log.Error(err, "failed to delete provider", "ApisixConsumer", utils.NamespacedName(ac)) return ctrl.Result{}, err } + r.Log.Info("deleted apisix consumer", "ApisixConsumer", utils.NamespacedName(ac)) return ctrl.Result{}, nil } r.Log.Error(err, "failed to get ApisixConsumer", "request", req.NamespacedName) diff --git a/internal/controller/apisixconsumer_controller_test.go b/internal/controller/apisixconsumer_controller_test.go new file mode 100644 index 00000000..7d3bc49c --- /dev/null +++ b/internal/controller/apisixconsumer_controller_test.go @@ -0,0 +1,139 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package controller + +import ( + "context" + "errors" + "net/http" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + apiv2 "github.com/apache/apisix-ingress-controller/api/v2" + "github.com/apache/apisix-ingress-controller/internal/manager/readiness" + "github.com/apache/apisix-ingress-controller/internal/provider" +) + +const testConsumerNamespace = "default" + +// recordingProvider records the objects passed to Delete and can be told to fail. +type recordingProvider struct { + deleted []types.NamespacedName + deleteErr error +} + +func (p *recordingProvider) Register(string, *http.ServeMux) {} + +func (p *recordingProvider) Update(context.Context, *provider.TranslateContext, client.Object) error { + return nil +} + +func (p *recordingProvider) Delete(_ context.Context, obj client.Object) error { + p.deleted = append(p.deleted, types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}) + return p.deleteErr +} + +func (p *recordingProvider) Start(context.Context) error { return nil } + +func (p *recordingProvider) NeedLeaderElection() bool { return true } + +func newApisixConsumerReconciler(t *testing.T, cli client.Client, p provider.Provider) *ApisixConsumerReconciler { + t.Helper() + + // A readiness manager with no registered GVKs becomes ready as soon as it is + // started. Readier.Done blocks on the started channel, so it must be started. + readier := readiness.NewReadinessManager(cli, logr.Discard()) + require.NoError(t, readier.Start(context.Background())) + + return &ApisixConsumerReconciler{ + Client: cli, + Scheme: cli.Scheme(), + Log: logr.Discard(), + Provider: p, + Readier: readier, + } +} + +func apisixConsumerScheme(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, apiv2.AddToScheme(scheme)) + return scheme +} + +// A deleted ApisixConsumer must be removed from the provider and then reported +// as reconciled. Returning the NotFound error instead makes controller-runtime +// treat the reconcile as failed and requeue it indefinitely with exponential +// backoff, even though the delete succeeded. +func TestApisixConsumerReconcile_DeletedObjectDoesNotRequeue(t *testing.T) { + cli := fake.NewClientBuilder().WithScheme(apisixConsumerScheme(t)).Build() + p := &recordingProvider{} + r := newApisixConsumerReconciler(t, cli, p) + + key := types.NamespacedName{Namespace: testConsumerNamespace, Name: "gone"} + result, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + assert.Equal(t, []types.NamespacedName{key}, p.deleted) +} + +// A provider failure must still surface, so the fix does not swallow real errors. +func TestApisixConsumerReconcile_DeleteErrorIsReturned(t *testing.T) { + cli := fake.NewClientBuilder().WithScheme(apisixConsumerScheme(t)).Build() + p := &recordingProvider{deleteErr: errors.New("provider unavailable")} + r := newApisixConsumerReconciler(t, cli, p) + + key := types.NamespacedName{Namespace: testConsumerNamespace, Name: "gone"} + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "provider unavailable") +} + +// A Get failure that is not NotFound must be returned, and must not be mistaken +// for a deletion. +func TestApisixConsumerReconcile_NonNotFoundGetErrorIsReturned(t *testing.T) { + cli := fake.NewClientBuilder(). + WithScheme(apisixConsumerScheme(t)). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(context.Context, client.WithWatch, client.ObjectKey, client.Object, ...client.GetOption) error { + return k8serrors.NewInternalError(errors.New("boom")) + }, + }). + Build() + p := &recordingProvider{} + r := newApisixConsumerReconciler(t, cli, p) + + key := types.NamespacedName{Namespace: testConsumerNamespace, Name: "present"} + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + + require.Error(t, err) + assert.True(t, k8serrors.IsInternalError(err), "want an internal error, got %v", err) + assert.Empty(t, p.deleted, "a non-NotFound Get error must not trigger a provider delete") +} From d6a9c8f336b90bb370531646d8b35aa49893fede Mon Sep 17 00:00:00 2001 From: Johannes Engler Date: Thu, 3 Sep 2026 06:07:03 +0200 Subject: [PATCH 2/5] fix(test): keep one recordingProvider stub so internal/controller builds (#2854) --- internal/controller/apisixconsumer_controller_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/controller/apisixconsumer_controller_test.go b/internal/controller/apisixconsumer_controller_test.go index 7d3bc49c..95c0bc0d 100644 --- a/internal/controller/apisixconsumer_controller_test.go +++ b/internal/controller/apisixconsumer_controller_test.go @@ -41,8 +41,10 @@ import ( const testConsumerNamespace = "default" -// recordingProvider records the objects passed to Delete and can be told to fail. +// recordingProvider records the objects passed to Update and Delete, and can be +// told to fail a delete. Shared by the reconciler tests in this package. type recordingProvider struct { + updated int deleted []types.NamespacedName deleteErr error } @@ -50,6 +52,7 @@ type recordingProvider struct { func (p *recordingProvider) Register(string, *http.ServeMux) {} func (p *recordingProvider) Update(context.Context, *provider.TranslateContext, client.Object) error { + p.updated++ return nil } From 121794d485c4e639620a6c9d38c1c7723998476f Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Wed, 9 Sep 2026 12:52:48 +0800 Subject: [PATCH 3/5] test: add recordingUpdater beside recordingProvider The reconciler tests being backported need a status.Updater stub as well. Upstream keeps it in gateway_controller_publishservice_test.go, which this repository has not backported, so it lives beside recordingProvider instead of pulling in an unrelated test file. --- internal/controller/apisixconsumer_controller_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/controller/apisixconsumer_controller_test.go b/internal/controller/apisixconsumer_controller_test.go index 95c0bc0d..bd0533d4 100644 --- a/internal/controller/apisixconsumer_controller_test.go +++ b/internal/controller/apisixconsumer_controller_test.go @@ -35,6 +35,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" apiv2 "github.com/apache/apisix-ingress-controller/api/v2" + "github.com/apache/apisix-ingress-controller/internal/controller/status" "github.com/apache/apisix-ingress-controller/internal/manager/readiness" "github.com/apache/apisix-ingress-controller/internal/provider" ) @@ -65,6 +66,15 @@ func (p *recordingProvider) Start(context.Context) error { return nil } func (p *recordingProvider) NeedLeaderElection() bool { return true } +// recordingUpdater captures the status updates a reconciler would write. Upstream +// keeps it in gateway_controller_publishservice_test.go, which has not been +// backported, so it lives beside recordingProvider here. +type recordingUpdater struct { + updates []status.Update +} + +func (u *recordingUpdater) Update(update status.Update) { u.updates = append(u.updates, update) } + func newApisixConsumerReconciler(t *testing.T, cli client.Client, p provider.Provider) *ApisixConsumerReconciler { t.Helper() From 85a5a8eb4c3df4d0820d16de2be60c602c452c12 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Wed, 9 Sep 2026 13:16:09 +0800 Subject: [PATCH 4/5] test: silence unused on the updater stub this PR does not use The unused linter flags recordingUpdater because nothing in this file constructs a reconciler with an Updater. It is here for the backports in flight that do. Suppress it with the reason spelled out, to be dropped once one of them lands. --- internal/controller/apisixconsumer_controller_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/controller/apisixconsumer_controller_test.go b/internal/controller/apisixconsumer_controller_test.go index bd0533d4..a6ebdbb9 100644 --- a/internal/controller/apisixconsumer_controller_test.go +++ b/internal/controller/apisixconsumer_controller_test.go @@ -68,11 +68,15 @@ func (p *recordingProvider) NeedLeaderElection() bool { return true } // recordingUpdater captures the status updates a reconciler would write. Upstream // keeps it in gateway_controller_publishservice_test.go, which has not been -// backported, so it lives beside recordingProvider here. -type recordingUpdater struct { +// backported, so it lives beside recordingProvider here. Nothing in this file +// needs it; it is here for the reconciler tests being backported alongside, which +// construct reconcilers that do have an Updater. Drop the suppression once one of +// them lands. +type recordingUpdater struct { //nolint:unused updates []status.Update } +//nolint:unused func (u *recordingUpdater) Update(update status.Update) { u.updates = append(u.updates, update) } func newApisixConsumerReconciler(t *testing.T, cli client.Client, p provider.Provider) *ApisixConsumerReconciler { From f7ea9e6bdc6963dc20043a84be0d94849133dc1f Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Wed, 9 Sep 2026 16:38:05 +0800 Subject: [PATCH 5/5] test: reuse the recordingProvider #471 landed on master #471 merged after this branch was cut and brought its own recordingProvider in httproute_controller_test.go, so the cherry-picked copy here was a second declaration in the same package. Git merged both files without conflict; the package simply stopped compiling. Drop the copy and use the one on master. recordingUpdater goes too: nothing in this file constructs a reconciler with an Updater, which is why it needed a nolint in the first place, and the backport that does need it adds it beside the provider stub instead. --- .../apisixconsumer_controller_test.go | 40 +------------------ 1 file changed, 2 insertions(+), 38 deletions(-) diff --git a/internal/controller/apisixconsumer_controller_test.go b/internal/controller/apisixconsumer_controller_test.go index a6ebdbb9..e2c17092 100644 --- a/internal/controller/apisixconsumer_controller_test.go +++ b/internal/controller/apisixconsumer_controller_test.go @@ -20,7 +20,6 @@ package controller import ( "context" "errors" - "net/http" "testing" "github.com/go-logr/logr" @@ -35,49 +34,14 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" apiv2 "github.com/apache/apisix-ingress-controller/api/v2" - "github.com/apache/apisix-ingress-controller/internal/controller/status" "github.com/apache/apisix-ingress-controller/internal/manager/readiness" "github.com/apache/apisix-ingress-controller/internal/provider" ) const testConsumerNamespace = "default" -// recordingProvider records the objects passed to Update and Delete, and can be -// told to fail a delete. Shared by the reconciler tests in this package. -type recordingProvider struct { - updated int - deleted []types.NamespacedName - deleteErr error -} - -func (p *recordingProvider) Register(string, *http.ServeMux) {} - -func (p *recordingProvider) Update(context.Context, *provider.TranslateContext, client.Object) error { - p.updated++ - return nil -} - -func (p *recordingProvider) Delete(_ context.Context, obj client.Object) error { - p.deleted = append(p.deleted, types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}) - return p.deleteErr -} - -func (p *recordingProvider) Start(context.Context) error { return nil } - -func (p *recordingProvider) NeedLeaderElection() bool { return true } - -// recordingUpdater captures the status updates a reconciler would write. Upstream -// keeps it in gateway_controller_publishservice_test.go, which has not been -// backported, so it lives beside recordingProvider here. Nothing in this file -// needs it; it is here for the reconciler tests being backported alongside, which -// construct reconcilers that do have an Updater. Drop the suppression once one of -// them lands. -type recordingUpdater struct { //nolint:unused - updates []status.Update -} - -//nolint:unused -func (u *recordingUpdater) Update(update status.Update) { u.updates = append(u.updates, update) } +// recordingProvider and recordingUpdater are declared in +// httproute_controller_test.go, shared by the reconciler tests in this package. func newApisixConsumerReconciler(t *testing.T, cli client.Client, p provider.Provider) *ApisixConsumerReconciler { t.Helper()