fix(keepersecurity): return NoSecretErr for missing records (#6816)

* fix(keepersecurity): return NoSecretErr for missing records

findByIDWithNameFallback returned a plain errors.New for a record that does
not exist, so errors.Is(err, esv1.NoSecretErr) was false and the provider
never signalled a missing secret.

The reconciler keys deletionPolicy off that sentinel
(externalsecret_controller_secret.go), so for Keeper users deletionPolicy
Delete and Merge never fired. A missing record also surfaced as a hard
error, which sends the ExternalSecret into workqueue backoff instead of
honouring spec.refreshInterval.

Only the record == nil branch is tagged. The failures wrapped by
findSecretByID and GetSecretsByTitle stay generic so that an outage is not
reported as a deletion.

Signed-off-by: alliasgher <alliasgher123@gmail.com>

* fix(keepersecurity): drop the now-unused no-secrets-found constant

errKeeperSecurityNoSecretsFound lost its only client.go reference when the
not-found return started wrapping esv1.NoSecretErr. The one remaining use
was a test mock returning it for an API failure, so a constant named 'no
secrets found' stood in for the opposite of what it says.

Also document that Keeper cannot distinguish a deleted record from one that
is no longer shared with the KSM application, which matters under
deletionPolicy: Delete.

Signed-off-by: alliasgher <alliasgher123@gmail.com>

---------

Signed-off-by: alliasgher <alliasgher123@gmail.com>
Co-authored-by: Alexander Chernov <alexander@chernov.it>

docs/provider/keeper-security.md

@@ -58,6 +58,8 @@ Be sure the `keepersecurity` provider is listed in the `Kind=SecretStore`
 
 **NOTE:** For complex [types](https://docs.keeper.io/secrets-manager/secrets-manager/about/field-record-types), like name, phone, bankAccount, which does not match with a single string value, external secrets will return the complete json string. Use the json template functions to decode.
 
+**NOTE:** A record that cannot be found is reported as a missing secret, which is what `deletionPolicy: Delete` and `deletionPolicy: Merge` act on. Keeper Secrets Manager returns an empty record set both for a record that was deleted and for one that is simply no longer shared with the KSM application, and the two are indistinguishable to the provider. So with `deletionPolicy: Delete`, revoking the application's access to a record removes the key from the target Secret exactly as if the record had been deleted. Use the default `deletionPolicy: Retain` if that is not what you want.
+
 ### Creating external secret
 To create a kubernetes secret from Keeper Secret Manager secret a `Kind=ExternalSecret` is needed.
 

providers/v1/keepersecurity/client.go

@@ -37,7 +37,7 @@ const (
 	errKeeperSecuritySecretsNotFound            = "unable to find secrets. %w"
 	errKeeperSecuritySecretNotFound             = "unable to find secret %s. Error: %w"
 	errKeeperSecuritySecretNotUnique            = "more than 1 secret %s found"
-	errKeeperSecurityNoSecretsFound             = "no secrets found"
+	errKeeperSecurityRecordNotFound             = "%w: no record matched %s"
 	errKeeperSecurityInvalidSecretInvalidFormat = "invalid secret. Invalid format: %w"
 	errKeeperSecurityInvalidSecretDuplicatedKey = "invalid Secret. Following keys are duplicated %s"
 	errKeeperSecurityInvalidProperty            = "invalid Property. Secret %s does not have any key matching %s"
@@ -121,6 +121,8 @@ func (c *Client) Validate() (esv1.ValidationResult, error) {
 // GetSecret retrieves a secret from Keeper Security by ID or name.
 // It first attempts to find the secret by ID, then falls back to name lookup.
 // The name lookup must be opted in by setting getByTitleFallback on the provider.
+// A record that does not exist yields esv1.NoSecretErr, which is what the
+// reconciler keys deletionPolicy off.
 func (c *Client) GetSecret(_ context.Context, ref esv1.ExternalSecretDataRemoteRef) ([]byte, error) {
 	secret, err := c.findByIDWithNameFallback(ref.Key)
 	if err != nil {
@@ -161,7 +163,10 @@ func (c *Client) findByIDWithNameFallback(key string) (*Secret, error) {
 	}
 
 	if record == nil {
-		return nil, errors.New(errKeeperSecurityNoSecretsFound)
+		// Only a genuinely absent record gets the sentinel; the API failures
+		// wrapped by findSecretByID/GetSecretsByTitle above must stay generic so
+		// an outage is not mistaken for a deletion.
+		return nil, fmt.Errorf(errKeeperSecurityRecordNotFound, esv1.NoSecretErr, key)
 	}
 
 	secret, err := c.getValidKeeperSecret(record)

providers/v1/keepersecurity/client_test.go

@@ -298,6 +298,9 @@ func TestClientGetSecret(t *testing.T) {
 		args    args
 		want    []byte
 		wantErr bool
+		// wantNoSecretErr is asserted for every case, so a failure that is not a
+		// missing record must not carry the sentinel either.
+		wantNoSecretErr bool
 	}{
 		{
 			name: "Get Secret with a property (no label)",
@@ -497,7 +500,8 @@ func TestClientGetSecret(t *testing.T) {
 					Key: "record5",
 				},
 			},
-			wantErr: true,
+			wantErr:         true,
+			wantNoSecretErr: true,
 		},
 		{
 			name: "Get non existing secret with fallback",
@@ -519,7 +523,8 @@ func TestClientGetSecret(t *testing.T) {
 					Key: "record5",
 				},
 			},
-			wantErr: true,
+			wantErr:         true,
+			wantNoSecretErr: true,
 		},
 		{
 			name: "Get valid secret with non existing property",
@@ -601,6 +606,9 @@ func TestClientGetSecret(t *testing.T) {
 				t.Errorf("GetSecret() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
+			if isNoSecret := errors.Is(err, esv1.NoSecretErr); isNoSecret != tt.wantNoSecretErr {
+				t.Errorf("GetSecret() errors.Is(err, NoSecretErr) = %v, want %v (err = %v)", isNoSecret, tt.wantNoSecretErr, err)
+			}
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("GetSecret() got = %v, want %v", got, tt.want)
 			}
@@ -618,11 +626,12 @@ func TestClientGetSecretMap(t *testing.T) {
 		ref esv1.ExternalSecretDataRemoteRef
 	}
 	tests := []struct {
-		name    string
-		fields  fields
-		args    args
-		want    map[string][]byte
-		wantErr bool
+		name            string
+		fields          fields
+		args            args
+		want            map[string][]byte
+		wantErr         bool
+		wantNoSecretErr bool
 	}{
 		{
 			name: "Get Secret with valid property (no label)",
@@ -715,11 +724,13 @@ func TestClientGetSecretMap(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "Get non existing secret",
+			// The API call itself fails here, so this must not be reported as a
+			// missing record.
+			name: "Get secret when the API call fails",
 			fields: fields{
 				ksmClient: &fake.MockKeeperClient{
 					GetSecretsFn: func(filter []string) ([]*ksm.Record, error) {
-						return nil, errors.New(errKeeperSecurityNoSecretsFound)
+						return nil, errors.New("keeper API unavailable")
 					},
 				},
 				folderID: folderID,
@@ -732,6 +743,25 @@ func TestClientGetSecretMap(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "Get non existing secret",
+			fields: fields{
+				ksmClient: &fake.MockKeeperClient{
+					GetSecretsFn: func(filter []string) ([]*ksm.Record, error) {
+						return []*ksm.Record{}, nil
+					},
+				},
+				folderID: folderID,
+			},
+			args: args{
+				ctx: context.Background(),
+				ref: esv1.ExternalSecretDataRemoteRef{
+					Key: "record5",
+				},
+			},
+			wantErr:         true,
+			wantNoSecretErr: true,
+		},
 		{
 			name: "Get Secret with invalid property",
 			fields: fields{
@@ -763,6 +793,9 @@ func TestClientGetSecretMap(t *testing.T) {
 				t.Errorf("GetSecretMap() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
+			if isNoSecret := errors.Is(err, esv1.NoSecretErr); isNoSecret != tt.wantNoSecretErr {
+				t.Errorf("GetSecretMap() errors.Is(err, NoSecretErr) = %v, want %v (err = %v)", isNoSecret, tt.wantNoSecretErr, err)
+			}
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("GetSecretMap() got = %v, want %v", got, tt.want)
 			}