From 6df5539f5781d946d0ed96b607288cb00092a67d Mon Sep 17 00:00:00 2001 From: vaspahomov Date: Fri, 1 Oct 2021 13:22:44 +0500 Subject: [PATCH] Add Disconnect call in Reconcile Signed-off-by: vaspahomov --- pkg/reconciler/managed/reconciler.go | 80 +++++++++++++++++++++-- pkg/reconciler/managed/reconciler_test.go | 25 +++++++ 2 files changed, 100 insertions(+), 5 deletions(-) diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index e8793c56e..93e555961 100644 --- 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" @@ -60,7 +61,7 @@ const ( // Event reasons. const ( - reasonCannotConnect event.Reason = "CannotConnectToProvider" + reasonCannotDisconnect event.Reason = "CannotDisconnectToProvider" reasonCannotInitialize event.Reason = "CannotInitializeManagedResource" reasonCannotResolveRefs event.Reason = "CannotResolveResourceReferences" reasonCannotObserve event.Reason = "CannotObserveExternalResource" @@ -187,6 +188,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 do nothing. Never returns error. +func NopDisconnect(_ context.Context) error { + return nil +} + +// closableExternalConnector is wrap ExternalConnecter in closableExternalConnector +// with nop Disconnect. +type closableExternalConnector struct { + c ExternalConnecter + d ExternalDisconnecter +} + +// Connect call ExternalConnecter's Connect +func (c *closableExternalConnector) Connect(ctx context.Context, mg resource.Managed) (ExternalClient, error) { + return c.c.Connect(ctx, mg) +} + +// Disconnect call ExternalDisconnecter's Disconnect +func (c *closableExternalConnector) Disconnect(ctx context.Context) error { + return c.d.Disconnect(ctx) +} + +// NewExternalConnectorClosable wraps ExternalConnecter in ExternalConnecterClosable +func NewExternalConnectorClosable(c ExternalConnecter, d ExternalDisconnecter) ExternalConnecterClosable { + return &closableExternalConnector{c, d} +} + +// An ExternalConnecterClosable produces a new ExternalClient given the supplied +// Managed resource. +type ExternalConnecterClosable 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 +238,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 +319,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 +463,12 @@ func defaultMRManaged(m manager.Manager) mrManaged { } type mrExternal struct { - ExternalConnecter + ExternalConnecterClosable } func defaultMRExternal() mrExternal { return mrExternal{ - ExternalConnecter: &NopConnecter{}, + ExternalConnecterClosable: &NopConnecter{}, } } @@ -456,7 +511,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.ExternalConnecterClosable = NewExternalConnectorClosable(c, ExternalDisconnectorFn(NopDisconnect)) + } +} + +// WithExternalConnecterClosable specifies how the Reconciler should connect and disconnect to the API +// used to sync and delete external resources. +func WithExternalConnecterClosable(c ExternalConnecterClosable) ReconcilerOption { + return func(r *Reconciler) { + r.external.ExternalConnecterClosable = c } } @@ -673,7 +736,7 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc // condition. If not, we requeue explicitly, which will trigger // backoff. log.Debug("Cannot connect to provider", "error", err) - record.Event(managed, event.Warning(reasonCannotConnect, err)) + record.Event(managed, event.Warning(reasonCannotDisconnect, err)) managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileConnect))) return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -926,6 +989,13 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + if err := r.external.Disconnect(ctx); err != nil { + log.Debug("Cannot disconnect from provider", "error", err) + record.Event(managed, event.Warning(reasonCannotDisconnect, err)) + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileDisconnect))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + } + // We've successfully updated our external resource. Per the below issue // nothing will notify us if and when the external resource we manage // changes, so we requeue a speculative reconcile after the specified poll diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 03d8423b5..9975f7403 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -286,6 +286,31 @@ func TestReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: true}}, }, + "ExternalDisconnectError": { + reason: "Errors disconnecting from the provider should trigger a requeue after a short wait.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil), + MockStatusUpdate: test.NewMockStatusUpdateFn(nil), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithExternalConnecterClosable(NewExternalConnectorClosable( + ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { + return nil, 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{