From 9024862f05958b218090976c853a8cc839ad2b7e Mon Sep 17 00:00:00 2001
From: Andrea Panattoni <apanatto@redhat.com>
Date: Mon, 8 May 2023 10:43:37 +0200
Subject: [PATCH] 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 <apanatto@redhat.com>
---
 pkg/webhook/validate.go      | 42 ++++++++++++++++++--
 pkg/webhook/validate_test.go | 76 ++++++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+), 4 deletions(-)

diff --git a/pkg/webhook/validate.go b/pkg/webhook/validate.go
index 971d147cde..e6d560c0d5 100644
--- a/pkg/webhook/validate.go
+++ b/pkg/webhook/validate.go
@@ -251,10 +251,27 @@ func validatePolicyForNodePolicy(
 		return true, nil
 	}
 
+	err := validatePfNames(current, previous)
+	if err != nil {
+		return false, err
+	}
+
+	err = validateExludeTopologyField(current, previous)
+	if err != nil {
+		return false, err
+	}
+
+	return true, 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 {
-			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,14 +279,31 @@ 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 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) bool {
diff --git a/pkg/webhook/validate_test.go b/pkg/webhook/validate_test.go
index 906d5e85a6..7db0887db0 100644
--- a/pkg/webhook/validate_test.go
+++ b/pkg/webhook/validate_test.go
@@ -298,6 +298,82 @@ func TestValidatePolicyForNodeStateWithUpdatedExistingVfRange(t *testing.T) {
 	g.Expect(ok).To(Equal(true))
 }
 
+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,
+		},
+	}
+
+	ok, err := validatePolicyForNodePolicy(current, previous)
+
+	g := NewGomegaWithT(t)
+	g.Expect(ok).To(Equal(false))
+	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,
+		},
+	}
+
+	ok, err := validatePolicyForNodePolicy(current, previous)
+
+	g := NewGomegaWithT(t)
+	g.Expect(ok).To(Equal(false))
+	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,
+		},
+	}
+
+	ok, err := validatePolicyForNodePolicy(current, previous)
+
+	g := NewGomegaWithT(t)
+	g.Expect(ok).To(Equal(true))
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
 func TestStaticValidateSriovNetworkNodePolicyWithValidVendorDevice(t *testing.T) {
 	policy := &SriovNetworkNodePolicy{
 		Spec: SriovNetworkNodePolicySpec{