Improve ClusterAccess library behavior

[Diaphteiros]

What would you like to be added:
The ClusterAccess library can be improved in the following ways:

• Currently, only a single AccessRequest or ClusterRequest is deleted, then the library returns and requests a requeue to wait for the deletion. This should instead delete all AccessRequests in the first step and then all ClusterRequests together, not only a single one.
• Calling the library's Reconcile method during deletion is problematic at the moment: In theory, it should be called beginning of the reconciliation to ensure that the access to the clusters is available in order to undeploy stuff. However, at the end of the reconciliation, ReconcileDelete should be called, which then removes some resources (AccessRequests first) and requests a requeue. If then Reconcile is called again, it will restore the already deleted AccessRequest, causing a loop of infinite reconciliations.
  • It is possible to figure out whether the AccessRequest has already been deleted, but this is not intuitive.

Solution Proposal:
@reshnm proposed the following approach to solve the deletion problem: During the regular Reconcile, AccessRequests are created with a finalizer. This means that if Reconcile is called during deletion, it can recognize that via the resource's DeletionTimestamp and not update the AccessRequest. During ReconcileDelete, the finalizer is only removed when it is the only one remaining on the AccessRequest. This ensures that the corresponding resources have already been deleted by the responsible ClusterProvider.
We apply the same logic when deleting ClusterRequests.

[Diaphteiros]

@robertgraeff @reshnm I tried to implement what we discussed yesterday, but it doesn't really work, unfortunately.

Our idea was to keep AccessReqest and ClusterRequest resources alive with a finalizer during the deletion. This way, one could call Reconcile without it restoring already deleted resources, because it recognizes the resource being in deletion.
The problem is that one call of ReconcileDelete then has to remove this 'last finalizer' from multiple resources at the same time. If any of these finalizer deletion operations fail, this would result in an error, which causes the current request to be requeued. Assuming the next Reconciliation calls Reconcile again - because our goal was to make it possible to call this during deletion - it will then re-create the resources where the finalizer deletion worked and not update the ones where it didn't work, because these are still in deletion.

To avoid this, the ClusterAccess Reconciler needs to know whether the resources belonging to a specific request are currently in deletion or not. However, it cannot derive this from the AccessRequests/ClusterRequests for the reason explained above (its impossible to remove all resources belonging to the same request atomically).
The only option I see is that the ClusterAccess Reconciler maintains an internal list of requests that are currently in deletion. 'in deletion' is here defined as 'ReconcileDelete has been called for this request but has not returned without being requeued'.

[robertgraeff]

@Diaphteiros @reshnm
We had also discussed the idea that the caller of the ClusterAccessReconciler (e.g. the service-provider-landscaper) could determine if the ClusterRequests/AccessRequests are already "in deletion mode". For this, I opened the PR fix: check if access requests are in deletion.

The PR has a function areAccessRequestsInDeletion to check for the deletion mode. Would it make sense to move this method to the ClusterAccessReconciler, so that it is available for all providers?

[Diaphteiros]

This sounds pretty much like what I had in mind: The ClusterAccessReconciler has an internal set where it stores the reconcile.Requests for which ReconcileDelete has been called but not yet returned without error or requeuing. Calling Reconcile on a request which is in this set won't do anything then (at least not re-deploy anything).