From ce840cae86e1951752f9780b9fe42eb4e15d6b5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=A9=AC=E6=B4=AA=E8=B4=9E?= Date: Thu, 28 Nov 2024 14:58:40 +0800 Subject: [PATCH 1/3] add anp/banp unit test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 马洪贞 --- pkg/controller/admin_network_policy_test.go | 144 ++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 pkg/controller/admin_network_policy_test.go diff --git a/pkg/controller/admin_network_policy_test.go b/pkg/controller/admin_network_policy_test.go new file mode 100644 index 00000000000..7e14f5102c4 --- /dev/null +++ b/pkg/controller/admin_network_policy_test.go @@ -0,0 +1,144 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/kubeovn/kube-ovn/pkg/util" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1alpha1 "sigs.k8s.io/network-policy-api/apis/v1alpha1" +) + +func TestValidateAnpConfig(t *testing.T) { + t.Parallel() + + fakeController := newFakeController(t) + ctrl := fakeController.fakeController + + anp := &v1alpha1.AdminNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "anp-prio", + }, + Spec: v1alpha1.AdminNetworkPolicySpec{ + Priority: 100, + Subject: v1alpha1.AdminNetworkPolicySubject{Namespaces: &metav1.LabelSelector{}}, + }, + } + + if ctrl.anpPrioNameMap == nil { + ctrl.anpPrioNameMap = make(map[int32]string, 1) + } + ctrl.anpPrioNameMap[anp.Spec.Priority] = anp.Name + "test" + t.Run("check same priority conflict", func(t *testing.T) { + err := ctrl.validateAnpConfig(anp) + require.ErrorContains(t, err, "can not create anp with same priority") + }) + + delete(ctrl.anpPrioNameMap, anp.Spec.Priority) + t.Run("check priority out of range", func(t *testing.T) { + err := ctrl.validateAnpConfig(anp) + require.ErrorContains(t, err, "is greater than max value") + }) +} + +func TestFetchCIDRAddrs(t *testing.T) { + t.Parallel() + + networks := []v1alpha1.CIDR{"10.10.10.0/24", "fd00:10:96::/112"} + t.Run("check cidr address", func(t *testing.T) { + v4Addresses, v6Addresses := fetchCIDRAddrs(networks) + require.EqualValues(t, v4Addresses, []string{"10.10.10.0/24"}) + require.EqualValues(t, v6Addresses, []string{"fd00:10:96::/112"}) + }) +} + +func TestIsRulesArrayEmpty(t *testing.T) { + t.Parallel() + + ruleNames := [util.AnpMaxRules]ChangedName{{isMatch: true}} + t.Run("check rules array empty", func(t *testing.T) { + isEmpty := isRulesArrayEmpty(ruleNames) + require.False(t, isEmpty) + }) +} + +func TestGetAnpName(t *testing.T) { + t.Parallel() + + anpName := "test" + t.Run("check anp name", func(t *testing.T) { + checkName := getAnpName(anpName) + require.EqualValues(t, checkName, anpName) + }) + + anpName = "123" + t.Run("check anp name start with digital", func(t *testing.T) { + checkName := getAnpName(anpName) + require.EqualValues(t, checkName, "anp"+anpName) + }) +} + +func TestGetAnpAddressSetName(t *testing.T) { + t.Parallel() + + pgName := "portgroup" + ruleName := "rule" + index := 1 + inAsV4Name := "portgroup.ingress.1.rule.IPv4" + inAsV6Name := "portgroup.ingress.1.rule.IPv6" + outAsV4Name := "portgroup.egress.1.rule.IPv4" + outAsV6Name := "portgroup.egress.1.rule.IPv6" + + t.Run("check ingress address_set name", func(t *testing.T) { + asV4Name, asV6Name := getAnpAddressSetName(pgName, ruleName, index, true) + require.EqualValues(t, asV4Name, inAsV4Name) + require.EqualValues(t, asV6Name, inAsV6Name) + }) + + t.Run("check egress address_set name", func(t *testing.T) { + asV4Name, asV6Name := getAnpAddressSetName(pgName, ruleName, index, false) + require.EqualValues(t, asV4Name, outAsV4Name) + require.EqualValues(t, asV6Name, outAsV6Name) + }) +} + +func TestIsLabelsMatch(t *testing.T) { + t.Parallel() + + nsLabelMap := make(map[string]string, 1) + nsLabelMap["nsName"] = "test-ns" + nsCmpLabelMap := make(map[string]string, 1) + nsCmpLabelMap["nsName"] = "test-ns-cmp" + nsLabels := metav1.LabelSelector{MatchLabels: nsLabelMap} + + t.Run("check namespace label match", func(t *testing.T) { + isMatch := isLabelsMatch(&nsLabels, nil, nsLabelMap, nil) + require.True(t, isMatch) + + isMatch = isLabelsMatch(&nsLabels, nil, nsCmpLabelMap, nil) + require.False(t, isMatch) + }) + + podLabelMap := make(map[string]string, 1) + podLabelMap["podName"] = "test-pod" + podCmpLabelMap := make(map[string]string, 1) + podCmpLabelMap["podName"] = "test-pod-cmp" + podLabels := metav1.LabelSelector{MatchLabels: podLabelMap} + nsPod := v1alpha1.NamespacedPod{NamespaceSelector: nsLabels, PodSelector: podLabels} + + t.Run("check pod label match", func(t *testing.T) { + isMatch := isLabelsMatch(nil, &nsPod, nsLabelMap, podLabelMap) + require.True(t, isMatch) + + isMatch = isLabelsMatch(nil, &nsPod, nsCmpLabelMap, podLabelMap) + require.False(t, isMatch) + + isMatch = isLabelsMatch(nil, &nsPod, nsLabelMap, podCmpLabelMap) + require.False(t, isMatch) + + isMatch = isLabelsMatch(nil, &nsPod, nsCmpLabelMap, podCmpLabelMap) + require.False(t, isMatch) + }) +} From 0ae880e3b6d9d3b3bf39cc1dd9feae0add441453 Mon Sep 17 00:00:00 2001 From: zhangzujian Date: Thu, 12 Dec 2024 08:38:33 +0000 Subject: [PATCH 2/3] update unittests Signed-off-by: zhangzujian --- pkg/controller/admin_network_policy.go | 32 +-- pkg/controller/admin_network_policy_test.go | 258 ++++++++++++++---- .../baseline_admin_network_policy.go | 15 +- .../baseline_admin_network_policy_test.go | 44 +++ 4 files changed, 276 insertions(+), 73 deletions(-) create mode 100644 pkg/controller/baseline_admin_network_policy_test.go diff --git a/pkg/controller/admin_network_policy.go b/pkg/controller/admin_network_policy.go index aaa9814f068..f9cc7fec543 100644 --- a/pkg/controller/admin_network_policy.go +++ b/pkg/controller/admin_network_policy.go @@ -232,7 +232,7 @@ func (c *Controller) handleAddAnp(key string) (err error) { } aclPriority := util.AnpACLMaxPriority - int(anp.Spec.Priority*100) - index - aclAction := convertAction(anpr.Action, "") + aclAction := anpACLAction(anpr.Action) rulePorts := []v1alpha1.AdminNetworkPolicyPort{} if anpr.Ports != nil { rulePorts = *anpr.Ports @@ -299,7 +299,7 @@ func (c *Controller) handleAddAnp(key string) (err error) { } aclPriority := util.AnpACLMaxPriority - int(anp.Spec.Priority*100) - index - aclAction := convertAction(anpr.Action, "") + aclAction := anpACLAction(anpr.Action) rulePorts := []v1alpha1.AdminNetworkPolicyPort{} if anpr.Ports != nil { rulePorts = *anpr.Ports @@ -947,35 +947,26 @@ func getAnpAddressSetName(pgName, ruleName string, index int, isIngress bool) (s return asV4Name, asV6Name } -func convertAction(anpRuleAction v1alpha1.AdminNetworkPolicyRuleAction, banpRuleAction v1alpha1.BaselineAdminNetworkPolicyRuleAction) (aclAction ovnnb.ACLAction) { - switch anpRuleAction { +func anpACLAction(action v1alpha1.AdminNetworkPolicyRuleAction) ovnnb.ACLAction { + switch action { case v1alpha1.AdminNetworkPolicyRuleActionAllow: - aclAction = ovnnb.ACLActionAllowRelated + return ovnnb.ACLActionAllowRelated case v1alpha1.AdminNetworkPolicyRuleActionDeny: - aclAction = ovnnb.ACLActionDrop + return ovnnb.ACLActionDrop case v1alpha1.AdminNetworkPolicyRuleActionPass: - aclAction = ovnnb.ACLActionPass + return ovnnb.ACLActionPass } - - switch banpRuleAction { - case v1alpha1.BaselineAdminNetworkPolicyRuleActionAllow: - aclAction = ovnnb.ACLActionAllowRelated - case v1alpha1.BaselineAdminNetworkPolicyRuleActionDeny: - aclAction = ovnnb.ACLActionDrop - } - return + return ovnnb.ACLActionDrop } func isRulesArrayEmpty(ruleNames [util.AnpMaxRules]ChangedName) bool { - isEmpty := true for _, ruleName := range ruleNames { // The ruleName can be omitted default if ruleName.curRuleName != "" || ruleName.isMatch { - isEmpty = false - break + return false } } - return isEmpty + return true } func (c *Controller) fetchNodesAddrs(nodeSelector labels.Selector) ([]string, []string, error) { @@ -1002,8 +993,7 @@ func (c *Controller) fetchNodesAddrs(nodeSelector labels.Selector) ([]string, [] } func fetchCIDRAddrs(networks []v1alpha1.CIDR) ([]string, []string) { - v4Addresses := make([]string, 0, len(networks)) - v6Addresses := make([]string, 0, len(networks)) + var v4Addresses, v6Addresses []string for _, network := range networks { if _, _, err := net.ParseCIDR(string(network)); err != nil { diff --git a/pkg/controller/admin_network_policy_test.go b/pkg/controller/admin_network_policy_test.go index 7e14f5102c4..342c5d47c2d 100644 --- a/pkg/controller/admin_network_policy_test.go +++ b/pkg/controller/admin_network_policy_test.go @@ -3,12 +3,13 @@ package controller import ( "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1alpha1 "sigs.k8s.io/network-policy-api/apis/v1alpha1" + "github.com/stretchr/testify/require" + "github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb" "github.com/kubeovn/kube-ovn/pkg/util" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - v1alpha1 "sigs.k8s.io/network-policy-api/apis/v1alpha1" ) func TestValidateAnpConfig(t *testing.T) { @@ -22,22 +23,24 @@ func TestValidateAnpConfig(t *testing.T) { Name: "anp-prio", }, Spec: v1alpha1.AdminNetworkPolicySpec{ - Priority: 100, + Priority: util.AnpMaxPriority, Subject: v1alpha1.AdminNetworkPolicySubject{Namespaces: &metav1.LabelSelector{}}, }, } - if ctrl.anpPrioNameMap == nil { - ctrl.anpPrioNameMap = make(map[int32]string, 1) - } - ctrl.anpPrioNameMap[anp.Spec.Priority] = anp.Name + "test" - t.Run("check same priority conflict", func(t *testing.T) { + t.Run("normal", func(t *testing.T) { + err := ctrl.validateAnpConfig(anp) + require.NoError(t, err) + }) + + t.Run("conflict priority", func(t *testing.T) { + ctrl.anpPrioNameMap = map[int32]string{anp.Spec.Priority: anp.Name + "-conflict"} err := ctrl.validateAnpConfig(anp) require.ErrorContains(t, err, "can not create anp with same priority") }) - delete(ctrl.anpPrioNameMap, anp.Spec.Priority) - t.Run("check priority out of range", func(t *testing.T) { + t.Run("priority out of range", func(t *testing.T) { + anp.Spec.Priority = util.AnpMaxPriority + 1 err := ctrl.validateAnpConfig(anp) require.ErrorContains(t, err, "is greater than max value") }) @@ -46,62 +49,179 @@ func TestValidateAnpConfig(t *testing.T) { func TestFetchCIDRAddrs(t *testing.T) { t.Parallel() - networks := []v1alpha1.CIDR{"10.10.10.0/24", "fd00:10:96::/112"} - t.Run("check cidr address", func(t *testing.T) { - v4Addresses, v6Addresses := fetchCIDRAddrs(networks) - require.EqualValues(t, v4Addresses, []string{"10.10.10.0/24"}) - require.EqualValues(t, v6Addresses, []string{"fd00:10:96::/112"}) - }) + tests := []struct { + name string + networks []v1alpha1.CIDR + v4Addrs []string + v6Addrs []string + }{ + { + name: "nil", + }, { + name: "empty", + networks: []v1alpha1.CIDR{}, + }, { + name: "ipv4 only", + networks: []v1alpha1.CIDR{"1.1.1.0/24"}, + v4Addrs: []string{"1.1.1.0/24"}, + }, + { + name: "ipv6 only", + networks: []v1alpha1.CIDR{"fd00::/64"}, + v6Addrs: []string{"fd00::/64"}, + }, + { + name: "mixed", + networks: []v1alpha1.CIDR{"1.1.1.0/24", "fd00::/64"}, + v4Addrs: []string{"1.1.1.0/24"}, + v6Addrs: []string{"fd00::/64"}, + }, + { + name: "invalid ipv4 cidr", + networks: []v1alpha1.CIDR{"1.1.1.0/33", "fd00::/64"}, + v6Addrs: []string{"fd00::/64"}, + }, + { + name: "invalid ipv6 cidr", + networks: []v1alpha1.CIDR{"1.1.1.0/24", "fd00::/129"}, + v4Addrs: []string{"1.1.1.0/24"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v4Addrs, v6Addrs := fetchCIDRAddrs(tt.networks) + require.Equal(t, tt.v4Addrs, v4Addrs) + require.Equal(t, tt.v6Addrs, v6Addrs) + }) + } } func TestIsRulesArrayEmpty(t *testing.T) { t.Parallel() - ruleNames := [util.AnpMaxRules]ChangedName{{isMatch: true}} - t.Run("check rules array empty", func(t *testing.T) { - isEmpty := isRulesArrayEmpty(ruleNames) - require.False(t, isEmpty) - }) + tests := []struct { + name string + arg [util.AnpMaxRules]ChangedName + ret bool + }{ + { + "empty", + [util.AnpMaxRules]ChangedName{}, + true, + }, + { + "rule name", + [util.AnpMaxRules]ChangedName{{curRuleName: "foo"}}, + false, + }, + { + "match", + [util.AnpMaxRules]ChangedName{{isMatch: true}}, + false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ret := isRulesArrayEmpty(tt.arg) + require.Equal(t, tt.ret, ret) + }) + } } func TestGetAnpName(t *testing.T) { t.Parallel() - anpName := "test" - t.Run("check anp name", func(t *testing.T) { - checkName := getAnpName(anpName) - require.EqualValues(t, checkName, anpName) - }) + tests := []struct { + name string + arg string + ret string + }{ + { + "normal", + "foo", + "foo", + }, + { + "start with digital", + "123", + "anp123", + }, + { + "start with hyphen", + "-foo", + "anp-foo", + }, + { + "start with underscore", + "_foo", + "anp_foo", + }, + { + "start with dot", + ".foo", + "anp.foo", + }, + { + "start with slash", + "/foo", + "anp/foo", + }, + { + "start with colon", + ":foo", + "anp:foo", + }, + } - anpName = "123" - t.Run("check anp name start with digital", func(t *testing.T) { - checkName := getAnpName(anpName) - require.EqualValues(t, checkName, "anp"+anpName) - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ret := getAnpName(tt.arg) + require.Equal(t, tt.ret, ret) + }) + } } func TestGetAnpAddressSetName(t *testing.T) { t.Parallel() - pgName := "portgroup" - ruleName := "rule" - index := 1 - inAsV4Name := "portgroup.ingress.1.rule.IPv4" - inAsV6Name := "portgroup.ingress.1.rule.IPv6" - outAsV4Name := "portgroup.egress.1.rule.IPv4" - outAsV6Name := "portgroup.egress.1.rule.IPv6" - - t.Run("check ingress address_set name", func(t *testing.T) { - asV4Name, asV6Name := getAnpAddressSetName(pgName, ruleName, index, true) - require.EqualValues(t, asV4Name, inAsV4Name) - require.EqualValues(t, asV6Name, inAsV6Name) - }) + tests := []struct { + name string + pgName string + ruleName string + index int + isIngress bool + v4Name string + v6Name string + }{ + { + "ingress", + "foo", + "bar", + 1, + true, + "foo.ingress.1.bar.IPv4", + "foo.ingress.1.bar.IPv6", + }, + { + "egress", + "bar", + "foo", + 0, + false, + "bar.egress.0.foo.IPv4", + "bar.egress.0.foo.IPv6", + }, + } - t.Run("check egress address_set name", func(t *testing.T) { - asV4Name, asV6Name := getAnpAddressSetName(pgName, ruleName, index, false) - require.EqualValues(t, asV4Name, outAsV4Name) - require.EqualValues(t, asV6Name, outAsV6Name) - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v4Name, v6Name := getAnpAddressSetName(tt.pgName, tt.ruleName, tt.index, tt.isIngress) + require.Equal(t, tt.v4Name, v4Name) + require.Equal(t, tt.v6Name, v6Name) + }) + } } func TestIsLabelsMatch(t *testing.T) { @@ -142,3 +262,41 @@ func TestIsLabelsMatch(t *testing.T) { require.False(t, isMatch) }) } + +func TestAnpACLAction(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + arg v1alpha1.AdminNetworkPolicyRuleAction + ret ovnnb.ACLAction + }{ + { + "allow", + v1alpha1.AdminNetworkPolicyRuleActionAllow, + ovnnb.ACLActionAllowRelated, + }, + { + "pass", + v1alpha1.AdminNetworkPolicyRuleActionPass, + ovnnb.ACLActionPass, + }, + { + "deny", + v1alpha1.AdminNetworkPolicyRuleActionDeny, + ovnnb.ACLActionDrop, + }, + { + "unknown", + "foo", + ovnnb.ACLActionDrop, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ret := anpACLAction(tt.arg) + require.Equal(t, tt.ret, ret) + }) + } +} diff --git a/pkg/controller/baseline_admin_network_policy.go b/pkg/controller/baseline_admin_network_policy.go index 819d7a18872..0ed57182fae 100644 --- a/pkg/controller/baseline_admin_network_policy.go +++ b/pkg/controller/baseline_admin_network_policy.go @@ -13,6 +13,7 @@ import ( v1alpha1 "sigs.k8s.io/network-policy-api/apis/v1alpha1" kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1" + "github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb" "github.com/kubeovn/kube-ovn/pkg/util" ) @@ -108,6 +109,16 @@ func (c *Controller) enqueueUpdateBanp(oldObj, newObj interface{}) { } } +func banpACLAction(action v1alpha1.BaselineAdminNetworkPolicyRuleAction) ovnnb.ACLAction { + switch action { + case v1alpha1.BaselineAdminNetworkPolicyRuleActionAllow: + return ovnnb.ACLActionAllowRelated + case v1alpha1.BaselineAdminNetworkPolicyRuleActionDeny: + return ovnnb.ACLActionDrop + } + return ovnnb.ACLActionDrop +} + func (c *Controller) handleAddBanp(key string) (err error) { // Only one banp with default name can be created in cluster, no need to check c.banpKeyMutex.LockKey(key) @@ -191,7 +202,7 @@ func (c *Controller) handleAddBanp(key string) (err error) { // use 1700-1800 for banp acl priority aclPriority := util.BanpACLMaxPriority - index - aclAction := convertAction("", banpr.Action) + aclAction := banpACLAction(banpr.Action) rulePorts := []v1alpha1.AdminNetworkPolicyPort{} if banpr.Ports != nil { rulePorts = *banpr.Ports @@ -258,7 +269,7 @@ func (c *Controller) handleAddBanp(key string) (err error) { } aclPriority := util.BanpACLMaxPriority - index - aclAction := convertAction("", banpr.Action) + aclAction := banpACLAction(banpr.Action) rulePorts := []v1alpha1.AdminNetworkPolicyPort{} if banpr.Ports != nil { rulePorts = *banpr.Ports diff --git a/pkg/controller/baseline_admin_network_policy_test.go b/pkg/controller/baseline_admin_network_policy_test.go new file mode 100644 index 00000000000..4ce2de21f22 --- /dev/null +++ b/pkg/controller/baseline_admin_network_policy_test.go @@ -0,0 +1,44 @@ +package controller + +import ( + "testing" + + v1alpha1 "sigs.k8s.io/network-policy-api/apis/v1alpha1" + + "github.com/stretchr/testify/require" + + "github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb" +) + +func TestBanpACLAction(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + arg v1alpha1.BaselineAdminNetworkPolicyRuleAction + ret ovnnb.ACLAction + }{ + { + "allow", + v1alpha1.BaselineAdminNetworkPolicyRuleActionAllow, + ovnnb.ACLActionAllowRelated, + }, + { + "deny", + v1alpha1.BaselineAdminNetworkPolicyRuleActionDeny, + ovnnb.ACLActionDrop, + }, + { + "unknown", + "foo", + ovnnb.ACLActionDrop, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ret := banpACLAction(tt.arg) + require.Equal(t, tt.ret, ret) + }) + } +} From 3e2160e6b7b3958c0447ebb78f9678876342ff40 Mon Sep 17 00:00:00 2001 From: zhangzujian Date: Thu, 12 Dec 2024 08:48:55 +0000 Subject: [PATCH 3/3] fix format Signed-off-by: zhangzujian --- pkg/controller/admin_network_policy_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/controller/admin_network_policy_test.go b/pkg/controller/admin_network_policy_test.go index 342c5d47c2d..bc8c3646db7 100644 --- a/pkg/controller/admin_network_policy_test.go +++ b/pkg/controller/admin_network_policy_test.go @@ -57,10 +57,12 @@ func TestFetchCIDRAddrs(t *testing.T) { }{ { name: "nil", - }, { + }, + { name: "empty", networks: []v1alpha1.CIDR{}, - }, { + }, + { name: "ipv4 only", networks: []v1alpha1.CIDR{"1.1.1.0/24"}, v4Addrs: []string{"1.1.1.0/24"},