apache / apache/apisix-ingress-controller
Plugin config unmarshal failures still log-and-skip in consumer.go, gateway.go and policies.go
- Dominant language
- Go
- Stars
- 1.1k
- Forks
- 390
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 12
Description
### Problem
#2814 makes 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 #2814 fixes — 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`** — `internal/webhook/v1/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 #2814.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by comparing the existing fail-closed behavior from #2814 with the log-and-skip sites in internal/adc/translator/consumer.go, gateway.go, and policies.go. Read internal/webhook/v1/adc_validation.go and gatewayproxy_webhook.go, then use the existing translator or envtest coverage to determine how each failure surfaces. Done means each path has an intentional, tested outcome and the PR explains the convention chosen.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- api, backend, devops
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100