Skip to content

Commit

Permalink
General cleanup in validate.go
Browse files Browse the repository at this point in the history 
We use more expressive return types and also reduce cyclomatic complexity.
  • Loading branch information
@vrindle
vrindle committed Apr 27, 2023
1 parent 3588c88 commit 9af322f
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 72 deletions.
89 changes: 45 additions & 44 deletions pkg/webhook/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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(),
Expand All @@ -189,98 +185,103 @@ 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
}
}
}

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
// 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 +294,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")
}
38 changes: 10 additions & 28 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,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{
Expand All @@ -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{
Expand All @@ -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) {
Expand All @@ -598,7 +586,6 @@ func TestStaticValidateSriovNetworkNodePolicyWithInvalidNicSelector(t *testing.T
}

func TestValidatePolicyForNodeStateWithValidNetFilter(t *testing.T) {
interfaceSelected = false
state := newNodeState()
policy := &SriovNetworkNodePolicy{
Spec: SriovNetworkNodePolicySpec{
Expand All @@ -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{
Expand Down Expand Up @@ -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))
}

0 comments on commit 9af322f

Please sign in to comment.