clean: Standardize and friendly name for validations policies (#6630)

Signed-off-by: Jean-Philippe Evrard <jean-philippe.evrard+rochepub@external.roche.com>

providers/v1/delinea/provider.go

@@ -25,6 +25,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
+	esmeta "github.com/external-secrets/external-secrets/apis/meta/v1"
 	"github.com/external-secrets/external-secrets/runtime/esutils"
 	"github.com/external-secrets/external-secrets/runtime/esutils/resolvers"
 )
@@ -33,8 +34,6 @@ var (
 	errEmptyTenant                   = errors.New("tenant must not be empty")
 	errEmptyClientID                 = errors.New("clientID must be set")
 	errEmptyClientSecret             = errors.New("clientSecret must be set")
-	errSecretRefAndValueConflict     = errors.New("cannot specify both secret reference and value")
-	errSecretRefAndValueMissing      = errors.New("must specify either secret reference or direct value")
 	errMissingStore                  = errors.New("missing store specification")
 	errInvalidSpec                   = errors.New("invalid specification for delinea provider")
 	errMissingSecretName             = errors.New("must specify a secret name")
@@ -93,6 +92,7 @@ func (p *Provider) NewClient(ctx context.Context, store esv1.GenericStore, kube
 	}, nil
 }
 
+// loadConfigSecret resolves a Delinea credential from either an inline value or a secret reference.
 func loadConfigSecret(
 	ctx context.Context,
 	storeKind string,
@@ -108,31 +108,40 @@ func loadConfigSecret(
 	return resolvers.SecretKeyRef(ctx, kube, storeKind, namespace, ref.SecretRef)
 }
 
-func validateStoreSecretRef(store esv1.GenericStore, ref *esv1.DelineaProviderSecretRef) error {
-	if ref.SecretRef != nil {
-		if err := esutils.ValidateReferentSecretSelector(store, *ref.SecretRef); err != nil {
+// delineaCredentialRefPolicy returns the validation policy for Delinea credential fields.
+func delineaCredentialRefPolicy(store esv1.GenericStore) esutils.ValueOrRefPolicy[esmeta.SecretKeySelector] {
+	return esutils.ValueOrRefPolicy[esmeta.SecretKeySelector]{
+		Presence:    esutils.RequireValueOrRef,
+		ValidateRef: validateDelineaCredentialSecretRef(store),
+	}
+}
+
+// validateDelineaCredentialSecretRef validates a Delinea credential secret reference against the store scope.
+func validateDelineaCredentialSecretRef(store esv1.GenericStore) func(esmeta.SecretKeySelector) error {
+	return func(ref esmeta.SecretKeySelector) error {
+		if err := esutils.ValidateReferentSecretSelector(store, ref); err != nil {
 			return err
 		}
+		return validateDelineaCredentialSecretRefNameAndKey(ref)
 	}
-
-	return validateSecretRef(ref)
 }
 
+// validateSecretRef validates a Delinea credential reference independently of a store.
 func validateSecretRef(ref *esv1.DelineaProviderSecretRef) error {
-	if ref.SecretRef != nil {
-		if ref.Value != "" {
-			return errSecretRefAndValueConflict
-		}
-		if ref.SecretRef.Name == "" {
-			return errMissingSecretName
-		}
-		if ref.SecretRef.Key == "" {
-			return errMissingSecretKey
-		}
-	} else if ref.Value == "" {
-		return errSecretRefAndValueMissing
-	}
+	return esutils.ValidateValueOrRef(ref.Value, ref.SecretRef, esutils.ValueOrRefPolicy[esmeta.SecretKeySelector]{
+		Presence:    esutils.RequireValueOrRef,
+		ValidateRef: validateDelineaCredentialSecretRefNameAndKey,
+	})
+}
 
+// validateDelineaCredentialSecretRefNameAndKey ensures a Delinea credential secret reference has both name and key.
+func validateDelineaCredentialSecretRefNameAndKey(ref esmeta.SecretKeySelector) error {
+	if ref.Name == "" {
+		return errMissingSecretName
+	}
+	if ref.Key == "" {
+		return errMissingSecretKey
+	}
 	return nil
 }
 
@@ -171,13 +180,11 @@ func getConfig(store esv1.GenericStore) (*esv1.DelineaProvider, error) {
 		return nil, errEmptyClientSecret
 	}
 
-	err := validateStoreSecretRef(store, cfg.ClientID)
-	if err != nil {
+	if err := esutils.ValidateValueOrRef(cfg.ClientID.Value, cfg.ClientID.SecretRef, delineaCredentialRefPolicy(store)); err != nil {
 		return nil, err
 	}
 
-	err = validateStoreSecretRef(store, cfg.ClientSecret)
-	if err != nil {
+	if err := esutils.ValidateValueOrRef(cfg.ClientSecret.Value, cfg.ClientSecret.SecretRef, delineaCredentialRefPolicy(store)); err != nil {
 		return nil, err
 	}
 

providers/v1/delinea/provider_test.go

@@ -30,6 +30,7 @@ import (
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 	v1 "github.com/external-secrets/external-secrets/apis/meta/v1"
+	"github.com/external-secrets/external-secrets/runtime/esutils"
 )
 
 func TestDoesConfigDependOnNamespace(t *testing.T) {
@@ -110,7 +111,7 @@ func TestValidateStore(t *testing.T) {
 				ClientID:     ambiguousSecretRef,
 				ClientSecret: validSecretRefUsingValue,
 			},
-			want: errSecretRefAndValueConflict,
+			want: esutils.ErrValueAndRefConflict,
 		},
 		"invalid with ambiguous clientSecret": {
 			cfg: esv1.DelineaProvider{
@@ -118,7 +119,7 @@ func TestValidateStore(t *testing.T) {
 				ClientID:     validSecretRefUsingValue,
 				ClientSecret: ambiguousSecretRef,
 			},
-			want: errSecretRefAndValueConflict,
+			want: esutils.ErrValueAndRefConflict,
 		},
 		"invalid with invalid clientID": {
 			cfg: esv1.DelineaProvider{
@@ -126,7 +127,7 @@ func TestValidateStore(t *testing.T) {
 				ClientID:     makeSecretRefUsingValue(""),
 				ClientSecret: validSecretRefUsingValue,
 			},
-			want: errSecretRefAndValueMissing,
+			want: esutils.ErrValueOrRefMissing,
 		},
 		"invalid with invalid clientSecret": {
 			cfg: esv1.DelineaProvider{
@@ -134,7 +135,7 @@ func TestValidateStore(t *testing.T) {
 				ClientID:     validSecretRefUsingValue,
 				ClientSecret: makeSecretRefUsingValue(""),
 			},
-			want: errSecretRefAndValueMissing,
+			want: esutils.ErrValueOrRefMissing,
 		},
 		"valid with tenant/clientID/clientSecret": {
 			cfg: esv1.DelineaProvider{

providers/v1/scaleway/provider.go

@@ -29,6 +29,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
+	esmeta "github.com/external-secrets/external-secrets/apis/meta/v1"
 	"github.com/external-secrets/external-secrets/runtime/esutils"
 	"github.com/external-secrets/external-secrets/runtime/esutils/resolvers"
 )
@@ -102,19 +103,17 @@ func loadConfigSecret(ctx context.Context, ref *esv1.ScalewayProviderSecretRef,
 }
 
 func validateSecretRef(store esv1.GenericStore, ref *esv1.ScalewayProviderSecretRef) error {
-	if ref.SecretRef != nil {
-		if ref.Value != "" {
-			return errors.New("cannot specify both secret reference and value")
-		}
-		err := esutils.ValidateReferentSecretSelector(store, *ref.SecretRef)
-		if err != nil {
-			return err
-		}
-	} else if ref.Value == "" {
-		return errors.New("must specify either secret reference or direct value")
-	}
-
-	return nil
+	return esutils.ValidateValueOrRef(ref.Value, ref.SecretRef, scalewaySecretRefPolicy(store))
+}
+
+// scalewaySecretRefPolicy returns the validation policy for Scaleway secret references.
+func scalewaySecretRefPolicy(store esv1.GenericStore) esutils.ValueOrRefPolicy[esmeta.SecretKeySelector] {
+	return esutils.ValueOrRefPolicy[esmeta.SecretKeySelector]{
+		Presence: esutils.RequireValueOrRef,
+		ValidateRef: func(ref esmeta.SecretKeySelector) error {
+			return esutils.ValidateReferentSecretSelector(store, ref)
+		},
+	}
 }
 
 func doesConfigDependOnNamespace(cfg *esv1.ScalewayProvider) bool {

providers/v1/secretserver/provider.go

@@ -27,6 +27,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
+	esmeta "github.com/external-secrets/external-secrets/apis/meta/v1"
 	"github.com/external-secrets/external-secrets/runtime/esutils"
 	"github.com/external-secrets/external-secrets/runtime/esutils/resolvers"
 )
@@ -35,14 +36,11 @@ var (
 	errEmptyUserName                 = errors.New("username must not be empty")
 	errEmptyPassword                 = errors.New("password must be set")
 	errEmptyServerURL                = errors.New("serverURL must be set")
-	errSecretRefAndValueConflict     = errors.New("cannot specify both secret reference and value")
-	errSecretRefAndValueMissing      = errors.New("must specify either secret reference or direct value")
 	errMissingStore                  = errors.New("missing store specification")
 	errInvalidSpec                   = errors.New("invalid specification for secret server provider")
 	errClusterStoreRequiresNamespace = errors.New("when using a ClusterSecretStore, namespaces must be explicitly set")
 	errMissingSecretName             = errors.New("must specify a secret name")
-
-	errMissingSecretKey = errors.New("must specify a secret key")
+	errMissingSecretKey              = errors.New("must specify a secret key")
 )
 
 // Provider struct that implements the ESO esv1.Provider.
@@ -148,30 +146,33 @@ func loadCredentials(ctx context.Context, store esv1.GenericStore, cfg *esv1.Sec
 	}, nil
 }
 
-func validateStoreSecretRef(store esv1.GenericStore, ref *esv1.SecretServerProviderRef) error {
-	if ref.SecretRef != nil {
-		if err := esutils.ValidateReferentSecretSelector(store, *ref.SecretRef); err != nil {
-			return err
-		}
+// secretServerCredentialRefPolicy returns the validation policy for Secret Server credential fields.
+func secretServerCredentialRefPolicy(store esv1.GenericStore) esutils.ValueOrRefPolicy[esmeta.SecretKeySelector] {
+	return esutils.ValueOrRefPolicy[esmeta.SecretKeySelector]{
+		Presence:    esutils.RequireValueOrRef,
+		ValidateRef: validateSecretServerCredentialSecretRef(store),
 	}
-	return validateSecretRef(ref)
 }
 
-func validateSecretRef(ref *esv1.SecretServerProviderRef) error {
-	if ref.SecretRef != nil {
-		if ref.Value != "" {
-			return errSecretRefAndValueConflict
+// validateSecretServerCredentialSecretRef validates a Secret Server credential secret reference against the store scope.
+func validateSecretServerCredentialSecretRef(store esv1.GenericStore) func(esmeta.SecretKeySelector) error {
+	return func(ref esmeta.SecretKeySelector) error {
+		if err := esutils.ValidateReferentSecretSelector(store, ref); err != nil {
+			return err
 		}
-		if ref.SecretRef.Name == "" {
+		if ref.Name == "" {
 			return errMissingSecretName
 		}
-		if ref.SecretRef.Key == "" {
+		if ref.Key == "" {
 			return errMissingSecretKey
 		}
-	} else if ref.Value == "" {
-		return errSecretRefAndValueMissing
+		return nil
 	}
-	return nil
+}
+
+// validateStoreSecretRef validates a Secret Server credential reference against the store scope.
+func validateStoreSecretRef(store esv1.GenericStore, ref *esv1.SecretServerProviderRef) error {
+	return esutils.ValidateValueOrRef(ref.Value, ref.SecretRef, secretServerCredentialRefPolicy(store))
 }
 
 func doesConfigDependOnNamespace(cfg *esv1.SecretServerProvider) bool {
@@ -219,12 +220,10 @@ func getConfig(store esv1.GenericStore) (*esv1.SecretServerProvider, error) {
 		return nil, errEmptyPassword
 	}
 
-	err := validateStoreSecretRef(store, cfg.Username)
-	if err != nil {
+	if err := validateStoreSecretRef(store, cfg.Username); err != nil {
 		return nil, err
 	}
-	err = validateStoreSecretRef(store, cfg.Password)
-	if err != nil {
+	if err := validateStoreSecretRef(store, cfg.Password); err != nil {
 		return nil, err
 	}
 	return cfg, nil

providers/v1/secretserver/provider_test.go

@@ -31,6 +31,7 @@ import (
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 	v1 "github.com/external-secrets/external-secrets/apis/meta/v1"
+	"github.com/external-secrets/external-secrets/runtime/esutils"
 )
 
 func TestDoesConfigDependOnNamespace(t *testing.T) {
@@ -151,7 +152,7 @@ func TestValidateStore(t *testing.T) {
 				Password:  validSecretRefUsingValue,
 				ServerURL: testURL,
 			},
-			want: errSecretRefAndValueConflict,
+			want: esutils.ErrValueAndRefConflict,
 		},
 		"invalid with ambiguous Password": {
 			cfg: esv1.SecretServerProvider{
@@ -159,7 +160,7 @@ func TestValidateStore(t *testing.T) {
 				Password:  ambiguousSecretRef,
 				ServerURL: testURL,
 			},
-			want: errSecretRefAndValueConflict,
+			want: esutils.ErrValueAndRefConflict,
 		},
 		"invalid with invalid Username": {
 			cfg: esv1.SecretServerProvider{
@@ -167,7 +168,7 @@ func TestValidateStore(t *testing.T) {
 				Password:  validSecretRefUsingValue,
 				ServerURL: testURL,
 			},
-			want: errSecretRefAndValueMissing,
+			want: esutils.ErrValueOrRefMissing,
 		},
 		"invalid with invalid Password": {
 			cfg: esv1.SecretServerProvider{
@@ -175,7 +176,7 @@ func TestValidateStore(t *testing.T) {
 				Password:  makeSecretRefUsingValue(""),
 				ServerURL: testURL,
 			},
-			want: errSecretRefAndValueMissing,
+			want: esutils.ErrValueOrRefMissing,
 		},
 		"valid with tenant/clientID/clientSecret": {
 			cfg: esv1.SecretServerProvider{
@@ -204,14 +205,14 @@ func TestValidateStore(t *testing.T) {
 				Token:     ambiguousSecretRef,
 				ServerURL: testURL,
 			},
-			want: errSecretRefAndValueConflict,
+			want: esutils.ErrValueAndRefConflict,
 		},
 		"invalid with invalid token": {
 			cfg: esv1.SecretServerProvider{
 				Token:     makeSecretRefUsingValue(""),
 				ServerURL: testURL,
 			},
-			want: errSecretRefAndValueMissing,
+			want: esutils.ErrValueOrRefMissing,
 		},
 	}
 	for name, tc := range tests {
@@ -714,7 +715,7 @@ func TestValidateStoreSecretRef(t *testing.T) {
 				},
 				Value: "some-value",
 			},
-			wantErr: errSecretRefAndValueConflict,
+			wantErr: esutils.ErrValueAndRefConflict,
 		},
 	}
 

runtime/esutils/validators.go

@@ -0,0 +1,95 @@
+/*
+Copyright © The ESO Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package esutils
+
+import (
+	"errors"
+	"fmt"
+)
+
+var (
+	// ErrValueAndRefConflict is returned when both a value and reference are set.
+	ErrValueAndRefConflict = errors.New("cannot specify both value and reference")
+	// ErrValueOrRefMissing is returned when neither a value nor reference is set.
+	ErrValueOrRefMissing = errors.New("must specify either value or reference")
+	// ErrRefRequired is returned when a reference is required but missing.
+	ErrRefRequired = errors.New("reference is required")
+	// ErrValueNotAllowed is returned when a value is set but only a reference is allowed.
+	ErrValueNotAllowed = errors.New("value must not be specified")
+	// ErrValueRequired is returned when a value is required but missing.
+	ErrValueRequired = errors.New("value is required")
+	// ErrRefNotAllowed is returned when a reference is set but only a value is allowed.
+	ErrRefNotAllowed = errors.New("reference must not be specified")
+)
+
+// RefPresencePolicy describes which side of a value/reference pair is allowed.
+type RefPresencePolicy int
+
+const (
+	// RequireValueOrRef requires exactly one of value or reference to be set.
+	RequireValueOrRef RefPresencePolicy = iota
+	// AllowValueOrRef allows neither value nor reference to be set, but rejects both being set.
+	AllowValueOrRef
+	// RequireRefOnly requires reference to be set and value to be empty.
+	RequireRefOnly
+	// RequireValueOnly requires value to be set and reference to be empty.
+	RequireValueOnly
+)
+
+// ValueOrRefPolicy configures ValidateValueOrRef.
+type ValueOrRefPolicy[T any] struct {
+	Presence    RefPresencePolicy
+	ValidateRef func(T) error
+}
+
+// ValidateValueOrRef validates fields that allow a direct value or a reference.
+func ValidateValueOrRef[T any](value string, ref *T, policy ValueOrRefPolicy[T]) error {
+	switch policy.Presence {
+	case RequireValueOrRef:
+		if value != "" && ref != nil {
+			return ErrValueAndRefConflict
+		}
+		if value == "" && ref == nil {
+			return ErrValueOrRefMissing
+		}
+	case AllowValueOrRef:
+		if value != "" && ref != nil {
+			return ErrValueAndRefConflict
+		}
+	case RequireRefOnly:
+		if ref == nil {
+			return ErrRefRequired
+		}
+		if value != "" {
+			return ErrValueNotAllowed
+		}
+	case RequireValueOnly:
+		if value == "" {
+			return ErrValueRequired
+		}
+		if ref != nil {
+			return ErrRefNotAllowed
+		}
+	default:
+		return fmt.Errorf("unknown value/reference presence policy: %d", policy.Presence)
+	}
+
+	if ref != nil && policy.ValidateRef != nil {
+		return policy.ValidateRef(*ref)
+	}
+	return nil
+}

runtime/esutils/validators_test.go

@@ -0,0 +1,176 @@
+/*
+Copyright © The ESO Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package esutils
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestValidateValueOrRef(t *testing.T) {
+	errRef := errors.New("ref")
+	ref := "ref"
+
+	tests := []struct {
+		name    string
+		value   string
+		ref     *string
+		policy  ValueOrRefPolicy[string]
+		wantErr error
+	}{
+		{
+			name:  "requires exactly one: value",
+			value: "value",
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireValueOrRef,
+			},
+		},
+		{
+			name: "requires exactly one: ref",
+			ref:  &ref,
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireValueOrRef,
+			},
+		},
+		{
+			name:  "requires exactly one: rejects both",
+			value: "value",
+			ref:   &ref,
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireValueOrRef,
+			},
+			wantErr: ErrValueAndRefConflict,
+		},
+		{
+			name: "requires exactly one: rejects missing both",
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireValueOrRef,
+			},
+			wantErr: ErrValueOrRefMissing,
+		},
+		{
+			name: "validates ref when present",
+			ref:  &ref,
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireValueOrRef,
+				ValidateRef: func(string) error {
+					return errRef
+				},
+			},
+			wantErr: errRef,
+		},
+		{
+			name:  "allows optional empty pair",
+			value: "",
+			ref:   nil,
+			policy: ValueOrRefPolicy[string]{
+				Presence: AllowValueOrRef,
+			},
+		},
+		{
+			name:  "allows optional value",
+			value: "value",
+			policy: ValueOrRefPolicy[string]{
+				Presence: AllowValueOrRef,
+			},
+		},
+		{
+			name: "allows optional ref",
+			ref:  &ref,
+			policy: ValueOrRefPolicy[string]{
+				Presence: AllowValueOrRef,
+			},
+		},
+		{
+			name:  "allows optional pair: rejects both",
+			value: "value",
+			ref:   &ref,
+			policy: ValueOrRefPolicy[string]{
+				Presence: AllowValueOrRef,
+			},
+			wantErr: ErrValueAndRefConflict,
+		},
+		{
+			name: "requires ref only",
+			ref:  &ref,
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireRefOnly,
+			},
+		},
+		{
+			name: "requires ref only: rejects missing ref",
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireRefOnly,
+			},
+			wantErr: ErrRefRequired,
+		},
+		{
+			name:  "requires ref only: rejects value",
+			value: "value",
+			ref:   &ref,
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireRefOnly,
+			},
+			wantErr: ErrValueNotAllowed,
+		},
+		{
+			name:  "requires value only",
+			value: "value",
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireValueOnly,
+			},
+		},
+		{
+			name: "requires value only: rejects missing value",
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireValueOnly,
+			},
+			wantErr: ErrValueRequired,
+		},
+		{
+			name:  "requires value only: rejects ref",
+			value: "value",
+			ref:   &ref,
+			policy: ValueOrRefPolicy[string]{
+				Presence: RequireValueOnly,
+			},
+			wantErr: ErrRefNotAllowed,
+		},
+		{
+			name: "rejects unknown policy",
+			policy: ValueOrRefPolicy[string]{
+				Presence: RefPresencePolicy(99),
+			},
+			wantErr: errors.New("unknown value/reference presence policy: 99"),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ValidateValueOrRef(tt.value, tt.ref, tt.policy)
+			if tt.wantErr == nil {
+				if got != nil {
+					t.Errorf("ValidateValueOrRef() got = %v, want no error", got)
+				}
+				return
+			}
+			if !errors.Is(got, tt.wantErr) && (got == nil || got.Error() != tt.wantErr.Error()) {
+				t.Errorf("ValidateValueOrRef() got = %v, want = %v", got, tt.wantErr)
+			}
+		})
+	}
+}