From 1ebc76808ad9a5da5d6962ed76debb173a7f54bf Mon Sep 17 00:00:00 2001 From: Alexander Hebel Date: Mon, 23 Dec 2024 14:42:17 +0100 Subject: [PATCH] Fix cloud profile machineImages validation --- pkg/admission/validator/cloudprofile.go | 2 +- .../validator/namespacedcloudprofile.go | 14 +-- pkg/apis/openstack/validation/cloudprofile.go | 77 +++++++++++- .../openstack/validation/cloudprofile_test.go | 116 ++++++++++++++---- 4 files changed, 166 insertions(+), 43 deletions(-) diff --git a/pkg/admission/validator/cloudprofile.go b/pkg/admission/validator/cloudprofile.go index 621939cb4..cf0046d39 100644 --- a/pkg/admission/validator/cloudprofile.go +++ b/pkg/admission/validator/cloudprofile.go @@ -47,5 +47,5 @@ func (cp *cloudProfile) Validate(_ context.Context, newObj, _ client.Object) err return err } - return openstackvalidation.ValidateCloudProfileConfig(cpConfig, providerConfigPath).ToAggregate() + return openstackvalidation.ValidateCloudProfileConfig(cpConfig, cloudProfile.Spec.MachineImages, providerConfigPath).ToAggregate() } diff --git a/pkg/admission/validator/namespacedcloudprofile.go b/pkg/admission/validator/namespacedcloudprofile.go index 435a82a9d..5fd525fbb 100644 --- a/pkg/admission/validator/namespacedcloudprofile.go +++ b/pkg/admission/validator/namespacedcloudprofile.go @@ -14,7 +14,6 @@ import ( "github.com/gardener/gardener/pkg/apis/core" gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1" "github.com/gardener/gardener/pkg/apis/core/v1beta1/constants" - "github.com/gardener/gardener/pkg/utils" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -97,12 +96,12 @@ func (p *namespacedCloudProfile) validateMachineImages(providerConfig *api.Cloud machineImagesPath := field.NewPath("spec.providerConfig.machineImages") for i, machineImage := range providerConfig.MachineImages { idxPath := machineImagesPath.Index(i) - allErrs = append(allErrs, validation.ValidateMachineImage(idxPath, machineImage)...) + allErrs = append(allErrs, validation.ValidateProviderMachineImage(idxPath, machineImage)...) } profileImages := util.NewCoreImagesContext(machineImages) parentImages := util.NewV1beta1ImagesContext(parentSpec.MachineImages) - providerImages := newProviderImagesContext(providerConfig.MachineImages) + providerImages := validation.NewProviderImagesContext(providerConfig.MachineImages) for _, machineImage := range profileImages.Images { // Check that for each new image version defined in the NamespacedCloudProfile, the image is also defined in the providerConfig. @@ -198,12 +197,3 @@ func validateMachineImageArchitectures(machineImage core.MachineImage, version c return allErrs } - -func newProviderImagesContext(providerImages []api.MachineImages) *util.ImagesContext[api.MachineImages, api.MachineImageVersion] { - return util.NewImagesContext( - utils.CreateMapFromSlice(providerImages, func(mi api.MachineImages) string { return mi.Name }), - func(mi api.MachineImages) map[string]api.MachineImageVersion { - return utils.CreateMapFromSlice(mi.Versions, func(v api.MachineImageVersion) string { return v.Version }) - }, - ) -} diff --git a/pkg/apis/openstack/validation/cloudprofile.go b/pkg/apis/openstack/validation/cloudprofile.go index c67d127ac..02fba6ed1 100644 --- a/pkg/apis/openstack/validation/cloudprofile.go +++ b/pkg/apis/openstack/validation/cloudprofile.go @@ -6,9 +6,12 @@ package validation import ( "fmt" + "maps" "net" "slices" + "github.com/gardener/gardener/extensions/pkg/util" + "github.com/gardener/gardener/pkg/apis/core" v1beta1constants "github.com/gardener/gardener/pkg/apis/core/v1beta1/constants" "github.com/gardener/gardener/pkg/utils" "k8s.io/apimachinery/pkg/util/sets" @@ -19,7 +22,7 @@ import ( ) // ValidateCloudProfileConfig validates a CloudProfileConfig object. -func ValidateCloudProfileConfig(cloudProfile *api.CloudProfileConfig, fldPath *field.Path) field.ErrorList { +func ValidateCloudProfileConfig(cloudProfile *api.CloudProfileConfig, machineImages []core.MachineImage, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} floatingPoolPath := fldPath.Child("constraints", "floatingPools") @@ -93,8 +96,9 @@ func ValidateCloudProfileConfig(cloudProfile *api.CloudProfileConfig, fldPath *f } for i, machineImage := range cloudProfile.MachineImages { idxPath := machineImagesPath.Index(i) - allErrs = append(allErrs, ValidateMachineImage(idxPath, machineImage)...) + allErrs = append(allErrs, ValidateProviderMachineImage(idxPath, machineImage)...) } + allErrs = append(allErrs, validateMachineImageMapping(machineImages, cloudProfile, field.NewPath("spec").Child("machineImages"))...) if len(cloudProfile.KeyStoneURL) == 0 && len(cloudProfile.KeyStoneURLs) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("keyStoneURL"), "must provide the URL to KeyStone")) @@ -153,8 +157,8 @@ func ValidateCloudProfileConfig(cloudProfile *api.CloudProfileConfig, fldPath *f return allErrs } -// ValidateMachineImage validates a CloudProfileConfig MachineImages entry. -func ValidateMachineImage(validationPath *field.Path, machineImage api.MachineImages) field.ErrorList { +// ValidateProviderMachineImage validates a CloudProfileConfig MachineImages entry. +func ValidateProviderMachineImage(validationPath *field.Path, machineImage api.MachineImages) field.ErrorList { allErrs := field.ErrorList{} if len(machineImage.Name) == 0 { @@ -189,6 +193,71 @@ func ValidateMachineImage(validationPath *field.Path, machineImage api.MachineIm return allErrs } +// NewProviderImagesContext creates a new ImagesContext for provider images. +func NewProviderImagesContext(providerImages []api.MachineImages) *util.ImagesContext[api.MachineImages, api.MachineImageVersion] { + return util.NewImagesContext( + utils.CreateMapFromSlice(providerImages, func(mi api.MachineImages) string { return mi.Name }), + func(mi api.MachineImages) map[string]api.MachineImageVersion { + return utils.CreateMapFromSlice(mi.Versions, func(v api.MachineImageVersion) string { return v.Version }) + }, + ) +} + +// validateMachineImageMapping validates that for each machine image there is a corresponding cpConfig image. +func validateMachineImageMapping(machineImages []core.MachineImage, cpConfig *api.CloudProfileConfig, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + providerImages := NewProviderImagesContext(cpConfig.MachineImages) + + // validate machine images + for idxImage, machineImage := range machineImages { + if len(machineImage.Versions) == 0 { + continue + } + machineImagePath := fldPath.Index(idxImage) + // validate that for each machine image there is a corresponding cpConfig image + if _, existsInConfig := providerImages.GetImage(machineImage.Name); !existsInConfig { + allErrs = append(allErrs, field.Required(machineImagePath, + fmt.Sprintf("must provide an image mapping for image %q in providerConfig", machineImage.Name))) + continue + } + // validate that for each machine image version entry a mapped entry in cpConfig exists + for idxVersion, version := range machineImage.Versions { + machineImageVersionPath := machineImagePath.Child("versions").Index(idxVersion) + for _, expectedArchitecture := range version.Architectures { + // validate machine image version architectures + if !slices.Contains(v1beta1constants.ValidArchitectures, expectedArchitecture) { + allErrs = append(allErrs, field.NotSupported( + machineImageVersionPath.Child("architectures"), + expectedArchitecture, v1beta1constants.ValidArchitectures)) + } + // validate that machine image version exists in cpConfig + imageVersion, exists := providerImages.GetImageVersion(machineImage.Name, version.Version) + if !exists { + allErrs = append(allErrs, field.Required(machineImageVersionPath, + fmt.Sprintf("machine image version %s@%s is not defined in the providerConfig", + machineImage.Name, version.Version), + )) + continue + } + // validate that machine image version with architecture x exists in cpConfig + architecturesMap := utils.CreateMapFromSlice(imageVersion.Regions, func(re api.RegionIDMapping) string { + return ptr.Deref(re.Architecture, v1beta1constants.ArchitectureAMD64) + }) + architectures := slices.Collect(maps.Keys(architecturesMap)) + if !slices.Contains(architectures, expectedArchitecture) { + allErrs = append(allErrs, field.Required(machineImageVersionPath, + fmt.Sprintf("missing providerConfig mapping for machine image version %s@%s and architecture: %s", + machineImage.Name, version.Version, expectedArchitecture), + )) + continue + } + } + } + } + + return allErrs +} + // validateLoadBalancerClass validates LoadBalancerClass object. func validateLoadBalancerClass(lbClass api.LoadBalancerClass, fldPath *field.Path) field.ErrorList { var allErrs = field.ErrorList{} diff --git a/pkg/apis/openstack/validation/cloudprofile_test.go b/pkg/apis/openstack/validation/cloudprofile_test.go index 6e7466fbf..131499407 100644 --- a/pkg/apis/openstack/validation/cloudprofile_test.go +++ b/pkg/apis/openstack/validation/cloudprofile_test.go @@ -5,6 +5,8 @@ package validation_test import ( + "github.com/gardener/gardener/pkg/apis/core" + v1beta1constants "github.com/gardener/gardener/pkg/apis/core/v1beta1/constants" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" @@ -18,11 +20,16 @@ import ( var _ = Describe("CloudProfileConfig validation", func() { Describe("#ValidateCloudProfileConfig", func() { var ( - cloudProfileConfig *api.CloudProfileConfig - fldPath *field.Path + cloudProfileConfig *api.CloudProfileConfig + machineImages []core.MachineImage + machineImageName string + machineImageVersion string + fldPath *field.Path ) BeforeEach(func() { + machineImageName = "ubuntu" + machineImageVersion = "1.2.3" cloudProfileConfig = &api.CloudProfileConfig{ Constraints: api.Constraints{ FloatingPools: []api.FloatingPool{ @@ -39,16 +46,32 @@ var _ = Describe("CloudProfileConfig validation", func() { KeyStoneURL: "http://url-to-keystone/v3", MachineImages: []api.MachineImages{ { - Name: "ubuntu", + Name: machineImageName, Versions: []api.MachineImageVersion{ { - Version: "1.2.3", + Version: machineImageVersion, Image: "ubuntu-1.2.3", + Regions: []api.RegionIDMapping{{ + Name: "eu01", + ID: "9afa968b-ed9e-4ba0-a394-f74cbb0313w2", + Architecture: ptr.To(v1beta1constants.ArchitectureAMD64), + }}, }, }, }, }, } + machineImages = []core.MachineImage{ + { + Name: machineImageName, + Versions: []core.MachineImageVersion{ + { + ExpirableVersion: core.ExpirableVersion{Version: machineImageVersion}, + Architectures: []string{v1beta1constants.ArchitectureAMD64}, + }, + }, + }, + } fldPath = field.NewPath("root") }) @@ -56,7 +79,7 @@ var _ = Describe("CloudProfileConfig validation", func() { It("should enforce that at least one pool has been defined", func() { cloudProfileConfig.Constraints.FloatingPools = []api.FloatingPool{} - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), @@ -73,7 +96,7 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), @@ -117,7 +140,7 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf( PointTo(MatchFields(IgnoreExtras, Fields{ @@ -142,7 +165,7 @@ var _ = Describe("CloudProfileConfig validation", func() { It("should enforce that at least one provider has been defined", func() { cloudProfileConfig.Constraints.LoadBalancerProviders = []api.LoadBalancerProvider{} - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), @@ -158,7 +181,7 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), @@ -181,7 +204,7 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeDuplicate), @@ -201,7 +224,7 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(BeEmpty()) }) @@ -216,7 +239,7 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(BeEmpty()) }) @@ -240,7 +263,7 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(BeEmpty()) }) @@ -250,7 +273,7 @@ var _ = Describe("CloudProfileConfig validation", func() { cloudProfileConfig.KeyStoneURL = "" cloudProfileConfig.KeyStoneURLs = nil - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), @@ -262,7 +285,7 @@ var _ = Describe("CloudProfileConfig validation", func() { cloudProfileConfig.KeyStoneURL = "" cloudProfileConfig.KeyStoneURLs = []api.KeyStoneURL{{}} - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), @@ -286,7 +309,7 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeDuplicate), @@ -298,7 +321,7 @@ var _ = Describe("CloudProfileConfig validation", func() { It("should forbid invalid keystone CA Certs", func() { cloudProfileConfig.KeyStoneCACert = ptr.To("foo") - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeInvalid), "Field": Equal("root.caCert"), @@ -310,7 +333,7 @@ var _ = Describe("CloudProfileConfig validation", func() { It("should forbid not invalid dns server ips", func() { cloudProfileConfig.DNSServers = []string{"not-a-valid-ip"} - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeInvalid), @@ -323,7 +346,7 @@ var _ = Describe("CloudProfileConfig validation", func() { It("should forbid not specifying a value when the key is present", func() { cloudProfileConfig.DHCPDomain = ptr.To("") - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), @@ -333,21 +356,29 @@ var _ = Describe("CloudProfileConfig validation", func() { }) Context("machine image validation", func() { + It("should pass validation", func() { + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) + Expect(errorList).To(BeEmpty()) + }) + It("should enforce that at least one machine image has been defined", func() { cloudProfileConfig.MachineImages = []api.MachineImages{} - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), "Field": Equal("root.machineImages"), + })), PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.machineImages[0]"), })))) }) It("should forbid unsupported machine image configuration", func() { cloudProfileConfig.MachineImages = []api.MachineImages{{}} - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), @@ -355,6 +386,9 @@ var _ = Describe("CloudProfileConfig validation", func() { })), PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), "Field": Equal("root.machineImages[0].versions"), + })), PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.machineImages[0]"), })))) }) @@ -366,14 +400,35 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), "Field": Equal("root.machineImages[0].versions[0].version"), + })), PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.machineImages[0]"), })))) }) + It("should forbid missing architecture mapping", func() { + machineImages[0].Versions[0].Architectures = []string{"arm64"} + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) + + Expect(errorList).To(ConsistOf( + PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.machineImages[0].versions[0]"), + })), + )) + }) + + It("should automatically use amd64", func() { + cloudProfileConfig.MachineImages[0].Versions[0].Regions[0].Architecture = nil + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) + Expect(errorList).To(BeEmpty()) + }) + Context("region mapping validation", func() { It("should forbid empty region name", func() { cloudProfileConfig.MachineImages = []api.MachineImages{ @@ -388,11 +443,14 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), "Field": Equal("root.machineImages[0].versions[0].regions[0].name"), + })), PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.machineImages[0]"), })))) }) @@ -409,11 +467,14 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired), "Field": Equal("root.machineImages[0].versions[0].regions[0].id"), + })), PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.machineImages[0]"), })))) }) @@ -444,11 +505,14 @@ var _ = Describe("CloudProfileConfig validation", func() { }, } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeNotSupported), "Field": Equal("root.machineImages[0].versions[0].regions[2].architecture"), + })), PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.machineImages[0]"), })))) }) }) @@ -461,7 +525,7 @@ var _ = Describe("CloudProfileConfig validation", func() { "", } - errorList := ValidateCloudProfileConfig(cloudProfileConfig, fldPath) + errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeRequired),