Repository: cloudnativelabs/kube-router
Pull request #1710 (2024-08-02 15:36:28)

fix: allow basic ICMPv6 neighbor discovery
@mrueg @k6av

This fixes the problem where if network policy is applied before any communication between two pods, all subsequent communication fails because the two pods aren't able to discovery each other.

Fixes: #1701 


Patch:
diff --git a/pkg/controllers/netpol/network_policy_controller.go b/pkg/controllers/netpol/network_policy_controller.go
index e4cc19f8..b3bb2338 100644
--- a/pkg/controllers/netpol/network_policy_controller.go
+++ b/pkg/controllers/netpol/network_policy_controller.go
@@ -581,12 +581,7 @@ func (npc *NetworkPolicyController) ensureExplicitAccept() {
 
 // Creates custom chains KUBE-NWPLCY-DEFAULT
 func (npc *NetworkPolicyController) ensureDefaultNetworkPolicyChain() {
-	for _, iptablesCmdHandler := range npc.iptablesCmdHandlers {
-		markArgs := make([]string, 0)
-		markComment := "rule to mark traffic matching a network policy"
-		markArgs = append(markArgs, "-j", "MARK", "-m", "comment", "--comment", markComment,
-			"--set-xmark", "0x10000/0x10000")
-
+	for family, iptablesCmdHandler := range npc.iptablesCmdHandlers {
 		exists, err := iptablesCmdHandler.ChainExists("filter", kubeDefaultNetpolChain)
 		if err != nil {
 			klog.Fatalf("failed to check for the existence of chain %s, error: %v", kubeDefaultNetpolChain, err)
@@ -598,6 +593,24 @@ func (npc *NetworkPolicyController) ensureDefaultNetworkPolicyChain() {
 					kubeDefaultNetpolChain, err.Error())
 			}
 		}
+
+		// Add common IPv4/IPv6 ICMP rules to the default network policy chain to ensure that pods communicate properly
+		icmpRules := utils.CommonICMPRules(family)
+		for _, icmpRule := range icmpRules {
+			icmpArgs := []string{"-m", "comment", "--comment", icmpRule.Comment, "-p", icmpRule.IPTablesProto,
+				icmpRule.IPTablesType, icmpRule.ICMPType, "-j", "ACCEPT"}
+			err = iptablesCmdHandler.AppendUnique("filter", kubeDefaultNetpolChain, icmpArgs...)
+			if err != nil {
+				klog.Fatalf("failed to run iptables command: %v", err)
+			}
+		}
+
+		// Start off by marking traffic with an invalid mark so that we can allow list only traffic accepted by a
+		// matching policy. Anything that still has 0x10000
+		markArgs := make([]string, 0)
+		markComment := "rule to mark traffic matching a network policy"
+		markArgs = append(markArgs, "-j", "MARK", "-m", "comment", "--comment", markComment,
+			"--set-xmark", "0x10000/0x10000")
 		err = iptablesCmdHandler.AppendUnique("filter", kubeDefaultNetpolChain, markArgs...)
 		if err != nil {
 			klog.Fatalf("Failed to run iptables command: %s", err.Error())
diff --git a/pkg/controllers/proxy/network_services_controller.go b/pkg/controllers/proxy/network_services_controller.go
index 4410a32d..e7be3d1d 100644
--- a/pkg/controllers/proxy/network_services_controller.go
+++ b/pkg/controllers/proxy/network_services_controller.go
@@ -473,47 +473,24 @@ func (nsc *NetworkServicesController) setupIpvsFirewall() error {
 		var comment string
 		var args []string
 		var exists bool
-		var icmpProto string
-		var icmpType string
 		var icmpRejectType string
-
 		//nolint:exhaustive // we don't need exhaustive searching for IP Families
 		switch family {
 		case v1.IPv4Protocol:
-			icmpProto = "icmp"
-			icmpType = "--icmp-type"
 			icmpRejectType = "icmp-port-unreachable"
 		case v1.IPv6Protocol:
-			icmpProto = "ipv6-icmp"
-			icmpType = "--icmpv6-type"
 			icmpRejectType = "icmp6-port-unreachable"
 		}
 
-		// Allow various types of ICMP that are important for routing
-		comment = "allow icmp echo requests to service IPs"
-		args = []string{"-m", "comment", "--comment", comment, "-p", icmpProto, icmpType, "echo-request",
-			"-j", "ACCEPT"}
-		err = iptablesCmdHandler.AppendUnique("filter", ipvsFirewallChainName, args...)
-		if err != nil {
-			return fmt.Errorf("failed to run iptables command: %s", err.Error())
-		}
-
-		comment = "allow icmp ttl exceeded messages to service IPs"
-		args = []string{"-m", "comment", "--comment", comment, "-p", icmpProto, icmpType, "time-exceeded",
-			"-j", "ACCEPT"}
-		err = iptablesCmdHandler.AppendUnique("filter", ipvsFirewallChainName, args...)
-		if err != nil {
-			return fmt.Errorf("failed to run iptables command: %s", err.Error())
-		}
-
-		// destination-unreachable here is also responsible for handling / allowing
-		// PMTU (https://en.wikipedia.org/wiki/Path_MTU_Discovery) responses
-		comment = "allow icmp destination unreachable messages to service IPs"
-		args = []string{"-m", "comment", "--comment", comment, "-p", icmpProto, icmpType, "destination-unreachable",
-			"-j", "ACCEPT"}
-		err = iptablesCmdHandler.AppendUnique("filter", ipvsFirewallChainName, args...)
-		if err != nil {
-			return fmt.Errorf("failed to run iptables command: %s", err.Error())
+		// Add common IPv4/IPv6 ICMP rules to the default network policy chain to ensure that pods communicate properly
+		icmpRules := utils.CommonICMPRules(family)
+		for _, icmpRule := range icmpRules {
+			icmpArgs := []string{"-m", "comment", "--comment", icmpRule.Comment, "-p", icmpRule.IPTablesProto,
+				icmpRule.IPTablesType, icmpRule.ICMPType, "-j", "ACCEPT"}
+			err = iptablesCmdHandler.AppendUnique("filter", ipvsFirewallChainName, icmpArgs...)
+			if err != nil {
+				return fmt.Errorf("failed to run iptables command: %v", err)
+			}
 		}
 
 		// Get into specific service specific allowances
diff --git a/pkg/utils/iptables.go b/pkg/utils/iptables.go
index d2f0009c..45cecdda 100644
--- a/pkg/utils/iptables.go
+++ b/pkg/utils/iptables.go
@@ -14,6 +14,13 @@ import (
 
 var hasWait bool
 
+const (
+	ICMPv4Proto = "icmp"
+	ICMPv4Type  = "--icmp-type"
+	ICMPv6Proto = "ipv6-icmp"
+	ICMPv6Type  = "--icmpv6-type"
+)
+
 // IPTablesHandler interface based on the IPTables struct from github.com/coreos/go-iptables
 // which allows to mock it.
 type IPTablesHandler interface {
@@ -43,6 +50,13 @@ type IPTablesHandler interface {
 	GetIptablesVersion() (int, int, int)
 }
 
+type ICMPRule struct {
+	IPTablesProto string
+	IPTablesType  string
+	ICMPType      string
+	Comment       string
+}
+
 //nolint:gochecknoinits // This is actually a good usage of the init() function
 func init() {
 	path, err := exec.LookPath("iptables-restore")
@@ -181,3 +195,29 @@ func (i *IPTablesSaveRestore) Restore(table string, data []byte) error {
 	}
 	return i.exec(i.restoreCmd, args, data, nil)
 }
+
+// CommonICMPRules returns a list of common ICMP rules that should always be allowed for given IP family
+func CommonICMPRules(family v1core.IPFamily) []ICMPRule {
+	// Allow various types of ICMP that are important for routing
+	// This first block applies to both IPv4 and IPv6 type rules
+	icmpRules := []ICMPRule{
+		{ICMPv4Proto, ICMPv4Type, "echo-request", "allow icmp echo requests"},
+		// destination-unreachable here is also responsible for handling / allowing PMTU
+		// (https://en.wikipedia.org/wiki/Path_MTU_Discovery) responses
+		{ICMPv4Proto, ICMPv4Type, "destination-unreachable", "allow icmp destination unreachable messages"},
+		{ICMPv4Proto, ICMPv4Type, "time-exceeded", "allow icmp time exceeded messages"},
+	}
+
+	if family == v1core.IPv6Protocol {
+		// Neighbor discovery packets are especially crucial here as without them pods will not communicate properly
+		// over IPv6. Neighbor discovery packets are essentially like ARP for IPv4 which was always allowed under.
+		// previous kube-router versions.
+		icmpRules = append(icmpRules, []ICMPRule{
+			{ICMPv6Proto, ICMPv6Type, "neighbor-solicitation", "allow icmp neighbor solicitation messages"},
+			{ICMPv6Proto, ICMPv6Type, "neighbor-advertisement", "allow icmp neighbor advertisement messages"},
+			{ICMPv6Proto, ICMPv6Type, "echo-reply", "allow icmp echo reply messages"},
+		}...)
+	}
+
+	return icmpRules
+}


Test patch:
diff --git a/pkg/utils/iptables_test.go b/pkg/utils/iptables_test.go
new file mode 100644
index 00000000..96e55403
--- /dev/null
+++ b/pkg/utils/iptables_test.go
@@ -0,0 +1,46 @@
+package utils
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	v1core "k8s.io/api/core/v1"
+)
+
+func TestCommonICMPRules(t *testing.T) {
+	tests := []struct {
+		name     string
+		family   v1core.IPFamily
+		expected []ICMPRule
+	}{
+		{
+			name:   "IPv4",
+			family: v1core.IPv4Protocol,
+			expected: []ICMPRule{
+				{"icmp", "--icmp-type", "echo-request", "allow icmp echo requests"},
+				{"icmp", "--icmp-type", "destination-unreachable", "allow icmp destination unreachable messages"},
+				{"icmp", "--icmp-type", "time-exceeded", "allow icmp time exceeded messages"},
+			},
+		},
+		{
+			name:   "IPv6",
+			family: v1core.IPv6Protocol,
+			expected: []ICMPRule{
+				{"icmp", "--icmp-type", "echo-request", "allow icmp echo requests"},
+				{"icmp", "--icmp-type", "destination-unreachable", "allow icmp destination unreachable messages"},
+				{"icmp", "--icmp-type", "time-exceeded", "allow icmp time exceeded messages"},
+				{"ipv6-icmp", "--icmpv6-type", "neighbor-solicitation", "allow icmp neighbor solicitation messages"},
+				{"ipv6-icmp", "--icmpv6-type", "neighbor-advertisement", "allow icmp neighbor advertisement messages"},
+				{"ipv6-icmp", "--icmpv6-type", "echo-reply", "allow icmp echo reply messages"},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := CommonICMPRules(tt.family)
+			assert.Equal(t, tt.expected, result, "CommonICMPRules(%v) = %v, want %v", tt.family, result, tt.expected)
+		})
+	}
+}