fix: allow basic ICMPv6 neighbor discovery

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.

Author: aauren

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())

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

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
+}

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)
+		})
+	}
+}