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
  • Loading branch information
@ty-dc
ty-dc committed Oct 25, 2024
1 parent 0045f95 commit ad18cb2
Show file tree
Hide file tree
Showing 5 changed files with 186 additions and 51 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 @@ -372,7 +372,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
55 changes: 44 additions & 11 deletions pkg/multuscniconfig/multusconfig_validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@
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"

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 @@ -27,15 +31,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 @@ -191,18 +195,47 @@ 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 nil
}
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))
}

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 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 @@ -54,6 +57,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 @@ -63,7 +68,7 @@ func (mcw *MultusConfigWebhook) ValidateCreate(ctx context.Context, obj runtime.
)
log.Sugar().Debugf("Request MultusConfig: %+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 @@ -86,7 +91,7 @@ func (mcw *MultusConfigWebhook) ValidateUpdate(ctx context.Context, oldObj, newO
log.Sugar().Debugf("Request old MultusConfig: %+v", *oldMultusConfig)
log.Sugar().Debugf("Request new MultusConfig: %+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
45 changes: 24 additions & 21 deletions test/doc/spidermultus.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,27 @@
| Case ID | Title | Priority | Smoke | Status | Other |
|---------|---------------------------------------------------------------------------------------------------------------------------------------------------------| -------- |-------| ------ | ----- |
| M00001 | testing creating spiderMultusConfig with cniType: macvlan and checking the net-attach-conf config if works | p1 | smoke | done | |
| M00002 | testing creating spiderMultusConfig with cniType: ipvlan and checking the net-attach-conf config if works | p1 | smoke | done | |
| M00003 | testing creating spiderMultusConfig with cniType: sriov and checking the net-attach-conf config if works | p1 | smoke | done | |
| 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 | |
| M00007 | 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 | | |
| M00011 | After deleting spiderMultusConfig, the corresponding net-attach-conf will also be deleted | p2 | | done | |
| M00013 | Update spidermultusConfig: add new bond config | p1 | smoke | | |
| M00014 | Manually delete the net-attach-conf of multus, it will be created automatically | p1 | | done | |
| M00015 | Customize net-attach-conf name via annotation multus.spidernet.io/cr-name | p2 | | done | |
| M00016 | webhook validation for multus.spidernet.io/cr-name | p3 | | done | |
| M00017 | Change net-attach-conf version via annotation multus.spidernet.io/cni-version | p2 | | done | |
| M00018 | webhook validation for multus.spidernet.io/cni-version | p3 | | done | |
| M00020 | Already have multus cr, spidermultus should take care of it | p3 | | done | |
| M00022 | The value of webhook verification cniType is inconsistent with cniConf | p3 | | done | |
| M00023 | vlan is not in the range of 0-4094 and will not be created | p3 | | done | |
| M00024 | set disableIPAM to true and see if multus's nad has ipam config | p3 | | done | |
| M00025 | set sriov.enableRdma to true and see if multus's nad has rdma config | p3 | | | |
| M00026 | set spidermultusconfig.spec to empty and see if works | p3 | | | |
| M00027 | test podRPFilter and hostRPFilter in spidermultusconfig | p3 | | done |
| M00028 | set hostRPFilter and podRPFilter to a invalid value | p3 | | done |
| M00029 | verify the podMACPrefix filed | p3 | | done | |
| M00002 | testing creating spiderMultusConfig with cniType: ipvlan and checking the net-attach-conf config if works | p1 | smoke | done | |
| M00003 | testing creating spiderMultusConfig with cniType: sriov and checking the net-attach-conf config if works | p1 | smoke | done | |
| 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 | |
| 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 | |
| M00011 | webhook validation for multus.spidernet.io/cr-name | p3 | | done | |
| M00012 | Change net-attach-conf version via annotation multus.spidernet.io/cni-version | p2 | | done | |
| M00013 | webhook validation for multus.spidernet.io/cni-version | p3 | | done | |
| M00014 | Already have multus cr, spidermultus should take care of it | p3 | | done | |
| M00015 | The value of webhook verification cniType is inconsistent with cniConf | p3 | | done | |
| M00016 | vlan is not in the range of 0-4094 and will not be created | p3 | | done | |
| M00017 | set disableIPAM to true and see if multus's nad has ipam config | p3 | | done | |
| 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 | |
| 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 | |
Loading

0 comments on commit ad18cb2

Please sign in to comment.