shreemaan-abhishek opened a new pull request, #2909: URL: https://github.com/apache/apisix-ingress-controller/pull/2909
### What problem does this PR solve? An APISIX annotation that fails to parse is logged and discarded, and the Ingress is published without the configuration it asked for. Two paths lead there: - `plugins.Parse` loops over the plugin handlers and, on a handler error, logs and `continue`s. The error never leaves the parser, so the caller sees a clean parse. `enable-csrf` with no `csrf-key` and an uncompilable `rewrite-target-regex` both produce a live route missing that plugin. - `translateAnnotations` collects the per-parser errors, and `TranslateIngressAnnotations` logs the joined error and returns the partial config. An invalid `upstream-scheme` or a non-integer `upstream-retries` drops the whole upstream block: scheme, retries and all three timeouts. In both cases `kubectl apply` succeeds, the route serves traffic, and a controller log line is the only trace. The author believes a plugin is active when it is not. ### What is changed and how does it work? - `plugins.Parse` returns its handler errors joined, together with the plugins that did parse. - `translateAnnotations` records a parser error and still merges that parser's non-nil output, so reporting the failure does not change what gets published. - A new `translator.ValidateIngressAnnotations` runs the parsers over an annotation map and returns the joined error. - The Ingress admission webhook calls it in `ValidateCreate` and `ValidateUpdate`, so an unparsable annotation fails `kubectl apply` with a message naming the parser. This replaces the single hardcoded `enable-csrf`/`csrf-key` rule with one rule that covers every parser, including handlers that grow error paths later. - `IngressReconciler` records a `Warning`/`InvalidAnnotations` event for what the webhook cannot reach: a disabled webhook, its `failurePolicy: Ignore`, and Ingresses admitted before this validation existed. Translation keeps publishing whatever parsed. Upgrading the controller therefore does not retract a route that is already serving traffic, which is why the translator is not made to fail hard here. Events are already covered by the controller's RBAC (`groups="",resources=events,verbs=create;patch`), so no manifest change is needed. ### Tests - `ValidateIngressAnnotations` table test: missing `csrf-key`, uncompilable `rewrite-target-regex`, invalid `upstream-scheme`, non-integer `upstream-retries`, and the valid cases. - Webhook tests asserting rejection on both create and update for each case, and acceptance of the equivalent valid annotations. - Reconciler tests for the Warning event, for silence when the annotations parse, and for a reconciler built without a recorder. - Regression tests that the new error does not change published config: a parser returning both output and an error still has its output merged, and a failing `csrf` handler leaves `ip-restriction`, `upstream.Scheme` and `EnableWebsocket` intact. ``` go build ./... go vet ./internal/... go test ./internal/adc/translator/... ./internal/webhook/... ./internal/controller/ ``` All pass. -- 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]
