You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "shreemaan-abhishek (via GitHub)" <gi...@apache.org> on 2026/07/20 10:57:23 UTC
[PR] fix: reject invalid plugin config instead of applying an empty one [apisix-ingress-controller]
shreemaan-abhishek opened a new pull request, #2814:
URL: https://github.com/apache/apisix-ingress-controller/pull/2814
### Type of change:
- [x] Bugfix
### What this PR does / why we need it:
When a plugin's `config` fails to unmarshal into an object (for example a
non-object value like `config: ["10.0.0.0/8"]`, which the CRD accepts
because `config` uses `x-kubernetes-preserve-unknown-fields`), the error
was logged and discarded, and the plugin was still applied with an empty
(or secret-only) config. So an `ip-restriction` plugin with a malformed
`whitelist` silently became `ip-restriction: {}` on the published route,
enforcing nothing while the resource reconciled green. The same path
serves `ApisixGlobalRule`, so a malformed global plugin config was
applied gateway-wide in mutated form.
This changes `buildPluginConfig` to return an error on a malformed
config and propagates it through every caller (route plugins,
stream-route plugins, referenced `ApisixPluginConfig`, `ApisixGlobalRule`,
Ingress plugin-config annotation, `ApisixConsumer` plugins). Translation
now fails instead of publishing a fabricated empty object, so the
resource keeps its last good state and the admission webhook rejects the
invalid config upfront. `fillPluginFromExtensionRef`, which silently
skipped such plugins, is aligned to the same fail-hard behavior.
### Pre-submission checklist:
- [x] Did you explain what problem does this PR solve? Or what new features have been added?
- [x] Have you added corresponding test cases?
- [ ] Have you modified the corresponding document?
- [x] Is this PR backward compatible?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: reject invalid plugin config instead of applying an empty one [apisix-ingress-controller]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #2814:
URL: https://github.com/apache/apisix-ingress-controller/pull/2814#issuecomment-5127841281
Checked this against a real API server (envtest, CRDs from `config/crd/bases`) rather than reasoning from the Go semantics alone, and `config: null` is not reachable.
The API server prunes the explicit null before it is persisted, so `plugin.Config.Raw` comes back empty rather than `[]byte("null")`:
```
ApisixRoute ACCEPTED (null) -> stored raw="" len=0
ApisixPluginConfig ACCEPTED (null) -> stored raw="" len=0
ApisixRoute ACCEPTED (["10.0.0.0/8"]) -> stored raw="[\"10.0.0.0/8\"]" len=14
ApisixRoute ACCEPTED ("oops") -> stored raw="\"oops\"" len=6
ApisixRoute ACCEPTED ({"whitelist":["10.0.0.0/8"]}) -> stored raw="{\"whitelist\":[\"10.0.0.0/8\"]}" len=28
```
With an empty `Raw` the `len(plugin.Config.Raw) > 0` guard is false, so the unmarshal is skipped entirely and `config` stays the empty non-nil map from `make`. There is no nil map, so no nil-map write in the `secretRef` merge and no `null` published for the plugin. The same holds on the `ApisixPluginConfig` path that `fillPluginFromExtensionRef` reads.
The non-object cases the PR does target (array, string, number) are persisted verbatim, which is what the new tests cover.
The variable shadowing in `fillPluginFromExtensionRef` was a fair point and is fixed.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: reject invalid plugin config instead of applying an empty one [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2814:
URL: https://github.com/apache/apisix-ingress-controller/pull/2814#discussion_r3681629544
##########
internal/adc/translator/httproute.go:
##########
@@ -676,7 +679,9 @@ func (t *Translator) TranslateHTTPRoute(tctx *provider.TranslateContext, httpRou
enableWebsocket, _ := t.translateBackendsToUpstreams(tctx, rule, httpRoute, service)
- 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 {
Review Comment:
Two consequences of aborting here worth being explicit about:
- It drops **every rule** of the HTTPRoute, not just the rule whose filter failed. Gateway API scopes filter/backendRef failures to the requests that would have hit them, never to the whole route.
- The failure is invisible. `httproute_controller.go` writes the status (`Accepted=True` / `ResolvedRefs=True`) *before* calling `Provider.Update`, and an error from `Update` only requeues — so the route reports accepted while nothing is programmed. There is no admission gate on this path either: `httproute_webhook.go` only does `collectWarnings` (no `adcValidator`), and v1alpha1 `PluginConfig` has no webhook at all. So unlike the apiv2 paths, the user gets no signal from either side.
Before this change the state was wrong but observable-by-behavior; after it, status and data plane disagree silently. If the fail-hard shape is kept for now, the status update needs to move after `Provider.Update` (or the error be folded into the condition).
##########
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
Review Comment:
Note this is the branch the spec text literally governs — "a reference to a custom filter type cannot be resolved" — and it still skips silently: the route is programmed with no plugins at all. The PR hardens the case the spec says nothing about (a resolvable extension object whose own config is malformed) and leaves this one as-is. Whatever shape is chosen for the malformed-config case should cover this branch too. Same for `loadPluginConfigPlugins` in `apisixroute.go`, where a missing `ApisixPluginConfig` silently drops all plugins of the rule.
##########
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:
Consider naming the object the config came from. When the plugin lives in a referenced `ApisixPluginConfig`, this message surfaces on the route's status and the user cannot tell which `ApisixPluginConfig` (of possibly several) is broken. `buildPluginConfig` already receives the namespace; passing the source name through would make the message actionable.
##########
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)
+ config := 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
+ if err := json.Unmarshal(plugin.Config.Raw, &config); err != nil {
+ return fmt.Errorf("failed to unmarshal config of plugin %s: %w", plugin.Name, err)
Review Comment:
This diverges from Gateway API, and in a way the codebase already has an answer for.
The spec's rule for a custom (`ExtensionRef`) filter that cannot be applied is that the route keeps matching and the affected requests get an error response — it must not resolve to "the filter is not there":
> If a reference to a custom filter type cannot be resolved, the filter MUST NOT be skipped. Instead, requests that would have been processed by that filter MUST receive a HTTP error response.
[gateway-api v1.6.0 `httproute_types.go#L847-L849`](https://github.com/kubernetes-sigs/gateway-api/blob/v1.6.0/apis/v1/httproute_types.go#L847-L849) ([rendered](https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.HTTPRouteFilter)). Same shape for backendRefs, where the failure semantics are explicitly per-request and local: [`httproute_types.go#L271-L282`](https://github.com/kubernetes-sigs/gateway-api/blob/v1.6.0/apis/v1/httproute_types.go#L271-L282).
Returning an error here means the route is never programmed, which is not equivalent: if another route also matches, the request is served **without** the filter — the outcome the sentence above forbids — and if nothing else matches it is a 404, indistinguishable from "no such route". It also widens a per-filter failure into a whole-route one (see the call site), and one bad `PluginConfig` takes down every route referencing it.
The conformant pattern is already implemented ~40 lines below, for backendRefs: keep the route, attach a `fault-injection` 500 — [`httproute.go#L663-L681`](https://github.com/apache/apisix-ingress-controller/blob/dad45c50c15152fdc71e875155cad1f039060cdb/internal/adc/translator/httproute.go#L663-L681), whose error is deliberately discarded at the call site precisely because it has been turned into a data-plane 500. Suggest reusing that here instead of failing translation, plus `ResolvedRefs=False` on the route. Given that, it may be cleanest to drop `httproute.go` / `grpcroute.go` from this PR and do them separately — the apiv2 paths don't carry these constraints.
##########
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)
Review Comment:
This makes apiv2 `ApisixConsumer` fail hard while its v1alpha1 counterpart keeps the old behavior — `consumer.go` logs and `continue`s for both the credential config and the consumer plugins, so a malformed credential is dropped and the consumer is published without it.
Still on the log-and-skip path after this PR:
- `consumer.go` — v1alpha1 Consumer credential config, and Consumer plugins
- `gateway.go` — GatewayProxy `plugins` and `pluginMetadata` (gateway-wide, the same blast radius the PR description cites for `ApisixGlobalRule`)
- `policies.go` — L4RoutePolicy plugins
The failure mode differs (they `continue`, so the plugin vanishes rather than becoming `{}`), but the user-visible outcome is the same class: the plugin does not run and everything reconciles green. Either sweep them in the same PR or say in the description why they are out of scope — otherwise the tree ends up with two deliberate, opposite conventions for the same situation.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: reject invalid plugin config instead of applying an empty one [apisix-ingress-controller]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: reject invalid plugin config instead of applying an empty one [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2814:
URL: https://github.com/apache/apisix-ingress-controller/pull/2814#discussion_r3681629555
##########
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:
Consider naming the object the config came from. When the plugin lives in a referenced `ApisixPluginConfig`, this message surfaces on the route's status and the user cannot tell which `ApisixPluginConfig` (of possibly several) is broken. `buildPluginConfig` already receives the namespace; passing the source name through would make the message actionable.
##########
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
Review Comment:
Note this is the branch the spec text literally governs — "a reference to a custom filter type cannot be resolved" — and it still skips silently: the route is programmed with no plugins at all. The PR hardens the case the spec says nothing about (a resolvable extension object whose own config is malformed) and leaves this one as-is. Whatever shape is chosen for the malformed-config case should cover this branch too. Same for `loadPluginConfigPlugins` in `apisixroute.go`, where a missing `ApisixPluginConfig` silently drops all plugins of the rule.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: reject invalid plugin config instead of applying an empty one [apisix-ingress-controller]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #2814:
URL: https://github.com/apache/apisix-ingress-controller/pull/2814#issuecomment-5215756232
Thanks, all three land. Dropped `httproute.go` and `grpcroute.go` from this PR; it is now the apiv2 and Ingress paths only.
**Gateway API conformance.** Agreed, and returning an error is the worse of the two wrong answers: the route is never programmed, so a request either falls through to another matching route and is served without the filter (the outcome the spec forbids) or 404s. The `backendErr` → `fault-injection` 500 block is the right shape to reuse, scoped per rule with `ResolvedRefs=False`.
**Blast radius and visibility.** Confirmed both halves. `TranslateHTTPRoute` aborts on the first bad filter, so every rule of the route goes, and `Reconcile` persists the status before calling `Provider.Update`, so a translation error only requeues while the object still reports `Accepted=True`. `grpcroute_controller.go` has the same ordering. That one is worth fixing on its own regardless of this PR, since it makes any failure on those paths invisible.
For what it is worth, the paths that remain here do surface the failure: `ApisixRoute` and `ApisixConsumer` run the real translator through `adcValidator` in the admission webhook, and `ApisixGlobalRule` and `Ingress` call `Provider.Update` before writing status, so the error reaches a condition. That asymmetry is exactly why the split makes sense.
**Two conventions.** Fair, and I would rather not leave it implicit, so the description now states what is excluded and why. The remaining sites need individual decisions rather than one sweep: v1alpha1 `Consumer` has an `adcValidator` case and could fail closed like apiv2 `ApisixConsumer`, but `GatewayProxy` and `L4RoutePolicy` need their failure to be observable first, and `L4RoutePolicy` has no webhook at all.
One aside from the earlier `config: null` discussion: `mergeL4PolicyPlugins` normalizes a nil `cfg`, but that branch is unreachable. For a field typed `apiextensionsv1.JSON` with `x-kubernetes-preserve-unknown-fields` and no `nullable: true`, the API server prunes the explicit null, so `Config.Raw` arrives empty and the `len(Raw) > 0` guard skips the unmarshal. Verified with envtest against the generated CRDs on both `ApisixRoute` and `ApisixPluginConfig`. Worth removing whenever that file is touched.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: reject invalid plugin config instead of applying an empty one [apisix-ingress-controller]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek merged PR #2814:
URL: https://github.com/apache/apisix-ingress-controller/pull/2814
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org