Validate excludeTopology field

Multiple SriovNetworkNodePolicies may target the same resource, but the new field must have the same value. Add a resource validation step to ensure this condition is met. Refactor `Spec.NicSelector.PfNames` check to its own function. Signed-off-by: Andrea Panattoni <[email protected]>
k8snetworkplumbingwg · May 31, 2023 · f55a17e · f55a17e
1 parent 2ce10ea
commit f55a17e
Show file tree

Hide file tree

Showing 2 changed files with 101 additions and 4 deletions.
diff --git a/pkg/webhook/validate.go b/pkg/webhook/validate.go
@@ -249,17 +249,28 @@ func validatePolicyForNodeState(policy *sriovnetworkv1.SriovNetworkNodePolicy, s
 	return nil
 }
 
-func validatePolicyForNodePolicy(
-	current *sriovnetworkv1.SriovNetworkNodePolicy,
-	previous *sriovnetworkv1.SriovNetworkNodePolicy,
-) error {
+func validatePolicyForNodePolicy(current *sriovnetworkv1.SriovNetworkNodePolicy, previous *sriovnetworkv1.SriovNetworkNodePolicy) error {
 	glog.V(2).Infof("validateConflictPolicy(): validate policy %s against policy %s",
 		current.GetName(), previous.GetName())
 
 	if current.GetName() == previous.GetName() {
 		return nil
 	}
 
+	err := validatePfNames(current, previous)
+	if err != nil {
+		return err
+	}
+
+	err = validateExludeTopologyField(current, previous)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func validatePfNames(current *sriovnetworkv1.SriovNetworkNodePolicy, previous *sriovnetworkv1.SriovNetworkNodePolicy) error {
 	for _, curPf := range current.Spec.NicSelector.PfNames {
 		curName, curRngSt, curRngEnd, err := sriovnetworkv1.ParsePFName(curPf)
 		if err != nil {
@@ -281,6 +292,19 @@ func validatePolicyForNodePolicy(
 	return nil
 }
 
+func validateExludeTopologyField(current *sriovnetworkv1.SriovNetworkNodePolicy, previous *sriovnetworkv1.SriovNetworkNodePolicy) error {
+	if current.Spec.ResourceName != previous.Spec.ResourceName {
+		return nil
+	}
+
+	if current.Spec.ExcludeTopology == previous.Spec.ExcludeTopology {
+		return nil
+	}
+
+	return fmt.Errorf("excludeTopology[%t] field conflicts with policy [%s].ExcludeTopology[%t] as they target the same resource[%s]",
+		current.Spec.ExcludeTopology, previous.GetName(), previous.Spec.ExcludeTopology, current.Spec.ResourceName)
+}
+
 func validateNicModel(selector *sriovnetworkv1.SriovNetworkNicSelector, iface *sriovnetworkv1.InterfaceExt, node *corev1.Node) error {
 	if selector.Vendor != "" && selector.Vendor != iface.Vendor {
 		return fmt.Errorf("selector vendor: %s is not equal to the interface vendor: %s", selector.Vendor, iface.Vendor)

diff --git a/pkg/webhook/validate_test.go b/pkg/webhook/validate_test.go
@@ -294,6 +294,79 @@ func TestValidatePolicyForNodeStateWithUpdatedExistingVfRange(t *testing.T) {
 	g.Expect(err).NotTo(HaveOccurred())
 }
 
+func TestValidatePoliciesWithDifferentExcludeTopologyForTheSameResource(t *testing.T) {
+	current := &SriovNetworkNodePolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "currentPolicy"},
+		Spec: SriovNetworkNodePolicySpec{
+			ResourceName:    "resourceX",
+			ExcludeTopology: true,
+		},
+	}
+
+	previous := &SriovNetworkNodePolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "previousPolicy"},
+		Spec: SriovNetworkNodePolicySpec{
+			ResourceName:    "resourceX",
+			ExcludeTopology: false,
+		},
+	}
+
+	err := validatePolicyForNodePolicy(current, previous)
+
+	g := NewGomegaWithT(t)
+	g.Expect(err).To(MatchError("excludeTopology[true] field conflicts with policy [previousPolicy].ExcludeTopology[false] as they target the same resource[resourceX]"))
+}
+
+func TestValidatePoliciesWithDifferentExcludeTopologyForTheSameResourceAndTheSamePF(t *testing.T) {
+	current := &SriovNetworkNodePolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "currentPolicy"},
+		Spec: SriovNetworkNodePolicySpec{
+			ResourceName:    "resourceX",
+			NumVfs:          10,
+			NicSelector:     SriovNetworkNicSelector{PfNames: []string{"eno1#0-4"}},
+			ExcludeTopology: true,
+		},
+	}
+
+	previous := &SriovNetworkNodePolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "previousPolicy"},
+		Spec: SriovNetworkNodePolicySpec{
+			ResourceName:    "resourceX",
+			NumVfs:          10,
+			NicSelector:     SriovNetworkNicSelector{PfNames: []string{"eno1#5-9"}},
+			ExcludeTopology: false,
+		},
+	}
+
+	err := validatePolicyForNodePolicy(current, previous)
+
+	g := NewGomegaWithT(t)
+	g.Expect(err).To(MatchError("excludeTopology[true] field conflicts with policy [previousPolicy].ExcludeTopology[false] as they target the same resource[resourceX]"))
+}
+
+func TestValidatePoliciesWithSameExcludeTopologyForTheSameResource(t *testing.T) {
+	current := &SriovNetworkNodePolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "currentPolicy"},
+		Spec: SriovNetworkNodePolicySpec{
+			ResourceName:    "resourceX",
+			ExcludeTopology: true,
+		},
+	}
+
+	previous := &SriovNetworkNodePolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "previousPolicy"},
+		Spec: SriovNetworkNodePolicySpec{
+			ResourceName:    "resourceX",
+			ExcludeTopology: true,
+		},
+	}
+
+	err := validatePolicyForNodePolicy(current, previous)
+
+	g := NewGomegaWithT(t)
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
 func TestStaticValidateSriovNetworkNodePolicyWithValidVendorDevice(t *testing.T) {
 	policy := &SriovNetworkNodePolicy{
 		Spec: SriovNetworkNodePolicySpec{