Skip to content

Commit

Permalink
Fix cloud profile validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@hebelsan
hebelsan committed Dec 20, 2024
1 parent 6607210 commit e5ba54e
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 74 deletions.
6 changes: 2 additions & 4 deletions pkg/admission/validator/cloudprofile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ func NewCloudProfileValidator(mgr manager.Manager) extensionswebhook.Validator {
}
}

var cpProviderConfigPath = specPath.Child("providerConfig")

// Validate validates the given cloud profile objects.
func (cp *cloudProfile) Validate(_ context.Context, newObj, _ client.Object) error {
cloudProfile, ok := newObj.(*core.CloudProfile)
Expand All @@ -41,13 +39,13 @@ func (cp *cloudProfile) Validate(_ context.Context, newObj, _ client.Object) err
}

if cloudProfile.Spec.ProviderConfig == nil {
return field.Required(cpProviderConfigPath, "providerConfig must be set for GCP cloud profiles")
return field.Required(specPath.Child("providerConfig"), "providerConfig must be set for GCP cloud profiles")
}

cpConfig, err := admission.DecodeCloudProfileConfig(cp.decoder, cloudProfile.Spec.ProviderConfig)
if err != nil {
return err
}

return gcpvalidation.ValidateCloudProfileConfig(cpConfig, cloudProfile.Spec.MachineImages, cpProviderConfigPath).ToAggregate()
return gcpvalidation.ValidateCloudProfileConfig(cpConfig, cloudProfile.Spec.MachineImages, specPath).ToAggregate()
}
22 changes: 2 additions & 20 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 @@ -89,7 +88,7 @@ func (p *namespacedCloudProfile) validateMachineImages(providerConfig *api.Cloud

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 All @@ -104,7 +103,7 @@ func (p *namespacedCloudProfile) validateMachineImages(providerConfig *api.Cloud
for _, version := range machineImage.Versions {
_, existsInParent := parentImages.GetImageVersion(machineImage.Name, version.Version)
for _, expectedArchitecture := range version.Architectures {
if _, exists := providerImages.GetImageVersion(machineImage.Name, versionArchitectureKey(version.Version, expectedArchitecture)); !existsInParent && !exists {
if _, exists := providerImages.GetImageVersion(machineImage.Name, validation.VersionArchitectureKey(version.Version, expectedArchitecture)); !existsInParent && !exists {
allErrs = append(allErrs, field.Required(
field.NewPath("spec.providerConfig.machineImages"),
fmt.Sprintf("machine image version %s@%s is not defined in the NamespacedCloudProfile providerConfig", machineImage.Name, version.Version),
Expand Down Expand Up @@ -155,20 +154,3 @@ func (p *namespacedCloudProfile) validateMachineImages(providerConfig *api.Cloud

return allErrs
}

func providerMachineImageKey(v api.MachineImageVersion) string {
return versionArchitectureKey(v.Version, ptr.Deref(v.Architecture, constants.ArchitectureAMD64))
}

func versionArchitectureKey(version, architecture string) string {
return version + "-" + architecture
}

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 providerMachineImageKey(v) })
},
)
}
24 changes: 14 additions & 10 deletions pkg/admission/validator/namespacedcloudprofile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ var _ = Describe("NamespacedCloudProfile Validator", func() {
"Detail": Equal("machine image version is not defined in the NamespacedCloudProfile"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.providerConfig.machineImages[0].versions"),
"Detail": Equal("must provide an image mapping for version \"1.2\""),
"Field": Equal("spec.providerConfig.machineImages[0].versions[0]"),
"Detail": Equal("machine image version [email protected] and architecture: amd64 is not defined in the providerConfig"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeForbidden),
"Field": Equal("spec.providerConfig.machineImages"),
Expand Down Expand Up @@ -226,9 +226,13 @@ var _ = Describe("NamespacedCloudProfile Validator", func() {

err := namespacedCloudProfileValidator.Validate(ctx, namespacedCloudProfile, nil)
Expect(err).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeForbidden),
"Field": Equal("spec.providerConfig.machineImages"),
"Detail": Equal("machine image version [email protected] has an excess entry for architecture \"amd64\", which is not defined in the machineImages spec"),
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.providerConfig.machineImages[0].versions[1]"),
"Detail": Equal("machine image version [email protected] and architecture: arm64 is not defined in the providerConfig"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.providerConfig.machineImages[0].versions[2]"),
"Detail": Equal("machine image version [email protected] and architecture: arm64 is not defined in the providerConfig"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.providerConfig.machineImages"),
Expand All @@ -238,9 +242,9 @@ var _ = Describe("NamespacedCloudProfile Validator", func() {
"Field": Equal("spec.providerConfig.machineImages"),
"Detail": Equal("machine image version [email protected] is not defined in the NamespacedCloudProfile providerConfig"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.providerConfig.machineImages[0].versions"),
"Detail": Equal("must provide an image mapping for version \"1.1-missing\""),
"Type": Equal(field.ErrorTypeForbidden),
"Field": Equal("spec.providerConfig.machineImages"),
"Detail": Equal("machine image version image[email protected] has an excess entry for architecture \"amd64\", which is not defined in the machineImages spec"),
}))))
})

Expand All @@ -263,8 +267,8 @@ var _ = Describe("NamespacedCloudProfile Validator", func() {
err := namespacedCloudProfileValidator.Validate(ctx, namespacedCloudProfile, nil)
Expect(err).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.providerConfig.machineImages"),
"Detail": Equal("must provide an image mapping for image \"image-3\""),
"Field": Equal("spec.providerConfig.machineImages[0]"),
"Detail": Equal("must provide an image mapping for image \"image-3\" in providerConfig"),
})), PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("spec.providerConfig.machineImages"),
Expand Down
110 changes: 75 additions & 35 deletions pkg/apis/gcp/validation/cloudprofile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,60 +6,100 @@ package validation

import (
"fmt"
"slices"

"github.com/gardener/gardener/extensions/pkg/util"
"github.com/gardener/gardener/pkg/apis/core"
"github.com/gardener/gardener/pkg/apis/core/helper"
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"

apisgcp "github.com/gardener/gardener-extension-provider-gcp/pkg/apis/gcp"
)

// ValidateCloudProfileConfig validates a CloudProfileConfig object.
func ValidateCloudProfileConfig(cpConfig *apisgcp.CloudProfileConfig, machineImages []core.MachineImage, fldPath *field.Path) field.ErrorList {
func ValidateCloudProfileConfig(cpConfig *apisgcp.CloudProfileConfig, machineImages []core.MachineImage, specPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
machineImagesPath := fldPath.Child("machineImages")

for _, image := range machineImages {
var processed bool
for i, imageConfig := range cpConfig.MachineImages {
if image.Name == imageConfig.Name {
allErrs = append(allErrs, validateVersions(imageConfig.Versions, helper.ToExpirableVersions(image.Versions), machineImagesPath.Index(i).Child("versions"))...)
processed = true
break
}
cpConfigImagesContext := NewProviderImagesContext(cpConfig.MachineImages)

// validate machine images
for idxImage, machineImage := range machineImages {
if len(machineImage.Versions) == 0 {
continue
}
if !processed && len(image.Versions) > 0 {
allErrs = append(allErrs, field.Required(machineImagesPath, fmt.Sprintf("must provide an image mapping for image %q", image.Name)))
machineImagePath := specPath.Child("machineImages").Index(idxImage)
// validate that for each machine image there is a corresponding cpConfig image
if _, existsInConfig := cpConfigImagesContext.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
}
}

return allErrs
}

func validateVersions(versionsConfig []apisgcp.MachineImageVersion, versions []core.ExpirableVersion, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

for _, version := range versions {
var processed bool
for j, versionConfig := range versionsConfig {
jdxPath := fldPath.Index(j)
if version.Version == versionConfig.Version {
if len(versionConfig.Image) == 0 {
allErrs = append(allErrs, field.Required(jdxPath.Child("image"), "must provide an image"))
// 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))
}
if !slices.Contains(v1beta1constants.ValidArchitectures, *versionConfig.Architecture) {
allErrs = append(allErrs, field.NotSupported(jdxPath.Child("architecture"), *versionConfig.Architecture, v1beta1constants.ValidArchitectures))
// validate machine image version with architecture x exists in cpConfig
_, exists := cpConfigImagesContext.GetImageVersion(machineImage.Name, VersionArchitectureKey(version.Version, expectedArchitecture))
if !exists {
allErrs = append(allErrs, field.Required(machineImageVersionPath,
fmt.Sprintf("machine image version %s@%s and architecture: %s is not defined in the providerConfig",
machineImage.Name, version.Version, expectedArchitecture),
))
continue
}
processed = true
break
}
}
if !processed {
allErrs = append(allErrs, field.Required(fldPath, fmt.Sprintf("must provide an image mapping for version %q", version.Version)))
}

// validate all cpConfig images fields
for imgIdx, providerImage := range cpConfig.MachineImages {
imagePath := specPath.Child("providerConfig").Child("machineImages").Index(imgIdx)
for versionIdx, version := range providerImage.Versions {
imageVersionPath := imagePath.Child("versions").Index(versionIdx)
versionArch := ptr.Deref(version.Architecture, v1beta1constants.ArchitectureAMD64)
// validate image version image field
if version.Image == "" {
allErrs = append(allErrs, field.Required(
imageVersionPath.Child("image"),
fmt.Sprintf("must provide the image field for image version %s@%s and architecture: %s",
providerImage.Name, version.Version, versionArch)))
}
// validate architecture field
if !slices.Contains(v1beta1constants.ValidArchitectures, versionArch) {
allErrs = append(allErrs, field.NotSupported(
imageVersionPath.Child("architecture"),
versionArch, v1beta1constants.ValidArchitectures))

}
}
}

return allErrs
}

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

func providerMachineImageKey(v apisgcp.MachineImageVersion) string {
return VersionArchitectureKey(v.Version, ptr.Deref(v.Architecture, v1beta1constants.ArchitectureAMD64))
}

// VersionArchitectureKey returns a key for a version and architecture.
func VersionArchitectureKey(version, architecture string) string {
return version + "-" + architecture
}
24 changes: 19 additions & 5 deletions pkg/apis/gcp/validation/cloudprofile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ 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 Down Expand Up @@ -37,7 +38,7 @@ var _ = Describe("CloudProfileConfig validation", func() {
{
Version: machineImageVersion,
Image: "path/to/gcp/image",
Architecture: ptr.To("amd64"),
Architecture: ptr.To(v1beta1constants.ArchitectureAMD64),
},
},
},
Expand All @@ -49,6 +50,7 @@ var _ = Describe("CloudProfileConfig validation", func() {
Versions: []core.MachineImageVersion{
{
ExpirableVersion: core.ExpirableVersion{Version: machineImageVersion},
Architectures: []string{v1beta1constants.ArchitectureAMD64},
},
},
},
Expand Down Expand Up @@ -88,7 +90,19 @@ var _ = Describe("CloudProfileConfig validation", func() {
Expect(errorList).To(ConsistOf(
PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("machineImages"),
"Field": Equal("machineImages[1]"),
})),
))
})

It("should forbid missing architecture mapping", func() {
machineImages[0].Versions[0].Architectures = []string{"arm64"}
errorList := ValidateCloudProfileConfig(cloudProfileConfig, machineImages, nilPath)

Expect(errorList).To(ConsistOf(
PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("machineImages[0].versions[0]"),
})),
))
})
Expand All @@ -102,15 +116,15 @@ var _ = Describe("CloudProfileConfig validation", func() {
Expect(errorList).To(ConsistOf(
PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("machineImages[0].versions"),
"Field": Equal("machineImages[0].versions[0]"),
})),
PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeRequired),
"Field": Equal("machineImages[0].versions[0].image"),
"Field": Equal("providerConfig.machineImages[0].versions[0].image"),
})),
PointTo(MatchFields(IgnoreExtras, Fields{
"Type": Equal(field.ErrorTypeNotSupported),
"Field": Equal("machineImages[0].versions[0].architecture"),
"Field": Equal("providerConfig.machineImages[0].versions[0].architecture"),
})),
))
})
Expand Down

0 comments on commit e5ba54e

Please sign in to comment.