From 0ae3afb4f329443c12cd3ae8c12541784f5fc5e6 Mon Sep 17 00:00:00 2001 From: "Kistner, Dominic" Date: Thu, 15 Jul 2021 12:34:32 +0200 Subject: [PATCH 1/2] Improve LoadBalancerClass validation Allow to change the name and purpose of load balancer classes in `.controlPlaneConfig.loadBalancerClasses[]`. The load balancer classes configuration need still to be semantically equal with the load balancer classes from the CloudProfile. Add validation to check if there are no load balancer classes with same name and if there are multiple default or private loadbalancer classes. --- pkg/apis/openstack/cloudprofile_test.go | 86 +++++++++ pkg/apis/openstack/openstack_suite_test.go | 27 +++ pkg/apis/openstack/types_cloudprofile.go | 23 +++ pkg/apis/openstack/validation/cloudprofile.go | 55 +++++- .../openstack/validation/cloudprofile_test.go | 172 +++++++++++++++--- pkg/apis/openstack/validation/controlplane.go | 9 +- 6 files changed, 333 insertions(+), 39 deletions(-) create mode 100644 pkg/apis/openstack/cloudprofile_test.go create mode 100644 pkg/apis/openstack/openstack_suite_test.go diff --git a/pkg/apis/openstack/cloudprofile_test.go b/pkg/apis/openstack/cloudprofile_test.go new file mode 100644 index 000000000..1d61d57df --- /dev/null +++ b/pkg/apis/openstack/cloudprofile_test.go @@ -0,0 +1,86 @@ +// Copyright (c) 2021 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package openstack_test + +import ( + . "github.com/gardener/gardener-extension-provider-openstack/pkg/apis/openstack" + "k8s.io/utils/pointer" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("LoadBalancerClass", func() { + Context("#IsSemanticallyEqual", func() { + var ( + loadBalancerClassA LoadBalancerClass + loadBalancerClassB LoadBalancerClass + ) + + BeforeEach(func() { + loadBalancerClassA = LoadBalancerClass{ + Name: "lbclass-A", + FloatingNetworkID: pointer.StringPtr("floating-network-id"), + FloatingSubnetID: pointer.StringPtr("floating-subnet-id"), + FloatingSubnetName: pointer.StringPtr("floating-subnet-name"), + FloatingSubnetTags: pointer.StringPtr("floating-subnet-tags"), + SubnetID: pointer.StringPtr("subnet-id"), + } + loadBalancerClassB = LoadBalancerClass{ + Name: "lbclass-B", + FloatingNetworkID: pointer.StringPtr("floating-network-id"), + FloatingSubnetID: pointer.StringPtr("floating-subnet-id"), + FloatingSubnetName: pointer.StringPtr("floating-subnet-name"), + FloatingSubnetTags: pointer.StringPtr("floating-subnet-tags"), + SubnetID: pointer.StringPtr("subnet-id"), + } + }) + + It("should return true as LoadBalancerClass are semantically equal with different names", func() { + Expect(loadBalancerClassA.IsSemanticallyEqual(loadBalancerClassB)).To(BeTrue()) + }) + + It("should return true as LoadBalancerClass are semantically equal with different purposes", func() { + loadBalancerClassA.Purpose = pointer.StringPtr("purpose-a") + loadBalancerClassB.Purpose = pointer.StringPtr("purpose-b") + Expect(loadBalancerClassA.IsSemanticallyEqual(loadBalancerClassB)).To(BeTrue()) + }) + + It("should return false as LoadBalancerClass are not semantically due to different floating network ids", func() { + loadBalancerClassB.FloatingNetworkID = pointer.StringPtr("floating-network-id-2") + Expect(loadBalancerClassA.IsSemanticallyEqual(loadBalancerClassB)).To(BeFalse()) + }) + + It("should return false as LoadBalancerClass are not semantically due to different floating subnet ids", func() { + loadBalancerClassB.FloatingSubnetID = pointer.StringPtr("floating-subnet-id-2") + Expect(loadBalancerClassA.IsSemanticallyEqual(loadBalancerClassB)).To(BeFalse()) + }) + + It("should return false as LoadBalancerClass are not semantically due to different floating subnet names", func() { + loadBalancerClassB.FloatingSubnetName = pointer.StringPtr("floating-subnet-name-2") + Expect(loadBalancerClassA.IsSemanticallyEqual(loadBalancerClassB)).To(BeFalse()) + }) + + It("should return false as LoadBalancerClass are not semantically due to different floating subnet tags", func() { + loadBalancerClassB.FloatingSubnetTags = pointer.StringPtr("floating-subnet-tags-2") + Expect(loadBalancerClassA.IsSemanticallyEqual(loadBalancerClassB)).To(BeFalse()) + }) + + It("should return false as LoadBalancerClass are not semantically due to different subnet ids", func() { + loadBalancerClassB.SubnetID = pointer.StringPtr("subnet-ids-2") + Expect(loadBalancerClassA.IsSemanticallyEqual(loadBalancerClassB)).To(BeFalse()) + }) + }) +}) diff --git a/pkg/apis/openstack/openstack_suite_test.go b/pkg/apis/openstack/openstack_suite_test.go new file mode 100644 index 000000000..18743a265 --- /dev/null +++ b/pkg/apis/openstack/openstack_suite_test.go @@ -0,0 +1,27 @@ +// Copyright (c) 2021 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package openstack_test + +import ( + "testing" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +func TestV1alpha1(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "API Openstack Suite") +} diff --git a/pkg/apis/openstack/types_cloudprofile.go b/pkg/apis/openstack/types_cloudprofile.go index 1fa2c6b47..52a469196 100644 --- a/pkg/apis/openstack/types_cloudprofile.go +++ b/pkg/apis/openstack/types_cloudprofile.go @@ -17,6 +17,8 @@ package openstack import ( "fmt" + "github.com/gardener/gardener-extension-provider-openstack/pkg/utils" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -106,6 +108,27 @@ type LoadBalancerClass struct { SubnetID *string } +// IsSemanticallyEqual checks if the load balancer class is semantically equal to +// another given load balancer class. Name and Purpose fields are allowed to be different. +func (l LoadBalancerClass) IsSemanticallyEqual(e LoadBalancerClass) bool { + if !utils.StringEqual(l.FloatingNetworkID, e.FloatingNetworkID) { + return false + } + if !utils.StringEqual(l.FloatingSubnetID, e.FloatingSubnetID) { + return false + } + if !utils.StringEqual(l.FloatingSubnetName, e.FloatingSubnetName) { + return false + } + if !utils.StringEqual(l.FloatingSubnetTags, e.FloatingSubnetTags) { + return false + } + if !utils.StringEqual(l.SubnetID, e.SubnetID) { + return false + } + return true +} + func (in LoadBalancerClass) String() string { result := fmt.Sprintf("Name: %q", in.Name) if in.Purpose != nil { diff --git a/pkg/apis/openstack/validation/cloudprofile.go b/pkg/apis/openstack/validation/cloudprofile.go index 4b3e27f91..8ad259b3d 100644 --- a/pkg/apis/openstack/validation/cloudprofile.go +++ b/pkg/apis/openstack/validation/cloudprofile.go @@ -66,10 +66,7 @@ func ValidateCloudProfileConfig(cloudProfile *api.CloudProfileConfig, fldPath *f } for i, pool := range cloudProfile.Constraints.FloatingPools { - lbClassPath := floatingPoolPath.Index(i).Child("loadBalancerClasses") - for j, class := range pool.LoadBalancerClasses { - allErrs = append(allErrs, ValidateLoadBalancerClasses(class, lbClassPath.Index(j))...) - } + allErrs = append(allErrs, ValidateLoadBalancerClasses(pool.LoadBalancerClasses, floatingPoolPath.Index(i).Child("loadBalancerClasses"))...) } loadBalancerProviderPath := fldPath.Child("constraints", "loadBalancerProviders") @@ -169,12 +166,12 @@ func ValidateCloudProfileConfig(cloudProfile *api.CloudProfileConfig, fldPath *f return allErrs } -// ValidateLoadBalancerClasses validates LoadBalancerClass object. -func ValidateLoadBalancerClasses(lbClass api.LoadBalancerClass, fldPath *field.Path) field.ErrorList { +// validateLoadBalancerClass validates LoadBalancerClass object. +func validateLoadBalancerClass(lbClass api.LoadBalancerClass, fldPath *field.Path) field.ErrorList { var allErrs = field.ErrorList{} if lbClass.Purpose != nil && *lbClass.Purpose != api.DefaultLoadBalancerClass && *lbClass.Purpose != api.PrivateLoadBalancerClass && *lbClass.Purpose != api.VPNLoadBalancerClass { - allErrs = append(allErrs, field.Invalid(fldPath, *lbClass.Purpose, fmt.Sprintf("Invalid LoadBalancerClass purpose. Valid values are %q or %q", api.DefaultLoadBalancerClass, api.PrivateLoadBalancerClass))) + allErrs = append(allErrs, field.Invalid(fldPath, *lbClass.Purpose, fmt.Sprintf("invalid LoadBalancerClass purpose. Valid values are %q or %q", api.DefaultLoadBalancerClass, api.PrivateLoadBalancerClass))) } if lbClass.FloatingSubnetID != nil && lbClass.FloatingSubnetName != nil && lbClass.FloatingSubnetTags != nil { @@ -192,3 +189,47 @@ func ValidateLoadBalancerClasses(lbClass api.LoadBalancerClass, fldPath *field.P return allErrs } + +func ValidateLoadBalancerClasses(loadBalancerClasses []api.LoadBalancerClass, fldPath *field.Path) field.ErrorList { + var ( + defaultClassExists bool + privateClassExists bool + + allErrs = field.ErrorList{} + lbClassNames = sets.NewString() + ) + + for i, class := range loadBalancerClasses { + lbClassPath := fldPath.Index(i) + + // Validate first the load balancer class itself. + allErrs = append(allErrs, validateLoadBalancerClass(class, lbClassPath)...) + + // All load balancer classes need to have an unique name. Check for duplicates. + if lbClassNames.Has(class.Name) { + allErrs = append(allErrs, field.Duplicate(lbClassPath.Child("name"), class.Name)) + } else { + lbClassNames.Insert(class.Name) + } + + // There can only be one default load balancer class. Check for multiple default classes. + if (class.Purpose != nil && *class.Purpose == api.DefaultLoadBalancerClass) || class.Name == api.DefaultLoadBalancerClass { + if defaultClassExists { + allErrs = append(allErrs, field.Invalid(fldPath, loadBalancerClasses, "not allowed to configure multiple default load balancer classes")) + } else { + defaultClassExists = true + } + } + + // There can only be one private load balancer class. Check for multiple private classes. + if (class.Purpose != nil && *class.Purpose == api.PrivateLoadBalancerClass) || class.Name == api.PrivateLoadBalancerClass { + if privateClassExists { + allErrs = append(allErrs, field.Invalid(fldPath, loadBalancerClasses, "not allowed to configure multiple private load balancer classes")) + } else { + privateClassExists = true + } + } + } + + return allErrs +} diff --git a/pkg/apis/openstack/validation/cloudprofile_test.go b/pkg/apis/openstack/validation/cloudprofile_test.go index 5c552623a..d4bf97eb9 100644 --- a/pkg/apis/openstack/validation/cloudprofile_test.go +++ b/pkg/apis/openstack/validation/cloudprofile_test.go @@ -19,7 +19,6 @@ import ( . "github.com/gardener/gardener-extension-provider-openstack/pkg/apis/openstack/validation" . "github.com/onsi/ginkgo" - . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" "k8s.io/apimachinery/pkg/util/validation/field" @@ -352,31 +351,150 @@ var _ = Describe("CloudProfileConfig validation", func() { }) var _ = Describe("LoadBalancerClass validation", func() { - DescribeTable("LoadBalancerClass", - func(lbClass api.LoadBalancerClass, expectError bool) { - errorList := ValidateLoadBalancerClasses(lbClass, field.NewPath("loadBalancerClassTest")) - if !expectError { - Expect(errorList).To(BeEmpty()) - } - }, - Entry("pass as LoadBalancerClass class is valid", api.LoadBalancerClass{Name: "A"}, false), - Entry("fail as LoadBalancerClass has invalid purpose", api.LoadBalancerClass{Purpose: pointer.StringPtr("invalid")}, true), - Entry("fail as LoadBalancerClass specifies floating subnet by id, name and tags in parallel", api.LoadBalancerClass{ - FloatingSubnetID: pointer.StringPtr("floating-subnet-id"), - FloatingSubnetName: pointer.StringPtr("floating-subnet-name"), - FloatingSubnetTags: pointer.StringPtr("floating-subnet-tags"), - }, true), - Entry("fail as LoadBalancerClass specifies floating subnet by id and name", api.LoadBalancerClass{ - FloatingSubnetID: pointer.StringPtr("floating-subnet-id"), - FloatingSubnetName: pointer.StringPtr("floating-subnet-name"), - }, true), - Entry("fail as LoadBalancerClass specifies floating subnet by id and tags", api.LoadBalancerClass{ - FloatingSubnetID: pointer.StringPtr("floating-subnet-id"), - FloatingSubnetTags: pointer.StringPtr("floating-subnet-tags"), - }, true), - Entry("fail as LoadBalancerClass specifies floating subnet by name and tags", api.LoadBalancerClass{ - FloatingSubnetName: pointer.StringPtr("floating-subnet-id"), - FloatingSubnetTags: pointer.StringPtr("floating-subnet-tags"), - }, true), + var ( + loadBalancerClasses []api.LoadBalancerClass + fieldPath *field.Path ) + + BeforeEach(func() { + fieldPath = field.NewPath("loadBalancerClasses") + + loadBalancerClasses = []api.LoadBalancerClass{{ + Name: "test1", + }} + }) + + Context("LoadBalancerClass", func() { + It("should pass as LoadBalancerClass is valid", func() { + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(BeEmpty()) + }) + + It("should fail as LoadBalancerClass has an invalid purpose", func() { + loadBalancerClasses[0].Purpose = pointer.StringPtr("invalid") + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeInvalid), + "Field": Equal("loadBalancerClasses[0]"), + })))) + }) + + It("should fail as LoadBalancerClass specifies floating subnet by id, name and tags in parallel", func() { + loadBalancerClasses[0].FloatingSubnetID = pointer.StringPtr("floating-subnet-id") + loadBalancerClasses[0].FloatingSubnetName = pointer.StringPtr("floating-subnet-name") + loadBalancerClasses[0].FloatingSubnetTags = pointer.StringPtr("floating-subnet-tags") + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeForbidden), + "Field": Equal("loadBalancerClasses[0]"), + })))) + }) + + It("should fail as LoadBalancerClass specifies floating subnet by id and name", func() { + loadBalancerClasses[0].FloatingSubnetID = pointer.StringPtr("floating-subnet-id") + loadBalancerClasses[0].FloatingSubnetName = pointer.StringPtr("floating-subnet-name") + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeForbidden), + "Field": Equal("loadBalancerClasses[0]"), + })))) + }) + + It("should fail as LoadBalancerClass specifies floating subnet by id and tags", func() { + loadBalancerClasses[0].FloatingSubnetID = pointer.StringPtr("floating-subnet-id") + loadBalancerClasses[0].FloatingSubnetTags = pointer.StringPtr("floating-subnet-tags") + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeForbidden), + "Field": Equal("loadBalancerClasses[0]"), + })))) + }) + + It("should fail as LoadBalancerClass specifies floating subnet by name and tags", func() { + loadBalancerClasses[0].FloatingSubnetName = pointer.StringPtr("floating-subnet-name") + loadBalancerClasses[0].FloatingSubnetTags = pointer.StringPtr("floating-subnet-tags") + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeForbidden), + "Field": Equal("loadBalancerClasses[0]"), + })))) + }) + }) + + Context("LoadBalancerClassList", func() { + BeforeEach(func() { + loadBalancerClasses = append(loadBalancerClasses, api.LoadBalancerClass{ + Name: "test2", + }) + }) + + It("should pass as no name clashes, no duplicate default or private LoadBalancerClasses", func() { + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(BeEmpty()) + }) + + It("should fail as names of LoadBalancerClasses are not unique", func() { + loadBalancerClasses[0].Name = "test" + loadBalancerClasses[1].Name = "test" + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeDuplicate), + "Field": Equal("loadBalancerClasses[1].name"), + })))) + }) + + Context("Default LoadBalancerClasses", func() { + It("should fail as there are multiple LoadBalancerClasses with purpose default", func() { + loadBalancerClasses[0].Purpose = pointer.StringPtr("default") + loadBalancerClasses[1].Purpose = pointer.StringPtr("default") + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeInvalid), + "Field": Equal("loadBalancerClasses"), + })))) + }) + + It("should fail as there are multiple default LoadBalancerClasses", func() { + loadBalancerClasses[0].Purpose = pointer.StringPtr("default") + loadBalancerClasses[1].Name = "default" + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeInvalid), + "Field": Equal("loadBalancerClasses"), + })))) + }) + }) + + Context("Private LoadBalancerClasses", func() { + It("should fail as there are multiple LoadBalancerClasses with purpose private", func() { + loadBalancerClasses[0].Purpose = pointer.StringPtr("private") + loadBalancerClasses[1].Purpose = pointer.StringPtr("private") + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeInvalid), + "Field": Equal("loadBalancerClasses"), + })))) + }) + + It("should fail as there are multiple private LoadBalancerClasses", func() { + loadBalancerClasses[0].Purpose = pointer.StringPtr("private") + loadBalancerClasses[1].Name = "private" + + errorList := ValidateLoadBalancerClasses(loadBalancerClasses, fieldPath) + Expect(errorList).To(ConsistOf(PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeInvalid), + "Field": Equal("loadBalancerClasses"), + })))) + }) + }) + }) }) diff --git a/pkg/apis/openstack/validation/controlplane.go b/pkg/apis/openstack/validation/controlplane.go index d0ad2ece5..4fb0fdd31 100644 --- a/pkg/apis/openstack/validation/controlplane.go +++ b/pkg/apis/openstack/validation/controlplane.go @@ -32,14 +32,13 @@ func ValidateControlPlaneConfig(controlPlaneConfig *api.ControlPlaneConfig, vers allErrs = append(allErrs, field.Required(fldPath.Child("loadBalancerProvider"), "must provide the name of a load balancer provider")) } + loadBalancerClassPath := fldPath.Child("loadBalancerClasses") + allErrs = append(allErrs, ValidateLoadBalancerClasses(controlPlaneConfig.LoadBalancerClasses, loadBalancerClassPath)...) for i, class := range controlPlaneConfig.LoadBalancerClasses { - lbClassPath := fldPath.Child("loadBalancerClasses").Index(i) - allErrs = append(allErrs, ValidateLoadBalancerClasses(class, lbClassPath)...) - // Do not allow that the user specify a vpn LoadBalancerClass in the controlplane config. // It need to come from the CloudProfile. if class.Purpose != nil && *class.Purpose == api.VPNLoadBalancerClass { - allErrs = append(allErrs, field.Invalid(lbClassPath, class.Purpose, fmt.Sprintf("not allowed to specify a LoadBalancerClass with purpose %q", api.VPNLoadBalancerClass))) + allErrs = append(allErrs, field.Invalid(loadBalancerClassPath.Index(i), class.Purpose, fmt.Sprintf("not allowed to specify a LoadBalancerClass with purpose %q", api.VPNLoadBalancerClass))) } } @@ -133,7 +132,7 @@ func validateLoadBalancerClassesConstraints(floatingPools []api.FloatingPool, sh ) for _, lbClass := range fp.LoadBalancerClasses { - if equality.Semantic.DeepEqual(shootLBClass, lbClass) { + if shootLBClass.IsSemanticallyEqual(lbClass) { valid = true break } From d3793962837b940efa6222e7549732933275f0f5 Mon Sep 17 00:00:00 2001 From: "Kistner, Dominic" Date: Mon, 19 Jul 2021 08:36:25 +0200 Subject: [PATCH 2/2] Address feedaback --- pkg/apis/openstack/validation/cloudprofile.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/apis/openstack/validation/cloudprofile.go b/pkg/apis/openstack/validation/cloudprofile.go index 8ad259b3d..b24e0986a 100644 --- a/pkg/apis/openstack/validation/cloudprofile.go +++ b/pkg/apis/openstack/validation/cloudprofile.go @@ -190,6 +190,7 @@ func validateLoadBalancerClass(lbClass api.LoadBalancerClass, fldPath *field.Pat return allErrs } +// ValidateLoadBalancerClasses validates a given list of LoadBalancerClass objects. func ValidateLoadBalancerClasses(loadBalancerClasses []api.LoadBalancerClass, fldPath *field.Path) field.ErrorList { var ( defaultClassExists bool