diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 31fa76e43..693c92f89 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" @@ -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 index 03d8423b5..525cfe52b 100644 --- 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{