Repository: kubernetes-sigs/security-profiles-operator (c)

Issue:
Unable to delete seccompprofile if one of node is deleted
#### What happened:

Unable to delete the seccompprofiles if one of the node deleted/updated because of the finalizers is not updated with the latest nodes. I'm not sure if this bug or as per design.
Thank you

#### What you expected to happen:

Possible if SPO update seccompprofiles on the Finalizers part to the latest k8s nodes?

#### How to reproduce it (as minimally and precisely as possible):

- Start minikube with 3 nodes
```$ minikube  start --kubernetes-version=v1.21.14 --nodes=3 --memory 4000 --cpus```
- Install SPO
- Check the default SeccomProfiles, there are 3 finalizers
```
$ kubectl get seccompprofiles -n security-profiles-operator log-enricher-trace -o json | jq .metadata.finalizers
[
            "minikube-m03-delete",
            "minikube-m02-delete",
            "minikube-delete"
]
```
- Delete one of the node
```
$ minikube node delete minikube-m03
```
- The SeccompProfiles still have `minikube-m03` as finalizers
```
$ kubectl get seccompprofiles -n security-profiles-operator log-enricher-trace -o json | jq .metadata.finalizers
[
            "minikube-m03-delete",
            "minikube-m02-delete",
            "minikube-delete"
]
```
- I can't delete the seccompprofiles because the `"minikube-m03-delete"` is still exist (the old node that already deleted), unless if i patch it the finalizers like:
```
$ kubectl patch seccompprofiles log-enricher-trace -n security-profiles-operator -p '{"metadata":{"finalizers":[]}}' --type=merge
```

#### Anything else we need to know?:

I observed this on our Amazon EKS cluster, but for this testing i tested it on minikube with the same behavior. On our Amazon EKS clusters we have patching cycle that will delete the old node and bring up the new node (with new latest image). So after the patching the finalizers on the seccompprofiles still showing the old nodes, this make we can't delete the seccompprofles.


#### Environment:

- Cloud provider or hardware configuration:
- OS (e.g: `cat /etc/os-release`):
- Kernel (e.g. `uname -a`):
- Others:


Pull request:
handle node deletion
#### What type of PR is this?
/kind bug

#### What this PR does / why we need it:

This pr fixes seccompprofiles deletion when a node is removed, we added a check to see if the node finalizer is a deleted node, if so, we remove such finalizer so the seccompprofile can be deleted without any issues.

The nodestatus controller detect the extra nodestatus object, and if it can't find the node associated with it, it will try to remove the node status object and node finalizer from corresponding the seccomp profile


#### Which issue(s) this PR fixes:

Fixes #1152


#### Does this PR have test?

Yes

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
NONE

```release-note
This pr fixes seccompprofiles deletion when a node is removed, we added a check to see if the node finalizer is a deleted node, if so, we remove such finalizer so the seccompprofile can be deleted without any issues.
```


Patch:
diff --git a/bundle/manifests/security-profiles-operator.clusterserviceversion.yaml b/bundle/manifests/security-profiles-operator.clusterserviceversion.yaml
index 3ff229bf8..2bc089ca4 100644
--- a/bundle/manifests/security-profiles-operator.clusterserviceversion.yaml
+++ b/bundle/manifests/security-profiles-operator.clusterserviceversion.yaml
@@ -159,6 +159,14 @@ spec:
     spec:
       clusterPermissions:
       - rules:
+        - apiGroups:
+          - ""
+          resources:
+          - nodes
+          verbs:
+          - get
+          - list
+          - watch
         - apiGroups:
           - ""
           resources:
@@ -335,6 +343,7 @@ spec:
           resources:
           - securityprofilenodestatuses
           verbs:
+          - delete
           - get
           - list
           - watch
diff --git a/deploy/base/role.yaml b/deploy/base/role.yaml
index 28b377e0a..f26df8c1a 100644
--- a/deploy/base/role.yaml
+++ b/deploy/base/role.yaml
@@ -5,6 +5,14 @@ metadata:
   creationTimestamp: null
   name: security-profiles-operator
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:
@@ -181,6 +189,7 @@ rules:
   resources:
   - securityprofilenodestatuses
   verbs:
+  - delete
   - get
   - list
   - watch
diff --git a/deploy/helm/templates/static-resources.yaml b/deploy/helm/templates/static-resources.yaml
index dc6e35ff8..2e0ef7c7a 100644
--- a/deploy/helm/templates/static-resources.yaml
+++ b/deploy/helm/templates/static-resources.yaml
@@ -41,6 +41,14 @@ metadata:
     app: security-profiles-operator
   name: security-profiles-operator
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:
@@ -217,6 +225,7 @@ rules:
   resources:
   - securityprofilenodestatuses
   verbs:
+  - delete
   - get
   - list
   - watch
diff --git a/deploy/namespace-operator.yaml b/deploy/namespace-operator.yaml
index ab06e60af..ff31ccc43 100644
--- a/deploy/namespace-operator.yaml
+++ b/deploy/namespace-operator.yaml
@@ -2015,6 +2015,14 @@ metadata:
     app: security-profiles-operator
   name: security-profiles-operator
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:
@@ -2191,6 +2199,7 @@ rules:
   resources:
   - securityprofilenodestatuses
   verbs:
+  - delete
   - get
   - list
   - watch
diff --git a/deploy/openshift-dev.yaml b/deploy/openshift-dev.yaml
index a8e136316..9c8ff07ef 100644
--- a/deploy/openshift-dev.yaml
+++ b/deploy/openshift-dev.yaml
@@ -2015,6 +2015,14 @@ metadata:
     app: security-profiles-operator
   name: security-profiles-operator
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:
@@ -2191,6 +2199,7 @@ rules:
   resources:
   - securityprofilenodestatuses
   verbs:
+  - delete
   - get
   - list
   - watch
diff --git a/deploy/operator.yaml b/deploy/operator.yaml
index 8c3c24334..fd76d6817 100644
--- a/deploy/operator.yaml
+++ b/deploy/operator.yaml
@@ -2015,6 +2015,14 @@ metadata:
     app: security-profiles-operator
   name: security-profiles-operator
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:
@@ -2191,6 +2199,7 @@ rules:
   resources:
   - securityprofilenodestatuses
   verbs:
+  - delete
   - get
   - list
   - watch
diff --git a/deploy/overlays/webhook/role.yaml b/deploy/overlays/webhook/role.yaml
index 97221506b..f5c1444a2 100644
--- a/deploy/overlays/webhook/role.yaml
+++ b/deploy/overlays/webhook/role.yaml
@@ -1,3 +1,3 @@
 ---
-- path: /rules/1
+- path: /rules/2
   op: remove
diff --git a/deploy/webhook-operator.yaml b/deploy/webhook-operator.yaml
index abe58c15d..adef0f16e 100644
--- a/deploy/webhook-operator.yaml
+++ b/deploy/webhook-operator.yaml
@@ -2087,6 +2087,14 @@ metadata:
     app: security-profiles-operator
   name: security-profiles-operator
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:
@@ -2252,6 +2260,7 @@ rules:
   resources:
   - securityprofilenodestatuses
   verbs:
+  - delete
   - get
   - list
   - watch
diff --git a/internal/pkg/manager/nodestatus/nodestatus.go b/internal/pkg/manager/nodestatus/nodestatus.go
index af95f1969..447db98ba 100644
--- a/internal/pkg/manager/nodestatus/nodestatus.go
+++ b/internal/pkg/manager/nodestatus/nodestatus.go
@@ -27,6 +27,7 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/event"
 	"github.com/go-logr/logr"
 	appsv1 "k8s.io/api/apps/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/selection"
@@ -98,7 +99,8 @@ func (r *StatusReconciler) Healthz(*http.Request) error {
 
 // Security Profiles Operator RBAC permissions to manage Node Statuses
 //nolint:lll // required for kubebuilder
-// +kubebuilder:rbac:groups=security-profiles-operator.x-k8s.io,resources=securityprofilenodestatuses,verbs=get;list;watch
+// +kubebuilder:rbac:groups=security-profiles-operator.x-k8s.io,resources=securityprofilenodestatuses,verbs=get;list;watch;delete
+// +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch
 
 // Reconcile reconciles a NodeStatus.
 func (r *StatusReconciler) Reconcile(_ context.Context, req reconcile.Request) (reconcile.Result, error) {
@@ -167,11 +169,26 @@ func (r *StatusReconciler) Reconcile(_ context.Context, req reconcile.Request) (
 	// make sure we have all the statuses already
 	hasStatuses := len(nodeStatusList.Items)
 	wantsStatuses := spodDS.Status.DesiredNumberScheduled
-	if wantsStatuses != int32(hasStatuses) {
+	if wantsStatuses > int32(hasStatuses) {
 		logger.Info("Not updating policy: not all statuses are ready",
 			"has", hasStatuses, "wants", wantsStatuses)
 		// Don't reconcile again, let's just wait for another update
 		return reconcile.Result{}, nil
+	} else if wantsStatuses < int32(hasStatuses) {
+		// this happens when nodes are removed from the cluster
+		logger.Info("Removing extra statuses", "has", hasStatuses, "wants", wantsStatuses)
+		nodeName, err := r.removeStatusForDeletedNode(ctx, nodeStatusList, lprof)
+		if err != nil {
+			return reconcile.Result{}, fmt.Errorf("cannot remove extra statuses: %w", err)
+		}
+		if nodeName != "" {
+			// remove the deleted node finalizer string from the profile
+			logger.Info("Removing finalizer from profile", "profile", prof.GetName(), "node", nodeName)
+			if err := util.RemoveFinalizer(ctx, r.client, prof, util.GetFinalizerNodeString(nodeName)); err != nil {
+				return reconcile.Result{}, fmt.Errorf("cannot remove finalizer from profile: %w", err)
+			}
+		}
+		return reconcile.Result{Requeue: true}, nil
 	}
 
 	lowestCommonState := statusv1alpha1.LowestState
@@ -183,6 +200,27 @@ func (r *StatusReconciler) Reconcile(_ context.Context, req reconcile.Request) (
 	return r.reconcileStatus(ctx, prof, lowestCommonState, lprof)
 }
 
+// removeStatusForDeletedNode removes the status for a node that has been deleted.
+func (r *StatusReconciler) removeStatusForDeletedNode(ctx context.Context,
+	nodeStatusList *statusv1alpha1.SecurityProfileNodeStatusList, logger logr.Logger,
+) (string, error) {
+	for i := range nodeStatusList.Items {
+		nodeName := nodeStatusList.Items[i].NodeName
+		node := &v1.Node{}
+		if err := r.client.Get(ctx, types.NamespacedName{Name: nodeName}, node); err != nil {
+			if util.IsNotFoundOrConflict(err) {
+				logger.Info("Removing node status for removed node", "node", nodeName)
+				if err := r.client.Delete(ctx, &nodeStatusList.Items[i]); err != nil {
+					return "", fmt.Errorf("cannot delete node status: %w", err)
+				}
+				return nodeName, nil
+			}
+			return "", fmt.Errorf("cannot get node: %w", err)
+		}
+	}
+	return "", nil
+}
+
 func (r *StatusReconciler) getDS(ctx context.Context, namespace string, l logr.Logger) (*appsv1.DaemonSet, error) {
 	dsSelect := labels.NewSelector()
 	spodDSFilter, err := labels.NewRequirement("spod", selection.Exists, []string{})
diff --git a/internal/pkg/manager/recordingmerger/merge_utils.go b/internal/pkg/manager/recordingmerger/merge_utils.go
index 057279914..94fc89284 100644
--- a/internal/pkg/manager/recordingmerger/merge_utils.go
+++ b/internal/pkg/manager/recordingmerger/merge_utils.go
@@ -185,8 +185,19 @@ func addAllow(union, additional selinuxprofileapi.Allow) selinuxprofileapi.Allow
 		if _, ok := union[labelKey]; !ok {
 			union[labelKey] = make(map[selinuxprofileapi.ObjectClassKey]selinuxprofileapi.PermissionSet)
 		}
+
 		for objClass, perms := range permMap {
-			union[labelKey][objClass] = append(union[labelKey][objClass], perms...)
+			allPerms := map[string]bool{}
+			for _, havePerm := range union[labelKey][objClass] {
+				allPerms[havePerm] = true
+			}
+
+			for _, newPerm := range perms {
+				_, ok := allPerms[newPerm]
+				if !ok {
+					union[labelKey][objClass] = append(union[labelKey][objClass], newPerm)
+				}
+			}
 		}
 	}
 
diff --git a/internal/pkg/nodestatus/nodestatus.go b/internal/pkg/nodestatus/nodestatus.go
index 9acc15241..5c29da390 100644
--- a/internal/pkg/nodestatus/nodestatus.go
+++ b/internal/pkg/nodestatus/nodestatus.go
@@ -25,7 +25,6 @@ import (
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
@@ -246,12 +245,7 @@ func getFinalizerString(pol profilebase.SecurityProfileBase, nodeName string) st
 	if pol.IsPartial() {
 		return partialProfileFinalizer
 	}
-
-	finalizerString := nodeName + "-deleted"
-	// Make sure the length of finalizer is not longer than 63 characters
-	if len(nodeName)+len("-deleted") > validation.DNS1123LabelMaxLength {
-		finalizerString = nodeName[:validation.DNS1123LabelMaxLength-len("-deleted")] + "-deleted"
-	}
+	finalizerString := util.GetFinalizerNodeString(nodeName)
 	return finalizerString
 }
 
diff --git a/internal/pkg/util/finalizers.go b/internal/pkg/util/finalizers.go
index 71e30000c..e8daba134 100644
--- a/internal/pkg/util/finalizers.go
+++ b/internal/pkg/util/finalizers.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 
+	"k8s.io/apimachinery/pkg/util/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
@@ -47,3 +48,13 @@ func RemoveFinalizer(ctx context.Context, c client.Client, pol client.Object, fi
 	controllerutil.RemoveFinalizer(pol, finalizer)
 	return c.Update(ctx, pol)
 }
+
+// GetFinalizerNodeString gets finalizer string from Node Name.
+func GetFinalizerNodeString(nodeName string) string {
+	finalizerString := nodeName + "-deleted"
+	// Make sure the length of finalizer is not longer than 63 characters
+	if len(nodeName)+len("-deleted") > validation.DNS1123LabelMaxLength {
+		finalizerString = nodeName[:validation.DNS1123LabelMaxLength-len("-deleted")] + "-deleted"
+	}
+	return finalizerString
+}


Test patch:
diff --git a/internal/pkg/manager/recordingmerger/merge_utils_test.go b/internal/pkg/manager/recordingmerger/merge_utils_test.go
index f035dee17..cf8ac2276 100644
--- a/internal/pkg/manager/recordingmerger/merge_utils_test.go
+++ b/internal/pkg/manager/recordingmerger/merge_utils_test.go
@@ -153,7 +153,7 @@ func TestMergeProfiles(t *testing.T) {
 								},
 							},
 							Allow: selinuxprofileapi.Allow{
-								"label_foo": {"oc_bar2": {"do_bar2"}, "oc_baz2": {"do_baz2"}},
+								"label_foo": {"oc_bar": {"do_bar"}, "oc_bar2": {"do_bar2"}, "oc_baz2": {"do_baz2"}},
 								"label_aaa": {"oc_aaa": {"do_aaa"}, "oc_bbb": {"do_bbb"}},
 							},
 						},
diff --git a/test/tc_delete_profiles_test.go b/test/tc_delete_profiles_test.go
index 06408b56e..5b736f964 100644
--- a/test/tc_delete_profiles_test.go
+++ b/test/tc_delete_profiles_test.go
@@ -34,11 +34,27 @@ apiVersion: security-profiles-operator.x-k8s.io/v1beta1
 kind: SeccompProfile
 metadata:
   name: delete-me
+  finalizers: [fake-node-deleted]
 spec:
   defaultAction: "SCMP_ACT_ALLOW"
 `
-		deleteProfileName = "delete-me"
-		deletePod         = `
+		deleteProfileName  = "delete-me"
+		fakeNodeStatusName = "delete-me-fake-node"
+		fakeNodeStatus     = `
+apiVersion: security-profiles-operator.x-k8s.io/v1alpha1
+kind: SecurityProfileNodeStatus
+metadata:
+  name: delete-me-fake-node
+  labels:
+    spo.x-k8s.io/node-name: fake-node
+    spo.x-k8s.io/profile-id: SeccompProfile-delete-me
+    spo.x-k8s.io/profile-kind: SeccompProfile
+    spo.x-k8s.io/profile-state: Installed
+nodeName: fake-node
+spec: {}
+status: Installed
+`
+		deletePod = `
 apiVersion: v1
 kind: Pod
 metadata:
@@ -113,6 +129,9 @@ spec:
 	for _, node := range nodes {
 		e.execNode(node, "test", "-f", profileOperatorPath)
 	}
+	e.logf("Create fake node status for profile")
+	e.writeAndCreate(fakeNodeStatus, "fake-node-status*.yaml")
+	time.Sleep(time.Second)
 	e.logf("Verifying profile deleted")
 	e.kubectl("delete", "seccompprofile", deleteProfileName)
 	time.Sleep(time.Second)
@@ -146,6 +165,8 @@ spec:
 		profileCleanup := e.writeAndCreate(deleteProfile, "delete-profile*.yaml")
 		defer profileCleanup() //nolint:gocritic // TODO: is this intentional?
 		e.waitFor("condition=ready", "seccompprofile", deleteProfileName)
+		e.logf("Create fake node status for profile")
+		e.writeAndCreate(fakeNodeStatus, "fake-node-status*.yaml")
 		podCleanup := e.writeAndCreate(fmt.Sprintf(testCase.podManifest, namespace), "delete-pod*.yaml")
 		defer podCleanup() //nolint:gocritic // TODO: is this intention?
 		e.waitFor("condition=ready", "pod", deletePodName)