tests for kafka webhook were implemented & clusters webhooks were ref…

…actored
instaclustr · Jan 16, 2024 · 988bad3 · 988bad3
1 parent 8ff29e9
commit 988bad3
Show file tree

Hide file tree

Showing 11 changed files with 411 additions and 127 deletions.
diff --git a/Makefile b/Makefile
@@ -78,8 +78,12 @@ test-kafkamanagement:
 test-users:
 	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./controllers/tests
 
+.PHONY: test-webhooks
+test-webhooks:
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./apis/clusters/v1beta1 -coverprofile cover.out
+
 .PHONY: test
- test: manifests generate fmt vet docker-build-server-stub run-server-stub envtest test-clusters test-clusterresources test-kafkamanagement test-users stop-server-stub
+ test: manifests generate fmt vet docker-build-server-stub run-server-stub envtest test-clusters test-clusterresources test-webhooks test-kafkamanagement test-users stop-server-stub
 
 .PHONY: goimports
 goimports:

diff --git a/apis/clusters/v1beta1/cassandra_webhook.go b/apis/clusters/v1beta1/cassandra_webhook.go
@@ -280,55 +280,27 @@ func (cs *CassandraSpec) validateDataCentresUpdate(oldSpec CassandraSpec) error
 		return models.ErrDecreasedDataCentresNumber
 	}
 
+	toValidate := map[string]*CassandraDataCentre{}
+	for _, dc := range oldSpec.DataCentres {
+		toValidate[dc.Name] = dc
+	}
+
 	for _, newDC := range cs.DataCentres {
-		var exists bool
-		for _, oldDC := range oldSpec.DataCentres {
-			if oldDC.Name == newDC.Name {
-				newDCImmutableFields := newDC.newImmutableFields()
-				oldDCImmutableFields := oldDC.newImmutableFields()
-
-				if *newDCImmutableFields != *oldDCImmutableFields {
-					return fmt.Errorf("cannot update immutable data centre fields: new spec: %v: old spec: %v", newDCImmutableFields, oldDCImmutableFields)
-				}
-
-				if ((newDC.NodesNumber*newDC.ReplicationFactor)/newDC.ReplicationFactor)%newDC.ReplicationFactor != 0 {
-					return fmt.Errorf("number of nodes must be a multiple of replication factor: %v", newDC.ReplicationFactor)
-				}
-
-				if newDC.NodesNumber < oldDC.NodesNumber {
-					return fmt.Errorf("deleting nodes is not supported. Number of nodes must be greater than: %v", oldDC.NodesNumber)
-				}
-
-				err := newDC.validateImmutableCloudProviderSettingsUpdate(oldDC.CloudProviderSettings)
-				if err != nil {
-					return err
-				}
-
-				err = validateTagsUpdate(newDC.Tags, oldDC.Tags)
-				if err != nil {
-					return err
-				}
-
-				if !oldDC.DebeziumEquals(newDC) {
-					return models.ErrDebeziumImmutable
-				}
-
-				exists = true
-				break
+		oldDC, ok := toValidate[newDC.Name]
+		if !ok {
+			if len(cs.DataCentres) == len(oldSpec.DataCentres) {
+				return fmt.Errorf("cannot change datacentre name: %v", newDC.Name)
 			}
-		}
 
-		if !exists {
-			err := newDC.DataCentre.ValidateCreation()
-			if err != nil {
+			if err := newDC.ValidateCreation(); err != nil {
 				return err
 			}
 
 			if !cs.PrivateNetworkCluster && newDC.PrivateIPBroadcastForDiscovery {
 				return fmt.Errorf("cannot use private ip broadcast for discovery on public network cluster")
 			}
 
-			err = validateReplicationFactor(models.CassandraReplicationFactors, newDC.ReplicationFactor)
+			err := validateReplicationFactor(models.CassandraReplicationFactors, newDC.ReplicationFactor)
 			if err != nil {
 				return err
 			}
@@ -337,9 +309,37 @@ func (cs *CassandraSpec) validateDataCentresUpdate(oldSpec CassandraSpec) error
 				return fmt.Errorf("number of nodes must be a multiple of replication factor: %v", newDC.ReplicationFactor)
 			}
 
-			return nil
+		}
+
+		newDCImmutableFields := newDC.newImmutableFields()
+		oldDCImmutableFields := oldDC.newImmutableFields()
+
+		if *newDCImmutableFields != *oldDCImmutableFields {
+			return fmt.Errorf("cannot update immutable data centre fields: new spec: %v: old spec: %v", newDCImmutableFields, oldDCImmutableFields)
+		}
+
+		if ((newDC.NodesNumber*newDC.ReplicationFactor)/newDC.ReplicationFactor)%newDC.ReplicationFactor != 0 {
+			return fmt.Errorf("number of nodes must be a multiple of replication factor: %v", newDC.ReplicationFactor)
+		}
 
+		if newDC.NodesNumber < oldDC.NodesNumber {
+			return fmt.Errorf("deleting nodes is not supported. Number of nodes must be greater than: %v", oldDC.NodesNumber)
 		}
+
+		err := newDC.validateImmutableCloudProviderSettingsUpdate(oldDC.CloudProviderSettings)
+		if err != nil {
+			return err
+		}
+
+		err = validateTagsUpdate(newDC.Tags, oldDC.Tags)
+		if err != nil {
+			return err
+		}
+
+		if !oldDC.DebeziumEquals(newDC) {
+			return models.ErrDebeziumImmutable
+		}
+
 	}
 
 	return nil

diff --git a/apis/clusters/v1beta1/kafka_webhook.go b/apis/clusters/v1beta1/kafka_webhook.go
@@ -186,12 +186,12 @@ func (kv *kafkaValidator) ValidateUpdate(ctx context.Context, old runtime.Object
 		return fmt.Errorf("cannot assert object %v to kafka", new.GetObjectKind())
 	}
 
-	kafkalog.Info("validate update", "name", k.Name)
-
 	if k.Status.ID == "" {
 		return kv.ValidateCreate(ctx, k)
 	}
 
+	kafkalog.Info("validate update", "name", k.Name)
+
 	// skip validation when handle external changes from Instaclustr
 	if k.Annotations[models.ExternalChangesAnnotation] == models.True {
 		return nil
@@ -347,34 +347,40 @@ func (ks *KafkaSpec) validateImmutableDataCentresFieldsUpdate(oldSpec *KafkaSpec
 		return models.ErrDecreasedDataCentresNumber
 	}
 
+	toValidate := map[string]*KafkaDataCentre{}
+	for _, dc := range oldSpec.DataCentres {
+		toValidate[dc.Name] = dc
+	}
+
 	for _, newDC := range ks.DataCentres {
-		for _, oldDC := range oldSpec.DataCentres {
-			if oldDC.Name == newDC.Name {
-				newDCImmutableFields := newDC.newImmutableFields()
-				oldDCImmutableFields := oldDC.newImmutableFields()
-
-				if *newDCImmutableFields != *oldDCImmutableFields {
-					return fmt.Errorf("cannot update immutable data centre fields: new spec: %v: old spec: %v", newDCImmutableFields, oldDCImmutableFields)
-				}
-
-				if newDC.NodesNumber < oldDC.NodesNumber {
-					return fmt.Errorf("deleting nodes is not supported. Number of nodes must be greater than: %v", oldDC.NodesNumber)
-				}
-
-				err := newDC.validateImmutableCloudProviderSettingsUpdate(oldDC.CloudProviderSettings)
-				if err != nil {
-					return err
-				}
-
-				err = validateTagsUpdate(newDC.Tags, oldDC.Tags)
-				if err != nil {
-					return err
-				}
-
-				if ok := isPrivateLinkValid(newDC.PrivateLink, oldDC.PrivateLink); !ok {
-					return fmt.Errorf("advertisedHostname field cannot be changed")
-				}
-			}
+		oldDC, ok := toValidate[newDC.Name]
+		if !ok {
+			return fmt.Errorf("cannot change datacentre name: %v", newDC.Name)
+		}
+
+		newDCImmutableFields := newDC.newImmutableFields()
+		oldDCImmutableFields := oldDC.newImmutableFields()
+
+		if *newDCImmutableFields != *oldDCImmutableFields {
+			return fmt.Errorf("cannot update immutable data centre fields: new spec: %v: old spec: %v", newDCImmutableFields, oldDCImmutableFields)
+		}
+
+		if newDC.NodesNumber < oldDC.NodesNumber {
+			return fmt.Errorf("deleting nodes is not supported. Number of nodes must be greater than: %v", oldDC.NodesNumber)
+		}
+
+		err := newDC.validateImmutableCloudProviderSettingsUpdate(oldDC.CloudProviderSettings)
+		if err != nil {
+			return err
+		}
+
+		err = validateTagsUpdate(newDC.Tags, oldDC.Tags)
+		if err != nil {
+			return err
+		}
+
+		if ok = isPrivateLinkValid(newDC.PrivateLink, oldDC.PrivateLink); !ok {
+			return fmt.Errorf("advertisedHostname field cannot be changed")
 		}
 	}