fix: add unknown status for secret store (#5070)

* check if validateStore status is unknown and update store status accordingly

Signed-off-by: Alvin Wonys <alvin.rw@proton.me>

* simplify validation result unknown check

Co-authored-by: Gustavo Fernandes de Carvalho <17139678+gusfcarvalho@users.noreply.github.com>
Signed-off-by: alvin-rw <83861143+alvin-rw@users.noreply.github.com>

* early return when validateStore result is unknown

Signed-off-by: Alvin Wonys <alvin.rw@proton.me>

* add test for validation result unknown checks

Signed-off-by: Alvin Wonys <alvin.rw@proton.me>

* update api specs docs

Signed-off-by: Alvin Wonys <alvin.rw@proton.me>

* change condition to invalid provider if failed validateStore

Co-authored-by: Gustavo Fernandes de Carvalho <17139678+gusfcarvalho@users.noreply.github.com>
Signed-off-by: alvin-rw <83861143+alvin-rw@users.noreply.github.com>

* change event to invalid provider config if failed validatedStore

Signed-off-by: Alvin Wonys <alvin.rw@proton.me>

* do not return error in case of validationUnknownError store validation

if store validation result is validationUnknownError, do not return error to prevent reconcile handler ignoring the returned Result

Co-authored-by: Gustavo Fernandes de Carvalho <17139678+gusfcarvalho@users.noreply.github.com>
Signed-off-by: alvin-rw <83861143+alvin-rw@users.noreply.github.com>

---------

Signed-off-by: Alvin Wonys <alvin.rw@proton.me>
Signed-off-by: alvin-rw <83861143+alvin-rw@users.noreply.github.com>
Co-authored-by: Gustavo Fernandes de Carvalho <17139678+gusfcarvalho@users.noreply.github.com>
Co-authored-by: Gergely Brautigam <skarlso777@gmail.com>

apis/externalsecrets/v1/secretstore_fake_types.go

@@ -16,7 +16,8 @@ package v1
 
 // FakeProvider configures a fake provider that returns static values.
 type FakeProvider struct {
-	Data []FakeProviderData `json:"data"`
+	Data             []FakeProviderData `json:"data"`
+	ValidationResult *ValidationResult  `json:"validationResult,omitempty"`
 }
 
 type FakeProviderData struct {

apis/externalsecrets/v1/secretstore_types.go

@@ -260,6 +260,7 @@ const (
 	ReasonInvalidStore          = "InvalidStoreConfiguration"
 	ReasonInvalidProviderConfig = "InvalidProviderConfig"
 	ReasonValidationFailed      = "ValidationFailed"
+	ReasonValidationUnknown     = "ValidationUnknown"
 	ReasonStoreValid            = "Valid"
 	StoreUnmaintained           = "StoreUnmaintained"
 )

apis/externalsecrets/v1/zz_generated.deepcopy.go

@@ -1676,6 +1676,11 @@ func (in *FakeProvider) DeepCopyInto(out *FakeProvider) {
 		*out = make([]FakeProviderData, len(*in))
 		copy(*out, *in)
 	}
+	if in.ValidationResult != nil {
+		in, out := &in.ValidationResult, &out.ValidationResult
+		*out = new(ValidationResult)
+		**out = **in
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FakeProvider.

config/crds/bases/external-secrets.io_clustersecretstores.yaml

@@ -1776,6 +1776,8 @@ spec:
                           - value
                           type: object
                         type: array
+                      validationResult:
+                        type: integer
                     required:
                     - data
                     type: object

config/crds/bases/external-secrets.io_secretstores.yaml

@@ -1776,6 +1776,8 @@ spec:
                           - value
                           type: object
                         type: array
+                      validationResult:
+                        type: integer
                     required:
                     - data
                     type: object

deploy/crds/bundle.yaml

@@ -3661,6 +3661,8 @@ spec:
                               - value
                             type: object
                           type: array
+                        validationResult:
+                          type: integer
                       required:
                         - data
                       type: object
@@ -14462,6 +14464,8 @@ spec:
                               - value
                             type: object
                           type: array
+                        validationResult:
+                          type: integer
                       required:
                         - data
                       type: object

docs/api/spec.md

@@ -4631,6 +4631,18 @@ map[string]string
 <td>
 </td>
 </tr>
+<tr>
+<td>
+<code>validationResult</code></br>
+<em>
+<a href="#external-secrets.io/v1.ValidationResult">
+ValidationResult
+</a>
+</em>
+</td>
+<td>
+</td>
+</tr>
 </tbody>
 </table>
 <h3 id="external-secrets.io/v1.FakeProviderData">FakeProviderData
@@ -9321,6 +9333,10 @@ External Secrets meta/v1.SecretKeySelector
 <h3 id="external-secrets.io/v1.ValidationResult">ValidationResult
 (<code>byte</code> alias)</p></h3>
 <p>
+(<em>Appears on:</em>
+<a href="#external-secrets.io/v1.FakeProvider">FakeProvider</a>)
+</p>
+<p>
 </p>
 <table>
 <thead>

pkg/controllers/secretstore/common.go

@@ -16,6 +16,7 @@ package secretstore
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -32,6 +33,7 @@ import (
 const (
 	errStoreClient         = "could not get provider client: %w"
 	errValidationFailed    = "could not validate provider: %w"
+	errValidationUnknown   = "could not determine validation status: %s"
 	errPatchStatus         = "unable to patch status: %w"
 	errUnableCreateClient  = "unable to create client"
 	errUnableValidateStore = "unable to validate store: %s"
@@ -40,6 +42,8 @@ const (
 	msgStoreNotMaintained = "store isn't currently maintained. Please plan and prepare accordingly."
 )
 
+var validationUnknownError = errors.New("could not determine validation status")
+
 type Opts struct {
 	ControllerClass string
 	GaugeVecGetter  metrics.GaugeVevGetter
@@ -74,6 +78,11 @@ func reconcile(ctx context.Context, req ctrl.Request, ss esapi.GenericStore, cl
 	err := validateStore(ctx, req.Namespace, opts.ControllerClass, ss, cl, opts.GaugeVecGetter, opts.Recorder)
 	if err != nil {
 		log.Error(err, "unable to validate store")
+		// in case of validation status unknown, validateStore will mark
+		// the store as ready but we should show ReasonValidationUnknown
+		if errors.Is(err, validationUnknownError) {
+			return ctrl.Result{RequeueAfter: requeueInterval}, nil
+		}
 		return ctrl.Result{}, err
 	}
 	storeProvider, err := esapi.GetProvider(ss)
@@ -123,16 +132,16 @@ func validateStore(ctx context.Context, namespace, controllerClass string, store
 		return fmt.Errorf(errStoreClient, err)
 	}
 	validationResult, err := cl.Validate()
-	if err != nil && validationResult != esapi.ValidationResultUnknown {
-		// Use ReasonInvalidProviderConfig for validation errors that indicate
-		// invalid configuration (like empty server URL)
-		reason := esapi.ReasonValidationFailed
-		if validationResult == esapi.ValidationResultError {
-			reason = esapi.ReasonInvalidProviderConfig
+	if err != nil {
+		if validationResult == esapi.ValidationResultUnknown {
+			cond := NewSecretStoreCondition(esapi.SecretStoreReady, v1.ConditionTrue, esapi.ReasonValidationUnknown, fmt.Sprintf(errValidationUnknown, err))
+			SetExternalSecretCondition(store, *cond, gaugeVecGetter)
+			recorder.Event(store, v1.EventTypeWarning, esapi.ReasonValidationUnknown, err.Error())
+			return validationUnknownError
 		}
-		cond := NewSecretStoreCondition(esapi.SecretStoreReady, v1.ConditionFalse, reason, fmt.Sprintf(errUnableValidateStore, err))
+		cond := NewSecretStoreCondition(esapi.SecretStoreReady, v1.ConditionFalse, esapi.ReasonInvalidProviderConfig, fmt.Sprintf(errUnableValidateStore, err))
 		SetExternalSecretCondition(store, *cond, gaugeVecGetter)
-		recorder.Event(store, v1.EventTypeWarning, reason, err.Error())
+		recorder.Event(store, v1.EventTypeWarning, esapi.ReasonInvalidProviderConfig, err.Error())
 		return fmt.Errorf(errValidationFailed, err)
 	}
 

pkg/controllers/secretstore/common_test.go

@@ -158,6 +158,43 @@ var _ = Describe("SecretStore reconcile", func() {
 
 	}
 
+	// an unknown store validation result should be reflected
+	// in the store status condition
+	validationUnknown := func(tc *testCase) {
+		spc := tc.store.GetSpec()
+		spc.Provider.Vault = nil
+		validationResultUnknown := esapi.ValidationResultUnknown
+		spc.Provider.Fake = &esapi.FakeProvider{
+			Data:             []esapi.FakeProviderData{},
+			ValidationResult: &validationResultUnknown,
+		}
+
+		tc.assert = func() {
+			Eventually(func() bool {
+				ss := tc.store.Copy()
+				err := k8sClient.Get(context.Background(), types.NamespacedName{
+					Name:      defaultStoreName,
+					Namespace: ss.GetNamespace(),
+				}, ss)
+				if err != nil {
+					return false
+				}
+
+				if len(ss.GetStatus().Conditions) != 1 {
+					return false
+				}
+
+				return ss.GetStatus().Conditions[0].Reason == esapi.ReasonValidationUnknown &&
+					ss.GetStatus().Conditions[0].Type == esapi.SecretStoreReady &&
+					ss.GetStatus().Conditions[0].Status == corev1.ConditionTrue &&
+					hasEvent(tc.store.GetTypeMeta().Kind, ss.GetName(), esapi.ReasonValidationUnknown)
+			}).
+				WithTimeout(time.Second * 5).
+				WithPolling(time.Second).
+				Should(BeTrue())
+		}
+	}
+
 	DescribeTable("Controller Reconcile logic", func(muts ...func(tc *testCase)) {
 		for _, mut := range muts {
 			mut(test)
@@ -171,12 +208,14 @@ var _ = Describe("SecretStore reconcile", func() {
 		Entry("[namespace] ignore stores with non-matching class", ignoreControllerClass),
 		Entry("[namespace] valid provider has status=ready", validProvider),
 		Entry("[namespace] valid provider has capabilities=ReadWrite", readWrite),
+		Entry("[namespace] validation unknown status should set ValidationUnknown condition", validationUnknown),
 
 		// cluster store
 		Entry("[cluster] invalid provider with secretStore should set InvalidStore condition", invalidProvider, useClusterStore),
 		Entry("[cluster] ignore stores with non-matching class", ignoreControllerClass, useClusterStore),
 		Entry("[cluster] valid provider has status=ready", validProvider, useClusterStore),
 		Entry("[cluster] valid provider has capabilities=ReadWrite", readWrite, useClusterStore),
+		Entry("[cluster] validation unknown status should set ValidationUnknown condition", validationUnknown, useClusterStore),
 	)
 
 })

pkg/provider/fake/fake.go

@@ -52,8 +52,9 @@ type Data struct {
 }
 type Config map[string]*Data
 type Provider struct {
-	config   Config
-	database map[string]Config
+	config           Config
+	database         map[string]Config
+	validationResult *esv1.ValidationResult
 }
 
 // Capabilities return the provider supported capabilities (ReadOnly, WriteOnly, ReadWrite).
@@ -90,7 +91,8 @@ func (p *Provider) NewClient(_ context.Context, store esv1.GenericStore, _ clien
 	}
 	p.database[store.GetName()] = cfg
 	return &Provider{
-		config: cfg,
+		config:           cfg,
+		validationResult: c.ValidationResult,
 	}, nil
 }
 
@@ -224,6 +226,9 @@ func (p *Provider) Close(_ context.Context) error {
 }
 
 func (p *Provider) Validate() (esv1.ValidationResult, error) {
+	if p.validationResult != nil && *p.validationResult != esv1.ValidationResultReady {
+		return *p.validationResult, errors.New("unable to validate")
+	}
 	return esv1.ValidationResultReady, nil
 }