Problem
#441 made the apiv2 plugin-config paths fail closed: a config that does not unmarshal into an object now fails translation instead of publishing the plugin with an empty config. Several equivalent sites still log the error and continue, so the plugin silently vanishes and the resource reconciles green.
Still on the log-and-skip path:
| Site |
What is dropped |
internal/adc/translator/consumer.go |
v1alpha1 Consumer credential config, and Consumer plugins |
internal/adc/translator/gateway.go |
GatewayProxy plugins and pluginMetadata (gateway-wide blast radius) |
internal/adc/translator/policies.go |
L4RoutePolicy plugins (mergeL4PolicyPlugins) |
The failure mode differs from the one #441 fixed — these continue, so the plugin disappears rather than becoming {} — but the user-visible outcome is the same class: the plugin does not run and nothing reports a problem. Left as is, the tree carries two deliberate and opposite conventions for the same situation.
Expected behavior
Settle on one convention per path. The right treatment is not uniform, because it depends on whether the failure is observable:
- v1alpha1
Consumer — adc_validation.go already has a case *v1alpha1.Consumer, so the admission webhook runs the real translator and a returned error is rejected at apply time. Fail closed here, matching apiv2 ApisixConsumer.
GatewayProxy — needs a check of whether gatewayproxy_webhook.go validates through the translator. If not, failing closed would be silent, and the failure needs a status condition first.L4RoutePolicy — no webhook. Same caveat: decide how the failure surfaces before changing the behavior.
Whichever way each one lands, the PR should say why, so the convention is documented rather than implicit.
Notes
mergeL4PolicyPlugins also carries an explicit cfg == nil normalization for a literal config: null. 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 entirely. Verified with envtest against the generated CRDs, on both ApisixRoute and ApisixPluginConfig. Worth removing while the file is being touched.
Context
Raised in review on #441.
Problem
#441 made the apiv2 plugin-config paths fail closed: a
configthat does not unmarshal into an object now fails translation instead of publishing the plugin with an empty config. Several equivalent sites still log the error andcontinue, so the plugin silently vanishes and the resource reconciles green.Still on the log-and-skip path:
internal/adc/translator/consumer.goConsumercredential config, andConsumerpluginsinternal/adc/translator/gateway.goGatewayProxypluginsandpluginMetadata(gateway-wide blast radius)internal/adc/translator/policies.goL4RoutePolicyplugins (mergeL4PolicyPlugins)The failure mode differs from the one #441 fixed — these
continue, so the plugin disappears rather than becoming{}— but the user-visible outcome is the same class: the plugin does not run and nothing reports a problem. Left as is, the tree carries two deliberate and opposite conventions for the same situation.Expected behavior
Settle on one convention per path. The right treatment is not uniform, because it depends on whether the failure is observable:
Consumer—adc_validation.goalready has acase *v1alpha1.Consumer, so the admission webhook runs the real translator and a returned error is rejected at apply time. Fail closed here, matching apiv2ApisixConsumer.GatewayProxy— needs a check of whethergatewayproxy_webhook.govalidates through the translator. If not, failing closed would be silent, and the failure needs a status condition first.L4RoutePolicy— no webhook. Same caveat: decide how the failure surfaces before changing the behavior.Whichever way each one lands, the PR should say why, so the convention is documented rather than implicit.
Notes
mergeL4PolicyPluginsalso carries an explicitcfg == nilnormalization for a literalconfig: null. That branch is unreachable: for a field typedapiextensionsv1.JSONwithx-kubernetes-preserve-unknown-fieldsand nonullable: true, the API server prunes the explicit null, soConfig.Rawarrives empty and thelen(Raw) > 0guard skips the unmarshal entirely. Verified with envtest against the generated CRDs, on bothApisixRouteandApisixPluginConfig. Worth removing while the file is being touched.Context
Raised in review on #441.