AlinsRan commented on issue #2842:
URL:
https://github.com/apache/apisix-ingress-controller/issues/2842#issuecomment-5486887486
**The three sites and all three webhook observations check out against
master. Two of the conclusions that follow need changing, and both make this
issue smaller.**
### `L4RoutePolicy` needs no design decision — GEP-713 already answered it
The caveat "no webhook, decide how the failure surfaces first" doesn't
apply. `L4RoutePolicy` is a Policy Attachment resource: `TargetRefs` is
`gatewayv1.LocalPolicyTargetReferenceWithSectionName`, `PolicyStatus` is
`gatewayv1.PolicyStatus`, and `policies.go:328` already writes
`status.ancestors[].conditions` using `PolicyConditionAccepted` /
`PolicyReasonAccepted` / `PolicyReasonConflicted`. The failure surface exists
and is wired. Make `mergeL4PolicyPlugins` return an error and that path turns
it into `Accepted=False`.
### Declaring `type: object` on the CRD erases three of the four sites
None of these `config` fields declare a `type` — they carry only
`x-kubernetes-preserve-unknown-fields: true`, which is why `config: [1,2]`
reaches the translator at all. Adding the type rejects it at apply time and
makes `consumer.go`, `gateway.go:254` and `policies.go:266` unreachable in a
single change, across every path that reaches them rather than just these
three. It's breaking for configs that are already invalid today, so it needs a
release note.
### `pluginMetadata` is the one site left, and the only one with no GEP to
follow
`GatewayProxy.PluginMetadata` is `map[string]apiextensionsv1.JSON`
(`gatewayproxy_types.go:43`), and in the generated CRD its
`additionalProperties` carries no `type` either
(`apisix.apache.org_gatewayproxies.yaml:45-50`) — a marker on `Plugin.Config`
doesn't reach a map value. So `fillPluginMetadataFromGatewayProxy` still needs
the question this issue asks, and `GatewayProxy` is neither a Route nor a
Policy, so there's nothing upstream to model it on. That one is worth
discussing; the rest resolves itself.
Also worth keeping in view: failing closed on `GatewayProxy` has
gateway-wide blast radius, and `gatewayproxy_webhook.go` does not validate
through the translator (only `collectWarnings` and
`validateGatewayProxyConflict`), so a fail-closed change there would swap a
silent no-op for a silent rejection until the status condition exists.
### Keep the `cfg == nil` branch
`json.Unmarshal([]byte("null"), &cfg)` does set a non-nil map back to nil,
so the guard is correct Go. What makes it unreachable is the API server pruning
the explicit null — deleting it in the same change that rewrites those schemas
trades three lines for a dependency on schema behaviour. If it goes, the PR
should state that dependency.
### The shared defect under this and #2841 is that the translator can't
return errors
The `fill*` / `merge*` / `load*` family mostly has no error return, so a
parse failure has nowhere to go but a log line. Exactly five of them return an
error today — the five #2814 changed. Doing the rest as its own
no-behaviour-change PR would unblock both issues and stop the tree from growing
a third convention.
Note the fix does not generalise across the two, though, and the line is
drawn by gateway-api itself by resource category rather than by which group
defined the CRD: Route (HTTPRoute/GRPCRoute) → data-plane error scoped to the
rule, per `httproute_types.go:847-849`; Policy (`L4RoutePolicy`) → GEP-713
ancestor conditions; neither (`Consumer`, `GatewayProxy`) → schema rejection or
an ordinary status condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
