fix(servicemanager): improve Secret-Data error handling (#52)

Co-authored-by: sdischer-sap <[email protected]>
SAP · Dec 16, 2024 · 4804cee · 4804cee
1 parent 9829296
commit 4804cee
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 16 deletions.
diff --git a/internal/clients/servicemanager/api_plan_resolver.go b/internal/clients/servicemanager/api_plan_resolver.go
@@ -14,7 +14,13 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-const ErrInvalidSecretData = "BindingCredentials can't be created from invalid secret data"
+const ErrUnmarshalToBindingCredentials = "JSON can not be unmarshalled to BindingCredentials"
+const ErrJsonMarshal = "Error marshalling secret data"
+const ErrMissingClientId = "Client ID (clientid) is missing"
+const ErrMissingClientSecret = "Client Secret (clientsecret) is missing"
+const ErrMissingSmUrl = "Service Manager URL (sm_url) is missing"
+const ErrMissingUrl = "Token URL (tokenurl) is missing"
+const ErrMissingXsappname = "Xsappname (xsappname) is missing"
 
 // PlanIdResolver used as its own resolval implementation downstream
 type PlanIdResolver interface {
@@ -31,19 +37,34 @@ func NewCredsFromOperatorSecret(secretData map[string][]byte) (BindingCredential
 		plain[k] = string(v)
 	}
 
-	var binding BindingCredentials
 	bytes, err := json.Marshal(plain)
 	if err != nil {
-		return BindingCredentials{}, errors.New(ErrInvalidSecretData)
+		return BindingCredentials{}, errors.New(ErrJsonMarshal)
 	}
+
+	var binding BindingCredentials
 	err = json.Unmarshal(bytes, &binding)
 	if err != nil {
-		return BindingCredentials{}, errors.New(ErrInvalidSecretData)
+		return BindingCredentials{}, errors.New(ErrUnmarshalToBindingCredentials)
+	}
+
+	if plain[apisv1alpha1.ResourceCredentialsXsuaaUrl] == "" {
+		return BindingCredentials{}, errors.New(ErrMissingUrl)
 	}
+
 	binding.Url = internal.Ptr(plain[apisv1alpha1.ResourceCredentialsXsuaaUrl])
 
-	if binding.Clientid == nil || binding.Clientsecret == nil || binding.SmUrl == nil || binding.Url == nil || binding.Xsappname == nil {
-		return BindingCredentials{}, errors.New(ErrInvalidSecretData)
+	if binding.Clientid == nil {
+		return BindingCredentials{}, errors.New(ErrMissingClientId)
+	}
+	if binding.Clientsecret == nil {
+		return BindingCredentials{}, errors.New(ErrMissingClientSecret)
+	}
+	if binding.SmUrl == nil {
+		return BindingCredentials{}, errors.New(ErrMissingSmUrl)
+	}
+	if binding.Xsappname == nil {
+		return BindingCredentials{}, errors.New(ErrMissingXsappname)
 	}
 
 	return binding, nil

diff --git a/internal/clients/servicemanager/api_plan_resolver_test.go b/internal/clients/servicemanager/api_plan_resolver_test.go
@@ -5,11 +5,10 @@ import (
 	"net/http"
 	"testing"
 
-	"github.com/crossplane/crossplane-runtime/pkg/test"
-	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
 	"github.com/sap/crossplane-provider-btp/internal"
 	servicemanager "github.com/sap/crossplane-provider-btp/internal/openapi_clients/btp-service-manager-api-go/pkg"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestNewServiceManagerClient(t *testing.T) {
@@ -174,17 +173,57 @@ func TestNewCredsFromOperatorSecret(t *testing.T) {
 		err    error
 	}{
 		{
-			name: "MissingAttributeError",
+			name: "Missing Attribute Error Client ID",
+			secret: map[string][]byte{
+				"clientsecret": []byte("someSecret"),
+				"sm_url":       []byte("https://valid.url"),
+				"tokenurl":     []byte("https://valid.url"),
+				"xsappname":    []byte("someXsAppName"),
+			},
+			err: errors.New(ErrMissingClientId),
+		},
+		{
+			name: "Missing Attribute Error Client Secret",
+			secret: map[string][]byte{
+				"clientid":  []byte("someClientId"),
+				"sm_url":    []byte("https://valid.url"),
+				"tokenurl":  []byte("https://valid.url"),
+				"xsappname": []byte("someXsAppName"),
+			},
+			err: errors.New(ErrMissingClientSecret),
+		},
+		{
+			name: "Missing Attribute Error token URL",
+			secret: map[string][]byte{
+				"clientid":     []byte("someClientId"),
+				"clientsecret": []byte("someSecret"),
+				"sm_url":       []byte("https://valid.url"),
+				"xsappname":    []byte("someXsAppName"),
+			},
+			err: errors.New(ErrMissingUrl),
+		},
+		{
+			name: "Missing Attribute Error SmUrl",
 			secret: map[string][]byte{
 				"clientid":     []byte("someClientId"),
 				"clientsecret": []byte("someSecret"),
 				"tokenurl":     []byte("https://valid.url"),
 				"xsappname":    []byte("someXsAppName"),
 			},
-			err: errors.New(ErrInvalidSecretData),
+			err: errors.New(ErrMissingSmUrl),
+		},
+		{
+			name: "Missing Attribute Error Xsappname",
+			secret: map[string][]byte{
+				"clientid":     []byte("someClientId"),
+				"clientsecret": []byte("someSecret"),
+				"sm_url":       []byte("https://valid.url"),
+				"tokenurl":     []byte("https://valid.url"),
+			},
+			err: errors.New(ErrMissingXsappname),
 		},
 		{
-			name: "SuccessfulMapping",
+			name: "Successful Mapping",
 			secret: map[string][]byte{
 				"clientid":     []byte("someClientId"),
 				"clientsecret": []byte("someSecret"),
@@ -204,11 +243,9 @@ func TestNewCredsFromOperatorSecret(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			o, err := NewCredsFromOperatorSecret(tc.secret)
-			if diff := cmp.Diff(tc.o, o); diff != "" {
-				t.Errorf("\nNewBindingCredentialsFromSecretData(): -want, +got:\n%s\n", diff)
-			}
-			if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" {
-				t.Errorf("\nNewBindingCredentialsFromSecretData(): -want error, +got error:\n%s\n", diff)
+			assert.Equal(t, tc.o, o)
+			if tc.err != nil {
+				assert.EqualError(t, err, tc.err.Error())
 			}
 
 		})