diff --git a/pkg/admission/validator/shoot.go b/pkg/admission/validator/shoot.go index 13caf27b3..24199fb34 100644 --- a/pkg/admission/validator/shoot.go +++ b/pkg/admission/validator/shoot.go @@ -162,8 +162,8 @@ func (s *shoot) validateCreate(ctx context.Context, shoot *core.Shoot) error { // 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 for i, worker := range shoot.Spec.Provider.Workers { - if worker.Minimum < int32(len(worker.Zones)) { - return fmt.Errorf("%s minimum value must be >= %d if maximum value > 0 (auto scaling to 0 & from 0 is not supported", workersPath.Index(i).Child("minimum").String(), len(worker.Zones)) + if err = gcpvalidation.ValidateWorkerAutoScaling(worker, workersPath.Index(i).Child("minimum").String()); err != nil { + return err } } diff --git a/pkg/apis/gcp/validation/shoot.go b/pkg/apis/gcp/validation/shoot.go index 094cf64d7..cf92003d6 100644 --- a/pkg/apis/gcp/validation/shoot.go +++ b/pkg/apis/gcp/validation/shoot.go @@ -18,8 +18,10 @@ import ( "fmt" "github.com/gardener/gardener/pkg/apis/core" + "github.com/gardener/gardener/pkg/apis/core/helper" "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" ) @@ -60,6 +62,15 @@ func ValidateWorkers(workers []core.Worker, fldPath *field.Path) field.ErrorList return allErrs } +// ValidateWorkerAutoScaling checks if the worker.minimum value is greater or equal to the number of worker.zones[] +// when the worker.maximum value is greater than zero. This check is necessary because autoscaling from 0 is not supported on gcp. +func ValidateWorkerAutoScaling(worker core.Worker, path string) error { + if worker.Maximum > 0 && worker.Minimum < int32(len(worker.Zones)) { + return fmt.Errorf("%s value must be >= %d (number of zones) if maximum value > 0 (auto scaling to 0 & from 0 is not supported)", path, len(worker.Zones)) + } + return nil +} + func validateVolume(vol *core.Volume, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if vol.Type == nil { @@ -75,19 +86,19 @@ 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 := helper.FindWorkerByName(oldWorkers, newWorker.Name) + + if oldWorker != nil && validation.ShouldEnforceImmutability(newWorker.Zones, oldWorker.Zones) { + allErrs = append(allErrs, apivalidation.ValidateImmutableField(newWorker.Zones, oldWorker.Zones, workerFldPath.Child("zones"))...) } - if newWorker.Minimum < int32(len(newWorker.Zones)) { - allErrs = append(allErrs, field.Forbidden(workerFldPath.Child("minimum"), fmt.Sprintf("minimum value must be >= %d if maximum value > 0 (auto scaling to 0 & from 0 is not supported", len(newWorker.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 !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 diff --git a/pkg/apis/gcp/validation/shoot_test.go b/pkg/apis/gcp/validation/shoot_test.go index 1a51c70de..9cc4075ba 100644 --- a/pkg/apis/gcp/validation/shoot_test.go +++ b/pkg/apis/gcp/validation/shoot_test.go @@ -69,6 +69,38 @@ var _ = Describe("Shoot validation", func() { }) }) + Describe("#ValidateWorkerAutoscaling", func() { + var worker core.Worker + + BeforeEach(func() { + worker = core.Worker{ + Minimum: 2, + Maximum: 4, + Zones: []string{"zone1", "zone2"}, + } + }) + + It("should not return an error if worker.minimum is equal to number of zones", func() { + Expect(ValidateWorkerAutoScaling(worker, "")).To(Succeed()) + }) + + It("should not return an error if worker.minimum is greater than number of zones", func() { + worker.Minimum = 3 + Expect(ValidateWorkerAutoScaling(worker, "")).To(Succeed()) + }) + + It("should not return an error if worker.maximum is 0 and worker.minimum is less than number of zones", func() { + worker.Minimum = 0 + worker.Maximum = 0 + Expect(ValidateWorkerAutoScaling(worker, "")).To(Succeed()) + }) + + It("should return an error if worker.minimum is less than number of zones", func() { + worker.Minimum = 1 + Expect(ValidateWorkerAutoScaling(worker, "")).To(HaveOccurred()) + }) + }) + Describe("#ValidateWorkersUpdate", func() { var workerPath = field.NewPath("spec", "workers") @@ -99,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{ @@ -124,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", @@ -173,7 +205,93 @@ var _ = Describe("Shoot validation", func() { errorList := ValidateWorkersUpdate(oldWorkers, newWorkers, workerPath) - Expect(errorList[0].Error()).To(Equal("spec.workers[0].minimum: Forbidden: minimum value must be >= " + fmt.Sprint(len(newWorkers[0].Zones)) + " if maximum value > 0 (auto scaling to 0 & from 0 is not supported")) + Expect(errorList).ToNot(HaveLen(0)) + 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).ToNot(HaveLen(0)) + 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()) }) }) })