diff --git a/pkg/webhook/validate.go b/pkg/webhook/validate.go index 971d147cde..69129c8555 100644 --- a/pkg/webhook/validate.go +++ b/pkg/webhook/validate.go @@ -26,10 +26,7 @@ const ( MlxMaxVFs = 128 ) -var ( - nodesSelected bool - interfaceSelected bool -) +var nodesSelected bool func validateSriovOperatorConfig(cr *sriovnetworkv1.SriovOperatorConfig, operation v1.Operation) (bool, []string, error) { glog.V(2).Infof("validateSriovOperatorConfig: %v", cr) @@ -170,7 +167,6 @@ func staticValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePol func dynamicValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePolicy) (bool, error) { nodesSelected = false - interfaceSelected = false nodeList, err := kubeclient.CoreV1().Nodes().List(context.Background(), metav1.ListOptions{ LabelSelector: labels.Set(cr.Spec.NodeSelector).String(), @@ -189,20 +185,9 @@ func dynamicValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePo for _, node := range nodeList.Items { if cr.Selected(&node) { nodesSelected = true - for _, ns := range nsList.Items { - if ns.GetName() == node.GetName() { - if ok, err := validatePolicyForNodeState(cr, &ns, &node); err != nil || !ok { - return false, err - } - } - } - // validate current policy against policies in API (may not be converted to SriovNetworkNodeState yet) - for _, np := range npList.Items { - if np.GetName() != cr.GetName() && np.Selected(&node) { - if ok, err := validatePolicyForNodePolicy(cr, &np); err != nil || !ok { - return false, err - } - } + err = validatePolicyForNodeStateAndPolicy(nsList, npList, &node, cr) + if err != nil { + return false, err } } } @@ -210,51 +195,67 @@ func dynamicValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePo if !nodesSelected { return false, fmt.Errorf("no matched node is selected by the nodeSelector in CR %s", cr.GetName()) } - if !interfaceSelected { - return false, fmt.Errorf("no supported NIC is selected by the nicSelector in CR %s", cr.GetName()) - } return true, nil } -func validatePolicyForNodeState(policy *sriovnetworkv1.SriovNetworkNodePolicy, state *sriovnetworkv1.SriovNetworkNodeState, node *corev1.Node) (bool, error) { +func validatePolicyForNodeStateAndPolicy(nsList *sriovnetworkv1.SriovNetworkNodeStateList, npList *sriovnetworkv1.SriovNetworkNodePolicyList, node *corev1.Node, cr *sriovnetworkv1.SriovNetworkNodePolicy) error { + for _, ns := range nsList.Items { + if ns.GetName() == node.GetName() { + if err := validatePolicyForNodeState(cr, &ns, node); err != nil { + return err + } + } + } + // validate current policy against policies in API (may not be converted to SriovNetworkNodeState yet) + for _, np := range npList.Items { + if np.GetName() != cr.GetName() && np.Selected(node) { + if err := validatePolicyForNodePolicy(cr, &np); err != nil { + return err + } + } + } + return nil +} + +func validatePolicyForNodeState(policy *sriovnetworkv1.SriovNetworkNodePolicy, state *sriovnetworkv1.SriovNetworkNodeState, node *corev1.Node) error { glog.V(2).Infof("validatePolicyForNodeState(): validate policy %s for node %s.", policy.GetName(), state.GetName()) for _, iface := range state.Status.Interfaces { - if validateNicModel(&policy.Spec.NicSelector, &iface, node) { - interfaceSelected = true + err := validateNicModel(&policy.Spec.NicSelector, &iface, node) + if err == nil { if policy.GetName() != constants.DefaultPolicyName && policy.Spec.NumVfs == 0 { - return false, fmt.Errorf("numVfs(%d) in CR %s is not allowed", policy.Spec.NumVfs, policy.GetName()) + return fmt.Errorf("numVfs(%d) in CR %s is not allowed", policy.Spec.NumVfs, policy.GetName()) } if policy.Spec.NumVfs > iface.TotalVfs && iface.Vendor == IntelID { - return false, fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), iface.TotalVfs) + return fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), iface.TotalVfs) } if policy.Spec.NumVfs > MlxMaxVFs && iface.Vendor == MellanoxID { - return false, fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), MlxMaxVFs) + return fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), MlxMaxVFs) } // vdpa: only mellanox cards are supported if policy.Spec.VdpaType == constants.VdpaTypeVirtio && iface.Vendor != MellanoxID { - return false, fmt.Errorf("vendor(%s) in CR %s not supported for virtio-vdpa", iface.Vendor, policy.GetName()) + return fmt.Errorf("vendor(%s) in CR %s not supported for virtio-vdpa", iface.Vendor, policy.GetName()) } } } - return true, nil + return nil } func validatePolicyForNodePolicy( current *sriovnetworkv1.SriovNetworkNodePolicy, previous *sriovnetworkv1.SriovNetworkNodePolicy, -) (bool, error) { +) error { glog.V(2).Infof("validateConflictPolicy(): validate policy %s against policy %s", current.GetName(), previous.GetName()) if current.GetName() == previous.GetName() { - return true, nil + return nil } for _, curPf := range current.Spec.NicSelector.PfNames { curName, curRngSt, curRngEnd, err := sriovnetworkv1.ParsePFName(curPf) if err != nil { - return false, fmt.Errorf("invalid PF name: %s", curPf) + return fmt.Errorf("invalid PF name: %s", curPf) } for _, prePf := range previous.Spec.NicSelector.PfNames { // Not validate return err of ParsePFName for previous PF @@ -262,25 +263,25 @@ func validatePolicyForNodePolicy( preName, preRngSt, preRngEnd, _ := sriovnetworkv1.ParsePFName(prePf) if curName == preName { if curRngEnd < preRngSt || curRngSt > preRngEnd { - return true, nil + return nil } else { - return false, fmt.Errorf("VF index range in %s is overlapped with existing policy %s", curPf, previous.GetName()) + return fmt.Errorf("VF index range in %s is overlapped with existing policy %s", curPf, previous.GetName()) } } } } - return true, nil + return nil } -func validateNicModel(selector *sriovnetworkv1.SriovNetworkNicSelector, iface *sriovnetworkv1.InterfaceExt, node *corev1.Node) bool { +func validateNicModel(selector *sriovnetworkv1.SriovNetworkNicSelector, iface *sriovnetworkv1.InterfaceExt, node *corev1.Node) error { if selector.Vendor != "" && selector.Vendor != iface.Vendor { - return false + return fmt.Errorf("selector vendor: %s is not equal to the interface vendor: %s", selector.Vendor, iface.Vendor) } if selector.DeviceID != "" && selector.DeviceID != iface.DeviceID { - return false + return fmt.Errorf("selector device ID: %s is not equal to the interface device ID: %s", selector.Vendor, iface.Vendor) } if len(selector.RootDevices) > 0 && !sriovnetworkv1.StringInArray(iface.PciAddress, selector.RootDevices) { - return false + return fmt.Errorf("interface PCI address: %s not found in root devices", iface.PciAddress) } if len(selector.PfNames) > 0 { var pfNames []string @@ -293,13 +294,13 @@ func validateNicModel(selector *sriovnetworkv1.SriovNetworkNicSelector, iface *s } } if !sriovnetworkv1.StringInArray(iface.Name, pfNames) { - return false + return fmt.Errorf("interface name: %s not found in physical function names", iface.PciAddress) } } // check the vendor/device ID to make sure only devices in supported list are allowed. if sriovnetworkv1.IsSupportedModel(iface.Vendor, iface.DeviceID) { - return true + return nil } // Check the vendor and device ID of the VF only if we are on a virtual environment @@ -307,9 +308,9 @@ func validateNicModel(selector *sriovnetworkv1.SriovNetworkNicSelector, iface *s if strings.Contains(strings.ToLower(node.Spec.ProviderID), strings.ToLower(key)) && selector.NetFilter != "" && selector.NetFilter == iface.NetFilter && sriovnetworkv1.IsVfSupportedModel(iface.Vendor, iface.DeviceID) { - return true + return nil } } - return false + return fmt.Errorf("vendor and device ID is not in supported list") } diff --git a/pkg/webhook/validate_test.go b/pkg/webhook/validate_test.go index 906d5e85a6..3199364abb 100644 --- a/pkg/webhook/validate_test.go +++ b/pkg/webhook/validate_test.go @@ -213,9 +213,8 @@ func TestValidatePolicyForNodeStateWithValidPolicy(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodeState(policy, state, NewNode()) + err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(Equal(true)) } func TestValidatePolicyForNodeStateWithInvalidNumVfsPolicy(t *testing.T) { @@ -240,9 +239,8 @@ func TestValidatePolicyForNodeStateWithInvalidNumVfsPolicy(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodeState(policy, state, NewNode()) + err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), state.Status.Interfaces[0].TotalVfs)))) - g.Expect(ok).To(Equal(false)) } func TestValidatePolicyForNodePolicyWithOverlappedVfRange(t *testing.T) { @@ -266,9 +264,8 @@ func TestValidatePolicyForNodePolicyWithOverlappedVfRange(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodePolicy(policy, appliedPolicy) + err := validatePolicyForNodePolicy(policy, appliedPolicy) g.Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("VF index range in %s is overlapped with existing policy %s", policy.Spec.NicSelector.PfNames[0], appliedPolicy.ObjectMeta.Name)))) - g.Expect(ok).To(Equal(false)) } func TestValidatePolicyForNodeStateWithUpdatedExistingVfRange(t *testing.T) { @@ -293,9 +290,8 @@ func TestValidatePolicyForNodeStateWithUpdatedExistingVfRange(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodePolicy(policy, appliedPolicy) + err := validatePolicyForNodePolicy(policy, appliedPolicy) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(Equal(true)) } func TestStaticValidateSriovNetworkNodePolicyWithValidVendorDevice(t *testing.T) { @@ -499,9 +495,8 @@ func TestValidatePolicyForNodeStateVdpaWithNotSupportedVendor(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodeState(policy, state, NewNode()) + err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("vendor(%s) in CR %s not supported for virtio-vdpa", state.Status.Interfaces[0].Vendor, policy.Name)))) - g.Expect(ok).To(Equal(false)) } func TestValidatePolicyForNodeStateWithInvalidDevice(t *testing.T) { @@ -527,13 +522,11 @@ func TestValidatePolicyForNodeStateWithInvalidDevice(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(cfg).ToNot(BeNil()) kubeclient = kubernetes.NewForConfigOrDie(cfg) - ok, err := validatePolicyForNodeState(policy, state, NewNode()) + err = validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(Equal(true)) } func TestValidatePolicyForNodeStateWithInvalidPfName(t *testing.T) { - interfaceSelected = false state := newNodeState() policy := &SriovNetworkNodePolicy{ Spec: SriovNetworkNodePolicySpec{ @@ -550,14 +543,11 @@ func TestValidatePolicyForNodeStateWithInvalidPfName(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodeState(policy, state, NewNode()) + err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(Equal(true)) - g.Expect(interfaceSelected).To(Equal(false)) } func TestValidatePolicyForNodeStateWithValidPfName(t *testing.T) { - interfaceSelected = false state := newNodeState() policy := &SriovNetworkNodePolicy{ Spec: SriovNetworkNodePolicySpec{ @@ -574,10 +564,8 @@ func TestValidatePolicyForNodeStateWithValidPfName(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodeState(policy, state, NewNode()) + err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(Equal(true)) - g.Expect(interfaceSelected).To(Equal(true)) } func TestStaticValidateSriovNetworkNodePolicyWithInvalidNicSelector(t *testing.T) { @@ -598,7 +586,6 @@ func TestStaticValidateSriovNetworkNodePolicyWithInvalidNicSelector(t *testing.T } func TestValidatePolicyForNodeStateWithValidNetFilter(t *testing.T) { - interfaceSelected = false state := newNodeState() policy := &SriovNetworkNodePolicy{ Spec: SriovNetworkNodePolicySpec{ @@ -615,14 +602,11 @@ func TestValidatePolicyForNodeStateWithValidNetFilter(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodeState(policy, state, NewNode()) + err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(Equal(true)) - g.Expect(interfaceSelected).To(Equal(true)) } func TestValidatePolicyForNodeStateWithValidVFAndNetFilter(t *testing.T) { - interfaceSelected = false state := &SriovNetworkNodeState{ Spec: SriovNetworkNodeStateSpec{ Interfaces: []Interface{ @@ -680,8 +664,6 @@ func TestValidatePolicyForNodeStateWithValidVFAndNetFilter(t *testing.T) { }, } g := NewGomegaWithT(t) - ok, err := validatePolicyForNodeState(policy, state, NewNode()) + err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(Equal(true)) - g.Expect(interfaceSelected).To(Equal(true)) }