From c108fa8eaff7a382b65f1103703303602c75980f Mon Sep 17 00:00:00 2001 From: Luca Bernstein Date: Mon, 22 Jul 2024 14:43:53 +0200 Subject: [PATCH] Add NamespacedCloudProfile support to Shoot validation. Switch from CloudProfile to CloudProfileSpec as a generic foundation for CloudProfile and NamespacedCloudProfile. Improve log message for possibly artificially crafted CloudProfile from NamespacedCloudProfile in Cluster resource. --- .../charts/application/templates/rbac.yaml | 1 + docs/usage/usage.md | 9 +- pkg/admission/validator/shoot.go | 39 ++- pkg/admission/validator/shoot_test.go | 269 ++++++++++++++++++ pkg/apis/azure/helper/scheme.go | 6 +- pkg/apis/azure/validation/infrastructure.go | 4 +- .../azure/validation/infrastructure_test.go | 4 +- 7 files changed, 311 insertions(+), 21 deletions(-) create mode 100644 pkg/admission/validator/shoot_test.go diff --git a/charts/gardener-extension-admission-azure/charts/application/templates/rbac.yaml b/charts/gardener-extension-admission-azure/charts/application/templates/rbac.yaml index fb74c7711..a76285f47 100644 --- a/charts/gardener-extension-admission-azure/charts/application/templates/rbac.yaml +++ b/charts/gardener-extension-admission-azure/charts/application/templates/rbac.yaml @@ -10,6 +10,7 @@ rules: - core.gardener.cloud resources: - cloudprofiles + - namespacedcloudprofiles verbs: - get - list diff --git a/docs/usage/usage.md b/docs/usage/usage.md index 08744a73d..8af9380d4 100644 --- a/docs/usage/usage.md +++ b/docs/usage/usage.md @@ -379,7 +379,8 @@ metadata: name: johndoe-azure namespace: garden-dev spec: - cloudProfileName: azure + cloudProfile: + name: azure region: westeurope secretBindingName: core-azure provider: @@ -441,7 +442,8 @@ metadata: name: johndoe-azure namespace: garden-dev spec: - cloudProfileName: azure + cloudProfile: + name: azure region: westeurope secretBindingName: core-azure provider: @@ -498,7 +500,8 @@ metadata: name: johndoe-azure namespace: garden-dev spec: - cloudProfileName: azure + cloudProfile: + name: azure region: westeurope secretBindingName: core-azure provider: diff --git a/pkg/admission/validator/shoot.go b/pkg/admission/validator/shoot.go index 5e79397fb..6c5c22c39 100644 --- a/pkg/admission/validator/shoot.go +++ b/pkg/admission/validator/shoot.go @@ -13,6 +13,8 @@ import ( "github.com/gardener/gardener/pkg/apis/core" gardencorehelper "github.com/gardener/gardener/pkg/apis/core/helper" gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1" + "github.com/gardener/gardener/pkg/utils/gardener" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/validation/field" @@ -37,6 +39,7 @@ var ( // shoot validates shoots type shoot struct { client client.Client + scheme *runtime.Scheme decoder runtime.Decoder lenientDecoder runtime.Decoder } @@ -45,6 +48,7 @@ type shoot struct { func NewShootValidator(mgr manager.Manager) extensionswebhook.Validator { return &shoot{ client: mgr.GetClient(), + scheme: mgr.GetScheme(), decoder: serializer.NewCodecFactory(mgr.GetScheme(), serializer.EnableStrict).UniversalDecoder(), lenientDecoder: serializer.NewCodecFactory(mgr.GetScheme()).UniversalDecoder(), } @@ -62,27 +66,36 @@ func (s *shoot) Validate(ctx context.Context, newObj, oldObj client.Object) erro return nil } - if shoot.Spec.CloudProfile == nil { - return fmt.Errorf("shoot.spec.cloudprofile must not be nil ") + shootV1Beta1 := &gardencorev1beta1.Shoot{ + TypeMeta: metav1.TypeMeta{ + APIVersion: gardencorev1beta1.SchemeGroupVersion.String(), + Kind: "Shoot", + }, } - - cloudProfile := &gardencorev1beta1.CloudProfile{} - if err := s.client.Get(ctx, client.ObjectKey{Name: shoot.Spec.CloudProfile.Name}, cloudProfile); err != nil { + err := s.scheme.Convert(shoot, shootV1Beta1, ctx) + if err != nil { + return err + } + cloudProfile, err := gardener.GetCloudProfile(ctx, s.client, shootV1Beta1) + if err != nil { return err } + if cloudProfile == nil { + return fmt.Errorf("cloudprofile could not be found") + } if oldObj != nil { oldShoot, ok := oldObj.(*core.Shoot) if !ok { return fmt.Errorf("wrong object type %T for old object", oldObj) } - return s.validateUpdate(oldShoot, shoot, cloudProfile) + return s.validateUpdate(oldShoot, shoot, &cloudProfile.Spec) } - return s.validateCreation(ctx, shoot, cloudProfile) + return s.validateCreation(ctx, shoot, &cloudProfile.Spec) } -func (s *shoot) validateCreation(_ context.Context, shoot *core.Shoot, cloudProfile *gardencorev1beta1.CloudProfile) error { +func (s *shoot) validateCreation(_ context.Context, shoot *core.Shoot, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec) error { infraConfig, err := checkAndDecodeInfrastructureConfig(s.decoder, shoot.Spec.Provider.InfrastructureConfig, infraConfigPath) if err != nil { return err @@ -96,10 +109,10 @@ func (s *shoot) validateCreation(_ context.Context, shoot *core.Shoot, cloudProf } } - return s.validateShoot(shoot, nil, infraConfig, cloudProfile, cpConfig).ToAggregate() + return s.validateShoot(shoot, nil, infraConfig, cloudProfileSpec, cpConfig).ToAggregate() } -func (s *shoot) validateShoot(shoot *core.Shoot, oldInfraConfig, infraConfig *api.InfrastructureConfig, cloudProfile *gardencorev1beta1.CloudProfile, cpConfig *api.ControlPlaneConfig) field.ErrorList { +func (s *shoot) validateShoot(shoot *core.Shoot, oldInfraConfig, infraConfig *api.InfrastructureConfig, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec, cpConfig *api.ControlPlaneConfig) field.ErrorList { allErrs := field.ErrorList{} // Network validation @@ -107,7 +120,7 @@ func (s *shoot) validateShoot(shoot *core.Shoot, oldInfraConfig, infraConfig *ap if infraConfig != nil { // Cloudprofile validation - allErrs = append(allErrs, azurevalidation.ValidateInfrastructureConfigAgainstCloudProfile(oldInfraConfig, infraConfig, shoot.Spec.Region, cloudProfile, infraConfigPath)...) + allErrs = append(allErrs, azurevalidation.ValidateInfrastructureConfigAgainstCloudProfile(oldInfraConfig, infraConfig, shoot.Spec.Region, cloudProfileSpec, infraConfigPath)...) // Provider validation allErrs = append(allErrs, azurevalidation.ValidateInfrastructureConfig(infraConfig, shoot.Spec.Networking, helper.HasShootVmoAlphaAnnotation(shoot.Annotations), infraConfigPath)...) } @@ -131,7 +144,7 @@ func (s *shoot) validateShoot(shoot *core.Shoot, oldInfraConfig, infraConfig *ap return allErrs } -func (s *shoot) validateUpdate(oldShoot, shoot *core.Shoot, cloudProfile *gardencorev1beta1.CloudProfile) error { +func (s *shoot) validateUpdate(oldShoot, shoot *core.Shoot, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec) error { // Decode the new infrastructure config. if shoot.Spec.Provider.InfrastructureConfig == nil { return field.Required(infraConfigPath, "InfrastructureConfig must be set for Azure shoots") @@ -167,7 +180,7 @@ func (s *shoot) validateUpdate(oldShoot, shoot *core.Shoot, cloudProfile *garden allErrs = append(allErrs, azurevalidation.ValidateVmoConfigUpdate(helper.HasShootVmoAlphaAnnotation(oldShoot.Annotations), helper.HasShootVmoAlphaAnnotation(shoot.Annotations), metaDataPath)...) allErrs = append(allErrs, azurevalidation.ValidateWorkersUpdate(oldShoot.Spec.Provider.Workers, shoot.Spec.Provider.Workers, workersPath)...) - allErrs = append(allErrs, s.validateShoot(shoot, oldInfraConfig, infraConfig, cloudProfile, cpConfig)...) + allErrs = append(allErrs, s.validateShoot(shoot, oldInfraConfig, infraConfig, cloudProfileSpec, cpConfig)...) return allErrs.ToAggregate() } diff --git a/pkg/admission/validator/shoot_test.go b/pkg/admission/validator/shoot_test.go new file mode 100644 index 000000000..c1a3fc0c7 --- /dev/null +++ b/pkg/admission/validator/shoot_test.go @@ -0,0 +1,269 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Gardener contributors +// +// SPDX-License-Identifier: Apache-2.0 + +package validator_test + +import ( + "context" + "encoding/json" + + extensionswebhook "github.com/gardener/gardener/extensions/pkg/webhook" + "github.com/gardener/gardener/pkg/apis/core" + gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1" + mockclient "github.com/gardener/gardener/third_party/mock/controller-runtime/client" + mockmanager "github.com/gardener/gardener/third_party/mock/controller-runtime/manager" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + . "github.com/onsi/gomega/gstruct" + "go.uber.org/mock/gomock" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/gardener/gardener-extension-provider-azure/pkg/admission/validator" + apisazure "github.com/gardener/gardener-extension-provider-azure/pkg/apis/azure" + apisazurev1alpha1 "github.com/gardener/gardener-extension-provider-azure/pkg/apis/azure/v1alpha1" + "github.com/gardener/gardener-extension-provider-azure/pkg/azure" +) + +var _ = Describe("Shoot validator", func() { + Describe("#Validate", func() { + const namespace = "garden-dev" + + var ( + shootValidator extensionswebhook.Validator + + ctrl *gomock.Controller + mgr *mockmanager.MockManager + c *mockclient.MockClient + cloudProfile *gardencorev1beta1.CloudProfile + namespacedCloudProfile *gardencorev1beta1.NamespacedCloudProfile + shoot *core.Shoot + + ctx = context.TODO() + cloudProfileKey = client.ObjectKey{Name: "azure"} + namespacedCloudProfileKey = client.ObjectKey{Name: "azure-nscpfl", Namespace: namespace} + + regionName = "westus" + imageName = "Foo" + imageVersion = "1.0.0" + architecture = ptr.To("analog") + ) + + BeforeEach(func() { + ctrl = gomock.NewController(GinkgoT()) + + scheme := runtime.NewScheme() + Expect(apisazure.AddToScheme(scheme)).To(Succeed()) + Expect(apisazurev1alpha1.AddToScheme(scheme)).To(Succeed()) + Expect(gardencorev1beta1.AddToScheme(scheme)).To(Succeed()) + + c = mockclient.NewMockClient(ctrl) + mgr = mockmanager.NewMockManager(ctrl) + + mgr.EXPECT().GetScheme().Return(scheme).Times(3) + mgr.EXPECT().GetClient().Return(c) + + shootValidator = validator.NewShootValidator(mgr) + + cloudProfile = &gardencorev1beta1.CloudProfile{ + ObjectMeta: metav1.ObjectMeta{ + Name: "azure", + }, + Spec: gardencorev1beta1.CloudProfileSpec{ + Regions: []gardencorev1beta1.Region{ + { + Name: regionName, + Zones: []gardencorev1beta1.AvailabilityZone{ + { + Name: "1", + }, + { + Name: "2", + }, + }, + }, + }, + ProviderConfig: &runtime.RawExtension{ + Raw: encode(&apisazurev1alpha1.CloudProfileConfig{ + TypeMeta: metav1.TypeMeta{ + APIVersion: apisazurev1alpha1.SchemeGroupVersion.String(), + Kind: "CloudProfileConfig", + }, + MachineImages: []apisazurev1alpha1.MachineImages{ + { + Name: imageName, + Versions: []apisazurev1alpha1.MachineImageVersion{ + { + Version: imageVersion, + }, + }, + }, + }, + }), + }, + }, + } + + namespacedCloudProfile = &gardencorev1beta1.NamespacedCloudProfile{ + ObjectMeta: metav1.ObjectMeta{ + Name: "azure-nscpfl", + }, + Spec: gardencorev1beta1.NamespacedCloudProfileSpec{ + Parent: gardencorev1beta1.CloudProfileReference{ + Kind: "CloudProfile", + Name: "azure", + }, + }, + Status: gardencorev1beta1.NamespacedCloudProfileStatus{ + CloudProfileSpec: cloudProfile.Spec, + }, + } + + shoot = &core.Shoot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: namespace, + }, + Spec: core.ShootSpec{ + CloudProfile: &core.CloudProfileReference{Name: "azure"}, + SeedName: ptr.To("azure"), + Provider: core.Provider{ + Type: azure.Type, + InfrastructureConfig: &runtime.RawExtension{ + Raw: encode(&apisazurev1alpha1.InfrastructureConfig{ + TypeMeta: metav1.TypeMeta{ + APIVersion: apisazurev1alpha1.SchemeGroupVersion.String(), + Kind: "InfrastructureConfig", + }, + Networks: apisazurev1alpha1.NetworkConfig{ + Workers: ptr.To("10.250.0.0/16"), + }, + Zoned: true, + }), + }, + Workers: []core.Worker{ + { + Name: "worker-1", + Volume: &core.Volume{ + VolumeSize: "50Gi", + Type: ptr.To("Volume"), + }, + Zones: []string{"1"}, + Machine: core.Machine{ + Image: &core.ShootMachineImage{ + Name: imageName, + Version: imageVersion, + }, + Architecture: architecture, + }, + }, + }, + }, + Region: regionName, + Networking: &core.Networking{ + Nodes: ptr.To("10.250.0.0/16"), + Type: ptr.To("cilium"), + }, + }, + } + }) + + Context("Shoot creation (old is nil)", func() { + It("should return err when new is not a Shoot", func() { + err := shootValidator.Validate(ctx, &corev1.Pod{}, nil) + Expect(err).To(MatchError("wrong object type *v1.Pod")) + }) + + It("should return err when infrastructureConfig is nil", func() { + c.EXPECT().Get(ctx, cloudProfileKey, &gardencorev1beta1.CloudProfile{}).SetArg(2, *cloudProfile) + shoot.Spec.Provider.InfrastructureConfig = nil + + err := shootValidator.Validate(ctx, shoot, nil) + Expect(err).To(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.provider.infrastructureConfig"), + }))) + }) + + It("should return err when infrastructureConfig fails to be decoded", func() { + c.EXPECT().Get(ctx, cloudProfileKey, &gardencorev1beta1.CloudProfile{}).SetArg(2, *cloudProfile) + shoot.Spec.Provider.InfrastructureConfig = &runtime.RawExtension{Raw: []byte("foo")} + + err := shootValidator.Validate(ctx, shoot, nil) + Expect(err).To(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeForbidden), + "Field": Equal("spec.provider.infrastructureConfig"), + }))) + }) + + It("should return err when worker is invalid", func() { + c.EXPECT().Get(ctx, cloudProfileKey, &gardencorev1beta1.CloudProfile{}).SetArg(2, *cloudProfile) + + shoot.Spec.Provider.Workers = []core.Worker{ + { + Name: "worker-1", + Volume: nil, + Zones: nil, + }, + } + + err := shootValidator.Validate(ctx, shoot, nil) + Expect(err).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.provider.workers[0].volume"), + })), PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeRequired), + "Field": Equal("spec.provider.workers[0].zones"), + })))) + }) + + It("should succeed for valid Shoot", func() { + c.EXPECT().Get(ctx, cloudProfileKey, &gardencorev1beta1.CloudProfile{}).SetArg(2, *cloudProfile) + + err := shootValidator.Validate(ctx, shoot, nil) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should also work for cloudProfileName instead of CloudProfile reference in Shoot", func() { + shoot.Spec.CloudProfileName = ptr.To("azure") + shoot.Spec.CloudProfile = nil + c.EXPECT().Get(ctx, cloudProfileKey, &gardencorev1beta1.CloudProfile{}).SetArg(2, *cloudProfile) + + err := shootValidator.Validate(ctx, shoot, nil) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should also work for NamespacedCloudProfile referenced from Shoot", func() { + shoot.Spec.CloudProfile = &core.CloudProfileReference{ + Kind: "NamespacedCloudProfile", + Name: "azure-nscpfl", + } + c.EXPECT().Get(ctx, namespacedCloudProfileKey, &gardencorev1beta1.NamespacedCloudProfile{}).SetArg(2, *namespacedCloudProfile) + + err := shootValidator.Validate(ctx, shoot, nil) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Context("Workerless Shoot", func() { + BeforeEach(func() { + shoot.Spec.Provider.Workers = nil + }) + + It("should not validate", func() { + err := shootValidator.Validate(ctx, shoot, nil) + Expect(err).NotTo(HaveOccurred()) + }) + }) + }) +}) + +func encode(obj runtime.Object) []byte { + data, _ := json.Marshal(obj) + return data +} diff --git a/pkg/apis/azure/helper/scheme.go b/pkg/apis/azure/helper/scheme.go index d6af9bd50..73da4144b 100644 --- a/pkg/apis/azure/helper/scheme.go +++ b/pkg/apis/azure/helper/scheme.go @@ -74,9 +74,13 @@ func InfrastructureStatusFromRaw(raw *runtime.RawExtension) (*api.Infrastructure func CloudProfileConfigFromCluster(cluster *controller.Cluster) (*api.CloudProfileConfig, error) { var cloudProfileConfig *api.CloudProfileConfig if cluster != nil && cluster.CloudProfile != nil && cluster.CloudProfile.Spec.ProviderConfig != nil && cluster.CloudProfile.Spec.ProviderConfig.Raw != nil { + cloudProfileSpecifier := fmt.Sprintf("cloudProfile '%q'", k8sclient.ObjectKeyFromObject(cluster.CloudProfile)) + if cluster.Shoot != nil && cluster.Shoot.Spec.CloudProfile != nil { + cloudProfileSpecifier = fmt.Sprintf("%s '%s/%s'", cluster.Shoot.Spec.CloudProfile.Kind, cluster.Shoot.Namespace, cluster.Shoot.Spec.CloudProfile.Name) + } cloudProfileConfig = &api.CloudProfileConfig{} if _, _, err := decoder.Decode(cluster.CloudProfile.Spec.ProviderConfig.Raw, nil, cloudProfileConfig); err != nil { - return nil, fmt.Errorf("could not decode providerConfig of cloudProfile for '%s': %w", k8sclient.ObjectKeyFromObject(cluster.CloudProfile), err) + return nil, fmt.Errorf("could not decode providerConfig of %s: %w", cloudProfileSpecifier, err) } } return cloudProfileConfig, nil diff --git a/pkg/apis/azure/validation/infrastructure.go b/pkg/apis/azure/validation/infrastructure.go index 5675dead9..29ccc4a61 100644 --- a/pkg/apis/azure/validation/infrastructure.go +++ b/pkg/apis/azure/validation/infrastructure.go @@ -25,14 +25,14 @@ const ( ) // ValidateInfrastructureConfigAgainstCloudProfile validates the InfrastructureConfig against the CloudProfile. -func ValidateInfrastructureConfigAgainstCloudProfile(oldInfra, infra *apisazure.InfrastructureConfig, shootRegion string, cloudProfile *gardencorev1beta1.CloudProfile, fld *field.Path) field.ErrorList { +func ValidateInfrastructureConfigAgainstCloudProfile(oldInfra, infra *apisazure.InfrastructureConfig, shootRegion string, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec, fld *field.Path) field.ErrorList { allErrs := field.ErrorList{} if helper.IsUsingSingleSubnetLayout(infra) { return allErrs } - for _, region := range cloudProfile.Spec.Regions { + for _, region := range cloudProfileSpec.Regions { if region.Name == shootRegion { allErrs = append(allErrs, validateInfrastructureConfigZones(oldInfra, infra, region.Zones, fld.Child("networks").Child("zones"))...) break diff --git a/pkg/apis/azure/validation/infrastructure_test.go b/pkg/apis/azure/validation/infrastructure_test.go index 7ad16b96c..66f7663c8 100644 --- a/pkg/apis/azure/validation/infrastructure_test.go +++ b/pkg/apis/azure/validation/infrastructure_test.go @@ -659,7 +659,7 @@ var _ = Describe("InfrastructureConfig validation", func() { It("should deny zones not present in cloudprofile", func() { infrastructureConfig.Networks.Zones[0].Name = 5 - errorList := ValidateInfrastructureConfigAgainstCloudProfile(nil, infrastructureConfig, region, cp, providerPath) + errorList := ValidateInfrastructureConfigAgainstCloudProfile(nil, infrastructureConfig, region, &cp.Spec, providerPath) Expect(errorList).NotTo(BeEmpty()) Expect(errorList).To(HaveLen(1)) Expect(errorList).To(ConsistOfFields(Fields{ @@ -669,7 +669,7 @@ var _ = Describe("InfrastructureConfig validation", func() { }) It("should allow zones removed from cloudprofile", func() { cp.Spec.Regions[0].Zones = cp.Spec.Regions[0].Zones[1:] - errorList := ValidateInfrastructureConfigAgainstCloudProfile(infrastructureConfig, infrastructureConfig, region, cp, providerPath) + errorList := ValidateInfrastructureConfigAgainstCloudProfile(infrastructureConfig, infrastructureConfig, region, &cp.Spec, providerPath) Expect(errorList).To(BeEmpty()) }) })