apache / apache/apisix-ingress-controller

Plugin config unmarshal failures still log-and-skip in consumer.go, gateway.go and policies.go

Open
#2,842 1 comment 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.