From f441d9358247bbab7134ade27bd944fc755dec5e Mon Sep 17 00:00:00 2001 From: Ye Ji Date: Fri, 30 Jun 2023 13:50:29 -0400 Subject: [PATCH] reject mutations to additionalLabels field (#917) K8s does not support mutating selectors and labels on statefulsets at the same time: https://github.com/kubernetes/kubernetes/issues/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. --- CHANGELOG.md | 3 ++ apis/v1alpha1/webhook.go | 18 +++++++- apis/v1alpha1/webhook_test.go | 87 +++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7f1c91d4..a92a9eede 100644 --- 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 index 1d5a21d26..3a132c8d0 100644 --- 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 index 0851da10f..67204f6c6 100644 --- 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) + } + } +}