api7 / api7/api7-ingress-controller
Plugin config unmarshal failures still log-and-skip in consumer.go, gateway.go and policies.go
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 5
- Forks
- 5
- Avg merge
- 2d 7h
- Merged PRs (30d)
- 10
Description
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.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
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.
Upstream: apache/apisix-ingress-controller#2842
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Trace the log-and-skip sites in internal/adc/translator/consumer.go, gateway.go, and policies.go, then inspect adc_validation.go and gatewayproxy_webhook.go to determine which failures are observable. Review the envtest behavior described for generated CRDs, including mergeL4PolicyPlugins. Done means each path has an intentional, documented failure convention with coverage for the affected plugin or policy behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100