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/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") } 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)