From d09829d919aa2c4bfcac08316e0bc2cd0c4461c3 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Sat, 10 Oct 2026 09:36:46 +0800 Subject: [PATCH] fix: surface Gateway API route translation failures in status (#2840) Call Provider.Update before persisting Accepted=True so a translation or buildConfig error is visible on HTTPRoute, GRPCRoute, TCPRoute, UDPRoute, and TLSRoute instead of a green status plus requeue. --- internal/controller/grpcroute_controller.go | 17 ++- .../controller/grpcroute_controller_test.go | 133 ++++++++++++++++++ internal/controller/httproute_controller.go | 26 ++-- .../httproute_controller_retract_test.go | 51 ++++++- .../controller/httproute_controller_test.go | 6 +- internal/controller/tcproute_controller.go | 16 ++- internal/controller/tlsroute_controller.go | 16 ++- internal/controller/udproute_controller.go | 16 ++- 8 files changed, 244 insertions(+), 37 deletions(-) create mode 100644 internal/controller/grpcroute_controller_test.go diff --git a/internal/controller/grpcroute_controller.go b/internal/controller/grpcroute_controller.go index a2f4c510..11a5a2ca 100644 --- a/internal/controller/grpcroute_controller.go +++ b/internal/controller/grpcroute_controller.go @@ -256,6 +256,17 @@ func (r *GRPCRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( ProcessBackendTrafficPolicy(r.Client, r.Log, tctx) + // Translate and sync before writing Accepted=True. A translation or + // buildConfig failure from Provider.Update used to leave a green status + // and only requeue, matching neither Ingress nor ApisixGlobalRule. + var syncErr error + if isRouteAccepted(gateways) && err == nil { + if syncErr = r.Provider.Update(ctx, tctx, gr); syncErr != nil { + acceptStatus.status = false + acceptStatus.msg = syncErr.Error() + } + } + // TODO: diff the old and new status gr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { @@ -287,11 +298,7 @@ func (r *GRPCRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) && err == nil { - routeToUpdate := gr - if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + return ctrl.Result{}, syncErr } // The route still resolves to one of our Gateways but no parent accepts it any diff --git a/internal/controller/grpcroute_controller_test.go b/internal/controller/grpcroute_controller_test.go new file mode 100644 index 00000000..35e7e869 --- /dev/null +++ b/internal/controller/grpcroute_controller_test.go @@ -0,0 +1,133 @@ +// 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" + "k8s.io/apimachinery/pkg/api/meta" + 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" +) + +func newGRPCRouteFixture(t *testing.T) (*GRPCRouteReconciler, *recordingProvider, *recordingUpdater) { + t.Helper() + + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, gatewayv1.Install(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + from := gatewayv1.NamespacesFromAll + 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.GRPCRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: retractRouteNamespace, Name: retractRouteName}, + Spec: gatewayv1.GRPCRouteSpec{ + 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{} + updater := &recordingUpdater{} + return &GRPCRouteReconciler{ + Client: cli, + Scheme: scheme, + Log: logr.Discard(), + Provider: prov, + Updater: updater, + Readier: readier, + }, prov, updater +} + +func grpcAcceptedConditionFromUpdater(t *testing.T, updater *recordingUpdater) *metav1.Condition { + t.Helper() + require.NotEmpty(t, updater.updates) + mutated, ok := updater.updates[0].Mutator.Mutate(&gatewayv1.GRPCRoute{}).(*gatewayv1.GRPCRoute) + require.True(t, ok) + require.Len(t, mutated.Status.Parents, 1) + accepted := meta.FindStatusCondition(mutated.Status.Parents[0].Conditions, string(gatewayv1.RouteConditionAccepted)) + require.NotNil(t, accepted) + return accepted +} + +// A translation / Provider.Update failure must be visible on the route status +// instead of Accepted=True plus a silent requeue. +func TestGRPCRouteReconcile_UpdateErrorIsVisibleOnStatus(t *testing.T) { + r, prov, updater := newGRPCRouteFixture(t) + prov.updateErr = errors.New("translation failed") + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: k8stypes.NamespacedName{Namespace: retractRouteNamespace, Name: retractRouteName}, + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "translation failed") + assert.Equal(t, 1, prov.updated) + assert.Empty(t, prov.deleted) + + accepted := grpcAcceptedConditionFromUpdater(t, updater) + assert.Equal(t, metav1.ConditionFalse, accepted.Status) + assert.Contains(t, accepted.Message, "translation failed") +} diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index e34c2c40..d2e507aa 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -273,6 +273,22 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( acceptStatus.msg = err.Error() } + // Translate and sync before writing Accepted=True. A translation or + // buildConfig failure from Provider.Update used to leave a green status + // and only requeue, matching neither Ingress nor ApisixGlobalRule. + var syncErr error + if isRouteAccepted(gateways) && err == nil { + routeToUpdate := hr + if filteredHTTPRoute != nil { + r.Log.V(1).Info("filtered httproute", "httproute", utils.NamespacedName(filteredHTTPRoute)) + routeToUpdate = filteredHTTPRoute + } + if syncErr = r.Provider.Update(ctx, tctx, routeToUpdate); syncErr != nil { + acceptStatus.status = false + acceptStatus.msg = syncErr.Error() + } + } + // TODO: diff the old and new status hr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { @@ -304,15 +320,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) && err == nil { - routeToUpdate := hr - if filteredHTTPRoute != nil { - r.Log.V(1).Info("filtered httproute", "httproute", utils.NamespacedName(filteredHTTPRoute)) - routeToUpdate = filteredHTTPRoute - } - if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + return ctrl.Result{}, syncErr } // The route still resolves to one of our Gateways but no parent accepts it any diff --git a/internal/controller/httproute_controller_retract_test.go b/internal/controller/httproute_controller_retract_test.go index a1572c91..66f38fc7 100644 --- a/internal/controller/httproute_controller_retract_test.go +++ b/internal/controller/httproute_controller_retract_test.go @@ -25,6 +25,7 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" k8stypes "k8s.io/apimachinery/pkg/types" @@ -36,6 +37,7 @@ import ( "github.com/apache/apisix-ingress-controller/api/v1alpha1" "github.com/apache/apisix-ingress-controller/internal/controller/config" + "github.com/apache/apisix-ingress-controller/internal/controller/indexer" "github.com/apache/apisix-ingress-controller/internal/manager/readiness" ) @@ -52,7 +54,7 @@ const ( func newHTTPRouteRetractFixture( t *testing.T, from gatewayv1.FromNamespaces, -) (*HTTPRouteReconciler, *recordingProvider) { +) (*HTTPRouteReconciler, *recordingProvider, *recordingUpdater) { t.Helper() scheme := runtime.NewScheme() @@ -95,20 +97,25 @@ func newHTTPRouteRetractFixture( cli := fake.NewClientBuilder().WithScheme(scheme). WithObjects([]client.Object{gatewayClass, gateway, route}...). WithStatusSubresource(route). + // The reconcile lists HTTPRoutePolicies by target, which the fake client + // only serves once the index exists. No policy is under test here. + WithIndex(&v1alpha1.HTTPRoutePolicy{}, indexer.PolicyTargetRefs, + func(client.Object) []string { return nil }). Build() readier := readiness.NewReadinessManager(cli, logr.Discard()) require.NoError(t, readier.Start(context.Background())) prov := &recordingProvider{} + updater := &recordingUpdater{} return &HTTPRouteReconciler{ Client: cli, Scheme: scheme, Log: logr.Discard(), Provider: prov, - Updater: &recordingUpdater{}, + Updater: updater, Readier: readier, - }, prov + }, prov, updater } func reconcileRetractHTTPRoute(t *testing.T, r *HTTPRouteReconciler) (ctrl.Result, error) { @@ -125,7 +132,7 @@ var retractRouteKey = k8stypes.NamespacedName{Namespace: retractRouteNamespace, // 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) + r, prov, _ := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromSame) result, err := reconcileRetractHTTPRoute(t, r) @@ -137,18 +144,21 @@ func TestHTTPRouteReconcile_RetractsWhenListenerStopsAllowingRoute(t *testing.T) // An accepted route must still be published and must not be retracted. func TestHTTPRouteReconcile_PublishesAcceptedRoute(t *testing.T) { - r, prov := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromAll) + r, prov, updater := 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) + + accepted := acceptedConditionFromUpdater(t, updater) + assert.Equal(t, metav1.ConditionTrue, accepted.Status) } // A provider failure while retracting must surface so the reconcile is retried. func TestHTTPRouteReconcile_RetractErrorIsReturned(t *testing.T) { - r, prov := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromSame) + r, prov, _ := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromSame) prov.deleteErr = errors.New("provider unavailable") _, err := reconcileRetractHTTPRoute(t, r) @@ -156,3 +166,32 @@ func TestHTTPRouteReconcile_RetractErrorIsReturned(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "provider unavailable") } + +func acceptedConditionFromUpdater(t *testing.T, updater *recordingUpdater) *metav1.Condition { + t.Helper() + require.NotEmpty(t, updater.updates) + mutated, ok := updater.updates[0].Mutator.Mutate(&gatewayv1.HTTPRoute{}).(*gatewayv1.HTTPRoute) + require.True(t, ok) + require.Len(t, mutated.Status.Parents, 1) + accepted := meta.FindStatusCondition(mutated.Status.Parents[0].Conditions, string(gatewayv1.RouteConditionAccepted)) + require.NotNil(t, accepted) + return accepted +} + +// A translation / Provider.Update failure must be visible on the route status +// instead of Accepted=True plus a silent requeue. +func TestHTTPRouteReconcile_UpdateErrorIsVisibleOnStatus(t *testing.T) { + r, prov, updater := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromAll) + prov.updateErr = errors.New("translation failed") + + _, err := reconcileRetractHTTPRoute(t, r) + + require.Error(t, err) + assert.Contains(t, err.Error(), "translation failed") + assert.Equal(t, 1, prov.updated) + assert.Empty(t, prov.deleted) + + accepted := acceptedConditionFromUpdater(t, updater) + assert.Equal(t, metav1.ConditionFalse, accepted.Status) + assert.Contains(t, accepted.Message, "translation failed") +} diff --git a/internal/controller/httproute_controller_test.go b/internal/controller/httproute_controller_test.go index 68a21c13..42daff8b 100644 --- a/internal/controller/httproute_controller_test.go +++ b/internal/controller/httproute_controller_test.go @@ -113,18 +113,20 @@ func TestHTTPRouteReconcile_EmptyGateways(t *testing.T) { } } -// 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 an update or a delete. type recordingProvider struct { updated int deleted []k8stypes.NamespacedName deleteErr error + updateErr error } func (p *recordingProvider) Register(string, *http.ServeMux) {} func (p *recordingProvider) Update(context.Context, *provider.TranslateContext, client.Object) error { p.updated++ - return nil + return p.updateErr } func (p *recordingProvider) Delete(_ context.Context, obj client.Object) error { diff --git a/internal/controller/tcproute_controller.go b/internal/controller/tcproute_controller.go index b8d6f97e..8428a55e 100644 --- a/internal/controller/tcproute_controller.go +++ b/internal/controller/tcproute_controller.go @@ -348,6 +348,16 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if r.supportsL4RoutePolicy { ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindTCPRoute) } + // Translate and sync before writing Accepted=True. A translation or + // buildConfig failure from Provider.Update used to leave a green status + // and only requeue, matching neither Ingress nor ApisixGlobalRule. + var syncErr error + if isRouteAccepted(gateways) { + if syncErr = r.Provider.Update(ctx, tctx, tr); syncErr != nil { + acceptStatus.status = false + acceptStatus.msg = syncErr.Error() + } + } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { parentStatus := gatewayv1.RouteParentStatus{} @@ -377,11 +387,7 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { - routeToUpdate := tr - if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + return ctrl.Result{}, syncErr } // The route still resolves to one of our Gateways but no parent accepts it any diff --git a/internal/controller/tlsroute_controller.go b/internal/controller/tlsroute_controller.go index 779b7603..f1b1792f 100644 --- a/internal/controller/tlsroute_controller.go +++ b/internal/controller/tlsroute_controller.go @@ -348,6 +348,16 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if r.supportsL4RoutePolicy { ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, types.KindTLSRoute) } + // Translate and sync before writing Accepted=True. A translation or + // buildConfig failure from Provider.Update used to leave a green status + // and only requeue, matching neither Ingress nor ApisixGlobalRule. + var syncErr error + if isRouteAccepted(gateways) { + if syncErr = r.Provider.Update(ctx, tctx, tr); syncErr != nil { + acceptStatus.status = false + acceptStatus.msg = syncErr.Error() + } + } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { parentStatus := gatewayv1.RouteParentStatus{} @@ -377,11 +387,7 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { - routeToUpdate := tr - if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + return ctrl.Result{}, syncErr } // The route still resolves to one of our Gateways but no parent accepts it any diff --git a/internal/controller/udproute_controller.go b/internal/controller/udproute_controller.go index 3ca88907..a9475941 100644 --- a/internal/controller/udproute_controller.go +++ b/internal/controller/udproute_controller.go @@ -348,6 +348,16 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if r.supportsL4RoutePolicy { ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindUDPRoute) } + // Translate and sync before writing Accepted=True. A translation or + // buildConfig failure from Provider.Update used to leave a green status + // and only requeue, matching neither Ingress nor ApisixGlobalRule. + var syncErr error + if isRouteAccepted(gateways) { + if syncErr = r.Provider.Update(ctx, tctx, tr); syncErr != nil { + acceptStatus.status = false + acceptStatus.msg = syncErr.Error() + } + } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { parentStatus := gatewayv1.RouteParentStatus{} @@ -377,11 +387,7 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { - routeToUpdate := tr - if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + return ctrl.Result{}, syncErr } // The route still resolves to one of our Gateways but no parent accepts it any