drop both IPv4 and IPv6 traffic in networkpolicy drop acl (#3940)

* drop both IPv4 and IPv6 traffic in networkpolicy drop acl Signed-off-by: 马洪贞 <[email protected]> * exec mockgen cmd Signed-off-by: 马洪贞 <[email protected]> --------- Signed-off-by: 马洪贞 <[email protected]>
kubeovn · Apr 22, 2024 · 6a3cdec · 6a3cdec
1 parent 4ff20a5
commit 6a3cdec
Show file tree

Hide file tree

Showing 10 changed files with 370 additions and 389 deletions.
diff --git a/go.mod b/go.mod
@@ -35,6 +35,7 @@ require (
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.8.4
 	github.com/vishvananda/netlink v1.2.1-beta.2
+	go.uber.org/mock v0.4.0
 	golang.org/x/exp v0.0.0-20231006140011-7918f672742d
 	golang.org/x/mod v0.17.0
 	golang.org/x/sys v0.19.0

diff --git a/go.sum b/go.sum
@@ -1903,6 +1903,8 @@ go.uber.org/goleak v1.2.0/go.mod h1:XJYK+MuIchqpmGmUSAzotztawfKvYLUIgg7guXrwVUo=
 go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4=
 go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
 go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
+go.uber.org/mock v0.4.0 h1:VcM4ZOtdbR4f6VXfiOpwpVJDL6lCReaZ6mw31wqh7KU=
+go.uber.org/mock v0.4.0/go.mod h1:a6FSlNadKUHUa9IP5Vyt1zh4fC7uAwxMutEAscFbkZc=
 go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
 go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
 go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0=

diff --git a/mocks/pkg/ovs/interface.go b/mocks/pkg/ovs/interface.go
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
@@ -3,8 +3,8 @@ package controller
 import (
 	"testing"
 
-	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 
 	"k8s.io/client-go/informers"
 	coreinformers "k8s.io/client-go/informers/core/v1"

diff --git a/pkg/controller/network_policy.go b/pkg/controller/network_policy.go
@@ -337,17 +337,16 @@ func (c *Controller) handleUpdateNp(key string) error {
 
 					ingressACLOps = append(ingressACLOps, ops...)
 				}
-
-				if err = c.OVNNbClient.Transact("add-ingress-acls", ingressACLOps); err != nil {
-					return fmt.Errorf("add ingress acls to %s: %v", pgName, err)
-				}
-
-				if err = c.OVNNbClient.SetACLLog(pgName, protocol, logEnable, true); err != nil {
-					// just log and do not return err here
-					klog.Errorf("failed to set ingress acl log for np %s, %v", key, err)
-				}
 			}
 		}
+		if err := c.OVNNbClient.Transact("add-ingress-acls", ingressACLOps); err != nil {
+			return fmt.Errorf("add ingress acls to %s: %v", pgName, err)
+		}
+
+		if err := c.OVNNbClient.SetACLLog(pgName, logEnable, true); err != nil {
+			// just log and do not return err here
+			klog.Errorf("failed to set ingress acl log for np %s, %v", key, err)
+		}
 
 		ass, err := c.OVNNbClient.ListAddressSets(map[string]string{
 			networkPolicyKey: fmt.Sprintf("%s/%s/%s", np.Namespace, npName, "ingress"),
@@ -491,16 +490,16 @@ func (c *Controller) handleUpdateNp(key string) error {
 					egressACLOps = append(egressACLOps, ops...)
 				}
 
-				if err = c.OVNNbClient.Transact("add-egress-acls", egressACLOps); err != nil {
-					return fmt.Errorf("add egress acls to %s: %v", pgName, err)
-				}
-
-				if err = c.OVNNbClient.SetACLLog(pgName, protocol, logEnable, false); err != nil {
-					// just log and do not return err here
-					klog.Errorf("failed to set egress acl log for np %s, %v", key, err)
-				}
 			}
 		}
+		if err := c.OVNNbClient.Transact("add-egress-acls", egressACLOps); err != nil {
+			return fmt.Errorf("add egress acls to %s: %v", pgName, err)
+		}
+
+		if err := c.OVNNbClient.SetACLLog(pgName, logEnable, false); err != nil {
+			// just log and do not return err here
+			klog.Errorf("failed to set egress acl log for np %s, %v", key, err)
+		}
 
 		ass, err := c.OVNNbClient.ListAddressSets(map[string]string{
 			networkPolicyKey: fmt.Sprintf("%s/%s/%s", np.Namespace, npName, "egress"),

diff --git a/pkg/controller/security_group_test.go b/pkg/controller/security_group_test.go
@@ -3,8 +3,8 @@ package controller
 import (
 	"testing"
 
-	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 
 	"github.com/kubeovn/kube-ovn/pkg/ovs"
 	"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb"

diff --git a/pkg/controller/subnet_test.go b/pkg/controller/subnet_test.go
@@ -4,8 +4,8 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 

diff --git a/pkg/ovs/interface.go b/pkg/ovs/interface.go
@@ -124,7 +124,7 @@ type ACL interface {
 	CreateSgBaseACL(sgName, direction string) error
 	UpdateSgACL(sg *kubeovnv1.SecurityGroup, direction string) error
 	UpdateLogicalSwitchACL(lsName string, subnetAcls []kubeovnv1.ACL) error
-	SetACLLog(pgName, protocol string, logEnable, isIngress bool) error
+	SetACLLog(pgName string, logEnable, isIngress bool) error
 	SetLogicalSwitchPrivate(lsName, cidrBlock, nodeSwitchCIDR string, allowSubnets []string) error
 	SGLostACL(sg *kubeovnv1.SecurityGroup) (bool, error)
 	DeleteAcls(parentName, parentType, direction string, externalIDs map[string]string) error

diff --git a/pkg/ovs/ovn-nb-acl.go b/pkg/ovs/ovn-nb-acl.go
@@ -24,15 +24,10 @@ func (c *OVNNbClient) UpdateIngressACLOps(pgName, asIngressName, asExceptName, p
 
 	if strings.HasSuffix(asIngressName, ".0") || strings.HasSuffix(asIngressName, ".all") {
 		// create the default drop rule for only once
-		ipSuffix := "ip4"
-		if protocol == kubeovnv1.ProtocolIPv6 {
-			ipSuffix = "ip6"
-		}
-
-		/* default drop acl */
+		// both IPv4 and IPv6 traffic should be forbade in dual-stack situation
 		allIPMatch := NewAndACLMatch(
 			NewACLMatch("outport", "==", "@"+pgName, ""),
-			NewACLMatch(ipSuffix, "", "", ""),
+			NewACLMatch("ip", "", "", ""),
 		)
 		options := func(acl *ovnnb.ACL) {
 			if logEnable {
@@ -74,15 +69,10 @@ func (c *OVNNbClient) UpdateEgressACLOps(pgName, asEgressName, asExceptName, pro
 
 	if strings.HasSuffix(asEgressName, ".0") || strings.HasSuffix(asEgressName, ".all") {
 		// create the default drop rule for only once
-		ipSuffix := "ip4"
-		if protocol == kubeovnv1.ProtocolIPv6 {
-			ipSuffix = "ip6"
-		}
-
-		/* default drop acl */
+		// both IPv4 and IPv6 traffic should be forbade in dual-stack situation
 		allIPMatch := NewAndACLMatch(
 			NewACLMatch("inport", "==", "@"+pgName, ""),
-			NewACLMatch(ipSuffix, "", "", ""),
+			NewACLMatch("ip", "", "", ""),
 		)
 		options := func(acl *ovnnb.ACL) {
 			if logEnable {
@@ -595,23 +585,18 @@ func (c *OVNNbClient) SetLogicalSwitchPrivate(lsName, cidrBlock, nodeSwitchCIDR
 	return nil
 }
 
-func (c *OVNNbClient) SetACLLog(pgName, protocol string, logEnable, isIngress bool) error {
+func (c *OVNNbClient) SetACLLog(pgName string, logEnable, isIngress bool) error {
 	direction := ovnnb.ACLDirectionToLport
 	portDirection := "outport"
 	if !isIngress {
 		direction = ovnnb.ACLDirectionFromLport
 		portDirection = "inport"
 	}
 
-	ipSuffix := "ip4"
-	if protocol == kubeovnv1.ProtocolIPv6 {
-		ipSuffix = "ip6"
-	}
-
 	// match all traffic to or from pgName
 	allIPMatch := NewAndACLMatch(
 		NewACLMatch(portDirection, "==", "@"+pgName, ""),
-		NewACLMatch(ipSuffix, "", "", ""),
+		NewACLMatch("ip", "", "", ""),
 	)
 
 	acl, err := c.GetACL(pgName, direction, util.IngressDefaultDrop, allIPMatch.String(), true)
@@ -624,6 +609,9 @@ func (c *OVNNbClient) SetACLLog(pgName, protocol string, logEnable, isIngress bo
 		return nil // skip if acl not found
 	}
 
+	if acl.Log == logEnable {
+		return nil
+	}
 	acl.Log = logEnable
 
 	err = c.UpdateACL(acl, &acl.Log)

diff --git a/pkg/ovs/ovn-nb-acl_test.go b/pkg/ovs/ovn-nb-acl_test.go
@@ -94,7 +94,7 @@ func (suite *OvnClientTestSuite) testUpdateIngressACLOps() {
 		require.NoError(t, err)
 		require.Len(t, ops, 4)
 
-		expect(ops[0].Row, "drop", ovnnb.ACLDirectionToLport, fmt.Sprintf("outport == @%s && ip4", pgName), util.IngressDefaultDrop)
+		expect(ops[0].Row, "drop", ovnnb.ACLDirectionToLport, fmt.Sprintf("outport == @%s && ip", pgName), util.IngressDefaultDrop)
 
 		matches := newNetworkPolicyACLMatch(pgName, asIngressName, asExceptName, protocol, ovnnb.ACLDirectionToLport, npp, nil)
 		i := 1
@@ -120,7 +120,7 @@ func (suite *OvnClientTestSuite) testUpdateIngressACLOps() {
 		require.NoError(t, err)
 		require.Len(t, ops, 3)
 
-		expect(ops[0].Row, "drop", ovnnb.ACLDirectionToLport, fmt.Sprintf("outport == @%s && ip6", pgName), util.IngressDefaultDrop)
+		expect(ops[0].Row, "drop", ovnnb.ACLDirectionToLport, fmt.Sprintf("outport == @%s && ip", pgName), util.IngressDefaultDrop)
 
 		matches := newNetworkPolicyACLMatch(pgName, asIngressName, asExceptName, protocol, ovnnb.ACLDirectionToLport, nil, nil)
 		i := 1
@@ -164,7 +164,7 @@ func (suite *OvnClientTestSuite) testUpdateEgressACLOps() {
 		require.NoError(t, err)
 		require.Len(t, ops, 4)
 
-		expect(ops[0].Row, "drop", ovnnb.ACLDirectionFromLport, fmt.Sprintf("inport == @%s && ip4", pgName), util.EgressDefaultDrop)
+		expect(ops[0].Row, "drop", ovnnb.ACLDirectionFromLport, fmt.Sprintf("inport == @%s && ip", pgName), util.EgressDefaultDrop)
 
 		matches := newNetworkPolicyACLMatch(pgName, asEgressName, asExceptName, protocol, ovnnb.ACLDirectionFromLport, npp, nil)
 		i := 1
@@ -190,7 +190,7 @@ func (suite *OvnClientTestSuite) testUpdateEgressACLOps() {
 		require.NoError(t, err)
 		require.Len(t, ops, 3)
 
-		expect(ops[0].Row, "drop", ovnnb.ACLDirectionFromLport, fmt.Sprintf("inport == @%s && ip6", pgName), util.EgressDefaultDrop)
+		expect(ops[0].Row, "drop", ovnnb.ACLDirectionFromLport, fmt.Sprintf("inport == @%s && ip", pgName), util.EgressDefaultDrop)
 
 		matches := newNetworkPolicyACLMatch(pgName, asEgressName, asExceptName, protocol, ovnnb.ACLDirectionFromLport, nil, nil)
 		i := 1
@@ -702,7 +702,7 @@ func (suite *OvnClientTestSuite) testSetACLLog() {
 	require.NoError(t, err)
 
 	t.Run("set ingress acl log to false", func(t *testing.T) {
-		match := fmt.Sprintf("outport == @%s && ip4", pgName)
+		match := fmt.Sprintf("outport == @%s && ip", pgName)
 		acl := newACL(pgName, ovnnb.ACLDirectionToLport, util.IngressDefaultDrop, match, ovnnb.ACLActionDrop, func(acl *ovnnb.ACL) {
 			acl.Name = &pgName
 			acl.Log = true
@@ -712,7 +712,7 @@ func (suite *OvnClientTestSuite) testSetACLLog() {
 		err = ovnClient.CreateAcls(pgName, portGroupKey, acl)
 		require.NoError(t, err)
 
-		err = ovnClient.SetACLLog(pgName, kubeovnv1.ProtocolIPv4, false, true)
+		err = ovnClient.SetACLLog(pgName, false, true)
 		require.NoError(t, err)
 
 		acl, err = ovnClient.GetACL(pgName, ovnnb.ACLDirectionToLport, util.IngressDefaultDrop, match, false)
@@ -721,7 +721,7 @@ func (suite *OvnClientTestSuite) testSetACLLog() {
 	})
 
 	t.Run("set egress acl log to false", func(t *testing.T) {
-		match := fmt.Sprintf("inport == @%s && ip4", pgName)
+		match := fmt.Sprintf("inport == @%s && ip", pgName)
 		acl := newACL(pgName, ovnnb.ACLDirectionFromLport, util.IngressDefaultDrop, match, ovnnb.ACLActionDrop, func(acl *ovnnb.ACL) {
 			acl.Name = &pgName
 			acl.Log = false
@@ -731,7 +731,7 @@ func (suite *OvnClientTestSuite) testSetACLLog() {
 		err = ovnClient.CreateAcls(pgName, portGroupKey, acl)
 		require.NoError(t, err)
 
-		err = ovnClient.SetACLLog(pgName, kubeovnv1.ProtocolIPv4, true, false)
+		err = ovnClient.SetACLLog(pgName, true, false)
 		require.NoError(t, err)
 
 		acl, err = ovnClient.GetACL(pgName, ovnnb.ACLDirectionFromLport, util.IngressDefaultDrop, match, false)