fix: fail loud when enable-csrf is set but csrf-key annotation is missing

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,11 @@ func (c *csrf) Handle(e annotations.Extractor) (any, error) {
 
 	key := e.GetStringAnnotation(annotations.AnnotationsCsrfKey)
 	if key == "" {
-		return nil, nil
+		// csrf requested without a key: the webhook rejects this, but log a
+		// breadcrumb for the cases it cannot cover (webhook off, pre-upgrade
+		// Ingress). Parse skips only this plugin, the rest of the route stands.
+		return nil, fmt.Errorf("annotation %q is enabled but %q is missing or empty",
+			annotations.AnnotationsEnableCsrf, annotations.AnnotationsCsrfKey)
 	}
 
 	return &adctypes.CSRFConfig{

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: errors so Parse logs a breadcrumb
 	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 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")
 }

internal/webhook/v1/ingress_webhook.go

@@ -28,6 +28,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
+	"github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations"
 	"github.com/apache/apisix-ingress-controller/internal/controller"
 	"github.com/apache/apisix-ingress-controller/internal/webhook/v1/reference"
 	sslvalidator "github.com/apache/apisix-ingress-controller/internal/webhook/v1/ssl"
@@ -76,6 +77,10 @@ func (v *IngressCustomValidator) ValidateCreate(ctx context.Context, obj runtime
 		return nil, nil
 	}
 
+	if err := validateAnnotations(ingress); err != nil {
+		return nil, err
+	}
+
 	detector := sslvalidator.NewConflictDetector(v.Client)
 	conflicts := detector.DetectConflicts(ctx, ingress)
 	if len(conflicts) > 0 {
@@ -97,6 +102,10 @@ func (v *IngressCustomValidator) ValidateUpdate(ctx context.Context, oldObj, new
 		return nil, nil
 	}
 
+	if err := validateAnnotations(ingress); err != nil {
+		return nil, err
+	}
+
 	detector := sslvalidator.NewConflictDetector(v.Client)
 	conflicts := detector.DetectConflicts(ctx, ingress)
 	if len(conflicts) > 0 {
@@ -107,6 +116,20 @@ func (v *IngressCustomValidator) ValidateUpdate(ctx context.Context, oldObj, new
 	return warnings, nil
 }
 
+// validateAnnotations rejects annotation combinations that would otherwise be
+// silently dropped, leaving the route without a requested security plugin.
+// enable-csrf with no csrf-key is refused here so kubectl apply fails loudly
+// instead of programming a route the operator believes is protected.
+func validateAnnotations(ingress *networkingv1.Ingress) error {
+	e := annotations.NewExtractor(ingress.Annotations)
+	if e.GetBoolAnnotation(annotations.AnnotationsEnableCsrf) &&
+		e.GetStringAnnotation(annotations.AnnotationsCsrfKey) == "" {
+		return fmt.Errorf("annotation %q is enabled but %q is missing or empty",
+			annotations.AnnotationsEnableCsrf, annotations.AnnotationsCsrfKey)
+	}
+	return nil
+}
+
 // ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type Ingress.
 func (v *IngressCustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
 	return nil, nil

internal/webhook/v1/ingress_webhook_test.go

@@ -113,3 +113,48 @@ func TestIngressCustomValidator_NoWarningsWhenReferencesExist(t *testing.T) {
 	require.NoError(t, err)
 	assert.Empty(t, warnings)
 }
+
+func csrfIngress(anno map[string]string) *networkingv1.Ingress {
+	return &networkingv1.Ingress{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-ingress", Namespace: "default", Annotations: anno},
+		Spec:       networkingv1.IngressSpec{},
+	}
+}
+
+func TestIngressCustomValidator_RejectsEnableCsrfWithoutKey(t *testing.T) {
+	validator := buildIngressValidator(t)
+
+	// missing csrf-key
+	_, err := validator.ValidateCreate(context.Background(), csrfIngress(map[string]string{
+		"k8s.apisix.apache.org/enable-csrf": "true",
+	}))
+	require.Error(t, err)
+
+	// empty csrf-key
+	_, err = validator.ValidateCreate(context.Background(), csrfIngress(map[string]string{
+		"k8s.apisix.apache.org/enable-csrf": "true",
+		"k8s.apisix.apache.org/csrf-key":    "",
+	}))
+	require.Error(t, err)
+
+	// the same invalid update must also be rejected, not only create
+	old := csrfIngress(nil)
+	_, err = validator.ValidateUpdate(context.Background(), old, csrfIngress(map[string]string{
+		"k8s.apisix.apache.org/enable-csrf": "true",
+	}))
+	require.Error(t, err)
+}
+
+func TestIngressCustomValidator_AllowsEnableCsrfWithKey(t *testing.T) {
+	validator := buildIngressValidator(t)
+
+	_, err := validator.ValidateCreate(context.Background(), csrfIngress(map[string]string{
+		"k8s.apisix.apache.org/enable-csrf": "true",
+		"k8s.apisix.apache.org/csrf-key":    "my-secret-key",
+	}))
+	require.NoError(t, err)
+
+	// csrf not enabled: key absence is fine
+	_, err = validator.ValidateCreate(context.Background(), csrfIngress(nil))
+	require.NoError(t, err)
+}