reject mutations to additionalLabels field (#917)

K8s does not support mutating selectors and labels on statefulsets at the same time: kubernetes/kubernetes#90519. This PR adds a validation hook to reject mutating additionalLabels, which will cause mutating selectors and labels on the underlying sts, and a constant reconciliation loop.
cockroachdb · Jun 30, 2023 · f441d93 · f441d93
1 parent e5af1e0
commit f441d93
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -37,6 +37,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * Correctly detect failed version checker Pods
 * retry cluster status updates, reducing test flakes
 
+## Changed
+* Update validation webhook to reject changes to cluster spec's AdditionalLabels field
+
 # [v2.7.0](https://github.com/cockroachdb/cockroach-operator/compare/v2.6.0...v2.7.0)
 
 ## Fixed

diff --git a/apis/v1alpha1/webhook.go b/apis/v1alpha1/webhook.go
@@ -18,6 +18,8 @@ package v1alpha1
 
 import (
 	"fmt"
+	"reflect"
+
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -51,7 +53,7 @@ func (r *CrdbCluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).For(r).Complete()
 }
 
-//+kubebuilder:webhook:path=/mutate-crdb-cockroachlabs-com-v1alpha1-crdbcluster,mutating=true,failurePolicy=fail,groups=crdb.cockroachlabs.com,resources=crdbclusters,verbs=create;update,versions=v1alpha1,name=mcrdbcluster.kb.io,sideEffects=None,admissionReviewVersions=v1
+// +kubebuilder:webhook:path=/mutate-crdb-cockroachlabs-com-v1alpha1-crdbcluster,mutating=true,failurePolicy=fail,groups=crdb.cockroachlabs.com,resources=crdbclusters,verbs=create;update,versions=v1alpha1,name=mcrdbcluster.kb.io,sideEffects=None,admissionReviewVersions=v1
 
 // Default implements webhook.Defaulter so a webhook will be registered for the type.
 func (r *CrdbCluster) Default() {
@@ -79,7 +81,7 @@ func (r *CrdbCluster) Default() {
 	}
 }
 
-//+kubebuilder:webhook:path=/validate-crdb-cockroachlabs-com-v1alpha1-crdbcluster,mutating=false,failurePolicy=fail,groups=crdb.cockroachlabs.com,resources=crdbclusters,verbs=create;update,versions=v1alpha1,name=vcrdbcluster.kb.io,sideEffects=None,admissionReviewVersions=v1
+// +kubebuilder:webhook:path=/validate-crdb-cockroachlabs-com-v1alpha1-crdbcluster,mutating=false,failurePolicy=fail,groups=crdb.cockroachlabs.com,resources=crdbclusters,verbs=create;update,versions=v1alpha1,name=vcrdbcluster.kb.io,sideEffects=None,admissionReviewVersions=v1
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
 func (r *CrdbCluster) ValidateCreate() error {
@@ -111,6 +113,18 @@ func (r *CrdbCluster) ValidateUpdate(old runtime.Object) error {
 	webhookLog.Info("validate update", "name", r.Name)
 	var errors []error
 
+	oldCluster, ok := old.(*CrdbCluster)
+	if !ok {
+		webhookLog.Info(fmt.Sprintf("unexpected old cluster type %T", old))
+	} else {
+		// Validate if labels changed.
+		// k8s does not support changing selector/labels on sts:
+		//  https://github.com/kubernetes/kubernetes/issues/90519.
+		if !reflect.DeepEqual(oldCluster.Spec.AdditionalLabels, r.Spec.AdditionalLabels) {
+			errors = append(errors, fmt.Errorf("mutating additionalLabels field is not supported"))
+		}
+	}
+
 	if r.Spec.Ingress != nil {
 		if err := r.ValidateIngress(); err != nil {
 			errors = append(errors, err...)

diff --git a/apis/v1alpha1/webhook_test.go b/apis/v1alpha1/webhook_test.go
@@ -23,6 +23,7 @@ import (
 	. "github.com/cockroachdb/cockroach-operator/apis/v1alpha1"
 	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
 func TestCrdbClusterDefault(t *testing.T) {
@@ -181,3 +182,89 @@ func TestUpdateCrdbCluster(t *testing.T) {
 		require.Equal(t, err.Error(), testcase.ErrMsg)
 	}
 }
+
+func TestUpdateCrdbClusterLabels(t *testing.T) {
+	oldCluster := CrdbCluster{
+		Spec: CrdbClusterSpec{
+			Image: &PodImage{},
+			AdditionalLabels: map[string]string{
+				"k": "v",
+			},
+		},
+	}
+	fs := v1.PersistentVolumeFilesystem
+
+	testcases := []struct {
+		Cluster     *CrdbCluster
+		ShouldError bool
+	}{
+		{
+			Cluster: &CrdbCluster{Spec: CrdbClusterSpec{
+				Image:            &PodImage{Name: "testImage"},
+				AdditionalLabels: map[string]string{"k": "v"},
+				DataStore: Volume{
+					VolumeClaim: &VolumeClaim{
+						PersistentVolumeClaimSpec: v1.PersistentVolumeClaimSpec{
+							VolumeMode: &fs,
+						},
+					},
+				},
+			}},
+			ShouldError: false,
+		},
+		{
+			Cluster: &CrdbCluster{Spec: CrdbClusterSpec{
+				Image:            &PodImage{Name: "testImage"},
+				AdditionalLabels: map[string]string{"k": "x"},
+				DataStore: Volume{
+					VolumeClaim: &VolumeClaim{
+						PersistentVolumeClaimSpec: v1.PersistentVolumeClaimSpec{
+							VolumeMode: &fs,
+						},
+					},
+				},
+			}},
+			// label k has a different value.
+			ShouldError: true,
+		},
+		{
+			Cluster: &CrdbCluster{Spec: CrdbClusterSpec{
+				Image: &PodImage{Name: "testImage"},
+				DataStore: Volume{
+					VolumeClaim: &VolumeClaim{
+						PersistentVolumeClaimSpec: v1.PersistentVolumeClaimSpec{
+							VolumeMode: &fs,
+						},
+					},
+				},
+			}},
+			// labels are missing / empty.
+			ShouldError: true,
+		},
+		{
+			Cluster: &CrdbCluster{Spec: CrdbClusterSpec{
+				Image:            &PodImage{Name: "testImage"},
+				AdditionalLabels: map[string]string{"k": "v", "kk": "v"},
+				DataStore: Volume{
+					VolumeClaim: &VolumeClaim{
+						PersistentVolumeClaimSpec: v1.PersistentVolumeClaimSpec{
+							VolumeMode: &fs,
+						},
+					},
+				},
+			}},
+			// labels contain additional kv.
+			ShouldError: true,
+		},
+	}
+
+	for _, tc := range testcases {
+		err := tc.Cluster.ValidateUpdate(runtime.Object(&oldCluster))
+		if tc.ShouldError {
+			require.Error(t, err)
+			require.Equal(t, err.Error(), "mutating additionalLabels field is not supported")
+		} else {
+			require.NoError(t, err)
+		}
+	}
+}