MGDAPI-3467 tenant deletion support

[MStokluska]

• adds support of deleting tenants via CR
  • https://issues.redhat.com/browse/MGDAPI-3467
  • https://issues.redhat.com/browse/THREESCALE-6051

Verification:

For RHOAM:

• install RHOAM locally from this branch: WIP MGDAPI-3177 wip pr for making 3scale cluster scoped integr8ly/integreatly-operator#2409 but replace the manifests/3scale/0.9.0 values for containerImage and image from quay.io/austincunningham/3scale-operator:cluster-scoped to quay.io/mstoklus/3scale-operator:v0.12.0
• wait for install to complete
• create tenant via CR
• confirm tenant got created in 3scale
• delete tenant CR
• confirm tenant has been marked for deletion in 3scale

[maksymvavilov]

Installed RHOAM using the quay.io/mstoklus/3scale-operator:v0.9.0 3scale image:

Created a tenant CR:

And watched a tenant being created in the 3scale:

After deleting a tenant CR the tenant was scheduled for a deletion:

[maksymvavilov]

Other failed checks are related to the k8s.io/utils licence approval 🤔 Anyway, the code looks good and performs well, hence

/lgtm

[eguzki]

IMO the tenant deletion support should be done at the tenant_controller.go on the entrypoint of reconciler.

The finalizer pattern used across other controllers we are working on is:

func (r *TenantReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {                                  
    _ = context.Background()                                                                                   
    reqLogger := r.Log.WithValues("tenant", req.NamespacedName)                                                
                                                                                                               
    // Fetch the Tenant instance                                                                               
    tenantR := &capabilitiesv1alpha1.Tenant{}                                                                  
    err := r.Client.Get(context.TODO(), req.NamespacedName, tenantR)                                           
    if err != nil {                                                                                            
        if errors.IsNotFound(err) {                                                                            
            // Request object not found, could have been deleted after reconcile request.                      
            // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. 
            // Return and don't requeue                                                                        
            reqLogger.Info("Tenant resource not found")                                                        
            return ctrl.Result{}, nil                                                                          
        }                                                                                                      
        // Error reading the object - requeue the request.                                                     
        return ctrl.Result{}, err                                                                              
    }       
    // Tenant has been marked for deletion                                                      
    if tenantR.GetDeletionTimestamp() != nil && controllerutil.ContainsFinalizer(tenantR, tenantFinalizer) {
         // your deletion handling
         .....
         
         // 
         //Remove finalizer and update the object.             
         controllerutil.RemoveFinalizer(tenantR, tenantFinalizer)   
         err = r.Client.Update(ctx, tenantR)                     
         logger.Info("Removing finalizer", "error", err)       
         return ctrl.Result{Requeue: true}, err                
     }

       // Ignore deleted resources, this can happen when foregroundDeletion is enabled                                     
 // https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/#foreground-cascading-deletion      
        if tenantR.GetDeletionTimestamp() != nil {                                                                             
              return ctrl.Result{}, nil                                                                                       
         } 
 
          if !controllerutil.ContainsFinalizer(tenantR, tenantFinalizer) {                    
                   controllerutil.AddFinalizer(tenantR, tenantFinalizer)                           
                    err := r.Client.Update(ctx, tenantR)                                         
                    logger.Info("Adding finalizer", "error", err)                              
                    return ctrl.Result{Requeue: true}, err                                     
            }                                                                              
            ...
                                                                                                                           

[MStokluska]

@eguzki hey, sorry for all the updates, I was trying to make the codeClimate a bit happier!

[MStokluska]

@eguzki I've addressed all your comments and explained the reasoning behind having it done in a certain way in the first place, I would appreciate a review again.
The only thing that I question is around the requeue if by any chance the 3scale api call fails on removing the tenant. But it might be easier to go through the scenarios on call.
Thanks for the reviews :)

[eguzki]

Overall looks good

I left some comments

[eguzki]

nitpick: this method does not add anything, does it?

[MStokluska]

Potentially it could be moved, although I was thinking it would be nice to have it in a single file, I was thinking of possibility moving the tenant create here too, but happy to remove it directly if that's what you preffer.

[MStokluska]

Updated

[eguzki]

Looks good

I think we should tackle the issue of cascade deletion of 3scale tenant objects (DeveloperUser, DeveloperAccount, Products, Backends, ActiveDocs) in another PR.

I am aware that the code coverage in the capabilites code is near to zero, but it is prepared to have tests (https://github.com/3scale/3scale-operator/blob/62f241c29eb4136e055269cf31ab7062c16e6bfa/controllers/capabilities/suite_test.go). If you are willing to add some tests, feel free.

Regarding upstream doc, I think it would be nice to have a note in the capabilities doc about the new feature. Also will be used by tech writers team for the official doc.

[codeclimate]

Code Climate has analyzed commit fa0538e and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2

View more on Code Climate.

[eguzki]

good job 🎖️