diff --git a/pkg/admission/validator/shoot.go b/pkg/admission/validator/shoot.go new file mode 100644 index 00000000..922a2430 --- /dev/null +++ b/pkg/admission/validator/shoot.go @@ -0,0 +1,68 @@ +// SPDX-FileCopyrightText: SAP SE or an SAP affiliate company and Gardener contributors +// +// SPDX-License-Identifier: Apache-2.0 + +package validator + +import ( + "context" + "fmt" + + extensionswebhook "github.com/gardener/gardener/extensions/pkg/webhook" + "github.com/gardener/gardener/pkg/apis/core" + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/manager" + + "github.com/stackitcloud/gardener-extension-provider-stackit/v2/pkg/apis/stackit/helper" + stackitvalidation "github.com/stackitcloud/gardener-extension-provider-stackit/v2/pkg/apis/stackit/validation" +) + +// NewShootValidator returns a new instance of a shoot validator. +func NewShootValidator(mgr manager.Manager) extensionswebhook.Validator { + return &shoot{} +} + +type shoot struct{} + +// Validate validates the given shoot objects. +func (s *shoot) Validate(_ context.Context, newObj, oldObj client.Object) error { + shoot, ok := newObj.(*core.Shoot) + if !ok { + return fmt.Errorf("wrong object type %T", newObj) + } + + if shoot.Spec.Provider.ControlPlaneConfig == nil { + return nil + } + + cpConfig, err := helper.ControlPlaneConfigFromRawExtension(shoot.Spec.Provider.ControlPlaneConfig) + if err != nil { + return err + } + + infraConfig, err := helper.InfrastructureConfigFromRawExtension(shoot.Spec.Provider.InfrastructureConfig) + if err != nil { + return err + } + + allErrs := field.ErrorList{} + + allErrs = append(allErrs, stackitvalidation.ValidateControlPlaneConfig(cpConfig, shoot.Spec.Kubernetes.Version, field.NewPath("spec").Child("providerConfig"))...) + + allErrs = append(allErrs, stackitvalidation.ValidateInfrastructureConfig(infraConfig, shoot.Spec.Networking.Nodes, field.NewPath("spec").Child("providerConfig").Child("infrastructureConfig"))...) + + if oldObj != nil { + oldShoot, ok := oldObj.(*core.Shoot) + if !ok { + return fmt.Errorf("wrong object type %T for old object", oldObj) + } + oldInfraConfig, err := helper.InfrastructureConfigFromRawExtension(oldShoot.Spec.Provider.InfrastructureConfig) + if err != nil { + return err + } + allErrs = append(allErrs, stackitvalidation.ValidateInfrastructureConfigUpdate(oldInfraConfig, infraConfig, field.NewPath("spec").Child("providerConfig").Child("infrastructureConfig"))...) + } + + return allErrs.ToAggregate() +} diff --git a/pkg/admission/validator/webhook.go b/pkg/admission/validator/webhook.go index 36a00aed..0a0321f5 100644 --- a/pkg/admission/validator/webhook.go +++ b/pkg/admission/validator/webhook.go @@ -26,6 +26,7 @@ func New(mgr manager.Manager) (*extensionswebhook.Webhook, error) { Path: "/webhooks/validate", Validators: map[extensionswebhook.Validator][]extensionswebhook.Type{ NewCloudProfileValidator(mgr): {{Obj: &core.CloudProfile{}}}, + NewShootValidator(mgr): {{Obj: &core.Shoot{}}}, NewNamespacedCloudProfileValidator(mgr): {{Obj: &core.NamespacedCloudProfile{}}}, }, Target: extensionswebhook.TargetSeed, diff --git a/pkg/apis/stackit/helper/scheme.go b/pkg/apis/stackit/helper/scheme.go index 49966b6f..9f092213 100644 --- a/pkg/apis/stackit/helper/scheme.go +++ b/pkg/apis/stackit/helper/scheme.go @@ -86,6 +86,16 @@ func CloudProfileConfigFromCluster(cluster *controller.Cluster) (*stackitv1alpha return cloudProfileConfig, nil } +func ControlPlaneConfigFromRawExtension(raw *runtime.RawExtension) (*stackitv1alpha1.ControlPlaneConfig, error) { + cpConfig := &stackitv1alpha1.ControlPlaneConfig{} + setGVK(cpConfig) + + if err := decode(raw, cpConfig); err != nil { + return nil, err + } + return cpConfig, nil +} + func CloudProfileConfigFromRawExtension(raw *runtime.RawExtension) (*stackitv1alpha1.CloudProfileConfig, error) { cpConfig := &stackitv1alpha1.CloudProfileConfig{} setGVK(cpConfig) diff --git a/pkg/apis/stackit/validation/controlplane.go b/pkg/apis/stackit/validation/controlplane.go index 3237f9c0..66e6d391 100644 --- a/pkg/apis/stackit/validation/controlplane.go +++ b/pkg/apis/stackit/validation/controlplane.go @@ -21,13 +21,10 @@ var ( ) // ValidateControlPlaneConfig validates a ControlPlaneConfig object. -func ValidateControlPlaneConfig(controlPlaneConfig *stackitv1alpha1.ControlPlaneConfig, infraConfig *stackitv1alpha1.InfrastructureConfig, version string, fldPath *field.Path) field.ErrorList { - allErrs := field.ErrorList{} +func ValidateControlPlaneConfig(controlPlaneConfig *stackitv1alpha1.ControlPlaneConfig, version string, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} // nolint:prealloc // size is not known yet - if controlPlaneConfig.CloudControllerManager != nil { - allErrs = append(allErrs, featurevalidation.ValidateFeatureGates(controlPlaneConfig.CloudControllerManager.FeatureGates, version, fldPath.Child("cloudControllerManager", "featureGates"))...) - allErrs = append(allErrs, validateCloudController(controlPlaneConfig.CloudControllerManager, fldPath.Child("cloudControllerManager"))...) - } + allErrs = append(allErrs, validateCloudController(controlPlaneConfig.CloudControllerManager, version, fldPath.Child("cloudControllerManager"))...) allErrs = append(allErrs, validateStorage(controlPlaneConfig.Storage, fldPath.Child("storage"))...) @@ -48,7 +45,7 @@ func ValidateControlPlaneConfigAgainstCloudProfile(oldCpConfig, cpConfig *stacki return allErrs } -func validateCloudController(cloudcontroller *stackitv1alpha1.CloudControllerManagerConfig, fldPath *field.Path) field.ErrorList { +func validateCloudController(cloudcontroller *stackitv1alpha1.CloudControllerManagerConfig, version string, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList if cloudcontroller == nil { return allErrs @@ -56,6 +53,8 @@ func validateCloudController(cloudcontroller *stackitv1alpha1.CloudControllerMan if cloudcontroller.Name != "" && !slices.Contains(validControllers, stackitv1alpha1.ControllerName(cloudcontroller.Name)) { allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), cloudcontroller.Name, "not supported ccm driver")) } + allErrs = append(allErrs, featurevalidation.ValidateFeatureGates(cloudcontroller.FeatureGates, version, fldPath.Child("featureGates"))...) + return allErrs } diff --git a/pkg/apis/stackit/validation/controlplane_test.go b/pkg/apis/stackit/validation/controlplane_test.go index 8ded76b1..e95a2ac9 100644 --- a/pkg/apis/stackit/validation/controlplane_test.go +++ b/pkg/apis/stackit/validation/controlplane_test.go @@ -18,17 +18,15 @@ var _ = Describe("ControlPlaneConfig validation", func() { var ( nilPath *field.Path controlPlane *stackitv1alpha1.ControlPlaneConfig - infraConfig *stackitv1alpha1.InfrastructureConfig ) BeforeEach(func() { controlPlane = &stackitv1alpha1.ControlPlaneConfig{} - infraConfig = &stackitv1alpha1.InfrastructureConfig{} }) Describe("#ValidateControlPlaneConfig", func() { It("should return no errors for a valid configuration", func() { - Expect(ValidateControlPlaneConfig(controlPlane, infraConfig, "", nilPath)).To(BeEmpty()) + Expect(ValidateControlPlaneConfig(controlPlane, "", nilPath)).To(BeEmpty()) }) It("should fail with invalid CCM feature gates", func() { @@ -39,7 +37,7 @@ var _ = Describe("ControlPlaneConfig validation", func() { }, } - errorList := ValidateControlPlaneConfig(controlPlane, infraConfig, "1.28.2", nilPath) + errorList := ValidateControlPlaneConfig(controlPlane, "1.28.2", nilPath) Expect(errorList).To(ConsistOf( PointTo(MatchFields(IgnoreExtras, Fields{ @@ -53,7 +51,7 @@ var _ = Describe("ControlPlaneConfig validation", func() { controlPlane.Storage = &stackitv1alpha1.Storage{ CSI: &stackitv1alpha1.CSI{Name: "stackit", CompatibilityMode: "bogus"}, } - Expect(ValidateControlPlaneConfig(controlPlane, infraConfig, "", nilPath)).To(ConsistOf( + Expect(ValidateControlPlaneConfig(controlPlane, "", nilPath)).To(ConsistOf( PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeInvalid), "Field": Equal("storage.csi.compatibilityMode"), diff --git a/pkg/apis/stackit/validation/infrastructure.go b/pkg/apis/stackit/validation/infrastructure.go index 581bf9b4..94b442a7 100644 --- a/pkg/apis/stackit/validation/infrastructure.go +++ b/pkg/apis/stackit/validation/infrastructure.go @@ -23,18 +23,9 @@ func ValidateInfrastructureConfig(infra *stackitv1alpha1.InfrastructureConfig, n allErrs = append(allErrs, field.Required(fldPath.Child("floatingPoolName"), "must provide the name of a floating pool")) } - networkingPath := field.NewPath("networking") - var nodes cidrvalidation.CIDR - if nodesCIDR != nil { - nodes = cidrvalidation.NewCIDR(*nodesCIDR, networkingPath.Child("nodes")) - } - networksPath := fldPath.Child("networks") - //nolint:staticcheck // SA1019: needed for migration purposes - if len(infra.Networks.Worker) == 0 && len(infra.Networks.Workers) == 0 { - allErrs = append(allErrs, field.Required(networksPath.Child("workers"), "must specify the network range for the worker network")) - } + // check InfrastructureConfig.networks.worker(s) is a valid cidr and not be set if a network id is provided. var workerCIDR cidrvalidation.CIDR //nolint:staticcheck // SA1019: needed for migration purposes if infra.Networks.Worker != "" { @@ -43,23 +34,38 @@ func ValidateInfrastructureConfig(infra *stackitv1alpha1.InfrastructureConfig, n allErrs = append(allErrs, cidrvalidation.ValidateCIDRParse(workerCIDR)...) //nolint:staticcheck // SA1019: needed for migration purposes allErrs = append(allErrs, cidrvalidation.ValidateCIDRIsCanonical(networksPath.Child("worker"), infra.Networks.Worker)...) + if infra.Networks.ID != nil { + allErrs = append(allErrs, field.Forbidden(networksPath.Child("worker"), "cant be set if a network id is provided")) + } } if infra.Networks.Workers != "" { workerCIDR = cidrvalidation.NewCIDR(infra.Networks.Workers, networksPath.Child("workers")) allErrs = append(allErrs, cidrvalidation.ValidateCIDRParse(workerCIDR)...) allErrs = append(allErrs, cidrvalidation.ValidateCIDRIsCanonical(networksPath.Child("workers"), infra.Networks.Workers)...) + if infra.Networks.ID != nil { + allErrs = append(allErrs, field.Forbidden(networksPath.Child("workers"), "cant be set if a network id is provided")) + } } - if nodes != nil { + // check if InfrastructureConfig.networks.worker(s) is a subset of spec.networking.nodes + var nodes cidrvalidation.CIDR + if nodesCIDR != nil { + nodes = cidrvalidation.NewCIDR(*nodesCIDR, field.NewPath("spec").Child("networking").Child("nodes")) allErrs = append(allErrs, nodes.ValidateSubset(workerCIDR)...) } + // validate that InfrastructureConfig.networks.id is a valid uuid if infra.Networks.ID != nil { if _, err := uuid.Parse(*infra.Networks.ID); err != nil { - allErrs = append(allErrs, field.Invalid(networksPath.Child("id"), infra.Networks.ID, "if network ID is provided it must be a valid OpenStack UUID")) + allErrs = append(allErrs, field.Invalid(networksPath.Child("id"), infra.Networks.ID, "if network ID is provided it must be a valid STACKIT Network ID")) } } + // eather InfrastructureConfig.networks.id or InfrastructureConfig.networks.worker(s) has to be set + if workerCIDR == nil && infra.Networks.ID == nil { + allErrs = append(allErrs, field.Required(networksPath.Child("workers"), "must specify the network range for the worker network or provide a network ID for the network")) + } + if infra.Networks.SubnetID != nil { if infra.Networks.ID == nil { allErrs = append(allErrs, field.Invalid(networksPath.Child("subnetId"), infra.Networks.SubnetID, "if subnet ID is provided a networkID must be provided")) diff --git a/pkg/apis/stackit/validation/infrastructure_test.go b/pkg/apis/stackit/validation/infrastructure_test.go index 7582ff3c..48be106f 100644 --- a/pkg/apis/stackit/validation/infrastructure_test.go +++ b/pkg/apis/stackit/validation/infrastructure_test.go @@ -32,9 +32,6 @@ var _ = Describe("InfrastructureConfig validation", func() { infrastructureConfig = &stackitv1alpha1.InfrastructureConfig{ FloatingPoolName: floatingPoolName1, Networks: stackitv1alpha1.Networks{ - Router: &stackitv1alpha1.Router{ - ID: "hugo", - }, Workers: "10.250.0.0/16", }, } @@ -88,6 +85,7 @@ var _ = Describe("InfrastructureConfig validation", func() { }) It("should forbid an invalid subnet id", func() { + infrastructureConfig.Networks.Workers = "" infrastructureConfig.Networks.ID = new(uuid.NewString()) infrastructureConfig.Networks.SubnetID = new("thisiswrong") @@ -101,6 +99,7 @@ var _ = Describe("InfrastructureConfig validation", func() { }) It("should allow an valid OpenStack UUID as subnet ID", func() { + infrastructureConfig.Networks.Workers = "" infrastructureConfig.Networks.ID = new(uuid.NewString()) infrastructureConfig.Networks.SubnetID = new(uuid.NewString()) @@ -111,7 +110,7 @@ var _ = Describe("InfrastructureConfig validation", func() { }) Context("CIDR", func() { - It("should forbid empty workers CIDR", func() { + It("should forbid empty workers CIDR and no network id", func() { infrastructureConfig.Networks.Workers = "" errorList := ValidateInfrastructureConfig(infrastructureConfig, &nodes, nilPath) @@ -119,7 +118,7 @@ var _ = Describe("InfrastructureConfig validation", func() { Expect(errorList).To(ConsistOfFields(Fields{ "Type": Equal(field.ErrorTypeRequired), "Field": Equal("networks.workers"), - "Detail": Equal("must specify the network range for the worker network"), + "Detail": Equal("must specify the network range for the worker network or provide a network ID for the network"), })) }) @@ -143,7 +142,7 @@ var _ = Describe("InfrastructureConfig validation", func() { Expect(errorList).To(ConsistOfFields(Fields{ "Type": Equal(field.ErrorTypeInvalid), "Field": Equal("networks.workers"), - "Detail": Equal(`must be a subset of "networking.nodes" ("10.250.0.0/16")`), + "Detail": Equal(`must be a subset of "spec.networking.nodes" ("10.250.0.0/16")`), })) }) @@ -163,6 +162,7 @@ var _ = Describe("InfrastructureConfig validation", func() { }) It("should forbid an invalid network id configuration", func() { + infrastructureConfig.Networks.Workers = "" invalidID := "thisiswrong" infrastructureConfig.Networks.ID = &invalidID @@ -174,7 +174,21 @@ var _ = Describe("InfrastructureConfig validation", func() { })) }) - It("should allow an valid OpenStack UUID as network ID", func() { + It("should forbid an valid UUID as network ID when workers is also set", func() { + id, err := uuid.NewUUID() + Expect(err).NotTo(HaveOccurred()) + infrastructureConfig.Networks.ID = new(id.String()) + + errorList := ValidateInfrastructureConfig(infrastructureConfig, &nodes, nilPath) + + Expect(errorList).To(ConsistOfFields(Fields{ + "Type": Equal(field.ErrorTypeForbidden), + "Field": Equal("networks.workers"), + })) + }) + + It("should allow empty workers CIDR and when valid network id is set", func() { + infrastructureConfig.Networks.Workers = "" id, err := uuid.NewUUID() Expect(err).NotTo(HaveOccurred()) infrastructureConfig.Networks.ID = new(id.String())