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

Disconnect() on per reconcile loop scoped external client #745

Merged
merged 1 commit into from
Jul 22, 2024
+129 −26
Merged

Disconnect() on per reconcile loop scoped external client #745

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
33 changes: 29 additions & 4 deletions pkg/reconciler/managed/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,9 @@ type ExternalConnecter interface {
}

// An ExternalDisconnecter disconnects from a provider.
//
// Deprecated: Please use Disconnect() on the ExternalClient for disconnecting
// from the provider.
type ExternalDisconnecter interface {
// Disconnect from the provider and close the ExternalClient.
Disconnect(ctx context.Context) error
Expand Down Expand Up @@ -330,15 +333,22 @@ type ExternalClient interface {
// Delete the external resource upon deletion of its associated Managed
// resource. Called when the managed resource has been deleted.
Delete(ctx context.Context, mg resource.Managed) error

// Disconnect from the provider and close the ExternalClient.
// Called at the end of reconcile loop. An ExternalClient not requiring
// to explicitly disconnect to cleanup it resources, can provide a no-op
// implementation which just return nil.
Disconnect(ctx context.Context) error
}

// ExternalClientFns are a series of functions that satisfy the ExternalClient
// interface.
type ExternalClientFns struct {
ObserveFn func(ctx context.Context, mg resource.Managed) (ExternalObservation, error)
CreateFn func(ctx context.Context, mg resource.Managed) (ExternalCreation, error)
UpdateFn func(ctx context.Context, mg resource.Managed) (ExternalUpdate, error)
DeleteFn func(ctx context.Context, mg resource.Managed) error
ObserveFn func(ctx context.Context, mg resource.Managed) (ExternalObservation, error)
CreateFn func(ctx context.Context, mg resource.Managed) (ExternalCreation, error)
UpdateFn func(ctx context.Context, mg resource.Managed) (ExternalUpdate, error)
DeleteFn func(ctx context.Context, mg resource.Managed) error
DisconnectFn func(ctx context.Context) error
}

// Observe the external resource the supplied Managed resource represents, if
Expand All @@ -365,6 +375,11 @@ func (e ExternalClientFns) Delete(ctx context.Context, mg resource.Managed) erro
return e.DeleteFn(ctx, mg)
}

// Disconnect the external client.
func (e ExternalClientFns) Disconnect(ctx context.Context) error {
return e.DisconnectFn(ctx)
}

// A NopConnecter does nothing.
type NopConnecter struct{}

Expand Down Expand Up @@ -394,6 +409,9 @@ func (c *NopClient) Update(_ context.Context, _ resource.Managed) (ExternalUpdat
// Delete does nothing. It never returns an error.
func (c *NopClient) Delete(_ context.Context, _ resource.Managed) error { return nil }

// Disconnect does nothing. It never returns an error.
func (c *NopClient) Disconnect(_ context.Context) error { return nil }

// An ExternalObservation is the result of an observation of an external
// resource.
type ExternalObservation struct {
Expand Down Expand Up @@ -603,6 +621,8 @@ func WithExternalConnecter(c ExternalConnecter) ReconcilerOption {

// WithExternalConnectDisconnecter specifies how the Reconciler should connect and disconnect to the API
// used to sync and delete external resources.
//
// Deprecated: Please use Disconnect() on the ExternalClient for disconnecting from the provider.
func WithExternalConnectDisconnecter(c ExternalConnectDisconnecter) ReconcilerOption {
return func(r *Reconciler) {
r.external.ExternalConnectDisconnecter = c
Expand Down Expand Up @@ -909,6 +929,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
log.Debug("Cannot disconnect from provider", "error", err)
record.Event(managed, event.Warning(reasonCannotDisconnect, err))
}

if err := external.Disconnect(ctx); err != nil {
log.Debug("Cannot disconnect from provider", "error", err)
record.Event(managed, event.Warning(reasonCannotDisconnect, err))
}
}()

observation, err := external.Observe(externalCtx, managed)
Expand Down
122 changes: 100 additions & 22 deletions pkg/reconciler/managed/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,17 +310,17 @@ func TestReconciler(t *testing.T) {
o: []ReconcilerOption{
WithInitializers(),
WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
WithExternalConnectDisconnecter(ExternalConnectDisconnecterFns{
ConnectFn: func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
c := &ExternalClientFns{
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
},
}
return c, nil
},
DisconnectFn: func(_ context.Context) error { return errBoom },
}),
WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
c := &ExternalClientFns{
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return errBoom
},
}
return c, nil
})),
WithConnectionPublishers(),
WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}),
},
Expand Down Expand Up @@ -353,6 +353,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{}, errBoom
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -381,6 +384,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: false}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -426,6 +432,9 @@ func TestReconciler(t *testing.T) {
DeleteFn: func(_ context.Context, _ resource.Managed) error {
return errBoom
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -471,6 +480,9 @@ func TestReconciler(t *testing.T) {
DeleteFn: func(_ context.Context, _ resource.Managed) error {
return nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -513,6 +525,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: false}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -558,6 +573,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: false}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -590,6 +608,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: false}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -692,6 +713,9 @@ func TestReconciler(t *testing.T) {
CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) {
return ExternalCreation{}, errBoom
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -735,6 +759,9 @@ func TestReconciler(t *testing.T) {
CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) {
return ExternalCreation{}, errBoom
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -781,6 +808,9 @@ func TestReconciler(t *testing.T) {
CreateFn: func(_ context.Context, _ resource.Managed) (ExternalCreation, error) {
return ExternalCreation{}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -825,6 +855,9 @@ func TestReconciler(t *testing.T) {
cd := ConnectionDetails{"create": []byte{}}
return ExternalCreation{ConnectionDetails: cd}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -907,6 +940,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true, ResourceLateInitialized: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -943,6 +979,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -973,6 +1012,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1013,6 +1055,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1048,6 +1093,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1096,6 +1144,9 @@ func TestReconciler(t *testing.T) {
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, errBoom
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1136,6 +1187,9 @@ func TestReconciler(t *testing.T) {
cd := ConnectionDetails{"update": []byte{}}
return ExternalUpdate{ConnectionDetails: cd}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1185,6 +1239,9 @@ func TestReconciler(t *testing.T) {
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1282,17 +1339,17 @@ func TestReconciler(t *testing.T) {
o: []ReconcilerOption{
WithInitializers(),
WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
WithExternalConnectDisconnecter(ExternalConnectDisconnecterFns{
ConnectFn: func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
c := &ExternalClientFns{
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
},
}
return c, nil
},
DisconnectFn: func(_ context.Context) error { return errBoom },
}),
WithExternalConnecter(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
c := &ExternalClientFns{
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
WithConnectionPublishers(),
WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}),
},
Expand Down Expand Up @@ -1439,6 +1496,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: false}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1478,6 +1538,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1522,6 +1585,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1652,6 +1718,9 @@ func TestReconciler(t *testing.T) {
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, errBoom
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1697,6 +1766,9 @@ func TestReconciler(t *testing.T) {
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1742,6 +1814,9 @@ func TestReconciler(t *testing.T) {
UpdateFn: func(_ context.Context, _ resource.Managed) (ExternalUpdate, error) {
return ExternalUpdate{}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down Expand Up @@ -1785,6 +1860,9 @@ func TestReconciler(t *testing.T) {
ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
return ExternalObservation{ResourceExists: true, ResourceUpToDate: true, ResourceLateInitialized: true}, nil
},
DisconnectFn: func(_ context.Context) error {
return nil
},
}
return c, nil
})),
Expand Down
Loading