Skip to content

Commit

Permalink
Fix cloud profile machineImages validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@hebelsan
hebelsan committed Dec 23, 2024
1 parent 687ca51 commit c740ae5
Show file tree
Hide file tree
Showing 4 changed files with 158 additions and 37 deletions.
2 changes: 1 addition & 1 deletion pkg/admission/validator/cloudprofile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,5 @@ func (cp *cloudProfile) Validate(_ context.Context, new, _ client.Object) error
return err
}

return awsvalidation.ValidateCloudProfileConfig(cpConfig, providerConfigPath).ToAggregate()
return awsvalidation.ValidateCloudProfileConfig(cpConfig, cloudProfile.Spec.MachineImages, providerConfigPath).ToAggregate()
}
14 changes: 2 additions & 12 deletions pkg/admission/validator/namespacedcloudprofile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer"
"k8s.io/apimachinery/pkg/util/validation/field"
Expand Down Expand Up @@ -86,12 +85,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.
Expand Down Expand Up @@ -187,12 +186,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 })
},
)
}
86 changes: 79 additions & 7 deletions pkg/apis/aws/validation/cloudprofile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,21 @@ package validation

import (
"fmt"
"maps"
"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/validation/field"
"k8s.io/utils/strings/slices"
"k8s.io/utils/ptr"

apisaws "github.com/gardener/gardener-extension-provider-aws/pkg/apis/aws"
)

// ValidateCloudProfileConfig validates a CloudProfileConfig object.
func ValidateCloudProfileConfig(cloudProfile *apisaws.CloudProfileConfig, fldPath *field.Path) field.ErrorList {
func ValidateCloudProfileConfig(cloudProfile *apisaws.CloudProfileConfig, machineImages []core.MachineImage, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

machineImagesPath := fldPath.Child("machineImages")
Expand All @@ -24,14 +29,15 @@ func ValidateCloudProfileConfig(cloudProfile *apisaws.CloudProfileConfig, fldPat
}
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"))...)

return allErrs
}

// ValidateMachineImage validates a CloudProfileConfig MachineImages entry.
func ValidateMachineImage(validationPath *field.Path, machineImage apisaws.MachineImages) field.ErrorList {
// ValidateProviderMachineImage validates a CloudProfileConfig MachineImages entry.
func ValidateProviderMachineImage(validationPath *field.Path, machineImage apisaws.MachineImages) field.ErrorList {
allErrs := field.ErrorList{}

if len(machineImage.Name) == 0 {
Expand All @@ -53,15 +59,81 @@ func ValidateMachineImage(validationPath *field.Path, machineImage apisaws.Machi
}
for k, region := range version.Regions {
kdxPath := jdxPath.Child("regions").Index(k)
arch := ptr.Deref(region.Architecture, v1beta1constants.ArchitectureAMD64)

if len(region.Name) == 0 {
allErrs = append(allErrs, field.Required(kdxPath.Child("name"), "must provide a name"))
}
if len(region.AMI) == 0 {
allErrs = append(allErrs, field.Required(kdxPath.Child("ami"), "must provide an ami"))
}
if !slices.Contains(v1beta1constants.ValidArchitectures, *region.Architecture) {
allErrs = append(allErrs, field.NotSupported(kdxPath.Child("architecture"), *region.Architecture, v1beta1constants.ValidArchitectures))
if !slices.Contains(v1beta1constants.ValidArchitectures, arch) {
allErrs = append(allErrs, field.NotSupported(kdxPath.Child("architecture"), arch, v1beta1constants.ValidArchitectures))
}
}
}

return allErrs
}

// NewProviderImagesContext creates a new ImagesContext for provider images.
func NewProviderImagesContext(providerImages []apisaws.MachineImages) *util.ImagesContext[apisaws.MachineImages, apisaws.MachineImageVersion] {
return util.NewImagesContext(
utils.CreateMapFromSlice(providerImages, func(mi apisaws.MachineImages) string { return mi.Name }),
func(mi apisaws.MachineImages) map[string]apisaws.MachineImageVersion {
return utils.CreateMapFromSlice(mi.Versions, func(v apisaws.MachineImageVersion) string { return v.Version })
},
)
}

// validateMachineImageMapping validates that for each machine image there is a corresponding cpConfig image.
func validateMachineImageMapping(machineImages []core.MachineImage, cpConfig *apisaws.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 apisaws.RegionAMIMapping) 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
}
}
}
}
Expand Down
93 changes: 76 additions & 17 deletions pkg/apis/aws/validation/cloudprofile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -17,53 +19,83 @@ import (

var _ = Describe("CloudProfileConfig validation", func() {
Describe("#ValidateCloudProfileConfig", func() {
var cloudProfileConfig *apisaws.CloudProfileConfig
var (
cloudProfileConfig *apisaws.CloudProfileConfig
machineImages []core.MachineImage
machineImageName string
machineImageVersion string
fldPath *field.Path
)

BeforeEach(func() {
machineImageName = "ubuntu"
machineImageVersion = "1.2.3"
cloudProfileConfig = &apisaws.CloudProfileConfig{
MachineImages: []apisaws.MachineImages{
{
Name: "ubuntu",
Name: machineImageName,
Versions: []apisaws.MachineImageVersion{
{
Version: "1.2.3",
Version: machineImageVersion,
Regions: []apisaws.RegionAMIMapping{
{
Name: "eu",
AMI: "ami-1234",
Architecture: ptr.To("amd64"),
Architecture: ptr.To(v1beta1constants.ArchitectureAMD64),
},
},
},
},
},
},
}
machineImages = []core.MachineImage{
{
Name: machineImageName,
Versions: []core.MachineImageVersion{
{
ExpirableVersion: core.ExpirableVersion{Version: machineImageVersion},
Architectures: []string{v1beta1constants.ArchitectureAMD64},
},
},
},
}
})

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 = []apisaws.MachineImages{}

errorList := ValidateCloudProfileConfig(cloudProfileConfig, field.NewPath("root"))
errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath)

Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("root.machineImages"),
"Field": Equal("machineImages"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.machineImages[0]"),
}))))
})

It("should forbid unsupported machine image configuration", func() {
cloudProfileConfig.MachineImages = []apisaws.MachineImages{{}}

errorList := ValidateCloudProfileConfig(cloudProfileConfig, field.NewPath("root"))
errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, fldPath)

Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("root.machineImages[0].name"),
"Field": Equal("machineImages[0].name"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("machineImages[0].versions"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("root.machineImages[0].versions"),
"Field": Equal("spec.machineImages[0]"),
}))))
})

Expand All @@ -75,14 +107,17 @@ var _ = Describe("CloudProfileConfig validation", func() {
},
}

errorList := ValidateCloudProfileConfig(cloudProfileConfig, field.NewPath("root"))
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"),
"Field": Equal("machineImages[0].versions[0].version"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("root.machineImages[0].versions[0].regions"),
"Field": Equal("machineImages[0].versions[0].regions"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.machineImages[0]"),
}))))
})

Expand All @@ -99,27 +134,51 @@ var _ = Describe("CloudProfileConfig validation", func() {
},
}

errorList := ValidateCloudProfileConfig(cloudProfileConfig, field.NewPath("root"))
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"),
"Field": Equal("machineImages[0].versions[0].regions[0].name"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("machineImages[0].versions[0].regions[0].ami"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("root.machineImages[0].versions[0].regions[0].ami"),
"Field": Equal("spec.machineImages[0]"),
}))))
})

It("should forbid unsupported machine image architecture configuration", func() {
cloudProfileConfig.MachineImages[0].Versions[0].Regions[0].Architecture = ptr.To("foo")

errorList := ValidateCloudProfileConfig(cloudProfileConfig, field.NewPath("root"))
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[0].architecture"),
"Field": Equal("machineImages[0].versions[0].regions[0].architecture"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.machineImages[0].versions[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())
})
})
})
})

0 comments on commit c740ae5

Please sign in to comment.