AlinsRan commented on code in PR #2813:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2813#discussion_r3654014927
##########
internal/adc/translator/annotations/plugins/plugins.go:
##########
@@ -60,8 +58,9 @@ func (p *plugins) Parse(e annotations.Extractor) (any, error)
{
for _, handler := range handlers {
out, err := handler.Handle(e)
if err != nil {
- log.Error(err, "Failed to handle annotation",
"handler", handler.PluginName())
- continue
+ // Fail closed: abort translation so the route is never
programmed
+ // while a requested plugin is missing.
+ return nil, fmt.Errorf("%s: %w", handler.PluginName(),
err)
Review Comment:
This is the line that makes the change much bigger than csrf. Combined with
`translateAnnotations` joining every parser error, the set of annotations that
can now fail the whole Ingress is: `upstream-scheme`, `upstream-retry` and the
three timeouts (upstream.go:51,58,66,74,82), a non-compiling rewrite regex
(rewrite.go:54), and csrf.
So an Ingress that has carried a typo'd annotation for months while happily
serving traffic stops reconciling on upgrade, and once the controller restarts
and rebuilds the ADC baseline the route disappears from the data plane — a 404
in production caused by an annotation that was previously inert.
I'd keep the fail-closed semantics, but this needs to ship with an entry in
`docs/en/latest/upgrade-guide.md` calling it out explicitly.
##########
internal/adc/translator/annotations/plugins/csrf.go:
##########
@@ -39,7 +41,10 @@ func (c *csrf) Handle(e annotations.Extractor) (any, error) {
key := e.GetStringAnnotation(annotations.AnnotationsCsrfKey)
if key == "" {
- return nil, nil
+ // csrf requested but the key is missing: fail loud instead of
+ // silently dropping the plugin and programming the route
unprotected.
+ return nil, fmt.Errorf("annotation %q is enabled but %q is
missing or empty",
Review Comment:
For this specific check — `enable-csrf` set, `csrf-key` empty — the
translator is the late place to catch it. It's a purely static property of the
object, and there is already an Ingress validating webhook
(`internal/webhook/v1/ingress_webhook.go`, path
`/validate-networking-k8s-io-v1-ingress`, currently doing SSL conflict
detection). Rejecting there means `kubectl apply` fails immediately with the
reason, instead of an object that looks accepted and quietly never reconciles.
The translator check should stay as the backstop, since the webhook is
registered with `failurePolicy=Ignore`. Fine as a follow-up, but worth stating
the intent here.
##########
internal/adc/translator/annotations_test.go:
##########
@@ -95,16 +95,35 @@ func TestTranslateIngressAnnotations(t *testing.T) {
name string
anno map[string]string
expected *IngressConfig
+ wantErr bool
}{
{
name: "no matching annotations",
anno: map[string]string{"upstream": "value1"},
expected: &IngressConfig{},
},
{
- name: "invalid scheme",
- anno:
map[string]string{annotations.AnnotationsUpstreamScheme: "invalid"},
- expected: &IngressConfig{},
+ // enable-csrf without a key must fail translation,
never reconcile
+ // into a route that silently lacks the csrf plugin.
+ name: "enable-csrf without csrf-key",
+ anno: map[string]string{
+ annotations.AnnotationsEnableCsrf: "true",
+ },
+ wantErr: true,
+ },
+ {
+ name: "enable-csrf with empty csrf-key",
+ anno: map[string]string{
+ annotations.AnnotationsEnableCsrf: "true",
+ annotations.AnnotationsCsrfKey: "",
+ },
+ wantErr: true,
+ },
+ {
+ // a typo'd scheme used to fall back to plaintext http
silently
+ name: "invalid scheme",
+ anno:
map[string]string{annotations.AnnotationsUpstreamScheme: "invalid"},
+ wantErr: true,
Review Comment:
This flipped assertion is the breaking change in miniature:
`upstream-scheme: invalid` went from "ignored, route works" to "entire Ingress
fails to translate". Right behavior, but the test only proves translation
returns an error — nothing proves the consequence the PR description leads
with, that the route is never programmed.
Could you add an e2e that applies an Ingress with `enable-csrf: "true"` and
no `csrf-key` and asserts the route is absent from the data plane? That's the
invariant worth locking down; the unit test alone would still pass if someone
later reinstated a `log and continue` at the provider layer.
--
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]
