From a5d92cfacb5be94bc70aa092af9f6976252fd66d Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Sun, 19 Jul 2026 14:49:19 +0545 Subject: [PATCH 1/2] fix: fail loud when enable-csrf is set but csrf-key is missing Previously, when the enable-csrf annotation was set to true but the csrf-key annotation was missing or empty, csrf.Handle returned (nil, nil), which is indistinguishable from the annotation being absent. The route was then programmed without the csrf plugin and the Ingress reconciled cleanly, so an endpoint the operator believed was protected ran unprotected with no error, event, or log. Return an error in this case so the parser surfaces it instead of silently dropping the security-relevant plugin. The error message does not echo the key value. --- internal/adc/translator/annotations/plugins/csrf.go | 7 ++++++- .../adc/translator/annotations/plugins/csrf_test.go | 10 ++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/internal/adc/translator/annotations/plugins/csrf.go b/internal/adc/translator/annotations/plugins/csrf.go index 1bee407c..df4ab3c9 100644 --- a/internal/adc/translator/annotations/plugins/csrf.go +++ b/internal/adc/translator/annotations/plugins/csrf.go @@ -16,6 +16,8 @@ package plugins import ( + "fmt" + adctypes "github.com/apache/apisix-ingress-controller/api/adc" "github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations" ) @@ -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", + annotations.AnnotationsEnableCsrf, annotations.AnnotationsCsrfKey) } return &adctypes.CSRFConfig{ diff --git a/internal/adc/translator/annotations/plugins/csrf_test.go b/internal/adc/translator/annotations/plugins/csrf_test.go index 0f681703..b7358714 100644 --- a/internal/adc/translator/annotations/plugins/csrf_test.go +++ b/internal/adc/translator/annotations/plugins/csrf_test.go @@ -43,10 +43,16 @@ func TestCSRFHandler(t *testing.T) { assert.Nil(t, err, "checking given error") assert.Nil(t, out, "checking given output") - // Test with enable-csrf true but no key + // Test with enable-csrf true but no key: must fail loud, not drop silently anno[annotations.AnnotationsEnableCsrf] = "true" delete(anno, annotations.AnnotationsCsrfKey) out, err = p.Handle(annotations.NewExtractor(anno)) - assert.Nil(t, err, "checking given error") + assert.Error(t, err, "expecting an error when csrf-key is missing") assert.Nil(t, out, "checking given output when key is missing") + + // Test with enable-csrf true but empty key: same fail-loud behavior + anno[annotations.AnnotationsCsrfKey] = "" + out, err = p.Handle(annotations.NewExtractor(anno)) + assert.Error(t, err, "expecting an error when csrf-key is empty") + assert.Nil(t, out, "checking given output when key is empty") } From 2d1b3ba2e647b6dbd0a37f23b9ec7098e8bcc0eb Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Fri, 24 Jul 2026 15:57:44 +0545 Subject: [PATCH 2/2] fix: fail Ingress translation when an annotation handler errors Returning an error from csrf.Handle was not enough to stop an unprotected route from being programmed: the error was swallowed at three separate points. 1. plugins.Parse logged the handler error and continued, returning a nil error. 2. TranslateIngressAnnotations logged the parser error and returned the config anyway. 3. TranslateIngress had no error to inspect, since the function above returned only a config. Propagate the error through all three so TranslateIngress fails. The provider aborts before building the sync task, so the route is no longer programmed and the failure surfaces on the reconcile instead of in a log line nobody reads. This also stops an invalid upstream-scheme/retry/timeout annotation from being silently ignored, which had the same silent-downgrade shape (a typo'd "https" fell back to plaintext http). --- internal/adc/translator/annotations.go | 8 ++--- .../translator/annotations/plugins/plugins.go | 9 +++-- internal/adc/translator/annotations_test.go | 34 ++++++++++++++++--- internal/adc/translator/ingress.go | 5 ++- 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/internal/adc/translator/annotations.go b/internal/adc/translator/annotations.go index 49486dc3..b91b2f09 100644 --- a/internal/adc/translator/annotations.go +++ b/internal/adc/translator/annotations.go @@ -50,15 +50,15 @@ var ingressAnnotationParsers = map[string]annotations.IngressAnnotationsParser{ "UseRegex": regex.NewParser(), } -func (t *Translator) TranslateIngressAnnotations(anno map[string]string) *IngressConfig { +func (t *Translator) TranslateIngressAnnotations(anno map[string]string) (*IngressConfig, error) { if len(anno) == 0 { - return nil + return nil, nil } ing := &IngressConfig{} if err := translateAnnotations(anno, ing); err != nil { - t.Log.Error(err, "failed to translate ingress annotations", "annotations", anno) + return nil, err } - return ing + return ing, nil } func translateAnnotations(anno map[string]string, dst any) error { diff --git a/internal/adc/translator/annotations/plugins/plugins.go b/internal/adc/translator/annotations/plugins/plugins.go index 2475ef55..7843ab8f 100644 --- a/internal/adc/translator/annotations/plugins/plugins.go +++ b/internal/adc/translator/annotations/plugins/plugins.go @@ -15,7 +15,7 @@ package plugins import ( - logf "sigs.k8s.io/controller-runtime/pkg/log" + "fmt" adctypes "github.com/apache/apisix-ingress-controller/api/adc" "github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations" @@ -33,8 +33,6 @@ type PluginAnnotationsHandler interface { } var ( - log = logf.Log.WithName("annotations").WithName("plugins").WithName("parser") - handlers = []PluginAnnotationsHandler{ NewRedirectHandler(), NewRewriteHandler(), @@ -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) } if out != nil { plugins[handler.PluginName()] = out diff --git a/internal/adc/translator/annotations_test.go b/internal/adc/translator/annotations_test.go index 8c8b1b96..b9fc28c0 100644 --- a/internal/adc/translator/annotations_test.go +++ b/internal/adc/translator/annotations_test.go @@ -92,6 +92,7 @@ func TestTranslateIngressAnnotations(t *testing.T) { name string anno map[string]string expected *IngressConfig + wantErr bool }{ { name: "no matching annotations", @@ -99,9 +100,27 @@ func TestTranslateIngressAnnotations(t *testing.T) { 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, }, { name: "http scheme", @@ -343,8 +362,15 @@ func TestTranslateIngressAnnotations(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { translator := &Translator{} - result := translator.TranslateIngressAnnotations(tt.anno) + result, err := translator.TranslateIngressAnnotations(tt.anno) + + if tt.wantErr { + assert.Error(t, err) + assert.Nil(t, result) + return + } + assert.NoError(t, err) assert.NotNil(t, result) assert.Equal(t, tt.expected, result) }) diff --git a/internal/adc/translator/ingress.go b/internal/adc/translator/ingress.go index 36e02c4e..e6082fb8 100644 --- a/internal/adc/translator/ingress.go +++ b/internal/adc/translator/ingress.go @@ -82,7 +82,10 @@ func (t *Translator) TranslateIngress( labels := label.GenLabel(obj) - config := t.TranslateIngressAnnotations(obj.Annotations) + config, err := t.TranslateIngressAnnotations(obj.Annotations) + if err != nil { + return nil, err + } t.Log.V(1).Info("translating Ingress Annotations", "config", config)