Skip to content

Commit

Permalink
fix: same-name conflict check specified by multus.spidernet.io/cr-name
Browse files Browse the repository at this point in the history 
Signed-off-by: tao.yang <tao.yang@daocloud.io>
  • Loading branch information
@ty-dc
ty-dc committed Oct 18, 2024
1 parent 50dcb53 commit ee6c7c7
Show file tree
Hide file tree
Showing 5 changed files with 165 additions and 33 deletions.
4 changes: 3 additions & 1 deletion cmd/spiderpool-controller/cmd/daemon.go
View file
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
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 {
// 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
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
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
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

0 comments on commit ee6c7c7

Please sign in to comment.