Make admission check of min worker pool and number of zones backward …

…compatible
gardener · Nov 17, 2021 · b3744af · b3744af
1 parent bacb347
commit b3744af
Show file tree

Hide file tree

Showing 2 changed files with 105 additions and 12 deletions.
diff --git a/pkg/apis/gcp/validation/shoot.go b/pkg/apis/gcp/validation/shoot.go
@@ -20,6 +20,7 @@ import (
 	"github.com/gardener/gardener/pkg/apis/core"
 	"github.com/gardener/gardener/pkg/apis/core/validation"
 
+	"k8s.io/apimachinery/pkg/api/equality"
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
@@ -84,22 +85,30 @@ func validateVolume(vol *core.Volume, fldPath *field.Path) field.ErrorList {
 func ValidateWorkersUpdate(oldWorkers, newWorkers []core.Worker, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	for i, newWorker := range newWorkers {
-
 		workerFldPath := fldPath.Index(i)
-		for _, oldWorker := range oldWorkers {
-			if newWorker.Name == oldWorker.Name {
-				if validation.ShouldEnforceImmutability(newWorker.Zones, oldWorker.Zones) {
-					allErrs = append(allErrs, apivalidation.ValidateImmutableField(newWorker.Zones, oldWorker.Zones, workerFldPath.Child("zones"))...)
-				}
-				break
-			}
+		oldWorker, found := getWorkerByName(newWorker.Name, oldWorkers)
+
+		if found && validation.ShouldEnforceImmutability(newWorker.Zones, oldWorker.Zones) {
+			allErrs = append(allErrs, apivalidation.ValidateImmutableField(newWorker.Zones, oldWorker.Zones, workerFldPath.Child("zones"))...)
 		}
 
 		// TODO: This check won't be needed after generic support to scale from zero is introduced in CA
 		// Ongoing issue - https://github.com/gardener/autoscaler/issues/27
-		if err := ValidateWorkerAutoScaling(newWorker, workerFldPath.Child("minimum").String()); err != nil {
-			allErrs = append(allErrs, field.Forbidden(workerFldPath.Child("minimum"), err.Error()))
+		if !equality.Semantic.DeepEqual(newWorker, oldWorker) {
+			if err := ValidateWorkerAutoScaling(newWorker, workerFldPath.Child("minimum").String()); err != nil {
+				allErrs = append(allErrs, field.Forbidden(workerFldPath.Child("minimum"), err.Error()))
+			}
 		}
 	}
 	return allErrs
 }
+
+func getWorkerByName(name string, workers []core.Worker) (core.Worker, bool) {
+	for _, w := range workers {
+		if w.Name == name {
+			return w, true
+		}
+	}
+
+	return core.Worker{}, false
+}
diff --git a/pkg/apis/gcp/validation/shoot_test.go b/pkg/apis/gcp/validation/shoot_test.go
@@ -131,7 +131,7 @@ var _ = Describe("Shoot validation", func() {
 
 			oldWorkers := []core.Worker{
 				{
-					Name: "worker-test",
+					Name: "worker-test2",
 					Machine: core.Machine{
 						Type: "n1-standard-2",
 						Image: &core.ShootMachineImage{
@@ -156,7 +156,7 @@ var _ = Describe("Shoot validation", func() {
 			Expect(errorList).To(BeEmpty())
 		})
 
-		It("should return an error because new worker pool added does not honor having min workers >= number of zones", func() {
+		It("should return an error because updated worker pool does not honor having min workers >= number of zones", func() {
 			newWorkers := []core.Worker{
 				{
 					Name: "worker-test",
@@ -207,6 +207,90 @@ var _ = Describe("Shoot validation", func() {
 
 			Expect(errorList[0].Error()).To(Equal("spec.workers[0].minimum: Forbidden: spec.workers[0].minimum value must be >= " + fmt.Sprint(len(newWorkers[0].Zones)) + " (number of zones) if maximum value > 0 (auto scaling to 0 & from 0 is not supported)"))
 		})
+
+		It("should return an error because newly added worker pool does not honor having min workers >= number of zones", func() {
+			newWorkers := []core.Worker{
+				{
+					Name: "worker-test",
+					Machine: core.Machine{
+						Type: "n1-standard-2",
+						Image: &core.ShootMachineImage{
+							Name:    "gardenlinux",
+							Version: "318.8.0",
+						},
+					},
+					Maximum: 6,
+					Minimum: 1,
+					Zones: []string{
+						"europe-west1-b",
+						"europe-west1-c",
+					},
+					Volume: &core.Volume{
+						Type:       pointer.StringPtr("pd-standard"),
+						VolumeSize: "50Gi",
+					},
+				},
+			}
+
+			oldWorkers := []core.Worker{}
+
+			errorList := ValidateWorkersUpdate(oldWorkers, newWorkers, workerPath)
+
+			Expect(errorList[0].Error()).To(Equal("spec.workers[0].minimum: Forbidden: spec.workers[0].minimum value must be >= " + fmt.Sprint(len(newWorkers[0].Zones)) + " (number of zones) if maximum value > 0 (auto scaling to 0 & from 0 is not supported)"))
+		})
+
+		It("should not return any error because existing worker pool does not honor having min workers >= number of zones", func() {
+			newWorkers := []core.Worker{
+				{
+					Name: "worker-test",
+					Machine: core.Machine{
+						Type: "n1-standard-2",
+						Image: &core.ShootMachineImage{
+							Name:    "gardenlinux",
+							Version: "318.8.0",
+						},
+					},
+					Maximum: 6,
+					Minimum: 2,
+					Zones: []string{
+						"europe-west1-b",
+						"europe-west1-c",
+						"europe-west1-d",
+					},
+					Volume: &core.Volume{
+						Type:       pointer.StringPtr("pd-standard"),
+						VolumeSize: "50Gi",
+					},
+				},
+			}
+
+			oldWorkers := []core.Worker{
+				{
+					Name: "worker-test",
+					Machine: core.Machine{
+						Type: "n1-standard-2",
+						Image: &core.ShootMachineImage{
+							Name:    "gardenlinux",
+							Version: "318.8.0",
+						},
+					},
+					Maximum: 6,
+					Minimum: 2,
+					Zones: []string{
+						"europe-west1-b",
+						"europe-west1-c",
+						"europe-west1-d",
+					},
+					Volume: &core.Volume{
+						Type:       pointer.StringPtr("pd-standard"),
+						VolumeSize: "50Gi",
+					},
+				},
+			}
+
+			errorList := ValidateWorkersUpdate(oldWorkers, newWorkers, workerPath)
+			Expect(errorList).To(BeEmpty())
+		})
 	})
 })