diff --git a/pkg/utils/nad.go b/pkg/utils/nad.go index 15dc64a3..b0c622bc 100644 --- a/pkg/utils/nad.go +++ b/pkg/utils/nad.go @@ -6,6 +6,11 @@ import ( "net" "strings" + ctlcniv1 "github.com/harvester/harvester/pkg/generated/controllers/k8s.cni.cncf.io/v1" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + cniv1 "github.com/containernetworking/cni/pkg/types" nadv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" @@ -138,7 +143,7 @@ func isMaskZero(ipnet *net.IPNet) bool { // if this nad is a storagenetwork nad func IsStorageNetworkNad(nad *nadv1.NetworkAttachmentDefinition) bool { - if nad == nil { + if nad == nil || nad.Namespace != harvesterutil.HarvesterSystemNamespaceName { return false } @@ -148,9 +153,79 @@ func IsStorageNetworkNad(nad *nadv1.NetworkAttachmentDefinition) bool { } // check name - if nad.Namespace == harvesterutil.HarvesterSystemNamespaceName && strings.HasPrefix(nad.Name, StorageNetworkNetAttachDefPrefix) { + if strings.HasPrefix(nad.Name, StorageNetworkNetAttachDefPrefix) { return true } return false } + +// filter the first storage network nad from a list of nads +func FilterFirstActiveStorageNetworkNad(nads []*nadv1.NetworkAttachmentDefinition) *nadv1.NetworkAttachmentDefinition { + if len(nads) == 0 { + return nil + } + for _, nad := range nads { + if IsStorageNetworkNad(nad) && nad.DeletionTimestamp == nil { + return nad + } + } + return nil +} + +type NadGetter struct { + nadCache ctlcniv1.NetworkAttachmentDefinitionCache +} + +func NewNadGetter(nadCache ctlcniv1.NetworkAttachmentDefinitionCache) *NadGetter { + return &NadGetter{nadCache: nadCache} +} + +// list all Nads attached to a cluster network +func (n *NadGetter) ListNadsOnClusterNetwork(cnName string) ([]*nadv1.NetworkAttachmentDefinition, error) { + nads, err := n.nadCache.List(corev1.NamespaceAll, labels.Set(map[string]string{ + KeyClusterNetworkLabel: cnName, + }).AsSelector()) + if err != nil { + return nil, err + } + + if len(nads) == 0 { + return nil, nil + } + return nads, nil +} + +func (n *NadGetter) GetFirstActiveStorageNetworkNadOnClusterNetwork(cnName string) (*nadv1.NetworkAttachmentDefinition, error) { + nads, err := n.nadCache.List(harvesterutil.HarvesterSystemNamespaceName, labels.Set(map[string]string{ + KeyClusterNetworkLabel: cnName, + }).AsSelector()) + if err != nil { + return nil, err + } + + if len(nads) == 0 { + return nil, nil + } + + return FilterFirstActiveStorageNetworkNad(nads), nil +} + +func (n *NadGetter) NadNamesOnClusterNetwork(cnName string) ([]string, error) { + nads, err := n.ListNadsOnClusterNetwork(cnName) + if err != nil { + return nil, err + } + return generateNadNameList(nads), nil +} + +func generateNadNameList(nads []*nadv1.NetworkAttachmentDefinition) []string { + if len(nads) == 0 { + return nil + } + nadStrList := make([]string, len(nads)) + for i, nad := range nads { + nadStrList[i] = nad.Namespace + "/" + nad.Name + } + return nadStrList +} diff --git a/pkg/utils/vmi.go b/pkg/utils/vmi.go index 4254d65c..fe213602 100644 --- a/pkg/utils/vmi.go +++ b/pkg/utils/vmi.go @@ -14,6 +14,10 @@ type VmiGetter struct { VmiCache ctlkubevirtv1.VirtualMachineInstanceCache } +func NewVmiGetter(vmiCache ctlkubevirtv1.VirtualMachineInstanceCache) *VmiGetter { + return &VmiGetter{VmiCache: vmiCache} +} + // WhoUseNad requires adding network indexer to the vmi cache before invoking it func (v *VmiGetter) WhoUseNad(nad *nadv1.NetworkAttachmentDefinition, nodesFilter mapset.Set[string]) ([]*kubevirtv1.VirtualMachineInstance, error) { // multus network name can be or / @@ -65,7 +69,7 @@ func (v *VmiGetter) WhoUseNads(nads []*nadv1.NetworkAttachmentDefinition, nodesF } // Get the vmi name list who uses the nad -func (v *VmiGetter) VmiNameWhoUseNad(nad *nadv1.NetworkAttachmentDefinition, nodesFilter mapset.Set[string]) ([]string, error) { +func (v *VmiGetter) VmiNamesWhoUseNad(nad *nadv1.NetworkAttachmentDefinition, nodesFilter mapset.Set[string]) ([]string, error) { vmis, err := v.WhoUseNad(nad, nodesFilter) if err != nil { return nil, err @@ -75,7 +79,7 @@ func (v *VmiGetter) VmiNameWhoUseNad(nad *nadv1.NetworkAttachmentDefinition, nod } // Get the vmi name list who uses a group of nads, duplicated names may exist when they attache to mutli nads -func (v *VmiGetter) VmiNameWhoUseNads(nads []*nadv1.NetworkAttachmentDefinition, nodesFilter mapset.Set[string]) ([]string, error) { +func (v *VmiGetter) VmiNamesWhoUseNads(nads []*nadv1.NetworkAttachmentDefinition, nodesFilter mapset.Set[string]) ([]string, error) { vmis, err := v.WhoUseNads(nads, nodesFilter) if err != nil { return nil, err diff --git a/pkg/webhook/clusternetwork/validator.go b/pkg/webhook/clusternetwork/validator.go index ce528ed7..736fad7c 100644 --- a/pkg/webhook/clusternetwork/validator.go +++ b/pkg/webhook/clusternetwork/validator.go @@ -8,7 +8,6 @@ import ( ctlkubevirtv1 "github.com/harvester/harvester/pkg/generated/controllers/kubevirt.io/v1" "github.com/harvester/webhook/pkg/server/admission" admissionregv1 "k8s.io/api/admissionregistration/v1" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" @@ -103,25 +102,18 @@ func (c *CnValidator) Delete(_ *admission.Request, oldObj runtime.Object) error } // all related nads should be deleted - nads, err := c.nadCache.List(corev1.NamespaceAll, labels.Set(map[string]string{ - utils.KeyClusterNetworkLabel: cn.Name, - }).AsSelector()) + nadGetter := utils.NewNadGetter(c.nadCache) + nadStrList, err := nadGetter.NadNamesOnClusterNetwork(cn.Name) if err != nil { - return fmt.Errorf(deleteErr, cn.Name, err) - } - - if len(nads) == 0 { - return nil + return err } - nadStrList := make([]string, len(nads)) - for i, nad := range nads { - nadStrList[i] = nad.Namespace + "/" + nad.Name + if len(nadStrList) > 0 { + return fmt.Errorf(deleteErr, cn.Name, fmt.Errorf("nads(s) %v under this clusternetwork are still existing", strings.Join(nadStrList, ", "))) } - return fmt.Errorf(deleteErr, cn.Name, fmt.Errorf("nads(s) %v under this clusternetwork are still existing", strings.Join(nadStrList, ", "))) - - // TODO: check vmi, vm as well? + // TODO: check vmi, vm as well? vmi is tied with NAD; vm will also be tied with NAD + return nil } func (c *CnValidator) Resource() admission.Resource { @@ -203,25 +195,18 @@ func (c *CnValidator) checkMTUOfUpdatedMgmtClusterNetwork(oldCn, newCn *networkv } // for mgmt network, the nad is not tied to any vlanconfig, check nad directly - nads, err := c.nadCache.List(corev1.NamespaceAll, labels.Set(map[string]string{ - utils.KeyClusterNetworkLabel: newCn.Name, - }).AsSelector()) + nadGetter := utils.NewNadGetter(c.nadCache) + nads, err := nadGetter.ListNadsOnClusterNetwork(newCn.Name) if err != nil { - return fmt.Errorf(updateErr, newCn.Name, err) - } - - if len(nads) == 0 { - return nil + return err } - for _, nad := range nads { - if utils.IsStorageNetworkNad(nad) && nad.DeletionTimestamp == nil { - return fmt.Errorf(updateErr, newCn.Name, fmt.Errorf("the MTU can't be changed from %v to %v as storage network nad %s is still attached", oldMtu, newMtu, nad.Name)) - } + if nad := utils.FilterFirstActiveStorageNetworkNad(nads); nad != nil { + return fmt.Errorf(updateErr, newCn.Name, fmt.Errorf("the MTU can't be changed from %v to %v as storage network nad %s is still attached", oldMtu, newMtu, nad.Name)) } - vmiGetter := utils.VmiGetter{VmiCache: c.vmiCache} - if vmiStrList, err := vmiGetter.VmiNameWhoUseNads(nads, nil); err != nil { + vmiGetter := utils.NewVmiGetter(c.vmiCache) + if vmiStrList, err := vmiGetter.VmiNamesWhoUseNads(nads, nil); err != nil { return err } else if len(vmiStrList) > 0 { return fmt.Errorf(updateErr, newCn.Name, fmt.Errorf("the MTU can't be changed from %v to %v as following VMs must be stopped at first: %s", oldMtu, newMtu, strings.Join(vmiStrList, ", "))) diff --git a/pkg/webhook/nad/validator.go b/pkg/webhook/nad/validator.go index 5a11bc94..d37ac655 100644 --- a/pkg/webhook/nad/validator.go +++ b/pkg/webhook/nad/validator.go @@ -175,18 +175,11 @@ func (v *Validator) checkRoute(config string) error { } func (v *Validator) checkVmi(nad *cniv1.NetworkAttachmentDefinition) error { - vmiGetter := utils.VmiGetter{VmiCache: v.vmiCache} - vmis, err := vmiGetter.WhoUseNad(nad, nil) - if err != nil { + vmiGetter := utils.NewVmiGetter(v.vmiCache) + if vmiStrList, err := vmiGetter.VmiNamesWhoUseNad(nad, nil); err != nil { return err - } - - if len(vmis) > 0 { - vmiNameList := make([]string, len(vmis), len(vmis)) - for i, vmi := range vmis { - vmiNameList[i] = vmi.Namespace + "/" + vmi.Name - } - return fmt.Errorf("it's still used by VM(s) %s which must be stopped at first", strings.Join(vmiNameList, ", ")) + } else if len(vmiStrList) > 0 { + return fmt.Errorf("it's still used by VM(s) %s which must be stopped at first", strings.Join(vmiStrList, ", ")) } return nil diff --git a/pkg/webhook/nad/validator_test.go b/pkg/webhook/nad/validator_test.go index 21848e4b..15b5d531 100644 --- a/pkg/webhook/nad/validator_test.go +++ b/pkg/webhook/nad/validator_test.go @@ -391,7 +391,7 @@ func TestDeleteNAD(t *testing.T) { currentNAD: &cniv1.NetworkAttachmentDefinition{ ObjectMeta: metav1.ObjectMeta{ Name: testNadName, - Namespace: testNamespace, + Namespace: harvesterutil.HarvesterSystemNamespaceName, // storagenetwork nad is in given namespace Annotations: map[string]string{utils.StorageNetworkAnnotation: "true"}, Labels: map[string]string{utils.KeyClusterNetworkLabel: testCnName}, }, diff --git a/pkg/webhook/vlanconfig/validator.go b/pkg/webhook/vlanconfig/validator.go index 02bc17fc..e19d4945 100644 --- a/pkg/webhook/vlanconfig/validator.go +++ b/pkg/webhook/vlanconfig/validator.go @@ -14,9 +14,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" - kubevirtv1 "kubevirt.io/api/core/v1" - - harvesterutil "github.com/harvester/harvester/pkg/util" networkv1 "github.com/harvester/harvester-network-controller/pkg/apis/network.harvesterhci.io/v1beta1" ctlnetworkv1 "github.com/harvester/harvester-network-controller/pkg/generated/controllers/network.harvesterhci.io/v1beta1" @@ -62,15 +59,26 @@ var _ admission.Validator = &Validator{} func (v *Validator) Create(_ *admission.Request, newObj runtime.Object) error { vc := newObj.(*networkv1.VlanConfig) + if len(vc.Spec.ClusterNetwork) > maxClusterNetworkNameLen { + return fmt.Errorf(createErr, vc.Name, fmt.Errorf("the length of the clusterNetwork name is "+ + "more than %d", maxClusterNetworkNameLen)) + } + if vc.Spec.ClusterNetwork == utils.ManagementClusterNetworkName { return fmt.Errorf(createErr, vc.Name, fmt.Errorf("cluster network can't be %s", utils.ManagementClusterNetworkName)) } + // check if clusternetwork exists + if _, err := v.cnCache.Get(vc.Spec.ClusterNetwork); err != nil { + return fmt.Errorf(createErr, vc.Name, fmt.Errorf("it refers to a none-existing cluster network %s or error %w", vc.Spec.ClusterNetwork, err)) + } + if err := v.validateMTU(vc); err != nil { return fmt.Errorf(createErr, vc.Name, err) } + // note: the mutator has patched the Annotations[utils.KeyMatchedNodes] if selector is set and exclude the witness-node nodes, err := getMatchNodes(vc) if err != nil { return fmt.Errorf(createErr, vc.Name, err) @@ -80,11 +88,6 @@ func (v *Validator) Create(_ *admission.Request, newObj runtime.Object) error { return fmt.Errorf(createErr, vc.Name, err) } - if len(vc.Spec.ClusterNetwork) > maxClusterNetworkNameLen { - return fmt.Errorf(createErr, vc.Name, fmt.Errorf("the length of the clusterNetwork name is "+ - "more than %d", maxClusterNetworkNameLen)) - } - return nil } @@ -101,10 +104,23 @@ func (v *Validator) Update(_ *admission.Request, oldObj, newObj runtime.Object) return nil } + // check if clusternetwork exists + if _, err := v.cnCache.Get(newVc.Spec.ClusterNetwork); err != nil { + return fmt.Errorf(createErr, newVc.Name, fmt.Errorf("it refers to a none-existing cluster network %s or error %w", newVc.Spec.ClusterNetwork, err)) + } + + // TODO: do not allow such change? + /* + if oldVc.Spec.ClusterNetwork != oldVc.Spec.ClusterNetwork { + return fmt.Errorf("cluster network can't be changed from %v to %v", oldVc.Spec.ClusterNetwork, oldVc.Spec.ClusterNetwork) + } + */ + if err := v.validateMTU(newVc); err != nil { return fmt.Errorf(updateErr, newVc.Name, err) } + // note: the mutator has patched the Annotations[utils.KeyMatchedNodes] if selector is set and exclude the witness-node newNodes, err := getMatchNodes(newVc) if err != nil { return fmt.Errorf(updateErr, newVc.Name, err) @@ -125,7 +141,7 @@ func (v *Validator) Update(_ *admission.Request, oldObj, newObj runtime.Object) return fmt.Errorf(updateErr, oldVc.Name, err) } - return nil + return v.checkStorageNetwork(oldVc, affectedNodes) } func getAffectedNodes(oldCn, newCn string, oldNodes, newNodes mapset.Set[string]) mapset.Set[string] { @@ -148,22 +164,7 @@ func (v *Validator) Delete(_ *admission.Request, oldObj runtime.Object) error { return fmt.Errorf(deleteErr, vc.Name, err) } - nads, err := v.nadCache.List(harvesterutil.HarvesterSystemNamespaceName, labels.Set(map[string]string{ - utils.KeyClusterNetworkLabel: vc.Spec.ClusterNetwork, - }).AsSelector()) - if err != nil { - return fmt.Errorf(deleteErr, vc.Name, err) - } - - if len(nads) > 0 { - for _, nad := range nads { - if nad.DeletionTimestamp == nil && utils.IsStorageNetworkNad(nad) { - return fmt.Errorf(deleteErr, vc.Name, fmt.Errorf(`storage network nad %s is still attached`, nad.Name)) - } - } - } - - return nil + return v.checkStorageNetwork(vc, nodes) } func (v *Validator) Resource() admission.Resource { @@ -205,40 +206,32 @@ func (v *Validator) checkOverlaps(vc *networkv1.VlanConfig, nodes mapset.Set[str // checkVmi is to confirm if any VMIs will be affected on affected nodes. Those VMIs must be stopped in advance. func (v *Validator) checkVmi(vc *networkv1.VlanConfig, nodes mapset.Set[string]) error { - // The vlanconfig is not allowed to be deleted if it has applied to some nodes and its clusternetwork is attached by some nads. + // The vlanconfig is not allowed to be deleted if it has been applied to some nodes and its clusternetwork is attached by some nads. vss, err := v.vsCache.List(labels.Set(map[string]string{utils.KeyVlanConfigLabel: vc.Name}).AsSelector()) if err != nil { - return fmt.Errorf(deleteErr, vc.Name, err) + return fmt.Errorf("failed to list vlanstatus erro %w", err) } if len(vss) == 0 { return nil } - nads, err := v.nadCache.List("", labels.Set(map[string]string{ - utils.KeyClusterNetworkLabel: vc.Spec.ClusterNetwork, - }).AsSelector()) - if err != nil { - return fmt.Errorf(deleteErr, vc.Name, err) + // affect no nodes + if nodes == nil || nodes.Cardinality() == 0 { + return nil } - vmiGetter := utils.VmiGetter{VmiCache: v.vmiCache} - vmis := make([]*kubevirtv1.VirtualMachineInstance, 0) - for _, nad := range nads { - vmisTemp, err := vmiGetter.WhoUseNad(nad, nodes) - if err != nil { - return err - } - vmis = append(vmis, vmisTemp...) + nadGetter := utils.NewNadGetter(v.nadCache) + nads, err := nadGetter.ListNadsOnClusterNetwork(vc.Spec.ClusterNetwork) + if err != nil { + return err } - if len(vmis) > 0 { - vmiStrList := make([]string, len(vmis)) - for i, vmi := range vmis { - vmiStrList[i] = vmi.Namespace + "/" + vmi.Name - } - - return fmt.Errorf("it's blocked by VM(s) %s which must be stopped at first", strings.Join(vmiStrList, ", ")) + vmiGetter := utils.NewVmiGetter(v.vmiCache) + if vmiStrList, err := vmiGetter.VmiNamesWhoUseNads(nads, nodes); err != nil { + return err + } else if len(vmiStrList) > 0 { + return fmt.Errorf("it is blocked by VM(s) %s which must be stopped at first", strings.Join(vmiStrList, ", ")) } return nil @@ -282,3 +275,23 @@ func (v *Validator) validateMTU(current *networkv1.VlanConfig) error { return nil } + +// if storagenetwork nad is there, and affected node number > 0, then deny +func (v *Validator) checkStorageNetwork(vc *networkv1.VlanConfig, nodes mapset.Set[string]) error { + // affect no nodes + if nodes == nil || nodes.Cardinality() == 0 { + return nil + } + + nadGetter := utils.NewNadGetter(v.nadCache) + nad, err := nadGetter.GetFirstActiveStorageNetworkNadOnClusterNetwork(vc.Spec.ClusterNetwork) + if err != nil { + return err + } + + if nad != nil { + return fmt.Errorf("the storage network nad %s is still attached", nad.Name) + } + + return nil +}