Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent changes of DC name or both adding and removing a DC at the same time. K8OP-263 #1426

Merged
merged 7 commits into from
Oct 7, 2024
31 changes: 31 additions & 0 deletions apis/k8ssandra/v1alpha1/k8ssandracluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Miles-Garnsey marked this conversation as resolved.
Show resolved Hide resolved
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
}
34 changes: 34 additions & 0 deletions apis/k8ssandra/v1alpha1/k8ssandracluster_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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))
Miles-Garnsey marked this conversation as resolved.
Show resolved Hide resolved
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))
}
7 changes: 5 additions & 2 deletions apis/k8ssandra/v1alpha1/k8ssandracluster_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Miles-Garnsey marked this conversation as resolved.
Show resolved Hide resolved
return nil, fmt.Errorf("renaming, as well as adding and removing DCs at the same time is prohibited as it can cause data loss")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] It would be nice to also include the names of the removed and added DCs.

}
// Verify that the cluster name override was not changed
if r.Spec.Cassandra.ClusterName != oldCluster.Spec.Cassandra.ClusterName {
return nil, ErrClusterName
Expand Down
12 changes: 11 additions & 1 deletion apis/k8ssandra/v1alpha1/k8ssandracluster_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
4 changes: 4 additions & 0 deletions controllers/k8ssandra/k8ssandracluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 14 additions & 2 deletions controllers/k8ssandra/k8ssandracluster_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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"
Miles-Garnsey marked this conversation as resolved.
Show resolved Hide resolved
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")
Expand Down
37 changes: 37 additions & 0 deletions controllers/k8ssandra/validate_cluster_update.go
Original file line number Diff line number Diff line change
@@ -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)
Miles-Garnsey marked this conversation as resolved.
Show resolved Hide resolved
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
}
Loading