Validate data or dataFrom existence (#2867)

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

apis/externalsecrets/v1beta1/externalsecret_validator.go

@@ -15,6 +15,7 @@ package v1beta1
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/runtime"
@@ -41,21 +42,26 @@ func validateExternalSecret(obj runtime.Object) (admission.Warnings, error) {
 		return nil, fmt.Errorf("unexpected type")
 	}
 
+	var errs error
 	if (es.Spec.Target.DeletionPolicy == DeletionPolicyDelete && es.Spec.Target.CreationPolicy == CreatePolicyMerge) ||
 		(es.Spec.Target.DeletionPolicy == DeletionPolicyDelete && es.Spec.Target.CreationPolicy == CreatePolicyNone) {
-		return nil, fmt.Errorf("deletionPolicy=Delete must not be used when the controller doesn't own the secret. Please set creationPolcy=Owner")
+		errs = errors.Join(errs, fmt.Errorf("deletionPolicy=Delete must not be used when the controller doesn't own the secret. Please set creationPolicy=Owner"))
 	}
 
 	if es.Spec.Target.DeletionPolicy == DeletionPolicyMerge && es.Spec.Target.CreationPolicy == CreatePolicyNone {
-		return nil, fmt.Errorf("deletionPolicy=Merge must not be used with creationPolcy=None. There is no Secret to merge with")
+		errs = errors.Join(errs, fmt.Errorf("deletionPolicy=Merge must not be used with creationPolicy=None. There is no Secret to merge with"))
+	}
+
+	if len(es.Spec.Data) == 0 && len(es.Spec.DataFrom) == 0 {
+		errs = errors.Join(errs, fmt.Errorf("either data or dataFrom should be specified"))
 	}
 
 	for _, ref := range es.Spec.DataFrom {
 		findOrExtract := ref.Find != nil || ref.Extract != nil
 		if findOrExtract && ref.SourceRef != nil && ref.SourceRef.GeneratorRef != nil {
-			return nil, fmt.Errorf("generator can not be used with find or extract")
+			errs = errors.Join(errs, fmt.Errorf("generator can not be used with find or extract"))
 		}
 	}
 
-	return nil, nil
+	return nil, errs
 }

apis/externalsecrets/v1beta1/externalsecret_validator_test.go

@@ -21,14 +21,14 @@ import (
 
 func TestValidateExternalSecret(t *testing.T) {
 	tests := []struct {
-		name    string
-		obj     runtime.Object
-		wantErr bool
+		name        string
+		obj         runtime.Object
+		expectedErr string
 	}{
 		{
-			name:    "nil",
-			obj:     nil,
-			wantErr: true,
+			name:        "nil",
+			obj:         nil,
+			expectedErr: "unexpected type",
 		},
 		{
 			name: "deletion policy delete",
@@ -38,9 +38,12 @@ func TestValidateExternalSecret(t *testing.T) {
 						DeletionPolicy: DeletionPolicyDelete,
 						CreationPolicy: CreatePolicyMerge,
 					},
+					Data: []ExternalSecretData{
+						{},
+					},
 				},
 			},
-			wantErr: true,
+			expectedErr: "deletionPolicy=Delete must not be used when the controller doesn't own the secret. Please set creationPolicy=Owner",
 		},
 		{
 			name: "deletion policy merge",
@@ -50,9 +53,19 @@ func TestValidateExternalSecret(t *testing.T) {
 						DeletionPolicy: DeletionPolicyMerge,
 						CreationPolicy: CreatePolicyNone,
 					},
+					Data: []ExternalSecretData{
+						{},
+					},
 				},
 			},
-			wantErr: true,
+			expectedErr: "deletionPolicy=Merge must not be used with creationPolicy=None. There is no Secret to merge with",
+		},
+		{
+			name: "both data and data_from are empty",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{},
+			},
+			expectedErr: "either data or dataFrom should be specified",
 		},
 		{
 			name: "generator with find",
@@ -68,7 +81,7 @@ func TestValidateExternalSecret(t *testing.T) {
 					},
 				},
 			},
-			wantErr: true,
+			expectedErr: "generator can not be used with find or extract",
 		},
 		{
 			name: "generator with extract",
@@ -84,21 +97,47 @@ func TestValidateExternalSecret(t *testing.T) {
 					},
 				},
 			},
-			wantErr: true,
+			expectedErr: "generator can not be used with find or extract",
+		},
+		{
+			name: "multiple errors",
+			obj: &ExternalSecret{
+				Spec: ExternalSecretSpec{
+					Target: ExternalSecretTarget{
+						DeletionPolicy: DeletionPolicyMerge,
+						CreationPolicy: CreatePolicyNone,
+					},
+				},
+			},
+			expectedErr: `deletionPolicy=Merge must not be used with creationPolicy=None. There is no Secret to merge with
+either data or dataFrom should be specified`,
 		},
 		{
 			name: "valid",
 			obj: &ExternalSecret{
 				Spec: ExternalSecretSpec{
-					DataFrom: []ExternalSecretDataFromRemoteRef{},
+					DataFrom: []ExternalSecretDataFromRemoteRef{
+						{},
+					},
 				},
 			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if _, err := validateExternalSecret(tt.obj); (err != nil) != tt.wantErr {
-				t.Errorf("validateExternalSecret() error = %v, wantErr %v", err, tt.wantErr)
+			_, err := validateExternalSecret(tt.obj)
+			if err != nil {
+				if tt.expectedErr == "" {
+					t.Fatalf("validateExternalSecret() returned an unexpected error: %v", err)
+				}
+
+				if err.Error() != tt.expectedErr {
+					t.Fatalf("validateExternalSecret() returned an unexpected error: got: %v, expected: %v", err, tt.expectedErr)
+				}
+				return
+			}
+			if tt.expectedErr != "" {
+				t.Errorf("validateExternalSecret() should have returned an error but got nil")
 			}
 		})
 	}