Merge pull request #139 from SAP/fix_service_instance_deletion

fix[112]: service instance deletion without secret

Author: Lasserich

internal/controller/serviceinstance/controller.go

@@ -135,8 +135,9 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 
 	// Check if the external resource exists
 	guid := meta.GetExternalName(cr)
-	r, err := serviceinstance.GetByIDOrSpec(ctx, c.serviceinstance, guid, cr.Spec.ForProvider)
 
+	// Normal (non‑deletion) observe path.
+	r, err := serviceinstance.GetByIDOrSpec(ctx, c.serviceinstance, guid, cr.Spec.ForProvider)
 	if err != nil {
 		if clients.ErrorIsNotFound(err) {
 			return managed.ExternalObservation{ResourceExists: false}, nil
@@ -157,6 +158,13 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 	// Update atProvider from the retrieved the service instance
 	serviceinstance.UpdateObservation(&cr.Status.AtProvider, r)
 
+	// If the CR is marked for deletion we stop normal observe logic.
+	// We report "resource exists" so Crossplane will call Delete() next.
+	// (Delete() will handle a "not found" case safely, so we don't check again here.)
+	if meta.WasDeleted(mg) {
+		return managed.ExternalObservation{ResourceExists: true}, nil
+	}
+
 	switch r.LastOperation.State {
 	case v1alpha1.LastOperationInitial, v1alpha1.LastOperationInProgress:
 		// Set the CR to unavailable and signal that the reconciler should not update the resource
@@ -189,17 +197,13 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 				return managed.ExternalObservation{ResourceExists: true}, errors.Wrap(err, errGetParameters)
 			}
 			cr.Status.AtProvider.Credentials = iSha256(cred)
-
 			credentialsUpToDate = jsonContain(cred, desiredCredentials)
 		} else {
 			desiredHash := iSha256(desiredCredentials)
-
 			credentialsUpToDate = bytes.Equal(desiredHash, cr.Status.AtProvider.Credentials)
 		}
-
 		// Check if the credentials in the spec match the credentials in the external resource
 		upToDate := credentialsUpToDate && serviceinstance.IsUpToDate(&cr.Spec.ForProvider, r)
-
 		return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: upToDate}, nil
 	default:
 		// should never reach here

internal/controller/serviceinstance/controller_test.go

@@ -83,6 +83,13 @@ func withDriftDetection(d bool) modifier {
 	}
 }
 
+func withDeletionTimestamp() modifier {
+	return func(r *v1alpha1.ServiceInstance) {
+		ts := metav1.Now()
+		r.ObjectMeta.DeletionTimestamp = &ts
+	}
+}
+
 func serviceInstance(typ string, m ...modifier) *v1alpha1.ServiceInstance {
 	r := &v1alpha1.ServiceInstance{
 		ObjectMeta: metav1.ObjectMeta{
@@ -379,6 +386,46 @@ func TestObserve(t *testing.T) {
 				return m
 			},
 		},
+		"DeletionFastPath_Exists": {
+			args: args{
+				mg: serviceInstance("managed",
+					withExternalName(guid),
+					withSpace(spaceGUID),
+					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
+					withDeletionTimestamp(), // triggers meta.WasDeleted short-circuit
+				),
+			},
+			want: want{
+				mg: serviceInstance("managed",
+					withExternalName(guid),
+					withSpace(spaceGUID),
+					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
+					withDeletionTimestamp(),
+					// Status updated by UpdateObservation before early return
+					withStatus(v1alpha1.ServiceInstanceObservation{ID: &guid, ServicePlan: &servicePlan}),
+				),
+				// Early return only sets ResourceExists: true
+				obs: managed.ExternalObservation{ResourceExists: true},
+				err: nil,
+			},
+			service: func() *fake.MockServiceInstance {
+				m := &fake.MockServiceInstance{}
+				m.On("Get", guid).Return(
+					// LastOperationFailed + Create would have produced ResourceExists:false in the normal path,
+					// proving we exited early.
+					&fake.NewServiceInstance("managed").
+						SetName(name).
+						SetGUID(guid).
+						SetServicePlan(servicePlan).
+						SetLastOperation(v1alpha1.LastOperationCreate, v1alpha1.LastOperationFailed).
+						ServiceInstance,
+					nil,
+				)
+				// Fallback shouldn't be called, keep safe default.
+				m.On("Single").Return(fake.ServiceInstanceNil, fake.ErrNoResultReturned)
+				return m
+			},
+		},
 		"DriftDetectionLoop": {
 			args: args{
 				mg: serviceInstance("managed", withExternalName(guid), withSpace(spaceGUID), withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}), withParameters("{\"foo\":\"bar\", \"baz\": 1}"), withDriftDetection(true)),

test/e2e/cloudfoundry_services_test.go

@@ -56,7 +56,7 @@ func TestCloudFoundryServices(t *testing.T) {
 		},
 	)
 
-	var steps = [...]string{"space", "service_instance", "ups", "scb_key"}
+	var steps = [...]string{"space", "service_instance", "ups", "ups_no_credentials", "scb_key"}
 	for _, name := range steps {
 		ft, ok := feats[name]
 		if !ok {