Skip to content

Commit

Permalink
Merge pull request #422 from vrindle/fix_error_message
Browse files Browse the repository at this point in the history 
General cleanup in validate.go
  • Loading branch information
@adrianchiris
adrianchiris authored May 31, 2023
2 parents 497f285 + bd6efc0 commit 6b81c4e
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 55 deletions.
79 changes: 44 additions & 35 deletions pkg/webhook/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,20 +189,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
}
}
}
Expand All @@ -217,70 +206,90 @@ func dynamicValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePo
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) {
err := validateNicModel(&policy.Spec.NicSelector, &iface, node)
if err == nil {
interfaceSelected = true
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
// since it should already be evaluated in previous run.
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
Expand All @@ -293,23 +302,23 @@ 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
for key := range utils.PlatformMap {
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")
}
30 changes: 10 additions & 20 deletions pkg/webhook/validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -527,9 +522,8 @@ 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) {
Expand All @@ -550,9 +544,8 @@ 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))
}

Expand All @@ -574,9 +567,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))
}

Expand Down Expand Up @@ -615,9 +607,8 @@ 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))
}

Expand Down Expand Up @@ -680,8 +671,7 @@ 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))
}

0 comments on commit 6b81c4e

Please sign in to comment.