Copilot commented on code in PR #2814:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2814#discussion_r3627943106
##########
internal/adc/translator/httproute.go:
##########
@@ -59,38 +59,41 @@ 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{
Namespace: namespace,
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 {
Review Comment:
This block still accepts `config: null` without error because
`json.Unmarshal` into a map returns a nil map on `null`. That would set the
plugin config to `null` (not `{}`) and doesn't align with the new fail-hard
behavior for non-object configs. Also, the local `pluginconfig` map shadows the
outer `pluginconfig` object, making the logic harder to follow.
##########
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`} {
Review Comment:
The non-object rejection test cases don't cover `null`, which is a special
case: unmarshalling `null` into a map succeeds and yields a nil map. Adding
`null` here would prevent regressions where `config: null` slips through
despite the fail-hard intent of this PR.
##########
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)
Review Comment:
`fillPluginFromExtensionRef` now returns an error on unmarshal failures; it
would be good to add a regression test for the `config: null` case as well
(since unmarshalling `null` into a map does not return an error, but should
still be rejected once the translator enforces object-only configs).
##########
internal/adc/translator/apisixroute.go:
##########
@@ -117,33 +124,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)
}
}
Review Comment:
`json.Unmarshal` treats a literal `null` as a successful decode into a nil
map (no error). With the current logic that means `config: null` would be
accepted and could propagate a nil config (and potentially panic when merging
SecretRef data). Since this PR aims to reject non-object configs, `null` should
be rejected explicitly (or normalized), similar to the existing nil-map
handling in `mergeL4PolicyPlugins`
(internal/adc/translator/policies.go:272-276).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
