api7 / api7/api7-ingress-controller

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

Open
#454 2 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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 Consumeradc_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.

Upstream: apache/apisix-ingress-controller#2842

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.