diff --git a/internal/adc/translator/l4route_test.go b/internal/adc/translator/l4route_test.go index 62d0ca03..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) diff --git a/internal/adc/translator/l4routepolicy_test.go b/internal/adc/translator/l4routepolicy_test.go index 289dab98..833db371 100644 --- a/internal/adc/translator/l4routepolicy_test.go +++ b/internal/adc/translator/l4routepolicy_test.go @@ -74,7 +74,7 @@ func TestAttachL4RoutePolicyPlugins_AttachesMatchingPolicy(t *testing.T) { } plugins := adctypes.Plugins{} - 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") @@ -97,7 +97,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) + assert.NoError(t, tr.AttachL4RoutePolicyPlugins(policies, "default", "my-udp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } @@ -115,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. - 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) } @@ -130,7 +130,7 @@ func TestAttachL4RoutePolicyPlugins_EmptyPlugins(t *testing.T) { } plugins := adctypes.Plugins{} - 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) } @@ -138,6 +138,6 @@ 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) + assert.NoError(t, tr.AttachL4RoutePolicyPlugins(nil, "default", "my-tcp-route", "TCPRoute", plugins, nil)) assert.Empty(t, plugins) } 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/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 1dc95631..2faa16b6 100644 --- a/internal/adc/translator/tlsroute.go +++ b/internal/adc/translator/tlsroute.go @@ -144,7 +144,10 @@ func (t *Translator) TranslateTLSRoute(tctx *provider.TranslateContext, tlsRoute } for _, host := range hosts { - streamRoutes := t.buildL4StreamRoutes(tctx, tlsRoute.Namespace, tlsRoute.Name, ruleIndex, "TLS", "TLSRoute", labels) + streamRoutes, err := t.buildL4StreamRoutes(tctx, tlsRoute.Namespace, tlsRoute.Name, ruleIndex, "TLS", "TLSRoute", labels) + if err != nil { + return nil, err + } for _, streamRoute := range streamRoutes { streamRoute.SNI = host } 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_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 1cfb5c7f..9643200d 100644 --- a/internal/controller/policies.go +++ b/internal/controller/policies.go @@ -38,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" @@ -300,26 +301,33 @@ 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}) + 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 && secretErr != 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: secretErr.Error(), + Message: renderErr.Error(), } } else if i == 0 { condition = metav1.Condition{ 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) +}