diff --git a/internal/adc/client/client.go b/internal/adc/client/client.go index 83d617a1f..675578fcc 100644 --- a/internal/adc/client/client.go +++ b/internal/adc/client/client.go @@ -178,8 +178,12 @@ func (c *Client) applyStoreChanges(args Task, isDelete bool) (StoreDelta, error) delta.Applied = args.Configs } - for _, cfg := range delta.Deleted { - if err := c.Store.Delete(cfg.Name, args.ResourceTypes, args.Labels); err != nil { + for key, cfg := range delta.Deleted { + resourceTypes := args.ResourceTypes + if args.Key.Kind == types.KindGateway { + resourceTypes = gatewayEvictionTypes(resourceTypes, c.ConfigManager.HasResource(key, isGatewayProxyReferrer)) + } + if err := c.Store.Delete(cfg.Name, resourceTypes, args.Labels); err != nil { c.log.Error(err, "store delete failed", "cfg", cfg, "args", args) return StoreDelta{}, errors.Wrap(err, fmt.Sprintf("store delete failed for config %s", cfg.Name)) } @@ -195,6 +199,26 @@ func (c *Client) applyStoreChanges(args Task, isDelete bool) (StoreDelta, error) return delta, nil } +// gatewayEvictionTypes evicts a Gateway's own global rule, stored under its labels, but +// keeps the GatewayProxy's plugin_metadata, shared by every Gateway and IngressClass +// referencing it, until the last of them is gone. Only the store is affected: the +// immediate sync keeps the caller's resource types, as ADC does not label-filter +// global rules. +func gatewayEvictionTypes(resourceTypes []string, referenced bool) []string { + resourceTypes = slices.DeleteFunc(slices.Clone(resourceTypes), func(t string) bool { + return t == adctypes.TypeGlobalRule || t == adctypes.TypePluginMetadata + }) + resourceTypes = append(resourceTypes, adctypes.TypeGlobalRule) + if !referenced { + resourceTypes = append(resourceTypes, adctypes.TypePluginMetadata) + } + return resourceTypes +} + +func isGatewayProxyReferrer(key types.NamespacedNameKind) bool { + return key.Kind == types.KindGateway || key.Kind == types.KindIngressClass +} + func (c *Client) applySync(ctx context.Context, args Task, delta StoreDelta) error { c.syncMu.RLock() defer c.syncMu.RUnlock() diff --git a/internal/controller/grpcroute_controller.go b/internal/controller/grpcroute_controller.go index 11a5a2caf..9db9261d3 100644 --- a/internal/controller/grpcroute_controller.go +++ b/internal/controller/grpcroute_controller.go @@ -57,6 +57,8 @@ type GRPCRouteReconciler struct { //nolint:revive Provider provider.Provider + parentResolver routeParentResolver + genericEvent chan event.GenericEvent Updater status.Updater @@ -94,7 +96,7 @@ func (r *GRPCRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true @@ -164,6 +166,7 @@ func (r *GRPCRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( gr := new(gatewayv1.GRPCRoute) if err := r.Get(ctx, req.NamespacedName, gr); err != nil { if client.IgnoreNotFound(err) == nil { + r.parentResolver.Forget(req.NamespacedName) gr.Namespace = req.Namespace gr.Name = req.Name @@ -192,17 +195,12 @@ func (r *GRPCRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( msg: "Route is accepted", } - gateways, unresolvedParents, err := ParseRouteParentRefs(ctx, r.Client, r.Log, gr, gr.Spec.ParentRefs) + gateways, err := r.parentResolver.Resolve(ctx, r.Client, r.Log, gr, gr.Spec.ParentRefs) if err != nil { return ctrl.Result{}, err } if len(gateways) == 0 { - if unresolvedParents { - // See the HTTPRoute reconciler: an unresolvable parentRef leaves ownership - // unknown rather than disproven, so the data plane must be left alone. - return ctrl.Result{}, nil - } // See the HTTPRoute reconciler: a route that no longer references a // Gateway managed by this controller must have its previously pushed // configuration removed, or the data plane keeps serving it. diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index d2e507aab..a556eb7ee 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -60,6 +60,8 @@ type HTTPRouteReconciler struct { //nolint:revive Provider provider.Provider + parentResolver routeParentResolver + genericEvent chan event.GenericEvent Updater status.Updater @@ -116,7 +118,7 @@ func (r *HTTPRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true @@ -162,6 +164,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( hr := new(gatewayv1.HTTPRoute) if err := r.Get(ctx, req.NamespacedName, hr); err != nil { if client.IgnoreNotFound(err) == nil { + r.parentResolver.Forget(req.NamespacedName) if err := r.updateHTTPRoutePolicyStatusOnDeleting(ctx, req.NamespacedName); err != nil { r.Log.Error(err, "failed to update HTTPRoutePolicy status on HTTPRoute deleting", "httproute", req.NamespacedName) } @@ -193,18 +196,12 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( msg: "Route is accepted", } - gateways, unresolvedParents, err := ParseRouteParentRefs(ctx, r.Client, r.Log, hr, hr.Spec.ParentRefs) + gateways, err := r.parentResolver.Resolve(ctx, r.Client, r.Log, hr, hr.Spec.ParentRefs) if err != nil { return ctrl.Result{}, err } if len(gateways) == 0 { - if unresolvedParents { - // A missing Gateway or GatewayClass leaves ownership unknown rather than - // disproven. GatewayClass is cluster-scoped, so while one is absent every - // route under it resolves empty and deleting would drain the data plane. - return ctrl.Result{}, nil - } // The route does not reference any Gateway managed by this controller. // It may have referenced one before, e.g. when its parentRefs are // repointed at a Gateway belonging to another GatewayClass, so the diff --git a/internal/controller/httproute_controller_test.go b/internal/controller/httproute_controller_test.go index 42daff8b0..6e9d2f1e1 100644 --- a/internal/controller/httproute_controller_test.go +++ b/internal/controller/httproute_controller_test.go @@ -43,11 +43,7 @@ type noopReadier struct { func (noopReadier) Done(client.Object, k8stypes.NamespacedName) {} -// TestHTTPRouteReconcile_EmptyGateways covers the two ways a route resolves to no -// Gateway of this controller. Only a resolved parent naming another controller -// proves the route is not ours; an unresolvable parent must leave the data plane -// alone, since a missing cluster-scoped GatewayClass empties the list for every -// route under it at once. +// Routes with no known managed parents retract any stale configuration. func TestHTTPRouteReconcile_EmptyGateways(t *testing.T) { scheme := parentRefTestScheme(t) @@ -85,9 +81,9 @@ func TestHTTPRouteReconcile_EmptyGateways(t *testing.T) { wantDelete: true, }, { - name: "gatewayclass missing", + name: "gatewayclass never resolved", objects: []client.Object{route, gw("gone")}, - wantDelete: false, + wantDelete: true, }, } { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/controller/route_parent_resolver.go b/internal/controller/route_parent_resolver.go new file mode 100644 index 000000000..89cab1c89 --- /dev/null +++ b/internal/controller/route_parent_resolver.go @@ -0,0 +1,101 @@ +// 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" + "sync" + + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + "github.com/apache/apisix-ingress-controller/internal/controller/config" +) + +type resolvedParentClass struct { + gatewayUID types.UID + class *gatewayv1.GatewayClass +} + +type routeParentOwnership struct { + uid types.UID + parents map[client.ObjectKey]resolvedParentClass +} + +// routeParentResolver remembers ownership only for parents previously resolved +// for this route. A never-resolved sibling must not prevent deletion cleanup. +// Like the provider's desired configuration, this state is rebuilt on startup. +type routeParentResolver struct { + mu sync.Mutex + parents map[client.ObjectKey]routeParentOwnership +} + +func (r *routeParentResolver) Forget(key client.ObjectKey) { + r.mu.Lock() + defer r.mu.Unlock() + delete(r.parents, key) +} + +func (r *routeParentResolver) Resolve(ctx context.Context, cli client.Client, log logr.Logger, route client.Object, refs []gatewayv1.ParentReference) ([]RouteParentRefContext, error) { + // controller-runtime never reconciles one route concurrently, so the lock only + // guards the map, not the parent lookups between the read and the write. + routeKey := client.ObjectKeyFromObject(route) + r.mu.Lock() + previous := r.parents[routeKey].parents + if r.parents[routeKey].uid != route.GetUID() { + previous = nil + } + r.mu.Unlock() + next := make(map[client.ObjectKey]resolvedParentClass) + gateways, err := parseRouteParentRefs(ctx, cli, log, route, refs, func(gw *gatewayv1.Gateway) (*gatewayv1.GatewayClass, error) { + key := client.ObjectKeyFromObject(gw) + gc := &gatewayv1.GatewayClass{} + if err := cli.Get(ctx, client.ObjectKey{Name: string(gw.Spec.GatewayClassName)}, gc); err != nil { + if client.IgnoreNotFound(err) != nil { + return nil, err + } + known, ok := previous[key] + if !ok || known.gatewayUID != gw.UID || known.class.Name != string(gw.Spec.GatewayClassName) { + return nil, nil + } + // Keep resolving the surviving Gateway's current listeners without + // retaining configuration belonging to a different, deleted parent. + gc = known.class + } + if string(gc.Spec.ControllerName) == config.ControllerConfig.ControllerName { + next[key] = resolvedParentClass{gatewayUID: gw.UID, class: gc} + } + return gc, nil + }) + if err != nil { + return nil, err + } + r.mu.Lock() + defer r.mu.Unlock() + if len(next) == 0 { + delete(r.parents, routeKey) + } else { + if r.parents == nil { + r.parents = make(map[client.ObjectKey]routeParentOwnership) + } + r.parents[routeKey] = routeParentOwnership{uid: route.GetUID(), parents: next} + } + return gateways, nil +} diff --git a/internal/controller/route_parent_resolver_test.go b/internal/controller/route_parent_resolver_test.go new file mode 100644 index 000000000..4ab3f5054 --- /dev/null +++ b/internal/controller/route_parent_resolver_test.go @@ -0,0 +1,192 @@ +// 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/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +func TestRouteParentResolverGatewayDeletion(t *testing.T) { + for _, tc := range []struct { + name string + route client.Object + protocol gatewayv1.ProtocolType + }{ + {"HTTPRoute", &gatewayv1.HTTPRoute{}, gatewayv1.HTTPProtocolType}, + {"GRPCRoute", &gatewayv1.GRPCRoute{}, gatewayv1.HTTPProtocolType}, + {"TCPRoute", &gatewayv1.TCPRoute{}, gatewayv1.TCPProtocolType}, + {"UDPRoute", &gatewayv1.UDPRoute{}, gatewayv1.UDPProtocolType}, + {"TLSRoute", &gatewayv1.TLSRoute{}, gatewayv1.TLSProtocolType}, + } { + t.Run(tc.name, func(t *testing.T) { + for _, knownSibling := range []bool{false, true} { + name := "never-resolved sibling" + if knownSibling { + name = "previously-resolved sibling" + } + t.Run(name, func(t *testing.T) { + ctx := context.Background() + route := tc.route.DeepCopyObject().(client.Object) + route.SetNamespace("default") + route.SetName("route") + route.SetUID("route-uid") + classA := newParentRefGatewayClass() + classB := classA.DeepCopy() + classB.Name = "sibling-class" + listener := gatewayv1.Listener{Name: "listener", Port: 9000, Protocol: tc.protocol} + if tc.protocol == gatewayv1.TLSProtocolType { + listener.TLS = &gatewayv1.ListenerTLSConfig{Mode: ptr.To(gatewayv1.TLSModePassthrough)} + } + a := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "a", UID: "a-uid"}, + Spec: gatewayv1.GatewaySpec{GatewayClassName: gatewayv1.ObjectName(classA.Name), Listeners: []gatewayv1.Listener{listener}}, + } + b := a.DeepCopy() + b.Name, b.UID = "b", "b-uid" + b.Spec.GatewayClassName = gatewayv1.ObjectName(classB.Name) + objects := []client.Object{classA, a, b} + if knownSibling { + objects = append(objects, classB) + } + cli := fake.NewClientBuilder().WithScheme(parentRefTestScheme(t)).WithObjects(objects...).Build() + refs := []gatewayv1.ParentReference{{Name: "a"}, {Name: "b"}} + resolver := &routeParentResolver{} + parents, err := resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.NoError(t, err) + expected := 1 + if knownSibling { + expected = 2 + } + require.Len(t, parents, expected) + require.True(t, isRouteAccepted(parents)) + if knownSibling { + require.NoError(t, cli.Delete(ctx, classB)) + } + require.NoError(t, cli.Delete(ctx, a)) + parents, err = resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.NoError(t, err) + if knownSibling { + require.Len(t, parents, 1) + require.Equal(t, "b", parents[0].Gateway.Name) + require.True(t, isRouteAccepted(parents)) + // Listener changes still apply while its class is absent. + b.Spec.Listeners[0].AllowedRoutes = &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{{Kind: "UnsupportedRoute"}}, + } + require.NoError(t, cli.Update(ctx, b)) + parents, err = resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.NoError(t, err) + require.False(t, isRouteAccepted(parents)) + require.NoError(t, cli.Delete(ctx, b)) + parents, err = resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.NoError(t, err) + } + require.Empty(t, parents) + require.Empty(t, resolver.parents) + }) + } + }) + } +} + +func TestRouteParentResolverDoesNotReuseOwnership(t *testing.T) { + for _, change := range []string{"gateway recreated", "route recreated", "class changed", "route deleted", "parent removed", "class reassigned"} { + t.Run(change, func(t *testing.T) { + ctx := context.Background() + gc := newParentRefGatewayClass() + gw := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "gw", UID: "gateway-uid"}, + Spec: gatewayv1.GatewaySpec{GatewayClassName: gatewayv1.ObjectName(gc.Name), Listeners: []gatewayv1.Listener{{Name: "http", Port: 80, Protocol: gatewayv1.HTTPProtocolType}}}, + } + route := &gatewayv1.HTTPRoute{ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route", UID: "route-uid"}} + cli := fake.NewClientBuilder().WithScheme(parentRefTestScheme(t)).WithObjects(gc, gw).Build() + resolver := &routeParentResolver{} + refs := []gatewayv1.ParentReference{{Name: "gw"}} + parents, err := resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.NoError(t, err) + require.Len(t, parents, 1) + require.NoError(t, cli.Delete(ctx, gc)) + switch change { + case "gateway recreated": + require.NoError(t, cli.Delete(ctx, gw)) + gw.ResourceVersion = "" + gw.UID = "new-gateway-uid" + require.NoError(t, cli.Create(ctx, gw)) + case "route recreated": + route.UID = "new-route-uid" + case "class changed": + gw.Spec.GatewayClassName = "different-class" + require.NoError(t, cli.Update(ctx, gw)) + case "route deleted": + resolver.Forget(client.ObjectKeyFromObject(route)) + case "parent removed": + _, err = resolver.Resolve(ctx, cli, logr.Discard(), route, nil) + require.NoError(t, err) + case "class reassigned": + gc.ResourceVersion = "" + gc.Spec.ControllerName = "other.example/controller" + require.NoError(t, cli.Create(ctx, gc)) + } + parents, err = resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.NoError(t, err) + require.Empty(t, parents) + }) + } +} + +func TestRouteParentResolverPreservesOwnershipOnReadError(t *testing.T) { + ctx := context.Background() + gc := newParentRefGatewayClass() + gw := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "gw", UID: "gateway-uid"}, + Spec: gatewayv1.GatewaySpec{GatewayClassName: gatewayv1.ObjectName(gc.Name), Listeners: []gatewayv1.Listener{{Name: "http", Port: 80, Protocol: gatewayv1.HTTPProtocolType}}}, + } + route := &gatewayv1.HTTPRoute{ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}} + fail := false + readErr := errors.New("temporary API failure") + cli := fake.NewClientBuilder().WithScheme(parentRefTestScheme(t)).WithObjects(gc, gw). + WithInterceptorFuncs(interceptor.Funcs{Get: func(ctx context.Context, cli client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*gatewayv1.GatewayClass); ok && fail { + return readErr + } + return cli.Get(ctx, key, obj, opts...) + }}).Build() + resolver := &routeParentResolver{} + refs := []gatewayv1.ParentReference{{Name: "gw"}} + _, err := resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.NoError(t, err) + fail = true + _, err = resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.ErrorIs(t, err, readErr) + fail = false + require.NoError(t, cli.Delete(ctx, gc)) + parents, err := resolver.Resolve(ctx, cli, logr.Discard(), route, refs) + require.NoError(t, err) + require.Len(t, parents, 1) +} diff --git a/internal/controller/tcproute_controller.go b/internal/controller/tcproute_controller.go index 8428a55e4..8116f4822 100644 --- a/internal/controller/tcproute_controller.go +++ b/internal/controller/tcproute_controller.go @@ -55,6 +55,8 @@ type TCPRouteReconciler struct { //nolint:revive Provider provider.Provider + parentResolver routeParentResolver + Updater status.Updater Readier readiness.ReadinessManager @@ -86,7 +88,7 @@ func (r *TCPRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true @@ -253,6 +255,7 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c tr := new(gatewayv1.TCPRoute) if err := r.Get(ctx, req.NamespacedName, tr); err != nil { if client.IgnoreNotFound(err) == nil { + r.parentResolver.Forget(req.NamespacedName) tr.Namespace = req.Namespace tr.Name = req.Name @@ -283,17 +286,12 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c msg: "Route is accepted", } - gateways, unresolvedParents, err := ParseRouteParentRefs(ctx, r.Client, r.Log, tr, tr.Spec.ParentRefs) + gateways, err := r.parentResolver.Resolve(ctx, r.Client, r.Log, tr, tr.Spec.ParentRefs) if err != nil { return ctrl.Result{}, err } if len(gateways) == 0 { - if unresolvedParents { - // See the HTTPRoute reconciler: an unresolvable parentRef leaves ownership - // unknown rather than disproven, so the data plane must be left alone. - return ctrl.Result{}, nil - } // See the HTTPRoute reconciler: a route that no longer references a // Gateway managed by this controller must have its previously pushed // configuration removed, or the data plane keeps serving it. diff --git a/internal/controller/tlsroute_controller.go b/internal/controller/tlsroute_controller.go index 1346c3ef2..1ba9f4f55 100644 --- a/internal/controller/tlsroute_controller.go +++ b/internal/controller/tlsroute_controller.go @@ -55,6 +55,8 @@ type TLSRouteReconciler struct { //nolint:revive Provider provider.Provider + parentResolver routeParentResolver + Updater status.Updater Readier readiness.ReadinessManager @@ -86,7 +88,7 @@ func (r *TLSRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true @@ -253,6 +255,7 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c tr := new(gatewayv1.TLSRoute) if err := r.Get(ctx, req.NamespacedName, tr); err != nil { if client.IgnoreNotFound(err) == nil { + r.parentResolver.Forget(req.NamespacedName) tr.Namespace = req.Namespace tr.Name = req.Name @@ -283,17 +286,12 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c msg: "Route is accepted", } - gateways, unresolvedParents, err := ParseRouteParentRefs(ctx, r.Client, r.Log, tr, tr.Spec.ParentRefs) + gateways, err := r.parentResolver.Resolve(ctx, r.Client, r.Log, tr, tr.Spec.ParentRefs) if err != nil { return ctrl.Result{}, err } if len(gateways) == 0 { - if unresolvedParents { - // See the HTTPRoute reconciler: an unresolvable parentRef leaves ownership - // unknown rather than disproven, so the data plane must be left alone. - return ctrl.Result{}, nil - } // See the HTTPRoute reconciler: a route that no longer references a // Gateway managed by this controller must have its previously pushed // configuration removed, or the data plane keeps serving it. diff --git a/internal/controller/udproute_controller.go b/internal/controller/udproute_controller.go index a94759418..3f3255d41 100644 --- a/internal/controller/udproute_controller.go +++ b/internal/controller/udproute_controller.go @@ -55,6 +55,8 @@ type UDPRouteReconciler struct { //nolint:revive Provider provider.Provider + parentResolver routeParentResolver + Updater status.Updater Readier readiness.ReadinessManager @@ -86,7 +88,7 @@ func (r *UDPRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true @@ -253,6 +255,7 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c tr := new(gatewayv1.UDPRoute) if err := r.Get(ctx, req.NamespacedName, tr); err != nil { if client.IgnoreNotFound(err) == nil { + r.parentResolver.Forget(req.NamespacedName) tr.Namespace = req.Namespace tr.Name = req.Name @@ -283,17 +286,12 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c msg: "Route is accepted", } - gateways, unresolvedParents, err := ParseRouteParentRefs(ctx, r.Client, r.Log, tr, tr.Spec.ParentRefs) + gateways, err := r.parentResolver.Resolve(ctx, r.Client, r.Log, tr, tr.Spec.ParentRefs) if err != nil { return ctrl.Result{}, err } if len(gateways) == 0 { - if unresolvedParents { - // See the HTTPRoute reconciler: an unresolvable parentRef leaves ownership - // unknown rather than disproven, so the data plane must be left alone. - return ctrl.Result{}, nil - } // See the HTTPRoute reconciler: a route that no longer references a // Gateway managed by this controller must have its previously pushed // configuration removed, or the data plane keeps serving it. diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 732a7414b..fe8b8e92a 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -337,21 +337,15 @@ func parentRefTargetsListenerExplicitly(parentRef gatewayv1.ParentReference) boo return parentRef.Port != nil } -// ParseRouteParentRefs resolves the parentRefs of a route to the Gateways this -// controller manages. The second return value reports that at least one -// parentRef could not be resolved, because its Gateway or that Gateway's -// GatewayClass does not exist. An empty gateway list then means "ownership -// unknown", not "owned by another controller", and callers must not act on the -// route's data plane configuration. -func ParseRouteParentRefs( +func parseRouteParentRefs( ctx context.Context, mgrc client.Client, log logr.Logger, route client.Object, parentRefs []gatewayv1.ParentReference, -) ([]RouteParentRefContext, bool, error) { + resolveClass func(*gatewayv1.Gateway) (*gatewayv1.GatewayClass, error), +) ([]RouteParentRefContext, error) { gateways := make([]RouteParentRefContext, 0) - unresolved := false for _, parentRef := range parentRefs { namespace := route.GetNamespace() if parentRef.Namespace != nil { @@ -369,24 +363,18 @@ func ParseRouteParentRefs( Name: name, }, &gateway); err != nil { if client.IgnoreNotFound(err) == nil { - unresolved = true + // A deleted Gateway no longer authorizes its routes to serve traffic. continue } - return nil, false, fmt.Errorf("failed to retrieve gateway for route: %w", err) + return nil, fmt.Errorf("failed to retrieve gateway for route: %w", err) } - gatewayClass := gatewayv1.GatewayClass{} - if err := mgrc.Get(ctx, client.ObjectKey{ - Name: string(gateway.Spec.GatewayClassName), - }, &gatewayClass); err != nil { - if client.IgnoreNotFound(err) == nil { - unresolved = true - continue - } - return nil, false, fmt.Errorf("failed to retrieve gatewayclass for gateway: %w", err) + gatewayClass, err := resolveClass(&gateway) + if err != nil { + return nil, fmt.Errorf("failed to retrieve gatewayclass for gateway: %w", err) } - if string(gatewayClass.Spec.ControllerName) != config.ControllerConfig.ControllerName { + if gatewayClass == nil || string(gatewayClass.Spec.ControllerName) != config.ControllerConfig.ControllerName { continue } @@ -521,7 +509,7 @@ func ParseRouteParentRefs( } } - return gateways, unresolved, nil + return gateways, nil } // reuseUnchangedListenerStatus keeps the previously published status when diff --git a/internal/controller/utils_parentref_test.go b/internal/controller/utils_parentref_test.go index f6e1ffa48..5624e62c1 100644 --- a/internal/controller/utils_parentref_test.go +++ b/internal/controller/utils_parentref_test.go @@ -94,7 +94,7 @@ func TestParseRouteParentRefs_ReasonOrderIndependent(t *testing.T) { cli := fake.NewClientBuilder().WithScheme(scheme). WithObjects(newParentRefGatewayClass(), gw).Build() - got, _, err := ParseRouteParentRefs(context.Background(), cli, logr.Discard(), route, + got, err := (&routeParentResolver{}).Resolve(context.Background(), cli, logr.Discard(), route, []gatewayv1.ParentReference{{Name: "gw"}}) require.NoError(t, err) require.Len(t, got, 1) @@ -145,7 +145,7 @@ func TestParseRouteParentRefs_ExplicitListenerMatch(t *testing.T) { port8080 := gatewayv1.PortNumber(8080) t.Run("invalid explicit ref is not satisfied by another gateway's listener", func(t *testing.T) { - got, _, err := ParseRouteParentRefs(context.Background(), cli, logr.Discard(), route, + got, err := (&routeParentResolver{}).Resolve(context.Background(), cli, logr.Discard(), route, []gatewayv1.ParentReference{ // Explicit sectionName "web" on gw-a, which has no such listener. {Name: "gw-a", SectionName: &webSection}, @@ -171,7 +171,7 @@ func TestParseRouteParentRefs_ExplicitListenerMatch(t *testing.T) { }) t.Run("explicit sectionName on the owning gateway is an explicit match", func(t *testing.T) { - got, _, err := ParseRouteParentRefs(context.Background(), cli, logr.Discard(), route, + got, err := (&routeParentResolver{}).Resolve(context.Background(), cli, logr.Discard(), route, []gatewayv1.ParentReference{{Name: "gw-b", SectionName: &webSection}}) require.NoError(t, err) require.Len(t, got, 1) @@ -180,7 +180,7 @@ func TestParseRouteParentRefs_ExplicitListenerMatch(t *testing.T) { }) t.Run("explicit port on the owning gateway is an explicit match", func(t *testing.T) { - got, _, err := ParseRouteParentRefs(context.Background(), cli, logr.Discard(), route, + got, err := (&routeParentResolver{}).Resolve(context.Background(), cli, logr.Discard(), route, []gatewayv1.ParentReference{{Name: "gw-b", Port: &port8080}}) require.NoError(t, err) require.Len(t, got, 1) @@ -189,7 +189,7 @@ func TestParseRouteParentRefs_ExplicitListenerMatch(t *testing.T) { }) t.Run("implicit ref is not an explicit match", func(t *testing.T) { - got, _, err := ParseRouteParentRefs(context.Background(), cli, logr.Discard(), route, + got, err := (&routeParentResolver{}).Resolve(context.Background(), cli, logr.Discard(), route, []gatewayv1.ParentReference{{Name: "gw-b"}}) require.NoError(t, err) require.Len(t, got, 1) @@ -222,7 +222,7 @@ func TestParseRouteParentRefs_ConflictingTLSModePort(t *testing.T) { cli := fake.NewClientBuilder().WithScheme(scheme). WithObjects(newParentRefGatewayClass(), gw).Build() - got, _, err := ParseRouteParentRefs(context.Background(), cli, logr.Discard(), route, + got, err := (&routeParentResolver{}).Resolve(context.Background(), cli, logr.Discard(), route, []gatewayv1.ParentReference{{Name: "gw"}}) require.NoError(t, err) require.Len(t, got, 1) @@ -232,11 +232,7 @@ func TestParseRouteParentRefs_ConflictingTLSModePort(t *testing.T) { assert.Equal(t, string(gatewayv1.RouteReasonNotAllowedByListeners), cond.Reason) } -// TestParseRouteParentRefs_UnresolvedParents verifies the second return value -// distinguishes "no parentRef named this controller" from "a parentRef could not -// be resolved". Callers delete data plane configuration on an empty gateway list, -// which is only correct in the first case: a GatewayClass is cluster-scoped, so -// while one is missing every route under it resolves to an empty list at once. +// Unknown parents must not prevent cleanup of previously managed configuration. func TestParseRouteParentRefs_UnresolvedParents(t *testing.T) { scheme := parentRefTestScheme(t) @@ -259,46 +255,40 @@ func TestParseRouteParentRefs_UnresolvedParents(t *testing.T) { } for _, tc := range []struct { - name string - objects []client.Object - parentRef string - wantGateways int - wantUnresolved bool + name string + objects []client.Object + parentRef string + wantGateways int }{ { - name: "gateway missing", - objects: []client.Object{newParentRefGatewayClass()}, - parentRef: "gw", - wantUnresolved: true, + name: "gateway missing retracts stale routes", + objects: []client.Object{newParentRefGatewayClass()}, + parentRef: "gw", }, { - name: "gatewayclass missing", - objects: []client.Object{gw("gw", "apisix")}, - parentRef: "gw", - wantUnresolved: true, + name: "gatewayclass missing", + objects: []client.Object{gw("gw", "apisix")}, + parentRef: "gw", }, { - name: "gateway of another controller", - objects: []client.Object{foreignClass, gw("gw", "other")}, - parentRef: "gw", - wantUnresolved: false, + name: "gateway of another controller", + objects: []client.Object{foreignClass, gw("gw", "other")}, + parentRef: "gw", }, { - name: "gateway of this controller", - objects: []client.Object{newParentRefGatewayClass(), gw("gw", "apisix")}, - parentRef: "gw", - wantGateways: 1, - wantUnresolved: false, + name: "gateway of this controller", + objects: []client.Object{newParentRefGatewayClass(), gw("gw", "apisix")}, + parentRef: "gw", + wantGateways: 1, }, } { t.Run(tc.name, func(t *testing.T) { cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.objects...).Build() - got, unresolved, err := ParseRouteParentRefs(context.Background(), cli, logr.Discard(), route, + got, err := (&routeParentResolver{}).Resolve(context.Background(), cli, logr.Discard(), route, []gatewayv1.ParentReference{{Name: gatewayv1.ObjectName(tc.parentRef)}}) require.NoError(t, err) assert.Len(t, got, tc.wantGateways) - assert.Equal(t, tc.wantUnresolved, unresolved) }) } } diff --git a/internal/provider/api7ee/gateway_delete_test.go b/internal/provider/api7ee/gateway_delete_test.go new file mode 100644 index 000000000..5ecd3ac9c --- /dev/null +++ b/internal/provider/api7ee/gateway_delete_test.go @@ -0,0 +1,102 @@ +// 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 api7ee + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "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" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + adctypes "github.com/apache/apisix-ingress-controller/api/adc" + adcclient "github.com/apache/apisix-ingress-controller/internal/adc/client" + "github.com/apache/apisix-ingress-controller/internal/controller" + "github.com/apache/apisix-ingress-controller/internal/controller/label" + "github.com/apache/apisix-ingress-controller/internal/manager/readiness" + "github.com/apache/apisix-ingress-controller/internal/types" + "github.com/apache/apisix-ingress-controller/internal/utils" +) + +type gatewayDeleteReadier struct{ readiness.ReadinessManager } + +func (gatewayDeleteReadier) Done(client.Object, k8stypes.NamespacedName) {} + +func TestGatewayDeletionRetractsRouteWithUnknownSibling(t *testing.T) { + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(adctypes.SyncResult{Status: adctypes.StatusSuccess}) + })) + defer server.Close() + t.Setenv("ADC_SERVER_URL", server.URL) + adc, err := adcclient.New(logr.Discard(), "api7ee", time.Second) + require.NoError(t, err) + d := &api7eeProvider{client: adc, syncCh: make(chan struct{}, 1), log: logr.Discard()} + + gateway := &gatewayv1.Gateway{ + TypeMeta: metav1.TypeMeta{Kind: "Gateway", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "deleted"}, + } + route := &gatewayv1.HTTPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + Spec: gatewayv1.HTTPRouteSpec{CommonRouteSpec: gatewayv1.CommonRouteSpec{ + ParentRefs: []gatewayv1.ParentReference{{Name: "deleted"}, {Name: "unresolved"}}, + }}, + } + sibling := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "unresolved"}, + Spec: gatewayv1.GatewaySpec{GatewayClassName: "missing-class"}, + } + proxy := types.NamespacedNameKind{Kind: "GatewayProxy", Namespace: "default", Name: "proxy"} + configs := map[types.NamespacedNameKind]adctypes.Config{proxy: {Name: proxy.String(), BackendType: "apisix", ServerAddrs: []string{"http://apisix:9180"}}} + + for _, obj := range []client.Object{gateway, route} { + resources := &adctypes.Resources{} + resourceType := adctypes.TypeSSL + if obj == route { + resourceType = adctypes.TypeService + resources.Services = []*adctypes.Service{{Metadata: adctypes.Metadata{ID: "stale-route", Labels: label.GenLabel(route)}}} + } + require.NoError(t, adc.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(obj), Configs: configs, ResourceTypes: []string{resourceType}, Resources: resources, Labels: label.GenLabel(obj), + })) + } + + require.NoError(t, d.Delete(context.Background(), gateway)) + scheme := runtime.NewScheme() + require.NoError(t, gatewayv1.Install(scheme)) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(route, sibling).Build() + r := &controller.HTTPRouteReconciler{Client: cli, Scheme: scheme, Log: logr.Discard(), Provider: d, Readier: gatewayDeleteReadier{}} + _, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(route)}) + require.NoError(t, err) + resources, err := adc.GetResources(proxy.String()) + require.NoError(t, err) + require.Empty(t, resources.Services, "deleted Gateway's route survives reconciliation with an unresolved sibling") +} diff --git a/internal/provider/api7ee/provider.go b/internal/provider/api7ee/provider.go index 72cdcbea1..76438d1c4 100644 --- a/internal/provider/api7ee/provider.go +++ b/internal/provider/api7ee/provider.go @@ -205,7 +205,9 @@ func (d *api7eeProvider) Delete(ctx context.Context, obj client.Object) error { resourceTypes = append(resourceTypes, "service") labels = label.GenLabel(obj) case *gatewayv1.Gateway: - // delete all resources + // Gateways sharing a GatewayProxy only own their own certificates. + resourceTypes = append(resourceTypes, "ssl") + labels = label.GenLabel(obj) case *networkingv1.Ingress: resourceTypes = append(resourceTypes, "service", "ssl") labels = label.GenLabel(obj) @@ -225,12 +227,20 @@ func (d *api7eeProvider) Delete(ctx context.Context, obj client.Object) error { labels = label.GenLabel(obj) } - return d.client.Delete(ctx, adcclient.Task{ + if err := d.client.Delete(ctx, adcclient.Task{ Key: nnk, Name: nnk.String(), Labels: labels, ResourceTypes: resourceTypes, - }) + }); err != nil { + return err + } + if _, ok := obj.(*gatewayv1.Gateway); ok { + // The scoped sync above only covers SSLs; push the rest of the proxy snapshot, + // including the Gateway's evicted global rule and plugin metadata. + d.syncNotify() + } + return nil } func (d *api7eeProvider) Start(ctx context.Context) error { @@ -252,11 +262,14 @@ func (d *api7eeProvider) Start(ctx context.Context) error { }) } - if d.SyncPeriod < 1 { - return nil + // A zero SyncPeriod only disables the periodic sync; syncCh still carries changes + // that the scoped syncs cannot push, such as an evicted Gateway's plugins. + var tick <-chan time.Time + if d.SyncPeriod > 0 { + ticker := time.NewTicker(d.SyncPeriod) + defer ticker.Stop() + tick = ticker.C } - ticker := time.NewTicker(d.SyncPeriod) - defer ticker.Stop() retrier := common.NewRetrier(common.NewExponentialBackoff(RetryBaseDelay, RetryMaxDelay)) @@ -264,7 +277,7 @@ func (d *api7eeProvider) Start(ctx context.Context) error { select { case <-d.syncCh: case <-retrier.C(): - case <-ticker.C: + case <-tick: case <-ctx.Done(): return nil } diff --git a/internal/provider/api7ee/provider_test.go b/internal/provider/api7ee/provider_test.go index 8ef493656..67dd9b04b 100644 --- a/internal/provider/api7ee/provider_test.go +++ b/internal/provider/api7ee/provider_test.go @@ -19,8 +19,12 @@ package api7ee import ( "context" + "encoding/json" + "net/http" + "net/http/httptest" "strings" "testing" + "time" "github.com/go-logr/logr" "github.com/go-logr/logr/funcr" @@ -34,8 +38,12 @@ import ( adctypes "github.com/apache/apisix-ingress-controller/api/adc" "github.com/apache/apisix-ingress-controller/api/v1alpha1" apiv2 "github.com/apache/apisix-ingress-controller/api/v2" + adcclient "github.com/apache/apisix-ingress-controller/internal/adc/client" "github.com/apache/apisix-ingress-controller/internal/controller/label" + "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" + "github.com/apache/apisix-ingress-controller/internal/types" "github.com/apache/apisix-ingress-controller/internal/utils" ) @@ -133,3 +141,118 @@ func TestUpdateKeepsLastKnownGoodStateWhenL4PolicyCannotRender(t *testing.T) { require.Len(t, resources.Services, 1) assert.Equal(t, "last-known-good", resources.Services[0].Name) } + +func TestDeleteGatewayPreservesSharedProxyResources(t *testing.T) { + var received adcclient.ADCServerRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, json.NewDecoder(r.Body).Decode(&received)) + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(adctypes.SyncResult{Status: adctypes.StatusSuccess}) + })) + defer server.Close() + t.Setenv("ADC_SERVER_URL", server.URL) + cli, err := adcclient.New(logr.Discard(), "api7ee", time.Second) + require.NoError(t, err) + d := &api7eeProvider{client: cli, syncCh: make(chan struct{}, 1), log: logr.Discard()} + gateway := &gatewayv1.Gateway{ + TypeMeta: metav1.TypeMeta{Kind: "Gateway", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "deleted"}, + } + other := gateway.DeepCopy() + other.Name = "remaining" + route := &gatewayv1.HTTPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + } + proxy := types.NamespacedNameKind{Kind: "GatewayProxy", Namespace: "default", Name: "proxy"} + configs := map[types.NamespacedNameKind]adctypes.Config{proxy: { + Name: proxy.String(), BackendType: "apisix", ServerAddrs: []string{"http://apisix:9180"}, + }} + for _, gw := range []*gatewayv1.Gateway{gateway, other} { + labels := label.GenLabel(gw) + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(gw), Configs: configs, Labels: labels, ResourceTypes: []string{adctypes.TypeSSL}, + Resources: &adctypes.Resources{SSLs: []*adctypes.SSL{{Metadata: adctypes.Metadata{ID: gw.Name, Labels: labels}}}}, + })) + } + labels := label.GenLabel(route) + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(route), Configs: configs, Labels: labels, ResourceTypes: []string{adctypes.TypeService}, + Resources: &adctypes.Resources{Services: []*adctypes.Service{{Metadata: adctypes.Metadata{ID: "route", Labels: labels}}}}, + })) + require.NoError(t, d.Delete(context.Background(), gateway)) + resources, err := cli.GetResources(proxy.String()) + require.NoError(t, err) + require.Len(t, resources.SSLs, 1) + assert.Equal(t, "remaining", resources.SSLs[0].ID) + require.Len(t, resources.Services, 1) + assert.Equal(t, "route", resources.Services[0].ID) + assert.Equal(t, []string{adctypes.TypeSSL}, received.Task.Opts.IncludeResourceType) + require.Len(t, d.syncCh, 1, "Gateway deletion must schedule a full sync for the evicted plugins") + assert.Equal(t, map[string]string{label.LabelResourceKey: label.GenLabel(gateway)[label.LabelResourceKey]}, received.Task.Opts.LabelSelector) + _, err = cli.Sync(context.Background()) + require.NoError(t, err) + assert.Equal(t, resources.SSLs, received.Task.Config.SSLs) + assert.Equal(t, resources.Services, received.Task.Config.Services) + assert.Empty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(gateway))) + assert.NotEmpty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(other))) + assert.NotEmpty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(route))) +} + +func TestStartConsumesSyncNotificationsWithoutSyncPeriod(t *testing.T) { + requests := make(chan struct{}, 10) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(adctypes.SyncResult{Status: adctypes.StatusSuccess}) + requests <- struct{}{} + })) + defer server.Close() + t.Setenv("ADC_SERVER_URL", server.URL) + cli, err := adcclient.New(logr.Discard(), "api7ee", time.Second) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + readier := readiness.NewReadinessManager(nil, logr.Discard()) + require.NoError(t, readier.Start(ctx)) + d := &api7eeProvider{ + client: cli, readier: readier, updater: discardUpdater{}, syncCh: make(chan struct{}, 1), log: logr.Discard(), + } + + route := &gatewayv1.HTTPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + } + proxy := types.NamespacedNameKind{Kind: "GatewayProxy", Namespace: "default", Name: "proxy"} + labels := label.GenLabel(route) + require.NoError(t, cli.UpdateConfig(ctx, adcclient.Task{ + Key: utils.NamespacedNameKind(route), Labels: labels, ResourceTypes: []string{adctypes.TypeService}, + Configs: map[types.NamespacedNameKind]adctypes.Config{proxy: { + Name: proxy.String(), BackendType: "apisix", ServerAddrs: []string{"http://apisix:9180"}, + }}, + Resources: &adctypes.Resources{Services: []*adctypes.Service{{Metadata: adctypes.Metadata{ID: "route", Labels: labels}}}}, + })) + + done := make(chan error, 1) + go func() { done <- d.Start(ctx) }() + + select { + case <-requests: + case <-time.After(5 * time.Second): + t.Fatal("startup sync did not run") + } + d.syncNotify() + select { + case <-requests: + case err := <-done: + t.Fatalf("Start returned before consuming the notification: %v", err) + case <-time.After(5 * time.Second): + t.Fatal("sync notification was not consumed") + } + cancel() + require.NoError(t, <-done) +} + +type discardUpdater struct{} + +func (discardUpdater) Update(status.Update) {} diff --git a/internal/provider/apisix/gateway_delete_test.go b/internal/provider/apisix/gateway_delete_test.go new file mode 100644 index 000000000..dda6d0bbb --- /dev/null +++ b/internal/provider/apisix/gateway_delete_test.go @@ -0,0 +1,102 @@ +// 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 apisix + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "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" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + adctypes "github.com/apache/apisix-ingress-controller/api/adc" + adcclient "github.com/apache/apisix-ingress-controller/internal/adc/client" + "github.com/apache/apisix-ingress-controller/internal/controller" + "github.com/apache/apisix-ingress-controller/internal/controller/label" + "github.com/apache/apisix-ingress-controller/internal/manager/readiness" + "github.com/apache/apisix-ingress-controller/internal/types" + "github.com/apache/apisix-ingress-controller/internal/utils" +) + +type gatewayDeleteReadier struct{ readiness.ReadinessManager } + +func (gatewayDeleteReadier) Done(client.Object, k8stypes.NamespacedName) {} + +func TestGatewayDeletionRetractsRouteWithUnknownSibling(t *testing.T) { + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(adctypes.SyncResult{Status: adctypes.StatusSuccess}) + })) + defer server.Close() + t.Setenv("ADC_SERVER_URL", server.URL) + adc, err := adcclient.New(logr.Discard(), "apisix", time.Second) + require.NoError(t, err) + d := &apisixProvider{client: adc, syncCh: make(chan struct{}, 1), log: logr.Discard()} + + gateway := &gatewayv1.Gateway{ + TypeMeta: metav1.TypeMeta{Kind: "Gateway", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "deleted"}, + } + route := &gatewayv1.HTTPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + Spec: gatewayv1.HTTPRouteSpec{CommonRouteSpec: gatewayv1.CommonRouteSpec{ + ParentRefs: []gatewayv1.ParentReference{{Name: "deleted"}, {Name: "unresolved"}}, + }}, + } + sibling := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "unresolved"}, + Spec: gatewayv1.GatewaySpec{GatewayClassName: "missing-class"}, + } + proxy := types.NamespacedNameKind{Kind: "GatewayProxy", Namespace: "default", Name: "proxy"} + configs := map[types.NamespacedNameKind]adctypes.Config{proxy: {Name: proxy.String(), BackendType: "apisix", ServerAddrs: []string{"http://apisix:9180"}}} + + for _, obj := range []client.Object{gateway, route} { + resources := &adctypes.Resources{} + resourceType := adctypes.TypeSSL + if obj == route { + resourceType = adctypes.TypeService + resources.Services = []*adctypes.Service{{Metadata: adctypes.Metadata{ID: "stale-route", Labels: label.GenLabel(route)}}} + } + require.NoError(t, adc.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(obj), Configs: configs, ResourceTypes: []string{resourceType}, Resources: resources, Labels: label.GenLabel(obj), + })) + } + + require.NoError(t, d.Delete(context.Background(), gateway)) + scheme := runtime.NewScheme() + require.NoError(t, gatewayv1.Install(scheme)) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(route, sibling).Build() + r := &controller.HTTPRouteReconciler{Client: cli, Scheme: scheme, Log: logr.Discard(), Provider: d, Readier: gatewayDeleteReadier{}} + _, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(route)}) + require.NoError(t, err) + resources, err := adc.GetResources(proxy.String()) + require.NoError(t, err) + require.Empty(t, resources.Services, "deleted Gateway's route survives reconciliation with an unresolved sibling") +} diff --git a/internal/provider/apisix/provider.go b/internal/provider/apisix/provider.go index e6b8c04d1..addd9f517 100644 --- a/internal/provider/apisix/provider.go +++ b/internal/provider/apisix/provider.go @@ -202,7 +202,9 @@ func (d *apisixProvider) Delete(ctx context.Context, obj client.Object) error { resourceTypes = append(resourceTypes, adctypes.TypeService) labels = label.GenLabel(obj) case *gatewayv1.Gateway: - // delete all resources + // Gateways sharing a GatewayProxy only own their own certificates. + resourceTypes = append(resourceTypes, adctypes.TypeSSL) + labels = label.GenLabel(obj) case *networkingv1.Ingress: resourceTypes = append(resourceTypes, adctypes.TypeService, adctypes.TypeSSL) labels = label.GenLabel(obj) diff --git a/internal/provider/apisix/provider_test.go b/internal/provider/apisix/provider_test.go index 50d9e420c..55e6bb3b9 100644 --- a/internal/provider/apisix/provider_test.go +++ b/internal/provider/apisix/provider_test.go @@ -19,6 +19,9 @@ package apisix import ( "context" + "encoding/json" + "net/http" + "net/http/httptest" "strings" "testing" "time" @@ -170,3 +173,115 @@ func TestUpdateKeepsLastKnownGoodStateWhenL4PolicyCannotRender(t *testing.T) { require.Len(t, resources.Services, 1) assert.Equal(t, "last-known-good", resources.Services[0].Name) } + +func TestDeleteGatewayPreservesSharedProxyResources(t *testing.T) { + var received adcclient.ADCServerRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, json.NewDecoder(r.Body).Decode(&received)) + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(adctypes.SyncResult{Status: adctypes.StatusSuccess}) + })) + defer server.Close() + t.Setenv("ADC_SERVER_URL", server.URL) + cli, err := adcclient.New(logr.Discard(), ProviderTypeAPISIX, time.Second) + require.NoError(t, err) + d := &apisixProvider{client: cli, syncCh: make(chan struct{}, 1), log: logr.Discard()} + gateway := &gatewayv1.Gateway{ + TypeMeta: metav1.TypeMeta{Kind: "Gateway", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "deleted"}, + } + other := gateway.DeepCopy() + other.Name = "remaining" + route := &gatewayv1.HTTPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + } + proxy := types.NamespacedNameKind{Kind: "GatewayProxy", Namespace: "default", Name: "proxy"} + configs := map[types.NamespacedNameKind]adctypes.Config{proxy: { + Name: proxy.String(), BackendType: "apisix", ServerAddrs: []string{"http://apisix:9180"}, + }} + for _, gw := range []*gatewayv1.Gateway{gateway, other} { + labels := label.GenLabel(gw) + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(gw), Configs: configs, Labels: labels, ResourceTypes: []string{adctypes.TypeSSL}, + Resources: &adctypes.Resources{SSLs: []*adctypes.SSL{{Metadata: adctypes.Metadata{ID: gw.Name, Labels: labels}}}}, + })) + } + labels := label.GenLabel(route) + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(route), Configs: configs, Labels: labels, ResourceTypes: []string{adctypes.TypeService}, + Resources: &adctypes.Resources{Services: []*adctypes.Service{{Metadata: adctypes.Metadata{ID: "route", Labels: labels}}}}, + })) + require.NoError(t, d.Delete(context.Background(), gateway)) + resources, err := cli.GetResources(proxy.String()) + require.NoError(t, err) + require.Len(t, resources.SSLs, 1) + assert.Equal(t, "remaining", resources.SSLs[0].ID) + require.Len(t, resources.Services, 1) + assert.Equal(t, "route", resources.Services[0].ID) + require.Len(t, d.syncCh, 1, "Gateway deletion must schedule the remaining shared snapshot") + _, err = cli.Sync(context.Background()) + require.NoError(t, err) + assert.Equal(t, resources.SSLs, received.Task.Config.SSLs) + assert.Equal(t, resources.Services, received.Task.Config.Services) + assert.Empty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(gateway))) + assert.NotEmpty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(other))) + assert.NotEmpty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(route))) +} + +func TestDeleteGatewayClearsProxyPluginsWithLastReferrer(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(adctypes.SyncResult{Status: adctypes.StatusSuccess}) + })) + defer server.Close() + t.Setenv("ADC_SERVER_URL", server.URL) + proxy := types.NamespacedNameKind{Kind: "GatewayProxy", Namespace: "default", Name: "proxy"} + configs := map[types.NamespacedNameKind]adctypes.Config{proxy: { + Name: proxy.String(), BackendType: "apisix", ServerAddrs: []string{"http://apisix:9180"}, + }} + pluginTypes := []string{adctypes.TypeGlobalRule, adctypes.TypeSSL, adctypes.TypePluginMetadata} + plugins := &adctypes.Resources{ + GlobalRules: adctypes.GlobalRule{"cors": map[string]any{}}, + PluginMetadata: adctypes.PluginMetadata{"http-logger": map[string]any{}}, + } + for name, referrers := range map[string][]types.NamespacedNameKind{ + "last referrer": nil, + "IngressClass remains": {{Kind: "IngressClass", Name: "apisix"}}, + "another Gateway remains": {{Kind: "Gateway", Namespace: "default", Name: "remaining"}}, + } { + t.Run(name, func(t *testing.T) { + cli, err := adcclient.New(logr.Discard(), ProviderTypeAPISIX, time.Second) + require.NoError(t, err) + d := &apisixProvider{client: cli, syncCh: make(chan struct{}, 1), log: logr.Discard()} + gateway := &gatewayv1.Gateway{ + TypeMeta: metav1.TypeMeta{Kind: "Gateway", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "deleted"}, + } + keys := append(referrers, utils.NamespacedNameKind(gateway)) + for _, key := range keys { + labels := map[string]string{label.LabelKind: key.Kind, label.LabelNamespace: key.Namespace, label.LabelName: key.Name} + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: key, Configs: configs, Labels: labels, ResourceTypes: pluginTypes, Resources: plugins, + })) + } + route := types.NamespacedNameKind{Kind: "HTTPRoute", Namespace: "default", Name: "route"} + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: route, Configs: configs, ResourceTypes: []string{adctypes.TypeService}, Resources: &adctypes.Resources{}, + })) + + require.NoError(t, d.Delete(context.Background(), gateway)) + resources, err := cli.GetResources(proxy.String()) + require.NoError(t, err) + if len(referrers) == 0 { + assert.Empty(t, resources.GlobalRules, "an attached route must not keep the GatewayProxy's plugins alive") + assert.Empty(t, resources.PluginMetadata) + } else { + assert.Equal(t, plugins.GlobalRules, resources.GlobalRules) + assert.Equal(t, plugins.PluginMetadata, resources.PluginMetadata) + } + globalRules, err := cli.ListGlobalRules(proxy.String()) + require.NoError(t, err) + assert.Len(t, globalRules, len(referrers), "the deleted Gateway's own global rule must go") + }) + } +} diff --git a/internal/provider/common/configmanager.go b/internal/provider/common/configmanager.go index 4e1d529b3..9542400f1 100644 --- a/internal/provider/common/configmanager.go +++ b/internal/provider/common/configmanager.go @@ -18,6 +18,7 @@ package common import ( + "slices" "sync" ) @@ -124,6 +125,18 @@ func (s *ConfigManager[K, T]) Update( return discard } +// HasResource reports whether a resource accepted by match still references config. +func (s *ConfigManager[K, T]) HasResource(config K, match func(K) bool) bool { + s.mu.Lock() + defer s.mu.Unlock() + for resource, configKeys := range s.resourceConfigKeys { + if match(resource) && slices.Contains(configKeys, config) { + return true + } + } + return false +} + func (s *ConfigManager[K, T]) Set(key K, cfg T) { s.mu.Lock() defer s.mu.Unlock() diff --git a/test/conformance/conformance_test.go b/test/conformance/conformance_test.go index 223a0a73c..9d794ab9f 100644 --- a/test/conformance/conformance_test.go +++ b/test/conformance/conformance_test.go @@ -39,15 +39,8 @@ var skippedTestsForKnownGaps = []string{ tests.HTTPRouteListenerHostnameMatching.ShortName, tests.GRPCRouteListenerHostnameMatching.ShortName, - // A backendRef that cannot be resolved must still produce a route that - // answers 500; today no route is generated at all, so the request 404s. - // These consistently pass in standalone mode and fail against the APISIX - // admin API, and which member of the group trips is not stable between - // runs, so all four are skipped together rather than one at a time. + // Omitted backendRefs do not yet inject the required HTTP 500 response. tests.HTTPRouteNoBackendRefs.ShortName, - tests.HTTPRouteInvalidBackendRefUnknownKind.ShortName, - tests.HTTPRouteInvalidCrossNamespaceBackendRef.ShortName, - tests.HTTPRouteInvalidNonExistentBackendRef.ShortName, // Terminate mode itself is covered by TLSRouteListenerTerminateSupportedKinds // and by the e2e TLSRoute suite. This provisional test additionally requires a // standalone Gateway with no GatewayProxy attached to reach Accepted=True, and diff --git a/test/e2e/crds/v2/tls.go b/test/e2e/crds/v2/tls.go index 5a64cebc0..c12588e1a 100644 --- a/test/e2e/crds/v2/tls.go +++ b/test/e2e/crds/v2/tls.go @@ -131,13 +131,15 @@ spec: assert.ElementsMatch(GinkgoT(), []string{host}, tls[0].Snis) By("test HTTPS request to dataplane") - Eventually(func() int { - return s.NewAPISIXHttpsClient("api6.com"). - GET("/get"). - WithHost("api6.com"). - Expect(). - Raw().StatusCode - }).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(Equal(http.StatusOK)) + s.RequestAssert(&scaffold.RequestAssert{ + Client: s.NewAPISIXHttpsClient("api6.com"), + Method: "GET", + Path: "/get", + Host: "api6.com", + Check: scaffold.WithExpectedStatus(http.StatusOK), + Timeout: 30 * time.Second, + Interval: time.Second, + }) s.NewAPISIXHttpsClient("api6.com"). GET("/get"). diff --git a/test/e2e/ingress/annotations.go b/test/e2e/ingress/annotations.go index 622d0cb82..2f813c884 100644 --- a/test/e2e/ingress/annotations.go +++ b/test/e2e/ingress/annotations.go @@ -623,19 +623,24 @@ spec: Check: scaffold.WithExpectedStatus(http.StatusPermanentRedirect), }) - _ = s.NewAPISIXClient(). - GET("/get"). - WithHost("httpbin.example"). - Expect(). - Status(http.StatusMovedPermanently). - Header("Location").IsEqual("https://httpbin.example:9443/get") - - _ = s.NewAPISIXClient(). - GET("/ip"). - WithHost("httpbin.example"). - Expect(). - Status(http.StatusPermanentRedirect). - Header("Location").IsEqual("/anything/ip") + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "httpbin.example", + Checks: []scaffold.ResponseCheckFunc{ + scaffold.WithExpectedStatus(http.StatusMovedPermanently), + scaffold.WithExpectedHeader("Location", "https://httpbin.example:9443/get"), + }, + }) + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/ip", + Host: "httpbin.example", + Checks: []scaffold.ResponseCheckFunc{ + scaffold.WithExpectedStatus(http.StatusPermanentRedirect), + scaffold.WithExpectedHeader("Location", "/anything/ip"), + }, + }) }) It("csrf", func() { diff --git a/test/e2e/scaffold/apisix_deployer.go b/test/e2e/scaffold/apisix_deployer.go index a08b38851..8edcf1d9d 100644 --- a/test/e2e/scaffold/apisix_deployer.go +++ b/test/e2e/scaffold/apisix_deployer.go @@ -282,6 +282,12 @@ func (s *APISIXDeployer) deployDataplane(opts *APISIXDeployOptions) *corev1.Serv LabelSelector: "app.kubernetes.io/name=apisix", }) Expect(err).ToNot(HaveOccurred(), "waiting for gateway pod ready") + } else { + // A terminating pod can still be Ready and selected by a new port-forward. + err = k8s.WaitUntilNumPodsCreatedE(s.GinkgoT, kubectlOpts, metav1.ListOptions{ + LabelSelector: "app.kubernetes.io/name=apisix", + }, 0, 60, time.Second) + Expect(err).ToNot(HaveOccurred(), "waiting for gateway pods to terminate") } Eventually(func() bool {