From cf11a8054354716a9782a75cf863e7a17a6812eb Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 15 Sep 2026 21:29:27 +0545 Subject: [PATCH 1/2] fix: reject invalid L4RoutePolicy plugin configs --- internal/adc/translator/l4route_test.go | 61 +++++ internal/adc/translator/l4routepolicy_test.go | 27 ++- internal/adc/translator/policies.go | 16 +- internal/adc/translator/tcproute.go | 14 +- internal/adc/translator/tlsroute.go | 4 +- internal/adc/translator/udproute.go | 6 +- .../l4routepolicy_invalid_config_test.go | 213 ++++++++++++++++++ internal/controller/policies.go | 59 ++++- internal/controller/tcproute_controller.go | 6 +- internal/controller/tlsroute_controller.go | 6 +- internal/controller/udproute_controller.go | 6 +- 11 files changed, 386 insertions(+), 32 deletions(-) create mode 100644 internal/controller/l4routepolicy_invalid_config_test.go diff --git a/internal/adc/translator/l4route_test.go b/internal/adc/translator/l4route_test.go index 62d0ca03..dc8494b6 100644 --- a/internal/adc/translator/l4route_test.go +++ b/internal/adc/translator/l4route_test.go @@ -273,6 +273,67 @@ func TestTranslateTLSRouteWithL4RoutePolicy(t *testing.T) { } } +func TestTranslateL4RouteRejectsInvalidPolicyPluginConfig(t *testing.T) { + for _, tt := range []struct { + name string + routeKind string + routeName string + translate func(*Translator, *provider.TranslateContext) (*TranslateResult, error) + }{ + { + name: "TCPRoute", + routeKind: "TCPRoute", + routeName: "my-tcp", + translate: func(tr *Translator, tctx *provider.TranslateContext) (*TranslateResult, error) { + return tr.TranslateTCPRoute(tctx, &gatewayv1.TCPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "my-tcp"}, + Spec: gatewayv1.TCPRouteSpec{Rules: []gatewayv1.TCPRouteRule{{}}}, + }) + }, + }, + { + name: "UDPRoute", + routeKind: "UDPRoute", + routeName: "my-udp", + translate: func(tr *Translator, tctx *provider.TranslateContext) (*TranslateResult, error) { + return tr.TranslateUDPRoute(tctx, &gatewayv1.UDPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "my-udp"}, + Spec: gatewayv1.UDPRouteSpec{Rules: []gatewayv1.UDPRouteRule{{}}}, + }) + }, + }, + { + name: "TLSRoute", + routeKind: "TLSRoute", + routeName: "my-tls", + translate: func(tr *Translator, tctx *provider.TranslateContext) (*TranslateResult, error) { + return tr.TranslateTLSRoute(tctx, &gatewayv1.TLSRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "my-tls"}, + Spec: gatewayv1.TLSRouteSpec{ + Hostnames: []gatewayv1.Hostname{"example.com"}, + Rules: []gatewayv1.TLSRouteRule{{}}, + }, + }) + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + tr := NewTranslator(logr.Discard(), "") + tctx := provider.NewDefaultTranslateContext(context.Background()) + policy := makeL4RoutePolicy("default", "invalid-policy", tt.routeKind, tt.routeName, []v1alpha1.Plugin{ + {Name: "ip-restriction", Config: mustJSON([]string{"not-an-object"})}, + }) + tctx.L4RoutePolicies[k8stypes.NamespacedName{Namespace: policy.Namespace, Name: policy.Name}] = policy + + result, err := tt.translate(tr, tctx) + + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), `plugin "ip-restriction"`) + }) + } +} + func TestTranslateTCPRouteUpstreamScheme(t *testing.T) { const ( namespace = "default" diff --git a/internal/adc/translator/l4routepolicy_test.go b/internal/adc/translator/l4routepolicy_test.go index 289dab98..6a41f1e9 100644 --- a/internal/adc/translator/l4routepolicy_test.go +++ b/internal/adc/translator/l4routepolicy_test.go @@ -23,6 +23,7 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8stypes "k8s.io/apimachinery/pkg/types" @@ -74,7 +75,7 @@ func TestAttachL4RoutePolicyPlugins_AttachesMatchingPolicy(t *testing.T) { } plugins := adctypes.Plugins{} - tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil) + require.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Len(t, plugins, 2) assert.Contains(t, plugins, "limit-conn") @@ -97,7 +98,7 @@ func TestAttachL4RoutePolicyPlugins_NoMatchOnKind(t *testing.T) { plugins := adctypes.Plugins{} // Looking for TCPRoute, but policy targets UDPRoute — should not match. - tr.AttachL4RoutePolicyPlugins(policies, "default", "my-udp-route", "TCPRoute", plugins, nil) + require.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-udp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } @@ -115,7 +116,7 @@ func TestAttachL4RoutePolicyPlugins_NoMatchOnNamespace(t *testing.T) { plugins := adctypes.Plugins{} // Route is in "default" namespace, policy is in "other-ns" — should not match. - tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil) + require.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } @@ -130,7 +131,7 @@ func TestAttachL4RoutePolicyPlugins_EmptyPlugins(t *testing.T) { } plugins := adctypes.Plugins{} - tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil) + require.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } @@ -138,6 +139,22 @@ func TestAttachL4RoutePolicyPlugins_EmptyPlugins(t *testing.T) { func TestAttachL4RoutePolicyPlugins_EmptyPolicies(t *testing.T) { tr := NewTranslator(logr.Discard(), "") plugins := adctypes.Plugins{} - tr.AttachL4RoutePolicyPlugins(nil, "default", "my-tcp-route", "TCPRoute", plugins, nil) + require.NoError(t, tr.AttachL4RoutePolicyPlugins(nil, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } + +func TestAttachL4RoutePolicyPlugins_ReturnsInvalidConfigError(t *testing.T) { + tr := NewTranslator(logr.Discard(), "") + policy := makeL4RoutePolicy("default", "invalid-policy", "TCPRoute", "my-tcp-route", []v1alpha1.Plugin{ + {Name: "ip-restriction", Config: mustJSON([]string{"not-an-object"})}, + }) + policies := map[k8stypes.NamespacedName]*v1alpha1.L4RoutePolicy{ + {Namespace: policy.Namespace, Name: policy.Name}: policy, + } + + err := tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", adctypes.Plugins{}, nil) + + require.Error(t, err) + assert.Contains(t, err.Error(), `plugin "ip-restriction"`) + assert.Contains(t, err.Error(), "invalid-policy") +} diff --git a/internal/adc/translator/policies.go b/internal/adc/translator/policies.go index b1a815b4..dd05c77a 100644 --- a/internal/adc/translator/policies.go +++ b/internal/adc/translator/policies.go @@ -18,6 +18,8 @@ package translator import ( + "fmt" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -229,9 +231,9 @@ func (t *Translator) AttachL4RoutePolicyPlugins( routeNamespace, routeName, routeKind string, plugins adctypes.Plugins, secrets map[types.NamespacedName]*corev1.Secret, -) { +) error { if len(policies) == 0 { - return + return nil } for _, policy := range policies { if policy.Namespace != routeNamespace { @@ -252,19 +254,19 @@ func (t *Translator) AttachL4RoutePolicyPlugins( if ref.SectionName != nil && *ref.SectionName != "" { continue } - t.mergeL4PolicyPlugins(policy, plugins, secrets) - return + return t.mergeL4PolicyPlugins(policy, plugins, secrets) } } + return nil } -func (t *Translator) mergeL4PolicyPlugins(policy *v1alpha1.L4RoutePolicy, plugins adctypes.Plugins, secrets map[types.NamespacedName]*corev1.Secret) { +func (t *Translator) mergeL4PolicyPlugins(policy *v1alpha1.L4RoutePolicy, plugins adctypes.Plugins, secrets map[types.NamespacedName]*corev1.Secret) error { for _, plugin := range policy.Spec.Plugins { cfg, err := renderPluginConfig(plugin, policy.Namespace, secrets) if err != nil { - t.Log.Error(err, "failed to render L4RoutePolicy plugin config", "plugin", plugin.Name, "policy", policy.Name) - continue + return fmt.Errorf("failed to render plugin %q from L4RoutePolicy %s/%s: %w", plugin.Name, policy.Namespace, policy.Name, err) } plugins[plugin.Name] = cfg } + return nil } diff --git a/internal/adc/translator/tcproute.go b/internal/adc/translator/tcproute.go index 7de4a67e..a57f316a 100644 --- a/internal/adc/translator/tcproute.go +++ b/internal/adc/translator/tcproute.go @@ -67,7 +67,7 @@ func listenerPortSet(tctx *provider.TranslateContext) map[int32]struct{} { // the match to work, so injection is opt-in (explicit sectionName/port targeting, // or more than one listener port). When it is not injected we keep the previous // single portless StreamRoute, preserving backward compatibility. -func (t *Translator) buildL4StreamRoutes(tctx *provider.TranslateContext, namespace, name string, ruleIndex int, typ, routeKind string, labels map[string]string) []*adctypes.StreamRoute { +func (t *Translator) buildL4StreamRoutes(tctx *provider.TranslateContext, namespace, name string, ruleIndex int, typ, routeKind string, labels map[string]string) ([]*adctypes.StreamRoute, error) { var ports []int32 if portSet := listenerPortSet(tctx); t.shouldInjectServerPortVars(tctx.HasExplicitListenerMatch, portSet) { ports = make([]int32, 0, len(portSet)) @@ -98,10 +98,12 @@ func (t *Translator) buildL4StreamRoutes(tctx *provider.TranslateContext, namesp // Attach L4RoutePolicy plugins at the stream_route level: the APISIX stream proxy // applies plugins from the stream_route, not from the service. streamRoute.Plugins = make(adctypes.Plugins) - t.AttachL4RoutePolicyPlugins(tctx.L4RoutePolicies, namespace, name, routeKind, streamRoute.Plugins, tctx.Secrets) + if err := t.AttachL4RoutePolicyPlugins(tctx.L4RoutePolicies, namespace, name, routeKind, streamRoute.Plugins, tctx.Secrets); err != nil { + return nil, err + } streamRoutes = append(streamRoutes, streamRoute) } - return streamRoutes + return streamRoutes, nil } func (t *Translator) TranslateTCPRoute(tctx *provider.TranslateContext, tcpRoute *gatewayv1.TCPRoute) (*TranslateResult, error) { @@ -212,7 +214,11 @@ func (t *Translator) TranslateTCPRoute(tctx *provider.TranslateContext, tcpRoute } } // TODO: support remote_addr, server_addr, sni - service.StreamRoutes = t.buildL4StreamRoutes(tctx, tcpRoute.Namespace, tcpRoute.Name, ruleIndex, "TCP", "TCPRoute", labels) + streamRoutes, err := t.buildL4StreamRoutes(tctx, tcpRoute.Namespace, tcpRoute.Name, ruleIndex, "TCP", "TCPRoute", labels) + if err != nil { + return nil, err + } + service.StreamRoutes = streamRoutes result.Services = append(result.Services, service) } diff --git a/internal/adc/translator/tlsroute.go b/internal/adc/translator/tlsroute.go index 8d1fd0a6..022bdef2 100644 --- a/internal/adc/translator/tlsroute.go +++ b/internal/adc/translator/tlsroute.go @@ -154,7 +154,9 @@ func (t *Translator) TranslateTLSRoute(tctx *provider.TranslateContext, tlsRoute // applies plugins from the stream_route, not from the service. With multiple SNIs // each stream_route carries its own copy of the plugins. streamRoute.Plugins = make(adctypes.Plugins) - t.AttachL4RoutePolicyPlugins(tctx.L4RoutePolicies, tlsRoute.Namespace, tlsRoute.Name, "TLSRoute", streamRoute.Plugins, tctx.Secrets) + if err := t.AttachL4RoutePolicyPlugins(tctx.L4RoutePolicies, tlsRoute.Namespace, tlsRoute.Name, "TLSRoute", streamRoute.Plugins, tctx.Secrets); err != nil { + return nil, err + } service.StreamRoutes = append(service.StreamRoutes, streamRoute) } diff --git a/internal/adc/translator/udproute.go b/internal/adc/translator/udproute.go index 6e6e23ac..7706aa60 100644 --- a/internal/adc/translator/udproute.go +++ b/internal/adc/translator/udproute.go @@ -139,7 +139,11 @@ func (t *Translator) TranslateUDPRoute(tctx *provider.TranslateContext, udpRoute } } // TODO: support remote_addr, server_addr, sni - service.StreamRoutes = t.buildL4StreamRoutes(tctx, udpRoute.Namespace, udpRoute.Name, ruleIndex, "UDP", "UDPRoute", labels) + streamRoutes, err := t.buildL4StreamRoutes(tctx, udpRoute.Namespace, udpRoute.Name, ruleIndex, "UDP", "UDPRoute", labels) + if err != nil { + return nil, err + } + service.StreamRoutes = streamRoutes result.Services = append(result.Services, service) } diff --git a/internal/controller/l4routepolicy_invalid_config_test.go b/internal/controller/l4routepolicy_invalid_config_test.go new file mode 100644 index 00000000..5642c690 --- /dev/null +++ b/internal/controller/l4routepolicy_invalid_config_test.go @@ -0,0 +1,213 @@ +// 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" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + 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" + + "github.com/apache/apisix-ingress-controller/api/v1alpha1" + "github.com/apache/apisix-ingress-controller/internal/controller/config" + "github.com/apache/apisix-ingress-controller/internal/controller/indexer" + "github.com/apache/apisix-ingress-controller/internal/controller/status" + "github.com/apache/apisix-ingress-controller/internal/provider" +) + +func TestL4RouteReconcileRejectsInvalidPolicyPluginConfig(t *testing.T) { + const ( + namespace = "default" + routeName = "route" + policyName = "policy" + ) + + tests := []struct { + name string + routeKind gatewayv1.Kind + protocol gatewayv1.ProtocolType + newRoute func() client.Object + reconcile func(client.Client, provider.Provider, status.Updater) error + parentRefs func(client.Object) []gatewayv1.RouteParentStatus + }{ + { + name: "TCPRoute", + routeKind: "TCPRoute", + protocol: gatewayv1.TCPProtocolType, + newRoute: func() client.Object { + return &gatewayv1.TCPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: routeName}, + Spec: gatewayv1.TCPRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ParentRefs: []gatewayv1.ParentReference{{Name: "gw"}}}, + Rules: []gatewayv1.TCPRouteRule{{}}, + }, + } + }, + reconcile: func(cli client.Client, prov provider.Provider, updater status.Updater) error { + r := &TCPRouteReconciler{Client: cli, Log: logr.Discard(), Provider: prov, Updater: updater, Readier: noopReadier{}, supportsL4RoutePolicy: true} + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: k8stypes.NamespacedName{Namespace: namespace, Name: routeName}}) + return err + }, + parentRefs: func(obj client.Object) []gatewayv1.RouteParentStatus { + return obj.(*gatewayv1.TCPRoute).Status.Parents + }, + }, + { + name: "UDPRoute", + routeKind: "UDPRoute", + protocol: gatewayv1.UDPProtocolType, + newRoute: func() client.Object { + return &gatewayv1.UDPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: routeName}, + Spec: gatewayv1.UDPRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ParentRefs: []gatewayv1.ParentReference{{Name: "gw"}}}, + Rules: []gatewayv1.UDPRouteRule{{}}, + }, + } + }, + reconcile: func(cli client.Client, prov provider.Provider, updater status.Updater) error { + r := &UDPRouteReconciler{Client: cli, Log: logr.Discard(), Provider: prov, Updater: updater, Readier: noopReadier{}, supportsL4RoutePolicy: true} + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: k8stypes.NamespacedName{Namespace: namespace, Name: routeName}}) + return err + }, + parentRefs: func(obj client.Object) []gatewayv1.RouteParentStatus { + return obj.(*gatewayv1.UDPRoute).Status.Parents + }, + }, + { + name: "TLSRoute", + routeKind: "TLSRoute", + protocol: gatewayv1.TLSProtocolType, + newRoute: func() client.Object { + return &gatewayv1.TLSRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: routeName}, + Spec: gatewayv1.TLSRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ParentRefs: []gatewayv1.ParentReference{{Name: "gw"}}}, + Hostnames: []gatewayv1.Hostname{"example.com"}, + Rules: []gatewayv1.TLSRouteRule{{}}, + }, + } + }, + reconcile: func(cli client.Client, prov provider.Provider, updater status.Updater) error { + r := &TLSRouteReconciler{Client: cli, Log: logr.Discard(), Provider: prov, Updater: updater, Readier: noopReadier{}, supportsL4RoutePolicy: true} + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: k8stypes.NamespacedName{Namespace: namespace, Name: routeName}}) + return err + }, + parentRefs: func(obj client.Object) []gatewayv1.RouteParentStatus { + return obj.(*gatewayv1.TLSRoute).Status.Parents + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + 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: namespace, Name: "gw"}, + Spec: gatewayv1.GatewaySpec{ + GatewayClassName: "apisix", + Listeners: []gatewayv1.Listener{{ + Name: "listener", + Protocol: tt.protocol, + Port: 9000, + }}, + }, + } + route := tt.newRoute() + policy := &v1alpha1.L4RoutePolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: policyName, Generation: 1}, + Spec: v1alpha1.L4RoutePolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReferenceWithSectionName{{ + LocalPolicyTargetReference: gatewayv1.LocalPolicyTargetReference{ + Group: gatewayv1.GroupName, + Kind: tt.routeKind, + Name: routeName, + }, + }}, + Plugins: []v1alpha1.Plugin{{ + Name: "ip-restriction", + Config: apiextensionsv1.JSON{Raw: []byte(`["should-not-appear"]`)}, + }}, + }, + } + + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(gatewayClass, gateway, route, policy). + WithIndex(&v1alpha1.L4RoutePolicy{}, indexer.PolicyTargetRefs, indexer.L4RoutePolicyIndexFunc). + Build() + prov := &recordingProvider{} + updater := &recordingUpdater{} + + require.NoError(t, tt.reconcile(cli, prov, updater)) + assert.Zero(t, prov.updated, "an invalid policy must keep the existing provider state") + assert.Empty(t, prov.deleted) + + var gotPolicyStatus *v1alpha1.L4RoutePolicy + var gotRouteStatus client.Object + for _, update := range updater.updates { + switch update.Resource.(type) { + case *v1alpha1.L4RoutePolicy: + gotPolicyStatus = update.Mutator.Mutate(policy.DeepCopy()).(*v1alpha1.L4RoutePolicy) + default: + gotRouteStatus = update.Mutator.Mutate(route.DeepCopyObject().(client.Object)) + } + } + + require.NotNil(t, gotPolicyStatus) + require.Len(t, gotPolicyStatus.Status.Ancestors, 1) + require.Len(t, gotPolicyStatus.Status.Ancestors[0].Conditions, 1) + condition := gotPolicyStatus.Status.Ancestors[0].Conditions[0] + assert.Equal(t, metav1.ConditionFalse, condition.Status) + assert.Equal(t, string(gatewayv1.PolicyReasonInvalid), condition.Reason) + assert.Contains(t, condition.Message, `plugin "ip-restriction" has an invalid configuration`) + assert.NotContains(t, condition.Message, "should-not-appear") + + require.NotNil(t, gotRouteStatus) + parents := tt.parentRefs(gotRouteStatus) + require.Len(t, parents, 1) + var accepted *metav1.Condition + for i := range parents[0].Conditions { + if parents[0].Conditions[i].Type == string(gatewayv1.RouteConditionAccepted) { + accepted = &parents[0].Conditions[i] + break + } + } + require.NotNil(t, accepted) + assert.Equal(t, metav1.ConditionTrue, accepted.Status) + }) + } +} diff --git a/internal/controller/policies.go b/internal/controller/policies.go index 1cfb5c7f..34b09aff 100644 --- a/internal/controller/policies.go +++ b/internal/controller/policies.go @@ -19,6 +19,8 @@ package controller import ( "context" + "encoding/json" + "errors" "fmt" "slices" "sort" @@ -52,6 +54,21 @@ type PolicyTargetKey struct { SectionName string } +type invalidL4RoutePolicyError struct { + err error +} + +func (e *invalidL4RoutePolicyError) Error() string { return e.err.Error() } +func (e *invalidL4RoutePolicyError) Unwrap() error { return e.err } + +func l4RoutePolicyReconcileError(err error) error { + var invalidErr *invalidL4RoutePolicyError + if errors.As(err, &invalidErr) { + return nil + } + return err +} + func (p PolicyTargetKey) String() string { return p.NsName.String() + "/" + p.GroupKind.String() + "/" + p.SectionName } @@ -267,15 +284,15 @@ func ProcessL4RoutePolicy( log logr.Logger, tctx *provider.TranslateContext, routeNamespace, routeName, routeKind string, -) { +) error { var list v1alpha1.L4RoutePolicyList key := indexer.GenIndexKeyWithGK(gatewayv1.GroupName, routeKind, routeNamespace, routeName) if err := c.List(tctx, &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil { log.Error(err, "failed to list L4RoutePolicy", "namespace", routeNamespace, "name", routeName, "kind", routeKind) - return + return err } if len(list.Items) == 0 { - return + return nil } // L4 routes have no addressable sections; a targetRef that specifies a sectionName @@ -284,7 +301,7 @@ func ProcessL4RoutePolicy( return !l4RoutePolicyMatchesRoute(p, routeKind, routeNamespace, routeName) }) if len(list.Items) == 0 { - return + return nil } // Deterministic conflict resolution: oldest creationTimestamp wins; tie-break by namespace/name. @@ -300,11 +317,14 @@ func ProcessL4RoutePolicy( }) winner := list.Items[0].DeepCopy() - // A policy whose Secrets cannot be read is not attached at all, so a route is never - // programmed with a subset of the plugins the policy asks for. - secretErr := loadPluginSecrets(tctx, c, tctx, winner.Namespace, winner.Spec.Plugins) - if secretErr != nil { - log.Error(secretErr, "failed to load Secrets referenced by L4RoutePolicy plugins", "policy", types.NamespacedName{Namespace: winner.Namespace, Name: winner.Name}) + // An invalid policy is not attached at all, so a route is never programmed with + // a subset of the plugins the policy asks for. + policyErr := validateL4RoutePolicyPluginConfigs(winner) + if policyErr == nil { + policyErr = loadPluginSecrets(tctx, c, tctx, winner.Namespace, winner.Spec.Plugins) + } + if policyErr != nil { + log.Error(policyErr, "failed to process L4RoutePolicy plugins", "policy", types.NamespacedName{Namespace: winner.Namespace, Name: winner.Name}) } else { tctx.L4RoutePolicies[types.NamespacedName{Namespace: winner.Namespace, Name: winner.Name}] = winner } @@ -312,14 +332,14 @@ func ProcessL4RoutePolicy( for i := range list.Items { policy := list.Items[i] var condition metav1.Condition - if i == 0 && secretErr != nil { + if i == 0 && policyErr != nil { condition = metav1.Condition{ Type: string(gatewayv1.PolicyConditionAccepted), Status: metav1.ConditionFalse, ObservedGeneration: policy.GetGeneration(), LastTransitionTime: metav1.Now(), Reason: string(gatewayv1.PolicyReasonInvalid), - Message: secretErr.Error(), + Message: policyErr.Error(), } } else if i == 0 { condition = metav1.Condition{ @@ -356,6 +376,23 @@ func ProcessL4RoutePolicy( }) } } + if policyErr != nil { + return &invalidL4RoutePolicyError{err: policyErr} + } + return nil +} + +func validateL4RoutePolicyPluginConfigs(policy *v1alpha1.L4RoutePolicy) error { + for _, plugin := range policy.Spec.Plugins { + if len(plugin.Config.Raw) == 0 { + continue + } + var config map[string]any + if err := json.Unmarshal(plugin.Config.Raw, &config); err != nil { + return fmt.Errorf("plugin %q has an invalid configuration: %w", plugin.Name, err) + } + } + return nil } // updateL4RoutePolicyStatusOnDeleting removes the deleted route's ancestor status entries diff --git a/internal/controller/tcproute_controller.go b/internal/controller/tcproute_controller.go index b8d6f97e..aed1707a 100644 --- a/internal/controller/tcproute_controller.go +++ b/internal/controller/tcproute_controller.go @@ -345,8 +345,9 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } ProcessBackendTrafficPolicy(r.Client, r.Log, tctx) + var l4RoutePolicyErr error if r.supportsL4RoutePolicy { - ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindTCPRoute) + l4RoutePolicyErr = ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindTCPRoute) } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { @@ -377,6 +378,9 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { + if l4RoutePolicyErr != nil { + return ctrl.Result{}, l4RoutePolicyReconcileError(l4RoutePolicyErr) + } routeToUpdate := tr if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err diff --git a/internal/controller/tlsroute_controller.go b/internal/controller/tlsroute_controller.go index 5bbd5a59..15efaa6b 100644 --- a/internal/controller/tlsroute_controller.go +++ b/internal/controller/tlsroute_controller.go @@ -337,8 +337,9 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } ProcessBackendTrafficPolicy(r.Client, r.Log, tctx) + var l4RoutePolicyErr error if r.supportsL4RoutePolicy { - ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, types.KindTLSRoute) + l4RoutePolicyErr = ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, types.KindTLSRoute) } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { @@ -369,6 +370,9 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { + if l4RoutePolicyErr != nil { + return ctrl.Result{}, l4RoutePolicyReconcileError(l4RoutePolicyErr) + } routeToUpdate := tr if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err diff --git a/internal/controller/udproute_controller.go b/internal/controller/udproute_controller.go index 3ca88907..bd9df870 100644 --- a/internal/controller/udproute_controller.go +++ b/internal/controller/udproute_controller.go @@ -345,8 +345,9 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } ProcessBackendTrafficPolicy(r.Client, r.Log, tctx) + var l4RoutePolicyErr error if r.supportsL4RoutePolicy { - ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindUDPRoute) + l4RoutePolicyErr = ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindUDPRoute) } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { @@ -377,6 +378,9 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { + if l4RoutePolicyErr != nil { + return ctrl.Result{}, l4RoutePolicyReconcileError(l4RoutePolicyErr) + } routeToUpdate := tr if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err From 4c0dec1909a88a9dd7c8d3f681a1564199774ad2 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 23 Sep 2026 13:41:59 +0545 Subject: [PATCH 2/2] fix: share L4 plugin configuration rendering --- internal/adc/translator/l4route_test.go | 101 ++++----- internal/adc/translator/l4routepolicy_test.go | 27 +-- internal/adc/translator/plugin.go | 28 +-- .../l4routepolicy_invalid_config_test.go | 213 ------------------ internal/controller/l4routepolicy_test.go | 82 +++++++ internal/controller/policies.go | 69 ++---- internal/controller/tcproute_controller.go | 6 +- internal/controller/tlsroute_controller.go | 6 +- internal/controller/udproute_controller.go | 6 +- internal/pluginconfig/renderer.go | 55 +++++ internal/provider/api7ee/provider_test.go | 74 ++++++ internal/provider/apisix/provider_test.go | 71 ++++++ 12 files changed, 352 insertions(+), 386 deletions(-) delete mode 100644 internal/controller/l4routepolicy_invalid_config_test.go create mode 100644 internal/controller/l4routepolicy_test.go create mode 100644 internal/pluginconfig/renderer.go diff --git a/internal/adc/translator/l4route_test.go b/internal/adc/translator/l4route_test.go index dc8494b6..5ac129da 100644 --- a/internal/adc/translator/l4route_test.go +++ b/internal/adc/translator/l4route_test.go @@ -43,6 +43,7 @@ func TestTranslateTCPRouteWithL4RoutePolicy(t *testing.T) { policy *v1alpha1.L4RoutePolicy wantPlugins []string wantNoPlugins bool + wantErr bool }{ { name: "attaches plugins from matching L4RoutePolicy", @@ -52,6 +53,13 @@ func TestTranslateTCPRouteWithL4RoutePolicy(t *testing.T) { }), wantPlugins: []string{"limit-conn", "ip-restriction"}, }, + { + name: "rejects a policy with a non-object plugin config", + policy: makeL4RoutePolicy("default", "tcp-policy", "TCPRoute", "my-tcp", []v1alpha1.Plugin{ + {Name: "ip-restriction", Config: mustJSON([]string{"10.0.0.0/8"})}, + }), + wantErr: true, + }, { name: "does not attach plugins from policy targeting different route kind", policy: makeL4RoutePolicy("default", "udp-policy", "UDPRoute", "my-tcp", []v1alpha1.Plugin{ @@ -96,6 +104,11 @@ func TestTranslateTCPRouteWithL4RoutePolicy(t *testing.T) { } result, err := translator.TranslateTCPRoute(tctx, route) + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, result) + return + } require.NoError(t, err) require.Len(t, result.Services, 1) require.NotEmpty(t, result.Services[0].StreamRoutes) @@ -118,6 +131,7 @@ func TestTranslateUDPRouteWithL4RoutePolicy(t *testing.T) { policy *v1alpha1.L4RoutePolicy wantPlugins []string wantNoPlugins bool + wantErr bool }{ { name: "attaches plugins from matching L4RoutePolicy", @@ -126,6 +140,13 @@ func TestTranslateUDPRouteWithL4RoutePolicy(t *testing.T) { }), wantPlugins: []string{"limit-conn"}, }, + { + name: "rejects a policy with a non-object plugin config", + policy: makeL4RoutePolicy("default", "udp-policy", "UDPRoute", "my-udp", []v1alpha1.Plugin{ + {Name: "ip-restriction", Config: mustJSON("10.0.0.0/8")}, + }), + wantErr: true, + }, { name: "does not attach plugins from policy targeting TCPRoute", policy: makeL4RoutePolicy("default", "tcp-policy", "TCPRoute", "my-udp", []v1alpha1.Plugin{ @@ -163,6 +184,11 @@ func TestTranslateUDPRouteWithL4RoutePolicy(t *testing.T) { } result, err := translator.TranslateUDPRoute(tctx, route) + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, result) + return + } require.NoError(t, err) require.Len(t, result.Services, 1) require.NotEmpty(t, result.Services[0].StreamRoutes) @@ -186,6 +212,7 @@ func TestTranslateTLSRouteWithL4RoutePolicy(t *testing.T) { hostnames []string wantPlugins []string wantNoPlugins bool + wantErr bool }{ { name: "attaches plugins from matching L4RoutePolicy", @@ -195,6 +222,14 @@ func TestTranslateTLSRouteWithL4RoutePolicy(t *testing.T) { hostnames: []string{"example.com"}, wantPlugins: []string{"ip-restriction"}, }, + { + name: "rejects a policy with a non-object plugin config", + policy: makeL4RoutePolicy("default", "tls-policy", "TLSRoute", "my-tls", []v1alpha1.Plugin{ + {Name: "ip-restriction", Config: mustJSON(true)}, + }), + hostnames: []string{"example.com"}, + wantErr: true, + }, { name: "plugins attached once per rule even with multiple SNI hostnames", policy: makeL4RoutePolicy("default", "tls-policy", "TLSRoute", "my-tls", []v1alpha1.Plugin{ @@ -248,6 +283,11 @@ func TestTranslateTLSRouteWithL4RoutePolicy(t *testing.T) { } result, err := translator.TranslateTLSRoute(tctx, route) + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, result) + return + } require.NoError(t, err) require.Len(t, result.Services, 1) @@ -273,67 +313,6 @@ func TestTranslateTLSRouteWithL4RoutePolicy(t *testing.T) { } } -func TestTranslateL4RouteRejectsInvalidPolicyPluginConfig(t *testing.T) { - for _, tt := range []struct { - name string - routeKind string - routeName string - translate func(*Translator, *provider.TranslateContext) (*TranslateResult, error) - }{ - { - name: "TCPRoute", - routeKind: "TCPRoute", - routeName: "my-tcp", - translate: func(tr *Translator, tctx *provider.TranslateContext) (*TranslateResult, error) { - return tr.TranslateTCPRoute(tctx, &gatewayv1.TCPRoute{ - ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "my-tcp"}, - Spec: gatewayv1.TCPRouteSpec{Rules: []gatewayv1.TCPRouteRule{{}}}, - }) - }, - }, - { - name: "UDPRoute", - routeKind: "UDPRoute", - routeName: "my-udp", - translate: func(tr *Translator, tctx *provider.TranslateContext) (*TranslateResult, error) { - return tr.TranslateUDPRoute(tctx, &gatewayv1.UDPRoute{ - ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "my-udp"}, - Spec: gatewayv1.UDPRouteSpec{Rules: []gatewayv1.UDPRouteRule{{}}}, - }) - }, - }, - { - name: "TLSRoute", - routeKind: "TLSRoute", - routeName: "my-tls", - translate: func(tr *Translator, tctx *provider.TranslateContext) (*TranslateResult, error) { - return tr.TranslateTLSRoute(tctx, &gatewayv1.TLSRoute{ - ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "my-tls"}, - Spec: gatewayv1.TLSRouteSpec{ - Hostnames: []gatewayv1.Hostname{"example.com"}, - Rules: []gatewayv1.TLSRouteRule{{}}, - }, - }) - }, - }, - } { - t.Run(tt.name, func(t *testing.T) { - tr := NewTranslator(logr.Discard(), "") - tctx := provider.NewDefaultTranslateContext(context.Background()) - policy := makeL4RoutePolicy("default", "invalid-policy", tt.routeKind, tt.routeName, []v1alpha1.Plugin{ - {Name: "ip-restriction", Config: mustJSON([]string{"not-an-object"})}, - }) - tctx.L4RoutePolicies[k8stypes.NamespacedName{Namespace: policy.Namespace, Name: policy.Name}] = policy - - result, err := tt.translate(tr, tctx) - - require.Error(t, err) - assert.Nil(t, result) - assert.Contains(t, err.Error(), `plugin "ip-restriction"`) - }) - } -} - func TestTranslateTCPRouteUpstreamScheme(t *testing.T) { const ( namespace = "default" diff --git a/internal/adc/translator/l4routepolicy_test.go b/internal/adc/translator/l4routepolicy_test.go index 6a41f1e9..833db371 100644 --- a/internal/adc/translator/l4routepolicy_test.go +++ b/internal/adc/translator/l4routepolicy_test.go @@ -23,7 +23,6 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8stypes "k8s.io/apimachinery/pkg/types" @@ -75,7 +74,7 @@ func TestAttachL4RoutePolicyPlugins_AttachesMatchingPolicy(t *testing.T) { } plugins := adctypes.Plugins{} - require.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) + assert.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Len(t, plugins, 2) assert.Contains(t, plugins, "limit-conn") @@ -98,7 +97,7 @@ func TestAttachL4RoutePolicyPlugins_NoMatchOnKind(t *testing.T) { plugins := adctypes.Plugins{} // Looking for TCPRoute, but policy targets UDPRoute — should not match. - require.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-udp-route", "TCPRoute", plugins, nil)) + assert.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-udp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } @@ -116,7 +115,7 @@ func TestAttachL4RoutePolicyPlugins_NoMatchOnNamespace(t *testing.T) { plugins := adctypes.Plugins{} // Route is in "default" namespace, policy is in "other-ns" — should not match. - require.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) + assert.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } @@ -131,7 +130,7 @@ func TestAttachL4RoutePolicyPlugins_EmptyPlugins(t *testing.T) { } plugins := adctypes.Plugins{} - require.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) + assert.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } @@ -139,22 +138,6 @@ func TestAttachL4RoutePolicyPlugins_EmptyPlugins(t *testing.T) { func TestAttachL4RoutePolicyPlugins_EmptyPolicies(t *testing.T) { tr := NewTranslator(logr.Discard(), "") plugins := adctypes.Plugins{} - require.NoError(t, tr.AttachL4RoutePolicyPlugins(nil, "default", "my-tcp-route", "TCPRoute", plugins, nil)) + assert.NoError(t, tr.AttachL4RoutePolicyPlugins(nil, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } - -func TestAttachL4RoutePolicyPlugins_ReturnsInvalidConfigError(t *testing.T) { - tr := NewTranslator(logr.Discard(), "") - policy := makeL4RoutePolicy("default", "invalid-policy", "TCPRoute", "my-tcp-route", []v1alpha1.Plugin{ - {Name: "ip-restriction", Config: mustJSON([]string{"not-an-object"})}, - }) - policies := map[k8stypes.NamespacedName]*v1alpha1.L4RoutePolicy{ - {Namespace: policy.Namespace, Name: policy.Name}: policy, - } - - err := tr.AttachL4RoutePolicyPlugins(policies, "default", "my-tcp-route", "TCPRoute", adctypes.Plugins{}, nil) - - require.Error(t, err) - assert.Contains(t, err.Error(), `plugin "ip-restriction"`) - assert.Contains(t, err.Error(), "invalid-policy") -} diff --git a/internal/adc/translator/plugin.go b/internal/adc/translator/plugin.go index d16180fc..b0817693 100644 --- a/internal/adc/translator/plugin.go +++ b/internal/adc/translator/plugin.go @@ -18,40 +18,16 @@ package translator import ( - "encoding/json" - "fmt" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "github.com/apache/apisix-ingress-controller/api/v1alpha1" - pkgutils "github.com/apache/apisix-ingress-controller/pkg/utils" + "github.com/apache/apisix-ingress-controller/internal/pluginconfig" ) // renderPluginConfig renders the configuration of an apisix.apache.org/v1alpha1 Plugin. // The data of the referenced Secret is merged over spec.config, with each Secret key // read as a dot separated path so that `session.secret` nests under `session`. func renderPluginConfig(plugin v1alpha1.Plugin, namespace string, secrets map[types.NamespacedName]*corev1.Secret) (map[string]any, error) { - config := make(map[string]any) - if len(plugin.Config.Raw) > 0 { - if err := json.Unmarshal(plugin.Config.Raw, &config); err != nil { - return nil, fmt.Errorf("failed to unmarshal config of plugin %s: %w", plugin.Name, err) - } - } - // A literal `config: null` unmarshals to a nil map, which serializes back to - // null and is rejected by most APISIX plugins; normalize it to an empty object. - if config == nil { - config = make(map[string]any) - } - if plugin.SecretRef == nil || plugin.SecretRef.Name == "" { - return config, nil - } - secret, ok := secrets[types.NamespacedName{Namespace: namespace, Name: plugin.SecretRef.Name}] - if !ok || secret == nil { - return nil, fmt.Errorf("secret %s/%s referenced by plugin %s not found", namespace, plugin.SecretRef.Name, plugin.Name) - } - for key, value := range secret.Data { - pkgutils.InsertKeyInMap(key, string(value), config) - } - return config, nil + return pluginconfig.Render(plugin, namespace, secrets) } diff --git a/internal/controller/l4routepolicy_invalid_config_test.go b/internal/controller/l4routepolicy_invalid_config_test.go deleted file mode 100644 index 5642c690..00000000 --- a/internal/controller/l4routepolicy_invalid_config_test.go +++ /dev/null @@ -1,213 +0,0 @@ -// 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" - "testing" - - "github.com/go-logr/logr" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - 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" - - "github.com/apache/apisix-ingress-controller/api/v1alpha1" - "github.com/apache/apisix-ingress-controller/internal/controller/config" - "github.com/apache/apisix-ingress-controller/internal/controller/indexer" - "github.com/apache/apisix-ingress-controller/internal/controller/status" - "github.com/apache/apisix-ingress-controller/internal/provider" -) - -func TestL4RouteReconcileRejectsInvalidPolicyPluginConfig(t *testing.T) { - const ( - namespace = "default" - routeName = "route" - policyName = "policy" - ) - - tests := []struct { - name string - routeKind gatewayv1.Kind - protocol gatewayv1.ProtocolType - newRoute func() client.Object - reconcile func(client.Client, provider.Provider, status.Updater) error - parentRefs func(client.Object) []gatewayv1.RouteParentStatus - }{ - { - name: "TCPRoute", - routeKind: "TCPRoute", - protocol: gatewayv1.TCPProtocolType, - newRoute: func() client.Object { - return &gatewayv1.TCPRoute{ - ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: routeName}, - Spec: gatewayv1.TCPRouteSpec{ - CommonRouteSpec: gatewayv1.CommonRouteSpec{ParentRefs: []gatewayv1.ParentReference{{Name: "gw"}}}, - Rules: []gatewayv1.TCPRouteRule{{}}, - }, - } - }, - reconcile: func(cli client.Client, prov provider.Provider, updater status.Updater) error { - r := &TCPRouteReconciler{Client: cli, Log: logr.Discard(), Provider: prov, Updater: updater, Readier: noopReadier{}, supportsL4RoutePolicy: true} - _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: k8stypes.NamespacedName{Namespace: namespace, Name: routeName}}) - return err - }, - parentRefs: func(obj client.Object) []gatewayv1.RouteParentStatus { - return obj.(*gatewayv1.TCPRoute).Status.Parents - }, - }, - { - name: "UDPRoute", - routeKind: "UDPRoute", - protocol: gatewayv1.UDPProtocolType, - newRoute: func() client.Object { - return &gatewayv1.UDPRoute{ - ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: routeName}, - Spec: gatewayv1.UDPRouteSpec{ - CommonRouteSpec: gatewayv1.CommonRouteSpec{ParentRefs: []gatewayv1.ParentReference{{Name: "gw"}}}, - Rules: []gatewayv1.UDPRouteRule{{}}, - }, - } - }, - reconcile: func(cli client.Client, prov provider.Provider, updater status.Updater) error { - r := &UDPRouteReconciler{Client: cli, Log: logr.Discard(), Provider: prov, Updater: updater, Readier: noopReadier{}, supportsL4RoutePolicy: true} - _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: k8stypes.NamespacedName{Namespace: namespace, Name: routeName}}) - return err - }, - parentRefs: func(obj client.Object) []gatewayv1.RouteParentStatus { - return obj.(*gatewayv1.UDPRoute).Status.Parents - }, - }, - { - name: "TLSRoute", - routeKind: "TLSRoute", - protocol: gatewayv1.TLSProtocolType, - newRoute: func() client.Object { - return &gatewayv1.TLSRoute{ - ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: routeName}, - Spec: gatewayv1.TLSRouteSpec{ - CommonRouteSpec: gatewayv1.CommonRouteSpec{ParentRefs: []gatewayv1.ParentReference{{Name: "gw"}}}, - Hostnames: []gatewayv1.Hostname{"example.com"}, - Rules: []gatewayv1.TLSRouteRule{{}}, - }, - } - }, - reconcile: func(cli client.Client, prov provider.Provider, updater status.Updater) error { - r := &TLSRouteReconciler{Client: cli, Log: logr.Discard(), Provider: prov, Updater: updater, Readier: noopReadier{}, supportsL4RoutePolicy: true} - _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: k8stypes.NamespacedName{Namespace: namespace, Name: routeName}}) - return err - }, - parentRefs: func(obj client.Object) []gatewayv1.RouteParentStatus { - return obj.(*gatewayv1.TLSRoute).Status.Parents - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - scheme := runtime.NewScheme() - 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: namespace, Name: "gw"}, - Spec: gatewayv1.GatewaySpec{ - GatewayClassName: "apisix", - Listeners: []gatewayv1.Listener{{ - Name: "listener", - Protocol: tt.protocol, - Port: 9000, - }}, - }, - } - route := tt.newRoute() - policy := &v1alpha1.L4RoutePolicy{ - ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: policyName, Generation: 1}, - Spec: v1alpha1.L4RoutePolicySpec{ - TargetRefs: []gatewayv1.LocalPolicyTargetReferenceWithSectionName{{ - LocalPolicyTargetReference: gatewayv1.LocalPolicyTargetReference{ - Group: gatewayv1.GroupName, - Kind: tt.routeKind, - Name: routeName, - }, - }}, - Plugins: []v1alpha1.Plugin{{ - Name: "ip-restriction", - Config: apiextensionsv1.JSON{Raw: []byte(`["should-not-appear"]`)}, - }}, - }, - } - - cli := fake.NewClientBuilder().WithScheme(scheme). - WithObjects(gatewayClass, gateway, route, policy). - WithIndex(&v1alpha1.L4RoutePolicy{}, indexer.PolicyTargetRefs, indexer.L4RoutePolicyIndexFunc). - Build() - prov := &recordingProvider{} - updater := &recordingUpdater{} - - require.NoError(t, tt.reconcile(cli, prov, updater)) - assert.Zero(t, prov.updated, "an invalid policy must keep the existing provider state") - assert.Empty(t, prov.deleted) - - var gotPolicyStatus *v1alpha1.L4RoutePolicy - var gotRouteStatus client.Object - for _, update := range updater.updates { - switch update.Resource.(type) { - case *v1alpha1.L4RoutePolicy: - gotPolicyStatus = update.Mutator.Mutate(policy.DeepCopy()).(*v1alpha1.L4RoutePolicy) - default: - gotRouteStatus = update.Mutator.Mutate(route.DeepCopyObject().(client.Object)) - } - } - - require.NotNil(t, gotPolicyStatus) - require.Len(t, gotPolicyStatus.Status.Ancestors, 1) - require.Len(t, gotPolicyStatus.Status.Ancestors[0].Conditions, 1) - condition := gotPolicyStatus.Status.Ancestors[0].Conditions[0] - assert.Equal(t, metav1.ConditionFalse, condition.Status) - assert.Equal(t, string(gatewayv1.PolicyReasonInvalid), condition.Reason) - assert.Contains(t, condition.Message, `plugin "ip-restriction" has an invalid configuration`) - assert.NotContains(t, condition.Message, "should-not-appear") - - require.NotNil(t, gotRouteStatus) - parents := tt.parentRefs(gotRouteStatus) - require.Len(t, parents, 1) - var accepted *metav1.Condition - for i := range parents[0].Conditions { - if parents[0].Conditions[i].Type == string(gatewayv1.RouteConditionAccepted) { - accepted = &parents[0].Conditions[i] - break - } - } - require.NotNil(t, accepted) - assert.Equal(t, metav1.ConditionTrue, accepted.Status) - }) - } -} diff --git a/internal/controller/l4routepolicy_test.go b/internal/controller/l4routepolicy_test.go new file mode 100644 index 00000000..b9235a31 --- /dev/null +++ b/internal/controller/l4routepolicy_test.go @@ -0,0 +1,82 @@ +// 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" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" + "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/indexer" + "github.com/apache/apisix-ingress-controller/internal/provider" +) + +func TestProcessL4RoutePolicy_InvalidPluginConfigSetsRejectedStatus(t *testing.T) { + policy := &v1alpha1.L4RoutePolicy{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "tcp-policy", + Generation: 3, + }, + Spec: v1alpha1.L4RoutePolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReferenceWithSectionName{{ + LocalPolicyTargetReference: gatewayv1.LocalPolicyTargetReference{ + Group: gatewayv1.GroupName, + Kind: "TCPRoute", + Name: "tcp-route", + }, + }}, + Plugins: []v1alpha1.Plugin{{ + Name: "ip-restriction", + Config: apiextensionsv1.JSON{Raw: []byte(`["10.0.0.0/8"]`)}, + }}, + }, + } + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(policy). + WithIndex(&v1alpha1.L4RoutePolicy{}, indexer.PolicyTargetRefs, indexer.L4RoutePolicyIndexFunc). + Build() + tctx := provider.NewDefaultTranslateContext(context.Background()) + tctx.RouteParentRefs = []gatewayv1.ParentReference{{Name: "gateway"}} + + ProcessL4RoutePolicy(cli, logr.Discard(), tctx, "default", "tcp-route", "TCPRoute") + + key := k8stypes.NamespacedName{Namespace: "default", Name: "tcp-policy"} + require.NotNil(t, tctx.L4RoutePolicies[key], "the policy must reach translation so rendering stops the update") + require.Len(t, tctx.StatusUpdaters, 1) + mutated := tctx.StatusUpdaters[0].Mutator.Mutate(&v1alpha1.L4RoutePolicy{}).(*v1alpha1.L4RoutePolicy) + require.Len(t, mutated.Status.Ancestors, 1) + require.Len(t, mutated.Status.Ancestors[0].Conditions, 1) + condition := mutated.Status.Ancestors[0].Conditions[0] + assert.Equal(t, string(gatewayv1.PolicyConditionAccepted), condition.Type) + assert.Equal(t, metav1.ConditionFalse, condition.Status) + assert.Equal(t, string(gatewayv1.PolicyReasonInvalid), condition.Reason) + assert.Equal(t, int64(3), condition.ObservedGeneration) + assert.Equal(t, `plugin "ip-restriction" has invalid configuration`, condition.Message) +} diff --git a/internal/controller/policies.go b/internal/controller/policies.go index 34b09aff..9643200d 100644 --- a/internal/controller/policies.go +++ b/internal/controller/policies.go @@ -19,8 +19,6 @@ package controller import ( "context" - "encoding/json" - "errors" "fmt" "slices" "sort" @@ -40,6 +38,7 @@ import ( "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/pluginconfig" "github.com/apache/apisix-ingress-controller/internal/provider" internaltypes "github.com/apache/apisix-ingress-controller/internal/types" "github.com/apache/apisix-ingress-controller/internal/utils" @@ -54,21 +53,6 @@ type PolicyTargetKey struct { SectionName string } -type invalidL4RoutePolicyError struct { - err error -} - -func (e *invalidL4RoutePolicyError) Error() string { return e.err.Error() } -func (e *invalidL4RoutePolicyError) Unwrap() error { return e.err } - -func l4RoutePolicyReconcileError(err error) error { - var invalidErr *invalidL4RoutePolicyError - if errors.As(err, &invalidErr) { - return nil - } - return err -} - func (p PolicyTargetKey) String() string { return p.NsName.String() + "/" + p.GroupKind.String() + "/" + p.SectionName } @@ -284,15 +268,15 @@ func ProcessL4RoutePolicy( log logr.Logger, tctx *provider.TranslateContext, routeNamespace, routeName, routeKind string, -) error { +) { var list v1alpha1.L4RoutePolicyList key := indexer.GenIndexKeyWithGK(gatewayv1.GroupName, routeKind, routeNamespace, routeName) if err := c.List(tctx, &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil { log.Error(err, "failed to list L4RoutePolicy", "namespace", routeNamespace, "name", routeName, "kind", routeKind) - return err + return } if len(list.Items) == 0 { - return nil + return } // L4 routes have no addressable sections; a targetRef that specifies a sectionName @@ -301,7 +285,7 @@ func ProcessL4RoutePolicy( return !l4RoutePolicyMatchesRoute(p, routeKind, routeNamespace, routeName) }) if len(list.Items) == 0 { - return nil + return } // Deterministic conflict resolution: oldest creationTimestamp wins; tie-break by namespace/name. @@ -317,29 +301,33 @@ func ProcessL4RoutePolicy( }) winner := list.Items[0].DeepCopy() - // An invalid policy is not attached at all, so a route is never programmed with - // a subset of the plugins the policy asks for. - policyErr := validateL4RoutePolicyPluginConfigs(winner) - if policyErr == nil { - policyErr = loadPluginSecrets(tctx, c, tctx, winner.Namespace, winner.Spec.Plugins) - } - if policyErr != nil { - log.Error(policyErr, "failed to process L4RoutePolicy plugins", "policy", types.NamespacedName{Namespace: winner.Namespace, Name: winner.Name}) + renderErr := loadPluginSecrets(tctx, c, tctx, winner.Namespace, winner.Spec.Plugins) + if renderErr == nil { + for _, plugin := range winner.Spec.Plugins { + if _, err := pluginconfig.Render(plugin, winner.Namespace, tctx.Secrets); err != nil { + log.Error(err, "failed to render L4RoutePolicy plugin config", "plugin", plugin.Name, "policy", types.NamespacedName{Namespace: winner.Namespace, Name: winner.Name}) + renderErr = fmt.Errorf("plugin %q has invalid configuration", plugin.Name) + break + } + } } else { - tctx.L4RoutePolicies[types.NamespacedName{Namespace: winner.Namespace, Name: winner.Name}] = winner + log.Error(renderErr, "failed to load Secrets referenced by L4RoutePolicy plugins", "policy", types.NamespacedName{Namespace: winner.Namespace, Name: winner.Name}) } + // Keep the winning policy in the translation context even when rendering failed. + // Translation must return the error instead of publishing the route without it. + tctx.L4RoutePolicies[types.NamespacedName{Namespace: winner.Namespace, Name: winner.Name}] = winner for i := range list.Items { policy := list.Items[i] var condition metav1.Condition - if i == 0 && policyErr != nil { + if i == 0 && renderErr != nil { condition = metav1.Condition{ Type: string(gatewayv1.PolicyConditionAccepted), Status: metav1.ConditionFalse, ObservedGeneration: policy.GetGeneration(), LastTransitionTime: metav1.Now(), Reason: string(gatewayv1.PolicyReasonInvalid), - Message: policyErr.Error(), + Message: renderErr.Error(), } } else if i == 0 { condition = metav1.Condition{ @@ -376,23 +364,6 @@ func ProcessL4RoutePolicy( }) } } - if policyErr != nil { - return &invalidL4RoutePolicyError{err: policyErr} - } - return nil -} - -func validateL4RoutePolicyPluginConfigs(policy *v1alpha1.L4RoutePolicy) error { - for _, plugin := range policy.Spec.Plugins { - if len(plugin.Config.Raw) == 0 { - continue - } - var config map[string]any - if err := json.Unmarshal(plugin.Config.Raw, &config); err != nil { - return fmt.Errorf("plugin %q has an invalid configuration: %w", plugin.Name, err) - } - } - return nil } // updateL4RoutePolicyStatusOnDeleting removes the deleted route's ancestor status entries diff --git a/internal/controller/tcproute_controller.go b/internal/controller/tcproute_controller.go index aed1707a..b8d6f97e 100644 --- a/internal/controller/tcproute_controller.go +++ b/internal/controller/tcproute_controller.go @@ -345,9 +345,8 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } ProcessBackendTrafficPolicy(r.Client, r.Log, tctx) - var l4RoutePolicyErr error if r.supportsL4RoutePolicy { - l4RoutePolicyErr = ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindTCPRoute) + ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindTCPRoute) } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { @@ -378,9 +377,6 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { - if l4RoutePolicyErr != nil { - return ctrl.Result{}, l4RoutePolicyReconcileError(l4RoutePolicyErr) - } routeToUpdate := tr if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err diff --git a/internal/controller/tlsroute_controller.go b/internal/controller/tlsroute_controller.go index 1c204718..779b7603 100644 --- a/internal/controller/tlsroute_controller.go +++ b/internal/controller/tlsroute_controller.go @@ -345,9 +345,8 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } ProcessBackendTrafficPolicy(r.Client, r.Log, tctx) - var l4RoutePolicyErr error if r.supportsL4RoutePolicy { - l4RoutePolicyErr = ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, types.KindTLSRoute) + ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, types.KindTLSRoute) } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { @@ -378,9 +377,6 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { - if l4RoutePolicyErr != nil { - return ctrl.Result{}, l4RoutePolicyReconcileError(l4RoutePolicyErr) - } routeToUpdate := tr if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err diff --git a/internal/controller/udproute_controller.go b/internal/controller/udproute_controller.go index bd9df870..3ca88907 100644 --- a/internal/controller/udproute_controller.go +++ b/internal/controller/udproute_controller.go @@ -345,9 +345,8 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } ProcessBackendTrafficPolicy(r.Client, r.Log, tctx) - var l4RoutePolicyErr error if r.supportsL4RoutePolicy { - l4RoutePolicyErr = ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindUDPRoute) + ProcessL4RoutePolicy(r.Client, r.Log, tctx, tr.Namespace, tr.Name, KindUDPRoute) } tr.Status.Parents = make([]gatewayv1.RouteParentStatus, 0, len(gateways)) for _, gateway := range gateways { @@ -378,9 +377,6 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c }) UpdateStatus(r.Updater, r.Log, tctx) if isRouteAccepted(gateways) { - if l4RoutePolicyErr != nil { - return ctrl.Result{}, l4RoutePolicyReconcileError(l4RoutePolicyErr) - } routeToUpdate := tr if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { return ctrl.Result{}, err diff --git a/internal/pluginconfig/renderer.go b/internal/pluginconfig/renderer.go new file mode 100644 index 00000000..06857ed0 --- /dev/null +++ b/internal/pluginconfig/renderer.go @@ -0,0 +1,55 @@ +// 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 pluginconfig + +import ( + "encoding/json" + "fmt" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/apache/apisix-ingress-controller/api/v1alpha1" + pkgutils "github.com/apache/apisix-ingress-controller/pkg/utils" +) + +// Render renders a v1alpha1 plugin configuration and merges referenced Secret data. +func Render(plugin v1alpha1.Plugin, namespace string, secrets map[types.NamespacedName]*corev1.Secret) (map[string]any, error) { + config := make(map[string]any) + if len(plugin.Config.Raw) > 0 { + if err := json.Unmarshal(plugin.Config.Raw, &config); err != nil { + return nil, fmt.Errorf("failed to unmarshal config of plugin %s: %w", plugin.Name, err) + } + } + // A literal `config: null` unmarshals to a nil map, which serializes back to + // null and is rejected by most APISIX plugins; normalize it to an empty object. + if config == nil { + config = make(map[string]any) + } + if plugin.SecretRef == nil || plugin.SecretRef.Name == "" { + return config, nil + } + secret, ok := secrets[types.NamespacedName{Namespace: namespace, Name: plugin.SecretRef.Name}] + if !ok || secret == nil { + return nil, fmt.Errorf("secret %s/%s referenced by plugin %s not found", namespace, plugin.SecretRef.Name, plugin.Name) + } + for key, value := range secret.Data { + pkgutils.InsertKeyInMap(key, string(value), config) + } + return config, nil +} diff --git a/internal/provider/api7ee/provider_test.go b/internal/provider/api7ee/provider_test.go index d1907321..8ef49365 100644 --- a/internal/provider/api7ee/provider_test.go +++ b/internal/provider/api7ee/provider_test.go @@ -22,12 +22,21 @@ import ( "strings" "testing" + "github.com/go-logr/logr" "github.com/go-logr/logr/funcr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + 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" + "github.com/apache/apisix-ingress-controller/internal/controller/label" + "github.com/apache/apisix-ingress-controller/internal/provider" + "github.com/apache/apisix-ingress-controller/internal/utils" ) func TestDeleteLogsObjectIdentityOnly(t *testing.T) { @@ -59,3 +68,68 @@ func TestDeleteLogsObjectIdentityOnly(t *testing.T) { assert.Contains(t, output, "default") assert.Contains(t, output, "consumer") } + +func TestUpdateKeepsLastKnownGoodStateWhenL4PolicyCannotRender(t *testing.T) { + rawProvider, err := New(logr.Discard(), nil, nil) + require.NoError(t, err) + d := rawProvider.(*api7eeProvider) + + route := &gatewayv1.TCPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "TCPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "tcp-route", + }, + Spec: gatewayv1.TCPRouteSpec{Rules: []gatewayv1.TCPRouteRule{{}}}, + } + gatewayProxy := v1alpha1.GatewayProxy{ + TypeMeta: metav1.TypeMeta{Kind: "GatewayProxy", APIVersion: v1alpha1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "proxy"}, + Spec: v1alpha1.GatewayProxySpec{Provider: &v1alpha1.GatewayProxyProvider{ + Type: v1alpha1.ProviderTypeControlPlane, + ControlPlane: &v1alpha1.ControlPlaneProvider{ + Endpoints: []string{"http://apisix:9180"}, + Auth: v1alpha1.ControlPlaneAuth{ + Type: v1alpha1.AuthTypeAdminKey, + AdminKey: &v1alpha1.AdminKeyAuth{Value: "key"}, + }, + }, + }}, + } + configName := utils.NamespacedNameKind(&gatewayProxy).String() + lastKnownGood := adctypes.NewDefaultService() + lastKnownGood.Name = "last-known-good" + lastKnownGood.ID = "last-known-good" + lastKnownGood.Labels = label.GenLabel(route) + require.NoError(t, d.client.Insert(configName, []string{adctypes.TypeService}, &adctypes.Resources{ + Services: []*adctypes.Service{lastKnownGood}, + }, lastKnownGood.Labels)) + + policy := &v1alpha1.L4RoutePolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "tcp-policy"}, + Spec: v1alpha1.L4RoutePolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReferenceWithSectionName{{ + LocalPolicyTargetReference: gatewayv1.LocalPolicyTargetReference{ + Group: gatewayv1.GroupName, + Kind: "TCPRoute", + Name: "tcp-route", + }, + }}, + Plugins: []v1alpha1.Plugin{{ + Name: "ip-restriction", + Config: apiextensionsv1.JSON{Raw: []byte(`[]`)}, + }}, + }, + } + tctx := provider.NewDefaultTranslateContext(context.Background()) + tctx.GatewayProxies[utils.NamespacedNameKind(&gatewayProxy)] = gatewayProxy + tctx.L4RoutePolicies[k8stypes.NamespacedName{Namespace: policy.Namespace, Name: policy.Name}] = policy + + err = d.Update(context.Background(), tctx, route) + + require.Error(t, err) + resources, getErr := d.client.GetResources(configName) + require.NoError(t, getErr) + require.Len(t, resources.Services, 1) + assert.Equal(t, "last-known-good", resources.Services[0].Name) +} diff --git a/internal/provider/apisix/provider_test.go b/internal/provider/apisix/provider_test.go index 7bcf6391..50d9e420 100644 --- a/internal/provider/apisix/provider_test.go +++ b/internal/provider/apisix/provider_test.go @@ -27,12 +27,17 @@ import ( "github.com/go-logr/logr/funcr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" 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/provider" "github.com/apache/apisix-ingress-controller/internal/types" "github.com/apache/apisix-ingress-controller/internal/utils" ) @@ -99,3 +104,69 @@ func TestDeleteNotifiesSyncOnlyWhenConfigWasRemoved(t *testing.T) { require.NoError(t, d.Delete(context.Background(), route)) require.Len(t, d.syncCh, 1, "removing configuration this controller pushed must trigger a sync") } + +func TestUpdateKeepsLastKnownGoodStateWhenL4PolicyCannotRender(t *testing.T) { + rawProvider, err := New(logr.Discard(), nil, nil) + require.NoError(t, err) + d := rawProvider.(*apisixProvider) + + route := &gatewayv1.TCPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "TCPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "tcp-route", + }, + Spec: gatewayv1.TCPRouteSpec{Rules: []gatewayv1.TCPRouteRule{{}}}, + } + gatewayProxy := v1alpha1.GatewayProxy{ + TypeMeta: metav1.TypeMeta{Kind: "GatewayProxy", APIVersion: v1alpha1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "proxy"}, + Spec: v1alpha1.GatewayProxySpec{Provider: &v1alpha1.GatewayProxyProvider{ + Type: v1alpha1.ProviderTypeControlPlane, + ControlPlane: &v1alpha1.ControlPlaneProvider{ + Endpoints: []string{"http://apisix:9180"}, + Auth: v1alpha1.ControlPlaneAuth{ + Type: v1alpha1.AuthTypeAdminKey, + AdminKey: &v1alpha1.AdminKeyAuth{Value: "key"}, + }, + }, + }}, + } + configName := utils.NamespacedNameKind(&gatewayProxy).String() + lastKnownGood := adctypes.NewDefaultService() + lastKnownGood.Name = "last-known-good" + lastKnownGood.ID = "last-known-good" + lastKnownGood.Labels = label.GenLabel(route) + require.NoError(t, d.client.Insert(configName, []string{adctypes.TypeService}, &adctypes.Resources{ + Services: []*adctypes.Service{lastKnownGood}, + }, lastKnownGood.Labels)) + + policy := &v1alpha1.L4RoutePolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "tcp-policy"}, + Spec: v1alpha1.L4RoutePolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReferenceWithSectionName{{ + LocalPolicyTargetReference: gatewayv1.LocalPolicyTargetReference{ + Group: gatewayv1.GroupName, + Kind: "TCPRoute", + Name: "tcp-route", + }, + }}, + Plugins: []v1alpha1.Plugin{{ + Name: "ip-restriction", + Config: apiextensionsv1.JSON{Raw: []byte(`[]`)}, + }}, + }, + } + tctx := provider.NewDefaultTranslateContext(context.Background()) + tctx.GatewayProxies[utils.NamespacedNameKind(&gatewayProxy)] = gatewayProxy + tctx.L4RoutePolicies[k8stypes.NamespacedName{Namespace: policy.Namespace, Name: policy.Name}] = policy + + err = d.Update(context.Background(), tctx, route) + + require.Error(t, err) + assert.Empty(t, d.syncCh) + resources, getErr := d.client.GetResources(configName) + require.NoError(t, getErr) + require.Len(t, resources.Services, 1) + assert.Equal(t, "last-known-good", resources.Services[0].Name) +}