AlinsRan commented on issue #2841: URL: https://github.com/apache/apisix-ingress-controller/issues/2841#issuecomment-5486884256
**Approach agreed — `fault-injection` is the right shape, and since GRPCRoute shares `fillPluginFromExtensionRef` one change covers both. Two things about scope, and a correction on the #2840 dependency.** ### A missing PluginConfig, not a malformed one, is the case that hurts This is framed around a `config` that fails to unmarshal — the rarest of the three ways `fillPluginFromExtensionRef` fails open. If the PluginConfig doesn't exist it `return`s and drops the whole ExtensionRef; if the Kind isn't `PluginConfig` the branch is never entered. Those two are what the spec sentence is about — a *reference* that cannot be resolved — and a missing PluginConfig is the ordinary case: apply ordering, a deleted object, a misspelled name. Fix only the unmarshal path and a route whose filter carries `key-auth` still goes live unauthenticated. ### The spec scopes the failure to the rule, and puts the status on `ResolvedRefs` — not `Accepted` Three things in the spec are easy to merge into one, and they're handled differently: 1. **ExtensionRef cannot be resolved** (`httproute_types.go:847-849`) — "**requests that would have been processed by that filter** MUST receive a HTTP error response". The wording scopes it to the rule, and requires the route to exist and answer. That rules out failing translation: a route that is never programmed yields a 404, or lets another route serve the request without the filter. 2. **Unknown `filter.Type` enum value** (`:854-856`) — `Accepted=False` + `UnsupportedValue`. This is about the enum, not about a reference that won't resolve. 3. **Unsupported filter combination or ordering** (`HTTPRouteRule.Filters` doc, `shared_types.go:444-478`) — rule invalid, `PartiallyInvalid`, and note `Accepted` MUST stay `True` on the Drop Rule path. Not applicable here either: attaching a 500 doesn't drop the rule. For (1) the spec names no condition, but `ResolvedRefs` covers it by definition — "whether the controller was able to resolve **all the object references** for the Route" (`shared_types.go:400-401`); `extensionRef` is a `LocalObjectReference`. No listed reason fits exactly, and `:414-416` explicitly permits other reasons. So the correct end state is `Accepted=True` + `ResolvedRefs=False` + the route programmed with a 500 on the affected rule. All three hold at once. ### That error is currently routed into `Accepted`, which is why this conflicts with #2840 rather than depending on it For the missing-PluginConfig case, `processHTTPRoute` sets `terror` at `httproute_controller.go:565`, which lands in `acceptStatus` at `:192` — so `Accepted=False` is written today. The route is programmed anyway, because `isRouteAccepted(gateways)` (`utils.go:1469`) only reads the Gateway parent conditions and never consults `acceptStatus`. Live behaviour is `Accepted=False` in status *and* an unauthenticated route in the data plane. That gap is what currently lets a fix here work at all. If #2840 is resolved by making the gate honour `acceptStatus` first, a missing PluginConfig stops the route from being programmed — and the 500 attached here never ships, which is exactly the outcome (1) forbids. So this needs to move the error off `Accepted` and onto `ResolvedRefs` **before** that gate changes, and #2840's fix has to exempt ResolvedRefs-class errors from blocking programming. ### Three things that will bite in implementation - The backend path writes `service.Plugins["fault-injection"]` at `:706`, filters run at `:709` — a plain assignment from the filter side silently overwrites the backend's message. - `fillPluginsFromHTTPRouteFilters` (`httproute.go:43`) returns nothing; propagating an error means changing its signature and both callers. - `ResolvedRefs` can't be set from the translator: `SetRouteConditionResolvedRefs` only consumes `backendRefErr`, set only for `RouteReasonInvalidKind` (`:190-198`). The controller's error routing has to change too. ### Conformance won't catch a mistake here ExtensionRef is Implementation-specific and the v1.6.0 conformance suite has no tests touching it. The MUST still stands, but review is the only thing enforcing it. -- 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]
