Merge pull request #296 from vaspahomov/feature/disconnect-in-reconcile

Add Disconnect call in Reconcile
crossplane · Dec 2, 2021 · 5cc9857 · 5cc9857
2 parents 21928d2 + a5ff67d
commit 5cc9857
Show file tree

Hide file tree

Showing 2 changed files with 151 additions and 3 deletions.
diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go
@@ -52,6 +52,7 @@ const (
 	errUpdateManagedAnnotations = "cannot update managed resource annotations"
 	errCreateIncomplete         = "cannot determine creation result - remove the " + meta.AnnotationKeyExternalCreatePending + " annotation if it is safe to proceed"
 	errReconcileConnect         = "connect failed"
+	errReconcileDisconnect      = "disconnect failed"
 	errReconcileObserve         = "observe failed"
 	errReconcileCreate          = "create failed"
 	errReconcileUpdate          = "update failed"
@@ -61,6 +62,7 @@ const (
 // Event reasons.
 const (
 	reasonCannotConnect       event.Reason = "CannotConnectToProvider"
+	reasonCannotDisconnect    event.Reason = "CannotDisconnectFromProvider"
 	reasonCannotInitialize    event.Reason = "CannotInitializeManagedResource"
 	reasonCannotResolveRefs   event.Reason = "CannotResolveResourceReferences"
 	reasonCannotObserve       event.Reason = "CannotObserveExternalResource"
@@ -187,6 +189,46 @@ type ExternalConnecter interface {
 	Connect(ctx context.Context, mg resource.Managed) (ExternalClient, error)
 }
 
+// An ExternalDisconnecter disconnect from provider.
+type ExternalDisconnecter interface {
+	// Disconnect from the provider and close an ExternalClient.
+	Disconnect(ctx context.Context) error
+}
+
+// NopDisconnect does nothing. Never returns error.
+func NopDisconnect(_ context.Context) error {
+	return nil
+}
+
+// externalConnectDisconnecter is wrap ExternalConnecter in externalConnectDisconnecter
+// with nop Disconnect.
+type externalConnectDisconnecter struct {
+	c ExternalConnecter
+	d ExternalDisconnecter
+}
+
+// Connect call ExternalConnecter's Connect
+func (c *externalConnectDisconnecter) Connect(ctx context.Context, mg resource.Managed) (ExternalClient, error) {
+	return c.c.Connect(ctx, mg)
+}
+
+// Disconnect call ExternalDisconnecter's Disconnect
+func (c *externalConnectDisconnecter) Disconnect(ctx context.Context) error {
+	return c.d.Disconnect(ctx)
+}
+
+// NewExternalConnectDisconnecter wraps ExternalConnecter in ExternalConnectDisconnecter
+func NewExternalConnectDisconnecter(c ExternalConnecter, d ExternalDisconnecter) ExternalConnectDisconnecter {
+	return &externalConnectDisconnecter{c, d}
+}
+
+// An ExternalConnectDisconnecter produces a new ExternalClient given the supplied
+// Managed resource.
+type ExternalConnectDisconnecter interface {
+	ExternalConnecter
+	ExternalDisconnecter
+}
+
 // An ExternalConnectorFn is a function that satisfies the ExternalConnecter
 // interface.
 type ExternalConnectorFn func(ctx context.Context, mg resource.Managed) (ExternalClient, error)
@@ -197,6 +239,15 @@ func (ec ExternalConnectorFn) Connect(ctx context.Context, mg resource.Managed)
 	return ec(ctx, mg)
 }
 
+// An ExternalDisconnectorFn is a function that satisfies the ExternalConnecter
+// interface.
+type ExternalDisconnectorFn func(ctx context.Context) error
+
+// Disconnect from provider and close an ExternalClient.
+func (ed ExternalDisconnectorFn) Disconnect(ctx context.Context) error {
+	return ed(ctx)
+}
+
 // An ExternalClient manages the lifecycle of an external resource.
 // None of the calls here should be blocking. All of the calls should be
 // idempotent. For example, Create call should not return AlreadyExists error
@@ -269,6 +320,11 @@ func (c *NopConnecter) Connect(_ context.Context, _ resource.Managed) (ExternalC
 	return &NopClient{}, nil
 }
 
+// Disconnect do nothing. It never returns an error.
+func (c *NopConnecter) Disconnect(_ context.Context) error {
+	return nil
+}
+
 // A NopClient does nothing.
 type NopClient struct{}
 
@@ -408,12 +464,12 @@ func defaultMRManaged(m manager.Manager) mrManaged {
 }
 
 type mrExternal struct {
-	ExternalConnecter
+	ExternalConnectDisconnecter
 }
 
 func defaultMRExternal() mrExternal {
 	return mrExternal{
-		ExternalConnecter: &NopConnecter{},
+		ExternalConnectDisconnecter: &NopConnecter{},
 	}
 }
 
@@ -456,7 +512,15 @@ func WithCreationGracePeriod(d time.Duration) ReconcilerOption {
 // used to sync and delete external resources.
 func WithExternalConnecter(c ExternalConnecter) ReconcilerOption {
 	return func(r *Reconciler) {
-		r.external.ExternalConnecter = c
+		r.external.ExternalConnectDisconnecter = NewExternalConnectDisconnecter(c, ExternalDisconnectorFn(NopDisconnect))
+	}
+}
+
+// WithExternalConnectDisconnecter specifies how the Reconciler should connect and disconnect to the API
+// used to sync and delete external resources.
+func WithExternalConnectDisconnecter(c ExternalConnectDisconnecter) ReconcilerOption {
+	return func(r *Reconciler) {
+		r.external.ExternalConnectDisconnecter = c
 	}
 }
 
@@ -677,6 +741,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 		managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileConnect)))
 		return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
 	}
+	defer func() {
+		if disconnectErr := r.external.Disconnect(ctx); disconnectErr != nil {
+			log.Debug("Cannot disconnect from provider", "error", disconnectErr)
+			record.Event(managed, event.Warning(reasonCannotDisconnect, disconnectErr))
+			managed.SetConditions(xpv1.ReconcileError(errors.Wrap(disconnectErr, errReconcileDisconnect)))
+		}
+	}()
 
 	observation, err := external.Observe(externalCtx, managed)
 	if err != nil {

diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go
@@ -286,6 +286,83 @@ func TestReconciler(t *testing.T) {
 			},
 			want: want{result: reconcile.Result{Requeue: true}},
 		},
+		"ExternalDisconnectError": {
+			reason: "Error disconnecting from the provider should not trigger requeue.",
+			args: args{
+				m: &fake.Manager{
+					Client: &test.MockClient{
+						MockGet: test.NewMockGetFn(nil),
+						MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+							want := &fake.Managed{}
+							want.SetConditions(xpv1.ReconcileSuccess())
+							if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
+								reason := "A successful no-op reconcile should be reported as a conditioned status."
+								t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff)
+							}
+							return nil
+						}),
+					},
+					Scheme: fake.SchemeWith(&fake.Managed{}),
+				},
+				mg: resource.ManagedKind(fake.GVK(&fake.Managed{})),
+				o: []ReconcilerOption{
+					WithInitializers(),
+					WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
+					WithExternalConnectDisconnecter(NewExternalConnectDisconnecter(
+						ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) {
+							c := &ExternalClientFns{
+								ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
+									return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
+								},
+							}
+							return c, nil
+						}), ExternalDisconnectorFn(func(_ context.Context) error {
+							return errBoom
+						})),
+					),
+					WithConnectionPublishers(),
+					WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}),
+				},
+			},
+			want: want{result: reconcile.Result{RequeueAfter: defaultpollInterval}},
+		},
+		"ExternalObserveErrorDisconnectError": {
+			reason: "Errors disconnecting from the provider after error observing the external resource should trigger a requeue after a short wait and return error.",
+			args: args{
+				m: &fake.Manager{
+					Client: &test.MockClient{
+						MockGet: test.NewMockGetFn(nil),
+						MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+							want := &fake.Managed{}
+							want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileObserve)))
+							if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" {
+								reason := "Errors observing the managed resource should be reported as a conditioned status."
+								t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff)
+							}
+							return nil
+						}),
+					},
+					Scheme: fake.SchemeWith(&fake.Managed{}),
+				},
+				mg: resource.ManagedKind(fake.GVK(&fake.Managed{})),
+				o: []ReconcilerOption{
+					WithInitializers(),
+					WithExternalConnectDisconnecter(NewExternalConnectDisconnecter(
+						ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) {
+							c := &ExternalClientFns{
+								ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
+									return ExternalObservation{}, errBoom
+								},
+							}
+							return c, nil
+						}), ExternalDisconnectorFn(func(_ context.Context) error {
+							return errBoom
+						})),
+					),
+				},
+			},
+			want: want{result: reconcile.Result{Requeue: true}},
+		},
 		"ExternalObserveError": {
 			reason: "Errors observing the external resource should trigger a requeue after a short wait.",
 			args: args{