From a0442e5abd95c65eccfda4c13c51dae9b4a3b0de Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Fri, 2 Aug 2024 10:34:38 -0500 Subject: [PATCH] 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. --- .../netpol/network_policy_controller.go | 25 +++++++--- .../proxy/network_services_controller.go | 41 ++++------------- pkg/utils/iptables.go | 40 ++++++++++++++++ pkg/utils/iptables_test.go | 46 +++++++++++++++++++ 4 files changed, 114 insertions(+), 38 deletions(-) create mode 100644 pkg/utils/iptables_test.go diff --git a/pkg/controllers/netpol/network_policy_controller.go b/pkg/controllers/netpol/network_policy_controller.go index e4cc19f8cc..b3bb233854 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 4410a32d45..e7be3d1dbe 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 d2f0009c62..45cecddad0 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 +} diff --git a/pkg/utils/iptables_test.go b/pkg/utils/iptables_test.go new file mode 100644 index 0000000000..96e5540388 --- /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) + }) + } +}