AlinsRan commented on code in PR #2813:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2813#discussion_r3662242235
##########
internal/webhook/v1/ingress_webhook.go:
##########
@@ -82,6 +83,10 @@ func (v *IngressCustomValidator) ValidateCreate(ctx
context.Context, obj runtime
return nil, fmt.Errorf("%s",
sslvalidator.FormatConflicts(conflicts))
}
+ if err := validateAnnotations(ingress); err != nil {
Review Comment:
The shipped Helm chart defaults `webhook.enabled: true`
(`charts/apisix-ingress-controller/values.yaml` in apisix-helm-chart), so
admission does run on the default deployment path. The `false` you're looking
at is the Go default in `NewWebhookConfig()` / `config/samples/config.yaml`,
which only applies when running the binary against a hand-written config.
That said, there is a real residual gap, just not the one you describe: the
translator path is *completely* silent today — `csrf.Handle` returns `nil, nil`
when the key is missing (`csrf.go:42`). With the webhook disabled, or for an
Ingress that predates the upgrade and therefore never gets re-admitted, there
is no signal at all. Let's fix that here: have `csrf.Handle` return an error
instead. `plugins.Parse` already logs it and skips only that plugin, so the
rest of the Ingress (TLS included) is untouched. webhook + error log is enough.
On "fail-closed for updates" — that was the previous revision of this PR and
it was reverted for the reasons discussed above: `Provider.Update` returns on
the translation error *before* `UpdateConfig`, so the old unprotected route
stays live in the data plane anyway, and dropping a route doesn't stop traffic
either, requests fall through to a broader wildcard route. Fail-closed there
doesn't protect anything, it just swaps one broken state for another.
Making admission mandatory for a single annotation is a policy change well
beyond the scope of this fix. If you think the webhook should be a hard
dependency, that's worth its own discussion.
--
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]
