From b2f5c8e6107a84ef03bc1d43e075c07841082931 Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Mon, 2 Apr 2018 11:53:12 +0800 Subject: [PATCH 1/4] fix localport open - iptables part changes --- pkg/proxy/iptables/proxier.go | 69 +++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 23 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 4b57e044065..de35c9f11fa 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -960,32 +960,55 @@ func (proxier *Proxier) syncProxyRules() { if svcInfo.NodePort != 0 { // Hold the local port open so no other process can open it // (because the socket might open but it would never work). - lp := utilproxy.LocalPort{ - Description: "nodePort for " + svcNameString, - IP: "", - Port: svcInfo.NodePort, - Protocol: protocol, + addresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer) + if err != nil { + glog.Errorf("Failed to get node ip address matching nodeport cidr") + continue } - if proxier.portsMap[lp] != nil { - glog.V(4).Infof("Port %s was open before and is still needed", lp.String()) - replacementPortsMap[lp] = proxier.portsMap[lp] - } else { - socket, err := proxier.portMapper.OpenLocalPort(&lp) - if err != nil { - glog.Errorf("can't open %s, skipping this nodePort: %v", lp.String(), err) - continue - } - if lp.Protocol == "udp" { - // TODO: We might have multiple services using the same port, and this will clear conntrack for all of them. - // 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 - err := conntrack.ClearEntriesForPort(proxier.exec, lp.Port, isIPv6, v1.ProtocolUDP) - if err != nil { - glog.Errorf("Failed to clear udp conntrack for port %d, error: %v", lp.Port, err) + + lps := make([]utilproxy.LocalPort, 0) + for address := range addresses { + if utilproxy.IsZeroCIDR(address) { + lp := utilproxy.LocalPort{ + Description: "nodePort for " + svcNameString, + IP: "", + Port: svcInfo.NodePort, + Protocol: protocol, } + lps = append(lps, lp) + break + } + lp := utilproxy.LocalPort{ + Description: "nodePort for " + svcNameString, + IP: address, + Port: svcInfo.NodePort, + Protocol: protocol, + } + lps = append(lps, lp) + } + + for _, lp := range lps { + if proxier.portsMap[lp] != nil { + glog.V(4).Infof("Port %s was open before and is still needed", lp.String()) + replacementPortsMap[lp] = proxier.portsMap[lp] + } else { + socket, err := proxier.portMapper.OpenLocalPort(&lp) + if err != nil { + glog.Errorf("can't open %s, skipping this nodePort: %v", lp.String(), err) + continue + } + if lp.Protocol == "udp" { + // TODO: We might have multiple services using the same port, and this will clear conntrack for all of them. + // 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 + err := conntrack.ClearEntriesForPort(proxier.exec, lp.Port, isIPv6, v1.ProtocolUDP) + if err != nil { + glog.Errorf("Failed to clear udp conntrack for port %d, error: %v", lp.Port, err) + } + } + replacementPortsMap[lp] = socket } - replacementPortsMap[lp] = socket } if hasEndpoints { From ac1cd3dcb4b6e1f1d44476fb6353e40daa9d6d0b Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Mon, 2 Apr 2018 11:53:37 +0800 Subject: [PATCH 2/4] fix localport open - ipvs part changes --- pkg/proxy/ipvs/proxier.go | 64 +++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 7f0e7630fdb..dbea191b190 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -985,27 +985,50 @@ func (proxier *Proxier) syncProxyRules() { } if svcInfo.NodePort != 0 { - lp := utilproxy.LocalPort{ - Description: "nodePort for " + svcNameString, - IP: "", - Port: svcInfo.NodePort, - Protocol: protocol, + addresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer) + if err != nil { + glog.Errorf("Failed to get node ip address matching nodeport cidr") + continue } - if proxier.portsMap[lp] != nil { - glog.V(4).Infof("Port %s was open before and is still needed", lp.String()) - replacementPortsMap[lp] = proxier.portsMap[lp] - } else { - socket, err := proxier.portMapper.OpenLocalPort(&lp) - if err != nil { - glog.Errorf("can't open %s, skipping this nodePort: %v", lp.String(), err) - continue + + lps := make([]utilproxy.LocalPort, 0) + for address := range addresses { + if utilproxy.IsZeroCIDR(address) { + lp := utilproxy.LocalPort{ + Description: "nodePort for " + svcNameString, + IP: "", + Port: svcInfo.NodePort, + Protocol: protocol, + } + lps = append(lps, lp) + break } - if lp.Protocol == "udp" { - isIPv6 := utilnet.IsIPv6(svcInfo.ClusterIP) - conntrack.ClearEntriesForPort(proxier.exec, lp.Port, isIPv6, clientv1.ProtocolUDP) + lp := utilproxy.LocalPort{ + Description: "nodePort for " + svcNameString, + IP: address, + Port: svcInfo.NodePort, + Protocol: protocol, } - replacementPortsMap[lp] = socket - } // We're holding the port, so it's OK to install ipvs rules. + lps = append(lps, lp) + } + + for _, lp := range lps { + if proxier.portsMap[lp] != nil { + glog.V(4).Infof("Port %s was open before and is still needed", lp.String()) + replacementPortsMap[lp] = proxier.portsMap[lp] + } else { + socket, err := proxier.portMapper.OpenLocalPort(&lp) + if err != nil { + glog.Errorf("can't open %s, skipping this nodePort: %v", lp.String(), err) + continue + } + if lp.Protocol == "udp" { + isIPv6 := utilnet.IsIPv6(svcInfo.ClusterIP) + conntrack.ClearEntriesForPort(proxier.exec, lp.Port, isIPv6, clientv1.ProtocolUDP) + } + replacementPortsMap[lp] = socket + } // We're holding the port, so it's OK to install ipvs rules. + } // Nodeports need SNAT, unless they're local. // ipset call @@ -1038,11 +1061,6 @@ func (proxier *Proxier) syncProxyRules() { // Build ipvs kernel routes for each node ip address nodeIPs := make([]net.IP, 0) - addresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer) - if err != nil { - glog.Errorf("Failed to get node ip address matching nodeport cidr") - continue - } for address := range addresses { if !utilproxy.IsZeroCIDR(address) { nodeIPs = append(nodeIPs, net.ParseIP(address)) From 8b16d66b46e8ca30052f22fcdf32660ed9dc7d67 Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Wed, 2 May 2018 17:02:07 +0800 Subject: [PATCH 3/4] add some comment message --- pkg/proxy/iptables/proxier.go | 4 +++- pkg/proxy/ipvs/proxier.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index de35c9f11fa..73b7b27d9de 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -962,7 +962,7 @@ func (proxier *Proxier) syncProxyRules() { // (because the socket might open but it would never work). addresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer) if err != nil { - glog.Errorf("Failed to get node ip address matching nodeport cidr") + glog.Errorf("Failed to get node ip address matching nodeport cidr: %v", err) continue } @@ -976,6 +976,7 @@ func (proxier *Proxier) syncProxyRules() { Protocol: protocol, } lps = append(lps, lp) + // If we encounter a zero CIDR, then there is no point in processing the rest of the addresses. break } lp := utilproxy.LocalPort{ @@ -987,6 +988,7 @@ func (proxier *Proxier) syncProxyRules() { lps = append(lps, lp) } + // For ports on node IPs, open the actual port and hold it. for _, lp := range lps { if proxier.portsMap[lp] != nil { glog.V(4).Infof("Port %s was open before and is still needed", lp.String()) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index dbea191b190..1c067a24d4f 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -987,7 +987,7 @@ func (proxier *Proxier) syncProxyRules() { if svcInfo.NodePort != 0 { addresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer) if err != nil { - glog.Errorf("Failed to get node ip address matching nodeport cidr") + glog.Errorf("Failed to get node ip address matching nodeport cidr: %v", err) continue } @@ -1001,6 +1001,7 @@ func (proxier *Proxier) syncProxyRules() { Protocol: protocol, } lps = append(lps, lp) + // If we encounter a zero CIDR, then there is no point in processing the rest of the addresses. break } lp := utilproxy.LocalPort{ @@ -1012,6 +1013,7 @@ func (proxier *Proxier) syncProxyRules() { lps = append(lps, lp) } + // For ports on node IPs, open the actual port and hold it. for _, lp := range lps { if proxier.portsMap[lp] != nil { glog.V(4).Infof("Port %s was open before and is still needed", lp.String()) From 029a16a1ebd85efc8c66abe431b657e6cad06540 Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Mon, 14 May 2018 16:07:13 +0800 Subject: [PATCH 4/4] fix review comments --- pkg/proxy/iptables/proxier.go | 18 +++++++----------- pkg/proxy/ipvs/proxier.go | 18 +++++++----------- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 73b7b27d9de..a4338f62d35 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -968,23 +968,19 @@ func (proxier *Proxier) syncProxyRules() { lps := make([]utilproxy.LocalPort, 0) for address := range addresses { - if utilproxy.IsZeroCIDR(address) { - lp := utilproxy.LocalPort{ - Description: "nodePort for " + svcNameString, - IP: "", - Port: svcInfo.NodePort, - Protocol: protocol, - } - lps = append(lps, lp) - // If we encounter a zero CIDR, then there is no point in processing the rest of the addresses. - break - } lp := utilproxy.LocalPort{ Description: "nodePort for " + svcNameString, IP: address, Port: svcInfo.NodePort, Protocol: protocol, } + if utilproxy.IsZeroCIDR(address) { + // Empty IP address means all + lp.IP = "" + lps = append(lps, lp) + // If we encounter a zero CIDR, then there is no point in processing the rest of the addresses. + break + } lps = append(lps, lp) } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 1c067a24d4f..8bcd30103be 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -993,23 +993,19 @@ func (proxier *Proxier) syncProxyRules() { lps := make([]utilproxy.LocalPort, 0) for address := range addresses { - if utilproxy.IsZeroCIDR(address) { - lp := utilproxy.LocalPort{ - Description: "nodePort for " + svcNameString, - IP: "", - Port: svcInfo.NodePort, - Protocol: protocol, - } - lps = append(lps, lp) - // If we encounter a zero CIDR, then there is no point in processing the rest of the addresses. - break - } lp := utilproxy.LocalPort{ Description: "nodePort for " + svcNameString, IP: address, Port: svcInfo.NodePort, Protocol: protocol, } + if utilproxy.IsZeroCIDR(address) { + // Empty IP address means all + lp.IP = "" + lps = append(lps, lp) + // If we encounter a zero CIDR, then there is no point in processing the rest of the addresses. + break + } lps = append(lps, lp) }