From ae327b96126b35025cdcbf6f0ccc210695167d77 Mon Sep 17 00:00:00 2001 From: "tao.yang" Date: Sat, 12 Oct 2024 18:14:33 +0800 Subject: [PATCH] fix: same-name conflict check specified by multus.spidernet.io/cr-name Signed-off-by: tao.yang --- cmd/spiderpool-controller/cmd/daemon.go | 4 +- pkg/multuscniconfig/multusconfig_validate.go | 57 ++++++++-- pkg/multuscniconfig/multusconfig_webhook.go | 13 ++- test/doc/spidermultus.md | 13 +-- test/e2e/spidermultus/spidermultus_test.go | 111 +++++++++++++++++-- 5 files changed, 165 insertions(+), 33 deletions(-) diff --git a/cmd/spiderpool-controller/cmd/daemon.go b/cmd/spiderpool-controller/cmd/daemon.go index 88023c0565..2750208b3a 100644 --- a/cmd/spiderpool-controller/cmd/daemon.go +++ b/cmd/spiderpool-controller/cmd/daemon.go @@ -386,7 +386,9 @@ func initControllerServiceManagers(ctx context.Context) { if controllerContext.Cfg.EnableMultusConfig { logger.Debug("Begin to set up MultusConfig webhook") - if err := (&multuscniconfig.MultusConfigWebhook{}).SetupWebhookWithManager(controllerContext.CRDManager); nil != err { + if err := (&multuscniconfig.MultusConfigWebhook{ + APIReader: controllerContext.CRDManager.GetAPIReader(), + }).SetupWebhookWithManager(controllerContext.CRDManager); nil != err { logger.Fatal(err.Error()) } } diff --git a/pkg/multuscniconfig/multusconfig_validate.go b/pkg/multuscniconfig/multusconfig_validate.go index 81d4650e1c..129e88b5f9 100644 --- a/pkg/multuscniconfig/multusconfig_validate.go +++ b/pkg/multuscniconfig/multusconfig_validate.go @@ -4,14 +4,18 @@ package multuscniconfig import ( + "context" "encoding/json" "fmt" + apierrors "k8s.io/apimachinery/pkg/api/errors" + ktypes "k8s.io/apimachinery/pkg/types" k8svalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/strings/slices" "github.com/containernetworking/cni/libcni" + netv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" "github.com/spidernet-io/spiderpool/cmd/spiderpool/cmd" "github.com/spidernet-io/spiderpool/pkg/constant" "github.com/spidernet-io/spiderpool/pkg/coordinatormanager" @@ -31,7 +35,7 @@ var ( annotationField = field.NewPath("metadata").Child("annotations") ) -func validate(oldMultusConfig, multusConfig *spiderpoolv2beta1.SpiderMultusConfig) *field.Error { +func (mcw *MultusConfigWebhook) validate(ctx context.Context, oldMultusConfig, multusConfig *spiderpoolv2beta1.SpiderMultusConfig) *field.Error { if oldMultusConfig != nil { err := validateCustomAnnoNameShouldNotBeChangeable(oldMultusConfig, multusConfig) if nil != err { @@ -39,7 +43,7 @@ func validate(oldMultusConfig, multusConfig *spiderpoolv2beta1.SpiderMultusConfi } } - err := validateAnnotation(multusConfig) + err := mcw.validateAnnotation(ctx, multusConfig) if nil != err { return err } @@ -260,18 +264,49 @@ func validateVlanId(vlanId int32) error { return nil } -func validateAnnotation(multusConfig *spiderpoolv2beta1.SpiderMultusConfig) *field.Error { - // validate the custom net-attach-def resource name - customMultusName, ok := multusConfig.Annotations[constant.AnnoNetAttachConfName] - if ok && customMultusName == "" { - return field.Invalid(annotationField, multusConfig.Annotations, "invalid custom net-attach-def resource empty name") +func (mcw *MultusConfigWebhook) validateAnnotation(ctx context.Context, multusConfig *spiderpoolv2beta1.SpiderMultusConfig) *field.Error { + // Helper function to check net-attach-def existence and ownership + checkNetAttachDef := func(namespace, name string) *field.Error { + netAttachDef := &netv1.NetworkAttachmentDefinition{} + err := mcw.APIReader.Get(ctx, ktypes.NamespacedName{Namespace: namespace, Name: name}, netAttachDef) + if err != nil { + if !apierrors.IsNotFound(err) { + return field.InternalError(annotationField, + fmt.Errorf("failed to retrieve net-attach-def %s/%s, unable to determine conflicts with existing resources. error: %v", namespace, name, err)) + } + } else { + for _, ownerRef := range netAttachDef.OwnerReferences { + if ownerRef.Kind == constant.KindSpiderMultusConfig && ownerRef.Name != multusConfig.Name { + // net-attach-def already exists and is managed by SpiderMultusConfig, do not allow the creation of SpiderMultusConfig to take over its management. + return field.Invalid(annotationField, multusConfig.Annotations, + fmt.Sprintf("the net-attach-def %s/%s already exists and is managed by SpiderMultusConfig %s/%s.", namespace, name, namespace, ownerRef.Name)) + } + } + // The net-attach-def already exists and is not managed by SpiderMultusConfig, allow the creation of SpiderMultusConfig to take over its management. + } + return nil } - if len(customMultusName) > k8svalidation.DNS1123SubdomainMaxLength { - return field.Invalid(annotationField, multusConfig.Annotations, - fmt.Sprintf("the custom net-attach-def resource name must be no more than %d characters", k8svalidation.DNS1123SubdomainMaxLength)) + + // Validate the annotation 'multus.spidernet.io/cr-name' to customize the net-attach-def resource name. + if customMultusName, hasCustomMultusName := multusConfig.Annotations[constant.AnnoNetAttachConfName]; hasCustomMultusName { + if customMultusName == "" { + return field.Invalid(annotationField, multusConfig.Annotations, "invalid custom net-attach-def resource empty name") + } + + if errs := k8svalidation.IsDNS1123Subdomain(customMultusName); len(errs) != 0 { + return field.Invalid(annotationField, multusConfig.Annotations, fmt.Sprintf("invalid custom net-attach-def resource name, err: %v", errs)) + } + + if err := checkNetAttachDef(multusConfig.Namespace, customMultusName); err != nil { + return err + } + } else { + if err := checkNetAttachDef(multusConfig.Namespace, multusConfig.Name); err != nil { + return err + } } - // validate the custom net-attach-def CNI version + // Validate the custom net-attach-def CNI version cniVersion, ok := multusConfig.Annotations[constant.AnnoMultusConfigCNIVersion] if ok && !slices.Contains(cmd.SupportCNIVersions, cniVersion) { return field.Invalid(annotationField, multusConfig.Annotations, fmt.Sprintf("unsupported CNI version %s", cniVersion)) diff --git a/pkg/multuscniconfig/multusconfig_webhook.go b/pkg/multuscniconfig/multusconfig_webhook.go index d89d09cc36..809f1cef59 100644 --- a/pkg/multuscniconfig/multusconfig_webhook.go +++ b/pkg/multuscniconfig/multusconfig_webhook.go @@ -12,6 +12,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -22,7 +23,9 @@ import ( var logger *zap.Logger -type MultusConfigWebhook struct{} +type MultusConfigWebhook struct { + APIReader client.Reader +} func (mcw *MultusConfigWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error { if logger == nil { @@ -36,7 +39,7 @@ func (mcw *MultusConfigWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error Complete() } -var _ webhook.CustomValidator = &MultusConfigWebhook{} +var _ webhook.CustomDefaulter = (*MultusConfigWebhook)(nil) // Default implements admission.CustomDefaulter. func (*MultusConfigWebhook) Default(ctx context.Context, obj runtime.Object) error { @@ -51,6 +54,8 @@ func (*MultusConfigWebhook) Default(ctx context.Context, obj runtime.Object) err return nil } +var _ webhook.CustomValidator = (*MultusConfigWebhook)(nil) + func (mcw *MultusConfigWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { multusConfig := obj.(*spiderpoolv2beta1.SpiderMultusConfig) @@ -60,7 +65,7 @@ func (mcw *MultusConfigWebhook) ValidateCreate(ctx context.Context, obj runtime. ) log.Sugar().Debugf("Request SpiderMultusConfig: %+v", *multusConfig) - err := validate(nil, multusConfig) + err := mcw.validate(logutils.IntoContext(ctx, logger), nil, multusConfig) if nil != err { return nil, apierrors.NewInvalid( spiderpoolv2beta1.SchemeGroupVersion.WithKind(constant.KindSpiderMultusConfig).GroupKind(), @@ -83,7 +88,7 @@ func (mcw *MultusConfigWebhook) ValidateUpdate(ctx context.Context, oldObj, newO log.Sugar().Debugf("Request old SpiderMultusConfig: %+v", *oldMultusConfig) log.Sugar().Debugf("Request new SpiderMultusConfig: %+v", *newMultusConfig) - err := validate(oldMultusConfig, newMultusConfig) + err := mcw.validate(logutils.IntoContext(ctx, logger), oldMultusConfig, newMultusConfig) if nil != err { return nil, apierrors.NewInvalid( spiderpoolv2beta1.SchemeGroupVersion.WithKind(constant.KindSpiderMultusConfig).GroupKind(), diff --git a/test/doc/spidermultus.md b/test/doc/spidermultus.md index 2bbdb840d7..001d8c8fb7 100644 --- a/test/doc/spidermultus.md +++ b/test/doc/spidermultus.md @@ -8,7 +8,7 @@ | M00004 | testing creating spiderMultusConfig with cniType: custom and checking the net-attach-conf config if works | p1 | smoke | done | | | M00005 | testing creating spiderMultusConfig with cniType: custom and invalid json config, expect error happened | p2 | | done | | | M00006 | testing creating spiderMultusConfig with cniType: macvlan with vlanId with two master with bond config and checking the net-attach-conf config if works | p1 | smoke | done | | -| M00007 | Manually delete the net-attach-conf of multus, it will be created automatically | p1 | smoke | done | +| M00007 | Manually delete the net-attach-conf of multus, it will be created automatically | p1 | smoke | done | | | M00008 | After deleting spiderMultusConfig, the corresponding net-attach-conf will also be deleted | p2 | | done | | | M00009 | Update spidermultusConfig: add new bond config | p1 | smoke | done | | | M00010 | Customize net-attach-conf name via annotation multus.spidernet.io/cr-name | p2 | | done | | @@ -22,9 +22,8 @@ | M00018 | set sriov.enableRdma to true and see if multus's nad has rdma config | p3 | | done | | | M00019 | set spidermultusconfig.spec to empty and see if works | p3 | | done | | | M00020 | annotating custom names that are too long or empty should fail | p3 | | done | | -| M00021 | create a spidermultusconfig and pod to verify chainCNI json config if works | p3 | | done | | -| M00022 | test podRPFilter and hostRPFilter in spidermultusconfig | p3 | | done | -| M00023 | set hostRPFilter and podRPFilter to a invalid value | p3 | | done | -| M00024 | verify the podMACPrefix filed | p3 | | done | | - - +| M00021 | create a spidermultusconfig and pod to verify chainCNI json config if works | p3 | | done | | +| M00022 | test podRPFilter and hostRPFilter in spidermultusconfig | p3 | | done | | +| M00023 | set hostRPFilter and podRPFilter to a invalid value | p3 | | done | | +| M00024 | verify the podMACPrefix filed | p3 | | done | | +| M00025 | The custom net-attach-conf name from the annotation multus.spidernet.io/cr-name doesn't follow Kubernetes naming rules and can't be created. | p3 | | done | | diff --git a/test/e2e/spidermultus/spidermultus_test.go b/test/e2e/spidermultus/spidermultus_test.go index bbfce4f905..1115a73942 100644 --- a/test/e2e/spidermultus/spidermultus_test.go +++ b/test/e2e/spidermultus/spidermultus_test.go @@ -179,23 +179,104 @@ var _ = Describe("test spidermultus", Label("SpiderMultusConfig"), func() { }, common.SpiderSyncMultusTime, common.ForcedWaitingTime).Should(BeTrue()) }) - It("annotating custom names that are too long or empty should fail", Label("M00011", "M00020"), func() { + It("webhook validation: New and existing SpiderMultusConfig in the same namespace with the same customMultusName will not be created due to a conflict.", Label("M00011"), func() { + // Create SpiderMultusConfig and customize the net-attach-def name by annotating multus.spidernet.io/cr-name + testSmc := smc.DeepCopy() + testSmc.Name = "test-smc-1-" + common.GenerateString(10, true) + sameCustomMultusName := "test-custom-multus-" + common.GenerateString(10, true) + testSmc.Annotations = map[string]string{constant.AnnoNetAttachConfName: sameCustomMultusName} + GinkgoWriter.Printf("spidermultus cr with annotations: '%+v' \n", testSmc.Annotations) + Expect(frame.CreateSpiderMultusInstance(testSmc)).NotTo(HaveOccurred()) + + Eventually(func() bool { + spiderMultusConfig, err := frame.GetSpiderMultusInstance(namespace, testSmc.Name) + if err != nil || spiderMultusConfig == nil { + return false + } + return true + }, common.SpiderSyncMultusTime, common.ForcedWaitingTime).Should(BeTrue()) + + // Create another SpiderMultusConfig with the same custom net-attach-def name + newSmc := smc.DeepCopy() + newSmc.Name = "test-another-smc-1-" + common.GenerateString(10, true) + newSmc.Annotations = map[string]string{constant.AnnoNetAttachConfName: sameCustomMultusName} + GinkgoWriter.Printf("spidermultus cr with annotations: %+v \n", newSmc.Annotations) + err := frame.CreateSpiderMultusInstance(newSmc) + GinkgoWriter.Printf("should fail to create, the error is: %v", err.Error()) + Expect(err).To(HaveOccurred()) + }) + + It("webhook validation: the custom net-attach-def name of SpiderMultusConfig conflicts with the existing SpiderMultusConfig name, and cannot be created.", Label("M00011"), func() { + // Create SpiderMultusConfig in advance + testSmc := smc.DeepCopy() + testSmc.Name = "test-smc-2-" + common.GenerateString(10, true) + Expect(frame.CreateSpiderMultusInstance(testSmc)).NotTo(HaveOccurred()) + Eventually(func() bool { + spiderMultusConfig, err := frame.GetSpiderMultusInstance(namespace, testSmc.Name) + if err != nil || spiderMultusConfig == nil { + return false + } + return true + }, common.SpiderSyncMultusTime, common.ForcedWaitingTime).Should(BeTrue()) + + // New SpiderMultusConfig's custom net-attach-def name conflicts with existing SpiderMultusConfig's name + newSmc := smc.DeepCopy() + newSmc.Name = "test-another-smc-2-" + common.GenerateString(10, true) + newSmc.Annotations = map[string]string{constant.AnnoNetAttachConfName: testSmc.Name} + GinkgoWriter.Printf("spidermultus cr with annotations: %+v \n", newSmc.Annotations) + err := frame.CreateSpiderMultusInstance(newSmc) + GinkgoWriter.Printf("should fail to create, the error is: %v", err.Error()) + Expect(err).To(HaveOccurred()) + }) + + It("webhook validation: the name of SpiderMultusConfig conflicts with the custom net-attach-def name of an existing SpiderMultusConfig, and cannot be created.", Label("M00011"), func() { + // Create SpiderMultusConfig and customize the net-attach-def name by annotating multus.spidernet.io/cr-name + testSmc := smc.DeepCopy() + testSmc.Name = "test-smc-3-" + common.GenerateString(10, true) + sameNewSmcName := "test-another-smc-3-" + common.GenerateString(10, true) + testSmc.ObjectMeta.Annotations = map[string]string{constant.AnnoNetAttachConfName: sameNewSmcName} + GinkgoWriter.Printf("spidermultus cr with annotations: '%+v' \n", testSmc.Annotations) + Expect(frame.CreateSpiderMultusInstance(testSmc)).NotTo(HaveOccurred()) + + Eventually(func() bool { + spiderMultusConfig, err := frame.GetSpiderMultusInstance(namespace, testSmc.Name) + if err != nil || spiderMultusConfig == nil { + return false + } + return true + }, common.SpiderSyncMultusTime, common.ForcedWaitingTime).Should(BeTrue()) + + // Create another SpiderMultusConfig with the same name as the existing SpidermultusConfig's custom net-attach-def. + newSmc := smc.DeepCopy() + newSmc.Name = sameNewSmcName + err := frame.CreateSpiderMultusInstance(newSmc) + GinkgoWriter.Printf("should fail to create, the error is: %v", err.Error()) + Expect(err).To(HaveOccurred()) + }) + + It("annotating custom names that are too long or empty should fail", Label("M00020"), func() { + testSmc := smc.DeepCopy() longCustomizedName := common.GenerateString(k8svalidation.DNS1123SubdomainMaxLength+1, true) - smc.ObjectMeta.Annotations = map[string]string{constant.AnnoNetAttachConfName: longCustomizedName} - GinkgoWriter.Printf("spidermultus cr with annotations: '%+v' \n", smc) - Expect(frame.CreateSpiderMultusInstance(smc)).To(HaveOccurred()) + testSmc.ObjectMeta.Annotations = map[string]string{constant.AnnoNetAttachConfName: longCustomizedName} + GinkgoWriter.Printf("spidermultus cr with annotations: '%+v' \n", testSmc.Annotations) + err := frame.CreateSpiderMultusInstance(testSmc) + errorMsg := fmt.Sprintf("must be no more than %d characters", k8svalidation.DNS1123SubdomainMaxLength) + Expect(err).To(MatchError(ContainSubstring(errorMsg))) emptyCustomizedName := "" - smc.ObjectMeta.Annotations = map[string]string{constant.AnnoNetAttachConfName: emptyCustomizedName} - GinkgoWriter.Printf("spidermultus cr with annotations: %+v \n", smc) - Expect(frame.CreateSpiderMultusInstance(smc)).To(HaveOccurred()) + testSmc.ObjectMeta.Annotations = map[string]string{constant.AnnoNetAttachConfName: emptyCustomizedName} + GinkgoWriter.Printf("spidermultus cr with annotations: %+v \n", testSmc.Annotations) + err = frame.CreateSpiderMultusInstance(testSmc) + errorMsg = "invalid custom net-attach-def resource empty name" + Expect(err).To(MatchError(ContainSubstring(errorMsg))) }) It("Change net-attach-conf version via annotation multus.spidernet.io/cni-version", Label("M00012"), func() { + testSmc := smc.DeepCopy() cniVersion := "0.4.0" - smc.ObjectMeta.Annotations = map[string]string{constant.AnnoMultusConfigCNIVersion: cniVersion} - GinkgoWriter.Printf("spidermultus cr with annotations: %+v \n", smc) - Expect(frame.CreateSpiderMultusInstance(smc)).NotTo(HaveOccurred()) + testSmc.ObjectMeta.Annotations = map[string]string{constant.AnnoMultusConfigCNIVersion: cniVersion} + GinkgoWriter.Printf("spidermultus cr with annotations: %+v \n", testSmc.Annotations) + Expect(frame.CreateSpiderMultusInstance(testSmc)).NotTo(HaveOccurred()) spiderMultusConfig, err := frame.GetSpiderMultusInstance(namespace, spiderMultusNadName) Expect(err).NotTo(HaveOccurred()) @@ -223,6 +304,16 @@ var _ = Describe("test spidermultus", Label("SpiderMultusConfig"), func() { // Mismatched versions, when doing a build, the error should occur here? Expect(frame.CreateSpiderMultusInstance(smc)).To(HaveOccurred()) }) + + It("The custom net-attach-conf name from the annotation multus.spidernet.io/cr-name doesn't follow Kubernetes naming rules and can't be created.", Label("M00025"), func() { + testSmc := smc.DeepCopy() + customNadName := "custom-error-name************" + testSmc.ObjectMeta.Annotations = map[string]string{constant.AnnoNetAttachConfName: customNadName} + GinkgoWriter.Printf("spidermultus cr with annotations: %+v \n", testSmc.Annotations) + err := frame.CreateSpiderMultusInstance(testSmc) + errorMsg := "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character" + Expect(err).To(MatchError(ContainSubstring(errorMsg))) + }) }) It("Already have multus cr, spidermultus should take care of it", Label("M00014"), func() {