From 28000f925f6724f8a31d1de261cfc3531c36e40b Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Mon, 30 Oct 2017 13:51:14 +0800 Subject: [PATCH] fix IPV6 judgement bug and add UTs --- pkg/proxy/iptables/proxier.go | 2 +- pkg/proxy/ipvs/proxier.go | 2 +- pkg/proxy/util/conntrack.go | 12 ++-- pkg/proxy/util/conntrack_test.go | 101 ++++++++++++++++++++++++++++++- 4 files changed, 109 insertions(+), 8 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 1561eff0a61..786434f7c10 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -1343,7 +1343,7 @@ func (proxier *Proxier) syncProxyRules() { // This is very low impact. The NodePort range is intentionally obscure, and unlikely to actually collide with real Services. // This only affects UDP connections, which are not common. // See issue: https://github.com/kubernetes/kubernetes/issues/49881 - isIPv6 := svcInfo.clusterIP.To4() != nil + isIPv6 := utilproxy.IsIPv6(svcInfo.clusterIP) err := utilproxy.ClearUDPConntrackForPort(proxier.exec, lp.Port, isIPv6) if err != nil { glog.Errorf("Failed to clear udp conntrack for port %d, error: %v", lp.Port, err) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 8510f7363e1..12b0bb93590 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1154,7 +1154,7 @@ func (proxier *Proxier) syncProxyRules() { continue } if lp.Protocol == "udp" { - isIPv6 := svcInfo.clusterIP.To4() != nil + isIPv6 := utilproxy.IsIPv6(svcInfo.clusterIP) utilproxy.ClearUDPConntrackForPort(proxier.exec, lp.Port, isIPv6) } replacementPortsMap[lp] = socket diff --git a/pkg/proxy/util/conntrack.go b/pkg/proxy/util/conntrack.go index 504085a5ae2..7806c3a1e8e 100644 --- a/pkg/proxy/util/conntrack.go +++ b/pkg/proxy/util/conntrack.go @@ -29,11 +29,15 @@ import ( const noConnectionToDelete = "0 flow entries have been deleted" -func isIPv6(ip string) bool { - netIP := net.ParseIP(ip) +func IsIPv6(netIP net.IP) bool { return netIP != nil && netIP.To4() == nil } +func IsIPv6String(ip string) bool { + netIP := net.ParseIP(ip) + return IsIPv6(netIP) +} + func parametersWithFamily(isIPv6 bool, parameters ...string) []string { if isIPv6 { parameters = append(parameters, "-f", "ipv6") @@ -44,7 +48,7 @@ func parametersWithFamily(isIPv6 bool, parameters ...string) []string { // ClearUDPConntrackForIP uses the conntrack tool to delete the conntrack entries // for the UDP connections specified by the given service IP func ClearUDPConntrackForIP(execer exec.Interface, ip string) error { - parameters := parametersWithFamily(isIPv6(ip), "-D", "--orig-dst", ip, "-p", "udp") + parameters := parametersWithFamily(IsIPv6String(ip), "-D", "--orig-dst", ip, "-p", "udp") err := ExecConntrackTool(execer, parameters...) if err != nil && !strings.Contains(err.Error(), noConnectionToDelete) { // TODO: Better handling for deletion failure. When failure occur, stale udp connection may not get flushed. @@ -89,7 +93,7 @@ func ClearUDPConntrackForPort(execer exec.Interface, port int, isIPv6 bool) erro // ClearUDPConntrackForPeers uses the conntrack tool to delete the conntrack entries // for the UDP connections specified by the {origin, dest} IP pair. func ClearUDPConntrackForPeers(execer exec.Interface, origin, dest string) error { - parameters := parametersWithFamily(isIPv6(origin), "-D", "--orig-dst", origin, "--dst-nat", dest, "-p", "udp") + parameters := parametersWithFamily(IsIPv6String(origin), "-D", "--orig-dst", origin, "--dst-nat", dest, "-p", "udp") err := ExecConntrackTool(execer, parameters...) if err != nil && !strings.Contains(err.Error(), noConnectionToDelete) { // TODO: Better handling for deletion failure. When failure occur, stale udp connection may not get flushed. diff --git a/pkg/proxy/util/conntrack_test.go b/pkg/proxy/util/conntrack_test.go index 6c0db300029..f3d8a145269 100644 --- a/pkg/proxy/util/conntrack_test.go +++ b/pkg/proxy/util/conntrack_test.go @@ -18,6 +18,7 @@ package util import ( "fmt" + "net" "strings" "testing" @@ -117,7 +118,7 @@ func TestClearUDPConntrackForIP(t *testing.T) { if err := ClearUDPConntrackForIP(&fexec, tc.ip); err != nil { t.Errorf("%s test case:, Unexpected error: %v", tc.name, err) } - expectCommand := fmt.Sprintf("conntrack -D --orig-dst %s -p udp", tc.ip) + familyParamStr(isIPv6(tc.ip)) + expectCommand := fmt.Sprintf("conntrack -D --orig-dst %s -p udp", tc.ip) + familyParamStr(IsIPv6String(tc.ip)) execCommand := strings.Join(fcmd.CombinedOutputLog[svcCount], " ") if expectCommand != execCommand { t.Errorf("%s test case: Expect command: %s, but executed %s", tc.name, expectCommand, execCommand) @@ -221,7 +222,7 @@ func TestDeleteUDPConnections(t *testing.T) { if err != nil { t.Errorf("%s test case: unexpected error: %v", tc.name, err) } - expectCommand := fmt.Sprintf("conntrack -D --orig-dst %s --dst-nat %s -p udp", tc.origin, tc.dest) + familyParamStr(isIPv6(tc.origin)) + expectCommand := fmt.Sprintf("conntrack -D --orig-dst %s --dst-nat %s -p udp", tc.origin, tc.dest) + familyParamStr(IsIPv6String(tc.origin)) execCommand := strings.Join(fcmd.CombinedOutputLog[i], " ") if expectCommand != execCommand { t.Errorf("%s test case: Expect command: %s, but executed %s", tc.name, expectCommand, execCommand) @@ -232,3 +233,99 @@ func TestDeleteUDPConnections(t *testing.T) { t.Errorf("Expect command executed %d times, but got %d", svcCount, fexec.CommandCalls) } } + +func TestIsIPv6String(t *testing.T) { + testCases := []struct { + ip string + expectIPv6 bool + }{ + { + ip: "127.0.0.1", + expectIPv6: false, + }, + { + ip: "192.168.0.0", + expectIPv6: false, + }, + { + ip: "1.2.3.4", + expectIPv6: false, + }, + { + ip: "bad ip", + expectIPv6: false, + }, + { + ip: "::1", + expectIPv6: true, + }, + { + ip: "fd00::600d:f00d", + expectIPv6: true, + }, + { + ip: "2001:db8::5", + expectIPv6: true, + }, + } + for i := range testCases { + isIPv6 := IsIPv6String(testCases[i].ip) + if isIPv6 != testCases[i].expectIPv6 { + t.Errorf("[%d] Expect ipv6 %v, got %v", i+1, testCases[i].expectIPv6, isIPv6) + } + } +} + +func TestIsIPv6(t *testing.T) { + testCases := []struct { + ip net.IP + expectIPv6 bool + }{ + { + ip: net.IPv4zero, + expectIPv6: false, + }, + { + ip: net.IPv4bcast, + expectIPv6: false, + }, + { + ip: net.ParseIP("127.0.0.1"), + expectIPv6: false, + }, + { + ip: net.ParseIP("10.20.40.40"), + expectIPv6: false, + }, + { + ip: net.ParseIP("172.17.3.0"), + expectIPv6: false, + }, + { + ip: nil, + expectIPv6: false, + }, + { + ip: net.IPv6loopback, + expectIPv6: true, + }, + { + ip: net.IPv6zero, + expectIPv6: true, + }, + { + ip: net.ParseIP("fd00::600d:f00d"), + expectIPv6: true, + }, + { + ip: net.ParseIP("2001:db8::5"), + expectIPv6: true, + }, + } + for i := range testCases { + isIPv6 := IsIPv6(testCases[i].ip) + if isIPv6 != testCases[i].expectIPv6 { + t.Errorf("[%d] Expect ipv6 %v, got %v", i+1, testCases[i].expectIPv6, isIPv6) + } + } +}