diff --git a/internal/adc/translator/apisixconsumer.go b/internal/adc/translator/apisixconsumer.go index 3cae6e08..cdd258ce 100644 --- a/internal/adc/translator/apisixconsumer.go +++ b/internal/adc/translator/apisixconsumer.go @@ -101,7 +101,10 @@ func (t *Translator) TranslateApisixConsumer(tctx *provider.TranslateContext, ac if !plugin.Enable { continue } - config := t.buildPluginConfig(plugin, ac.Namespace, tctx.Secrets) + config, err := t.buildPluginConfig(plugin, ac.Namespace, tctx.Secrets) + if err != nil { + return nil, err + } plugins[plugin.Name] = config } diff --git a/internal/adc/translator/apisixroute.go b/internal/adc/translator/apisixroute.go index f89a2053..e9ea8694 100644 --- a/internal/adc/translator/apisixroute.go +++ b/internal/adc/translator/apisixroute.go @@ -63,7 +63,10 @@ func (t *Translator) TranslateApisixRoute(tctx *provider.TranslateContext, ar *a func (t *Translator) translateHTTPRule(tctx *provider.TranslateContext, ar *apiv2.ApisixRoute, rule apiv2.ApisixRouteHTTP, ruleIndex int) (*adc.Service, error) { timeout := t.buildTimeout(rule) - plugins := t.buildPlugins(tctx, ar, rule) + plugins, err := t.buildPlugins(tctx, ar, rule) + if err != nil { + return nil, err + } vars, err := rule.Match.NginxVars.ToVars() if err != nil { @@ -91,24 +94,28 @@ func (t *Translator) buildTimeout(rule apiv2.ApisixRouteHTTP) *adc.Timeout { } } -func (t *Translator) buildPlugins(tctx *provider.TranslateContext, ar *apiv2.ApisixRoute, rule apiv2.ApisixRouteHTTP) adc.Plugins { +func (t *Translator) buildPlugins(tctx *provider.TranslateContext, ar *apiv2.ApisixRoute, rule apiv2.ApisixRouteHTTP) (adc.Plugins, error) { plugins := make(adc.Plugins) // Load plugins from referenced PluginConfig - t.loadPluginConfigPlugins(tctx, ar, rule, plugins) + if err := t.loadPluginConfigPlugins(tctx, ar, rule, plugins); err != nil { + return nil, err + } // Apply plugins from the route itself - t.loadRoutePlugins(tctx, ar, rule.Plugins, plugins) + if err := t.loadRoutePlugins(tctx, ar, rule.Plugins, plugins); err != nil { + return nil, err + } // Add authentication plugins t.addAuthenticationPlugins(rule, plugins) - return plugins + return plugins, nil } -func (t *Translator) loadPluginConfigPlugins(tctx *provider.TranslateContext, ar *apiv2.ApisixRoute, rule apiv2.ApisixRouteHTTP, plugins adc.Plugins) { +func (t *Translator) loadPluginConfigPlugins(tctx *provider.TranslateContext, ar *apiv2.ApisixRoute, rule apiv2.ApisixRouteHTTP, plugins adc.Plugins) error { if rule.PluginConfigName == "" { - return + return nil } pcNamespace := ar.Namespace @@ -119,33 +126,41 @@ func (t *Translator) loadPluginConfigPlugins(tctx *provider.TranslateContext, ar pcKey := types.NamespacedName{Namespace: pcNamespace, Name: rule.PluginConfigName} pc, ok := tctx.ApisixPluginConfigs[pcKey] if !ok || pc == nil { - return + return nil } for _, plugin := range pc.Spec.Plugins { if !plugin.Enable { continue } - config := t.buildPluginConfig(plugin, pc.Namespace, tctx.Secrets) + config, err := t.buildPluginConfig(plugin, pc.Namespace, tctx.Secrets) + if err != nil { + return err + } plugins[plugin.Name] = config } + return nil } -func (t *Translator) loadRoutePlugins(tctx *provider.TranslateContext, ar *apiv2.ApisixRoute, routePlugins []apiv2.ApisixRoutePlugin, plugins adc.Plugins) { +func (t *Translator) loadRoutePlugins(tctx *provider.TranslateContext, ar *apiv2.ApisixRoute, routePlugins []apiv2.ApisixRoutePlugin, plugins adc.Plugins) error { for _, plugin := range routePlugins { if !plugin.Enable { continue } - config := t.buildPluginConfig(plugin, ar.Namespace, tctx.Secrets) + config, err := t.buildPluginConfig(plugin, ar.Namespace, tctx.Secrets) + if err != nil { + return err + } plugins[plugin.Name] = config } + return nil } -func (t *Translator) buildPluginConfig(plugin apiv2.ApisixRoutePlugin, namespace string, secrets map[types.NamespacedName]*corev1.Secret) map[string]any { +func (t *Translator) buildPluginConfig(plugin apiv2.ApisixRoutePlugin, 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 { - t.Log.Error(err, "failed to unmarshal plugin config") + return nil, fmt.Errorf("failed to unmarshal config of plugin %s: %w", plugin.Name, err) } } if plugin.SecretRef != "" { @@ -155,7 +170,7 @@ func (t *Translator) buildPluginConfig(plugin apiv2.ApisixRoutePlugin, namespace } } } - return config + return config, nil } func (t *Translator) addAuthenticationPlugins(rule apiv2.ApisixRouteHTTP, plugins adc.Plugins) { @@ -473,7 +488,9 @@ func (t *Translator) translateApisixRouteBackendResolveGranularityEndpoint(tctx func (t *Translator) translateStreamRule(tctx *provider.TranslateContext, ar *apiv2.ApisixRoute, part apiv2.ApisixRouteStream) (*adc.Service, error) { // add stream route plugins plugins := make(adc.Plugins) - t.loadRoutePlugins(tctx, ar, part.Plugins, plugins) + if err := t.loadRoutePlugins(tctx, ar, part.Plugins, plugins); err != nil { + return nil, err + } sr := adc.NewDefaultStreamRoute() sr.Name = adc.ComposeStreamRouteName(ar.Namespace, ar.Name, part.Name, part.Protocol) diff --git a/internal/adc/translator/globalrule.go b/internal/adc/translator/globalrule.go index b54e1283..a75260c9 100644 --- a/internal/adc/translator/globalrule.go +++ b/internal/adc/translator/globalrule.go @@ -37,7 +37,10 @@ func (t *Translator) TranslateApisixGlobalRule(tctx *provider.TranslateContext, continue } - pluginConfig := t.buildPluginConfig(plugin, obj.Namespace, tctx.Secrets) + pluginConfig, err := t.buildPluginConfig(plugin, obj.Namespace, tctx.Secrets) + if err != nil { + return nil, err + } plugins[plugin.Name] = pluginConfig } diff --git a/internal/adc/translator/grpcroute.go b/internal/adc/translator/grpcroute.go index d1fa2eb4..46607cf7 100644 --- a/internal/adc/translator/grpcroute.go +++ b/internal/adc/translator/grpcroute.go @@ -38,7 +38,7 @@ func (t *Translator) fillPluginsFromGRPCRouteFilters( namespace string, filters []gatewayv1.GRPCRouteFilter, tctx *provider.TranslateContext, -) { +) error { for _, filter := range filters { switch filter.Type { case gatewayv1.GRPCRouteFilterRequestHeaderModifier: @@ -48,9 +48,12 @@ func (t *Translator) fillPluginsFromGRPCRouteFilters( case gatewayv1.GRPCRouteFilterResponseHeaderModifier: t.fillPluginFromHTTPResponseHeaderFilter(plugins, filter.ResponseHeaderModifier) case gatewayv1.GRPCRouteFilterExtensionRef: - t.fillPluginFromExtensionRef(plugins, namespace, filter.ExtensionRef, tctx) + if err := t.fillPluginFromExtensionRef(plugins, namespace, filter.ExtensionRef, tctx); err != nil { + return err + } } } + return nil } func calculateGRPCRoutePriority(match *gatewayv1.GRPCRouteMatch, ruleIndex int, hosts []string) uint64 { @@ -283,7 +286,9 @@ func (t *Translator) TranslateGRPCRoute(tctx *provider.TranslateContext, grpcRou } } - t.fillPluginsFromGRPCRouteFilters(service.Plugins, grpcRoute.GetNamespace(), rule.Filters, tctx) + if err := t.fillPluginsFromGRPCRouteFilters(service.Plugins, grpcRoute.GetNamespace(), rule.Filters, tctx); err != nil { + return nil, err + } matches := rule.Matches if len(matches) == 0 { diff --git a/internal/adc/translator/httproute.go b/internal/adc/translator/httproute.go index 3dcb00a8..dca2bb2d 100644 --- a/internal/adc/translator/httproute.go +++ b/internal/adc/translator/httproute.go @@ -45,7 +45,7 @@ func (t *Translator) fillPluginsFromHTTPRouteFilters( filters []gatewayv1.HTTPRouteFilter, matches []gatewayv1.HTTPRouteMatch, tctx *provider.TranslateContext, -) { +) error { for _, filter := range filters { switch filter.Type { case gatewayv1.HTTPRouteFilterRequestHeaderModifier: @@ -59,16 +59,19 @@ func (t *Translator) fillPluginsFromHTTPRouteFilters( case gatewayv1.HTTPRouteFilterResponseHeaderModifier: t.fillPluginFromHTTPResponseHeaderFilter(plugins, filter.ResponseHeaderModifier) case gatewayv1.HTTPRouteFilterExtensionRef: - t.fillPluginFromExtensionRef(plugins, namespace, filter.ExtensionRef, tctx) + if err := t.fillPluginFromExtensionRef(plugins, namespace, filter.ExtensionRef, tctx); err != nil { + return err + } case gatewayv1.HTTPRouteFilterCORS: t.fillPluginFromHTTPCORSFilter(plugins, filter.CORS) } } + return nil } -func (t *Translator) fillPluginFromExtensionRef(plugins adctypes.Plugins, namespace string, extensionRef *gatewayv1.LocalObjectReference, tctx *provider.TranslateContext) { +func (t *Translator) fillPluginFromExtensionRef(plugins adctypes.Plugins, namespace string, extensionRef *gatewayv1.LocalObjectReference, tctx *provider.TranslateContext) error { if extensionRef == nil { - return + return nil } if extensionRef.Kind == internaltypes.KindPluginConfig { pluginconfig := tctx.PluginConfigs[types.NamespacedName{ @@ -76,21 +79,21 @@ func (t *Translator) fillPluginFromExtensionRef(plugins adctypes.Plugins, namesp Name: string(extensionRef.Name), }] if pluginconfig == nil { - return + return nil } for _, plugin := range pluginconfig.Spec.Plugins { pluginName := plugin.Name pluginconfig := make(map[string]any) if len(plugin.Config.Raw) > 0 { if err := json.Unmarshal(plugin.Config.Raw, &pluginconfig); err != nil { - t.Log.Error(err, "plugin config unmarshal failed", "plugin", plugin.Name) - continue + return fmt.Errorf("failed to unmarshal config of plugin %s: %w", plugin.Name, err) } } plugins[pluginName] = pluginconfig } t.Log.V(1).Info("fill plugin from extension ref", "plugins", plugins) } + return nil } func (t *Translator) fillPluginFromURLRewriteFilter(plugins adctypes.Plugins, urlRewrite *gatewayv1.HTTPURLRewriteFilter, matches []gatewayv1.HTTPRouteMatch) { @@ -668,7 +671,9 @@ func (t *Translator) TranslateHTTPRoute(tctx *provider.TranslateContext, httpRou } } - t.fillPluginsFromHTTPRouteFilters(service.Plugins, httpRoute.GetNamespace(), rule.Filters, rule.Matches, tctx) + if err := t.fillPluginsFromHTTPRouteFilters(service.Plugins, httpRoute.GetNamespace(), rule.Filters, rule.Matches, tctx); err != nil { + return nil, err + } matches := rule.Matches if len(matches) == 0 { diff --git a/internal/adc/translator/ingress.go b/internal/adc/translator/ingress.go index 36e02c4e..6779a704 100644 --- a/internal/adc/translator/ingress.go +++ b/internal/adc/translator/ingress.go @@ -104,7 +104,11 @@ func (t *Translator) TranslateIngress( for j, path := range rule.HTTP.Paths { index := fmt.Sprintf("%d-%d", i, j) - if svc := t.buildServiceFromIngressPath(tctx, obj, config, &path, index, hosts, labels); svc != nil { + svc, err := t.buildServiceFromIngressPath(tctx, obj, config, &path, index, hosts, labels) + if err != nil { + return nil, err + } + if svc != nil { result.Services = append(result.Services, svc) } } @@ -147,9 +151,9 @@ func (t *Translator) buildServiceFromIngressPath( index string, hosts []string, labels map[string]string, -) *adctypes.Service { +) (*adctypes.Service, error) { if path.Backend.Service == nil { - return nil + return nil, nil } service := adctypes.NewDefaultService() @@ -162,7 +166,10 @@ func (t *Translator) buildServiceFromIngressPath( protocol := t.resolveIngressUpstream(tctx, obj, config, path.Backend.Service, upstream) service.Upstream = upstream - route := t.buildRouteFromIngressPath(tctx, obj, path, config, index, labels) + route, err := t.buildRouteFromIngressPath(tctx, obj, path, config, index, labels) + if err != nil { + return nil, err + } // Check if websocket is enabled via annotation first, then fall back to appProtocol detection if config != nil && config.EnableWebsocket { route.EnableWebsocket = ptr.To(true) @@ -172,7 +179,7 @@ func (t *Translator) buildServiceFromIngressPath( service.Routes = []*adctypes.Route{route} t.fillHTTPRoutePoliciesForIngress(tctx, service.Routes) - return service + return service, nil } func (t *Translator) resolveIngressUpstream( @@ -260,7 +267,7 @@ func (t *Translator) buildRouteFromIngressPath( config *IngressConfig, index string, labels map[string]string, -) *adctypes.Route { +) (*adctypes.Route, error) { route := adctypes.NewDefaultRoute() route.Name = adctypes.ComposeRouteName(obj.Namespace, obj.Name, index) route.ID = id.GenID(route.Name) @@ -306,7 +313,11 @@ func (t *Translator) buildRouteFromIngressPath( if config != nil { // check if PluginConfig is specified if config.PluginConfigName != "" { - route.Plugins = t.loadPluginConfigPluginsForIngress(tctx, obj.Namespace, config.PluginConfigName) + plugins, err := t.loadPluginConfigPluginsForIngress(tctx, obj.Namespace, config.PluginConfigName) + if err != nil { + return nil, err + } + route.Plugins = plugins } // apply plugins from annotations @@ -321,10 +332,10 @@ func (t *Translator) buildRouteFromIngressPath( } route.Uris = uris - return route + return route, nil } -func (t *Translator) loadPluginConfigPluginsForIngress(tctx *provider.TranslateContext, namespace, pluginConfigName string) adctypes.Plugins { +func (t *Translator) loadPluginConfigPluginsForIngress(tctx *provider.TranslateContext, namespace, pluginConfigName string) (adctypes.Plugins, error) { plugins := make(adctypes.Plugins) pcKey := types.NamespacedName{ @@ -333,18 +344,21 @@ func (t *Translator) loadPluginConfigPluginsForIngress(tctx *provider.TranslateC } pc, ok := tctx.ApisixPluginConfigs[pcKey] if !ok || pc == nil { - return plugins + return plugins, nil } for _, plugin := range pc.Spec.Plugins { if !plugin.Enable { continue } - config := t.buildPluginConfig(plugin, namespace, tctx.Secrets) + config, err := t.buildPluginConfig(plugin, namespace, tctx.Secrets) + if err != nil { + return nil, err + } plugins[plugin.Name] = config } - return plugins + return plugins, nil } // translateEndpointSliceForIngress create upstream nodes from EndpointSlice diff --git a/internal/adc/translator/pluginconfig_test.go b/internal/adc/translator/pluginconfig_test.go new file mode 100644 index 00000000..ad75f7e5 --- /dev/null +++ b/internal/adc/translator/pluginconfig_test.go @@ -0,0 +1,183 @@ +// 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 translator + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "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/provider" + internaltypes "github.com/apache/apisix-ingress-controller/internal/types" +) + +func TestBuildPluginConfig_NonObjectConfigIsRejected(t *testing.T) { + translator := NewTranslator(logr.Discard()) + + for _, raw := range []string{`["10.0.0.0/8"]`, `"whitelist"`, `42`} { + plugin := apiv2.ApisixRoutePlugin{ + Name: "ip-restriction", + Enable: true, + Config: apiextensionsv1.JSON{Raw: []byte(raw)}, + } + config, err := translator.buildPluginConfig(plugin, "default", nil) + assert.Error(t, err, "config %s must be rejected", raw) + assert.ErrorContains(t, err, "ip-restriction") + assert.Nil(t, config) + } +} + +func TestBuildPluginConfig_ValidConfigWithSecretRef(t *testing.T) { + translator := NewTranslator(logr.Discard()) + + plugin := apiv2.ApisixRoutePlugin{ + Name: "ip-restriction", + Enable: true, + Config: apiextensionsv1.JSON{Raw: []byte(`{"whitelist":["10.0.0.0/8"]}`)}, + SecretRef: "cred", + } + secrets := map[types.NamespacedName]*corev1.Secret{ + {Namespace: "default", Name: "cred"}: { + Data: map[string][]byte{"message": []byte("denied")}, + }, + } + config, err := translator.buildPluginConfig(plugin, "default", secrets) + assert.NoError(t, err) + assert.Equal(t, []any{"10.0.0.0/8"}, config["whitelist"]) + assert.Equal(t, "denied", config["message"]) +} + +func TestBuildPlugins_MalformedRoutePluginFailsTranslation(t *testing.T) { + translator := NewTranslator(logr.Discard()) + tctx := provider.NewDefaultTranslateContext(context.Background()) + + ar := &apiv2.ApisixRoute{ + ObjectMeta: metav1.ObjectMeta{Name: "test-route", Namespace: "default"}, + } + rule := apiv2.ApisixRouteHTTP{ + Name: "rule1", + Plugins: []apiv2.ApisixRoutePlugin{{ + Name: "ip-restriction", + Enable: true, + Config: apiextensionsv1.JSON{Raw: []byte(`["10.0.0.0/8"]`)}, + }}, + } + + plugins, err := translator.buildPlugins(tctx, ar, rule) + assert.Error(t, err) + assert.Nil(t, plugins) +} + +func TestBuildPlugins_MalformedReferencedPluginConfigFailsTranslation(t *testing.T) { + translator := NewTranslator(logr.Discard()) + tctx := provider.NewDefaultTranslateContext(context.Background()) + tctx.ApisixPluginConfigs[types.NamespacedName{Namespace: "default", Name: "pc"}] = &apiv2.ApisixPluginConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "pc", Namespace: "default"}, + Spec: apiv2.ApisixPluginConfigSpec{ + Plugins: []apiv2.ApisixRoutePlugin{{ + Name: "ip-restriction", + Enable: true, + Config: apiextensionsv1.JSON{Raw: []byte(`["10.0.0.0/8"]`)}, + }}, + }, + } + + ar := &apiv2.ApisixRoute{ + ObjectMeta: metav1.ObjectMeta{Name: "test-route", Namespace: "default"}, + } + rule := apiv2.ApisixRouteHTTP{ + Name: "rule1", + PluginConfigName: "pc", + } + + plugins, err := translator.buildPlugins(tctx, ar, rule) + assert.Error(t, err) + assert.Nil(t, plugins) +} + +func TestTranslateApisixGlobalRule_MalformedPluginConfigFailsTranslation(t *testing.T) { + translator := NewTranslator(logr.Discard()) + tctx := provider.NewDefaultTranslateContext(context.Background()) + + obj := &apiv2.ApisixGlobalRule{ + ObjectMeta: metav1.ObjectMeta{Name: "test-global-rule", Namespace: "default"}, + Spec: apiv2.ApisixGlobalRuleSpec{ + Plugins: []apiv2.ApisixRoutePlugin{{ + Name: "ip-restriction", + Enable: true, + Config: apiextensionsv1.JSON{Raw: []byte(`["10.0.0.0/8"]`)}, + }}, + }, + } + + result, err := translator.TranslateApisixGlobalRule(tctx, obj) + assert.Error(t, err) + assert.Nil(t, result) +} + +func TestLoadPluginConfigPluginsForIngress_MalformedPluginConfigFailsTranslation(t *testing.T) { + translator := NewTranslator(logr.Discard()) + tctx := provider.NewDefaultTranslateContext(context.Background()) + tctx.ApisixPluginConfigs[types.NamespacedName{Namespace: "default", Name: "pc"}] = &apiv2.ApisixPluginConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "pc", Namespace: "default"}, + Spec: apiv2.ApisixPluginConfigSpec{ + Plugins: []apiv2.ApisixRoutePlugin{{ + Name: "ip-restriction", + Enable: true, + Config: apiextensionsv1.JSON{Raw: []byte(`["10.0.0.0/8"]`)}, + }}, + }, + } + + plugins, err := translator.loadPluginConfigPluginsForIngress(tctx, "default", "pc") + assert.Error(t, err) + assert.Nil(t, plugins) +} + +func TestFillPluginFromExtensionRef_MalformedPluginConfigFailsTranslation(t *testing.T) { + translator := NewTranslator(logr.Discard()) + tctx := provider.NewDefaultTranslateContext(context.Background()) + tctx.PluginConfigs[types.NamespacedName{Namespace: "default", Name: "pc"}] = &v1alpha1.PluginConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "pc", Namespace: "default"}, + Spec: v1alpha1.PluginConfigSpec{ + Plugins: []v1alpha1.Plugin{{ + Name: "ip-restriction", + Config: apiextensionsv1.JSON{Raw: []byte(`["10.0.0.0/8"]`)}, + }}, + }, + } + + plugins := make(adctypes.Plugins) + ref := &gatewayv1.LocalObjectReference{ + Kind: gatewayv1.Kind(internaltypes.KindPluginConfig), + Name: "pc", + } + err := translator.fillPluginFromExtensionRef(plugins, "default", ref, tctx) + assert.Error(t, err) + assert.Empty(t, plugins) +}