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.go b/controllers/servicebinding_controller.go index 1afad4a8..10c8d6fa 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -123,67 +123,52 @@ 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.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) - } + if len(serviceBinding.Status.OperationURL) > 0 { + // ongoing operation - poll status from SM + return r.poll(ctx, serviceBinding, serviceInstance) } - isBindingReady := condition != nil && condition.Status == metav1.ConditionTrue - if isBindingReady { - if isStaleServiceBinding(serviceBinding) { - return r.handleStaleServiceBinding(ctx, serviceBinding) - } + 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 initCredRotationIfRequired(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 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 } } - if isBindingReady { - log.Info("Binding in final state") - return r.maintain(ctx, serviceBinding) - } - - if serviceNotUsable(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 + // 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) } - 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, false) + if initCredRotationIfRequired(serviceBinding) { + log.Info("cred rotation required, updating status") + return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) + } - return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, utils.UpdateStatus(ctx, r.Client, serviceBinding) + log.Info("binding in final state, maintaining secret") + return r.maintain(ctx, serviceBinding, serviceInstance) } //set owner instance only for original bindings (not rotated) @@ -199,6 +184,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) } @@ -224,29 +210,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{}). @@ -458,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 = "" @@ -491,43 +455,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 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) error { + log := utils.GetLogger(ctx) + 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 } + + log.Info("binding's secret was not found") + r.Recorder.Event(serviceBinding, corev1.EventTypeWarning, "SecretDeleted", "SecretDeleted") } - if shouldUpdateStatus { - log.Info(fmt.Sprintf("maintanance required for binding %s", binding.Name)) - return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, binding) + log.Info("maintaining binding's secret") + 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 ctrl.Result{}, nil + return utils.UpdateStatus(ctx, r.Client, serviceBinding) } func (r *ServiceBindingReconciler) getServiceInstanceForBinding(ctx context.Context, binding *v1.ServiceBinding) (*v1.ServiceInstance, error) { @@ -917,17 +889,10 @@ 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 { - 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) @@ -957,12 +922,14 @@ 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 } // 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) @@ -992,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) @@ -1089,7 +1067,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] @@ -1154,14 +1132,8 @@ func bindingAlreadyOwnedByInstance(instance *v1.ServiceInstance, binding *v1.Ser return false } -func serviceNotUsable(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 +func serviceInstanceReady(instance *v1.ServiceInstance) bool { + 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 c7003100..a7c67d3c 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -4,13 +4,13 @@ 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" ctrl "sigs.k8s.io/controller-runtime" @@ -324,20 +324,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"} @@ -541,14 +538,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") + waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", "service instance is not ready") - utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance, false) + createdInstance.Status.Ready = metav1.ConditionTrue updateInstanceStatus(ctx, createdInstance) waitForResourceToBeReady(ctx, binding) }) @@ -556,6 +553,7 @@ var _ = Describe("ServiceBinding controller", func() { When("referenced service instance is not ready", func() { It("should retry and succeed once the instance is ready", func() { + createdInstance.Status.Ready = metav1.ConditionFalse fakeClient.StatusReturns(&smClientTypes.Operation{ResourceID: fakeInstanceID, State: smClientTypes.INPROGRESS}, nil) utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "", createdInstance, false) createdInstance.Status.OperationURL = "/1234" @@ -564,8 +562,9 @@ var _ = Describe("ServiceBinding controller", func() { createdBinding, err := createBindingWithoutAssertions(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, "") + createdInstance.Status.Ready = metav1.ConditionTrue utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance, false) createdInstance.Status.OperationType = "" createdInstance.Status.OperationURL = "" @@ -574,6 +573,19 @@ var _ = Describe("ServiceBinding controller", func() { }) }) + 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 := 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()) + }) + }) + When("secretTemplate", func() { It("should succeed to create the secret", func() { ctx := context.Background() @@ -813,7 +825,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( diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 13be1eae..85623a91 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -104,8 +104,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