Fix namespace selector comparison for webhook

We should only compare supported labels and not everything on the namespace, otherwise we can end up being stuck in a `SPOD` with the `UPDATING` state. We now only compare supported webhook labels and use them as indicator if it requires and update or not. Fixes #1981 Signed-off-by: Sascha Grunert <[email protected]>
kubernetes-sigs · Nov 28, 2023 · d7f11ef · d7f11ef
1 parent 47c73e8
commit d7f11ef
Show file tree

Hide file tree

Showing 3 changed files with 159 additions and 8 deletions.
diff --git a/internal/pkg/manager/spod/bindata/webhook.go b/internal/pkg/manager/spod/bindata/webhook.go
@@ -251,10 +251,13 @@ func webhookNeedsUpdate(existing, configured *admissionregv1.MutatingWebhook) bo
 		return true
 	}
 
-	if existing.NamespaceSelector != nil &&
-		configured.NamespaceSelector != nil &&
-		!reflect.DeepEqual(*existing.NamespaceSelector, *configured.NamespaceSelector) {
-		return true
+	if existing.NamespaceSelector != nil && configured.NamespaceSelector != nil {
+		// Only compare managed labels, all others are out of scope
+		for _, label := range []string{EnableBindingLabel, EnableRecordingLabel} {
+			if namespaceSelectorUnequalForLabel(label, existing.NamespaceSelector, configured.NamespaceSelector) {
+				return true
+			}
+		}
 	}
 
 	if existing.ObjectSelector == nil && configured.ObjectSelector != nil ||
@@ -272,6 +275,41 @@ func webhookNeedsUpdate(existing, configured *admissionregv1.MutatingWebhook) bo
 	return false
 }
 
+// namespaceSelectorUnequalForLabel checks if the selected label matches the
+// provided LabelSelectors and returns true if they're unequal.
+func namespaceSelectorUnequalForLabel(label string, existing, configured *metav1.LabelSelector) bool {
+	var (
+		i, j                                 int
+		existingHasLabel, configuredHasLabel bool
+	)
+
+	for i = range existing.MatchExpressions {
+		if existing.MatchExpressions[i].Key == label {
+			existingHasLabel = true
+			break
+		}
+	}
+
+	for j = range configured.MatchExpressions {
+		if configured.MatchExpressions[j].Key == label {
+			configuredHasLabel = true
+			break
+		}
+	}
+
+	if existingHasLabel != configuredHasLabel {
+		return true
+	}
+
+	// Check if values match
+	if existingHasLabel && configuredHasLabel &&
+		!reflect.DeepEqual(existing.MatchExpressions[i], configured.MatchExpressions[j]) {
+		return true
+	}
+
+	return false
+}
+
 func (w *Webhook) Update(ctx context.Context, c client.Client) error {
 	for k, o := range w.objectMap() {
 		if err := c.Patch(ctx, o, client.Merge); err != nil {

diff --git a/internal/pkg/manager/spod/bindata/webhook_test.go b/internal/pkg/manager/spod/bindata/webhook_test.go
@@ -0,0 +1,112 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package bindata
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+const testLabel = "test"
+
+func TestNamespaceSelectorUnequalForLabel(t *testing.T) {
+	t.Parallel()
+	for _, tc := range []struct {
+		name                 string
+		existing, configured *metav1.LabelSelector
+		expected             bool
+	}{
+		{
+			name:       "label not available in both selectors",
+			existing:   &metav1.LabelSelector{},
+			configured: &metav1.LabelSelector{},
+			expected:   false,
+		},
+		{
+			name: "label requirements are equal",
+			existing: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{
+				{
+					Key:      testLabel,
+					Operator: metav1.LabelSelectorOpExists,
+					Values:   []string{"foo"},
+				},
+			}},
+			configured: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{
+				{
+					Key:      testLabel,
+					Operator: metav1.LabelSelectorOpExists,
+					Values:   []string{"foo"},
+				},
+			}},
+			expected: false,
+		},
+		{
+			name: "label requirements are not equal in value",
+			existing: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{
+				{
+					Key:      testLabel,
+					Operator: metav1.LabelSelectorOpExists,
+					Values:   []string{"foo"},
+				},
+			}},
+			configured: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{
+				{
+					Key:      testLabel,
+					Operator: metav1.LabelSelectorOpExists,
+					Values:   []string{"bar"},
+				},
+			}},
+			expected: true,
+		},
+		{
+			name:     "label requirements are not equal (existing does not have the expression)",
+			existing: &metav1.LabelSelector{},
+			configured: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{
+				{
+					Key:      testLabel,
+					Operator: metav1.LabelSelectorOpExists,
+					Values:   []string{"bar"},
+				},
+			}},
+			expected: true,
+		},
+		{
+			name: "label requirements are not equal (configured does not have the expression)",
+			existing: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{
+				{
+					Key:      testLabel,
+					Operator: metav1.LabelSelectorOpExists,
+					Values:   []string{"bar"},
+				},
+			}},
+			configured: &metav1.LabelSelector{},
+			expected:   true,
+		},
+	} {
+		existing := tc.existing
+		configured := tc.configured
+		expected := tc.expected
+
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			res := namespaceSelectorUnequalForLabel(testLabel, existing, configured)
+			assert.Equal(t, expected, res)
+		})
+	}
+}
diff --git a/internal/pkg/manager/spod/spod_controller.go b/internal/pkg/manager/spod/spod_controller.go
@@ -214,6 +214,7 @@ func (r *ReconcileSPOd) Reconcile(ctx context.Context, req reconcile.Request) (r
 	}
 
 	if spodUpdate || hookUpdate {
+		r.log.Info("Updating spod", "spodUpdate", spodUpdate, "hookUpdate", hookUpdate)
 		updatedSPod := foundSPOd.DeepCopy()
 		updatedSPod.Spec.Template = configuredSPOd.Spec.Template
 		updateErr := r.handleUpdate(
@@ -241,7 +242,7 @@ func (r *ReconcileSPOd) handleInitialStatus(
 	spod *spodv1alpha1.SecurityProfilesOperatorDaemon,
 	l logr.Logger,
 ) (res reconcile.Result, err error) {
-	l.Info("Adding an initial status to the SPOD Instance")
+	l.Info("Adding an initial status to the SPOD instance")
 	sCopy := spod.DeepCopy()
 	sCopy.Status.StatePending()
 	updateErr := r.client.Status().Update(ctx, sCopy)
@@ -256,7 +257,7 @@ func (r *ReconcileSPOd) handleCreatingStatus(
 	spod *spodv1alpha1.SecurityProfilesOperatorDaemon,
 	l logr.Logger,
 ) (res reconcile.Result, err error) {
-	l.Info("Adding 'Creating' status to the SPOD Instance")
+	l.Info("Adding 'Creating' status to the SPOD instance")
 	sCopy := spod.DeepCopy()
 	sCopy.Status.StateCreating()
 	updateErr := r.client.Status().Update(ctx, sCopy)
@@ -271,7 +272,7 @@ func (r *ReconcileSPOd) handleUpdatingStatus(
 	spod *spodv1alpha1.SecurityProfilesOperatorDaemon,
 	l logr.Logger,
 ) (res reconcile.Result, err error) {
-	l.Info("Adding 'Updating' status to the SPOD Instance")
+	l.Info("Adding 'Updating' status to the SPOD instance")
 	sCopy := spod.DeepCopy()
 	sCopy.Status.StateUpdating()
 	updateErr := r.client.Status().Update(ctx, sCopy)
@@ -295,7 +296,7 @@ func (r *ReconcileSPOd) handleRunningStatus(
 	spod *spodv1alpha1.SecurityProfilesOperatorDaemon,
 	l logr.Logger,
 ) (res reconcile.Result, err error) {
-	l.Info("Adding 'Running' status to the SPOD Instance")
+	l.Info("Adding 'Running' status to the SPOD instance")
 	sCopy := spod.DeepCopy()
 	sCopy.Status.StateRunning()
 	updateErr := r.client.Status().Update(ctx, sCopy)