From 7ddb75571c0aaf58fc059e53e8074f8546a89e8d Mon Sep 17 00:00:00 2001 From: Miles Garnsey <11435896+Miles-Garnsey@users.noreply.github.com> Date: Mon, 7 Oct 2024 10:44:01 +0400 Subject: [PATCH] Prevent changes of DC name or both adding and removing a DC at the same time. K8OP-263 (#1426) * Prevent changes of DC name or both adding and removing a DC at the same time. --- .../v1alpha1/k8ssandracluster_types.go | 31 ++++++++++++++++ .../v1alpha1/k8ssandracluster_types_test.go | 34 +++++++++++++++++ .../v1alpha1/k8ssandracluster_webhook.go | 7 +++- .../v1alpha1/k8ssandracluster_webhook_test.go | 12 +++++- .../k8ssandra/k8ssandracluster_controller.go | 4 ++ .../k8ssandracluster_controller_test.go | 16 +++++++- .../k8ssandra/validate_cluster_update.go | 37 +++++++++++++++++++ 7 files changed, 136 insertions(+), 5 deletions(-) create mode 100644 controllers/k8ssandra/validate_cluster_update.go diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go index b4d3203c3..1c52b9056 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go @@ -573,3 +573,34 @@ func (kc *K8ssandraCluster) GetClusterIdHash() string { func (k *K8ssandraCluster) GenerationChanged() bool { return k.Status.ObservedGeneration < k.Generation } + +func DcAdded(oldSpec K8ssandraClusterSpec, newSpec K8ssandraClusterSpec) bool { + oldDcs := make([]string, 0) + for _, dc := range oldSpec.Cassandra.Datacenters { + oldDcs = append(oldDcs, dc.Meta.Name) + } + wasAdded := false + for _, dc := range newSpec.Cassandra.Datacenters { + if !utils.SliceContains(oldDcs, dc.Meta.Name) { + wasAdded = true + break + } + } + return wasAdded + +} + +func DcRemoved(oldSpec K8ssandraClusterSpec, newSpec K8ssandraClusterSpec) bool { + newDcs := make([]string, 0) + for _, dc := range newSpec.Cassandra.Datacenters { + newDcs = append(newDcs, dc.Meta.Name) + } + wasRemoved := false + for _, dc := range oldSpec.Cassandra.Datacenters { + if !utils.SliceContains(newDcs, dc.Meta.Name) { + wasRemoved = true + break + } + } + return wasRemoved +} diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go index 154620021..e588cb487 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go @@ -10,6 +10,7 @@ import ( cassdcapi "github.com/k8ssandra/cass-operator/apis/cassandra/v1beta1" stargateapi "github.com/k8ssandra/k8ssandra-operator/apis/stargate/v1alpha1" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestK8ssandraCluster(t *testing.T) { @@ -215,3 +216,36 @@ func TestGenerationChanged(t *testing.T) { kc.ObjectMeta.Generation = 3 assert.True(kc.GenerationChanged()) } + +func TestDcRemoved(t *testing.T) { + kcOld := createClusterObjWithCassandraConfig("testcluster", "testns") + kcNew := kcOld.DeepCopy() + require.False(t, DcRemoved(kcOld.Spec, kcNew.Spec)) + kcOld.Spec.Cassandra.Datacenters = append(kcOld.Spec.Cassandra.Datacenters, CassandraDatacenterTemplate{ + Meta: EmbeddedObjectMeta{ + Name: "dc2", + }, + }) + require.True(t, DcRemoved(kcOld.Spec, kcNew.Spec)) + kcOld = createClusterObjWithCassandraConfig("testcluster", "testns") + kcNew = kcOld.DeepCopy() + kcNew.Spec.Cassandra.Datacenters[0].Meta.Name = "newName" + require.True(t, DcRemoved(kcOld.Spec, kcNew.Spec)) +} + +func TestDcAdded(t *testing.T) { + kcOld := createClusterObjWithCassandraConfig("testcluster", "testns") + kcNew := kcOld.DeepCopy() + require.False(t, DcAdded(kcOld.Spec, kcNew.Spec)) + kcNew.Spec.Cassandra.Datacenters = append(kcOld.Spec.Cassandra.Datacenters, CassandraDatacenterTemplate{ + Meta: EmbeddedObjectMeta{ + Name: "dc2", + }, + }) + require.True(t, DcAdded(kcOld.Spec, kcNew.Spec)) + + kcOld = createClusterObjWithCassandraConfig("testcluster", "testns") + kcNew = kcOld.DeepCopy() + kcNew.Spec.Cassandra.Datacenters[0].Meta.Name = "newName" + require.True(t, DcAdded(kcOld.Spec, kcNew.Spec)) +} diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook.go index 4501ebae2..92043e3c7 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook.go @@ -18,9 +18,10 @@ package v1alpha1 import ( "fmt" + "strings" + "github.com/Masterminds/semver/v3" reaperapi "github.com/k8ssandra/k8ssandra-operator/apis/reaper/v1alpha1" - "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation" @@ -172,7 +173,9 @@ func (r *K8ssandraCluster) ValidateUpdate(old runtime.Object) (admission.Warning if err := validateUpdateNumTokens(oldCluster.Spec.Cassandra, r.Spec.Cassandra); err != nil { return nil, err } - + if DcRemoved(oldCluster.Spec, r.Spec) && DcAdded(oldCluster.Spec, r.Spec) { + return nil, fmt.Errorf("renaming, as well as adding and removing DCs at the same time is prohibited as it can cause data loss") + } // Verify that the cluster name override was not changed if r.Spec.Cassandra.ClusterName != oldCluster.Spec.Cassandra.ClusterName { return nil, ErrClusterName diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go index f39ad758f..570c7e772 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go @@ -20,12 +20,13 @@ import ( "context" "crypto/tls" "fmt" - "k8s.io/apimachinery/pkg/api/resource" "net" "path/filepath" "testing" "time" + "k8s.io/apimachinery/pkg/api/resource" + logrusr "github.com/bombsimon/logrusr/v2" "github.com/k8ssandra/cass-operator/apis/cassandra/v1beta1" "github.com/k8ssandra/k8ssandra-operator/pkg/clientcache" @@ -179,6 +180,7 @@ func TestWebhook(t *testing.T) { t.Run("MedusaConfigNonLocalNamespace", testMedusaNonLocalNamespace) t.Run("AutomatedUpdateAnnotation", testAutomatedUpdateAnnotation) t.Run("ReaperStorage", testReaperStorage) + t.Run("NoDCRename", testNoDCRename) } func testContextValidation(t *testing.T) { @@ -650,3 +652,11 @@ func testAutomatedUpdateAnnotation(t *testing.T) { cluster.Annotations[AutomatedUpdateAnnotation] = string("true") require.Error(cluster.validateK8ssandraCluster()) } + +func testNoDCRename(t *testing.T) { + kcOld := createClusterObjWithCassandraConfig("testcluster", "testns") + kcNew := kcOld.DeepCopy() + kcNew.Spec.Cassandra.Datacenters[0].Meta.Name = "newdc1name" + _, err := kcNew.ValidateUpdate(kcOld) + require.Error(t, err) +} diff --git a/controllers/k8ssandra/k8ssandracluster_controller.go b/controllers/k8ssandra/k8ssandracluster_controller.go index 36035e8f3..84cf7cc3f 100644 --- a/controllers/k8ssandra/k8ssandracluster_controller.go +++ b/controllers/k8ssandra/k8ssandracluster_controller.go @@ -119,6 +119,10 @@ func (r *K8ssandraClusterReconciler) reconcile(ctx context.Context, kc *api.K8ss return recResult.Output() } + if err := validateK8ssandraCluster(*kc); err != nil { + return reconcile.Result{}, err + } + if kc.Spec.Cassandra == nil { // TODO handle the scenario of CassandraClusterTemplate being set to nil after having a non-nil value return ctrl.Result{}, nil diff --git a/controllers/k8ssandra/k8ssandracluster_controller_test.go b/controllers/k8ssandra/k8ssandracluster_controller_test.go index 96f7cb39b..cad05d642 100644 --- a/controllers/k8ssandra/k8ssandracluster_controller_test.go +++ b/controllers/k8ssandra/k8ssandracluster_controller_test.go @@ -116,7 +116,7 @@ func TestK8ssandraCluster(t *testing.T) { t.Run("ApplyClusterWithEncryptionOptionsExternalSecrets", testEnv.ControllerTest(ctx, applyClusterWithEncryptionOptionsExternalSecrets)) t.Run("StopDatacenter", testEnv.ControllerTest(ctx, stopDc)) t.Run("ConvertSystemReplicationAnnotation", testEnv.ControllerTest(ctx, convertSystemReplicationAnnotation)) - t.Run("ChangeClusterNameFails", testEnv.ControllerTest(ctx, changeClusterNameFails)) + t.Run("ChangeClusterDcNameFails", testEnv.ControllerTest(ctx, changeClusterDcNameFails)) t.Run("InjectContainersAndVolumes", testEnv.ControllerTest(ctx, injectContainersAndVolumes)) t.Run("CreateMultiDcDseCluster", testEnv.ControllerTest(ctx, createMultiDcDseCluster)) t.Run("PerNodeConfiguration", testEnv.ControllerTest(ctx, perNodeConfiguration)) @@ -2334,7 +2334,7 @@ func convertSystemReplicationAnnotation(t *testing.T, ctx context.Context, f *fr // Create a cluster with server and client encryption but client encryption stores missing. // Verify that dc1 never gets created. -func changeClusterNameFails(t *testing.T, ctx context.Context, f *framework.Framework, namespace string) { +func changeClusterDcNameFails(t *testing.T, ctx context.Context, f *framework.Framework, namespace string) { require := require.New(t) clusterName := "cluster-with-encryption" @@ -2366,6 +2366,13 @@ func changeClusterNameFails(t *testing.T, ctx context.Context, f *framework.Fram K8sContext: f.DataPlaneContexts[0], Size: dc1Size, }, + { + Meta: api.EmbeddedObjectMeta{ + Name: "dc2", + }, + K8sContext: f.DataPlaneContexts[0], + Size: dc1Size, + }, }, ServerEncryptionStores: &encryption.Stores{ KeystoreSecretRef: &encryption.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{ @@ -2398,7 +2405,12 @@ func changeClusterNameFails(t *testing.T, ctx context.Context, f *framework.Fram err = f.Client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: clusterName}, k8c) require.NoError(err, "failed to get K8ssandraCluster") + k8c.Spec.Cassandra.Datacenters[0].Meta.Name = "newDC1" + err = f.Client.Update(ctx, k8c) + require.Error(err, "failed to update K8ssandraCluster") + // Change the cluster name + k8c.Spec.Cassandra.Datacenters[0].Meta.Name = "dc1" k8c.Spec.Cassandra.ClusterName = newClusterName err = f.Client.Update(ctx, k8c) require.Error(err, "failed to update K8ssandraCluster") diff --git a/controllers/k8ssandra/validate_cluster_update.go b/controllers/k8ssandra/validate_cluster_update.go new file mode 100644 index 000000000..a349e25a7 --- /dev/null +++ b/controllers/k8ssandra/validate_cluster_update.go @@ -0,0 +1,37 @@ +package k8ssandra + +import ( + "fmt" + + api "github.com/k8ssandra/k8ssandra-operator/apis/k8ssandra/v1alpha1" + "github.com/k8ssandra/k8ssandra-operator/pkg/utils" +) + +func validateK8ssandraCluster(kc api.K8ssandraCluster) error { + oldDCs := make([]string, 0) + for dcName := range kc.Status.Datacenters { + oldDCs = append(oldDCs, dcName) + } + newDcs := make([]string, 0) + for _, dc := range kc.Spec.Cassandra.Datacenters { + newDcs = append(newDcs, dc.Meta.Name) + } + wasAdded := false + wasRemoved := false + for _, dc := range kc.Spec.Cassandra.Datacenters { + if !utils.SliceContains(oldDCs, dc.Meta.Name) { + wasAdded = true + break + } + } + for dcName := range kc.Status.Datacenters { + if !utils.SliceContains(newDcs, dcName) { + wasRemoved = true + break + } + } + if wasAdded && wasRemoved { + return fmt.Errorf("cannot add and remove datacenters at the same time") + } + return nil +}