diff --git a/internal/controller/apisixroute_controller.go b/internal/controller/apisixroute_controller.go index ca281416..e6d8b484 100644 --- a/internal/controller/apisixroute_controller.go +++ b/internal/controller/apisixroute_controller.go @@ -176,6 +176,21 @@ func (r *ApisixRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, client.IgnoreNotFound(err) } if err = r.processApisixRoute(tctx, &ar); err != nil { + // A reference the route needs is gone, so the route can no longer be + // translated. Retract what an earlier reconcile published: the store is what + // every sync pushes, so leaving it in place keeps the data plane serving the + // last good configuration while the status says the spec is invalid. + if types.IsDependencyMissing(err) { + if derr := r.Provider.Delete(ctx, &ar); derr != nil { + r.Log.Error(derr, "failed to delete apisixroute", "apisixroute", utils.NamespacedName(&ar)) + return ctrl.Result{}, derr + } + // The deferred updateStatus still reports the reason. Returning the + // error as well would requeue forever with backoff: the reference does + // not come back on its own, and the ApisixPluginConfig watch reconciles + // the route again when it does. + return ctrl.Result{}, nil + } return ctrl.Result{}, err } if err = r.Provider.Update(ctx, tctx, &ar); err != nil { @@ -304,10 +319,15 @@ func (r *ApisixRouteReconciler) validatePluginConfig(tctx *provider.TranslateCon pcNN = utils.NamespacedName(&pc) ) if err := r.Get(tctx, pcNN, &pc); err != nil { - return types.ReasonError{ - Reason: string(apiv2.ConditionReasonInvalidSpec), - Message: fmt.Sprintf("failed to get ApisixPluginConfig: %s", pcNN), + if !k8serrors.IsNotFound(err) { + // A read failure is transient: retry it rather than reporting the + // reference as invalid and retracting the route. + return err } + return types.DependencyMissingError{Err: types.ReasonError{ + Reason: string(apiv2.ConditionReasonInvalidSpec), + Message: fmt.Sprintf("ApisixPluginConfig not found: %s", pcNN), + }} } // Check if ApisixPluginConfig has IngressClassName and if it matches diff --git a/internal/controller/ingress_controller.go b/internal/controller/ingress_controller.go index 26c93fab..402f2770 100644 --- a/internal/controller/ingress_controller.go +++ b/internal/controller/ingress_controller.go @@ -26,6 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" networkingv1 "k8s.io/api/networking/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -210,6 +211,20 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // process plugin config annotation if err := r.processPluginConfig(tctx, ingress); err != nil { r.Log.Error(err, "failed to process PluginConfig annotation", "ingress", ingress.Name) + // The referenced ApisixPluginConfig is gone, so the Ingress can no longer be + // translated. Retract what an earlier reconcile published: the store is what + // every sync pushes, so leaving it in place keeps the data plane applying the + // deleted plugin configuration. + if internaltypes.IsDependencyMissing(err) { + if derr := r.Provider.Delete(ctx, ingress); derr != nil { + r.Log.Error(derr, "failed to delete ingress", "ingress", utils.NamespacedName(ingress)) + return ctrl.Result{}, derr + } + // Requeueing would retry forever with backoff for a reference that does + // not come back on its own; the ApisixPluginConfig watch reconciles the + // Ingress again when it does. + return ctrl.Result{}, nil + } return ctrl.Result{}, err } @@ -670,6 +685,9 @@ func (r *IngressReconciler) processPluginConfig(tctx *provider.TranslateContext, if err := r.Get(tctx, pcNN, &pc); err != nil { r.Log.Error(err, "failed to get ApisixPluginConfig", "pluginconfig", pcNN) + if k8serrors.IsNotFound(err) { + return internaltypes.DependencyMissingError{Err: err} + } return err } diff --git a/internal/controller/pluginconfig_retract_test.go b/internal/controller/pluginconfig_retract_test.go new file mode 100644 index 00000000..47b205ae --- /dev/null +++ b/internal/controller/pluginconfig_retract_test.go @@ -0,0 +1,284 @@ +// 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" + "net/http" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + 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" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + "github.com/apache/apisix-ingress-controller/internal/provider" + + "github.com/apache/apisix-ingress-controller/api/v1alpha1" + apiv2 "github.com/apache/apisix-ingress-controller/api/v2" + "github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations" + "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/controller/status" + "github.com/apache/apisix-ingress-controller/internal/manager/readiness" +) + +const ( + retractPluginConfigNamespace = "default" + retractPluginConfigName = "shared" +) + +// pluginConfigProvider and pluginConfigUpdater are this file's own stubs. Upstream +// shares recordingProvider and recordingUpdater across the reconciler tests; the +// copy of recordingProvider here counts Update calls, which the shared one does +// not, and there is no shared updater stub to reuse. Fold them together when the +// upstream test scaffolding is backported. +type pluginConfigProvider struct { + updated int + deleted []k8stypes.NamespacedName + deleteErr error +} + +func (p *pluginConfigProvider) Register(string, *http.ServeMux) {} + +func (p *pluginConfigProvider) Update(context.Context, *provider.TranslateContext, client.Object) error { + p.updated++ + return nil +} + +func (p *pluginConfigProvider) Delete(_ context.Context, obj client.Object) error { + p.deleted = append(p.deleted, k8stypes.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}) + return p.deleteErr +} + +func (p *pluginConfigProvider) Start(context.Context) error { return nil } + +func (p *pluginConfigProvider) NeedLeaderElection() bool { return true } + +type pluginConfigUpdater struct { + updates []status.Update +} + +func (u *pluginConfigUpdater) Update(update status.Update) { u.updates = append(u.updates, update) } + +func retractPluginConfigScheme(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, apiv2.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + return scheme +} + +func retractIngressClass() *networkingv1.IngressClass { + return &networkingv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{Name: "apisix"}, + Spec: networkingv1.IngressClassSpec{Controller: config.GetControllerName()}, + } +} + +func newRetractReadier(t *testing.T, cli client.Client) readiness.ReadinessManager { + t.Helper() + readier := readiness.NewReadinessManager(cli, logr.Discard()) + require.NoError(t, readier.Start(context.Background())) + return readier +} + +// failGetOn makes Get fail with a non-NotFound error for objects of type T, so a +// transient read failure can be told apart from an absent reference. +func failGetOn[T client.Object]() interceptor.Funcs { + return interceptor.Funcs{ + Get: func(ctx context.Context, cli client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(T); ok { + return k8serrors.NewInternalError(errors.New("boom")) + } + return cli.Get(ctx, key, obj, opts...) + }, + } +} + +func newApisixRoutePluginConfigFixture( + t *testing.T, + interceptorFuncs interceptor.Funcs, + extraObjects ...client.Object, +) (*ApisixRouteReconciler, *pluginConfigProvider) { + t.Helper() + + scheme := retractPluginConfigScheme(t) + route := &apiv2.ApisixRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: retractPluginConfigNamespace, Name: "route"}, + Spec: apiv2.ApisixRouteSpec{ + IngressClassName: "apisix", + HTTP: []apiv2.ApisixRouteHTTP{{ + Name: "rule", + PluginConfigName: retractPluginConfigName, + Match: apiv2.ApisixRouteHTTPMatch{Hosts: []string{"repro.test"}, Paths: []string{"/*"}}, + Backends: []apiv2.ApisixRouteHTTPBackend{{ + ServiceName: "backend", + ServicePort: intstr.FromInt32(80), + }}, + }}, + }, + } + + objects := append([]client.Object{retractIngressClass(), route}, extraObjects...) + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(objects...). + WithStatusSubresource(route). + WithInterceptorFuncs(interceptorFuncs). + Build() + + prov := &pluginConfigProvider{} + return &ApisixRouteReconciler{ + Client: cli, + Scheme: scheme, + Log: logr.Discard(), + Provider: prov, + Updater: &pluginConfigUpdater{}, + Readier: newRetractReadier(t, cli), + }, prov +} + +func newIngressPluginConfigFixture( + t *testing.T, + interceptorFuncs interceptor.Funcs, + extraObjects ...client.Object, +) (*IngressReconciler, *pluginConfigProvider) { + t.Helper() + + scheme := retractPluginConfigScheme(t) + ingress := &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: retractPluginConfigNamespace, + Name: "ing", + Annotations: map[string]string{annotations.AnnotationsPluginConfigName: retractPluginConfigName}, + }, + Spec: networkingv1.IngressSpec{IngressClassName: ptrTo("apisix")}, + } + + objects := append([]client.Object{retractIngressClass(), ingress}, extraObjects...) + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(objects...). + WithStatusSubresource(ingress). + WithInterceptorFuncs(interceptorFuncs). + // 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() + + prov := &pluginConfigProvider{} + return &IngressReconciler{ + Client: cli, + Scheme: scheme, + Log: logr.Discard(), + Provider: prov, + Updater: &pluginConfigUpdater{}, + Readier: newRetractReadier(t, cli), + }, prov +} + +func ptrTo[T any](v T) *T { return &v } + +var ( + retractApisixRouteKey = k8stypes.NamespacedName{Namespace: retractPluginConfigNamespace, Name: "route"} + retractIngressKey = k8stypes.NamespacedName{Namespace: retractPluginConfigNamespace, Name: "ing"} +) + +// Deleting a shared ApisixPluginConfig leaves the referencing ApisixRoute in place +// but untranslatable. Its published configuration must be retracted, otherwise the +// data plane keeps applying the deleted plugins while the status reports the spec +// as invalid, and only deleting the route itself clears it. +func TestApisixRouteReconcile_RetractsWhenPluginConfigIsMissing(t *testing.T) { + r, prov := newApisixRoutePluginConfigFixture(t, interceptor.Funcs{}) + + result, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: retractApisixRouteKey}) + + // No error and no requeue: the reference does not come back on its own, so + // retrying it forever with backoff only produces log noise. The + // ApisixPluginConfig watch reconciles the route again when it returns. + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + assert.Equal(t, []k8stypes.NamespacedName{retractApisixRouteKey}, prov.deleted) + assert.Zero(t, prov.updated) +} + +// A read failure that is not NotFound is transient. Retracting on it would drop a +// working route because the API server hiccuped. +func TestApisixRouteReconcile_KeepsRouteWhenPluginConfigReadFails(t *testing.T) { + r, prov := newApisixRoutePluginConfigFixture(t, failGetOn[*apiv2.ApisixPluginConfig]()) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: retractApisixRouteKey}) + + require.Error(t, err) + assert.True(t, k8serrors.IsInternalError(err), "want the transient error to surface, got %v", err) + assert.Empty(t, prov.deleted, "a transient read failure must not retract the route") +} + +// The same applies to an Ingress that names the plugin config through its +// annotation. +func TestIngressReconcile_RetractsWhenPluginConfigIsMissing(t *testing.T) { + r, prov := newIngressPluginConfigFixture(t, interceptor.Funcs{}) + + result, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: retractIngressKey}) + + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + assert.Equal(t, []k8stypes.NamespacedName{retractIngressKey}, prov.deleted) + assert.Zero(t, prov.updated) +} + +func TestIngressReconcile_KeepsIngressWhenPluginConfigReadFails(t *testing.T) { + r, prov := newIngressPluginConfigFixture(t, failGetOn[*apiv2.ApisixPluginConfig]()) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: retractIngressKey}) + + require.Error(t, err) + assert.True(t, k8serrors.IsInternalError(err), "want the transient error to surface, got %v", err) + assert.Empty(t, prov.deleted, "a transient read failure must not retract the Ingress") +} + +// With the plugin config present the resources must still be published. +func TestReconcile_PublishesWhenPluginConfigExists(t *testing.T) { + pc := &apiv2.ApisixPluginConfig{ + ObjectMeta: metav1.ObjectMeta{Namespace: retractPluginConfigNamespace, Name: retractPluginConfigName}, + } + + ar, arProv := newApisixRoutePluginConfigFixture(t, interceptor.Funcs{}, pc.DeepCopy()) + _, err := ar.Reconcile(context.Background(), ctrl.Request{NamespacedName: retractApisixRouteKey}) + require.NoError(t, err) + assert.Empty(t, arProv.deleted) + assert.Equal(t, 1, arProv.updated) + + ing, ingProv := newIngressPluginConfigFixture(t, interceptor.Funcs{}, pc.DeepCopy()) + _, err = ing.Reconcile(context.Background(), ctrl.Request{NamespacedName: retractIngressKey}) + require.NoError(t, err) + assert.Empty(t, ingProv.deleted) + assert.Equal(t, 1, ingProv.updated) +} diff --git a/internal/types/error.go b/internal/types/error.go index 1388637d..6b757fb8 100644 --- a/internal/types/error.go +++ b/internal/types/error.go @@ -37,6 +37,29 @@ func (e ReasonError) Error() string { return e.Message } +// DependencyMissingError marks a validation failure caused by a referenced object +// that is absent rather than temporarily unreadable. The distinction matters when +// deciding what to do with configuration already published for the owner: an +// absent reference will not come back on its own, so keeping the last good +// configuration leaves the data plane contradicting the Accepted=False status +// written alongside it, while a transient read failure must be retried instead. +type DependencyMissingError struct { + Err error +} + +func (e DependencyMissingError) Error() string { + return e.Err.Error() +} + +func (e DependencyMissingError) Unwrap() error { + return e.Err +} + +func IsDependencyMissing(err error) bool { + var dme DependencyMissingError + return errors.As(err, &dme) +} + func IsSomeReasonError[Reason ~string](err error, reasons ...Reason) bool { if err == nil { return false diff --git a/test/e2e/crds/v2/pluginconfig.go b/test/e2e/crds/v2/pluginconfig.go index 13867f1b..8112a1ef 100644 --- a/test/e2e/crds/v2/pluginconfig.go +++ b/test/e2e/crds/v2/pluginconfig.go @@ -115,6 +115,91 @@ spec: Eventually(request).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(Equal(http.StatusNotFound)) }) + It("Test ApisixRoute stops serving when its ApisixPluginConfig is deleted", func() { + const pluginConfigSpec = ` +apiVersion: apisix.apache.org/v2 +kind: ApisixPluginConfig +metadata: + name: shared-plugin-config +spec: + ingressClassName: %s + plugins: + - name: response-rewrite + enable: true + config: + headers: + X-Revocation-Test: "must-disappear" +` + + const routeSpec = ` +apiVersion: apisix.apache.org/v2 +kind: ApisixRoute +metadata: + name: referencing-route +spec: + ingressClassName: %s + http: + - name: rule0 + match: + paths: + - /* + backends: + - serviceName: httpbin-service-e2e-test + servicePort: 80 + plugin_config_name: shared-plugin-config +` + + applyPluginConfig := func() { + var pluginConfig apiv2.ApisixPluginConfig + applier.MustApplyAPIv2(types.NamespacedName{Namespace: s.Namespace(), Name: "shared-plugin-config"}, + &pluginConfig, fmt.Sprintf(pluginConfigSpec, s.Namespace())) + } + + By("apply ApisixPluginConfig and a route that references it") + applyPluginConfig() + var apisixRoute apiv2.ApisixRoute + applier.MustApplyAPIv2(types.NamespacedName{Namespace: s.Namespace(), Name: "referencing-route"}, + &apisixRoute, fmt.Sprintf(routeSpec, s.Namespace())) + + By("the plugin takes effect") + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Checks: []scaffold.ResponseCheckFunc{ + scaffold.WithExpectedStatus(http.StatusOK), + scaffold.WithExpectedHeader("X-Revocation-Test", "must-disappear"), + }, + }) + + By("delete only the ApisixPluginConfig, leaving the route in place") + Expect(s.DeleteResource("ApisixPluginConfig", "shared-plugin-config")). + ShouldNot(HaveOccurred(), "deleting ApisixPluginConfig") + + By("the route stops being served") + // Without the retraction the route keeps forwarding and keeps applying the + // deleted plugin, so the header would still come back with a 200. + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Check: scaffold.WithExpectedStatus(http.StatusNotFound), + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + + By("recreating the ApisixPluginConfig under the same name restores the route") + applyPluginConfig() + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Checks: []scaffold.ResponseCheckFunc{ + scaffold.WithExpectedStatus(http.StatusOK), + scaffold.WithExpectedHeader("X-Revocation-Test", "must-disappear"), + }, + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + }) + It("Test ApisixPluginConfig update", func() { const apisixPluginConfigSpecV1 = ` apiVersion: apisix.apache.org/v2 diff --git a/test/e2e/ingress/annotations.go b/test/e2e/ingress/annotations.go index 1a522c9b..622d0cb8 100644 --- a/test/e2e/ingress/annotations.go +++ b/test/e2e/ingress/annotations.go @@ -751,6 +751,91 @@ spec: Expect(err).NotTo(HaveOccurred(), "unmarshalling echo plugin config") Expect(echoConfig["body"]).To(Equal("hello from plugin config"), "checking echo plugin body") }) + It("stops serving when the referenced ApisixPluginConfig is deleted", func() { + pluginConfig := ` +apiVersion: apisix.apache.org/v2 +kind: ApisixPluginConfig +metadata: + name: revoked-plugin-config +spec: + ingressClassName: %s + plugins: + - name: response-rewrite + enable: true + config: + headers: + X-Revocation-Test: "must-disappear" +` + ingressWithPluginConfig := ` +apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + name: revoked-plugin-config-test + annotations: + k8s.apisix.apache.org/plugin-config-name: "revoked-plugin-config" +spec: + ingressClassName: %s + rules: + - host: revoked-plugin-config.example + http: + paths: + - path: /get + pathType: Exact + backend: + service: + name: httpbin-service-e2e-test + port: + number: 80 +` + applyPluginConfig := func() { + Expect(s.CreateResourceFromString(fmt.Sprintf(pluginConfig, s.Namespace()))). + ShouldNot(HaveOccurred(), "creating ApisixPluginConfig") + } + + applyPluginConfig() + Expect(s.CreateResourceFromString(fmt.Sprintf(ingressWithPluginConfig, s.Namespace()))). + ShouldNot(HaveOccurred(), "creating Ingress") + + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "revoked-plugin-config.example", + Checks: []scaffold.ResponseCheckFunc{ + scaffold.WithExpectedStatus(http.StatusOK), + scaffold.WithExpectedHeader("X-Revocation-Test", "must-disappear"), + }, + }) + + By("delete only the ApisixPluginConfig, leaving the Ingress in place") + Expect(s.DeleteResource("ApisixPluginConfig", "revoked-plugin-config")). + ShouldNot(HaveOccurred(), "deleting ApisixPluginConfig") + + // Without the retraction the Ingress keeps forwarding and keeps applying + // the deleted plugin, so the header would still come back with a 200. + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "revoked-plugin-config.example", + Check: scaffold.WithExpectedStatus(http.StatusNotFound), + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + + By("recreating it under the same name restores the Ingress") + applyPluginConfig() + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "revoked-plugin-config.example", + Checks: []scaffold.ResponseCheckFunc{ + scaffold.WithExpectedStatus(http.StatusOK), + scaffold.WithExpectedHeader("X-Revocation-Test", "must-disappear"), + }, + Timeout: time.Second * 30, + Interval: time.Second * 2, + }) + }) + It("methods", func() { Expect(s.CreateResourceFromString(fmt.Sprintf(allowMethods, s.Namespace()))).ShouldNot(HaveOccurred(), "creating Ingress") Expect(s.CreateResourceFromString(fmt.Sprintf(blockMethods, s.Namespace()))).ShouldNot(HaveOccurred(), "creating Ingress")