Limited Cache bug fix - fallback to non cached client (#452)

--------- Co-authored-by: I501080 <[email protected]>
SAP · Jul 18, 2024 · f3f0f6d · f3f0f6d
1 parent 44adf1f
commit f3f0f6d
Show file tree

Hide file tree

Showing 11 changed files with 231 additions and 132 deletions.
diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go
@@ -66,12 +66,11 @@ const (
 // ServiceBindingReconciler reconciles a ServiceBinding object
 type ServiceBindingReconciler struct {
 	client.Client
-	Log            logr.Logger
-	Scheme         *runtime.Scheme
-	GetSMClient    func(ctx context.Context, secretResolver *utils.SecretResolver, resourceNamespace, btpAccessSecretName string) (sm.Client, error)
-	Config         config.Config
-	SecretResolver *utils.SecretResolver
-	Recorder       record.EventRecorder
+	Log         logr.Logger
+	Scheme      *runtime.Scheme
+	GetSMClient func(ctx context.Context, resourceNamespace, btpAccessSecretName string) (sm.Client, error)
+	Config      config.Config
+	Recorder    record.EventRecorder
 }
 
 // +kubebuilder:rbac:groups=services.cloud.sap.com,resources=servicebindings,verbs=get;list;watch;create;update;patch;delete
@@ -205,7 +204,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 			return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding)
 		}
 
-		smClient, err := r.GetSMClient(ctx, r.SecretResolver, getBTPAccessSecretNamespace(serviceBinding), serviceInstance.Spec.BTPAccessCredentialsSecret)
+		smClient, err := r.GetSMClient(ctx, getBTPAccessSecretNamespace(serviceBinding), serviceInstance.Spec.BTPAccessCredentialsSecret)
 		if err != nil {
 			return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceBinding)
 		}
@@ -228,7 +227,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 func (r *ServiceBindingReconciler) updateSecret(ctx context.Context, serviceBinding *servicesv1.ServiceBinding, serviceInstance *servicesv1.ServiceInstance, log logr.Logger) error {
 	log.Info("Updating secret according to the new template")
-	smClient, err := r.GetSMClient(ctx, r.SecretResolver, getBTPAccessSecretNamespace(serviceBinding), serviceInstance.Spec.BTPAccessCredentialsSecret)
+	smClient, err := r.GetSMClient(ctx, getBTPAccessSecretNamespace(serviceBinding), serviceInstance.Spec.BTPAccessCredentialsSecret)
 	if err != nil {
 		return err
 	}
@@ -260,7 +259,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s
 	log := utils.GetLogger(ctx)
 	log.Info("Creating smBinding in SM")
 	serviceBinding.Status.InstanceID = serviceInstance.Status.InstanceID
-	_, bindingParameters, err := utils.BuildSMRequestParameters(r.Client, serviceBinding.Namespace, serviceBinding.Spec.ParametersFrom, serviceBinding.Spec.Parameters)
+	_, bindingParameters, err := utils.BuildSMRequestParameters(serviceBinding.Namespace, serviceBinding.Spec.ParametersFrom, serviceBinding.Spec.Parameters)
 	if err != nil {
 		log.Error(err, "failed to parse smBinding parameters")
 		return utils.MarkAsNonTransientError(ctx, r.Client, smClientTypes.CREATE, err.Error(), serviceBinding)
@@ -323,7 +322,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s
 func (r *ServiceBindingReconciler) delete(ctx context.Context, serviceBinding *servicesv1.ServiceBinding, btpAccessCredentialsSecret string) (ctrl.Result, error) {
 	log := utils.GetLogger(ctx)
 	if controllerutil.ContainsFinalizer(serviceBinding, common.FinalizerName) {
-		smClient, err := r.GetSMClient(ctx, r.SecretResolver, getBTPAccessSecretNamespace(serviceBinding), btpAccessCredentialsSecret)
+		smClient, err := r.GetSMClient(ctx, getBTPAccessSecretNamespace(serviceBinding), btpAccessCredentialsSecret)
 		if err != nil {
 			return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceBinding)
 		}
@@ -386,7 +385,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *ser
 	log := utils.GetLogger(ctx)
 	log.Info(fmt.Sprintf("resource is in progress, found operation url %s", serviceBinding.Status.OperationURL))
 
-	smClient, err := r.GetSMClient(ctx, r.SecretResolver, getBTPAccessSecretNamespace(serviceBinding), btpAccessCredentialsSecret)
+	smClient, err := r.GetSMClient(ctx, getBTPAccessSecretNamespace(serviceBinding), btpAccessCredentialsSecret)
 	if err != nil {
 		return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceBinding)
 	}
@@ -502,7 +501,9 @@ func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *servic
 		shouldUpdateStatus = true
 	}
 	if !utils.IsFailed(binding) {
-		if _, err := r.getSecret(ctx, binding.Namespace, binding.Spec.SecretName); err != nil {
+		secret, err := r.getSecret(ctx, binding.Namespace, binding.Spec.SecretName)
+		if err != nil {
+			// secret was deleted
 			if apierrors.IsNotFound(err) && !utils.IsMarkedForDeletion(binding.ObjectMeta) {
 				log.Info(fmt.Sprintf("secret not found recovering binding %s", binding.Name))
 				binding.Status.BindingID = ""
@@ -513,6 +514,17 @@ func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *servic
 			} else {
 				return ctrl.Result{}, err
 			}
+		} else { // secret exists, validate it has the required labels
+			if secret.Labels == nil {
+				secret.Labels = map[string]string{}
+			}
+			if secret.Labels[common.ManagedByBTPOperatorLabel] != "true" {
+				secret.Labels[common.ManagedByBTPOperatorLabel] = "true"
+				if err = r.Client.Update(ctx, secret); err != nil {
+					log.Error(err, "failed to update secret labels")
+					return ctrl.Result{}, err
+				}
+			}
 		}
 	}
 
@@ -605,7 +617,7 @@ func (r *ServiceBindingReconciler) storeBindingSecret(ctx context.Context, k8sBi
 		return err
 	}
 
-	if len(secret.Labels) == 0 {
+	if secret.Labels == nil {
 		secret.Labels = map[string]string{}
 	}
 	secret.Labels[common.ManagedByBTPOperatorLabel] = "true"
@@ -623,6 +635,7 @@ func (r *ServiceBindingReconciler) createBindingSecret(ctx context.Context, k8sB
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        k8sBinding.Spec.SecretName,
 			Annotations: map[string]string{"binding": k8sBinding.Name},
+			Labels:      map[string]string{common.ManagedByBTPOperatorLabel: "true"},
 			Namespace:   k8sBinding.Namespace,
 		},
 		Data: credentialsMap,
@@ -715,6 +728,10 @@ func (r *ServiceBindingReconciler) createBindingSecretFromSecretTemplate(ctx con
 	}
 	secret.SetNamespace(k8sBinding.Namespace)
 	secret.SetName(k8sBinding.Spec.SecretName)
+	if secret.Labels == nil {
+		secret.Labels = map[string]string{}
+	}
+	secret.Labels[common.ManagedByBTPOperatorLabel] = "true"
 
 	// if no data provided use the default data
 	if len(secret.Data) == 0 && len(secret.StringData) == 0 {
@@ -797,7 +814,7 @@ func (r *ServiceBindingReconciler) deleteSecretAndRemoveFinalizer(ctx context.Co
 
 func (r *ServiceBindingReconciler) getSecret(ctx context.Context, namespace string, name string) (*corev1.Secret, error) {
 	secret := &corev1.Secret{}
-	err := r.Client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, secret)
+	err := utils.GetSecretWithFallback(ctx, types.NamespacedName{Namespace: namespace, Name: name}, secret)
 	return secret, err
 }
 
@@ -942,7 +959,7 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, bindin
 	}
 
 	if len(bindings.Items) == 0 {
-		smClient, err := r.GetSMClient(ctx, r.SecretResolver, getBTPAccessSecretNamespace(binding), btpAccessCredentialsSecret)
+		smClient, err := r.GetSMClient(ctx, getBTPAccessSecretNamespace(binding), btpAccessCredentialsSecret)
 		if err != nil {
 			return err
 		}

diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go
@@ -53,12 +53,11 @@ import (
 // ServiceInstanceReconciler reconciles a ServiceInstance object
 type ServiceInstanceReconciler struct {
 	client.Client
-	Log            logr.Logger
-	Scheme         *runtime.Scheme
-	GetSMClient    func(ctx context.Context, secretResolver *utils.SecretResolver, resourceNamespace, btpAccessSecretName string) (sm.Client, error)
-	Config         config.Config
-	SecretResolver *utils.SecretResolver
-	Recorder       record.EventRecorder
+	Log         logr.Logger
+	Scheme      *runtime.Scheme
+	GetSMClient func(ctx context.Context, resourceNamespace, btpAccessSecretName string) (sm.Client, error)
+	Config      config.Config
+	Recorder    record.EventRecorder
 }
 
 // +kubebuilder:rbac:groups=services.cloud.sap.com,resources=serviceinstances,verbs=get;list;watch;create;update;patch;delete
@@ -123,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, r.SecretResolver, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
+	smClient, err := r.GetSMClient(ctx, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
 	if err != nil {
 		log.Error(err, "failed to get sm client")
 		return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceInstance)
@@ -173,7 +172,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient
 	log := utils.GetLogger(ctx)
 	log.Info("Creating instance in SM")
 	updateHashedSpecValue(serviceInstance)
-	_, instanceParameters, err := utils.BuildSMRequestParameters(r.Client, serviceInstance.Namespace, serviceInstance.Spec.ParametersFrom, serviceInstance.Spec.Parameters)
+	_, instanceParameters, err := utils.BuildSMRequestParameters(serviceInstance.Namespace, serviceInstance.Spec.ParametersFrom, serviceInstance.Spec.Parameters)
 	if err != nil {
 		// if parameters are invalid there is nothing we can do, the user should fix it according to the error message in the condition
 		log.Error(err, "failed to parse instance parameters")
@@ -229,7 +228,7 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient
 
 	updateHashedSpecValue(serviceInstance)
 
-	_, instanceParameters, err := utils.BuildSMRequestParameters(r.Client, serviceInstance.Namespace, serviceInstance.Spec.ParametersFrom, serviceInstance.Spec.Parameters)
+	_, instanceParameters, err := utils.BuildSMRequestParameters(serviceInstance.Namespace, serviceInstance.Spec.ParametersFrom, serviceInstance.Spec.Parameters)
 	if err != nil {
 		log.Error(err, "failed to parse instance parameters")
 		return utils.MarkAsNonTransientError(ctx, r.Client, smClientTypes.UPDATE, fmt.Sprintf("failed to parse parameters: %v", err.Error()), serviceInstance)
@@ -267,7 +266,7 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, serviceI
 	log := utils.GetLogger(ctx)
 
 	if controllerutil.ContainsFinalizer(serviceInstance, common.FinalizerName) {
-		smClient, err := r.GetSMClient(ctx, r.SecretResolver, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
+		smClient, err := r.GetSMClient(ctx, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
 		if err != nil {
 			log.Error(err, "failed to get sm client")
 			return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceInstance)
@@ -349,7 +348,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharing(ctx context.Context, s
 func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *servicesv1.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, r.SecretResolver, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
+	smClient, err := r.GetSMClient(ctx, serviceInstance.Namespace, serviceInstance.Spec.BTPAccessCredentialsSecret)
 	if err != nil {
 		log.Error(err, "failed to get sm client")
 		return utils.MarkAsTransientError(ctx, r.Client, common.Unknown, err, serviceInstance)

diff --git a/controllers/suite_test.go b/controllers/suite_test.go
@@ -28,10 +28,11 @@ import (
 	"testing"
 	"time"
 
+	"github.com/SAP/sap-btp-service-operator/internal/utils"
+
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
 	"github.com/SAP/sap-btp-service-operator/api/common"
-	"github.com/SAP/sap-btp-service-operator/internal/utils"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 
 	. "github.com/onsi/ginkgo"
@@ -119,6 +120,8 @@ var _ = BeforeSuite(func(done Done) {
 	Expect(err).ToNot(HaveOccurred())
 	Expect(k8sClient).ToNot(BeNil())
 
+	utils.InitializeSecretsClient(k8sClient, nil, config.Config{EnableLimitedCache: false})
+
 	webhookInstallOptions := &testEnv.WebhookInstallOptions
 
 	k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
@@ -155,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, _ *utils.SecretResolver, _, _ string) (sm.Client, error) {
+		GetSMClient: func(_ context.Context, _, _ string) (sm.Client, error) {
 			return fakeClient, nil
 		},
 		Config:   testConfig,
@@ -167,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, _ *utils.SecretResolver, _, _ string) (sm.Client, error) {
+		GetSMClient: func(_ context.Context, _, _ string) (sm.Client, error) {
 			return fakeClient, nil
 		},
 		Config:   testConfig,

diff --git a/go.mod b/go.mod
@@ -27,6 +27,7 @@ require (
 	github.com/cespare/xxhash/v2 v2.2.0 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/emicklei/go-restful/v3 v3.11.0 // indirect
+	github.com/evanphx/json-patch v4.12.0+incompatible // indirect
 	github.com/evanphx/json-patch/v5 v5.9.0 // indirect
 	github.com/fsnotify/fsnotify v1.7.0 // indirect
 	github.com/go-logr/zapr v1.3.0 // indirect

diff --git a/internal/utils/parameters.go b/internal/utils/parameters.go
@@ -9,10 +9,8 @@ import (
 	servicesv1 "github.com/SAP/sap-btp-service-operator/api/v1"
 	corev1 "k8s.io/api/core/v1"
 
-	"k8s.io/apimachinery/pkg/types"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/yaml"
 )
 
@@ -22,11 +20,11 @@ import (
 // secret values.
 // The second return value is parameters marshalled to byt array
 // The third return value is any error that caused the function to fail.
-func BuildSMRequestParameters(kubeClient client.Client, namespace string, parametersFrom []servicesv1.ParametersFromSource, parameters *runtime.RawExtension) (map[string]interface{}, []byte, error) {
+func BuildSMRequestParameters(namespace string, parametersFrom []servicesv1.ParametersFromSource, parameters *runtime.RawExtension) (map[string]interface{}, []byte, error) {
 	params := make(map[string]interface{})
 	if len(parametersFrom) > 0 {
 		for _, p := range parametersFrom {
-			fps, err := fetchParametersFromSource(kubeClient, namespace, &p)
+			fps, err := fetchParametersFromSource(namespace, &p)
 			if err != nil {
 				return nil, nil, err
 			}
@@ -96,9 +94,9 @@ func unmarshalJSON(in []byte) (map[string]interface{}, error) {
 }
 
 // fetchSecretKeyValue requests and returns the contents of the given secret key
-func fetchSecretKeyValue(kubeClient client.Client, namespace string, secretKeyRef *servicesv1.SecretKeyReference) ([]byte, error) {
+func fetchSecretKeyValue(namespace string, secretKeyRef *servicesv1.SecretKeyReference) ([]byte, error) {
 	secret := &corev1.Secret{}
-	err := kubeClient.Get(context.Background(), types.NamespacedName{Namespace: namespace, Name: secretKeyRef.Name}, secret)
+	err := GetSecretWithFallback(context.Background(), types.NamespacedName{Namespace: namespace, Name: secretKeyRef.Name}, secret)
 
 	if err != nil {
 		return nil, err
@@ -108,10 +106,10 @@ func fetchSecretKeyValue(kubeClient client.Client, namespace string, secretKeyRe
 
 // fetchParametersFromSource fetches data from a specified external source and
 // represents it in the parameters map format
-func fetchParametersFromSource(kubeClient client.Client, namespace string, parametersFrom *servicesv1.ParametersFromSource) (map[string]interface{}, error) {
+func fetchParametersFromSource(namespace string, parametersFrom *servicesv1.ParametersFromSource) (map[string]interface{}, error) {
 	var params map[string]interface{}
 	if parametersFrom.SecretKeyRef != nil {
-		data, err := fetchSecretKeyValue(kubeClient, namespace, parametersFrom.SecretKeyRef)
+		data, err := fetchSecretKeyValue(namespace, parametersFrom.SecretKeyRef)
 		if err != nil {
 			return nil, err
 		}

diff --git a/internal/utils/parameters_test.go b/internal/utils/parameters_test.go
@@ -13,7 +13,7 @@ var _ = Describe("Parameters", func() {
 			var parametersFrom []v1.ParametersFromSource
 			parameters := (*runtime.RawExtension)(nil)
 
-			params, rawParam, err := BuildSMRequestParameters(nil, "", parametersFrom, parameters)
+			params, rawParam, err := BuildSMRequestParameters("", parametersFrom, parameters)
 
 			Expect(err).To(BeNil())
 			Expect(params).To(BeNil())
@@ -25,7 +25,7 @@ var _ = Describe("Parameters", func() {
 				Raw: []byte(`{"key":"value"}`),
 			}
 
-			params, rawParam, err := BuildSMRequestParameters(nil, "", parametersFrom, parameters)
+			params, rawParam, err := BuildSMRequestParameters("", parametersFrom, parameters)
 
 			Expect(err).To(BeNil())
 			Expect(params).To(Equal(map[string]interface{}{"key": "value"}))