diff --git a/internal/controller/grpcroute_controller.go b/internal/controller/grpcroute_controller.go index fdbc9a12..ef4d937a 100644 --- a/internal/controller/grpcroute_controller.go +++ b/internal/controller/grpcroute_controller.go @@ -280,6 +280,21 @@ func (r *GRPCRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + gr.TypeMeta = metav1.TypeMeta{ + Kind: KindGRPCRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, gr); err != nil { + r.Log.Error(err, "failed to delete grpcroute", "grpcroute", utils.NamespacedName(gr)) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index 7f3525b6..453e4358 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -306,6 +306,21 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + hr.TypeMeta = metav1.TypeMeta{ + Kind: KindHTTPRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, hr); err != nil { + r.Log.Error(err, "failed to delete httproute", "httproute", utils.NamespacedName(hr)) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/internal/controller/httproute_controller_retract_test.go b/internal/controller/httproute_controller_retract_test.go new file mode 100644 index 00000000..a1572c91 --- /dev/null +++ b/internal/controller/httproute_controller_retract_test.go @@ -0,0 +1,158 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package controller + +import ( + "context" + "errors" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + "github.com/apache/apisix-ingress-controller/api/v1alpha1" + "github.com/apache/apisix-ingress-controller/internal/controller/config" + "github.com/apache/apisix-ingress-controller/internal/manager/readiness" +) + +const ( + retractGatewayNamespace = "infra" + retractRouteNamespace = "tenant" + retractRouteName = "route" +) + +// newHTTPRouteRetractFixture builds a Gateway of our class in retractGatewayNamespace +// and an HTTPRoute in retractRouteNamespace that names it as its parent. from +// controls the listener's allowedRoutes, which is what revoking cross-namespace +// access changes. +func newHTTPRouteRetractFixture( + t *testing.T, + from gatewayv1.FromNamespaces, +) (*HTTPRouteReconciler, *recordingProvider) { + t.Helper() + + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, gatewayv1.Install(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + gatewayClass := &gatewayv1.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{Name: "apisix"}, + Spec: gatewayv1.GatewayClassSpec{ + ControllerName: gatewayv1.GatewayController(config.ControllerConfig.ControllerName), + }, + } + gateway := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Namespace: retractGatewayNamespace, Name: "gw"}, + Spec: gatewayv1.GatewaySpec{ + GatewayClassName: "apisix", + Listeners: []gatewayv1.Listener{{ + Name: "http", + Protocol: gatewayv1.HTTPProtocolType, + Port: 80, + AllowedRoutes: &gatewayv1.AllowedRoutes{ + Namespaces: &gatewayv1.RouteNamespaces{From: &from}, + }, + }}, + }, + } + route := &gatewayv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: retractRouteNamespace, Name: retractRouteName}, + Spec: gatewayv1.HTTPRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ + ParentRefs: []gatewayv1.ParentReference{{ + Name: gatewayv1.ObjectName(gateway.Name), + Namespace: (*gatewayv1.Namespace)(&gateway.Namespace), + }}, + }, + }, + } + + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects([]client.Object{gatewayClass, gateway, route}...). + WithStatusSubresource(route). + Build() + + readier := readiness.NewReadinessManager(cli, logr.Discard()) + require.NoError(t, readier.Start(context.Background())) + + prov := &recordingProvider{} + return &HTTPRouteReconciler{ + Client: cli, + Scheme: scheme, + Log: logr.Discard(), + Provider: prov, + Updater: &recordingUpdater{}, + Readier: readier, + }, prov +} + +func reconcileRetractHTTPRoute(t *testing.T, r *HTTPRouteReconciler) (ctrl.Result, error) { + t.Helper() + return r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: k8stypes.NamespacedName{Namespace: retractRouteNamespace, Name: retractRouteName}, + }) +} + +var retractRouteKey = k8stypes.NamespacedName{Namespace: retractRouteNamespace, Name: retractRouteName} + +// Narrowing a listener's allowedRoutes leaves the HTTPRoute in place but stops it +// being accepted. The configuration an earlier reconcile published must be +// retracted, otherwise the data plane keeps serving a route the Gateway no longer +// admits and only deleting the HTTPRoute clears it. +func TestHTTPRouteReconcile_RetractsWhenListenerStopsAllowingRoute(t *testing.T) { + r, prov := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromSame) + + result, err := reconcileRetractHTTPRoute(t, r) + + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + assert.Equal(t, []k8stypes.NamespacedName{retractRouteKey}, prov.deleted) + assert.Zero(t, prov.updated, "a route that is not accepted must not be published") +} + +// An accepted route must still be published and must not be retracted. +func TestHTTPRouteReconcile_PublishesAcceptedRoute(t *testing.T) { + r, prov := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromAll) + + _, err := reconcileRetractHTTPRoute(t, r) + + require.NoError(t, err) + assert.Empty(t, prov.deleted, "an accepted route must not be retracted") + assert.Equal(t, 1, prov.updated) +} + +// A provider failure while retracting must surface so the reconcile is retried. +func TestHTTPRouteReconcile_RetractErrorIsReturned(t *testing.T) { + r, prov := newHTTPRouteRetractFixture(t, gatewayv1.NamespacesFromSame) + prov.deleteErr = errors.New("provider unavailable") + + _, err := reconcileRetractHTTPRoute(t, r) + + require.Error(t, err) + assert.Contains(t, err.Error(), "provider unavailable") +} diff --git a/internal/controller/httproute_controller_test.go b/internal/controller/httproute_controller_test.go index 63762a49..68a21c13 100644 --- a/internal/controller/httproute_controller_test.go +++ b/internal/controller/httproute_controller_test.go @@ -32,6 +32,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + "github.com/apache/apisix-ingress-controller/internal/controller/status" "github.com/apache/apisix-ingress-controller/internal/manager/readiness" "github.com/apache/apisix-ingress-controller/internal/provider" ) @@ -114,6 +115,7 @@ func TestHTTPRouteReconcile_EmptyGateways(t *testing.T) { // recordingProvider records the objects passed to Delete and can be told to fail. type recordingProvider struct { + updated int deleted []k8stypes.NamespacedName deleteErr error } @@ -121,6 +123,7 @@ type recordingProvider struct { func (p *recordingProvider) Register(string, *http.ServeMux) {} func (p *recordingProvider) Update(context.Context, *provider.TranslateContext, client.Object) error { + p.updated++ return nil } @@ -132,3 +135,10 @@ func (p *recordingProvider) Delete(_ context.Context, obj client.Object) error { func (p *recordingProvider) Start(context.Context) error { return nil } func (p *recordingProvider) NeedLeaderElection() bool { return true } + +// recordingUpdater captures the status updates a reconciler would write. +type recordingUpdater struct { + updates []status.Update +} + +func (u *recordingUpdater) Update(update status.Update) { u.updates = append(u.updates, update) } diff --git a/internal/controller/tcproute_controller.go b/internal/controller/tcproute_controller.go index 4cf16ebe..d543988e 100644 --- a/internal/controller/tcproute_controller.go +++ b/internal/controller/tcproute_controller.go @@ -371,6 +371,21 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + tr.TypeMeta = metav1.TypeMeta{ + Kind: KindTCPRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, tr); err != nil { + r.Log.Error(err, "failed to delete tcproute", "tcproute", utils.NamespacedName(tr)) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/internal/controller/tlsroute_controller.go b/internal/controller/tlsroute_controller.go index c9df0e62..e62abe99 100644 --- a/internal/controller/tlsroute_controller.go +++ b/internal/controller/tlsroute_controller.go @@ -363,6 +363,21 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + tr.TypeMeta = metav1.TypeMeta{ + Kind: types.KindTLSRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, tr); err != nil { + r.Log.Error(err, "failed to delete tlsroute", "tlsroute", utils.NamespacedName(tr)) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/internal/controller/udproute_controller.go b/internal/controller/udproute_controller.go index 2c8a910b..e1c0f4d9 100644 --- a/internal/controller/udproute_controller.go +++ b/internal/controller/udproute_controller.go @@ -371,6 +371,21 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err } + return ctrl.Result{}, nil + } + + // The route still resolves to one of our Gateways but no parent accepts it any + // more, so retract what an earlier reconcile published. The store is what every + // sync pushes, so leaving the entry keeps the data plane serving the route. + // Provider.Delete derives the resource labels from the object Kind, which is not + // set on every object read through the client. + tr.TypeMeta = metav1.TypeMeta{ + Kind: KindUDPRoute, + APIVersion: gatewayv1.GroupVersion.String(), + } + if err := r.Provider.Delete(ctx, tr); err != nil { + r.Log.Error(err, "failed to delete udproute", "udproute", utils.NamespacedName(tr)) + return ctrl.Result{}, err } return ctrl.Result{}, nil } diff --git a/test/e2e/gatewayapi/httproute.go b/test/e2e/gatewayapi/httproute.go index 094113e1..046a871d 100644 --- a/test/e2e/gatewayapi/httproute.go +++ b/test/e2e/gatewayapi/httproute.go @@ -205,6 +205,128 @@ spec: }) }) + Context("HTTPRoute revoked by its listener", func() { + // The listener starts out admitting HTTPRoute and is then narrowed to + // GRPCRoute only. The route object is untouched throughout, which is the + // point: revoking a route's access must not require editing the route. + var gatewayAllowingKinds = ` +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: %s +spec: + gatewayClassName: %s + listeners: + - name: http1 + protocol: HTTP + port: 80 + allowedRoutes: + kinds: + - group: gateway.networking.k8s.io + kind: %s + infrastructure: + parametersRef: + group: apisix.apache.org + kind: GatewayProxy + name: apisix-proxy-config +` + + var route = ` +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: httpbin +spec: + parentRefs: + - name: %s + hostnames: + - httpbin.example + rules: + - matches: + - path: + type: Exact + value: /get + backendRefs: + - name: httpbin-service-e2e-test + port: 80 +` + + // allowKind rewrites the listener to admit only the given route kind. + var allowKind = func(kind string) { + Expect(s.CreateResourceFromString( + fmt.Sprintf(gatewayAllowingKinds, s.Namespace(), s.Namespace(), kind), + )).NotTo(HaveOccurred(), "applying Gateway allowing "+kind) + } + + BeforeEach(func() { + By("create GatewayProxy") + Expect(s.CreateResourceFromString(s.GetGatewayProxySpec())).NotTo(HaveOccurred(), "creating GatewayProxy") + + By("create GatewayClass") + Expect(s.CreateResourceFromString(s.GetGatewayClassYaml())).NotTo(HaveOccurred(), "creating GatewayClass") + s.RetryAssertion(func() string { + gcyaml, _ := s.GetResourceYaml("GatewayClass", s.Namespace()) + return gcyaml + }).Should(ContainSubstring("message: the gatewayclass has been accepted by the apisix-ingress-controller"), + "check GatewayClass condition") + + By("create Gateway admitting HTTPRoute") + allowKind("HTTPRoute") + s.RetryAssertion(func() string { + gwyaml, _ := s.GetResourceYaml("Gateway", s.Namespace()) + return gwyaml + }).Should(ContainSubstring("message: the gateway has been accepted by the apisix-ingress-controller"), + "check Gateway condition status") + }) + + It("stops serving the route and resumes when the listener admits it again", func() { + By("create HTTPRoute") + s.ResourceApplied("HTTPRoute", "httpbin", fmt.Sprintf(route, s.Namespace()), 1) + + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "httpbin.example", + Check: scaffold.WithExpectedStatus(http.StatusOK), + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + + By("narrow the listener to GRPCRoute, leaving the HTTPRoute untouched") + allowKind("GRPCRoute") + + By("the route reports that no listener accepts it") + s.RetryAssertion(func() string { + routeYaml, _ := s.GetResourceYaml("HTTPRoute", "httpbin") + return routeYaml + }).Should(ContainSubstring("reason: NotAllowedByListeners"), "check HTTPRoute condition") + + By("and the data plane stops serving it") + // Without the retraction the previously published route keeps + // forwarding, so the status and the data plane disagree until the + // HTTPRoute itself is deleted. + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "httpbin.example", + Check: scaffold.WithExpectedStatus(http.StatusNotFound), + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + + By("restore the listener and the route is served again") + allowKind("HTTPRoute") + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "httpbin.example", + Check: scaffold.WithExpectedStatus(http.StatusOK), + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + }) + }) + Context("HTTPRoute with Multiple Gateway", Serial, func() { var additionalGatewayGroupID string var additionalSvc *corev1.Service