Disconnect ExternalClient

[denniskniep]

Problem

I am currently struggling with appropriately disconnecting the external client after it was used.

My Reconciler is created with WithExternalConnectDisconnecter here

I implemented the connect and disconnect method. But disconnect does not have any further specific parameters, so it seems to be intended to close the whole Connector. But there is only one Connector per Reconciler. Meaning if the Connectorś disconnect method is called and then the Reconciler.Reconcile gets another call it will call Connect() on an already Disconnected connector.

crossplane-runtime/pkg/reconciler/managed/reconciler.go

Lines 892 to 912 in fc03661

	external, err := r.external.Connect(externalCtx, managed)
	if err != nil {
	// We'll usually hit this case if our Provider or its secret are missing
	// or invalid. If this is first time we encounter this issue we'll be
	// requeued implicitly when we update our status with the new error
	// condition. If not, we requeue explicitly, which will trigger
	// backoff.
	log.Debug("Cannot connect to provider", "error", err)
	if kerrors.IsConflict(err) {
	return reconcile.Result{Requeue: true}, nil
	}
	record.Event(managed, event.Warning(reasonCannotConnect, err))
	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 err := r.external.Disconnect(ctx); err != nil {
	log.Debug("Cannot disconnect from provider", "error", err)
	record.Event(managed, event.Warning(reasonCannotDisconnect, err))
	}
	}()

Currently the Disconnect call is:
r.external.Disconnect(ctx);

Proposal

Wouldn´t it be better to pass either external as a parameter to disconnect
r.external.Disconnect(ctx, external);

or directly call Disconnect on external (without r.)
external.Disconnect(ctx)

Benefit is, that we can close precisely an reconcile call scoped client. And not disconnect the whole connector of the reconciler, though it is still in use and might run another reconcile