Skip to content

Commit

Permalink
Merge pull request #745 from ravilr/externalClient_disconnect
Browse files Browse the repository at this point in the history 
Disconnect() on per reconcile loop scoped external client
  • Loading branch information
@bobh66
bobh66 authored Jul 22, 2024
2 parents ecb6cee + 84ee48a commit 1e7193e
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 26 deletions.
33 changes: 29 additions & 4 deletions pkg/reconciler/managed/reconciler.go
View file
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
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

0 comments on commit 1e7193e

Please sign in to comment.