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