From 7ebf72a94879e5d32d3b6b22c89c7358a2e9c64a Mon Sep 17 00:00:00 2001 From: Luca Bernstein Date: Mon, 22 Jul 2024 15:24:56 +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 | 6 +- pkg/admission/validator/shoot.go | 46 +++++++------- pkg/admission/validator/shoot_test.go | 60 +++++++++++++++---- pkg/apis/aws/helper/scheme.go | 6 +- pkg/apis/aws/validation/infrastructure.go | 4 +- .../aws/validation/infrastructure_test.go | 12 ++-- 7 files changed, 94 insertions(+), 41 deletions(-) diff --git a/charts/gardener-extension-admission-aws/charts/application/templates/rbac.yaml b/charts/gardener-extension-admission-aws/charts/application/templates/rbac.yaml index fb74c7711..a76285f47 100644 --- a/charts/gardener-extension-admission-aws/charts/application/templates/rbac.yaml +++ b/charts/gardener-extension-admission-aws/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 baf4ca966..78a955a66 100644 --- a/docs/usage/usage.md +++ b/docs/usage/usage.md @@ -468,7 +468,8 @@ metadata: name: johndoe-aws namespace: garden-dev spec: - cloudProfileName: aws + cloudProfile: + name: aws region: eu-central-1 secretBindingName: core-aws provider: @@ -531,7 +532,8 @@ metadata: name: johndoe-aws namespace: garden-dev spec: - cloudProfileName: aws + cloudProfile: + name: aws region: eu-central-1 secretBindingName: core-aws provider: diff --git a/pkg/admission/validator/shoot.go b/pkg/admission/validator/shoot.go index 1eb551da7..434b43e92 100644 --- a/pkg/admission/validator/shoot.go +++ b/pkg/admission/validator/shoot.go @@ -13,6 +13,7 @@ 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" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/validation/field" @@ -27,7 +28,6 @@ import ( 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(), } @@ -37,7 +37,6 @@ type shoot struct { client client.Client decoder runtime.Decoder lenientDecoder runtime.Decoder - scheme *runtime.Scheme } // Validate validates the given shoot object. @@ -52,15 +51,28 @@ func (s *shoot) Validate(ctx context.Context, new, old client.Object) error { return nil } + shootV1Beta1 := &gardencorev1beta1.Shoot{} + err := gardencorev1beta1.Convert_core_Shoot_To_v1beta1_Shoot(shoot, shootV1Beta1, nil) + 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 old != nil { oldShoot, ok := old.(*core.Shoot) if !ok { return fmt.Errorf("wrong object type %T for old object", old) } - return s.validateShootUpdate(ctx, oldShoot, shoot) + return s.validateShootUpdate(ctx, oldShoot, shoot, &cloudProfile.Spec) } - return s.validateShootCreation(ctx, shoot) + return s.validateShootCreation(ctx, shoot, &cloudProfile.Spec) } func (s *shoot) validateShoot(_ context.Context, shoot *core.Shoot) error { @@ -121,7 +133,7 @@ func (s *shoot) validateShoot(_ context.Context, shoot *core.Shoot) error { return nil } -func (s *shoot) validateShootUpdate(ctx context.Context, oldShoot, shoot *core.Shoot) error { +func (s *shoot) validateShootUpdate(ctx context.Context, oldShoot, shoot *core.Shoot, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec) error { var ( fldPath = field.NewPath("spec", "provider") infraConfigFldPath = fldPath.Child("infrastructureConfig") @@ -136,7 +148,7 @@ func (s *shoot) validateShootUpdate(ctx context.Context, oldShoot, shoot *core.S return err } - awsCloudProfile, infraConfig, err := s.baseShootValidation(ctx, shoot, oldInfraConfig, fldPath) + awsCloudProfile, infraConfig, err := s.baseShootValidation(ctx, shoot, cloudProfileSpec, oldInfraConfig, fldPath) if err != nil { return err } @@ -158,12 +170,12 @@ func (s *shoot) validateShootUpdate(ctx context.Context, oldShoot, shoot *core.S return s.validateShoot(ctx, shoot) } -func (s *shoot) validateShootCreation(ctx context.Context, shoot *core.Shoot) error { +func (s *shoot) validateShootCreation(ctx context.Context, shoot *core.Shoot, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec) error { var ( fldPath = field.NewPath("spec", "provider") ) - awsCloudProfile, _, err := s.baseShootValidation(ctx, shoot, nil, fldPath) + awsCloudProfile, _, err := s.baseShootValidation(ctx, shoot, cloudProfileSpec, nil, fldPath) if err != nil { return err } @@ -175,9 +187,8 @@ func (s *shoot) validateShootCreation(ctx context.Context, shoot *core.Shoot) er return s.validateShoot(ctx, shoot) } -func (s *shoot) baseShootValidation(ctx context.Context, shoot *core.Shoot, oldInfraConfig *api.InfrastructureConfig, fldPath *field.Path) (*api.CloudProfileConfig, *api.InfrastructureConfig, error) { +func (s *shoot) baseShootValidation(ctx context.Context, shoot *core.Shoot, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec, oldInfraConfig *api.InfrastructureConfig, fldPath *field.Path) (*api.CloudProfileConfig, *api.InfrastructureConfig, error) { var ( - commonCloudProfile = &gardencorev1beta1.CloudProfile{} infraConfigFldPath = fldPath.Child("infrastructureConfig") ) @@ -190,28 +201,23 @@ func (s *shoot) baseShootValidation(ctx context.Context, shoot *core.Shoot, oldI return nil, nil, err } - if shoot.Spec.CloudProfile == nil { + if cloudProfileSpec == nil { return nil, nil, fmt.Errorf("shoot.spec.cloudprofile must not be nil ") } - - if err := s.client.Get(ctx, client.ObjectKey{Name: shoot.Spec.CloudProfile.Name}, commonCloudProfile); err != nil { - return nil, nil, err - } - - awsCloudProfile, err := decodeCloudProfileConfig(s.decoder, commonCloudProfile.Spec.ProviderConfig) + awsCloudProfile, err := decodeCloudProfileConfig(s.decoder, cloudProfileSpec.ProviderConfig) if err != nil { return nil, nil, err } - if err = s.validateAgainstCloudProfile(ctx, shoot, oldInfraConfig, infraConfig, commonCloudProfile, infraConfigFldPath); err != nil { + if err = s.validateAgainstCloudProfile(ctx, shoot, oldInfraConfig, infraConfig, cloudProfileSpec, infraConfigFldPath); err != nil { return nil, nil, err } return awsCloudProfile, infraConfig, nil } -func (s *shoot) validateAgainstCloudProfile(_ context.Context, shoot *core.Shoot, oldInfraConfig, infraConfig *api.InfrastructureConfig, cloudProfile *gardencorev1beta1.CloudProfile, fldPath *field.Path) error { - if errList := awsvalidation.ValidateInfrastructureConfigAgainstCloudProfile(oldInfraConfig, infraConfig, shoot, cloudProfile, fldPath); len(errList) != 0 { +func (s *shoot) validateAgainstCloudProfile(_ context.Context, shoot *core.Shoot, oldInfraConfig, infraConfig *api.InfrastructureConfig, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec, fldPath *field.Path) error { + if errList := awsvalidation.ValidateInfrastructureConfigAgainstCloudProfile(oldInfraConfig, infraConfig, shoot, cloudProfileSpec, fldPath); len(errList) != 0 { return errList.ToAggregate() } diff --git a/pkg/admission/validator/shoot_test.go b/pkg/admission/validator/shoot_test.go index 6950b964e..947ece40a 100644 --- a/pkg/admission/validator/shoot_test.go +++ b/pkg/admission/validator/shoot_test.go @@ -36,15 +36,17 @@ var _ = Describe("Shoot validator", func() { var ( shootValidator extensionswebhook.Validator - ctrl *gomock.Controller - mgr *mockmanager.MockManager - c *mockclient.MockClient - cloudProfile *gardencorev1beta1.CloudProfile - shoot *core.Shoot - - ctx = context.TODO() - cloudProfileKey = client.ObjectKey{Name: "aws"} - gp2type = string(apisaws.VolumeTypeGP2) + ctrl *gomock.Controller + mgr *mockmanager.MockManager + c *mockclient.MockClient + cloudProfile *gardencorev1beta1.CloudProfile + namespacedCloudProfile *gardencorev1beta1.NamespacedCloudProfile + shoot *core.Shoot + + ctx = context.Background() + cloudProfileKey = client.ObjectKey{Name: "aws"} + namespacedCloudProfileKey = client.ObjectKey{Name: "aws-nscpfl", Namespace: namespace} + gp2type = string(apisaws.VolumeTypeGP2) regionName = "us-west" imageName = "Foo" @@ -63,7 +65,7 @@ var _ = Describe("Shoot validator", func() { c = mockclient.NewMockClient(ctrl) mgr = mockmanager.NewMockManager(ctrl) - mgr.EXPECT().GetScheme().Return(scheme).Times(3) + mgr.EXPECT().GetScheme().Return(scheme).Times(2) mgr.EXPECT().GetClient().Return(c) shootValidator = validator.NewShootValidator(mgr) @@ -114,6 +116,21 @@ var _ = Describe("Shoot validator", func() { }, } + namespacedCloudProfile = &gardencorev1beta1.NamespacedCloudProfile{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aws-nscpfl", + }, + Spec: gardencorev1beta1.NamespacedCloudProfileSpec{ + Parent: gardencorev1beta1.CloudProfileReference{ + Kind: "CloudProfile", + Name: "aws", + }, + }, + Status: gardencorev1beta1.NamespacedCloudProfileStatus{ + CloudProfileSpec: cloudProfile.Spec, + }, + } + shoot = &core.Shoot{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", @@ -121,6 +138,7 @@ var _ = Describe("Shoot validator", func() { }, Spec: core.ShootSpec{ CloudProfile: &core.CloudProfileReference{ + Kind: "CloudProfile", Name: cloudProfile.Name, }, Provider: core.Provider{ @@ -178,6 +196,7 @@ var _ = Describe("Shoot validator", func() { }) 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) @@ -188,6 +207,7 @@ var _ = Describe("Shoot validator", func() { }) 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) @@ -366,6 +386,26 @@ var _ = Describe("Shoot validator", func() { 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("aws") + 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: "aws-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() { diff --git a/pkg/apis/aws/helper/scheme.go b/pkg/apis/aws/helper/scheme.go index 955362581..e8f737ccc 100644 --- a/pkg/apis/aws/helper/scheme.go +++ b/pkg/apis/aws/helper/scheme.go @@ -36,9 +36,13 @@ func init() { 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/aws/validation/infrastructure.go b/pkg/apis/aws/validation/infrastructure.go index ce8d27988..fd84049c1 100644 --- a/pkg/apis/aws/validation/infrastructure.go +++ b/pkg/apis/aws/validation/infrastructure.go @@ -23,11 +23,11 @@ import ( var gatewayEndpointPattern = regexp.MustCompile(`^\w+(\.\w+)*$`) // ValidateInfrastructureConfigAgainstCloudProfile validates the given `InfrastructureConfig` against the given `CloudProfile`. -func ValidateInfrastructureConfigAgainstCloudProfile(oldInfra, infra *apisaws.InfrastructureConfig, shoot *core.Shoot, cloudProfile *gardencorev1beta1.CloudProfile, fldPath *field.Path) field.ErrorList { +func ValidateInfrastructureConfigAgainstCloudProfile(oldInfra, infra *apisaws.InfrastructureConfig, shoot *core.Shoot, cloudProfileSpec *gardencorev1beta1.CloudProfileSpec, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} shootRegion := shoot.Spec.Region - for _, region := range cloudProfile.Spec.Regions { + for _, region := range cloudProfileSpec.Regions { if region.Name == shootRegion { allErrs = append(allErrs, validateInfrastructureConfigZones(oldInfra, infra, region.Zones, fldPath.Child("network"))...) break diff --git a/pkg/apis/aws/validation/infrastructure_test.go b/pkg/apis/aws/validation/infrastructure_test.go index bd3a39686..f6bc31747 100644 --- a/pkg/apis/aws/validation/infrastructure_test.go +++ b/pkg/apis/aws/validation/infrastructure_test.go @@ -101,14 +101,14 @@ var _ = Describe("InfrastructureConfig validation", func() { }) It("should pass because zone is configured in CloudProfile", func() { - errorList := ValidateInfrastructureConfigAgainstCloudProfile(nil, infrastructureConfig, shoot, cloudProfile, &field.Path{}) + errorList := ValidateInfrastructureConfigAgainstCloudProfile(nil, infrastructureConfig, shoot, &cloudProfile.Spec, &field.Path{}) Expect(errorList).To(BeEmpty()) }) It("should forbid because zone is not specified in CloudProfile", func() { infrastructureConfig.Networks.Zones[0].Name = "not-available" - errorList := ValidateInfrastructureConfigAgainstCloudProfile(nil, infrastructureConfig, shoot, cloudProfile, field.NewPath("spec")) + errorList := ValidateInfrastructureConfigAgainstCloudProfile(nil, infrastructureConfig, shoot, &cloudProfile.Spec, field.NewPath("spec")) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeNotSupported), @@ -118,7 +118,7 @@ var _ = Describe("InfrastructureConfig validation", func() { It("should forbid because zone is duplicate", func() { infrastructureConfig.Networks.Zones = append(infrastructureConfig.Networks.Zones, infrastructureConfig.Networks.Zones[0]) - errorList := ValidateInfrastructureConfigAgainstCloudProfile(nil, infrastructureConfig, shoot, cloudProfile, field.NewPath("spec")) + errorList := ValidateInfrastructureConfigAgainstCloudProfile(nil, infrastructureConfig, shoot, &cloudProfile.Spec, field.NewPath("spec")) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeDuplicate), @@ -129,7 +129,7 @@ var _ = Describe("InfrastructureConfig validation", func() { It("should forbid zone update because zone is duplicate", func() { oldInfra := infrastructureConfig.DeepCopy() infrastructureConfig.Networks.Zones = append(infrastructureConfig.Networks.Zones, infrastructureConfig.Networks.Zones[0]) - errorList := ValidateInfrastructureConfigAgainstCloudProfile(oldInfra, infrastructureConfig, shoot, cloudProfile, field.NewPath("spec")) + errorList := ValidateInfrastructureConfigAgainstCloudProfile(oldInfra, infrastructureConfig, shoot, &cloudProfile.Spec, field.NewPath("spec")) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeDuplicate), @@ -141,7 +141,7 @@ var _ = Describe("InfrastructureConfig validation", func() { infrastructureConfig.Networks.Zones[0].Name = "not-available" oldInfrastructureConfig := infrastructureConfig.DeepCopy() - errorList := ValidateInfrastructureConfigAgainstCloudProfile(oldInfrastructureConfig, infrastructureConfig, shoot, cloudProfile, field.NewPath("spec")) + errorList := ValidateInfrastructureConfigAgainstCloudProfile(oldInfrastructureConfig, infrastructureConfig, shoot, &cloudProfile.Spec, field.NewPath("spec")) Expect(errorList).To(BeEmpty()) }) @@ -150,7 +150,7 @@ var _ = Describe("InfrastructureConfig validation", func() { oldInfrastructureConfig := infrastructureConfig.DeepCopy() infrastructureConfig.Networks.Zones[0].Name = "not-available" - errorList := ValidateInfrastructureConfigAgainstCloudProfile(oldInfrastructureConfig, infrastructureConfig, shoot, cloudProfile, field.NewPath("spec")) + errorList := ValidateInfrastructureConfigAgainstCloudProfile(oldInfrastructureConfig, infrastructureConfig, shoot, &cloudProfile.Spec, field.NewPath("spec")) Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeNotSupported),