Simplify the solution

Author: Mykhailo Pechkurov

internal/controller/serviceinstance/controller.go

@@ -136,31 +136,6 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 	// Check if the external resource exists
 	guid := meta.GetExternalName(cr)
 
-	// Deletion fast‑path:
-	// If the object has a deletion timestamp we only need to know whether the external
-	// resource still exists; we skip secret resolution, drift detection and state logic.
-	if meta.WasDeleted(cr) {
-		r, err := serviceinstance.GetByIDOrSpec(ctx, c.serviceinstance, guid, cr.Spec.ForProvider)
-		if err != nil {
-			if clients.ErrorIsNotFound(err) {
-				// Already gone externally; no delete call needed.
-				return managed.ExternalObservation{ResourceExists: false}, nil
-			}
-			return managed.ExternalObservation{}, errors.Wrap(err, errGet)
-		}
-		if r == nil {
-			return managed.ExternalObservation{ResourceExists: false}, nil
-		}
-		if guid != r.GUID {
-			meta.SetExternalName(cr, r.GUID)
-			if err := c.kube.Update(ctx, cr); err != nil {
-				return managed.ExternalObservation{}, errors.Wrap(err, errUpdateCR)
-			}
-		}
-		// We deliberately report up-to-date; reconciler will invoke Delete().
-		return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
-	}
-
 	// Normal (non‑deletion) observe path.
 	r, err := serviceinstance.GetByIDOrSpec(ctx, c.serviceinstance, guid, cr.Spec.ForProvider)
 	if err != nil {
@@ -183,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

internal/controller/serviceinstance/controller_test.go

@@ -392,41 +392,7 @@ func TestObserve(t *testing.T) {
 					withExternalName(guid),
 					withSpace(spaceGUID),
 					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
-					withDeletionTimestamp(), // mark as deleting
-				),
-			},
-			want: want{
-				mg: serviceInstance("managed",
-					withExternalName(guid),
-					withSpace(spaceGUID),
-					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
-					withDeletionTimestamp(),
-				),
-				obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true},
-				err: nil,
-			},
-			service: func() *fake.MockServiceInstance {
-				m := &fake.MockServiceInstance{}
-				m.On("Get", guid).Return(
-					&fake.NewServiceInstance("managed").
-						SetName(name).
-						SetGUID(guid).
-						SetServicePlan(servicePlan).
-						SetLastOperation(v1alpha1.LastOperationCreate, v1alpha1.LastOperationSucceeded).ServiceInstance,
-					nil,
-				)
-				// Single() normally not called if Get by GUID succeeds; keep a safe default.
-				m.On("Single").Return(fake.ServiceInstanceNil, fake.ErrNoResultReturned)
-				return m
-			},
-		},
-		"DeletionFastPath_NotFound": {
-			args: args{
-				mg: serviceInstance("managed",
-					withExternalName(guid),
-					withSpace(spaceGUID),
-					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
-					withDeletionTimestamp(),
+					withDeletionTimestamp(), // triggers meta.WasDeleted short-circuit
 				),
 			},
 			want: want{
@@ -435,103 +401,30 @@ func TestObserve(t *testing.T) {
 					withSpace(spaceGUID),
 					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
 					withDeletionTimestamp(),
+					// Status updated by UpdateObservation before early return
+					withStatus(v1alpha1.ServiceInstanceObservation{ID: &guid, ServicePlan: &servicePlan}),
 				),
-				obs: managed.ExternalObservation{ResourceExists: false},
+				// 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(
-					fake.ServiceInstanceNil,
-					fake.ErrNoResultReturned,
-				)
-				m.On("Single").Return(
-					fake.ServiceInstanceNil,
-					fake.ErrNoResultReturned,
-				)
-				return m
-			},
-		},
-		"DeletionFastPath_AdoptExternalName": {
-			args: args{
-				mg: serviceInstance("managed",
-					withExternalName("not-guid"), // force mismatch
-					withSpace(spaceGUID),
-					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
-					withDeletionTimestamp(), // trigger deletion fast-path
-				),
-			},
-			want: want{
-				mg: serviceInstance("managed",
-					withExternalName(guid), // updated annotation expected
-					withSpace(spaceGUID),
-					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
-					withDeletionTimestamp(),
-				),
-				obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true},
-				err: nil,
-			},
-			service: func() *fake.MockServiceInstance {
-				m := &fake.MockServiceInstance{}
-				m.On("Get", "not-guid").Return(
-					fake.ServiceInstanceNil,
-					fake.ErrNoResultReturned, // fall back to Single()
-				)
-				m.On("Single").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.LastOperationSucceeded).
-						ServiceInstance,
-					nil,
-				)
-				return m
-			},
-			kube: &test.MockClient{
-				MockUpdate: test.NewMockUpdateFn(nil), // successful update
-			},
-		},
-		"DeletionFastPath_AdoptExternalNameUpdateError": {
-			args: args{
-				mg: serviceInstance("managed",
-					withExternalName("not-guid"),
-					withSpace(spaceGUID),
-					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
-					withDeletionTimestamp(),
-				),
-			},
-			want: want{
-				mg: serviceInstance("managed",
-					// meta.SetExternalName already applied in-memory even though Update fails
-					withExternalName(guid),
-					withSpace(spaceGUID),
-					withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
-					withDeletionTimestamp(),
-				),
-				obs: managed.ExternalObservation{},     // aborted with error
-				err: errors.Wrap(errBoom, errUpdateCR), // wrapped update error
-			},
-			service: func() *fake.MockServiceInstance {
-				m := &fake.MockServiceInstance{}
-				m.On("Get", "not-guid").Return(
-					fake.ServiceInstanceNil,
-					fake.ErrNoResultReturned,
-				)
-				m.On("Single").Return(
-					&fake.NewServiceInstance("managed").
-						SetName(name).
-						SetGUID(guid).
-						SetServicePlan(servicePlan).
-						SetLastOperation(v1alpha1.LastOperationCreate, v1alpha1.LastOperationSucceeded).
+						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
 			},
-			kube: &test.MockClient{
-				MockUpdate: test.NewMockUpdateFn(errBoom), // force failure -> cover error wrap
-			},
 		},
 		"DriftDetectionLoop": {
 			args: args{
@@ -603,15 +496,10 @@ func TestObserve(t *testing.T) {
 	for n, tc := range cases {
 		t.Run(n, func(t *testing.T) {
 			t.Logf("Testing: %s", t.Name())
-
-			kubeClient := tc.kube
-			if kubeClient == nil {
-				kubeClient = &test.MockClient{
-					MockUpdate: test.NewMockUpdateFn(nil),
-				}
-			}
 			c := &external{
-				kube: kubeClient,
+				kube: &test.MockClient{
+					MockUpdate: test.NewMockUpdateFn(nil),
+				},
 				serviceinstance: &serviceinstance.Client{
 					ServiceInstance: tc.service(),
 					Job:             nil,