Internal - BTP access secret resolving improvement (#456)

SAP · Aug 6, 2024 · c2d3f55 · c2d3f55
1 parent 332bf3a
commit c2d3f55
Show file tree

Hide file tree

Showing 14 changed files with 734 additions and 627 deletions.
diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
diff --git a/config/crd/bases/services.cloud.sap.com_servicebindings.yaml b/config/crd/bases/services.cloud.sap.com_servicebindings.yaml
diff --git a/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml b/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -2,7 +2,6 @@
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
-  creationTimestamp: null
   name: manager-role
 rules:
 - apiGroups:

diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml
@@ -2,7 +2,6 @@
 apiVersion: admissionregistration.k8s.io/v1
 kind: MutatingWebhookConfiguration
 metadata:
-  creationTimestamp: null
   name: mutating-webhook-configuration
 webhooks:
 - admissionReviewVersions:
@@ -51,7 +50,6 @@ webhooks:
 apiVersion: admissionregistration.k8s.io/v1
 kind: ValidatingWebhookConfiguration
 metadata:
-  creationTimestamp: null
   name: validating-webhook-configuration
 webhooks:
 - admissionReviewVersions:

diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go
diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go
@@ -36,7 +36,7 @@ import (
 	"k8s.io/client-go/util/workqueue"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 
-	servicesv1 "github.com/SAP/sap-btp-service-operator/api/v1"
+	v1 "github.com/SAP/sap-btp-service-operator/api/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 
 	"github.com/google/uuid"
@@ -55,7 +55,7 @@ type ServiceInstanceReconciler struct {
 	client.Client
 	Log         logr.Logger
 	Scheme      *runtime.Scheme
-	GetSMClient func(ctx context.Context, resourceNamespace, btpAccessSecretName string) (sm.Client, error)
+	GetSMClient func(ctx context.Context, serviceInstance *v1.ServiceInstance) (sm.Client, error)
 	Config      config.Config
 	Recorder    record.EventRecorder
 }
@@ -69,7 +69,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	log := r.Log.WithValues("serviceinstance", req.NamespacedName).WithValues("correlation_id", uuid.New().String())
 	ctx = context.WithValue(ctx, utils.LogKey{}, log)
 
-	serviceInstance := &servicesv1.ServiceInstance{}
+	serviceInstance := &v1.ServiceInstance{}
 	if err := r.Client.Get(ctx, req.NamespacedName, serviceInstance); err != nil {
 		if !apierrors.IsNotFound(err) {
 			log.Error(err, "unable to fetch ServiceInstance")
@@ -122,7 +122,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	log.Info(fmt.Sprintf("instance is not in final state, handling... (generation: %d, observedGen: %d", serviceInstance.Generation, serviceInstance.Status.ObservedGeneration))
 	serviceInstance.SetObservedGeneration(serviceInstance.Generation)
 
-	smClient, err := r.GetSMClient(ctx, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
+	smClient, err := r.GetSMClient(ctx, serviceInstance)
 	if err != nil {
 		log.Error(err, "failed to get sm client")
 		return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceInstance)
@@ -163,12 +163,12 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 
 func (r *ServiceInstanceReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
-		For(&servicesv1.ServiceInstance{}).
+		For(&v1.ServiceInstance{}).
 		WithOptions(controller.Options{RateLimiter: workqueue.NewItemExponentialFailureRateLimiter(r.Config.RetryBaseDelay, r.Config.RetryMaxDelay)}).
 		Complete(r)
 }
 
-func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) {
+func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient sm.Client, serviceInstance *v1.ServiceInstance) (ctrl.Result, error) {
 	log := utils.GetLogger(ctx)
 	log.Info("Creating instance in SM")
 	updateHashedSpecValue(serviceInstance)
@@ -222,7 +222,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient
 	return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance)
 }
 
-func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) {
+func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient sm.Client, serviceInstance *v1.ServiceInstance) (ctrl.Result, error) {
 	log := utils.GetLogger(ctx)
 	log.Info(fmt.Sprintf("updating instance %s in SM", serviceInstance.Status.InstanceID))
 
@@ -262,11 +262,11 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient
 	return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance)
 }
 
-func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) {
+func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, serviceInstance *v1.ServiceInstance) (ctrl.Result, error) {
 	log := utils.GetLogger(ctx)
 
 	if controllerutil.ContainsFinalizer(serviceInstance, common.FinalizerName) {
-		smClient, err := r.GetSMClient(ctx, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
+		smClient, err := r.GetSMClient(ctx, serviceInstance)
 		if err != nil {
 			log.Error(err, "failed to get sm client")
 			return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceInstance)
@@ -311,7 +311,7 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, serviceI
 	return ctrl.Result{}, nil
 }
 
-func (r *ServiceInstanceReconciler) handleInstanceSharing(ctx context.Context, serviceInstance *servicesv1.ServiceInstance, smClient sm.Client) (ctrl.Result, error) {
+func (r *ServiceInstanceReconciler) handleInstanceSharing(ctx context.Context, serviceInstance *v1.ServiceInstance, smClient sm.Client) (ctrl.Result, error) {
 	log := utils.GetLogger(ctx)
 	log.Info("Handling change in instance sharing")
 
@@ -345,10 +345,10 @@ func (r *ServiceInstanceReconciler) handleInstanceSharing(ctx context.Context, s
 	return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance)
 }
 
-func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) {
+func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *v1.ServiceInstance) (ctrl.Result, error) {
 	log := utils.GetLogger(ctx)
 	log.Info(fmt.Sprintf("resource is in progress, found operation url %s", serviceInstance.Status.OperationURL))
-	smClient, err := r.GetSMClient(ctx, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
+	smClient, err := r.GetSMClient(ctx, serviceInstance)
 	if err != nil {
 		log.Error(err, "failed to get sm client")
 		return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceInstance)
@@ -359,7 +359,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *s
 		log.Info(fmt.Sprintf("failed to fetch operation, got error from SM: %s", statusErr.Error()), "operationURL", serviceInstance.Status.OperationURL)
 		utils.SetInProgressConditions(ctx, serviceInstance.Status.OperationType, string(smClientTypes.INPROGRESS), serviceInstance)
 		// if failed to read operation status we cleanup the status to trigger re-sync from SM
-		freshStatus := servicesv1.ServiceInstanceStatus{Conditions: serviceInstance.GetConditions(), ObservedGeneration: serviceInstance.Generation}
+		freshStatus := v1.ServiceInstanceStatus{Conditions: serviceInstance.GetConditions(), ObservedGeneration: serviceInstance.Generation}
 		if utils.IsMarkedForDeletion(serviceInstance.ObjectMeta) {
 			freshStatus.InstanceID = serviceInstance.Status.InstanceID
 		}
@@ -425,7 +425,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *s
 	return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance)
 }
 
-func (r *ServiceInstanceReconciler) handleAsyncDelete(ctx context.Context, serviceInstance *servicesv1.ServiceInstance, opURL string) (ctrl.Result, error) {
+func (r *ServiceInstanceReconciler) handleAsyncDelete(ctx context.Context, serviceInstance *v1.ServiceInstance, opURL string) (ctrl.Result, error) {
 	serviceInstance.Status.OperationURL = opURL
 	serviceInstance.Status.OperationType = smClientTypes.DELETE
 	utils.SetInProgressConditions(ctx, smClientTypes.DELETE, "", serviceInstance)
@@ -437,7 +437,7 @@ func (r *ServiceInstanceReconciler) handleAsyncDelete(ctx context.Context, servi
 	return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil
 }
 
-func (r *ServiceInstanceReconciler) getInstanceForRecovery(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance) (*smClientTypes.ServiceInstance, error) {
+func (r *ServiceInstanceReconciler) getInstanceForRecovery(ctx context.Context, smClient sm.Client, serviceInstance *v1.ServiceInstance) (*smClientTypes.ServiceInstance, error) {
 	log := utils.GetLogger(ctx)
 	parameters := sm.Parameters{
 		FieldQuery: []string{
@@ -462,7 +462,7 @@ func (r *ServiceInstanceReconciler) getInstanceForRecovery(ctx context.Context,
 	return nil, nil
 }
 
-func (r *ServiceInstanceReconciler) recover(ctx context.Context, smClient sm.Client, k8sInstance *servicesv1.ServiceInstance, smInstance *smClientTypes.ServiceInstance) (ctrl.Result, error) {
+func (r *ServiceInstanceReconciler) recover(ctx context.Context, smClient sm.Client, k8sInstance *v1.ServiceInstance, smInstance *smClientTypes.ServiceInstance) (ctrl.Result, error) {
 	log := utils.GetLogger(ctx)
 
 	log.Info(fmt.Sprintf("found existing instance in SM with id %s, updating status", smInstance.ID))
@@ -546,7 +546,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte
 	return ctrl.Result{Requeue: isTransient}, utils.UpdateStatus(ctx, r.Client, object)
 }
 
-func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance) bool {
+func isFinalState(ctx context.Context, serviceInstance *v1.ServiceInstance) bool {
 	log := utils.GetLogger(ctx)
 	if utils.IsMarkedForDeletion(serviceInstance.ObjectMeta) {
 		log.Info("instance is not in final state, it is marked for deletion")
@@ -579,7 +579,7 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan
 	return true
 }
 
-func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool {
+func updateRequired(serviceInstance *v1.ServiceInstance) bool {
 	//update is not supported for failed instances (this can occur when instance creation was asynchronously)
 	if serviceInstance.Status.Ready != metav1.ConditionTrue {
 		return false
@@ -593,7 +593,7 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool {
 	return getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec
 }
 
-func sharingUpdateRequired(serviceInstance *servicesv1.ServiceInstance) bool {
+func sharingUpdateRequired(serviceInstance *v1.ServiceInstance) bool {
 	//relevant only for non-shared instances - sharing instance is possible only for usable instances
 	if serviceInstance.Status.Ready != metav1.ConditionTrue {
 		return false
@@ -661,7 +661,7 @@ func getTags(tags []byte) ([]string, error) {
 	return tagsArr, nil
 }
 
-func getSpecHash(serviceInstance *servicesv1.ServiceInstance) string {
+func getSpecHash(serviceInstance *v1.ServiceInstance) string {
 	spec := serviceInstance.Spec
 	spec.Shared = ptr.To(false)
 	specBytes, _ := json.Marshal(spec)
@@ -699,7 +699,7 @@ func setSharedCondition(object common.SAPBTPResource, status metav1.ConditionSta
 	object.SetConditions(conditions)
 }
 
-func updateHashedSpecValue(serviceInstance *servicesv1.ServiceInstance) {
+func updateHashedSpecValue(serviceInstance *v1.ServiceInstance) {
 	serviceInstance.Status.HashedSpec = getSpecHash(serviceInstance)
 }
 

diff --git a/controllers/suite_test.go b/controllers/suite_test.go
@@ -158,7 +158,7 @@ var _ = BeforeSuite(func(done Done) {
 		Client: k8sManager.GetClient(),
 		Scheme: k8sManager.GetScheme(),
 		Log:    ctrl.Log.WithName("controllers").WithName("ServiceInstance"),
-		GetSMClient: func(_ context.Context, _, _ string) (sm.Client, error) {
+		GetSMClient: func(_ context.Context, _ *v1.ServiceInstance) (sm.Client, error) {
 			return fakeClient, nil
 		},
 		Config:   testConfig,
@@ -170,7 +170,7 @@ var _ = BeforeSuite(func(done Done) {
 		Client: k8sManager.GetClient(),
 		Scheme: k8sManager.GetScheme(),
 		Log:    ctrl.Log.WithName("controllers").WithName("ServiceBinding"),
-		GetSMClient: func(_ context.Context, _, _ string) (sm.Client, error) {
+		GetSMClient: func(_ context.Context, _ *v1.ServiceInstance) (sm.Client, error) {
 			return fakeClient, nil
 		},
 		Config:   testConfig,

diff --git a/internal/utils/secret_resolver.go b/internal/utils/secret_resolver.go
@@ -88,8 +88,8 @@ func (sr *secretClient) getSecretForResource(ctx context.Context, namespace, nam
 	}
 
 	// secret not found in resource namespace, search for namespace-specific secret in management namespace
-	sr.Log.Info(fmt.Sprintf("Searching a secret for namespace %s in the management namespace %s", namespace, sr.ManagementNamespace))
-	err := sr.getWithClientFallback(ctx, types.NamespacedName{Namespace: sr.ManagementNamespace, Name: fmt.Sprintf("%s-%s", namespace, name)}, secretForResource)
+	var err error
+	secretForResource, err = secretsClient.getSecretFromManagementNamespace(ctx, fmt.Sprintf("%s-%s", namespace, name))
 	if err == nil {
 		return secretForResource, nil
 	}
@@ -100,10 +100,10 @@ func (sr *secretClient) getSecretForResource(ctx context.Context, namespace, nam
 	}
 
 	// namespace-specific secret not found in management namespace, fallback to central cluster secret
-	return sr.getDefaultSecret(ctx, name)
+	return sr.getClusterDefaultSecret(ctx, name)
 }
 
-func (sr *secretClient) getDefaultSecret(ctx context.Context, name string) (*v1.Secret, error) {
+func (sr *secretClient) getClusterDefaultSecret(ctx context.Context, name string) (*v1.Secret, error) {
 	secretForResource := &v1.Secret{}
 	sr.Log.Info(fmt.Sprintf("Searching for cluster secret %s in releaseNamespace %s", name, sr.ReleaseNamespace))
 	err := sr.getWithClientFallback(ctx, types.NamespacedName{Namespace: sr.ReleaseNamespace, Name: name}, secretForResource)

diff --git a/internal/utils/sm_utils.go b/internal/utils/sm_utils.go
@@ -4,21 +4,35 @@ import (
 	"context"
 	"fmt"
 
+	v1 "github.com/SAP/sap-btp-service-operator/api/v1"
 	"github.com/SAP/sap-btp-service-operator/client/sm"
-	v1 "k8s.io/api/core/v1"
+	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-func GetSMClient(ctx context.Context, resourceNamespace, btpAccessSecretName string) (sm.Client, error) {
-	log := GetLogger(ctx)
+type InvalidCredentialsError struct{}
 
-	if len(btpAccessSecretName) > 0 {
-		return getBTPAccessClient(ctx, btpAccessSecretName)
-	}
+func (ic *InvalidCredentialsError) Error() string {
+	return "invalid Service-Manager credentials, contact your cluster administrator"
+}
+
+func GetSMClient(ctx context.Context, serviceInstance *v1.ServiceInstance) (sm.Client, error) {
+	log := GetLogger(ctx)
+	var err error
 
-	secret, err := GetSecretForResource(ctx, resourceNamespace, SAPBTPOperatorSecretName)
-	if err != nil {
-		return nil, err
+	var secret *corev1.Secret
+	if len(serviceInstance.Spec.BTPAccessCredentialsSecret) > 0 {
+		secret, err = GetSecretFromManagementNamespace(ctx, serviceInstance.Spec.BTPAccessCredentialsSecret)
+		if err != nil {
+			log.Error(err, "failed to get secret BTPAccessCredentialsSecret")
+			return nil, err
+		}
+	} else {
+		secret, err = GetSecretForResource(ctx, serviceInstance.Namespace, SAPBTPOperatorSecretName)
+		if err != nil {
+			log.Error(err, "failed to get secret for instance")
+			return nil, err
+		}
 	}
 
 	clientConfig := &sm.ClientConfig{
@@ -27,8 +41,8 @@ func GetSMClient(ctx context.Context, resourceNamespace, btpAccessSecretName str
 		URL:            string(secret.Data["sm_url"]),
 		TokenURL:       string(secret.Data["tokenurl"]),
 		TokenURLSuffix: string(secret.Data["tokenurlsuffix"]),
-		TLSPrivateKey:  string(secret.Data[v1.TLSPrivateKeyKey]),
-		TLSCertKey:     string(secret.Data[v1.TLSCertKey]),
+		TLSPrivateKey:  string(secret.Data[corev1.TLSPrivateKeyKey]),
+		TLSCertKey:     string(secret.Data[corev1.TLSCertKey]),
 		SSLDisabled:    false,
 	}
 
@@ -39,45 +53,24 @@ func GetSMClient(ctx context.Context, resourceNamespace, btpAccessSecretName str
 
 	//backward compatibility (tls data in a dedicated secret)
 	if len(clientConfig.ClientSecret) == 0 && (len(clientConfig.TLSPrivateKey) == 0 || len(clientConfig.TLSCertKey) == 0) {
-		tlsSecret, err := GetSecretForResource(ctx, resourceNamespace, SAPBTPOperatorTLSSecretName)
+		if len(serviceInstance.Spec.BTPAccessCredentialsSecret) > 0 && !clientConfig.IsValid() {
+			log.Info("btpAccess secret found but did not contain all the required data")
+			return nil, fmt.Errorf("invalid Service-Manager credentials, contact your cluster administrator")
+		}
+
+		tlsSecret, err := GetSecretForResource(ctx, serviceInstance.Namespace, SAPBTPOperatorTLSSecretName)
 		if client.IgnoreNotFound(err) != nil {
 			return nil, err
 		}
 
-		if tlsSecret == nil || len(tlsSecret.Data) == 0 || len(tlsSecret.Data[v1.TLSCertKey]) == 0 || len(tlsSecret.Data[v1.TLSPrivateKeyKey]) == 0 {
+		if tlsSecret == nil || len(tlsSecret.Data) == 0 || len(tlsSecret.Data[corev1.TLSCertKey]) == 0 || len(tlsSecret.Data[corev1.TLSPrivateKeyKey]) == 0 {
 			log.Info("clientsecret not found in SM credentials, and tls secret is invalid")
-			return nil, fmt.Errorf("invalid Service-Manager credentials, contact your cluster administrator")
+			return nil, &InvalidCredentialsError{}
 		}
 
 		log.Info("found tls configuration")
-		clientConfig.TLSCertKey = string(tlsSecret.Data[v1.TLSCertKey])
-		clientConfig.TLSPrivateKey = string(tlsSecret.Data[v1.TLSPrivateKeyKey])
-	}
-
-	return sm.NewClient(ctx, clientConfig, nil)
-}
-
-func getBTPAccessClient(ctx context.Context, secretName string) (sm.Client, error) {
-	log := GetLogger(ctx)
-	secret, err := GetSecretFromManagementNamespace(ctx, secretName)
-	if err != nil {
-		return nil, err
-	}
-
-	clientConfig := &sm.ClientConfig{
-		ClientID:       string(secret.Data["clientid"]),
-		ClientSecret:   string(secret.Data["clientsecret"]),
-		URL:            string(secret.Data["sm_url"]),
-		TokenURL:       string(secret.Data["tokenurl"]),
-		TokenURLSuffix: string(secret.Data["tokenurlsuffix"]),
-		TLSPrivateKey:  string(secret.Data[v1.TLSPrivateKeyKey]),
-		TLSCertKey:     string(secret.Data[v1.TLSCertKey]),
-		SSLDisabled:    false,
-	}
-
-	if !clientConfig.IsValid() {
-		log.Info("btpAccess secret found but did not contain all the required data")
-		return nil, fmt.Errorf("invalid Service-Manager credentials, contact your cluster administrator")
+		clientConfig.TLSCertKey = string(tlsSecret.Data[corev1.TLSCertKey])
+		clientConfig.TLSPrivateKey = string(tlsSecret.Data[corev1.TLSPrivateKeyKey])
 	}
 
 	return sm.NewClient(ctx, clientConfig, nil)