Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tweak ExternalDisconnecter implementation #306

Merged
merged 1 commit into from
Dec 2, 2021
Merged

Tweak ExternalDisconnecter implementation #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 38 additions & 32 deletions pkg/reconciler/managed/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ 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"
Expand Down Expand Up @@ -189,37 +188,32 @@ type ExternalConnecter interface {
Connect(ctx context.Context, mg resource.Managed) (ExternalClient, error)
}

// An ExternalDisconnecter disconnect from provider.
// An ExternalDisconnecter disconnects from a provider.
type ExternalDisconnecter interface {
// Disconnect from the provider and close an ExternalClient.
// Disconnect from the provider and close the 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 {
// A NopDisconnecter converts an ExternalConnecter into an
// ExternalConnectDisconnecter with a no-op Disconnect method.
type NopDisconnecter struct {
negz marked this conversation as resolved.
Show resolved Hide resolved
c ExternalConnecter
d ExternalDisconnecter
}

// Connect call ExternalConnecter's Connect
func (c *externalConnectDisconnecter) Connect(ctx context.Context, mg resource.Managed) (ExternalClient, error) {
// Connect calls the underlying ExternalConnecter's Connect method.
func (c *NopDisconnecter) 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)
// Disconnect does nothing. It never returns an error.
func (c *NopDisconnecter) Disconnect(_ context.Context) error {
return nil
}

// NewExternalConnectDisconnecter wraps ExternalConnecter in ExternalConnectDisconnecter
func NewExternalConnectDisconnecter(c ExternalConnecter, d ExternalDisconnecter) ExternalConnectDisconnecter {
return &externalConnectDisconnecter{c, d}
// NewNopDisconnecter converts an ExternalConnecter into an
// ExternalConnectDisconnecter with a no-op Disconnect method.
func NewNopDisconnecter(c ExternalConnecter) ExternalConnectDisconnecter {
return &NopDisconnecter{c}
}

// An ExternalConnectDisconnecter produces a new ExternalClient given the supplied
Expand All @@ -243,11 +237,29 @@ func (ec ExternalConnectorFn) Connect(ctx context.Context, mg resource.Managed)
// interface.
type ExternalDisconnectorFn func(ctx context.Context) error

// Disconnect from provider and close an ExternalClient.
// Disconnect from provider and close the ExternalClient.
func (ed ExternalDisconnectorFn) Disconnect(ctx context.Context) error {
return ed(ctx)
}

// ExternalConnectDisconnecterFns are functions that satisfy the
// ExternalConnectDisconnecter interface.
type ExternalConnectDisconnecterFns struct {
ConnectFn func(ctx context.Context, mg resource.Managed) (ExternalClient, error)
DisconnectFn func(ctx context.Context) error
}

// Connect to the provider specified by the supplied managed resource and
// produce an ExternalClient.
func (fns ExternalConnectDisconnecterFns) Connect(ctx context.Context, mg resource.Managed) (ExternalClient, error) {
return fns.ConnectFn(ctx, mg)
}

// Disconnect from the provider and close the ExternalClient.
func (fns ExternalConnectDisconnecterFns) Disconnect(ctx context.Context) error {
return fns.DisconnectFn(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
Expand Down Expand Up @@ -320,11 +332,6 @@ 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{}

Expand Down Expand Up @@ -469,7 +476,7 @@ type mrExternal struct {

func defaultMRExternal() mrExternal {
return mrExternal{
ExternalConnectDisconnecter: &NopConnecter{},
ExternalConnectDisconnecter: NewNopDisconnecter(&NopConnecter{}),
}
}

Expand Down Expand Up @@ -512,7 +519,7 @@ func WithCreationGracePeriod(d time.Duration) ReconcilerOption {
// used to sync and delete external resources.
func WithExternalConnecter(c ExternalConnecter) ReconcilerOption {
return func(r *Reconciler) {
r.external.ExternalConnectDisconnecter = NewExternalConnectDisconnecter(c, ExternalDisconnectorFn(NopDisconnect))
r.external.ExternalConnectDisconnecter = NewNopDisconnecter(c)
}
}

Expand Down Expand Up @@ -742,10 +749,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
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)))
if err := r.external.Disconnect(ctx); err != nil {
log.Debug("Cannot disconnect from provider", "error", err)
record.Event(managed, event.Warning(reasonCannotDisconnect, err))
}
}()

Expand Down
48 changes: 5 additions & 43 deletions pkg/reconciler/managed/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,61 +308,23 @@ func TestReconciler(t *testing.T) {
o: []ReconcilerOption{
WithInitializers(),
WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
WithExternalConnectDisconnecter(NewExternalConnectDisconnecter(
ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) {
WithExternalConnectDisconnecter(ExternalConnectDisconnecterFns{
ConnectFn: 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
})),
),
},
DisconnectFn: 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{
Expand Down