diff --git a/api/v1/serviceinstance_validating_webhook.go b/api/v1/serviceinstance_validating_webhook.go index 54dacc53..92f02723 100644 --- a/api/v1/serviceinstance_validating_webhook.go +++ b/api/v1/serviceinstance_validating_webhook.go @@ -50,7 +50,7 @@ func SetAllowMultipleTenants(isAllowed bool) { func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err error) { serviceinstancelog.Info("validate create", "name", si.Name) if !allowMultipleTenants && len(si.Spec.SubaccountID) > 0 { - serviceinstancelog.Error(fmt.Errorf("invalid subaccountID property"), "the operator installation does not allow multiple subaccunts") + serviceinstancelog.Error(fmt.Errorf("invalid subaccountID property"), "the operator installation does not allow multiple subaccounts") return nil, fmt.Errorf("setting the subaccountID property is not allowed") } return nil, nil @@ -58,6 +58,7 @@ func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err er func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) { serviceinstancelog.Info("validate update", "name", si.Name) + oldInstance := old.(*ServiceInstance) if oldInstance.Spec.SubaccountID != si.Spec.SubaccountID { return nil, fmt.Errorf("changing the subaccountID for an existing instance is not allowed") diff --git a/config/samples/services_v1_serviceinstance.yaml b/config/samples/services_v1_serviceinstance.yaml index e7c65cbd..bd19f598 100644 --- a/config/samples/services_v1_serviceinstance.yaml +++ b/config/samples/services_v1_serviceinstance.yaml @@ -4,4 +4,4 @@ metadata: name: sample-instance-1 spec: serviceOfferingName: service-manager - servicePlanName: subaccount-admin + servicePlanName: subaccount-audit diff --git a/controllers/base_controller.go b/controllers/base_controller.go index c9e49cd7..5944dd80 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -202,7 +202,7 @@ func setInProgressConditions(ctx context.Context, operationType smClientTypes.Op Status: metav1.ConditionFalse, Reason: getConditionReason(operationType, smClientTypes.INPROGRESS), Message: message, - ObservedGeneration: object.GetGeneration(), + ObservedGeneration: object.GetObservedGeneration(), } meta.SetStatusCondition(&conditions, lastOpCondition) meta.SetStatusCondition(&conditions, getReadyCondition(object)) @@ -230,14 +230,14 @@ func setSuccessConditions(operationType smClientTypes.OperationCategory, object Status: metav1.ConditionTrue, Reason: getConditionReason(operationType, smClientTypes.SUCCEEDED), Message: message, - ObservedGeneration: object.GetGeneration(), + ObservedGeneration: object.GetObservedGeneration(), } readyCondition := metav1.Condition{ Type: api.ConditionReady, Status: metav1.ConditionTrue, Reason: Provisioned, Message: message, - ObservedGeneration: object.GetGeneration(), + ObservedGeneration: object.GetObservedGeneration(), } meta.SetStatusCondition(&conditions, lastOpCondition) meta.SetStatusCondition(&conditions, readyCondition) @@ -257,7 +257,7 @@ func setCredRotationInProgressConditions(reason, message string, object api.SAPB Status: metav1.ConditionTrue, Reason: reason, Message: message, - ObservedGeneration: object.GetGeneration(), + ObservedGeneration: object.GetObservedGeneration(), } meta.SetStatusCondition(&conditions, credRotCondition) object.SetConditions(conditions) @@ -286,7 +286,7 @@ func setFailureConditions(operationType smClientTypes.OperationCategory, errorMe Status: metav1.ConditionFalse, Reason: reason, Message: message, - ObservedGeneration: object.GetGeneration(), + ObservedGeneration: object.GetObservedGeneration(), } meta.SetStatusCondition(&conditions, lastOpCondition) @@ -295,7 +295,7 @@ func setFailureConditions(operationType smClientTypes.OperationCategory, errorMe Status: metav1.ConditionTrue, Reason: reason, Message: message, - ObservedGeneration: object.GetGeneration(), + ObservedGeneration: object.GetObservedGeneration(), } meta.SetStatusCondition(&conditions, failedCondition) meta.SetStatusCondition(&conditions, getReadyCondition(object)) @@ -310,7 +310,7 @@ func setBlockedCondition(ctx context.Context, message string, object api.SAPBTPR lastOpCondition.Reason = Blocked } -func isDelete(object metav1.ObjectMeta) bool { +func isMarkedForDeletion(object metav1.ObjectMeta) bool { return !object.DeletionTimestamp.IsZero() } @@ -400,5 +400,5 @@ func getReadyCondition(object api.SAPBTPResource) metav1.Condition { reason = Provisioned } - return metav1.Condition{Type: api.ConditionReady, Status: status, Reason: reason, ObservedGeneration: object.GetGeneration()} + return metav1.Condition{Type: api.ConditionReady, Status: status, Reason: reason} } diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 97192640..f0800c27 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -92,7 +92,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - if isDelete(serviceBinding.ObjectMeta) { + if isMarkedForDeletion(serviceBinding.ObjectMeta) { return r.delete(ctx, serviceBinding, serviceInstance.Spec.SubaccountID) } @@ -348,7 +348,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, serviceBinding *ser freshStatus := servicesv1.ServiceBindingStatus{ Conditions: serviceBinding.GetConditions(), } - if isDelete(serviceBinding.ObjectMeta) { + if isMarkedForDeletion(serviceBinding.ObjectMeta) { freshStatus.BindingID = serviceBinding.Status.BindingID } serviceBinding.Status = freshStatus @@ -451,7 +451,7 @@ func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *servic if !isFailed(binding) { if _, err := r.getSecret(ctx, binding.Namespace, binding.Spec.SecretName); err != nil { - if apierrors.IsNotFound(err) && !isDelete(binding.ObjectMeta) { + if apierrors.IsNotFound(err) && !isMarkedForDeletion(binding.ObjectMeta) { log.Info(fmt.Sprintf("secret not found recovering binding %s", binding.Name)) binding.Status.BindingID = "" binding.Status.Ready = metav1.ConditionFalse @@ -911,7 +911,7 @@ func (r *ServiceBindingReconciler) recover(ctx context.Context, serviceBinding * } func isStaleServiceBinding(binding *servicesv1.ServiceBinding) bool { - if isDelete(binding.ObjectMeta) { + if isMarkedForDeletion(binding.ObjectMeta) { return false } @@ -995,7 +995,7 @@ func bindingAlreadyOwnedByInstance(instance *servicesv1.ServiceInstance, binding } func serviceNotUsable(instance *servicesv1.ServiceInstance) bool { - if isDelete(instance.ObjectMeta) { + if isMarkedForDeletion(instance.ObjectMeta) { return true } if len(instance.Status.Conditions) != 0 { diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 22e909d4..2e4a7401 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -69,7 +69,6 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, client.IgnoreNotFound(err) } serviceInstance = serviceInstance.DeepCopy() - serviceInstance.SetObservedGeneration(serviceInstance.Generation) if len(serviceInstance.GetConditions()) == 0 { err := r.init(ctx, serviceInstance) @@ -78,7 +77,17 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } - if isDelete(serviceInstance.ObjectMeta) { + if isFinalState(ctx, serviceInstance) { + if len(serviceInstance.Status.HashedSpec) == 0 { + updateHashedSpecValue(serviceInstance) + return ctrl.Result{}, r.Client.Status().Update(ctx, serviceInstance) + } + return ctrl.Result{}, nil + } + + if isMarkedForDeletion(serviceInstance.ObjectMeta) { + // delete updates the generation + serviceInstance.SetObservedGeneration(serviceInstance.Generation) return r.deleteInstance(ctx, serviceInstance) } @@ -95,14 +104,8 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } - if isFinalState(serviceInstance) { - log.Info(fmt.Sprintf("Final state, spec did not change, and we are not in progress - ignoring... Generation is - %v", serviceInstance.Generation)) - if len(serviceInstance.Status.HashedSpec) == 0 { - updateHashedSpecValue(serviceInstance) - return ctrl.Result{}, r.Client.Status().Update(ctx, serviceInstance) - } - return ctrl.Result{}, nil - } + 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, serviceInstance.Spec.SubaccountID) if err != nil { @@ -342,7 +345,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, serviceInstance *s setInProgressConditions(ctx, serviceInstance.Status.OperationType, statusErr.Error(), serviceInstance) // if failed to read operation status we cleanup the status to trigger re-sync from SM freshStatus := servicesv1.ServiceInstanceStatus{Conditions: serviceInstance.GetConditions(), ObservedGeneration: serviceInstance.Generation} - if isDelete(serviceInstance.ObjectMeta) { + if isMarkedForDeletion(serviceInstance.ObjectMeta) { freshStatus.InstanceID = serviceInstance.Status.InstanceID } serviceInstance.Status = freshStatus @@ -512,24 +515,37 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte return ctrl.Result{Requeue: isTransient}, r.updateStatus(ctx, object) } -func isFinalState(serviceInstance *servicesv1.ServiceInstance) bool { - // succeeded condition represents last operation, and it is constantly synced with generation - succeededCondition := meta.FindStatusCondition(serviceInstance.GetConditions(), api.ConditionSucceeded) - if succeededCondition == nil || succeededCondition.ObservedGeneration != serviceInstance.Generation { +func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance) bool { + log := GetLogger(ctx) + if 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())) return false } // succeeded=false for current generation, and without failed=true --> transient error retry if isInProgress(serviceInstance) { + log.Info("instance is not in final state, sync operation is in progress") return false } - // for cases of instance update while polling for create/update - if getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec { + if sharingUpdateRequired(serviceInstance) { + log.Info("instance is not in final state, need to sync sharing status") + if len(serviceInstance.Status.HashedSpec) == 0 { + updateHashedSpecValue(serviceInstance) + } return false } - return !sharingUpdateRequired(serviceInstance) + log.Info(fmt.Sprintf("instance is in final state (generation: %d)", serviceInstance.Generation)) + return true } // TODO unit test diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index a97e6728..e062aa10 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -34,7 +34,6 @@ const ( testNamespace = "ic-test-namespace" fakeOfferingName = "offering-a" fakePlanName = "plan-a" - testSubaccountID = "subaccountID" ) var _ = Describe("ServiceInstance controller", func() { @@ -669,7 +668,6 @@ var _ = Describe("ServiceInstance controller", func() { deleteInstance(ctx, serviceInstance, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, DeleteInProgress, "") }) - When("polling ends with success", func() { BeforeEach(func() { fakeClient.StatusReturns(&smclientTypes.Operation{ @@ -742,6 +740,28 @@ var _ = Describe("ServiceInstance controller", func() { }) }) + Describe("full reconcile", func() { + When("instance hashedSpec is not initialized", func() { + BeforeEach(func() { + serviceInstance = createInstance(ctx, instanceSpec, true) + }) + It("should not send update request and update the hashed spec", func() { + hashed := serviceInstance.Status.HashedSpec + serviceInstance.Status.HashedSpec = "" + Expect(k8sClient.Status().Update(ctx, serviceInstance)).To(Succeed()) + + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{Name: serviceInstance.Name, Namespace: serviceInstance.Namespace}, serviceInstance) + if err != nil { + return false + } + cond := meta.FindStatusCondition(serviceInstance.GetConditions(), api.ConditionSucceeded) + return serviceInstance.Status.HashedSpec == hashed && cond != nil && cond.Reason == Created + }, timeout, interval).Should(BeTrue()) + }) + }) + }) + Context("Recovery", func() { When("instance exists in SM", func() { recoveredInstance := smclientTypes.ServiceInstance{ @@ -1042,7 +1062,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) When("Succeeded is false", func() { @@ -1068,11 +1088,34 @@ var _ = Describe("ServiceInstance controller", func() { }, } instance.SetGeneration(1) - Expect(isFinalState(instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) }) + When("async operation in progress", func() { + It("should return false", func() { + var instance = &v1.ServiceInstance{ + Status: v1.ServiceInstanceStatus{ + Conditions: []metav1.Condition{ + { + Type: api.ConditionReady, + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + }, + }, + HashedSpec: "929e78f4449f8036ce39da3cc3e7eaea", + OperationURL: "/operations/somepollingurl", + ObservedGeneration: 2, + }, + Spec: v1.ServiceInstanceSpec{ + ExternalName: "name", + }} + instance.SetGeneration(2) + Expect(isFinalState(ctx, instance)).To(BeFalse()) + }) + }) + When("in progress", func() { It("should return false", func() { var instance = &v1.ServiceInstance{ @@ -1095,7 +1138,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1121,7 +1164,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1153,7 +1196,7 @@ var _ = Describe("ServiceInstance controller", func() { Shared: pointer.Bool(true), }} instance.SetGeneration(2) - Expect(isFinalState(instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1177,14 +1220,15 @@ var _ = Describe("ServiceInstance controller", func() { Status: metav1.ConditionTrue, }, }, - HashedSpec: "929e78f4449f8036ce39da3cc3e7eaea", + HashedSpec: "929e78f4449f8036ce39da3cc3e7eaea", + ObservedGeneration: 2, }, Spec: v1.ServiceInstanceSpec{ ExternalName: "name", Shared: pointer.Bool(true), }} instance.SetGeneration(2) - Expect(isFinalState(instance)).To(BeTrue()) + Expect(isFinalState(ctx, instance)).To(BeTrue()) }) }) })