fix: retract route configuration when no parent accepts the route (backport apache/apisix-ingress-controller#2858)

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", utils.NamespacedName(gr))
+		return ctrl.Result{}, err
 	}
 	return ctrl.Result{}, nil
 }

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", utils.NamespacedName(hr))
+		return ctrl.Result{}, err
 	}
 	return ctrl.Result{}, nil
 }

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")
+}

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,13 +115,15 @@ 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
 }
 
 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) }

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", utils.NamespacedName(tr))
+		return ctrl.Result{}, err
 	}
 	return ctrl.Result{}, nil
 }

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", utils.NamespacedName(tr))
+		return ctrl.Result{}, err
 	}
 	return ctrl.Result{}, nil
 }

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", utils.NamespacedName(tr))
+		return ctrl.Result{}, err
 	}
 	return ctrl.Result{}, nil
 }