From 490874d31988623f7d1ff299889ca49d4d03eb79 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Wed, 9 Sep 2026 16:36:27 +0800 Subject: [PATCH 1/2] fix: retract route configuration when no parent accepts the route Backports apache/apisix-ingress-controller#2858. #471 covered the exit where ParseRouteParentRefs resolves no Gateway of ours. The neighbouring one is still open: the parentRef does resolve to one of our Gateways, but no listener accepts the route, so isRouteAccepted is false and the update is skipped with no matching delete. Narrowing a listener's allowedRoutes, removing the listener a sectionName points at, or editing hostnames until they no longer intersect all reach it. The route reports Accepted=False while the old data plane route keeps forwarding, and deleting the route object is the only way to clear it, which is the wrong remedy when the edit was meant to revoke a tenant's access without touching their objects. Retract on that path in all five reconcilers, and set TypeMeta first for the same reason #471 does: Provider.Delete derives the resource labels from the object Kind. recordingProvider gains an updated counter and recordingUpdater is added beside it, so a test can assert that a route is not published rather than only that it is deleted. --- internal/controller/grpcroute_controller.go | 15 ++ internal/controller/httproute_controller.go | 15 ++ .../httproute_controller_retract_test.go | 158 ++++++++++++++++++ .../controller/httproute_controller_test.go | 10 ++ internal/controller/tcproute_controller.go | 15 ++ internal/controller/tlsroute_controller.go | 15 ++ internal/controller/udproute_controller.go | 15 ++ test/e2e/gatewayapi/httproute.go | 122 ++++++++++++++ 8 files changed, 365 insertions(+) create mode 100644 internal/controller/httproute_controller_retract_test.go diff --git a/internal/controller/grpcroute_controller.go b/internal/controller/grpcroute_controller.go index fdbc9a12..ff0e90fe 100644 --- a/internal/controller/grpcroute_controller.go +++ b/internal/controller/grpcroute_controller.go @@ -280,6 +280,21 @@ func (r *GRPCRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + gr.TypeMeta = metav1.TypeMeta{ + Kind: KindGRPCRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, gr); err != nil { + r.Log.Error(err, "failed to delete grpcroute", "grpcroute", gr) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index 7f3525b6..59585afd 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -306,6 +306,21 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + hr.TypeMeta = metav1.TypeMeta{ + Kind: KindHTTPRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, hr); err != nil { + r.Log.Error(err, "failed to delete httproute", "httproute", hr) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/internal/controller/httproute_controller_retract_test.go b/internal/controller/httproute_controller_retract_test.go new file mode 100644 index 00000000..a1572c91 --- /dev/null +++ b/internal/controller/httproute_controller_retract_test.go @@ -0,0 +1,158 @@ +// 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" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + "github.com/apache/apisix-ingress-controller/api/v1alpha1" + "github.com/apache/apisix-ingress-controller/internal/controller/config" + "github.com/apache/apisix-ingress-controller/internal/manager/readiness" +) + +const ( + retractGatewayNamespace = "infra" + retractRouteNamespace = "tenant" + retractRouteName = "route" +) + +// newHTTPRouteRetractFixture builds a Gateway of our class in retractGatewayNamespace +// and an HTTPRoute in retractRouteNamespace that names it as its parent. from +// controls the listener's allowedRoutes, which is what revoking cross-namespace +// access changes. +func newHTTPRouteRetractFixture( + t *testing.T, + from gatewayv1.FromNamespaces, +) (*HTTPRouteReconciler, *recordingProvider) { + t.Helper() + + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, gatewayv1.Install(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + gatewayClass := &gatewayv1.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{Name: "apisix"}, + Spec: gatewayv1.GatewayClassSpec{ + ControllerName: gatewayv1.GatewayController(config.ControllerConfig.ControllerName), + }, + } + gateway := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Namespace: retractGatewayNamespace, Name: "gw"}, + Spec: gatewayv1.GatewaySpec{ + GatewayClassName: "apisix", + Listeners: []gatewayv1.Listener{{ + Name: "http", + Protocol: gatewayv1.HTTPProtocolType, + Port: 80, + AllowedRoutes: &gatewayv1.AllowedRoutes{ + Namespaces: &gatewayv1.RouteNamespaces{From: &from}, + }, + }}, + }, + } + route := &gatewayv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: retractRouteNamespace, Name: retractRouteName}, + Spec: gatewayv1.HTTPRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ + ParentRefs: []gatewayv1.ParentReference{{ + Name: gatewayv1.ObjectName(gateway.Name), + Namespace: (*gatewayv1.Namespace)(&gateway.Namespace), + }}, + }, + }, + } + + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects([]client.Object{gatewayClass, gateway, route}...). + WithStatusSubresource(route). + Build() + + readier := readiness.NewReadinessManager(cli, logr.Discard()) + require.NoError(t, readier.Start(context.Background())) + + prov := &recordingProvider{} + return &HTTPRouteReconciler{ + Client: cli, + Scheme: scheme, + Log: logr.Discard(), + Provider: prov, + Updater: &recordingUpdater{}, + Readier: readier, + }, prov +} + +func reconcileRetractHTTPRoute(t *testing.T, r *HTTPRouteReconciler) (ctrl.Result, error) { + t.Helper() + return r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: k8stypes.NamespacedName{Namespace: retractRouteNamespace, Name: retractRouteName}, + }) +} + +var retractRouteKey = k8stypes.NamespacedName{Namespace: retractRouteNamespace, Name: retractRouteName} + +// Narrowing a listener's allowedRoutes leaves the HTTPRoute in place but stops it +// being accepted. The configuration an earlier reconcile published must be +// retracted, otherwise the data plane keeps serving a route the Gateway no longer +// admits and only deleting the HTTPRoute clears it. +func TestHTTPRouteReconcile_RetractsWhenListenerStopsAllowingRoute(t *testing.T) { + r, prov := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromSame) + + result, err := reconcileRetractHTTPRoute(t, r) + + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + assert.Equal(t, []k8stypes.NamespacedName{retractRouteKey}, prov.deleted) + assert.Zero(t, prov.updated, "a route that is not accepted must not be published") +} + +// An accepted route must still be published and must not be retracted. +func TestHTTPRouteReconcile_PublishesAcceptedRoute(t *testing.T) { + r, prov := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromAll) + + _, err := reconcileRetractHTTPRoute(t, r) + + require.NoError(t, err) + assert.Empty(t, prov.deleted, "an accepted route must not be retracted") + assert.Equal(t, 1, prov.updated) +} + +// A provider failure while retracting must surface so the reconcile is retried. +func TestHTTPRouteReconcile_RetractErrorIsReturned(t *testing.T) { + r, prov := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromSame) + prov.deleteErr = errors.New("provider unavailable") + + _, err := reconcileRetractHTTPRoute(t, r) + + require.Error(t, err) + assert.Contains(t, err.Error(), "provider unavailable") +} diff --git a/internal/controller/httproute_controller_test.go b/internal/controller/httproute_controller_test.go index 63762a49..68a21c13 100644 --- a/internal/controller/httproute_controller_test.go +++ b/internal/controller/httproute_controller_test.go @@ -32,6 +32,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + "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" ) @@ -114,6 +115,7 @@ func TestHTTPRouteReconcile_EmptyGateways(t *testing.T) { // recordingProvider records the objects passed to Delete and can be told to fail. type recordingProvider struct { + updated int deleted []k8stypes.NamespacedName deleteErr error } @@ -121,6 +123,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 } @@ -132,3 +135,10 @@ func (p *recordingProvider) Delete(_ context.Context, obj client.Object) error { 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. +type recordingUpdater struct { + updates []status.Update +} + +func (u *recordingUpdater) Update(update status.Update) { u.updates = append(u.updates, update) } diff --git a/internal/controller/tcproute_controller.go b/internal/controller/tcproute_controller.go index 4cf16ebe..000fbad2 100644 --- a/internal/controller/tcproute_controller.go +++ b/internal/controller/tcproute_controller.go @@ -371,6 +371,21 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + tr.TypeMeta = metav1.TypeMeta{ + Kind: KindTCPRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, tr); err != nil { + r.Log.Error(err, "failed to delete tcproute", "tcproute", tr) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/internal/controller/tlsroute_controller.go b/internal/controller/tlsroute_controller.go index c9df0e62..25231256 100644 --- a/internal/controller/tlsroute_controller.go +++ b/internal/controller/tlsroute_controller.go @@ -363,6 +363,21 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + tr.TypeMeta = metav1.TypeMeta{ + Kind: types.KindTLSRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, tr); err != nil { + r.Log.Error(err, "failed to delete tlsroute", "tlsroute", tr) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/internal/controller/udproute_controller.go b/internal/controller/udproute_controller.go index 2c8a910b..64d263e6 100644 --- a/internal/controller/udproute_controller.go +++ b/internal/controller/udproute_controller.go @@ -371,6 +371,21 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + tr.TypeMeta = metav1.TypeMeta{ + Kind: KindUDPRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, tr); err != nil { + r.Log.Error(err, "failed to delete udproute", "udproute", tr) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/test/e2e/gatewayapi/httproute.go b/test/e2e/gatewayapi/httproute.go index 094113e1..046a871d 100644 --- a/test/e2e/gatewayapi/httproute.go +++ b/test/e2e/gatewayapi/httproute.go @@ -205,6 +205,128 @@ spec: }) }) + Context("HTTPRoute revoked by its listener", func() { + // The listener starts out admitting HTTPRoute and is then narrowed to + // GRPCRoute only. The route object is untouched throughout, which is the + // point: revoking a route's access must not require editing the route. + var gatewayAllowingKinds = ` +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: %s +spec: + gatewayClassName: %s + listeners: + - name: http1 + protocol: HTTP + port: 80 + allowedRoutes: + kinds: + - group: gateway.networking.k8s.io + kind: %s + infrastructure: + parametersRef: + group: apisix.apache.org + kind: GatewayProxy + name: apisix-proxy-config +` + + var route = ` +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: httpbin +spec: + parentRefs: + - name: %s + hostnames: + - httpbin.example + rules: + - matches: + - path: + type: Exact + value: /get + backendRefs: + - name: httpbin-service-e2e-test + port: 80 +` + + // allowKind rewrites the listener to admit only the given route kind. + var allowKind = func(kind string) { + Expect(s.CreateResourceFromString( + fmt.Sprintf(gatewayAllowingKinds, s.Namespace(), s.Namespace(), kind), + )).NotTo(HaveOccurred(), "applying Gateway allowing "+kind) + } + + BeforeEach(func() { + By("create GatewayProxy") + Expect(s.CreateResourceFromString(s.GetGatewayProxySpec())).NotTo(HaveOccurred(), "creating GatewayProxy") + + By("create GatewayClass") + Expect(s.CreateResourceFromString(s.GetGatewayClassYaml())).NotTo(HaveOccurred(), "creating GatewayClass") + s.RetryAssertion(func() string { + gcyaml, _ := s.GetResourceYaml("GatewayClass", s.Namespace()) + return gcyaml + }).Should(ContainSubstring("message: the gatewayclass has been accepted by the apisix-ingress-controller"), + "check GatewayClass condition") + + By("create Gateway admitting HTTPRoute") + allowKind("HTTPRoute") + s.RetryAssertion(func() string { + gwyaml, _ := s.GetResourceYaml("Gateway", s.Namespace()) + return gwyaml + }).Should(ContainSubstring("message: the gateway has been accepted by the apisix-ingress-controller"), + "check Gateway condition status") + }) + + It("stops serving the route and resumes when the listener admits it again", func() { + By("create HTTPRoute") + s.ResourceApplied("HTTPRoute", "httpbin", fmt.Sprintf(route, s.Namespace()), 1) + + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "httpbin.example", + Check: scaffold.WithExpectedStatus(http.StatusOK), + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + + By("narrow the listener to GRPCRoute, leaving the HTTPRoute untouched") + allowKind("GRPCRoute") + + By("the route reports that no listener accepts it") + s.RetryAssertion(func() string { + routeYaml, _ := s.GetResourceYaml("HTTPRoute", "httpbin") + return routeYaml + }).Should(ContainSubstring("reason: NotAllowedByListeners"), "check HTTPRoute condition") + + By("and the data plane stops serving it") + // Without the retraction the previously published route keeps + // forwarding, so the status and the data plane disagree until the + // HTTPRoute itself is deleted. + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "httpbin.example", + Check: scaffold.WithExpectedStatus(http.StatusNotFound), + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + + By("restore the listener and the route is served again") + allowKind("HTTPRoute") + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "httpbin.example", + Check: scaffold.WithExpectedStatus(http.StatusOK), + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + }) + }) + Context("HTTPRoute with Multiple Gateway", Serial, func() { var additionalGatewayGroupID string var additionalSvc *corev1.Service From 4305b28479e2b2269b23236394cb2541a1b7813e Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Thu, 10 Sep 2026 07:17:57 +0800 Subject: [PATCH 2/2] fix: log the route name, not the whole object, when retraction fails An HTTPRoute can carry a RequestHeaderModifier that sets Authorization, so logging the object writes its header values into the controller log, which is usually read by a wider audience than the API. The retraction paths copied the shape of the NotFound branch above them; the delete paths in the ApisixPluginConfig and servicePort backports already log utils.NamespacedName, so this also makes the five consistent with those. Only the lines this change introduces are touched. --- internal/controller/grpcroute_controller.go | 2 +- internal/controller/httproute_controller.go | 2 +- internal/controller/tcproute_controller.go | 2 +- internal/controller/tlsroute_controller.go | 2 +- internal/controller/udproute_controller.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/controller/grpcroute_controller.go b/internal/controller/grpcroute_controller.go index ff0e90fe..ef4d937a 100644 --- a/internal/controller/grpcroute_controller.go +++ b/internal/controller/grpcroute_controller.go @@ -293,7 +293,7 @@ func (r *GRPCRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( APIVersion: gatewayv1.GroupVersion.String(), } if err := r.Provider.Delete(ctx, gr); err != nil { - r.Log.Error(err, "failed to delete grpcroute", "grpcroute", gr) + r.Log.Error(err, "failed to delete grpcroute", "grpcroute", utils.NamespacedName(gr)) return ctrl.Result{}, err } return ctrl.Result{}, nil diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index 59585afd..453e4358 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -319,7 +319,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( APIVersion: gatewayv1.GroupVersion.String(), } if err := r.Provider.Delete(ctx, hr); err != nil { - r.Log.Error(err, "failed to delete httproute", "httproute", hr) + r.Log.Error(err, "failed to delete httproute", "httproute", utils.NamespacedName(hr)) return ctrl.Result{}, err } return ctrl.Result{}, nil diff --git a/internal/controller/tcproute_controller.go b/internal/controller/tcproute_controller.go index 000fbad2..d543988e 100644 --- a/internal/controller/tcproute_controller.go +++ b/internal/controller/tcproute_controller.go @@ -384,7 +384,7 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c APIVersion: gatewayv1.GroupVersion.String(), } if err := r.Provider.Delete(ctx, tr); err != nil { - r.Log.Error(err, "failed to delete tcproute", "tcproute", tr) + r.Log.Error(err, "failed to delete tcproute", "tcproute", utils.NamespacedName(tr)) return ctrl.Result{}, err } return ctrl.Result{}, nil diff --git a/internal/controller/tlsroute_controller.go b/internal/controller/tlsroute_controller.go index 25231256..e62abe99 100644 --- a/internal/controller/tlsroute_controller.go +++ b/internal/controller/tlsroute_controller.go @@ -376,7 +376,7 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c APIVersion: gatewayv1.GroupVersion.String(), } if err := r.Provider.Delete(ctx, tr); err != nil { - r.Log.Error(err, "failed to delete tlsroute", "tlsroute", tr) + r.Log.Error(err, "failed to delete tlsroute", "tlsroute", utils.NamespacedName(tr)) return ctrl.Result{}, err } return ctrl.Result{}, nil diff --git a/internal/controller/udproute_controller.go b/internal/controller/udproute_controller.go index 64d263e6..e1c0f4d9 100644 --- a/internal/controller/udproute_controller.go +++ b/internal/controller/udproute_controller.go @@ -384,7 +384,7 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c APIVersion: gatewayv1.GroupVersion.String(), } if err := r.Provider.Delete(ctx, tr); err != nil { - r.Log.Error(err, "failed to delete udproute", "udproute", tr) + r.Log.Error(err, "failed to delete udproute", "udproute", utils.NamespacedName(tr)) return ctrl.Result{}, err } return ctrl.Result{}, nil