Remove validation for expired Kubernetes and MachineImage version…

…s in the `CloudProfile`, drop expired versions on creation (gardener#8297) * Drop expired `Kubernetes` and `MachineImage` versions from cloudprofile on creation * Fix redundant type array errors Not related to this PR * Remove validation for expired versions in the cloudprofile * Address PR review feedback
nickytd · Aug 7, 2023 · 2a6f538 · 2a6f538
1 parent 57afa64
commit 2a6f538
Show file tree

Hide file tree

Showing 6 changed files with 216 additions and 199 deletions.
diff --git a/pkg/apis/core/helper/helpers_test.go b/pkg/apis/core/helper/helpers_test.go
@@ -921,11 +921,11 @@ var _ = Describe("helper", func() {
 			Expect(ShootEnablesSSHAccess(shoot)).To(Equal(expectedResult))
 		},
 
-		Entry("should return false when shoot provider has zero workers", nil, nil, false),
-		Entry("should return true when shoot provider has no WorkersSettings", []core.Worker{core.Worker{}}, nil, true),
-		Entry("should return true when shoot worker settings has no SSHAccess", []core.Worker{core.Worker{}}, &core.WorkersSettings{}, true),
-		Entry("should return true when shoot worker settings has SSHAccess set to true", []core.Worker{core.Worker{}}, &core.WorkersSettings{SSHAccess: &core.SSHAccess{Enabled: true}}, true),
-		Entry("should return false when shoot worker settings has SSHAccess set to false", []core.Worker{core.Worker{}}, &core.WorkersSettings{SSHAccess: &core.SSHAccess{Enabled: false}}, false),
+		Entry("should return false when shoot provider has zero workers", []core.Worker{}, nil, false),
+		Entry("should return true when shoot provider has no WorkersSettings", []core.Worker{{Name: "worker"}}, nil, true),
+		Entry("should return true when shoot worker settings has no SSHAccess", []core.Worker{{Name: "worker"}}, &core.WorkersSettings{}, true),
+		Entry("should return true when shoot worker settings has SSHAccess set to true", []core.Worker{{Name: "worker"}}, &core.WorkersSettings{SSHAccess: &core.SSHAccess{Enabled: true}}, true),
+		Entry("should return false when shoot worker settings has SSHAccess set to false", []core.Worker{{Name: "worker"}}, &core.WorkersSettings{SSHAccess: &core.SSHAccess{Enabled: false}}, false),
 	)
 
 	Describe("#DeterminePrimaryIPFamily", func() {

diff --git a/pkg/apis/core/validation/cloudprofile.go b/pkg/apis/core/validation/cloudprofile.go
@@ -17,7 +17,6 @@ package validation
 import (
 	"fmt"
 	"regexp"
-	"time"
 
 	"github.com/Masterminds/semver"
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
@@ -32,40 +31,6 @@ import (
 	"github.com/gardener/gardener/pkg/utils"
 )
 
-// ValidateCloudProfileCreation validates a CloudProfile object when it is initially created.
-func ValidateCloudProfileCreation(cloudProfile *core.CloudProfile) field.ErrorList {
-	allErrs := field.ErrorList{}
-	fldPath := field.NewPath("spec")
-
-	// check that during creation no version can have an expiration date in the past
-	fldPathKubernetes := fldPath.Child("kubernetes", "versions")
-	for index, version := range cloudProfile.Spec.Kubernetes.Versions {
-		allErrs = append(allErrs, validateVersionExpiration(version, fldPathKubernetes.Index(index))...)
-	}
-
-	fldPathMachineImage := fldPath.Child("machineImages")
-	for index, image := range cloudProfile.Spec.MachineImages {
-		fldPathImage := fldPathMachineImage.Index(index)
-		for index, version := range image.Versions {
-			fldPathImageVersion := fldPathImage.Child("versions")
-			allErrs = append(allErrs, validateVersionExpiration(version.ExpirableVersion, fldPathImageVersion.Index(index))...)
-		}
-	}
-
-	allErrs = append(allErrs, ValidateCloudProfile(cloudProfile)...)
-
-	return allErrs
-}
-
-// validateVersionExpiration validates that the version has no expiration date in the past
-func validateVersionExpiration(version core.ExpirableVersion, fldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-	if version.ExpirationDate != nil && version.ExpirationDate.Time.UTC().Before(time.Now().UTC()) {
-		allErrs = append(allErrs, field.Forbidden(fldPath, fmt.Sprintf("unable to create version %q. Creating a version with expiration date in the past is not allowed", version.Version)))
-	}
-	return allErrs
-}
-
 // ValidateCloudProfile validates a CloudProfile object.
 func ValidateCloudProfile(cloudProfile *core.CloudProfile) field.ErrorList {
 	allErrs := field.ErrorList{}
@@ -90,41 +55,7 @@ func ValidateCloudProfileUpdate(newProfile, oldProfile *core.CloudProfile) field
 // ValidateCloudProfileSpecUpdate validates the spec update of a CloudProfile
 func ValidateCloudProfileSpecUpdate(new, old *core.CloudProfileSpec, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	allErrs = append(allErrs, validateCloudProfileVersionsUpdate(new.Kubernetes.Versions, old.Kubernetes.Versions, fldPath.Child("kubernetes", "versions"))...)
-
-	for _, oldImage := range old.MachineImages {
-		for index, newImage := range new.MachineImages {
-			if oldImage.Name == newImage.Name {
-				allErrs = append(
-					allErrs,
-					validateCloudProfileVersionsUpdate(
-						helper.ToExpirableVersions(newImage.Versions),
-						helper.ToExpirableVersions(oldImage.Versions),
-						fldPath.Child("machineImages").Index(index).Child("versions"),
-					)...,
-				)
-			}
-		}
-	}
-
-	return allErrs
-}
-
-// ValidateCloudProfileAddedVersions validates versions added to the CloudProfile
-func ValidateCloudProfileAddedVersions(versions []core.ExpirableVersion, addedVersions map[string]int, fldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-	for _, index := range addedVersions {
-		version := versions[index]
-		allErrs = append(allErrs, validateVersionExpiration(version, fldPath.Index(index))...)
-	}
-	return allErrs
-}
 
-// validateCloudProfileVersionsUpdate validates versions added to the CloudProfile
-func validateCloudProfileVersionsUpdate(new, old []core.ExpirableVersion, fldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
-	versions := helper.GetAddedVersions(old, new)
-	allErrs = append(allErrs, ValidateCloudProfileAddedVersions(new, versions, fldPath)...)
 	return allErrs
 }
 

diff --git a/pkg/apis/core/validation/cloudprofile_test.go b/pkg/apis/core/validation/cloudprofile_test.go
@@ -1090,128 +1090,4 @@ var _ = Describe("CloudProfile Validation Tests ", func() {
 			})
 		})
 	})
-
-	Context("#ValidateCloudProfileAddedVersions - Update adding versions to CloudProfile (Kubernetes/MachineImage versions)", func() {
-		var versions []core.ExpirableVersion
-		BeforeEach(func() {
-			versions = []core.ExpirableVersion{
-				{Version: "2135.6.3"},
-				{Version: "2135.6.2"},
-				{Version: "2135.6.1"},
-				{Version: "2135.6.0"},
-			}
-		})
-
-		It("Allow adding versions", func() {
-			addedVersions := map[string]int{
-				versions[0].Version: 0,
-			}
-			errorList := ValidateCloudProfileAddedVersions(versions, addedVersions, field.NewPath("spec.kubernetes.versions"))
-
-			Expect(errorList).To(BeEmpty())
-		})
-
-		It("Do not allow adding versions with expiration date in the past", func() {
-			addedVersions := map[string]int{
-				versions[0].Version: 0,
-				versions[2].Version: 2,
-			}
-			versions[2].ExpirationDate = dateInThePast
-			errorList := ValidateCloudProfileAddedVersions(versions, addedVersions, field.NewPath("spec.kubernetes.versions"))
-
-			Expect(errorList).To(HaveLen(1))
-			Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{
-				"Type":   Equal(field.ErrorTypeForbidden),
-				"Field":  Equal("spec.kubernetes.versions[2]"),
-				"Detail": ContainSubstring("a version with expiration date in the past is not allowed"),
-			}))))
-		})
-	})
-
-	Describe("#ValidateCloudProfileCreation", func() {
-		var cloudProfile *core.CloudProfile
-		BeforeEach(func() {
-			cloudProfile = &core.CloudProfile{
-				ObjectMeta: metav1.ObjectMeta{
-					ResourceVersion: "dummy",
-					Name:            "dummy",
-				},
-				Spec: core.CloudProfileSpec{
-					Type: "aws",
-					MachineImages: []core.MachineImage{
-						{
-							Name: "some-machineimage",
-							Versions: []core.MachineImageVersion{
-								{
-									ExpirableVersion: core.ExpirableVersion{
-										Version: "1.2.3",
-									},
-									CRI: []core.CRI{{Name: "docker"}},
-								},
-							},
-						},
-					},
-					Kubernetes: core.KubernetesSettings{Versions: []core.ExpirableVersion{
-						{Version: "1.17.2"}}},
-					Regions: []core.Region{
-						{
-							Name: regionName,
-							Zones: []core.AvailabilityZone{
-								{Name: zoneName},
-							},
-						},
-					},
-					MachineTypes: machineTypesConstraint,
-					VolumeTypes:  volumeTypesConstraint,
-				},
-			}
-		})
-
-		Context("Kubernetes versions", func() {
-			It("it should return an error when creating a CloudProfile having a version with expiration date in the past", func() {
-				versions := []core.ExpirableVersion{
-					{Version: "1.17.2"},
-					{Version: "1.17.1", ExpirationDate: dateInThePast},
-				}
-				cloudProfile.Spec.Kubernetes.Versions = versions
-				errorList := ValidateCloudProfileCreation(cloudProfile)
-
-				Expect(errorList).To(HaveLen(1))
-				Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{
-					"Type":   Equal(field.ErrorTypeForbidden),
-					"Field":  Equal("spec.kubernetes.versions[1]"),
-					"Detail": ContainSubstring("a version with expiration date in the past is not allowed"),
-				}))))
-			})
-		})
-
-		Context("Machine image versions", func() {
-			It("it should return an error when creating a CloudProfile having a Machine image version with expiration date in the past", func() {
-				versions := []core.MachineImageVersion{
-					{
-						ExpirableVersion: core.ExpirableVersion{
-							Version: "1.17.2",
-						},
-						CRI: []core.CRI{{Name: "docker"}},
-					},
-					{
-						ExpirableVersion: core.ExpirableVersion{
-							Version:        "1.17.1",
-							ExpirationDate: dateInThePast,
-						},
-						CRI: []core.CRI{{Name: "docker"}},
-					},
-				}
-				cloudProfile.Spec.MachineImages[0].Versions = versions
-				errorList := ValidateCloudProfileCreation(cloudProfile)
-
-				Expect(errorList).To(HaveLen(1))
-				Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{
-					"Type":   Equal(field.ErrorTypeForbidden),
-					"Field":  Equal("spec.machineImages[0].versions[1]"),
-					"Detail": ContainSubstring("a version with expiration date in the past is not allowed"),
-				}))))
-			})
-		})
-	})
 })
diff --git a/pkg/registry/core/cloudprofile/cloudprofile_suite_test.go b/pkg/registry/core/cloudprofile/cloudprofile_suite_test.go
@@ -0,0 +1,27 @@
+// Copyright 2023 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cloudprofile_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestCloudProfile(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Registry Core CloudProfile Suite")
+}
diff --git a/pkg/registry/core/cloudprofile/strategy.go b/pkg/registry/core/cloudprofile/strategy.go
@@ -16,6 +16,7 @@ package cloudprofile
 
 import (
 	"context"
+	"time"
 
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -39,11 +40,14 @@ func (cloudProfileStrategy) NamespaceScoped() bool {
 }
 
 func (cloudProfileStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
+	cloudprofile := obj.(*core.CloudProfile)
+
+	dropExpiredVersions(cloudprofile)
 }
 
 func (cloudProfileStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	cloudprofile := obj.(*core.CloudProfile)
-	return validation.ValidateCloudProfileCreation(cloudprofile)
+	return validation.ValidateCloudProfile(cloudprofile)
 }
 
 func (cloudProfileStrategy) Canonicalize(obj runtime.Object) {
@@ -76,3 +80,29 @@ func (cloudProfileStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Ob
 func (cloudProfileStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string {
 	return nil
 }
+
+func dropExpiredVersions(cloudProfile *core.CloudProfile) {
+	var validKubernetesVersions []core.ExpirableVersion
+
+	for _, version := range cloudProfile.Spec.Kubernetes.Versions {
+		if version.ExpirationDate != nil && version.ExpirationDate.Time.Before(time.Now()) {
+			continue
+		}
+		validKubernetesVersions = append(validKubernetesVersions, version)
+	}
+
+	cloudProfile.Spec.Kubernetes.Versions = validKubernetesVersions
+
+	for i, machineImage := range cloudProfile.Spec.MachineImages {
+		var validMachineImageVersions []core.MachineImageVersion
+
+		for _, version := range machineImage.Versions {
+			if version.ExpirationDate != nil && version.ExpirationDate.Time.Before(time.Now()) {
+				continue
+			}
+			validMachineImageVersions = append(validMachineImageVersions, version)
+		}
+
+		cloudProfile.Spec.MachineImages[i].Versions = validMachineImageVersions
+	}
+}