fix: apply coderabbit suggestions

- clarify documentation

- add error handling when errshould is empty in tests

- add protection to OkmsClient mocks

- add error persitence in fake resolver

Signed-off-by: Jordan Sauvain <jordan.sauvain@ovhcloud.com>

docs/provider/ovhcloud.md

@@ -74,7 +74,7 @@ data:
 ```
 
 !!! note
-     A `ClusterSecretStore` configuration is the same except you have to provide the `tokenSecretRef` `namespace`.  
+     A `ClusterSecretStore` configuration is the same except you must provide the `namespace` for `tokenSecretRef`, `certSecretRef` and `keySecretRef` according to your chosen authentication method.  
 
 ### <u>ExternalSecret</u>
  
@@ -206,7 +206,7 @@ spec:
   target:
     name: secret-example
   data:
-    - secretKey: foo
+    - secretKey: type
       remoteRef:
         key: creds
         property: type
@@ -214,7 +214,7 @@ spec:
 Resulting Kubernetes Secret data:
 ```json
 {
-  "foo": "credential"
+  "type": "credential"
 }
 ```
 - Nested value using `data`
@@ -231,7 +231,7 @@ spec:
   target:
     name: secret-example
   data:
-    - secretKey: foo
+    - secretKey: kevin-token
       remoteRef:
         key: creds
         property: users.kevin.token

providers/v1/ovh/client_get_all_secrets.go

@@ -203,12 +203,12 @@ func filterSecretsListWithRegexp(ctx context.Context, cl *ovhClient, secrets []s
 		// Insert the secret if no regex is provided;
 		// otherwise, insert only matching secrets.
 		secretData, ok, err := fetchSecretData(ctx, cl, secret, regex, ref)
-		if ok {
-			secretsDataMap[secret] = secretData
-		}
 		if err != nil {
 			return map[string][]byte{}, err
 		}
+		if ok {
+			secretsDataMap[secret] = secretData
+		}
 	}
 	if len(secretsDataMap) == 0 {
 		return map[string][]byte{}, buildNoMatchError(ref.Name, ref.Path)

providers/v1/ovh/client_get_all_secrets_test.go

@@ -222,6 +222,9 @@ func TestGetAllSecrets(t *testing.T) {
 					t.Errorf("\nexpected value: %s\nactual value:   %v\n\n", testCase.errshould, err)
 				}
 				return
+			} else if err != nil {
+				t.Errorf("\nunexpected error: %v\n\n", err)
+				return
 			}
 			if !reflect.DeepEqual(testCase.should, secrets) {
 				t.Errorf("\nexpected value: %v\nactual value:   %v\n\n", convertByteMapToStringMap(testCase.should), convertByteMapToStringMap(secrets))

providers/v1/ovh/client_get_secret_map_test.go

@@ -143,6 +143,9 @@ func TestGetSecretMap(t *testing.T) {
 					t.Errorf("\nexpected value: %s\nactual value:   %v\n\n", testCase.errshould, err)
 				}
 				return
+			} else if err != nil {
+				t.Errorf("\nunexpected error: %v\n\n", err)
+				return
 			}
 			if !reflect.DeepEqual(testCase.should, secret) {
 				t.Errorf("\nexpected value: %v\nactual value:   %v\n\n", convertByteMapToStringMap(testCase.should), convertByteMapToStringMap(secret))

providers/v1/ovh/client_get_secret_test.go

@@ -124,6 +124,9 @@ func TestGetSecret(t *testing.T) {
 					t.Errorf("\nexpected error: %s\nactual error:   %v\n\n", testCase.errshould, err)
 				}
 				return
+			} else if err != nil {
+				t.Errorf("\nunexpected error: %v\n\n", err)
+				return
 			}
 			if testCase.should != "" && string(secret) != testCase.should {
 				t.Errorf("\nexpected value: %q\nactual value:   %q\n\n", testCase.should, string(secret))

providers/v1/ovh/client_push_secret_test.go

@@ -182,6 +182,8 @@ func TestPushSecret(t *testing.T) {
 				} else if err.Error() != testCase.errshould {
 					t.Errorf("\nexpected error: %s\nactual error:   %v\n\n", testCase.errshould, err)
 				}
+			} else if err != nil {
+				t.Errorf("\nunexpected error: %v\n\n", err)
 			}
 		})
 	}

providers/v1/ovh/client_secret_exists_test.go

@@ -74,6 +74,9 @@ func TestSecretExists(t *testing.T) {
 					t.Errorf("\nexpected error: %s\nactual error:   %v\n\n", testCase.errshould, err)
 				}
 				return
+			} else if err != nil {
+				t.Errorf("\nunexpected error: %v\n\n", err)
+				return
 			}
 			if exists != testCase.should {
 				t.Errorf("\nexpected value: %t\nactual value:   %t\n\n", testCase.should, exists)

providers/v1/ovh/fake/fake_okms_client.go

@@ -131,7 +131,10 @@ func NewGetSecretV2Fn(path string, err error) GetSecretV2Fn {
 }
 
 func (f FakeOkmsClient) ListSecretV2(ctx context.Context, okmsID uuid.UUID, pageSize *uint32, pageCursor *string) (*types.ListSecretV2ResponseWithPagination, error) {
-	return f.ListSecretV2Fn()
+	if f.ListSecretV2Fn != nil {
+		return f.ListSecretV2Fn()
+	}
+	return NewListSecretV2Fn(nil)()
 }
 func NewListSecretV2Fn(err error) ListSecretV2Fn {
 	return func() (*types.ListSecretV2ResponseWithPagination, error) {
@@ -152,7 +155,10 @@ func NewListSecretV2Fn(err error) ListSecretV2Fn {
 }
 
 func (f FakeOkmsClient) PostSecretV2(ctx context.Context, okmsID uuid.UUID, body types.PostSecretV2Request) (*types.PostSecretV2Response, error) {
-	return f.PostSecretV2Fn()
+	if f.PostSecretV2Fn != nil {
+		return f.PostSecretV2Fn()
+	}
+	return NewPostSecretV2Fn(nil)()
 }
 func NewPostSecretV2Fn(err error) PostSecretV2Fn {
 	return func() (*types.PostSecretV2Response, error) {
@@ -161,7 +167,10 @@ func NewPostSecretV2Fn(err error) PostSecretV2Fn {
 }
 
 func (f FakeOkmsClient) PutSecretV2(ctx context.Context, okmsID uuid.UUID, path string, cas *uint32, body types.PutSecretV2Request) (*types.PutSecretV2Response, error) {
-	return f.PutSecretV2Fn()
+	if f.PutSecretV2Fn != nil {
+		return f.PutSecretV2Fn()
+	}
+	return NewPutSecretV2Fn(nil)()
 }
 func NewPutSecretV2Fn(err error) PutSecretV2Fn {
 	return func() (*types.PutSecretV2Response, error) {
@@ -170,7 +179,10 @@ func NewPutSecretV2Fn(err error) PutSecretV2Fn {
 }
 
 func (f FakeOkmsClient) DeleteSecretV2(ctx context.Context, okmsID uuid.UUID, path string) error {
-	return f.DeleteSecretV2Fn()
+	if f.DeleteSecretV2Fn != nil {
+		return f.DeleteSecretV2Fn()
+	}
+	return NewDeleteSecretV2Fn(nil)()
 }
 func NewDeleteSecretV2Fn(err error) DeleteSecretV2Fn {
 	return func() error {
@@ -186,7 +198,10 @@ func NewDeleteSecretV2Fn(err error) DeleteSecretV2Fn {
 //
 // This implementation returns a list of secrets from fakeSecretStorage variable.
 func (f FakeOkmsClient) GetSecretsMetadata(ctx context.Context, okmsID uuid.UUID, path string, list bool) (*types.GetMetadataResponse, error) {
-	return f.GetSecretsMetadataFn()
+	if f.GetSecretsMetadataFn != nil {
+		return f.GetSecretsMetadataFn()
+	}
+	return NewGetSecretsMetadataFn(path, nil)()
 }
 func NewGetSecretsMetadataFn(path string, err error) GetSecretsMetadataFn {
 	return func() (*types.GetMetadataResponse, error) {

providers/v1/ovh/fake/fake_resolver.go

@@ -32,6 +32,7 @@ type FakeResolver struct {
 	Once    sync.Once
 	keyPEM  string
 	certPEM string
+	err     error
 }
 
 type FakeSecretKeyResolver struct {
@@ -43,11 +44,10 @@ func (fr *FakeSecretKeyResolver) Resolve(_ context.Context, _ kclient.Client, _,
 		return "Valid", nil
 	}
 	if ref.Name == "Valid mtls client certificate" || ref.Name == "Valid mtls client key" {
-		var err error
 		fr.fakeResolver.Once.Do(func() {
 			var privKey *rsa.PrivateKey
-			privKey, err = rsa.GenerateKey(rand.Reader, 2048)
-			if err != nil {
+			privKey, fr.fakeResolver.err = rsa.GenerateKey(rand.Reader, 2048)
+			if fr.fakeResolver.err != nil {
 				return
 			}
 			fr.fakeResolver.keyPEM = string(pem.EncodeToMemory(&pem.Block{
@@ -57,8 +57,8 @@ func (fr *FakeSecretKeyResolver) Resolve(_ context.Context, _ kclient.Client, _,
 
 			template := x509.Certificate{}
 			var cert []byte
-			cert, err = x509.CreateCertificate(rand.Reader, &template, &template, &privKey.PublicKey, privKey)
-			if err != nil {
+			cert, fr.fakeResolver.err = x509.CreateCertificate(rand.Reader, &template, &template, &privKey.PublicKey, privKey)
+			if fr.fakeResolver.err != nil {
 				return
 			}
 			fr.fakeResolver.certPEM = string(pem.EncodeToMemory(&pem.Block{
@@ -67,8 +67,8 @@ func (fr *FakeSecretKeyResolver) Resolve(_ context.Context, _ kclient.Client, _,
 			}))
 		})
 
-		if err != nil {
-			return "", err
+		if fr.fakeResolver.err != nil {
+			return "", fr.fakeResolver.err
 		}
 
 		if ref.Name == "Valid mtls client certificate" {

providers/v1/ovh/provider_test.go

@@ -285,6 +285,8 @@ func TestNewClient(t *testing.T) {
 				} else if err.Error() != testCase.errshould {
 					t.Errorf("\nexpected error: %s\nactual error:   %v\n\n", testCase.errshould, err)
 				}
+			} else if err != nil {
+				t.Errorf("\nunexpected error: %v\n\n", err)
 			}
 		})
 	}
@@ -474,6 +476,8 @@ func TestValidateStore(t *testing.T) {
 				} else if err.Error() != testCase.errshould {
 					t.Errorf("\nexpected error: %s\nactual error:   %v\n\n", testCase.errshould, err)
 				}
+			} else if err != nil {
+				t.Errorf("\nunexpected error: %v\n\n", err)
 			}
 		})
 	}