Skip to content

Commit 75088d8

Browse files
author
Mykhailo Pechkurov
committed
Update the logic for the observe function for a deletion fast-path
1 parent b50cf12 commit 75088d8

2 files changed

Lines changed: 190 additions & 21 deletions

File tree

‎internal/controller/serviceinstance/controller.go‎

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,34 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
135135

136136
// Check if the external resource exists
137137
guid := meta.GetExternalName(cr)
138-
r, err := serviceinstance.GetByIDOrSpec(ctx, c.serviceinstance, guid, cr.Spec.ForProvider)
139138

139+
// Deletion fast‑path:
140+
// If the object has a deletion timestamp we only need to know whether the external
141+
// resource still exists; we skip secret resolution, drift detection and state logic.
142+
if meta.WasDeleted(cr) {
143+
r, err := serviceinstance.GetByIDOrSpec(ctx, c.serviceinstance, guid, cr.Spec.ForProvider)
144+
if err != nil {
145+
if clients.ErrorIsNotFound(err) {
146+
// Already gone externally; no delete call needed.
147+
return managed.ExternalObservation{ResourceExists: false}, nil
148+
}
149+
return managed.ExternalObservation{}, errors.Wrap(err, errGet)
150+
}
151+
if r == nil {
152+
return managed.ExternalObservation{ResourceExists: false}, nil
153+
}
154+
if guid != r.GUID {
155+
meta.SetExternalName(cr, r.GUID)
156+
if err := c.kube.Update(ctx, cr); err != nil {
157+
return managed.ExternalObservation{}, errors.Wrap(err, errUpdateCR)
158+
}
159+
}
160+
// We deliberately report up-to-date; reconciler will invoke Delete().
161+
return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
162+
}
163+
164+
// Normal (non‑deletion) observe path.
165+
r, err := serviceinstance.GetByIDOrSpec(ctx, c.serviceinstance, guid, cr.Spec.ForProvider)
140166
if err != nil {
141167
if clients.ErrorIsNotFound(err) {
142168
return managed.ExternalObservation{ResourceExists: false}, nil
@@ -179,7 +205,7 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
179205
var credentialsUpToDate bool
180206
desiredCredentials, err := extractCredentialSpec(ctx, c.kube, cr.Spec.ForProvider)
181207
if err != nil {
182-
return managed.ExternalObservation{}, checkDeletion(cr, errors.Wrap(err, errSecret))
208+
return managed.ExternalObservation{}, errors.Wrap(err, errSecret)
183209
}
184210
// If parameter drift detection is enable, get actual credentials from the service instance
185211
if cr.Spec.EnableParameterDriftDetection {
@@ -189,17 +215,13 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
189215
return managed.ExternalObservation{ResourceExists: true}, errors.Wrap(err, errGetParameters)
190216
}
191217
cr.Status.AtProvider.Credentials = iSha256(cred)
192-
193218
credentialsUpToDate = jsonContain(cred, desiredCredentials)
194219
} else {
195220
desiredHash := iSha256(desiredCredentials)
196-
197221
credentialsUpToDate = bytes.Equal(desiredHash, cr.Status.AtProvider.Credentials)
198222
}
199-
200223
// Check if the credentials in the spec match the credentials in the external resource
201224
upToDate := credentialsUpToDate && serviceinstance.IsUpToDate(&cr.Spec.ForProvider, r)
202-
203225
return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: upToDate}, nil
204226
default:
205227
// should never reach here
@@ -209,18 +231,6 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
209231
}
210232
}
211233

212-
// checkDeletion returns nil if the ServiceInstance is already marked for deletion.
213-
// Why: Avoid noisy retries while Kubernetes finalizers finish cleanup.
214-
// Params:
215-
// cr - ServiceInstance under reconciliation
216-
// err - original error (returned only when not deleting)
217-
func checkDeletion(cr *v1alpha1.ServiceInstance, err error) error {
218-
if cr.GetDeletionTimestamp() != nil {
219-
return nil
220-
}
221-
return err
222-
}
223-
224234
// Create attempts to create the external resource.
225235
func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.ExternalCreation, error) {
226236
cr, ok := mg.(*v1alpha1.ServiceInstance)

‎internal/controller/serviceinstance/controller_test.go‎

Lines changed: 162 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,13 @@ func withDriftDetection(d bool) modifier {
8383
}
8484
}
8585

86+
func withDeletionTimestamp() modifier {
87+
return func(r *v1alpha1.ServiceInstance) {
88+
ts := metav1.Now()
89+
r.ObjectMeta.DeletionTimestamp = &ts
90+
}
91+
}
92+
8693
func serviceInstance(typ string, m ...modifier) *v1alpha1.ServiceInstance {
8794
r := &v1alpha1.ServiceInstance{
8895
ObjectMeta: metav1.ObjectMeta{
@@ -379,6 +386,153 @@ func TestObserve(t *testing.T) {
379386
return m
380387
},
381388
},
389+
"DeletionFastPath_Exists": {
390+
args: args{
391+
mg: serviceInstance("managed",
392+
withExternalName(guid),
393+
withSpace(spaceGUID),
394+
withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
395+
withDeletionTimestamp(), // mark as deleting
396+
),
397+
},
398+
want: want{
399+
mg: serviceInstance("managed",
400+
withExternalName(guid),
401+
withSpace(spaceGUID),
402+
withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
403+
withDeletionTimestamp(),
404+
),
405+
obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true},
406+
err: nil,
407+
},
408+
service: func() *fake.MockServiceInstance {
409+
m := &fake.MockServiceInstance{}
410+
m.On("Get", guid).Return(
411+
&fake.NewServiceInstance("managed").
412+
SetName(name).
413+
SetGUID(guid).
414+
SetServicePlan(servicePlan).
415+
SetLastOperation(v1alpha1.LastOperationCreate, v1alpha1.LastOperationSucceeded).ServiceInstance,
416+
nil,
417+
)
418+
// Single() normally not called if Get by GUID succeeds; keep a safe default.
419+
m.On("Single").Return(fake.ServiceInstanceNil, fake.ErrNoResultReturned)
420+
return m
421+
},
422+
},
423+
"DeletionFastPath_NotFound": {
424+
args: args{
425+
mg: serviceInstance("managed",
426+
withExternalName(guid),
427+
withSpace(spaceGUID),
428+
withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
429+
withDeletionTimestamp(),
430+
),
431+
},
432+
want: want{
433+
mg: serviceInstance("managed",
434+
withExternalName(guid),
435+
withSpace(spaceGUID),
436+
withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
437+
withDeletionTimestamp(),
438+
),
439+
obs: managed.ExternalObservation{ResourceExists: false},
440+
err: nil,
441+
},
442+
service: func() *fake.MockServiceInstance {
443+
m := &fake.MockServiceInstance{}
444+
m.On("Get", guid).Return(
445+
fake.ServiceInstanceNil,
446+
fake.ErrNoResultReturned,
447+
)
448+
m.On("Single").Return(
449+
fake.ServiceInstanceNil,
450+
fake.ErrNoResultReturned,
451+
)
452+
return m
453+
},
454+
},
455+
"DeletionFastPath_AdoptExternalName": {
456+
args: args{
457+
mg: serviceInstance("managed",
458+
withExternalName("not-guid"), // force mismatch
459+
withSpace(spaceGUID),
460+
withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
461+
withDeletionTimestamp(), // trigger deletion fast-path
462+
),
463+
},
464+
want: want{
465+
mg: serviceInstance("managed",
466+
withExternalName(guid), // updated annotation expected
467+
withSpace(spaceGUID),
468+
withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
469+
withDeletionTimestamp(),
470+
),
471+
obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true},
472+
err: nil,
473+
},
474+
service: func() *fake.MockServiceInstance {
475+
m := &fake.MockServiceInstance{}
476+
m.On("Get", "not-guid").Return(
477+
fake.ServiceInstanceNil,
478+
fake.ErrNoResultReturned, // fall back to Single()
479+
)
480+
m.On("Single").Return(
481+
&fake.NewServiceInstance("managed").
482+
SetName(name).
483+
SetGUID(guid).
484+
SetServicePlan(servicePlan).
485+
SetLastOperation(v1alpha1.LastOperationCreate, v1alpha1.LastOperationSucceeded).
486+
ServiceInstance,
487+
nil,
488+
)
489+
return m
490+
},
491+
kube: &test.MockClient{
492+
MockUpdate: test.NewMockUpdateFn(nil), // successful update
493+
},
494+
},
495+
"DeletionFastPath_AdoptExternalNameUpdateError": {
496+
args: args{
497+
mg: serviceInstance("managed",
498+
withExternalName("not-guid"),
499+
withSpace(spaceGUID),
500+
withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
501+
withDeletionTimestamp(),
502+
),
503+
},
504+
want: want{
505+
mg: serviceInstance("managed",
506+
// meta.SetExternalName already applied in-memory even though Update fails
507+
withExternalName(guid),
508+
withSpace(spaceGUID),
509+
withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}),
510+
withDeletionTimestamp(),
511+
),
512+
obs: managed.ExternalObservation{}, // aborted with error
513+
err: errors.Wrap(errBoom, errUpdateCR), // wrapped update error
514+
},
515+
service: func() *fake.MockServiceInstance {
516+
m := &fake.MockServiceInstance{}
517+
m.On("Get", "not-guid").Return(
518+
fake.ServiceInstanceNil,
519+
fake.ErrNoResultReturned,
520+
)
521+
m.On("Single").Return(
522+
&fake.NewServiceInstance("managed").
523+
SetName(name).
524+
SetGUID(guid).
525+
SetServicePlan(servicePlan).
526+
SetLastOperation(v1alpha1.LastOperationCreate, v1alpha1.LastOperationSucceeded).
527+
ServiceInstance,
528+
nil,
529+
)
530+
return m
531+
},
532+
kube: &test.MockClient{
533+
MockUpdate: test.NewMockUpdateFn(errBoom), // force failure -> cover error wrap
534+
},
535+
},
382536
"DriftDetectionLoop": {
383537
args: args{
384538
mg: serviceInstance("managed", withExternalName(guid), withSpace(spaceGUID), withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan}), withParameters("{\"foo\":\"bar\", \"baz\": 1}"), withDriftDetection(true)),
@@ -449,10 +603,15 @@ func TestObserve(t *testing.T) {
449603
for n, tc := range cases {
450604
t.Run(n, func(t *testing.T) {
451605
t.Logf("Testing: %s", t.Name())
452-
c := &external{
453-
kube: &test.MockClient{
606+
607+
kubeClient := tc.kube
608+
if kubeClient == nil {
609+
kubeClient = &test.MockClient{
454610
MockUpdate: test.NewMockUpdateFn(nil),
455-
},
611+
}
612+
}
613+
c := &external{
614+
kube: kubeClient,
456615
serviceinstance: &serviceinstance.Client{
457616
ServiceInstance: tc.service(),
458617
Job: nil,

0 commit comments

Comments
 (0)