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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
