diff --git a/api/common/common.go b/api/common/common.go index cec801e3..a0ecf534 100644 --- a/api/common/common.go +++ b/api/common/common.go @@ -3,6 +3,8 @@ package common import ( "fmt" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -74,11 +76,18 @@ type SAPBTPResource interface { GetParameters() *runtime.RawExtension GetStatus() interface{} SetStatus(status interface{}) - GetObservedGeneration() int64 - SetObservedGeneration(int64) DeepClone() SAPBTPResource SetReady(metav1.ConditionStatus) GetReady() metav1.ConditionStatus GetAnnotations() map[string]string SetAnnotations(map[string]string) } + +func GetObservedGeneration(obj SAPBTPResource) int64 { + cond := meta.FindStatusCondition(obj.GetConditions(), ConditionSucceeded) + observedGen := int64(0) + if cond != nil { + observedGen = cond.ObservedGeneration + } + return observedGen +} diff --git a/api/v1/servicebinding_types.go b/api/v1/servicebinding_types.go index db432556..01296125 100644 --- a/api/v1/servicebinding_types.go +++ b/api/v1/servicebinding_types.go @@ -123,9 +123,6 @@ type ServiceBindingStatus struct { // Service binding conditions Conditions []metav1.Condition `json:"conditions"` - // Last generation that was acted on - ObservedGeneration int64 `json:"observedGeneration,omitempty"` - // Indicates whether binding is ready for usage Ready metav1.ConditionStatus `json:"ready,omitempty"` @@ -179,14 +176,6 @@ func (sb *ServiceBinding) SetStatus(status interface{}) { sb.Status = status.(ServiceBindingStatus) } -func (sb *ServiceBinding) GetObservedGeneration() int64 { - return sb.Status.ObservedGeneration -} - -func (sb *ServiceBinding) SetObservedGeneration(newObserved int64) { - sb.Status.ObservedGeneration = newObserved -} - func (sb *ServiceBinding) DeepClone() common.SAPBTPResource { return sb.DeepCopy() } diff --git a/api/v1/servicebinding_types_test.go b/api/v1/servicebinding_types_test.go index c0a91b2f..c0e713f9 100644 --- a/api/v1/servicebinding_types_test.go +++ b/api/v1/servicebinding_types_test.go @@ -66,18 +66,6 @@ var _ = Describe("Service Binding Type Test", func() { Expect(binding.GetControllerName()).To(Equal(common.ServiceBindingController)) }) - It("should update observed generation", func() { - Expect(binding.Status.ObservedGeneration).To(Equal(int64(0))) - binding.SetObservedGeneration(2) - Expect(binding.GetObservedGeneration()).To(Equal(int64(2))) - }) - - It("should update observed generation", func() { - Expect(binding.Status.ObservedGeneration).To(Equal(int64(0))) - binding.SetObservedGeneration(2) - Expect(binding.Status.ObservedGeneration).To(Equal(int64(2))) - }) - It("should update ready", func() { Expect(binding.Status.Ready).To(Equal(metav1.ConditionStatus(""))) binding.SetReady(metav1.ConditionTrue) diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index e6e967ff..4ed0b597 100644 --- a/api/v1/serviceinstance_types.go +++ b/api/v1/serviceinstance_types.go @@ -17,11 +17,16 @@ limitations under the License. package v1 import ( + "crypto/md5" + "encoding/hex" + "encoding/json" + "github.com/SAP/sap-btp-service-operator/api/common" "github.com/SAP/sap-btp-service-operator/client/sm/types" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -107,9 +112,6 @@ type ServiceInstanceStatus struct { // Service instance conditions Conditions []metav1.Condition `json:"conditions"` - // Last generation that was acted on - ObservedGeneration int64 `json:"observedGeneration,omitempty"` - // Indicates whether instance is ready for usage Ready metav1.ConditionStatus `json:"ready,omitempty"` @@ -165,14 +167,6 @@ func (si *ServiceInstance) SetStatus(status interface{}) { si.Status = status.(ServiceInstanceStatus) } -func (si *ServiceInstance) GetObservedGeneration() int64 { - return si.Status.ObservedGeneration -} - -func (si *ServiceInstance) SetObservedGeneration(newObserved int64) { - si.Status.ObservedGeneration = newObserved -} - func (si *ServiceInstance) DeepClone() common.SAPBTPResource { return si.DeepCopy() } @@ -207,6 +201,15 @@ func init() { func (si *ServiceInstance) Hub() {} -func (si *ServiceInstance) ShouldBeShared() bool { +func (si *ServiceInstance) GetShared() bool { return si.Spec.Shared != nil && *si.Spec.Shared } + +func (si *ServiceInstance) GetSpecHash() string { + spec := si.Spec + spec.Shared = ptr.To(false) + specBytes, _ := json.Marshal(spec) + s := string(specBytes) + hash := md5.Sum([]byte(s)) + return hex.EncodeToString(hash[:]) +} diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index 35fa441c..a63ddf1c 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -96,12 +96,6 @@ var _ = Describe("Service Instance Type Test", func() { Expect(instance.GetControllerName()).To(Equal(common.ServiceInstanceController)) }) - It("should update observed generation", func() { - Expect(instance.Status.ObservedGeneration).To(Equal(int64(0))) - instance.SetObservedGeneration(2) - Expect(instance.GetObservedGeneration()).To(Equal(int64(2))) - }) - It("should update ready", func() { Expect(instance.Status.Ready).To(Equal(metav1.ConditionStatus(""))) instance.SetReady(metav1.ConditionTrue) diff --git a/api/v1/webhooks/serviceinstance_mutating_webhook.go b/api/v1/webhooks/serviceinstance_mutating_webhook.go index b66951a1..5b1cad08 100644 --- a/api/v1/webhooks/serviceinstance_mutating_webhook.go +++ b/api/v1/webhooks/serviceinstance_mutating_webhook.go @@ -44,9 +44,12 @@ func (s *ServiceInstanceDefaulter) Handle(_ context.Context, req admission.Reque return admission.Errored(http.StatusInternalServerError, err) } - if !reflect.DeepEqual(instance.Spec.UserInfo, oldInstance.Spec.UserInfo) { + if instance.Spec.UserInfo == nil { + instance.Spec.UserInfo = oldInstance.Spec.UserInfo + } else if !reflect.DeepEqual(instance.Spec.UserInfo, oldInstance.Spec.UserInfo) { return admission.Errored(http.StatusBadRequest, fmt.Errorf("modifying spec.userInfo is not allowed")) - } else if !reflect.DeepEqual(oldInstance.Spec, instance.Spec) { //UserInfo is updated only when spec is changed + } + if !reflect.DeepEqual(oldInstance.Spec, instance.Spec) { //UserInfo is updated only when spec is changed instance.Spec.UserInfo = &v1.UserInfo{ Username: req.UserInfo.Username, UID: req.UserInfo.UID, diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index fef2f309..1afad4a8 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -91,8 +91,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, client.IgnoreNotFound(err) } serviceBinding = serviceBinding.DeepCopy() - log.Info(fmt.Sprintf("Current generation is %v and observed is %v", serviceBinding.Generation, serviceBinding.GetObservedGeneration())) - serviceBinding.SetObservedGeneration(serviceBinding.Generation) + log.Info(fmt.Sprintf("Current generation is %v and observed is %v", serviceBinding.Generation, common.GetObservedGeneration(serviceBinding))) if len(serviceBinding.GetConditions()) == 0 { if err := utils.InitConditions(ctx, r.Client, serviceBinding); err != nil { @@ -182,7 +181,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque 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) + utils.SetInProgressConditions(ctx, smClientTypes.CREATE, fmt.Sprintf("creation in progress, waiting for service instance '%s' to be ready", serviceBinding.Spec.ServiceInstanceName), serviceBinding, false) return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, utils.UpdateStatus(ctx, r.Client, serviceBinding) } @@ -242,7 +241,7 @@ func (r *ServiceBindingReconciler) updateSecret(ctx context.Context, serviceBind return err } log.Info("Updating binding", "bindingID", smBinding.ID) - utils.SetSuccessConditions(smClientTypes.UPDATE, serviceBinding) + utils.SetSuccessConditions(smClientTypes.UPDATE, serviceBinding, false) } } return nil @@ -291,7 +290,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s log.Info("Create smBinding request is async") serviceBinding.Status.OperationURL = operationURL serviceBinding.Status.OperationType = smClientTypes.CREATE - utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "", serviceBinding) + utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "", serviceBinding, false) if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { log.Error(err, "unable to update ServiceBinding status") return ctrl.Result{}, err @@ -313,7 +312,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s serviceBinding.Status.BindingID = smBinding.ID serviceBinding.Status.SubaccountID = subaccountID serviceBinding.Status.Ready = metav1.ConditionTrue - utils.SetSuccessConditions(smClientTypes.CREATE, serviceBinding) + utils.SetSuccessConditions(smClientTypes.CREATE, serviceBinding, false) log.Info("Updating binding", "bindingID", smBinding.ID) return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) @@ -336,7 +335,7 @@ func (r *ServiceBindingReconciler) delete(ctx context.Context, serviceBinding *v if smBinding != nil { log.Info("binding exists in SM continue with deletion") serviceBinding.Status.BindingID = smBinding.ID - utils.SetInProgressConditions(ctx, smClientTypes.DELETE, "delete after recovery", serviceBinding) + utils.SetInProgressConditions(ctx, smClientTypes.DELETE, "delete after recovery", serviceBinding, false) return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceBinding) } @@ -367,7 +366,7 @@ func (r *ServiceBindingReconciler) delete(ctx context.Context, serviceBinding *v log.Info("Deleting binding async") serviceBinding.Status.OperationURL = operationURL serviceBinding.Status.OperationType = smClientTypes.DELETE - utils.SetInProgressConditions(ctx, smClientTypes.DELETE, "", serviceBinding) + utils.SetInProgressConditions(ctx, smClientTypes.DELETE, "", serviceBinding, false) if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { return ctrl.Result{}, err } @@ -392,7 +391,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1. status, statusErr := smClient.Status(serviceBinding.Status.OperationURL, nil) if statusErr != nil { log.Info(fmt.Sprintf("failed to fetch operation, got error from SM: %s", statusErr.Error()), "operationURL", serviceBinding.Status.OperationURL) - utils.SetInProgressConditions(ctx, serviceBinding.Status.OperationType, string(smClientTypes.INPROGRESS), serviceBinding) + utils.SetInProgressConditions(ctx, serviceBinding.Status.OperationType, string(smClientTypes.INPROGRESS), serviceBinding, false) freshStatus := v1.ServiceBindingStatus{ Conditions: serviceBinding.GetConditions(), } @@ -414,7 +413,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1. fallthrough case smClientTypes.PENDING: if len(status.Description) != 0 { - utils.SetInProgressConditions(ctx, status.Type, status.Description, serviceBinding) + utils.SetInProgressConditions(ctx, status.Type, status.Description, serviceBinding, true) if err := utils.UpdateStatus(ctx, r.Client, serviceBinding); err != nil { log.Error(err, "unable to update ServiceBinding polling description") return ctrl.Result{}, err @@ -423,7 +422,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1. return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil case smClientTypes.FAILED: // non transient error - should not retry - utils.SetFailureConditions(status.Type, status.Description, serviceBinding) + utils.SetFailureConditions(status.Type, status.Description, serviceBinding, true) if serviceBinding.Status.OperationType == smClientTypes.DELETE { serviceBinding.Status.OperationURL = "" serviceBinding.Status.OperationType = "" @@ -438,7 +437,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1. return ctrl.Result{}, fmt.Errorf(errMsg) } case smClientTypes.SUCCEEDED: - utils.SetSuccessConditions(status.Type, serviceBinding) + utils.SetSuccessConditions(status.Type, serviceBinding, true) switch serviceBinding.Status.OperationType { case smClientTypes.CREATE: smBinding, err := smClient.GetBindingByID(serviceBinding.Status.BindingID, nil) @@ -453,7 +452,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *v1. if err := r.storeBindingSecret(ctx, serviceBinding, smBinding); err != nil { return r.handleSecretError(ctx, smClientTypes.CREATE, err, serviceBinding) } - utils.SetSuccessConditions(status.Type, serviceBinding) + utils.SetSuccessConditions(status.Type, serviceBinding, false) case smClientTypes.DELETE: return r.deleteSecretAndRemoveFinalizer(ctx, serviceBinding) } @@ -495,10 +494,6 @@ func (r *ServiceBindingReconciler) getBindingForRecovery(ctx context.Context, sm func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *v1.ServiceBinding) (ctrl.Result, error) { log := utils.GetLogger(ctx) shouldUpdateStatus := false - if binding.Generation != binding.Status.ObservedGeneration { - binding.SetObservedGeneration(binding.Generation) - shouldUpdateStatus = true - } if !utils.IsFailed(binding) { secret, err := r.getSecret(ctx, binding.Namespace, binding.Spec.SecretName) if err != nil { @@ -507,7 +502,7 @@ func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *v1.Ser 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) + utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "recreating deleted secret", binding, false) shouldUpdateStatus = true r.Recorder.Event(binding, corev1.EventTypeWarning, "SecretDeleted", "SecretDeleted") } else { @@ -565,7 +560,6 @@ func (r *ServiceBindingReconciler) setOwner(ctx context.Context, serviceInstance } func (r *ServiceBindingReconciler) resyncBindingStatus(ctx context.Context, k8sBinding *v1.ServiceBinding, smBinding *smClientTypes.ServiceBinding) { - k8sBinding.Status.ObservedGeneration = k8sBinding.Generation k8sBinding.Status.BindingID = smBinding.ID k8sBinding.Status.InstanceID = smBinding.ServiceInstanceID k8sBinding.Status.OperationURL = "" @@ -587,11 +581,11 @@ func (r *ServiceBindingReconciler) resyncBindingStatus(ctx context.Context, k8sB case smClientTypes.INPROGRESS: k8sBinding.Status.OperationURL = sm.BuildOperationURL(smBinding.LastOperation.ID, smBinding.ID, smClientTypes.ServiceBindingsURL) k8sBinding.Status.OperationType = smBinding.LastOperation.Type - utils.SetInProgressConditions(ctx, smBinding.LastOperation.Type, smBinding.LastOperation.Description, k8sBinding) + utils.SetInProgressConditions(ctx, smBinding.LastOperation.Type, smBinding.LastOperation.Description, k8sBinding, false) case smClientTypes.SUCCEEDED: - utils.SetSuccessConditions(operationType, k8sBinding) + utils.SetSuccessConditions(operationType, k8sBinding, false) case smClientTypes.FAILED: - utils.SetFailureConditions(operationType, description, k8sBinding) + utils.SetFailureConditions(operationType, description, k8sBinding, false) } } @@ -621,6 +615,11 @@ func (r *ServiceBindingReconciler) storeBindingSecret(ctx context.Context, k8sBi } secret.Labels[common.ManagedByBTPOperatorLabel] = "true" + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + secret.Annotations["binding"] = k8sBinding.Name + return r.createOrUpdateBindingSecret(ctx, k8sBinding, secret) } @@ -988,7 +987,7 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, bindin binding.Status.BindingID = "" binding.Status.Ready = metav1.ConditionFalse - utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "rotating binding credentials", binding) + utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "rotating binding credentials", binding, false) utils.SetCredRotationInProgressConditions(common.CredRotating, "", binding) return utils.UpdateStatus(ctx, r.Client, binding) } diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 1d1033e9..c7003100 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -22,7 +22,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" @@ -36,8 +35,6 @@ import ( var _ = Describe("ServiceBinding controller", func() { var ( - ctx context.Context - createdInstance *v1.ServiceInstance createdBinding *v1.ServiceBinding @@ -47,9 +44,10 @@ var _ = Describe("ServiceBinding controller", func() { bindingName string instanceName string instanceExternalName string + paramsSecret *corev1.Secret ) - createBindingWithoutAssertionsAndWait := func(ctx context.Context, name, namespace, instanceName, instanceNamespace, externalName string, secretTemplate string, wait bool) (*v1.ServiceBinding, error) { + createBindingWithoutAssertions := func(ctx context.Context, name, namespace, instanceName, instanceNamespace, externalName string, secretTemplate string, wait bool) (*v1.ServiceBinding, error) { binding := generateBasicBindingTemplate(name, namespace, instanceName, instanceNamespace, externalName, secretTemplate) if err := k8sClient.Create(ctx, binding); err != nil { return nil, err @@ -72,45 +70,13 @@ var _ = Describe("ServiceBinding controller", func() { return createdBinding, nil } - createBindingWithoutAssertions := func(ctx context.Context, name, namespace, instanceName, instanceNamespace, externalName string, secretTemplate string) (*v1.ServiceBinding, error) { - return createBindingWithoutAssertionsAndWait(ctx, name, namespace, instanceName, instanceNamespace, externalName, secretTemplate, false) - } - - createBindingWithTransitError := func(ctx context.Context, name, namespace, instanceName, externalName, failureMessage string, secretTemplate string) { - binding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, "", externalName, secretTemplate) - if err != nil { - Expect(err.Error()).To(ContainSubstring(failureMessage)) - } else { - waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, failureMessage) - } - } - - createBindingWithError := func(ctx context.Context, name, namespace, instanceName, externalName, failureMessage string, secretTemplate string) { - binding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, "", externalName, secretTemplate) - if err != nil { - Expect(err.Error()).To(ContainSubstring(failureMessage)) - } else { - waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", failureMessage) - } - } - - createBindingWithBlockedError := func(ctx context.Context, name, namespace, instanceName, externalName, failureMessage string) *v1.ServiceBinding { - binding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, "", externalName, "") - if err != nil { - Expect(err.Error()).To(ContainSubstring(failureMessage)) - } else { - waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", failureMessage) - } - return binding - } - - createBinding := func(ctx context.Context, name, namespace, instanceName, instanceNamespace, externalName string, secretTemplate string) *v1.ServiceBinding { - createdBinding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, instanceNamespace, externalName, secretTemplate) + createAndValidateBinding := func(ctx context.Context, name, namespace, instanceName, instanceNamespace, externalName string, secretTemplate string) *v1.ServiceBinding { + createdBinding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, instanceNamespace, externalName, secretTemplate, false) Expect(err).ToNot(HaveOccurred()) Expect(createdBinding.Status.InstanceID).ToNot(BeEmpty()) Expect(createdBinding.Status.BindingID).To(Equal(fakeBindingID)) Expect(createdBinding.Spec.SecretName).To(Not(BeEmpty())) - Expect(int(createdBinding.Status.ObservedGeneration)).To(Equal(1)) + Expect(common.GetObservedGeneration(createdBinding)).To(Equal(int64(1))) Expect(string(createdBinding.Spec.Parameters.Raw)).To(ContainSubstring("\"key\":\"value\"")) smBinding, _, _ := fakeClient.BindArgsForCall(0) params := smBinding.Parameters @@ -182,26 +148,24 @@ var _ = Describe("ServiceBinding controller", func() { fakeClient.GetInstanceByIDReturns(smInstance, nil) defaultLookupKey = types.NamespacedName{Namespace: bindingTestNamespace, Name: bindingName} - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: bindingTestNamespace, Name: "param-secret"}, &corev1.Secret{}) - if apierrors.IsNotFound(err) { - createParamsSecret(bindingTestNamespace) - } else { - Expect(err).ToNot(HaveOccurred()) - } - createdInstance = createInstance(ctx, instanceName, bindingTestNamespace, instanceExternalName) + paramsSecret = createParamsSecret(ctx, "binding-params-secret", bindingTestNamespace) + }) AfterEach(func() { if createdBinding != nil { fakeClient.UnbindReturns("", nil) - deleteAndWait(ctx, types.NamespacedName{Name: createdBinding.Name, Namespace: createdBinding.Namespace}, &v1.ServiceBinding{}) + deleteAndWait(ctx, createdBinding) } if createdInstance != nil { - deleteAndWait(ctx, types.NamespacedName{Name: instanceName, Namespace: bindingTestNamespace}, &v1.ServiceInstance{}) + fakeClient.DeprovisionReturns("", nil) + deleteAndWait(ctx, createdInstance) } + deleteAndWait(ctx, paramsSecret) + createdBinding = nil createdInstance = nil }) @@ -210,15 +174,17 @@ var _ = Describe("ServiceBinding controller", func() { Context("invalid parameters", func() { When("service instance name is not provided", func() { It("should fail", func() { - createBindingWithTransitError(ctx, bindingName, bindingTestNamespace, "", "", - "spec.serviceInstanceName in body should be at least 1 chars long", "") + _, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, "", "", "", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("spec.serviceInstanceName in body should be at least 1 chars long")) }) }) When("referenced service instance does not exist", func() { It("should fail", func() { - createBindingWithBlockedError(ctx, bindingName, bindingTestNamespace, "no-such-instance", "", - "couldn't find the service instance") + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, "no-such-instance", "", "", "", false) + Expect(err).ToNot(HaveOccurred()) + waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, "", "couldn't find the service instance") }) }) @@ -235,7 +201,7 @@ var _ = Describe("ServiceBinding controller", func() { }) AfterEach(func() { - deleteAndWait(ctx, types.NamespacedName{Name: secretName, Namespace: bindingTestNamespace}, &corev1.Secret{}) + deleteAndWait(ctx, secret) }) When("name is already taken", func() { @@ -284,7 +250,7 @@ var _ = Describe("ServiceBinding controller", func() { Context("sync", func() { It("Should create binding and store the binding credentials in a secret", func() { - createdBinding = createBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "") + createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "") Expect(createdBinding.Spec.ExternalName).To(Equal("binding-external-name")) Expect(createdBinding.Spec.UserInfo).NotTo(BeNil()) @@ -354,7 +320,7 @@ var _ = Describe("ServiceBinding controller", func() { When("secret deleted by user", func() { It("should recreate the secret", func() { - createdBinding = createBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "") + createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "") secretLookupKey := types.NamespacedName{Name: createdBinding.Spec.SecretName, Namespace: createdBinding.Namespace} bindingSecret := getSecret(ctx, secretLookupKey.Name, secretLookupKey.Namespace, true) originalSecretUID := bindingSecret.UID @@ -395,7 +361,9 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should fail with the error returned from SM", func() { - createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage, "") + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) + Expect(err).ToNot(HaveOccurred()) + waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", errorMessage) }) }) @@ -410,7 +378,7 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should eventually succeed", func() { - b, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", true) + b, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", true) Expect(err).ToNot(HaveOccurred()) Expect(isResourceReady(b)).To(BeTrue()) }) @@ -426,7 +394,9 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should fail", func() { - createBindingWithTransitError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage, "") + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) + Expect(err).ToNot(HaveOccurred()) + waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, errorMessage) }) }) @@ -437,7 +407,7 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should detect the error as transient and eventually succeed", func() { - createdBinding, _ := createBindingWithoutAssertionsAndWait(ctx, + createdBinding, _ := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, @@ -464,7 +434,9 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should detect the error as non-transient and fail", func() { - createBindingWithTransitError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage, "") + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "", false) + Expect(err).ToNot(HaveOccurred()) + waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, errorMessage) }) }) @@ -476,7 +448,7 @@ var _ = Describe("ServiceBinding controller", func() { }) It("creation will fail with appropriate message", func() { - createdBinding, _ = createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "") + createdBinding, _ = createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "", false) waitForResourceCondition(ctx, createdBinding, common.ConditionFailed, metav1.ConditionTrue, "CreateFailed", "failed to create secret") }) }) @@ -490,7 +462,7 @@ var _ = Describe("ServiceBinding controller", func() { When("bind polling returns success", func() { It("Should create binding and store the binding credentials in a secret", func() { fakeClient.StatusReturns(&smClientTypes.Operation{ResourceID: fakeBindingID, State: smClientTypes.SUCCEEDED}, nil) - createdBinding = createBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "") + createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "") }) }) @@ -502,7 +474,10 @@ var _ = Describe("ServiceBinding controller", func() { State: smClientTypes.FAILED, Description: errorMessage, }, nil) - createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "existing-name", errorMessage, "") + + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "existing-name", "", false) + Expect(err).ToNot(HaveOccurred()) + waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", errorMessage) }) }) @@ -513,7 +488,7 @@ var _ = Describe("ServiceBinding controller", func() { fakeClient.GetBindingByIDReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, LastOperation: &smClientTypes.Operation{State: smClientTypes.SUCCEEDED, Type: smClientTypes.CREATE}}, nil) }) It("should eventually succeed", func() { - binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "") + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "", false) Expect(err).ToNot(HaveOccurred()) waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", "no polling for you") fakeClient.ListBindingsReturns(&smClientTypes.ServiceBindings{ @@ -541,7 +516,7 @@ var _ = Describe("ServiceBinding controller", func() { common.UseInstanceMetadataNameInSecret: "true", } updateInstance(ctx, createdInstance) - createdBinding = createBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "") + createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "") bindingSecret := getSecret(ctx, createdBinding.Spec.SecretName, createdBinding.Namespace, true) validateInstanceInfo(bindingSecret, instanceName) validateSecretMetadata(bindingSecret, nil) @@ -550,7 +525,7 @@ var _ = Describe("ServiceBinding controller", func() { When("external name is not provided", func() { It("succeeds and uses the k8s name as external name", func() { - createdBinding = createBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "") + createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "", "") Expect(createdBinding.Spec.ExternalName).To(Equal(createdBinding.Name)) }) }) @@ -566,10 +541,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) + utils.SetFailureConditions(smClientTypes.CREATE, "Failed to create instance (test)", createdInstance, false) updateInstanceStatus(ctx, createdInstance) - binding := createBindingWithBlockedError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", "is not usable") - utils.SetSuccessConditions(smClientTypes.CREATE, 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) updateInstanceStatus(ctx, createdInstance) waitForResourceToBeReady(ctx, binding) }) @@ -578,16 +557,16 @@ var _ = Describe("ServiceBinding controller", func() { 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) + utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "", createdInstance, false) createdInstance.Status.OperationURL = "/1234" createdInstance.Status.OperationType = smClientTypes.CREATE updateInstanceStatus(ctx, createdInstance) - 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()) Expect(utils.IsInProgress(createdBinding)).To(BeTrue()) - utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance) + utils.SetSuccessConditions(smClientTypes.CREATE, createdInstance, false) createdInstance.Status.OperationType = "" createdInstance.Status.OperationURL = "" updateInstanceStatus(ctx, createdInstance) @@ -610,7 +589,7 @@ stringData: newKey: {{ .credentials.secret_key }} tags: {{ .instance.tags }}`) - createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) + createdBinding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) Expect(err).ToNot(HaveOccurred()) Expect(isResourceReady(createdBinding)).To(BeTrue()) By("Verify binding secret created") @@ -632,7 +611,7 @@ stringData: newKey: {{ .credentials.secret_key }} tags: {{ .instance.tags }}`) - createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) + createdBinding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) Expect(err).ToNot(HaveOccurred()) Expect(isResourceReady(createdBinding)).To(BeTrue()) By("Verify binding secret created") @@ -649,7 +628,9 @@ stringData: kind: Secret metadata: name: my-secret-name`) - createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "", "the Secret template is invalid: Secret's metadata field", secretTemplate) + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, false) + Expect(err).ToNot(HaveOccurred()) + waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", "the Secret template is invalid: Secret's metadata field") }) It("should fail to create the secret if wrong template key in the spec.secretTemplate is provided", func() { ctx := context.Background() @@ -659,7 +640,7 @@ stringData: stringData: foo: {{ .non_existing_key }}`) - binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate) + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, false) Expect(err).To(BeNil()) bindingLookupKey := getResourceNamespacedName(binding) Eventually(func() bool { @@ -675,7 +656,9 @@ stringData: secretTemplate := dedent.Dedent(` apiVersion: v1 kind: Pod`) - createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "", "but needs to be of kind 'Secret'", secretTemplate) + binding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, false) + Expect(err).ToNot(HaveOccurred()) + waitForResourceCondition(ctx, binding, common.ConditionFailed, metav1.ConditionTrue, "", "but needs to be of kind 'Secret'") }) It("should succeed to create the secret- empty data", func() { ctx := context.Background() @@ -689,7 +672,7 @@ metadata: instance_name: {{ .instance.instance_name }} stringData:`) - createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) + createdBinding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) Expect(err).ToNot(HaveOccurred()) Expect(isResourceReady(createdBinding)).To(BeTrue()) By("Verify binding secret created") @@ -722,7 +705,7 @@ metadata: annotations: instance_name: {{ .instance.instance_name }}`) - createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) + createdBinding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) Expect(err).ToNot(HaveOccurred()) Expect(isResourceReady(createdBinding)).To(BeTrue()) By("Verify binding secret created") @@ -761,7 +744,7 @@ stringData: newKey: {{ .credentials.auth.basic.password }} tags: {{ .instance.tags }}`) - createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) + createdBinding, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) Expect(err).ToNot(HaveOccurred()) Expect(isResourceReady(createdBinding)).To(BeTrue()) By("Verify binding secret created") @@ -786,7 +769,7 @@ metadata: instance_name: {{ .instance.instance_name }} stringData: newKey: {{ .credentials.secret_key }}`) - createdBinding = createBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", secretTemplate) + createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", secretTemplate) fakeClient.GetBindingByIDReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, nil) Expect(isResourceReady(createdBinding)).To(BeTrue()) }) @@ -922,14 +905,9 @@ stringData: }) Context("Delete", func() { - deleteAndValidate := func(binding *v1.ServiceBinding) { - deleteAndWait(ctx, getResourceNamespacedName(createdBinding), &v1.ServiceBinding{}) - err := k8sClient.Get(ctx, types.NamespacedName{Name: binding.Spec.SecretName, Namespace: binding.Namespace}, &corev1.Secret{}) - Expect(apierrors.IsNotFound(err)).To(BeTrue()) - } BeforeEach(func() { - createdBinding = createBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "") + createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "") Expect(isResourceReady(createdBinding)).To(BeTrue()) }) @@ -939,7 +917,7 @@ stringData: fakeClient.UnbindReturns("", nil) }) It("should delete the k8s binding and secret", func() { - deleteAndValidate(createdBinding) + deleteAndWait(ctx, createdBinding) }) }) @@ -964,7 +942,7 @@ stringData: }) It("recovers the binding and delete the k8s binding and secret", func() { - deleteAndValidate(createdBinding) + deleteAndWait(ctx, createdBinding) }) }) @@ -975,7 +953,7 @@ stringData: }) AfterEach(func() { fakeClient.UnbindReturns("", nil) - deleteAndValidate(createdBinding) + deleteAndWait(ctx, createdBinding) }) It("should not remove finalizer and keep the secret", func() { @@ -1000,7 +978,7 @@ stringData: }) It("should eventually succeed", func() { - deleteAndValidate(createdBinding) + deleteAndWait(ctx, createdBinding) }) }) @@ -1009,7 +987,7 @@ stringData: fakeClient.UnbindReturns("", nil) }) It("should succeed", func() { - createdBinding, err := createBindingWithoutAssertions(ctx, bindingName+"-new", bindingTestNamespace, "non-exist-instance", "", "binding-external-name", "") + createdBinding, err := createBindingWithoutAssertions(ctx, bindingName+"-new", bindingTestNamespace, "non-exist-instance", "", "binding-external-name", "", false) Expect(err).ToNot(HaveOccurred()) createdBinding.Finalizers = []string{common.FinalizerName} Expect(k8sClient.Update(ctx, createdBinding)) @@ -1021,7 +999,7 @@ stringData: cond := meta.FindStatusCondition(createdBinding.GetConditions(), common.ConditionSucceeded) return cond != nil && cond.Reason == common.Blocked }, timeout, interval).Should(BeTrue()) - deleteAndValidate(createdBinding) + deleteAndWait(ctx, createdBinding) }) }) }) @@ -1038,7 +1016,7 @@ stringData: It("should delete the k8s binding and secret", func() { Expect(k8sClient.Delete(ctx, createdBinding)).To(Succeed()) - deleteAndValidate(createdBinding) + deleteAndWait(ctx, createdBinding) }) }) @@ -1063,7 +1041,7 @@ stringData: return failedCond != nil && strings.Contains(failedCond.Message, errorMessage) }, timeout, interval).Should(BeTrue()) fakeClient.UnbindReturns("", nil) - deleteAndValidate(createdBinding) + deleteAndWait(ctx, createdBinding) }) }) @@ -1085,7 +1063,7 @@ stringData: return cond != nil && strings.Contains(cond.Message, string(smClientTypes.INPROGRESS)) }, timeout, interval).Should(BeTrue()) fakeClient.UnbindReturns("", nil) - deleteAndValidate(createdBinding) + deleteAndWait(ctx, createdBinding) }) }) }) @@ -1128,7 +1106,7 @@ stringData: When(fmt.Sprintf("last operation is %s %s", testCase.lastOpType, testCase.lastOpState), func() { It("should resync status", func() { var err error - createdBinding, err = createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "fake-binding-external-name", "", false) + createdBinding, err = createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "fake-binding-external-name", "", false) Expect(err).ToNot(HaveOccurred()) smCallArgs := fakeClient.ListBindingsArgsForCall(0) Expect(smCallArgs.LabelQuery).To(HaveLen(1)) @@ -1180,7 +1158,7 @@ stringData: It("should resync successfully", func() { var err error - createdBinding, err = createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "fake-binding-external-name", "", false) + createdBinding, err = createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "fake-binding-external-name", "", false) Expect(err).ToNot(HaveOccurred()) }) }) @@ -1189,7 +1167,7 @@ stringData: Context("Credential Rotation", func() { BeforeEach(func() { fakeClient.RenameBindingReturns(nil, nil) - createdBinding = createBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "") + createdBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, "", "binding-external-name", "") fakeClient.ListBindingsStub = func(params *sm.Parameters) (*smClientTypes.ServiceBindings, error) { if params == nil || params.FieldQuery == nil || len(params.FieldQuery) == 0 { return nil, nil @@ -1343,19 +1321,13 @@ stringData: Context("Cross Namespace", func() { var crossBinding *v1.ServiceBinding - var paramsSecret *corev1.Secret + var serviceInstanceInAnotherNamespace *v1.ServiceInstance BeforeEach(func() { - paramsSecret = &corev1.Secret{} - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "param-secret"}, paramsSecret) - if apierrors.IsNotFound(err) { - createParamsSecret(testNamespace) - } else { - Expect(err).ToNot(HaveOccurred()) - } + serviceInstanceInAnotherNamespace = createInstance(ctx, instanceName, testNamespace, instanceExternalName) }) AfterEach(func() { - deleteAndWait(ctx, types.NamespacedName{Namespace: testNamespace, Name: "param-secret"}, &corev1.Secret{}) + deleteAndWait(ctx, serviceInstanceInAnotherNamespace) }) When("binding is created in a different namespace than the instance", func() { @@ -1365,7 +1337,7 @@ stringData: } }) It("should succeed", func() { - crossBinding = createBinding(ctx, bindingName, testNamespace, instanceName, bindingTestNamespace, "cross-binding-external-name", "") + crossBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, testNamespace, "cross-binding-external-name", "") By("Verify binding secret created") getSecret(ctx, createdBinding.Spec.SecretName, createdBinding.Namespace, true) @@ -1375,7 +1347,7 @@ stringData: Context("cred rotation", func() { BeforeEach(func() { fakeClient.RenameBindingReturns(nil, nil) - crossBinding = createBinding(ctx, bindingName, testNamespace, instanceName, bindingTestNamespace, "cross-binding-external-name", "") + crossBinding = createAndValidateBinding(ctx, bindingName, bindingTestNamespace, instanceName, testNamespace, "cross-binding-external-name", "") fakeClient.ListBindingsStub = func(params *sm.Parameters) (*smClientTypes.ServiceBindings, error) { if params == nil || params.FieldQuery == nil || len(params.FieldQuery) == 0 { return nil, nil @@ -1402,12 +1374,12 @@ stringData: }) AfterEach(func() { if crossBinding != nil { - Expect(k8sClient.Delete(ctx, crossBinding)) + deleteAndWait(ctx, crossBinding) } }) It("should rotate the credentials and create old binding", func() { - key := types.NamespacedName{Name: bindingName, Namespace: testNamespace} + key := types.NamespacedName{Name: bindingName, Namespace: bindingTestNamespace} Expect(k8sClient.Get(ctx, key, crossBinding)).To(Succeed()) crossBinding.Spec.CredRotationPolicy = &v1.CredentialsRotationPolicy{ Enabled: true, @@ -1417,7 +1389,7 @@ stringData: var secret *corev1.Secret Eventually(func() bool { - secret = getSecret(ctx, crossBinding.Spec.SecretName, testNamespace, true) + secret = getSecret(ctx, crossBinding.Spec.SecretName, bindingTestNamespace, true) secret.Data = map[string][]byte{} return k8sClient.Update(ctx, secret) == nil }, timeout, interval).Should(BeTrue()) @@ -1430,19 +1402,19 @@ stringData: return err == nil && myBinding.Status.LastCredentialsRotationTime != nil && len(myBinding.Status.Conditions) == 2 }, timeout, interval).Should(BeTrue()) - secret = getSecret(ctx, myBinding.Spec.SecretName, testNamespace, true) + secret = getSecret(ctx, myBinding.Spec.SecretName, bindingTestNamespace, true) val := secret.Data["secret_key"] Expect(string(val)).To(Equal("secret_value")) bindingList := &v1.ServiceBindingList{} Eventually(func() bool { - Expect(k8sClient.List(ctx, bindingList, client.MatchingLabels{common.StaleBindingIDLabel: myBinding.Status.BindingID}, client.InNamespace(testNamespace))).To(Succeed()) + Expect(k8sClient.List(ctx, bindingList, client.MatchingLabels{common.StaleBindingIDLabel: myBinding.Status.BindingID}, client.InNamespace(bindingTestNamespace))).To(Succeed()) return len(bindingList.Items) > 0 }, timeout, interval).Should(BeTrue()) oldBinding := bindingList.Items[0] Expect(oldBinding.Spec.CredRotationPolicy.Enabled).To(BeFalse()) - secret = getSecret(ctx, oldBinding.Spec.SecretName, testNamespace, true) + secret = getSecret(ctx, oldBinding.Spec.SecretName, bindingTestNamespace, true) val = secret.Data["secret_key2"] Expect(string(val)).To(Equal("secret_value2")) }) @@ -1464,7 +1436,7 @@ func generateBasicBindingTemplate(name, namespace, instanceName, instanceNamespa binding.Spec.ParametersFrom = []v1.ParametersFromSource{ { SecretKeyRef: &v1.SecretKeyReference{ - Name: "param-secret", + Name: "binding-params-secret", Key: "secret-parameter", }, }, diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 2cb1d798..13be1eae 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -18,14 +18,10 @@ package controllers import ( "context" - "crypto/md5" - "encoding/hex" "encoding/json" "fmt" "net/http" - "k8s.io/utils/ptr" - "github.com/SAP/sap-btp-service-operator/api/common" "github.com/SAP/sap-btp-service-operator/internal/config" "github.com/SAP/sap-btp-service-operator/internal/utils" @@ -81,6 +77,9 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } serviceInstance = serviceInstance.DeepCopy() + if utils.IsMarkedForDeletion(serviceInstance.ObjectMeta) { + return r.deleteInstance(ctx, serviceInstance) + } if len(serviceInstance.GetConditions()) == 0 { err := utils.InitConditions(ctx, r.Client, serviceInstance) if err != nil { @@ -100,12 +99,6 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - if utils.IsMarkedForDeletion(serviceInstance.ObjectMeta) { - // delete updates the generation - serviceInstance.SetObservedGeneration(serviceInstance.Generation) - return r.deleteInstance(ctx, serviceInstance) - } - if len(serviceInstance.Status.OperationURL) > 0 { // ongoing operation - poll status from SM return r.poll(ctx, serviceInstance) @@ -119,9 +112,6 @@ 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) if err != nil { log.Error(err, "failed to get sm client") @@ -145,19 +135,15 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Update if updateRequired(serviceInstance) { - if res, err := r.updateInstance(ctx, smClient, serviceInstance); err != nil { - log.Info("got error while trying to update instance") - return ctrl.Result{}, err - } else if res.Requeue { - return res, nil - } + return r.updateInstance(ctx, smClient, serviceInstance) } - // Handle instance share if needed - if sharingUpdateRequired(serviceInstance) { + // share/unshare + if shareOrUnshareRequired(serviceInstance) { return r.handleInstanceSharing(ctx, serviceInstance, smClient) } + log.Info("No action required") return ctrl.Result{}, nil } @@ -176,7 +162,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient 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") - return utils.MarkAsNonTransientError(ctx, r.Client, smClientTypes.CREATE, err, serviceInstance) + return utils.MarkAsTransientError(ctx, r.Client, smClientTypes.CREATE, err, serviceInstance) } provision, provisionErr := smClient.Provision(&smClientTypes.ServiceInstance{ @@ -211,14 +197,14 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient log.Info("Provision request is in progress (async)") serviceInstance.Status.OperationURL = provision.Location serviceInstance.Status.OperationType = smClientTypes.CREATE - utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "", serviceInstance) + utils.SetInProgressConditions(ctx, smClientTypes.CREATE, "", serviceInstance, false) return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, utils.UpdateStatus(ctx, r.Client, serviceInstance) } log.Info(fmt.Sprintf("Instance provisioned successfully, instanceID: %s, subaccountID: %s", serviceInstance.Status.InstanceID, serviceInstance.Status.SubaccountID)) - utils.SetSuccessConditions(smClientTypes.CREATE, serviceInstance) + utils.SetSuccessConditions(smClientTypes.CREATE, serviceInstance, false) return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance) } @@ -226,14 +212,13 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient log := utils.GetLogger(ctx) log.Info(fmt.Sprintf("updating instance %s in SM", serviceInstance.Status.InstanceID)) - updateHashedSpecValue(serviceInstance) - _, 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, err, serviceInstance) + return utils.MarkAsTransientError(ctx, r.Client, smClientTypes.UPDATE, err, serviceInstance) } + updateHashedSpecValue(serviceInstance) _, operationURL, err := smClient.UpdateInstance(serviceInstance.Status.InstanceID, &smClientTypes.ServiceInstance{ Name: serviceInstance.Spec.ExternalName, ServicePlanID: serviceInstance.Spec.ServicePlanID, @@ -249,8 +234,7 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient log.Info(fmt.Sprintf("Update request accepted, operation URL: %s", operationURL)) serviceInstance.Status.OperationURL = operationURL serviceInstance.Status.OperationType = smClientTypes.UPDATE - utils.SetInProgressConditions(ctx, smClientTypes.UPDATE, "", serviceInstance) - + utils.SetInProgressConditions(ctx, smClientTypes.UPDATE, "", serviceInstance, false) if err := utils.UpdateStatus(ctx, r.Client, serviceInstance); err != nil { return ctrl.Result{}, err } @@ -258,13 +242,14 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil } log.Info("Instance updated successfully") - utils.SetSuccessConditions(smClientTypes.UPDATE, serviceInstance) + utils.SetSuccessConditions(smClientTypes.UPDATE, serviceInstance, false) return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance) } func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, serviceInstance *v1.ServiceInstance) (ctrl.Result, error) { log := utils.GetLogger(ctx) + log.Info("deleting instance") if controllerutil.ContainsFinalizer(serviceInstance, common.FinalizerName) { smClient, err := r.GetSMClient(ctx, serviceInstance) if err != nil { @@ -280,7 +265,7 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, serviceI if smInstance != nil { log.Info("instance exists in SM continue with deletion") serviceInstance.Status.InstanceID = smInstance.ID - utils.SetInProgressConditions(ctx, smClientTypes.DELETE, "delete after recovery", serviceInstance) + utils.SetInProgressConditions(ctx, smClientTypes.DELETE, "delete after recovery", serviceInstance, false) return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance) } log.Info("instance does not exists in SM, removing finalizer") @@ -315,8 +300,8 @@ func (r *ServiceInstanceReconciler) handleInstanceSharing(ctx context.Context, s log := utils.GetLogger(ctx) log.Info("Handling change in instance sharing") - if serviceInstance.ShouldBeShared() { - log.Info("Service instance is shouldBeShared, sharing the instance") + if serviceInstance.GetShared() { + log.Info("Service instance appears to be unshared, sharing the instance") err := smClient.ShareInstance(serviceInstance.Status.InstanceID, utils.BuildUserInfo(ctx, serviceInstance.Spec.UserInfo)) if err != nil { log.Error(err, "failed to share instance") @@ -325,7 +310,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharing(ctx context.Context, s log.Info("instance shared successfully") setSharedCondition(serviceInstance, metav1.ConditionTrue, common.ShareSucceeded, "instance shared successfully") } else { //un-share - log.Info("Service instance is un-shouldBeShared, un-sharing the instance") + log.Info("Service instance appears to be shared, un-sharing the instance") err := smClient.UnShareInstance(serviceInstance.Status.InstanceID, utils.BuildUserInfo(ctx, serviceInstance.Spec.UserInfo)) if err != nil { log.Error(err, "failed to un-share instance") @@ -357,9 +342,9 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *v status, statusErr := smClient.Status(serviceInstance.Status.OperationURL, nil) if statusErr != nil { 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) + utils.SetInProgressConditions(ctx, serviceInstance.Status.OperationType, string(smClientTypes.INPROGRESS), serviceInstance, false) // if failed to read operation status we cleanup the status to trigger re-sync from SM - freshStatus := v1.ServiceInstanceStatus{Conditions: serviceInstance.GetConditions(), ObservedGeneration: serviceInstance.Generation} + freshStatus := v1.ServiceInstanceStatus{Conditions: serviceInstance.GetConditions()} if utils.IsMarkedForDeletion(serviceInstance.ObjectMeta) { freshStatus.InstanceID = serviceInstance.Status.InstanceID } @@ -380,7 +365,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *v case smClientTypes.PENDING: if len(status.Description) > 0 { log.Info(fmt.Sprintf("last operation description is '%s'", status.Description)) - utils.SetInProgressConditions(ctx, status.Type, status.Description, serviceInstance) + utils.SetInProgressConditions(ctx, status.Type, status.Description, serviceInstance, true) if err := utils.UpdateStatus(ctx, r.Client, serviceInstance); err != nil { log.Error(err, "unable to update ServiceInstance polling description") return ctrl.Result{}, err @@ -389,7 +374,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *v return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil case smClientTypes.FAILED: errMsg := getErrorMsgFromLastOperation(status) - utils.SetFailureConditions(status.Type, errMsg, serviceInstance) + utils.SetFailureConditions(status.Type, errMsg, serviceInstance, true) // in order to delete eventually the object we need return with error if serviceInstance.Status.OperationType == smClientTypes.DELETE { serviceInstance.Status.OperationURL = "" @@ -416,7 +401,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *v return ctrl.Result{}, err } } - utils.SetSuccessConditions(status.Type, serviceInstance) + utils.SetSuccessConditions(status.Type, serviceInstance, true) } serviceInstance.Status.OperationURL = "" @@ -428,7 +413,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *v 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) + utils.SetInProgressConditions(ctx, smClientTypes.DELETE, "", serviceInstance, false) if err := utils.UpdateStatus(ctx, r.Client, serviceInstance); err != nil { return ctrl.Result{}, err @@ -467,14 +452,6 @@ func (r *ServiceInstanceReconciler) recover(ctx context.Context, smClient sm.Cli log.Info(fmt.Sprintf("found existing instance in SM with id %s, updating status", smInstance.ID)) updateHashedSpecValue(k8sInstance) - // set observed generation to 0 because we dont know which generation the current state in SM represents, - // unless the generation is 1 and SM is in the same state as operator - if k8sInstance.Generation == 1 { - k8sInstance.SetObservedGeneration(1) - } else { - k8sInstance.SetObservedGeneration(0) - } - if smInstance.Ready { k8sInstance.Status.Ready = metav1.ConditionTrue } @@ -509,11 +486,11 @@ func (r *ServiceInstanceReconciler) recover(ctx context.Context, smClient sm.Cli case smClientTypes.INPROGRESS: k8sInstance.Status.OperationURL = sm.BuildOperationURL(smInstance.LastOperation.ID, smInstance.ID, smClientTypes.ServiceInstancesURL) k8sInstance.Status.OperationType = smInstance.LastOperation.Type - utils.SetInProgressConditions(ctx, smInstance.LastOperation.Type, smInstance.LastOperation.Description, k8sInstance) + utils.SetInProgressConditions(ctx, smInstance.LastOperation.Type, smInstance.LastOperation.Description, k8sInstance, false) case smClientTypes.SUCCEEDED: - utils.SetSuccessConditions(operationType, k8sInstance) + utils.SetSuccessConditions(operationType, k8sInstance, false) case smClientTypes.FAILED: - utils.SetFailureConditions(operationType, description, k8sInstance) + utils.SetFailureConditions(operationType, description, k8sInstance, false) } return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, k8sInstance) @@ -548,16 +525,15 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte 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") - return false - } + if len(serviceInstance.Status.OperationURL) > 0 { log.Info(fmt.Sprintf("instance is not in final state, async operation is in progress (%s)", serviceInstance.Status.OperationURL)) return false } - if serviceInstance.Generation != serviceInstance.GetObservedGeneration() { - log.Info(fmt.Sprintf("instance is not in final state, generation: %d, observedGen: %d", serviceInstance.Generation, serviceInstance.GetObservedGeneration())) + + observedGen := common.GetObservedGeneration(serviceInstance) + if serviceInstance.Generation != observedGen { + log.Info(fmt.Sprintf("instance is not in final state, generation: %d, observedGen: %d", serviceInstance.Generation, observedGen)) return false } @@ -567,7 +543,7 @@ func isFinalState(ctx context.Context, serviceInstance *v1.ServiceInstance) bool return false } - if sharingUpdateRequired(serviceInstance) { + if shareOrUnshareRequired(serviceInstance) { log.Info("instance is not in final state, need to sync sharing status") if len(serviceInstance.Status.HashedSpec) == 0 { updateHashedSpecValue(serviceInstance) @@ -590,35 +566,31 @@ func updateRequired(serviceInstance *v1.ServiceInstance) bool { return true } - return getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec + return serviceInstance.GetSpecHash() != serviceInstance.Status.HashedSpec } -func sharingUpdateRequired(serviceInstance *v1.ServiceInstance) bool { +func shareOrUnshareRequired(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 } sharedCondition := meta.FindStatusCondition(serviceInstance.GetConditions(), common.ConditionShared) - shouldBeShared := serviceInstance.ShouldBeShared() - if sharedCondition == nil { - return shouldBeShared + return serviceInstance.GetShared() } if sharedCondition.Reason == common.ShareNotSupported { return false } - if sharedCondition.Reason == common.InProgress || sharedCondition.Reason == common.ShareFailed || sharedCondition.Reason == common.UnShareFailed { - return true - } - - if shouldBeShared { - return sharedCondition.Status == metav1.ConditionFalse + if sharedCondition.Status == metav1.ConditionFalse { + // instance does not appear to be shared, should share it if shared is requested + return serviceInstance.GetShared() } - return sharedCondition.Status == metav1.ConditionTrue + // instance appears to be shared, should unshare it if shared is not requested + return !serviceInstance.GetShared() } func getOfferingTags(smClient sm.Client, planID string) ([]string, error) { @@ -661,19 +633,6 @@ func getTags(tags []byte) ([]string, error) { return tagsArr, nil } -func getSpecHash(serviceInstance *v1.ServiceInstance) string { - spec := serviceInstance.Spec - spec.Shared = ptr.To(false) - specBytes, _ := json.Marshal(spec) - s := string(specBytes) - return generateEncodedMD5Hash(s) -} - -func generateEncodedMD5Hash(str string) string { - hash := md5.Sum([]byte(str)) - return hex.EncodeToString(hash[:]) -} - func setSharedCondition(object common.SAPBTPResource, status metav1.ConditionStatus, reason, msg string) { conditions := object.GetConditions() // align all conditions to latest generation @@ -700,7 +659,7 @@ func setSharedCondition(object common.SAPBTPResource, status metav1.ConditionSta } func updateHashedSpecValue(serviceInstance *v1.ServiceInstance) { - serviceInstance.Status.HashedSpec = getSpecHash(serviceInstance) + serviceInstance.Status.HashedSpec = serviceInstance.GetSpecHash() } func getErrorMsgFromLastOperation(status *smClientTypes.Operation) string { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 01fb7399..7c8cecae 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -42,11 +42,10 @@ const ( var _ = Describe("ServiceInstance controller", func() { var ( - ctx context.Context - serviceInstance *v1.ServiceInstance fakeInstanceName string defaultLookupKey types.NamespacedName + paramsSecret *corev1.Secret ) instanceSpec := v1.ServiceInstanceSpec{ @@ -59,7 +58,7 @@ var _ = Describe("ServiceInstance controller", func() { ParametersFrom: []v1.ParametersFromSource{ { SecretKeyRef: &v1.SecretKeyReference{ - Name: "param-secret", + Name: "instance-params-secret", Key: "secret-parameter", }, }, @@ -77,21 +76,21 @@ var _ = Describe("ServiceInstance controller", func() { ParametersFrom: []v1.ParametersFromSource{ { SecretKeyRef: &v1.SecretKeyReference{ - Name: "param-secret", + Name: "instance-params-secret", Key: "secret-parameter", }, }, }, } - createInstance := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, annotations map[string]string, waitForReady bool) *v1.ServiceInstance { + createInstance := func(ctx context.Context, instanceName string, instanceSpec v1.ServiceInstanceSpec, annotations map[string]string, waitForReady bool) *v1.ServiceInstance { instance := &v1.ServiceInstance{ TypeMeta: metav1.TypeMeta{ APIVersion: "services.cloud.sap.com/v1", Kind: "ServiceInstance", }, ObjectMeta: metav1.ObjectMeta{ - Name: fakeInstanceName, + Name: instanceName, Namespace: testNamespace, Annotations: annotations, }, @@ -138,18 +137,14 @@ var _ = Describe("ServiceInstance controller", func() { fakeClient.DeprovisionReturns("", nil) fakeClient.GetInstanceByIDReturns(&smclientTypes.ServiceInstance{ID: fakeInstanceID, Ready: true, LastOperation: &smClientTypes.Operation{State: smClientTypes.SUCCEEDED, Type: smClientTypes.CREATE}}, nil) - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "param-secret"}, &corev1.Secret{}) - if apierrors.IsNotFound(err) { - createParamsSecret(testNamespace) - } else { - Expect(err).ToNot(HaveOccurred()) - } + paramsSecret = createParamsSecret(ctx, "instance-params-secret", testNamespace) }) AfterEach(func() { if serviceInstance != nil { - deleteInstance(ctx, serviceInstance, true) + deleteAndWait(ctx, serviceInstance) } + deleteAndWait(ctx, paramsSecret) }) Describe("Create", func() { @@ -204,7 +199,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("provisioning should fail", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForInstanceConditionAndMessage(ctx, defaultLookupKey, common.ConditionSucceeded, "provided plan id does not match") }) }) @@ -214,14 +209,14 @@ var _ = Describe("ServiceInstance controller", func() { Context("Sync", func() { When("provision request to SM succeeds", func() { It("should provision instance of the provided offering and plan name successfully", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) Expect(serviceInstance.Status.SubaccountID).To(Equal(fakeSubaccountID)) Expect(serviceInstance.Spec.ExternalName).To(Equal(fakeInstanceExternalName)) Expect(serviceInstance.Name).To(Equal(fakeInstanceName)) Expect(serviceInstance.Status.HashedSpec).To(Not(BeNil())) Expect(string(serviceInstance.Spec.Parameters.Raw)).To(ContainSubstring("\"key\":\"value\"")) - Expect(serviceInstance.Status.HashedSpec).To(Equal(getSpecHash(serviceInstance))) + Expect(serviceInstance.Status.HashedSpec).To(Equal(serviceInstance.GetSpecHash())) smInstance, _, _, _, _, _ := fakeClient.ProvisionArgsForCall(0) params := smInstance.Parameters Expect(params).To(ContainSubstring("\"key\":\"value\"")) @@ -242,7 +237,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should have failure condition", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionTrue, common.Created, "") }) }) @@ -255,7 +250,7 @@ var _ = Describe("ServiceInstance controller", func() { Description: errMessage, }) fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionTrue, common.Created, "") }) It("provision fails until timeout", func() { @@ -263,7 +258,7 @@ var _ = Describe("ServiceInstance controller", func() { StatusCode: http.StatusBadRequest, Description: errMessage, }) - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, errMessage) }) }) @@ -278,7 +273,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should retry until success", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionTrue, common.Created, "") }) }) @@ -290,7 +285,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should be transient error and eventually succeed", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, tooManyRequestsError) fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) waitForResourceToBeReady(ctx, serviceInstance) @@ -303,7 +298,7 @@ var _ = Describe("ServiceInstance controller", func() { fakeClient.ProvisionReturnsOnCall(2, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) It("should fail first and then succeed", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionTrue, common.Created, "") Expect(fakeClient.ProvisionCallCount()).To(BeNumerically(">", 1)) }) @@ -326,7 +321,7 @@ var _ = Describe("ServiceInstance controller", func() { fakeClient.GetInstanceByIDReturns(&smclientTypes.ServiceInstance{Labels: map[string][]string{"subaccount_id": {fakeSubaccountID}}}, nil) }) It("should update in progress condition and provision the instance successfully", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) fakeClient.StatusReturns(&smclientTypes.Operation{ ID: "1234", Type: smClientTypes.CREATE, @@ -339,7 +334,7 @@ var _ = Describe("ServiceInstance controller", func() { When("polling ends with failure", func() { It("should update to failure condition with the broker err description", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) fakeClient.StatusReturns(&smclientTypes.Operation{ ID: "1234", Type: smClientTypes.CREATE, @@ -352,7 +347,7 @@ var _ = Describe("ServiceInstance controller", func() { When("updating during create", func() { It("should update the instance after created successfully", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, "") newName := "new-name" + uuid.New().String() @@ -380,7 +375,7 @@ var _ = Describe("ServiceInstance controller", func() { When("deleting while create is in progress", func() { It("should be deleted successfully", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) By("waiting for instance to be CreateInProgress") waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, "") @@ -412,7 +407,7 @@ var _ = Describe("ServiceInstance controller", func() { ServicePlanName: "a-plan-name", ServiceOfferingName: "an-offering-name", } - serviceInstance = createInstance(ctx, withoutExternal, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, withoutExternal, nil, true) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) Expect(serviceInstance.Spec.ExternalName).To(Equal(fakeInstanceName)) Expect(serviceInstance.Name).To(Equal(fakeInstanceName)) @@ -422,7 +417,7 @@ var _ = Describe("ServiceInstance controller", func() { Describe("Update", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) Expect(serviceInstance.Spec.ExternalName).To(Equal(fakeInstanceExternalName)) }) @@ -611,7 +606,7 @@ var _ = Describe("ServiceInstance controller", func() { When("subaccount id changed", func() { It("should fail", func() { deleteInstance(ctx, serviceInstance, true) - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) serviceInstance.Spec.BTPAccessCredentialsSecret = "12345" err := k8sClient.Update(ctx, serviceInstance) Expect(err).To(HaveOccurred()) @@ -634,7 +629,7 @@ var _ = Describe("ServiceInstance controller", func() { Describe("Delete", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) fakeClient.DeprovisionReturns("", nil) }) AfterEach(func() { @@ -783,7 +778,7 @@ var _ = Describe("ServiceInstance controller", func() { Describe("full reconcile", func() { When("instance hashedSpec is not initialized", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) }) It("should not send update request and update the hashed spec", func() { hashed := serviceInstance.Status.HashedSpec @@ -814,7 +809,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should call correctly to SM and recover the instance", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) smCallArgs := fakeClient.ListInstancesArgsForCall(0) @@ -837,12 +832,12 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should recover the existing instance and poll until instance is ready", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) key := getResourceNamespacedName(serviceInstance) Eventually(func() bool { _ = k8sClient.Get(ctx, key, serviceInstance) return serviceInstance.Status.InstanceID == fakeInstanceID - }, timeout, interval).Should(BeTrue(), eventuallyMsgForResource("service instance id not recovered", key, serviceInstance)) + }, timeout, interval).Should(BeTrue(), eventuallyMsgForResource("service instance id not recovered", serviceInstance)) Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) Expect(fakeClient.ListInstancesCallCount()).To(BeNumerically(">", 0)) fakeClient.StatusReturns(&smclientTypes.Operation{ResourceID: fakeInstanceID, State: smClientTypes.SUCCEEDED, Type: smClientTypes.CREATE}, nil) @@ -858,7 +853,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should recover the existing instance and update condition failure", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateFailed, "") Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) @@ -876,7 +871,7 @@ var _ = Describe("ServiceInstance controller", func() { recoveredInstance.Ready = true }) It("should recover the instance with status Ready=true", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceToBeReady(ctx, serviceInstance) Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) @@ -887,7 +882,7 @@ var _ = Describe("ServiceInstance controller", func() { recoveredInstance.Ready = false }) It("should recover the instance with status Ready=false", func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionFailed, metav1.ConditionTrue, common.CreateFailed, "") Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) @@ -904,14 +899,14 @@ var _ = Describe("ServiceInstance controller", func() { When("creating instance with shared=true", func() { It("should succeed to provision and sharing the instance", func() { fakeClient.ShareInstanceReturns(nil) - serviceInstance = createInstance(ctx, sharedInstanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, sharedInstanceSpec, nil, true) waitForInstanceToBeShared(ctx, serviceInstance) }) }) Context("sharing an existing instance", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) }) When("updating existing instance to shared", func() { @@ -952,7 +947,7 @@ var _ = Describe("ServiceInstance controller", func() { StatusCode: http.StatusBadRequest, Description: "errMessage", }) - serviceInstance = createInstance(ctx, sharedInstanceSpec, nil, false) + serviceInstance = createInstance(ctx, fakeInstanceName, sharedInstanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, "") Expect(fakeClient.ShareInstanceCallCount()).To(BeZero()) }) @@ -960,7 +955,7 @@ var _ = Describe("ServiceInstance controller", func() { When("instance is valid and share failed", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, instanceSpec, nil, true) }) When("shared failed with rate limit error", func() { @@ -1022,7 +1017,7 @@ var _ = Describe("ServiceInstance controller", func() { Context("un-sharing an existing shared instance", func() { BeforeEach(func() { fakeClient.ShareInstanceReturns(nil) - serviceInstance = createInstance(ctx, sharedInstanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, sharedInstanceSpec, nil, true) waitForInstanceToBeShared(ctx, serviceInstance) }) @@ -1061,7 +1056,7 @@ var _ = Describe("ServiceInstance controller", func() { When("instance is valid and un-share failed", func() { BeforeEach(func() { fakeClient.ShareInstanceReturns(nil) - serviceInstance = createInstance(ctx, sharedInstanceSpec, nil, true) + serviceInstance = createInstance(ctx, fakeInstanceName, sharedInstanceSpec, nil, true) waitForInstanceToBeShared(ctx, serviceInstance) fakeClient.UnShareInstanceReturns(&sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, @@ -1136,6 +1131,9 @@ var _ = Describe("ServiceInstance controller", func() { When("async operation in progress", func() { It("should return false", func() { var instance = &v1.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + }, Status: v1.ServiceInstanceStatus{ Conditions: []metav1.Condition{ { @@ -1144,9 +1142,8 @@ var _ = Describe("ServiceInstance controller", func() { ObservedGeneration: 1, }, }, - HashedSpec: "929e78f4449f8036ce39da3cc3e7eaea", - OperationURL: "/operations/somepollingurl", - ObservedGeneration: 2, + HashedSpec: "929e78f4449f8036ce39da3cc3e7eaea", + OperationURL: "/operations/somepollingurl", }, Spec: v1.ServiceInstanceSpec{ ExternalName: "name", @@ -1195,7 +1192,7 @@ var _ = Describe("ServiceInstance controller", func() { { Type: common.ConditionSucceeded, Status: metav1.ConditionTrue, - ObservedGeneration: 2, + ObservedGeneration: 1, }, }, HashedSpec: "bla", @@ -1243,6 +1240,9 @@ var _ = Describe("ServiceInstance controller", func() { When("in final state", func() { It("should return true", func() { var instance = &v1.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + }, Status: v1.ServiceInstanceStatus{ Conditions: []metav1.Condition{ { @@ -1260,8 +1260,7 @@ var _ = Describe("ServiceInstance controller", func() { Status: metav1.ConditionTrue, }, }, - HashedSpec: "929e78f4449f8036ce39da3cc3e7eaea", - ObservedGeneration: 2, + HashedSpec: "929e78f4449f8036ce39da3cc3e7eaea", }, Spec: v1.ServiceInstanceSpec{ ExternalName: "name", diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 211eaaad..111dda18 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -230,7 +230,7 @@ var _ = AfterSuite(func() { }) func isResourceReady(resource common.SAPBTPResource) bool { - return resource.GetObservedGeneration() == resource.GetGeneration() && + return common.GetObservedGeneration(resource) == resource.GetGeneration() && meta.IsStatusConditionPresentAndEqual(resource.GetConditions(), common.ConditionReady, metav1.ConditionTrue) } @@ -245,7 +245,7 @@ func waitForResourceCondition(ctx context.Context, resource common.SAPBTPResourc return false } - if resource.GetObservedGeneration() != resource.GetGeneration() { + if common.GetObservedGeneration(resource) != resource.GetGeneration() { return false } @@ -268,10 +268,7 @@ func waitForResourceCondition(ctx context.Context, resource common.SAPBTPResourc return true }, timeout*3, interval).Should(BeTrue(), - eventuallyMsgForResource( - fmt.Sprintf("expected condition: {type: %s, status: %s, reason: %s, message: %s} was not met", conditionType, status, reason, message), - key, - resource), + eventuallyMsgForResource(fmt.Sprintf("expected condition: {type: %s, status: %s, reason: %s, message: %s} was not met. %v", conditionType, status, reason, message, resource.GetConditions()), resource), ) } @@ -279,51 +276,48 @@ func getResourceNamespacedName(resource client.Object) types.NamespacedName { return types.NamespacedName{Namespace: resource.GetNamespace(), Name: resource.GetName()} } -func deleteAndWait(ctx context.Context, key types.NamespacedName, resource client.Object) { - wait := true +func deleteAndWait(ctx context.Context, resource client.Object) { Eventually(func() bool { - if err := k8sClient.Get(ctx, key, resource); err != nil { - if apierrors.IsNotFound(err) { - wait = false - return true - } - return false - } - - if err := k8sClient.Delete(ctx, resource); err != nil { + if err := k8sClient.Get(ctx, getResourceNamespacedName(resource), resource); err != nil { if apierrors.IsNotFound(err) { - wait = false return true } return false } - return true - }, timeout, interval).Should(BeTrue(), eventuallyMsgForResource("failed to mark for deletion", key, resource)) - - if wait { - waitForResourceToBeDeleted(ctx, key, resource) - } + err := k8sClient.Delete(ctx, resource) + return apierrors.IsNotFound(err) + }, timeout, interval).Should(BeTrue(), eventuallyMsgForResource("failed to delete resource", resource)) } func waitForResourceToBeDeleted(ctx context.Context, key types.NamespacedName, resource client.Object) { Eventually(func() bool { return apierrors.IsNotFound(k8sClient.Get(ctx, key, resource)) - }, timeout, interval).Should(BeTrue(), eventuallyMsgForResource("resource is not deleted", key, resource)) + }, timeout, interval).Should(BeTrue(), eventuallyMsgForResource("resource is not deleted", resource)) } -func createParamsSecret(namespace string) { +func createParamsSecret(ctx context.Context, secretName, namespace string) *corev1.Secret { credentialsMap := make(map[string][]byte) credentialsMap["secret-parameter"] = []byte("{\"secret-key\":\"secret-value\"}") + return createSecret(ctx, secretName, namespace, credentialsMap) +} + +func createSecret(ctx context.Context, secretName string, namespace string, credentialsMap map[string][]byte) *corev1.Secret { secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "param-secret", + Name: secretName, Namespace: namespace, }, Data: credentialsMap, } - Expect(k8sClient.Create(context.Background(), secret)).ToNot(HaveOccurred()) + Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{Name: secretName, Namespace: namespace}, secret) + return err == nil + }, timeout, interval).Should(BeTrue()) + return secret } func printSection(str string) { @@ -359,7 +353,7 @@ func getTransientBrokerError(errorMessage string) error { } } -func eventuallyMsgForResource(message string, key types.NamespacedName, resource client.Object) string { +func eventuallyMsgForResource(message string, resource client.Object) string { gvk, _ := apiutil.GVKForObject(resource, scheme.Scheme) - return fmt.Sprintf("eventaully failure for %s %s. message: %s", gvk.Kind, key.String(), message) + return fmt.Sprintf("eventaully failure for %s %s. message: %s", gvk.Kind, getResourceNamespacedName(resource), message) } diff --git a/internal/utils/condition_utils.go b/internal/utils/condition_utils.go index 6d82cc40..21174963 100644 --- a/internal/utils/condition_utils.go +++ b/internal/utils/condition_utils.go @@ -14,7 +14,7 @@ import ( func InitConditions(ctx context.Context, k8sClient client.Client, obj common.SAPBTPResource) error { obj.SetReady(metav1.ConditionFalse) - SetInProgressConditions(ctx, smClientTypes.CREATE, "Pending", obj) + SetInProgressConditions(ctx, smClientTypes.CREATE, "Pending", obj, false) return UpdateStatus(ctx, k8sClient, obj) } @@ -55,7 +55,7 @@ func GetConditionReason(opType smClientTypes.OperationCategory, state smClientTy return common.Unknown } -func SetInProgressConditions(ctx context.Context, operationType smClientTypes.OperationCategory, message string, object common.SAPBTPResource) { +func SetInProgressConditions(ctx context.Context, operationType smClientTypes.OperationCategory, message string, object common.SAPBTPResource, isAsyncOperation bool) { log := GetLogger(ctx) if len(message) == 0 { if operationType == smClientTypes.CREATE { @@ -71,12 +71,16 @@ func SetInProgressConditions(ctx context.Context, operationType smClientTypes.Op if len(conditions) > 0 { meta.RemoveStatusCondition(&conditions, common.ConditionFailed) } + observedGen := object.GetGeneration() + if isAsyncOperation { + observedGen = getLastObservedGen(object) + } lastOpCondition := metav1.Condition{ Type: common.ConditionSucceeded, Status: metav1.ConditionFalse, Reason: GetConditionReason(operationType, smClientTypes.INPROGRESS), Message: message, - ObservedGeneration: object.GetObservedGeneration(), + ObservedGeneration: observedGen, } meta.SetStatusCondition(&conditions, lastOpCondition) meta.SetStatusCondition(&conditions, getReadyCondition(object)) @@ -85,7 +89,7 @@ func SetInProgressConditions(ctx context.Context, operationType smClientTypes.Op log.Info(fmt.Sprintf("setting inProgress conditions: reason: %s, message:%s, generation: %d", lastOpCondition.Reason, message, object.GetGeneration())) } -func SetSuccessConditions(operationType smClientTypes.OperationCategory, object common.SAPBTPResource) { +func SetSuccessConditions(operationType smClientTypes.OperationCategory, object common.SAPBTPResource, isAsyncOperation bool) { var message string if operationType == smClientTypes.CREATE { message = fmt.Sprintf("%s provisioned successfully", object.GetControllerName()) @@ -99,19 +103,23 @@ func SetSuccessConditions(operationType smClientTypes.OperationCategory, object if len(conditions) > 0 { meta.RemoveStatusCondition(&conditions, common.ConditionFailed) } + observedGen := object.GetGeneration() + if isAsyncOperation { + observedGen = getLastObservedGen(object) + } lastOpCondition := metav1.Condition{ Type: common.ConditionSucceeded, Status: metav1.ConditionTrue, Reason: GetConditionReason(operationType, smClientTypes.SUCCEEDED), Message: message, - ObservedGeneration: object.GetObservedGeneration(), + ObservedGeneration: observedGen, } readyCondition := metav1.Condition{ Type: common.ConditionReady, Status: metav1.ConditionTrue, Reason: common.Provisioned, Message: message, - ObservedGeneration: object.GetObservedGeneration(), + ObservedGeneration: observedGen, } meta.SetStatusCondition(&conditions, lastOpCondition) meta.SetStatusCondition(&conditions, readyCondition) @@ -130,13 +138,13 @@ func SetCredRotationInProgressConditions(reason, message string, object common.S Status: metav1.ConditionTrue, Reason: reason, Message: message, - ObservedGeneration: object.GetObservedGeneration(), + ObservedGeneration: object.GetGeneration(), } meta.SetStatusCondition(&conditions, credRotCondition) object.SetConditions(conditions) } -func SetFailureConditions(operationType smClientTypes.OperationCategory, errorMessage string, object common.SAPBTPResource) { +func SetFailureConditions(operationType smClientTypes.OperationCategory, errorMessage string, object common.SAPBTPResource, isAsyncOperation bool) { var message string if operationType == smClientTypes.CREATE { message = fmt.Sprintf("%s create failed: %s", object.GetControllerName(), errorMessage) @@ -153,23 +161,27 @@ func SetFailureConditions(operationType smClientTypes.OperationCategory, errorMe reason = object.GetConditions()[0].Reason } + observedGen := object.GetGeneration() + if isAsyncOperation { + observedGen = getLastObservedGen(object) + } conditions := object.GetConditions() lastOpCondition := metav1.Condition{ Type: common.ConditionSucceeded, Status: metav1.ConditionFalse, Reason: reason, Message: message, - ObservedGeneration: object.GetObservedGeneration(), + ObservedGeneration: observedGen, } - meta.SetStatusCondition(&conditions, lastOpCondition) - failedCondition := metav1.Condition{ Type: common.ConditionFailed, Status: metav1.ConditionTrue, Reason: reason, Message: message, - ObservedGeneration: object.GetObservedGeneration(), + ObservedGeneration: observedGen, } + + meta.SetStatusCondition(&conditions, lastOpCondition) meta.SetStatusCondition(&conditions, failedCondition) meta.SetStatusCondition(&conditions, getReadyCondition(object)) @@ -180,15 +192,14 @@ func MarkAsNonTransientError(ctx context.Context, k8sClient client.Client, opera log := GetLogger(ctx) errMsg := err.Error() log.Info(fmt.Sprintf("operation %s of %s encountered a non transient error %s, setting failure conditions", operationType, object.GetControllerName(), errMsg)) - SetFailureConditions(operationType, errMsg, object) - object.SetObservedGeneration(object.GetGeneration()) + SetFailureConditions(operationType, errMsg, object, false) return ctrl.Result{}, UpdateStatus(ctx, k8sClient, object) } func MarkAsTransientError(ctx context.Context, k8sClient client.Client, operationType smClientTypes.OperationCategory, err error, object common.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) log.Info(fmt.Sprintf("operation %s of %s encountered a transient error %s, retrying operation :)", operationType, object.GetControllerName(), err.Error())) - SetInProgressConditions(ctx, operationType, err.Error(), object) + SetInProgressConditions(ctx, operationType, err.Error(), object, false) if updateErr := UpdateStatus(ctx, k8sClient, object); updateErr != nil { return ctrl.Result{}, updateErr } @@ -198,7 +209,7 @@ func MarkAsTransientError(ctx context.Context, k8sClient client.Client, operatio // blocked condition marks to the user that action from his side is required, this is considered as in progress operation func SetBlockedCondition(ctx context.Context, message string, object common.SAPBTPResource) { - SetInProgressConditions(ctx, common.Unknown, message, object) + SetInProgressConditions(ctx, common.Unknown, message, object, false) lastOpCondition := meta.FindStatusCondition(object.GetConditions(), common.ConditionSucceeded) lastOpCondition.Reason = common.Blocked } @@ -229,3 +240,12 @@ func getReadyCondition(object common.SAPBTPResource) metav1.Condition { return metav1.Condition{Type: common.ConditionReady, Status: status, Reason: reason} } + +func getLastObservedGen(object common.SAPBTPResource) int64 { + conditions := object.GetConditions() + cond := meta.FindStatusCondition(conditions, common.ConditionSucceeded) + if cond != nil { + return cond.ObservedGeneration + } + return 0 +} diff --git a/internal/utils/condition_utils_test.go b/internal/utils/condition_utils_test.go index a044eaf7..db278169 100644 --- a/internal/utils/condition_utils_test.go +++ b/internal/utils/condition_utils_test.go @@ -120,7 +120,7 @@ var _ = Describe("Condition Utils", func() { It("should set in-progress conditions", func() { resource = getBinding() - SetInProgressConditions(ctx, smClientTypes.CREATE, "Pending", resource) + SetInProgressConditions(ctx, smClientTypes.CREATE, "Pending", resource, false) // Add assertions to check the state of the resource after calling SetInProgressConditions Expect(resource.GetConditions()).ToNot(BeEmpty()) @@ -133,7 +133,7 @@ var _ = Describe("Condition Utils", func() { operationType := smClientTypes.CREATE resource = getBinding() - SetSuccessConditions(operationType, resource) + SetSuccessConditions(operationType, resource, false) // Add assertions to check the state of the resource after calling SetSuccessConditions Expect(resource.GetConditions()).ToNot(BeEmpty()) @@ -160,7 +160,7 @@ var _ = Describe("Condition Utils", func() { It("should set failure conditions", func() { operationType := smClientTypes.CREATE errorMessage := "Operation failed" - SetFailureConditions(operationType, errorMessage, resource) + SetFailureConditions(operationType, errorMessage, resource, false) Expect(resource.GetConditions()).ToNot(BeEmpty()) Expect(meta.IsStatusConditionPresentAndEqual(resource.GetConditions(), common.ConditionReady, metav1.ConditionFalse)).To(BeTrue()) }) @@ -304,6 +304,34 @@ var _ = Describe("Condition Utils", func() { Expect(result).Should(BeFalse()) }) }) + + Context("getLastObservedGen", func() { + It("should return the last observed generation from the conditions", func() { + resource := &v1.ServiceBinding{ + Status: v1.ServiceBindingStatus{ + Conditions: []metav1.Condition{ + { + Type: common.ConditionSucceeded, + Status: metav1.ConditionTrue, + ObservedGeneration: 5, + }, + }, + }, + } + + Expect(getLastObservedGen(resource)).To(Equal(int64(5))) + }) + + It("should return 0 if the ConditionSucceeded condition is not present", func() { + resource := &v1.ServiceBinding{ + Status: v1.ServiceBindingStatus{ + Conditions: []metav1.Condition{}, + }, + } + + Expect(getLastObservedGen(resource)).To(Equal(int64(0))) + }) + }) }) func getBinding() *v1.ServiceBinding {