Handle upgrade unable to delete duplicate set of resources

Upon uprade with custom CR name, was failing to cleanup the extra set of resources that are name dependent (context in #106). The issue was that we only give delete permissions to the fixed names, and hence we cant expect the operator to be able to delete the old duplicate set. Signed-off-by: Alex Kalenyuk <[email protected]>
kubevirt · Jun 17, 2021 · f1df3ba · f1df3ba
1 parent 1183615
commit f1df3ba
Show file tree

Hide file tree

Showing 7 changed files with 23 additions and 165 deletions.
diff --git a/deploy/operator.yaml b/deploy/operator.yaml
@@ -140,13 +140,6 @@ rules:
   - get
   - watch
   - create
-- apiGroups:
-  - apps
-  resources:
-  - daemonsets
-  resourceNames:
-  - hostpath-provisioner
-  verbs:
   - delete
   - update
 - apiGroups:
@@ -166,19 +159,8 @@ rules:
   - ""
   resources:
   - serviceaccounts
-  resourceNames:
-  - hostpath-provisioner-admin
   verbs:
   - '*'
-- apiGroups:
-  - ""
-  resources:
-  - serviceaccounts
-  verbs:
-  - list
-  - get
-  - watch
-  - create
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding

diff --git a/pkg/controller/hostpathprovisioner/controller_test.go b/pkg/controller/hostpathprovisioner/controller_test.go
@@ -893,6 +893,14 @@ func createServiceAccountWithNameThatDependsOnCr() *corev1.ServiceAccount {
 			Name:      "test-name-admin",
 			Namespace: "test-namespace",
 			Labels:    labels,
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: "hostpathprovisioner.kubevirt.io/test",
+					Kind:       "HostPathProvisioner",
+					Name:       "test-name",
+					UID:        "1234",
+				},
+			},
 		},
 	}
 }
diff --git a/pkg/controller/hostpathprovisioner/daemonset.go b/pkg/controller/hostpathprovisioner/daemonset.go
@@ -233,7 +233,12 @@ func (r *ReconcileHostPathProvisioner) getDuplicateDaemonSet(customCrName, names
 
 	for _, ds := range dsList.Items {
 		if ds.Name != MultiPurposeHostPathProvisionerName {
-			dups = append(dups, ds)
+			for _, ownerRef := range ds.OwnerReferences {
+				if ownerRef.Kind == "HostPathProvisioner" && ownerRef.Name == customCrName {
+					dups = append(dups, ds)
+					break
+				}
+			}
 		}
 	}
 

diff --git a/pkg/controller/hostpathprovisioner/rbac.go b/pkg/controller/hostpathprovisioner/rbac.go
@@ -22,38 +22,24 @@ import (
 	"reflect"
 
 	"github.com/go-logr/logr"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 	hostpathprovisionerv1 "kubevirt.io/hostpath-provisioner-operator/pkg/apis/hostpathprovisioner/v1beta1"
 )
 
 func (r *ReconcileHostPathProvisioner) reconcileClusterRoleBinding(reqLogger logr.Logger, cr *hostpathprovisionerv1.HostPathProvisioner, namespace string, recorder record.EventRecorder) (reconcile.Result, error) {
-	// Previous versions created resources with names that depend on the CR, whereas now, we have fixed names for those.
-	// We will remove those and have the next loop create the resources with fixed names so we don't end up with two sets of hpp resources.
-	dups, err := r.getDuplicateClusterRoleBinding(cr.Name)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
-	for _, dup := range dups {
-		if err := r.deleteClusterRoleBindingObject(dup.Name); err != nil {
-			return reconcile.Result{}, err
-		}
-	}
-
 	// Define a new ClusterRoleBinding object
 	desired := createClusterRoleBindingObject(namespace)
 	setLastAppliedConfiguration(desired)
 	// Check if this ClusterRoleBinding already exists
 	found := &rbacv1.ClusterRoleBinding{}
-	err = r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name}, found)
+	err := r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name}, found)
 	if err != nil && errors.IsNotFound(err) {
 		reqLogger.Info("Creating a new ClusterRoleBinding", "ClusterRoleBinding.Name", desired.Name)
 		recorder.Event(cr, corev1.EventTypeNormal, createResourceStart, fmt.Sprintf(createMessageStart, desired, desired.Name))
@@ -142,25 +128,13 @@ func (r *ReconcileHostPathProvisioner) deleteClusterRoleBindingObject(name strin
 }
 
 func (r *ReconcileHostPathProvisioner) reconcileClusterRole(reqLogger logr.Logger, cr *hostpathprovisionerv1.HostPathProvisioner, recorder record.EventRecorder) (reconcile.Result, error) {
-	// Previous versions created resources with names that depend on the CR, whereas now, we have fixed names for those.
-	// We will remove those and have the next loop create the resources with fixed names so we don't end up with two sets of hpp resources.
-	dups, err := r.getDuplicateClusterRole(cr.Name)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
-	for _, dup := range dups {
-		if err := r.deleteClusterRoleObject(dup.Name); err != nil {
-			return reconcile.Result{}, err
-		}
-	}
-
 	// Define a new ClusterRole object
 	desired := createClusterRoleObject()
 	setLastAppliedConfiguration(desired)
 
 	// Check if this ClusterRole already exists
 	found := &rbacv1.ClusterRole{}
-	err = r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name}, found)
+	err := r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name}, found)
 	if err != nil && errors.IsNotFound(err) {
 		reqLogger.Info("Creating a new ClusterRole", "ClusterRole.Name", desired.Name)
 		recorder.Event(cr, corev1.EventTypeNormal, createResourceStart, fmt.Sprintf(createMessageStart, desired, desired.Name))
@@ -305,51 +279,3 @@ func (r *ReconcileHostPathProvisioner) deleteClusterRoleObject(name string) erro
 
 	return nil
 }
-
-// getDuplicateClusterRole will give us duplicate ClusterRoles from a previous version if they exist.
-// This is possible from a previous HPP version where the resources (DaemonSet, RBAC) were named depending on the CR, whereas now, we have fixed names for those.
-func (r *ReconcileHostPathProvisioner) getDuplicateClusterRole(customCrName string) ([]rbacv1.ClusterRole, error) {
-	crList := &rbacv1.ClusterRoleList{}
-	dups := make([]rbacv1.ClusterRole, 0)
-
-	ls, err := labels.Parse(fmt.Sprintf("k8s-app in (%s, %s)", MultiPurposeHostPathProvisionerName, customCrName))
-	if err != nil {
-		return dups, err
-	}
-	lo := &client.ListOptions{LabelSelector: ls}
-	if err := r.client.List(context.TODO(), crList, lo); err != nil {
-		return dups, err
-	}
-
-	for _, cr := range crList.Items {
-		if cr.Name != MultiPurposeHostPathProvisionerName {
-			dups = append(dups, cr)
-		}
-	}
-
-	return dups, nil
-}
-
-// getDuplicateClusterRoleBinding will give us duplicate ClusterRoleBindings from a previous version if they exist.
-// This is possible from a previous HPP version where the resources (DaemonSet, RBAC) were named depending on the CR, whereas now, we have fixed names for those.
-func (r *ReconcileHostPathProvisioner) getDuplicateClusterRoleBinding(customCrName string) ([]rbacv1.ClusterRoleBinding, error) {
-	crbList := &rbacv1.ClusterRoleBindingList{}
-	dups := make([]rbacv1.ClusterRoleBinding, 0)
-
-	ls, err := labels.Parse(fmt.Sprintf("k8s-app in (%s, %s)", MultiPurposeHostPathProvisionerName, customCrName))
-	if err != nil {
-		return dups, err
-	}
-	lo := &client.ListOptions{LabelSelector: ls}
-	if err := r.client.List(context.TODO(), crbList, lo); err != nil {
-		return dups, err
-	}
-
-	for _, crb := range crbList.Items {
-		if crb.Name != MultiPurposeHostPathProvisionerName {
-			dups = append(dups, crb)
-		}
-	}
-
-	return dups, nil
-}
diff --git a/pkg/controller/hostpathprovisioner/scc.go b/pkg/controller/hostpathprovisioner/scc.go
@@ -25,13 +25,11 @@ import (
 	secv1 "github.com/openshift/api/security/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/tools/record"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	hostpathprovisionerv1 "kubevirt.io/hostpath-provisioner-operator/pkg/apis/hostpathprovisioner/v1beta1"
 )
@@ -41,25 +39,13 @@ func (r *ReconcileHostPathProvisioner) reconcileSecurityContextConstraints(reqLo
 		return reconcile.Result{}, err
 	}
 
-	// Previous versions created resources with names that depend on the CR, whereas now, we have fixed names for those.
-	// We will remove those and have the next loop create the resources with fixed names so we don't end up with two sets of hpp resources.
-	dups, err := r.getDuplicateSecurityContextConstraints(cr.Name)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
-	for _, dup := range dups {
-		if err := r.deleteSCC(dup.Name); err != nil {
-			return reconcile.Result{}, err
-		}
-	}
-
 	// Define a new SecurityContextConstraints object
 	desired := createSecurityContextConstraintsObject(namespace)
 	setLastAppliedConfiguration(desired)
 
 	// Check if this SecurityContextConstraints already exists
 	found := &secv1.SecurityContextConstraints{}
-	err = r.client.Get(context.TODO(), types.NamespacedName{Name: MultiPurposeHostPathProvisionerName}, found)
+	err := r.client.Get(context.TODO(), types.NamespacedName{Name: MultiPurposeHostPathProvisionerName}, found)
 	if err != nil && errors.IsNotFound(err) {
 		reqLogger.Info("Creating a new SecurityContextConstraints", "SecurityContextConstraints.Name", desired.Name)
 		recorder.Event(cr, corev1.EventTypeNormal, createResourceStart, fmt.Sprintf(createMessageStart, desired, desired.Name))
@@ -181,27 +167,3 @@ func (r *ReconcileHostPathProvisioner) checkSCCUsed() (bool, error) {
 	}
 	return true, nil
 }
-
-// getDuplicateSecurityContextConstraints will give us duplicate SecurityContextConstraints from a previous version if they exist.
-// This is possible from a previous HPP version where the resources (DaemonSet, RBAC) were named depending on the CR, whereas now, we have fixed names for those.
-func (r *ReconcileHostPathProvisioner) getDuplicateSecurityContextConstraints(customCrName string) ([]secv1.SecurityContextConstraints, error) {
-	sccList := &secv1.SecurityContextConstraintsList{}
-	dups := make([]secv1.SecurityContextConstraints, 0)
-
-	ls, err := labels.Parse(fmt.Sprintf("k8s-app in (%s, %s)", MultiPurposeHostPathProvisionerName, customCrName))
-	if err != nil {
-		return dups, err
-	}
-	lo := &client.ListOptions{LabelSelector: ls}
-	if err := r.client.List(context.TODO(), sccList, lo); err != nil {
-		return dups, err
-	}
-
-	for _, scc := range sccList.Items {
-		if scc.Name != MultiPurposeHostPathProvisionerName {
-			dups = append(dups, scc)
-		}
-	}
-
-	return dups, nil
-}
diff --git a/pkg/controller/hostpathprovisioner/serviceaccount.go b/pkg/controller/hostpathprovisioner/serviceaccount.go
@@ -154,7 +154,12 @@ func (r *ReconcileHostPathProvisioner) getDuplicateServiceAccount(customCrName,
 
 	for _, sa := range saList.Items {
 		if sa.Name != ControllerServiceAccountName {
-			dups = append(dups, sa)
+			for _, ownerRef := range sa.OwnerReferences {
+				if ownerRef.Kind == "HostPathProvisioner" && ownerRef.Name == customCrName {
+					dups = append(dups, sa)
+					break
+				}
+			}
 		}
 	}
 

diff --git a/tools/csv-generator.go b/tools/csv-generator.go
@@ -340,19 +340,6 @@ func getOperatorRules() *[]rbacv1.PolicyRule {
 				"get",
 				"watch",
 				"create",
-			},
-		},
-		{
-			APIGroups: []string{
-				"apps",
-			},
-			Resources: []string{
-				"daemonsets",
-			},
-			ResourceNames: []string{
-				"hostpath-provisioner",
-			},
-			Verbs: []string{
 				"delete",
 				"update",
 			},
@@ -387,27 +374,10 @@ func getOperatorRules() *[]rbacv1.PolicyRule {
 			Resources: []string{
 				"serviceaccounts",
 			},
-			ResourceNames: []string{
-				"hostpath-provisioner-admin",
-			},
 			Verbs: []string{
 				"*",
 			},
 		},
-		{
-			APIGroups: []string{
-				"",
-			},
-			Resources: []string{
-				"serviceaccounts",
-			},
-			Verbs: []string{
-				"list",
-				"get",
-				"watch",
-				"create",
-			},
-		},
 	}
 }