From 460fe6356a12a48ce8223d65eea1f8da80d5992f Mon Sep 17 00:00:00 2001 From: I501080 Date: Mon, 16 Dec 2024 11:51:14 +0200 Subject: [PATCH 01/12] create binding while instance update is in progress --- controllers/servicebinding_controller.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index fef2f309..7821ea85 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -171,7 +171,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return r.maintain(ctx, serviceBinding) } - if serviceNotUsable(serviceInstance) { + if serviceInstanceNotUsable(serviceInstance) { instanceErr := fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName) utils.SetBlockedCondition(ctx, instanceErr.Error(), serviceBinding) if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { @@ -180,13 +180,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, instanceErr } - if utils.IsInProgress(serviceInstance) { - log.Info(fmt.Sprintf("Service instance with k8s name %s is not ready for binding yet", serviceInstance.Name)) - utils.SetInProgressConditions(ctx, smClientTypes.CREATE, fmt.Sprintf("creation in progress, waiting for service instance '%s' to be ready", serviceBinding.Spec.ServiceInstanceName), serviceBinding) - - return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, utils.UpdateStatus(ctx, r.Client, serviceBinding) - } - //set owner instance only for original bindings (not rotated) if serviceBinding.Labels == nil || len(serviceBinding.Labels[common.StaleBindingIDLabel]) == 0 { if !bindingAlreadyOwnedByInstance(serviceInstance, serviceBinding) && @@ -1155,14 +1148,11 @@ func bindingAlreadyOwnedByInstance(instance *v1.ServiceInstance, binding *v1.Ser return false } -func serviceNotUsable(instance *v1.ServiceInstance) bool { +func serviceInstanceNotUsable(instance *v1.ServiceInstance) bool { if utils.IsMarkedForDeletion(instance.ObjectMeta) { return true } - if len(instance.Status.Conditions) != 0 { - return instance.Status.Conditions[0].Reason == utils.GetConditionReason(smClientTypes.CREATE, smClientTypes.FAILED) - } - return false + return instance.Status.Ready != metav1.ConditionTrue } func getInstanceNameForSecretCredentials(instance *v1.ServiceInstance) []byte { From 3f8c34804a4205b7e3b38de1a5e97d93a4b83f1b Mon Sep 17 00:00:00 2001 From: I501080 Date: Mon, 16 Dec 2024 17:02:00 +0200 Subject: [PATCH 02/12] fix tests --- controllers/servicebinding_controller.go | 8 ++-- controllers/servicebinding_controller_test.go | 44 +++++++++---------- 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 7821ea85..62e82a2d 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -171,7 +171,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return r.maintain(ctx, serviceBinding) } - if serviceInstanceNotUsable(serviceInstance) { + if !serviceInstanceUsable(serviceInstance) { instanceErr := fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName) utils.SetBlockedCondition(ctx, instanceErr.Error(), serviceBinding) if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { @@ -1148,11 +1148,11 @@ func bindingAlreadyOwnedByInstance(instance *v1.ServiceInstance, binding *v1.Ser return false } -func serviceInstanceNotUsable(instance *v1.ServiceInstance) bool { +func serviceInstanceUsable(instance *v1.ServiceInstance) bool { if utils.IsMarkedForDeletion(instance.ObjectMeta) { - return true + return false } - return instance.Status.Ready != metav1.ConditionTrue + return instance.Status.Ready == metav1.ConditionTrue } func getInstanceNameForSecretCredentials(instance *v1.ServiceInstance) []byte { diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 1d1033e9..31a0b10c 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -4,15 +4,14 @@ import ( "context" "encoding/json" "errors" - "github.com/lithammer/dedent" - authv1 "k8s.io/api/authentication/v1" - "net/http" - "strings" - "github.com/SAP/sap-btp-service-operator/api/common" "github.com/SAP/sap-btp-service-operator/internal/utils" + "github.com/lithammer/dedent" + authv1 "k8s.io/api/authentication/v1" "k8s.io/utils/pointer" + "net/http" ctrl "sigs.k8s.io/controller-runtime" + "strings" v1 "github.com/SAP/sap-btp-service-operator/api/v1" "github.com/SAP/sap-btp-service-operator/client/sm" @@ -564,37 +563,34 @@ var _ = Describe("ServiceBinding controller", func() { }) }) - When("referenced service instance is failed", func() { - It("should retry and succeed once the instance is ready", func() { - utils.SetFailureConditions(smClientTypes.CREATE, "Failed to create instance (test)", createdInstance) - updateInstanceStatus(ctx, createdInstance) - binding := createBindingWithBlockedError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", "is not usable") - utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance) - updateInstanceStatus(ctx, createdInstance) - waitForResourceToBeReady(ctx, binding) - }) - }) - When("referenced service instance is not ready", func() { It("should retry and succeed once the instance is ready", func() { - fakeClient.StatusReturns(&smClientTypes.Operation{ResourceID: fakeInstanceID, State: smClientTypes.INPROGRESS}, nil) - utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "", createdInstance) - createdInstance.Status.OperationURL = "/1234" - createdInstance.Status.OperationType = smClientTypes.CREATE + createdInstance.Status.Ready = metav1.ConditionFalse updateInstanceStatus(ctx, createdInstance) createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) Expect(err).ToNot(HaveOccurred()) - Expect(utils.IsInProgress(createdBinding)).To(BeTrue()) + waitForResourceCondition(ctx, createdBinding, common.ConditionSucceeded, metav1.ConditionFalse, common.Blocked, "") - utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance) - createdInstance.Status.OperationType = "" - createdInstance.Status.OperationURL = "" + createdInstance.Status.Ready = metav1.ConditionTrue updateInstanceStatus(ctx, createdInstance) waitForResourceToBeReady(ctx, createdBinding) }) }) + When("referenced service instance is being deleted", func() { + It("should fail", func() { + createdInstance.Finalizers = append(createdInstance.Finalizers, "fake/finalizer") + updateInstance(ctx, createdInstance) + Expect(k8sClient.Delete(ctx, createdInstance)).To(Succeed()) + + createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) + Expect(err).ToNot(HaveOccurred()) + waitForResourceCondition(ctx, createdBinding, common.ConditionSucceeded, metav1.ConditionFalse, common.Blocked, "") + Expect(utils.RemoveFinalizer(ctx, k8sClient, createdInstance, "fake/finalizer")).To(Succeed()) + }) + }) + When("secretTemplate", func() { It("should succeed to create the secret", func() { ctx := context.Background() From 14b7a227121d80ac137cca0eccba947dc3cce096 Mon Sep 17 00:00:00 2001 From: I501080 Date: Mon, 16 Dec 2024 18:58:21 +0200 Subject: [PATCH 03/12] . --- controllers/servicebinding_controller.go | 51 +++++++++++++---------- controllers/serviceinstance_controller.go | 3 +- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 62e82a2d..f0f5b0a2 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -129,48 +129,44 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return r.poll(ctx, serviceBinding, serviceInstance) } - if !controllerutil.ContainsFinalizer(serviceBinding, common.FinalizerName) { - controllerutil.AddFinalizer(serviceBinding, common.FinalizerName) + if controllerutil.AddFinalizer(serviceBinding, common.FinalizerName) { log.Info(fmt.Sprintf("added finalizer '%s' to service binding", common.FinalizerName)) if err := r.Client.Update(ctx, serviceBinding); err != nil { return ctrl.Result{}, err } } - condition := meta.FindStatusCondition(serviceBinding.Status.Conditions, common.ConditionReady) - if serviceBinding.Status.BindingID != "" && condition != nil && condition.ObservedGeneration != serviceBinding.Generation { - err := r.updateSecret(ctx, serviceBinding, serviceInstance, log) - if err != nil { - return r.handleSecretError(ctx, smClientTypes.UPDATE, err, serviceBinding) - } - err = utils.UpdateStatus(ctx, r.Client, serviceBinding) - if err != nil { - return r.handleSecretError(ctx, smClientTypes.UPDATE, err, serviceBinding) - } + readyCond := meta.FindStatusCondition(serviceBinding.Status.Conditions, common.ConditionReady) + if serviceBinding.Status.BindingID != "" && readyCond != nil && readyCond.ObservedGeneration != serviceBinding.Generation { + // cred rotation or secret template changed + log.Info("binding's generation changed, maintaining secret") + return r.maintainSecret(ctx, serviceBinding, serviceInstance) } - isBindingReady := condition != nil && condition.Status == metav1.ConditionTrue + isBindingReady := readyCond != nil && readyCond.Status == metav1.ConditionTrue + isCredRotationInProgress := meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) if isBindingReady { if isStaleServiceBinding(serviceBinding) { return r.handleStaleServiceBinding(ctx, serviceBinding) } - if initCredRotationIfRequired(serviceBinding) { - return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) + if !isCredRotationInProgress { + if initCredRotationIfRequired(serviceBinding) { + log.Info("cred rotation required, updating status") + return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) + } else { + log.Info("Binding in final state, maintaining secret") + return r.maintain(ctx, serviceBinding) + } } } - if meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) { + if isCredRotationInProgress { if err := r.rotateCredentials(ctx, serviceBinding, serviceInstance); err != nil { return ctrl.Result{}, err } } - if isBindingReady { - log.Info("Binding in final state") - return r.maintain(ctx, serviceBinding) - } - if !serviceInstanceUsable(serviceInstance) { instanceErr := fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName) utils.SetBlockedCondition(ctx, instanceErr.Error(), serviceBinding) @@ -528,6 +524,17 @@ func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *v1.Ser return ctrl.Result{}, nil } +func (r *ServiceBindingReconciler) maintainSecret(ctx context.Context, serviceBinding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance) (ctrl.Result, error) { + log := utils.GetLogger(ctx) + if err := r.updateSecret(ctx, serviceBinding, serviceInstance, log); err != nil { + return r.handleSecretError(ctx, smClientTypes.UPDATE, err, serviceBinding) + } + if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { + return r.handleSecretError(ctx, smClientTypes.UPDATE, err, serviceBinding) + } + return ctrl.Result{}, nil +} + func (r *ServiceBindingReconciler) getServiceInstanceForBinding(ctx context.Context, binding *v1.ServiceBinding) (*v1.ServiceInstance, error) { log := utils.GetLogger(ctx) serviceInstance := &v1.ServiceInstance{} @@ -1083,7 +1090,7 @@ func isStaleServiceBinding(binding *v1.ServiceBinding) bool { } func initCredRotationIfRequired(binding *v1.ServiceBinding) bool { - if utils.IsFailed(binding) || !credRotationEnabled(binding) || meta.IsStatusConditionTrue(binding.Status.Conditions, common.ConditionCredRotationInProgress) { + if utils.IsFailed(binding) || !credRotationEnabled(binding) { return false } _, forceRotate := binding.Annotations[common.ForceRotateAnnotation] diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 2cb1d798..7d3f169b 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -111,8 +111,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ return r.poll(ctx, serviceInstance) } - if !controllerutil.ContainsFinalizer(serviceInstance, common.FinalizerName) { - controllerutil.AddFinalizer(serviceInstance, common.FinalizerName) + if controllerutil.AddFinalizer(serviceInstance, common.FinalizerName) { log.Info(fmt.Sprintf("added finalizer '%s' to service instance", common.FinalizerName)) if err := r.Client.Update(ctx, serviceInstance); err != nil { return ctrl.Result{}, err From cd8fe56a914fe22a4232e4545ce35bb270820f34 Mon Sep 17 00:00:00 2001 From: I501080 Date: Tue, 17 Dec 2024 15:25:44 +0200 Subject: [PATCH 04/12] fix tests --- controllers/servicebinding_controller.go | 83 +++++++++---------- controllers/servicebinding_controller_test.go | 39 ++++----- 2 files changed, 54 insertions(+), 68 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 86d121df..e842b2eb 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -136,12 +136,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque } readyCond := meta.FindStatusCondition(serviceBinding.Status.Conditions, common.ConditionReady) - if serviceBinding.Status.BindingID != "" && readyCond != nil && readyCond.ObservedGeneration != serviceBinding.Generation { - // cred rotation or secret template changed - log.Info("binding's generation changed, maintaining secret") - return r.maintainSecret(ctx, serviceBinding, serviceInstance) - } - isBindingReady := readyCond != nil && readyCond.Status == metav1.ConditionTrue isCredRotationInProgress := meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) if isBindingReady { @@ -155,7 +149,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) } else { log.Info("Binding in final state, maintaining secret") - return r.maintain(ctx, serviceBinding) + return r.maintain(ctx, serviceBinding, serviceInstance) } } } @@ -480,54 +474,51 @@ func (r *ServiceBindingReconciler) getBindingForRecovery(ctx context.Context, sm return nil, nil } -func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *v1.ServiceBinding) (ctrl.Result, error) { +func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *v1.ServiceBinding, instance *v1.ServiceInstance) (ctrl.Result, error) { log := utils.GetLogger(ctx) - shouldUpdateStatus := false - if !utils.IsFailed(binding) { - 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 = "" - binding.Status.Ready = metav1.ConditionFalse - utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "recreating deleted secret", binding, false) - shouldUpdateStatus = true - r.Recorder.Event(binding, corev1.EventTypeWarning, "SecretDeleted", "SecretDeleted") - } 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 - } - } - } - } - - if shouldUpdateStatus { - log.Info(fmt.Sprintf("maintanance required for binding %s", binding.Name)) - return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, binding) + if err := r.maintainSecret(ctx, binding, instance); err != nil { + log.Error(err, "failed to maintain secret") + return r.handleSecretError(ctx, smClientTypes.UPDATE, err, binding) } + log.Info("maintain finished successfully") return ctrl.Result{}, nil } -func (r *ServiceBindingReconciler) maintainSecret(ctx context.Context, serviceBinding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance) (ctrl.Result, error) { +func (r *ServiceBindingReconciler) maintainSecret(ctx context.Context, serviceBinding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance) error { log := utils.GetLogger(ctx) - if err := r.updateSecret(ctx, serviceBinding, serviceInstance, log); err != nil { - return r.handleSecretError(ctx, smClientTypes.UPDATE, err, serviceBinding) + if common.GetObservedGeneration(serviceBinding) == serviceBinding.Generation { + log.Info("observed generation is up to date, checking if secret exists") + if _, err := r.getSecret(ctx, serviceBinding.Namespace, serviceBinding.Spec.SecretName); err == nil { + log.Info("secret exists, no need to maintain secret") + return nil + } else { + log.Info("binding's secret does not exist") + r.Recorder.Event(serviceBinding, corev1.EventTypeWarning, "SecretDeleted", "SecretDeleted") + } } - if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { - return r.handleSecretError(ctx, smClientTypes.UPDATE, err, serviceBinding) + + log.Info("maintaining binding's secret") + smClient, err := r.GetSMClient(ctx, serviceInstance) + if err != nil { + return err } - return ctrl.Result{}, nil + smBinding, err := smClient.GetBindingByID(serviceBinding.Status.BindingID, nil) + if err != nil { + log.Error(err, "failed to get binding for update secret") + return err + } + if smBinding != nil { + if smBinding.Credentials != nil { + if err = r.storeBindingSecret(ctx, serviceBinding, smBinding); err != nil { + return err + } + log.Info("Updating binding", "bindingID", smBinding.ID) + utils.SetSuccessConditions(smClientTypes.UPDATE, serviceBinding, false) + } + } + + return utils.UpdateStatus(ctx, r.Client, serviceBinding) } func (r *ServiceBindingReconciler) getServiceInstanceForBinding(ctx context.Context, binding *v1.ServiceBinding) (*v1.ServiceInstance, error) { diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 7ef8dd79..8b641851 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -4,15 +4,14 @@ import ( "context" "encoding/json" "errors" - "github.com/lithammer/dedent" - authv1 "k8s.io/api/authentication/v1" - "net/http" - "strings" - "github.com/SAP/sap-btp-service-operator/api/common" "github.com/SAP/sap-btp-service-operator/internal/utils" + "github.com/lithammer/dedent" + authv1 "k8s.io/api/authentication/v1" "k8s.io/utils/pointer" + "net/http" ctrl "sigs.k8s.io/controller-runtime" + "strings" v1 "github.com/SAP/sap-btp-service-operator/api/v1" "github.com/SAP/sap-btp-service-operator/client/sm" @@ -22,7 +21,6 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -325,20 +323,17 @@ var _ = Describe("ServiceBinding controller", func() { secretLookupKey := types.NamespacedName{Name: createdBinding.Spec.SecretName, Namespace: createdBinding.Namespace} bindingSecret := getSecret(ctx, secretLookupKey.Name, secretLookupKey.Namespace, true) originalSecretUID := bindingSecret.UID - fakeClient.ListBindingsReturns(&smClientTypes.ServiceBindings{ - ServiceBindings: []smClientTypes.ServiceBinding{ - { - ID: createdBinding.Status.BindingID, - Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}"), - LastOperation: &smClientTypes.Operation{ - Type: smClientTypes.CREATE, - State: smClientTypes.SUCCEEDED, - Description: "fake-description", - }, - }, + Expect(k8sClient.Delete(ctx, bindingSecret)).To(Succeed()) + + fakeClient.GetBindingByIDReturns(&smClientTypes.ServiceBinding{ + ID: createdBinding.Status.BindingID, + Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}"), + LastOperation: &smClientTypes.Operation{ + Type: smClientTypes.CREATE, + State: smClientTypes.SUCCEEDED, + Description: "fake-description", }, }, nil) - Expect(k8sClient.Delete(ctx, bindingSecret)).To(Succeed()) //tickle the binding createdBinding.Annotations = map[string]string{"tickle": "true"} @@ -542,14 +537,14 @@ var _ = Describe("ServiceBinding controller", func() { When("referenced service instance is failed", func() { It("should retry and succeed once the instance is ready", func() { - utils.SetFailureConditions(smClientTypes.CREATE, "Failed to create instance (test)", createdInstance, false) + createdInstance.Status.Ready = metav1.ConditionFalse updateInstanceStatus(ctx, createdInstance) binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) Expect(err).ToNot(HaveOccurred()) waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", "is not usable") - utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance, false) + createdInstance.Status.Ready = metav1.ConditionTrue updateInstanceStatus(ctx, createdInstance) waitForResourceToBeReady(ctx, binding) }) @@ -583,7 +578,7 @@ var _ = Describe("ServiceBinding controller", func() { updateInstance(ctx, createdInstance) Expect(k8sClient.Delete(ctx, createdInstance)).To(Succeed()) - createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) + createdBinding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) Expect(err).ToNot(HaveOccurred()) waitForResourceCondition(ctx, createdBinding, common.ConditionSucceeded, metav1.ConditionFalse, common.Blocked, "") Expect(utils.RemoveFinalizer(ctx, k8sClient, createdInstance, "fake/finalizer")).To(Succeed()) @@ -829,7 +824,7 @@ stringData: }) }) - When("secretTemplate is changed", func() { + When("secretTemplate is changed", func() { It("should succeed to create the secret", func() { ctx := context.Background() secretTemplate := dedent.Dedent( From 4d6c03e35dd1d4b675120521945b2b73cd98f43c Mon Sep 17 00:00:00 2001 From: I501080 Date: Tue, 17 Dec 2024 15:36:01 +0200 Subject: [PATCH 05/12] lint --- controllers/servicebinding_controller.go | 35 ++++-------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index e842b2eb..0b697046 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -147,10 +147,10 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque if initCredRotationIfRequired(serviceBinding) { log.Info("cred rotation required, updating status") return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) - } else { - log.Info("Binding in final state, maintaining secret") - return r.maintain(ctx, serviceBinding, serviceInstance) } + + log.Info("binding in final state, maintaining secret") + return r.maintain(ctx, serviceBinding, serviceInstance) } } @@ -207,29 +207,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } -func (r *ServiceBindingReconciler) updateSecret(ctx context.Context, serviceBinding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance, log logr.Logger) error { - log.Info("Updating secret according to the new template") - smClient, err := r.GetSMClient(ctx, serviceInstance) - if err != nil { - return err - } - smBinding, err := smClient.GetBindingByID(serviceBinding.Status.BindingID, nil) - if err != nil { - log.Error(err, "failed to get binding for update secret") - return err - } - if smBinding != nil { - if smBinding.Credentials != nil { - if err = r.storeBindingSecret(ctx, serviceBinding, smBinding); err != nil { - return err - } - log.Info("Updating binding", "bindingID", smBinding.ID) - utils.SetSuccessConditions(smClientTypes.UPDATE, serviceBinding, false) - } - } - return nil -} - func (r *ServiceBindingReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&v1.ServiceBinding{}). @@ -492,10 +469,10 @@ func (r *ServiceBindingReconciler) maintainSecret(ctx context.Context, serviceBi if _, err := r.getSecret(ctx, serviceBinding.Namespace, serviceBinding.Spec.SecretName); err == nil { log.Info("secret exists, no need to maintain secret") return nil - } else { - log.Info("binding's secret does not exist") - r.Recorder.Event(serviceBinding, corev1.EventTypeWarning, "SecretDeleted", "SecretDeleted") } + + log.Info("binding's secret was not found") + r.Recorder.Event(serviceBinding, corev1.EventTypeWarning, "SecretDeleted", "SecretDeleted") } log.Info("maintaining binding's secret") From 40079a92ee114b673420950840e0e6367084b1b6 Mon Sep 17 00:00:00 2001 From: I501080 Date: Tue, 17 Dec 2024 15:37:50 +0200 Subject: [PATCH 06/12] lint --- config/crd/bases/services.cloud.sap.com_servicebindings.yaml | 4 ---- .../crd/bases/services.cloud.sap.com_serviceinstances.yaml | 4 ---- controllers/servicebinding_controller_test.go | 5 +++-- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/config/crd/bases/services.cloud.sap.com_servicebindings.yaml b/config/crd/bases/services.cloud.sap.com_servicebindings.yaml index 9f6384d3..0cfb92f0 100644 --- a/config/crd/bases/services.cloud.sap.com_servicebindings.yaml +++ b/config/crd/bases/services.cloud.sap.com_servicebindings.yaml @@ -273,10 +273,6 @@ spec: description: Indicates when binding secret was rotated format: date-time type: string - observedGeneration: - description: Last generation that was acted on - format: int64 - type: integer operationType: description: The operation type (CREATE/UPDATE/DELETE) for ongoing operation diff --git a/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml b/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml index 745cfde5..e32cd757 100644 --- a/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml +++ b/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml @@ -255,10 +255,6 @@ spec: description: The generated ID of the instance, will be automatically filled once the instance is created type: string - observedGeneration: - description: Last generation that was acted on - format: int64 - type: integer operationType: description: The operation type (CREATE/UPDATE/DELETE) for ongoing operation diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 8b641851..aac6910e 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -4,14 +4,15 @@ import ( "context" "encoding/json" "errors" + "net/http" + "strings" + "github.com/SAP/sap-btp-service-operator/api/common" "github.com/SAP/sap-btp-service-operator/internal/utils" "github.com/lithammer/dedent" authv1 "k8s.io/api/authentication/v1" "k8s.io/utils/pointer" - "net/http" ctrl "sigs.k8s.io/controller-runtime" - "strings" v1 "github.com/SAP/sap-btp-service-operator/api/v1" "github.com/SAP/sap-btp-service-operator/client/sm" From 2a7fe01fa0f87a711fa9de3c20a53cfdea57acdf Mon Sep 17 00:00:00 2001 From: I501080 Date: Tue, 17 Dec 2024 15:47:13 +0200 Subject: [PATCH 07/12] . --- controllers/servicebinding_controller.go | 26 +++++++++++------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 0b697046..9dc45d41 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -135,29 +135,27 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque } } + isCredRotationInProgress := meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) + if isCredRotationInProgress { + if err := r.rotateCredentials(ctx, serviceBinding, serviceInstance); err != nil { + return ctrl.Result{}, err + } + } + readyCond := meta.FindStatusCondition(serviceBinding.Status.Conditions, common.ConditionReady) isBindingReady := readyCond != nil && readyCond.Status == metav1.ConditionTrue - isCredRotationInProgress := meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) if isBindingReady { if isStaleServiceBinding(serviceBinding) { return r.handleStaleServiceBinding(ctx, serviceBinding) } - if !isCredRotationInProgress { - if initCredRotationIfRequired(serviceBinding) { - log.Info("cred rotation required, updating status") - return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) - } - - log.Info("binding in final state, maintaining secret") - return r.maintain(ctx, serviceBinding, serviceInstance) + if initCredRotationIfRequired(serviceBinding) { + log.Info("cred rotation required, updating status") + return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) } - } - if isCredRotationInProgress { - if err := r.rotateCredentials(ctx, serviceBinding, serviceInstance); err != nil { - return ctrl.Result{}, err - } + log.Info("binding in final state, maintaining secret") + return r.maintain(ctx, serviceBinding, serviceInstance) } if !serviceInstanceUsable(serviceInstance) { From 51feeb92479d9e6c09e63af9a10c92b998bc3fb0 Mon Sep 17 00:00:00 2001 From: I501080 Date: Tue, 17 Dec 2024 16:35:33 +0200 Subject: [PATCH 08/12] logs --- controllers/servicebinding_controller.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 9dc45d41..78b38505 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -123,11 +123,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return r.delete(ctx, serviceBinding, serviceInstance) } - if len(serviceBinding.Status.OperationURL) > 0 { - // ongoing operation - poll status from SM - return r.poll(ctx, serviceBinding, serviceInstance) - } - if controllerutil.AddFinalizer(serviceBinding, common.FinalizerName) { log.Info(fmt.Sprintf("added finalizer '%s' to service binding", common.FinalizerName)) if err := r.Client.Update(ctx, serviceBinding); err != nil { @@ -135,8 +130,13 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque } } - isCredRotationInProgress := meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) - if isCredRotationInProgress { + if len(serviceBinding.Status.OperationURL) > 0 { + // ongoing operation - poll status from SM + return r.poll(ctx, serviceBinding, serviceInstance) + } + + if meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) { + log.Info("rotating credentials") if err := r.rotateCredentials(ctx, serviceBinding, serviceInstance); err != nil { return ctrl.Result{}, err } @@ -146,6 +146,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque isBindingReady := readyCond != nil && readyCond.Status == metav1.ConditionTrue if isBindingReady { if isStaleServiceBinding(serviceBinding) { + log.Info("binding is stale, handling") return r.handleStaleServiceBinding(ctx, serviceBinding) } @@ -180,6 +181,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque if serviceBinding.Status.BindingID == "" { if err := r.validateSecretNameIsAvailable(ctx, serviceBinding); err != nil { + log.Error(err, "secret validation failed") utils.SetBlockedCondition(ctx, err.Error(), serviceBinding) return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) } From c0d6231cc6f8be834c6a76274222e95a9fec2968 Mon Sep 17 00:00:00 2001 From: I501080 Date: Thu, 19 Dec 2024 13:26:52 +0200 Subject: [PATCH 09/12] review --- controllers/servicebinding_controller.go | 26 ++++++++++++------------ 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 78b38505..cd14acb0 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -135,6 +135,16 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return r.poll(ctx, serviceBinding, serviceInstance) } + if !serviceInstanceUsable(serviceInstance) { + instanceErr := fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName) + utils.SetBlockedCondition(ctx, instanceErr.Error(), serviceBinding) + if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, instanceErr + } + + // should rotate creds if meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionCredRotationInProgress) { log.Info("rotating credentials") if err := r.rotateCredentials(ctx, serviceBinding, serviceInstance); err != nil { @@ -142,9 +152,8 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque } } - readyCond := meta.FindStatusCondition(serviceBinding.Status.Conditions, common.ConditionReady) - isBindingReady := readyCond != nil && readyCond.Status == metav1.ConditionTrue - if isBindingReady { + // is binding ready + if meta.IsStatusConditionTrue(serviceBinding.Status.Conditions, common.ConditionReady) { if isStaleServiceBinding(serviceBinding) { log.Info("binding is stale, handling") return r.handleStaleServiceBinding(ctx, serviceBinding) @@ -159,15 +168,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return r.maintain(ctx, serviceBinding, serviceInstance) } - if !serviceInstanceUsable(serviceInstance) { - instanceErr := fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName) - utils.SetBlockedCondition(ctx, instanceErr.Error(), serviceBinding) - if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, instanceErr - } - //set owner instance only for original bindings (not rotated) if serviceBinding.Labels == nil || len(serviceBinding.Labels[common.StaleBindingIDLabel]) == 0 { if !bindingAlreadyOwnedByInstance(serviceInstance, serviceBinding) && @@ -885,7 +885,6 @@ func (r *ServiceBindingReconciler) addInstanceInfo(ctx context.Context, binding } func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, binding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance) error { - suffix := "-" + utils.RandStringRunes(6) log := utils.GetLogger(ctx) if binding.Annotations != nil { if _, ok := binding.Annotations[common.ForceRotateAnnotation]; ok { @@ -931,6 +930,7 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, bindin } // rename current binding + suffix := "-" + utils.RandStringRunes(6) log.Info("Credentials rotation - renaming binding to old in SM", "current", binding.Spec.ExternalName) if _, errRenaming := smClient.RenameBinding(binding.Status.BindingID, binding.Spec.ExternalName+suffix, binding.Name+suffix); errRenaming != nil { log.Error(errRenaming, "Credentials rotation - failed renaming binding to old in SM", "binding", binding.Spec.ExternalName) From 7436aeddb2c27c7714e96a08c5b1bcac06b93ae6 Mon Sep 17 00:00:00 2001 From: I501080 Date: Thu, 19 Dec 2024 15:33:33 +0200 Subject: [PATCH 10/12] review --- controllers/servicebinding_controller.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index cd14acb0..8246219e 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -135,13 +135,16 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return r.poll(ctx, serviceBinding, serviceInstance) } - if !serviceInstanceUsable(serviceInstance) { - instanceErr := fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName) - utils.SetBlockedCondition(ctx, instanceErr.Error(), serviceBinding) - if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, instanceErr + if utils.IsMarkedForDeletion(serviceInstance.ObjectMeta) { + log.Info(fmt.Sprintf("service instance name: %s namespace: %s is marked for deletion, unable to create binding", serviceInstance.Name, serviceInstance.Namespace)) + utils.SetBlockedCondition(ctx, "instance is in deletion process", serviceBinding) + return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) + } + + if !serviceInstanceReady(serviceInstance) { + log.Info(fmt.Sprintf("service instance name: %s namespace: %s is not ready, unable to create binding", serviceInstance.Name, serviceInstance.Namespace)) + utils.SetBlockedCondition(ctx, "service instance is not ready", serviceBinding) + return ctrl.Result{Requeue: true}, utils.UpdateStatus(ctx, r.Client, serviceBinding) } // should rotate creds @@ -418,6 +421,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1. } } + log.Info(fmt.Sprintf("finished polling operation %s '%s'", serviceBinding.Status.OperationType, serviceBinding.Status.OperationURL)) serviceBinding.Status.OperationURL = "" serviceBinding.Status.OperationType = "" @@ -1122,10 +1126,7 @@ func bindingAlreadyOwnedByInstance(instance *v1.ServiceInstance, binding *v1.Ser return false } -func serviceInstanceUsable(instance *v1.ServiceInstance) bool { - if utils.IsMarkedForDeletion(instance.ObjectMeta) { - return false - } +func serviceInstanceReady(instance *v1.ServiceInstance) bool { return instance.Status.Ready == metav1.ConditionTrue } From b6f86d88ad2bb9cb25579aeb71f7e0b76f06a4f3 Mon Sep 17 00:00:00 2001 From: I501080 Date: Thu, 19 Dec 2024 16:12:38 +0200 Subject: [PATCH 11/12] fix test --- controllers/servicebinding_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index aac6910e..a7c67d3c 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -543,7 +543,7 @@ var _ = Describe("ServiceBinding controller", func() { binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) Expect(err).ToNot(HaveOccurred()) - waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", "is not usable") + waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", "service instance is not ready") createdInstance.Status.Ready = metav1.ConditionTrue updateInstanceStatus(ctx, createdInstance) From 158d27781b6cf5224a386fa037eec1866b51957f Mon Sep 17 00:00:00 2001 From: I501080 Date: Thu, 19 Dec 2024 16:20:55 +0200 Subject: [PATCH 12/12] . --- controllers/servicebinding_controller.go | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 8246219e..10c8d6fa 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -890,15 +890,9 @@ func (r *ServiceBindingReconciler) addInstanceInfo(ctx context.Context, binding func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, binding *v1.ServiceBinding, serviceInstance *v1.ServiceInstance) error { log := utils.GetLogger(ctx) - if binding.Annotations != nil { - if _, ok := binding.Annotations[common.ForceRotateAnnotation]; ok { - log.Info("Credentials rotation - deleting force rotate annotation") - delete(binding.Annotations, common.ForceRotateAnnotation) - if err := r.Client.Update(ctx, binding); err != nil { - log.Info("Credentials rotation - failed to delete force rotate annotation") - return err - } - } + if err := r.removeForceRotateAnnotationIfNeeded(ctx, binding, log); err != nil { + log.Info("Credentials rotation - failed to delete force rotate annotation") + return err } credInProgressCondition := meta.FindStatusCondition(binding.GetConditions(), common.ConditionCredRotationInProgress) @@ -928,6 +922,7 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, bindin } if len(bindings.Items) == 0 { + // create the backup binding smClient, err := r.GetSMClient(ctx, serviceInstance) if err != nil { return err @@ -964,6 +959,17 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, bindin return utils.UpdateStatus(ctx, r.Client, binding) } +func (r *ServiceBindingReconciler) removeForceRotateAnnotationIfNeeded(ctx context.Context, binding *v1.ServiceBinding, log logr.Logger) error { + if binding.Annotations != nil { + if _, ok := binding.Annotations[common.ForceRotateAnnotation]; ok { + log.Info("Credentials rotation - deleting force rotate annotation") + delete(binding.Annotations, common.ForceRotateAnnotation) + return r.Client.Update(ctx, binding) + } + } + return nil +} + func (r *ServiceBindingReconciler) stopRotation(ctx context.Context, binding *v1.ServiceBinding) error { conditions := binding.GetConditions() meta.RemoveStatusCondition(&conditions, common.ConditionCredRotationInProgress)