Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: same-name conflict check specified by multus.spidernet.io/cr-name #4168

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+165 −33
Open

fix: same-name conflict check specified by multus.spidernet.io/cr-name #4168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cmd/spiderpool-controller/cmd/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}
Expand Down
57 changes: 46 additions & 11 deletions pkg/multuscniconfig/multusconfig_validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -31,15 +35,15 @@ 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 {
return err
}
}

err := validateAnnotation(multusConfig)
err := mcw.validateAnnotation(ctx, multusConfig)
if nil != err {
return err
}
Expand Down Expand Up @@ -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))
Expand Down
13 changes: 9 additions & 4 deletions pkg/multuscniconfig/multusconfig_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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)

Expand All @@ -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(),
Expand All @@ -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(),
Expand Down
13 changes: 6 additions & 7 deletions test/doc/spidermultus.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 | |
Expand All @@ -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 | |
111 changes: 101 additions & 10 deletions test/e2e/spidermultus/spidermultus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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() {
Expand Down
Loading