From 956754bcfaa667b46c585ec82ac458c4dc15d1ad Mon Sep 17 00:00:00 2001 From: Miles Garnsey Date: Fri, 4 Oct 2024 13:02:19 +0400 Subject: [PATCH 1/7] Prevent changes of DC name or both adding and removing a DC at the same time. --- .../v1alpha1/k8ssandracluster_types.go | 31 ++++++++++++++++ .../v1alpha1/k8ssandracluster_webhook.go | 7 +++- .../k8ssandra/k8ssandracluster_controller.go | 4 ++ .../k8ssandracluster_controller_test.go | 15 +++++++- .../k8ssandra/validate_cluster_update.go | 37 +++++++++++++++++++ 5 files changed, 90 insertions(+), 4 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_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/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..2147e5dd3 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,6 +2405,10 @@ 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.ClusterName = newClusterName err = f.Client.Update(ctx, k8c) diff --git a/controllers/k8ssandra/validate_cluster_update.go b/controllers/k8ssandra/validate_cluster_update.go new file mode 100644 index 000000000..5951d3d3c --- /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 +} From 51df207ea3d0e5a6037cd183a481803acba94b07 Mon Sep 17 00:00:00 2001 From: Miles Garnsey Date: Fri, 4 Oct 2024 14:11:42 +0400 Subject: [PATCH 2/7] Linting. --- controllers/k8ssandra/validate_cluster_update.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/k8ssandra/validate_cluster_update.go b/controllers/k8ssandra/validate_cluster_update.go index 5951d3d3c..a349e25a7 100644 --- a/controllers/k8ssandra/validate_cluster_update.go +++ b/controllers/k8ssandra/validate_cluster_update.go @@ -9,7 +9,7 @@ import ( func validateK8ssandraCluster(kc api.K8ssandraCluster) error { oldDCs := make([]string, 0) - for dcName, _ := range kc.Status.Datacenters { + for dcName := range kc.Status.Datacenters { oldDCs = append(oldDCs, dcName) } newDcs := make([]string, 0) @@ -24,7 +24,7 @@ func validateK8ssandraCluster(kc api.K8ssandraCluster) error { break } } - for dcName, _ := range kc.Status.Datacenters { + for dcName := range kc.Status.Datacenters { if !utils.SliceContains(newDcs, dcName) { wasRemoved = true break From 4e2e498e7287bd1542394a747a7e08e2250e40b7 Mon Sep 17 00:00:00 2001 From: Miles Garnsey Date: Fri, 4 Oct 2024 14:19:26 +0400 Subject: [PATCH 3/7] Fix issue where DC name change would have caused a test to pass even when it shouldn't. --- controllers/k8ssandra/k8ssandracluster_controller_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/k8ssandra/k8ssandracluster_controller_test.go b/controllers/k8ssandra/k8ssandracluster_controller_test.go index 2147e5dd3..cad05d642 100644 --- a/controllers/k8ssandra/k8ssandracluster_controller_test.go +++ b/controllers/k8ssandra/k8ssandracluster_controller_test.go @@ -2410,6 +2410,7 @@ func changeClusterDcNameFails(t *testing.T, ctx context.Context, f *framework.Fr 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") From afb9ee4b234af19a2286cb3b425df700e8d648f9 Mon Sep 17 00:00:00 2001 From: Miles Garnsey Date: Fri, 4 Oct 2024 14:29:46 +0400 Subject: [PATCH 4/7] Unit tests for webhook. --- .../v1alpha1/k8ssandracluster_webhook_test.go | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go index f39ad758f..e2d93af3f 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" @@ -650,3 +651,25 @@ func testAutomatedUpdateAnnotation(t *testing.T) { cluster.Annotations[AutomatedUpdateAnnotation] = string("true") require.Error(cluster.validateK8ssandraCluster()) } + +func TestDcRemoved(t *testing.T) { + kcOld := createClusterObjWithCassandraConfig("testcluster", "testns") + kcNew := kcOld.DeepCopy() + kcOld.Spec.Cassandra.Datacenters = append(kcOld.Spec.Cassandra.Datacenters, CassandraDatacenterTemplate{ + Meta: EmbeddedObjectMeta{ + Name: "dc2", + }, + }) + require.True(t, DcRemoved(kcOld.Spec, kcNew.Spec)) +} + +func TestDcAdded(t *testing.T) { + kcOld := createClusterObjWithCassandraConfig("testcluster", "testns") + kcNew := kcOld.DeepCopy() + kcNew.Spec.Cassandra.Datacenters = append(kcOld.Spec.Cassandra.Datacenters, CassandraDatacenterTemplate{ + Meta: EmbeddedObjectMeta{ + Name: "dc2", + }, + }) + require.True(t, DcAdded(kcOld.Spec, kcNew.Spec)) +} From 504a6d21ee0875bd5e61db77b7390e94eea83710 Mon Sep 17 00:00:00 2001 From: Miles Garnsey Date: Fri, 4 Oct 2024 14:33:24 +0400 Subject: [PATCH 5/7] Ensure that `TestAutomatedUpdateAnnotation` test actually runs. --- apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go index e2d93af3f..bf5f0171d 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go @@ -635,7 +635,7 @@ func TestValidateUpdateNumTokens(t *testing.T) { } } -func testAutomatedUpdateAnnotation(t *testing.T) { +func TestAutomatedUpdateAnnotation(t *testing.T) { require := require.New(t) createNamespace(require, "automated-update-namespace") cluster := createMinimalClusterObj("automated-update-test", "automated-update-namespace") From 7f38e2124dcb14fa762deea685f7867511c9f245 Mon Sep 17 00:00:00 2001 From: Miles Garnsey Date: Fri, 4 Oct 2024 14:38:14 +0400 Subject: [PATCH 6/7] More tests. --- .../v1alpha1/k8ssandracluster_types_test.go | 23 +++++++++++++++++ .../v1alpha1/k8ssandracluster_webhook_test.go | 25 +++++-------------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go index 154620021..d5e8c6243 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,25 @@ 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() + kcOld.Spec.Cassandra.Datacenters = append(kcOld.Spec.Cassandra.Datacenters, CassandraDatacenterTemplate{ + Meta: EmbeddedObjectMeta{ + Name: "dc2", + }, + }) + require.True(t, DcRemoved(kcOld.Spec, kcNew.Spec)) +} + +func TestDcAdded(t *testing.T) { + kcOld := createClusterObjWithCassandraConfig("testcluster", "testns") + kcNew := kcOld.DeepCopy() + kcNew.Spec.Cassandra.Datacenters = append(kcOld.Spec.Cassandra.Datacenters, CassandraDatacenterTemplate{ + Meta: EmbeddedObjectMeta{ + Name: "dc2", + }, + }) + require.True(t, DcAdded(kcOld.Spec, kcNew.Spec)) +} diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go index bf5f0171d..570c7e772 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go @@ -180,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) { @@ -635,7 +636,7 @@ func TestValidateUpdateNumTokens(t *testing.T) { } } -func TestAutomatedUpdateAnnotation(t *testing.T) { +func testAutomatedUpdateAnnotation(t *testing.T) { require := require.New(t) createNamespace(require, "automated-update-namespace") cluster := createMinimalClusterObj("automated-update-test", "automated-update-namespace") @@ -652,24 +653,10 @@ func TestAutomatedUpdateAnnotation(t *testing.T) { require.Error(cluster.validateK8ssandraCluster()) } -func TestDcRemoved(t *testing.T) { +func testNoDCRename(t *testing.T) { kcOld := createClusterObjWithCassandraConfig("testcluster", "testns") kcNew := kcOld.DeepCopy() - kcOld.Spec.Cassandra.Datacenters = append(kcOld.Spec.Cassandra.Datacenters, CassandraDatacenterTemplate{ - Meta: EmbeddedObjectMeta{ - Name: "dc2", - }, - }) - require.True(t, DcRemoved(kcOld.Spec, kcNew.Spec)) -} - -func TestDcAdded(t *testing.T) { - kcOld := createClusterObjWithCassandraConfig("testcluster", "testns") - kcNew := kcOld.DeepCopy() - kcNew.Spec.Cassandra.Datacenters = append(kcOld.Spec.Cassandra.Datacenters, CassandraDatacenterTemplate{ - Meta: EmbeddedObjectMeta{ - Name: "dc2", - }, - }) - require.True(t, DcAdded(kcOld.Spec, kcNew.Spec)) + kcNew.Spec.Cassandra.Datacenters[0].Meta.Name = "newdc1name" + _, err := kcNew.ValidateUpdate(kcOld) + require.Error(t, err) } From 50432b1db3b7142423edf630729cb502685d1bd1 Mon Sep 17 00:00:00 2001 From: Miles Garnsey Date: Fri, 4 Oct 2024 15:03:36 +0400 Subject: [PATCH 7/7] Additional tests to ensure that DC added/removed returns false when it should. --- .../k8ssandra/v1alpha1/k8ssandracluster_types_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go b/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go index d5e8c6243..e588cb487 100644 --- a/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go +++ b/apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go @@ -220,21 +220,32 @@ func TestGenerationChanged(t *testing.T) { 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)) }