handle special case for imported cert secret type (#2629)

Signed-off-by: shanti.gundumalla@ibm.com <shanti.gundumalla@ibm.com>
Co-authored-by: shanti.gundumalla@ibm.com <shanti.gundumalla@ibm.com>

docs/provider/ibm-secrets-manager.md

@@ -197,7 +197,7 @@ Below example creates a kubernetes secret based on ID of the secret in Secrets M
 {% include 'ibm-external-secret.yaml' %}
 ```
 
-Alternatively, secret name can be specified instead of secret ID.
+Alternatively, secret name can be specified instead of secret ID. However, note that ESO makes an additional call to fetch the relevant secret ID for the specified secret name.
 
 ```yaml
 {% include 'ibm-external-secret-by-name.yaml' %}

pkg/provider/ibm/provider.go

@@ -227,8 +227,14 @@ func getImportCertSecret(ibm *providerIBM, secretName *string, ref esv1beta1.Ext
 	if err != nil {
 		return nil, err
 	}
-	if val, ok := secMap[ref.Property]; ok {
+	val, ok := secMap[ref.Property]
+	if ok {
 		return []byte(val.(string)), nil
+	} else if ref.Property == privateKeyConst {
+		// we want to return an empty string in case the secret doesn't contain a private key
+		// this is to ensure that secret of type 'kubernetes.io/tls' gets created as expected, even with an empty private key
+		fmt.Printf("warn: %s is empty for secret %s\n", privateKeyConst, *secretName)
+		return []byte(""), nil
 	}
 	return nil, fmt.Errorf("key %s does not exist in secret %s", ref.Property, ref.Key)
 }
@@ -418,6 +424,7 @@ func (ibm *providerIBM) GetSecretMap(_ context.Context, ref esv1beta1.ExternalSe
 	}
 
 	secretMap := make(map[string][]byte)
+	secMapBytes := make(map[string][]byte)
 	response, err := getSecretData(ibm, &secretName, secretType)
 	if err != nil {
 		return nil, err
@@ -430,36 +437,42 @@ func (ibm *providerIBM) GetSecretMap(_ context.Context, ref esv1beta1.ExternalSe
 	if ref.MetadataPolicy == esv1beta1.ExternalSecretMetadataPolicyFetch {
 		secretMap = populateSecretMap(secretMap, secMap)
 	}
+	secMapBytes = populateSecretMap(secMapBytes, secMap)
 
 	switch secretType {
 	case sm.Secret_SecretType_Arbitrary:
-		secretMap[arbitraryConst] = []byte(fmt.Sprintf("%v", secMap[payloadConst]))
+		secretMap[arbitraryConst] = secMapBytes[payloadConst]
 		return secretMap, nil
 
 	case sm.Secret_SecretType_UsernamePassword:
-		secretMap[usernameConst] = []byte(fmt.Sprintf("%v", secMap[usernameConst]))
-		secretMap[passwordConst] = []byte(fmt.Sprintf("%v", secMap[passwordConst]))
+		secretMap[usernameConst] = secMapBytes[usernameConst]
+		secretMap[passwordConst] = secMapBytes[passwordConst]
 		return secretMap, nil
 
 	case sm.Secret_SecretType_IamCredentials:
-		secretMap[apikeyConst] = []byte(fmt.Sprintf("%v", secMap[smAPIKeyConst]))
+		secretMap[apikeyConst] = secMapBytes[smAPIKeyConst]
 		return secretMap, nil
 
 	case sm.Secret_SecretType_ImportedCert:
-		secretMap[certificateConst] = []byte(fmt.Sprintf("%v", secMap[certificateConst]))
-		secretMap[intermediateConst] = []byte(fmt.Sprintf("%v", secMap[intermediateConst]))
-		secretMap[privateKeyConst] = []byte(fmt.Sprintf("%v", secMap[privateKeyConst]))
+		secretMap[certificateConst] = secMapBytes[certificateConst]
+		secretMap[intermediateConst] = secMapBytes[intermediateConst]
+		if v, ok := secMapBytes[privateKeyConst]; ok {
+			secretMap[privateKeyConst] = v
+		} else {
+			fmt.Printf("warn: %s is empty for secret %s\n", privateKeyConst, secretName)
+			secretMap[privateKeyConst] = []byte("")
+		}
 		return secretMap, nil
 
 	case sm.Secret_SecretType_PublicCert:
-		secretMap[certificateConst] = []byte(fmt.Sprintf("%v", secMap[certificateConst]))
-		secretMap[intermediateConst] = []byte(fmt.Sprintf("%v", secMap[intermediateConst]))
-		secretMap[privateKeyConst] = []byte(fmt.Sprintf("%v", secMap[privateKeyConst]))
+		secretMap[certificateConst] = secMapBytes[certificateConst]
+		secretMap[intermediateConst] = secMapBytes[intermediateConst]
+		secretMap[privateKeyConst] = secMapBytes[privateKeyConst]
 		return secretMap, nil
 
 	case sm.Secret_SecretType_PrivateCert:
-		secretMap[certificateConst] = []byte(fmt.Sprintf("%v", secMap[certificateConst]))
-		secretMap[privateKeyConst] = []byte(fmt.Sprintf("%v", secMap[privateKeyConst]))
+		secretMap[certificateConst] = secMapBytes[certificateConst]
+		secretMap[privateKeyConst] = secMapBytes[privateKeyConst]
 		return secretMap, nil
 
 	case sm.Secret_SecretType_Kv:

pkg/provider/ibm/provider_test.go

@@ -340,6 +340,22 @@ func TestIBMSecretManagerGetSecret(t *testing.T) {
 	}
 	setSecretCert := funcSetCertSecretTest(importedCert, "good case: imported_cert type with property", sm.Secret_SecretType_ImportedCert, true)
 
+	// good case: imported_cert type without a private_key
+	importedCertNoPvtKey := func(smtc *secretManagerTestCase) {
+		secret := &sm.ImportedCertificate{
+			SecretType:  utilpointer.To(sm.Secret_SecretType_ImportedCert),
+			Name:        utilpointer.To("testyname"),
+			ID:          utilpointer.To(secretUUID),
+			Certificate: utilpointer.To(secretCertificate),
+		}
+		smtc.name = "good case: imported cert without private key"
+		smtc.apiInput.ID = utilpointer.To(secretUUID)
+		smtc.apiOutput = secret
+		smtc.ref.Key = "imported_cert/" + secretUUID
+		smtc.ref.Property = "private_key"
+		smtc.expectedSecret = ""
+	}
+
 	// bad case: imported_cert type without property
 	badSecretCert := funcSetCertSecretTest(importedCert, "bad case: imported_cert type without property", sm.Secret_SecretType_ImportedCert, false)
 
@@ -493,6 +509,7 @@ func TestIBMSecretManagerGetSecret(t *testing.T) {
 		makeValidSecretManagerTestCaseCustom(setSecretKVWithOutKey),
 		makeValidSecretManagerTestCaseCustom(badSecretKV),
 		makeValidSecretManagerTestCaseCustom(badSecretCert),
+		makeValidSecretManagerTestCaseCustom(importedCertNoPvtKey),
 		makeValidSecretManagerTestCaseCustom(setSecretPublicCert),
 		makeValidSecretManagerTestCaseCustom(badSecretPublicCert),
 		makeValidSecretManagerTestCaseCustom(setSecretPrivateCert),
@@ -500,16 +517,16 @@ func TestIBMSecretManagerGetSecret(t *testing.T) {
 	}
 
 	sm := providerIBM{}
-	for k, v := range successCases {
+	for _, v := range successCases {
 		t.Run(v.name, func(t *testing.T) {
 			sm.IBMClient = v.mockClient
 			sm.cache = NewCache(10, 1*time.Minute)
 			out, err := sm.GetSecret(context.Background(), *v.ref)
 			if !ErrorContains(err, v.expectError) {
-				t.Errorf("[%d] unexpected error: %s, expected: '%s'", k, err.Error(), v.expectError)
+				t.Errorf("unexpected error:\n%s, expected:\n'%s'", err.Error(), v.expectError)
 			}
 			if string(out) != v.expectedSecret {
-				t.Errorf("[%d] unexpected secret: expected %s, got %s", k, v.expectedSecret, string(out))
+				t.Errorf("unexpected secret: expected:\n%s\ngot:\n%s", v.expectedSecret, string(out))
 			}
 		})
 	}
@@ -779,6 +796,29 @@ func TestGetSecretMap(t *testing.T) {
 		}
 	}
 
+	// good case: imported_cert without a private_key
+	setimportedCertWithNoPvtKey := func(smtc *secretManagerTestCase) {
+		secret := &sm.ImportedCertificate{
+			CreatedBy:    utilpointer.To("testCreatedBy"),
+			CreatedAt:    &strfmt.DateTime{},
+			Downloaded:   utilpointer.To(false),
+			Labels:       []string{"abc", "def", "xyz"},
+			LocksTotal:   utilpointer.To(int64(20)),
+			Certificate:  utilpointer.To(secretCertificate),
+			Intermediate: utilpointer.To(secretIntermediate),
+		}
+		smtc.name = "good case: imported_cert without private key"
+		smtc.apiInput.ID = utilpointer.To(secretUUID)
+		smtc.apiOutput = secret
+		smtc.ref.Key = "imported_cert/" + secretUUID
+
+		smtc.expectedData = map[string][]byte{
+			"certificate":  []byte(secretCertificate),
+			"intermediate": []byte(secretIntermediate),
+			"private_key":  []byte(""),
+		}
+	}
+
 	// good case: public_cert with metadata
 	setPublicCertWithMetadata := func(smtc *secretManagerTestCase) {
 		secret := &sm.PublicCertificate{
@@ -992,6 +1032,7 @@ func TestGetSecretMap(t *testing.T) {
 		makeValidSecretManagerTestCaseCustom(badSecretKVWithUnknownProperty),
 		makeValidSecretManagerTestCaseCustom(setSecretPublicCert),
 		makeValidSecretManagerTestCaseCustom(setSecretPrivateCert),
+		makeValidSecretManagerTestCaseCustom(setimportedCertWithNoPvtKey),
 		makeValidSecretManagerTestCaseCustom(setSecretIamWithMetadata),
 		makeValidSecretManagerTestCaseCustom(setArbitraryWithMetadata),
 		makeValidSecretManagerTestCaseCustom(setSecretUserPassWithMetadata),
@@ -1003,15 +1044,15 @@ func TestGetSecretMap(t *testing.T) {
 	}
 
 	sm := providerIBM{}
-	for k, v := range successCases {
+	for _, v := range successCases {
 		t.Run(v.name, func(t *testing.T) {
 			sm.IBMClient = v.mockClient
 			out, err := sm.GetSecretMap(context.Background(), *v.ref)
 			if !ErrorContains(err, v.expectError) {
-				t.Errorf(" unexpected error: %s, expected: '%s'", err.Error(), v.expectError)
+				t.Errorf("unexpected error: %s, expected: '%s'", err.Error(), v.expectError)
 			}
 			if err == nil && !reflect.DeepEqual(out, v.expectedData) {
-				t.Errorf("[%d] unexpected secret data: expected %+v, got %v", k, v.expectedData, out)
+				t.Errorf("unexpected secret data: expected:\n%+v\ngot:\n%+v", v.expectedData, out)
 			}
 		})
 	}