Add IPv6 support to iptables proxier #48551

leblancd · 2017-07-06T15:55:12Z

Add IPv6 support to iptables proxier

The following changes are proposed for the iptables proxier:

There are three places where a string specifying IP:port is parsed
using something like this:
if index := strings.Index(e.endpoint, ":"); index != -1 {
This will fail for IPv6 since V6 addresses contain colons. Also,
the V6 address is expected to be surrounded by square brackets
(i.e. []:). Fix this by replacing call to Index with
call to LastIndex() and stripping out square brackets.
The String() method for the localPort struct should put square brackets
around IPv6 addresses.
The logging in the merge() method for proxyServiceMap should put brackets
around IPv6 addresses.
There are several places where filterRules destination is hardcoded to
/32. This should be a /128 for IPv6 case.
Add IPv6 unit test cases

Note: I've left out most of the UT test cases that I had included in my original version of this
PR because the number of lines of code change were much too large for a single review.
I'm including a minimum of UT with this current version of the PR.

fixes #48550

What this PR does / why we need it:
This PR addresses several issues in the iptables proxier for handling IPv6 addresses
that were found via visual code inspection, including:

There are three places where a string specifying IP:port using something like the following:
if index := strings.Index(e.endpoint, ":"); index != -1 {
This will fail for IPv6 since V6 addresses contains many colons, and the V6 address is expected
to be enclosed in square brackets when followed by :.
The String() method for the localPort struct should put square brackets around IPv6 addresses.
The logging in the merge() method for proxyServiceMap should put brackets around IPv6
addresses.
There are several places where filterRules destination is hardcoded to /32.
Should be a /128 for IPv6 case.
More IPv6 unit test cases are needed.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #48550

Special notes for your reviewer:

Release note:

NONE

k8s-ci-robot · 2017-07-06T15:55:21Z

Hi @leblancd. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

cmluciano · 2017-07-06T17:47:58Z

@k8s-bot ok to test

pmichali

Bunch of questions in-line. Mostly minor stuff.

pmichali · 2017-07-07T11:13:52Z

pkg/proxy/iptables/proxier.go

+	if index := strings.LastIndex(s, ":"); index != -1 {
+		ip := s[0:index]
+		// Strip off any surrounding brackets.
+		re := regexp.MustCompile(`\[(.*)\]`)


Would a less compute intensive solution be to just check for a '[' at position zero and then take slice w/o first and last character?

yes, regex seems overpowered for this

pmichali · 2017-07-07T11:16:47Z

pkg/proxy/iptables/proxier.go

+// ipPort returns a string of the form "<ip>:<port>" (for IPv4)
+// or "[<ip>]:<port>" (for IPv6) for a given IP address and port
+func ipPort(ipStr string, port int) string {
+	ip := net.ParseIP(ipStr)


Do we know that ipStr will always be an IP address (and not a name)? Is it guaranteed to always be valid?

net.JoinHostPort() seems to do exactly what this does, if we just fmt.Sprintf("%d", port) or stconv.Itoa(port)

@thockin: Yes, that will do the trick, thanks!

pmichali · 2017-07-07T11:25:51Z

pkg/proxy/iptables/proxier.go

+// IPv4 and <ip-address>/128 for IPv6
+func hostAddress(ip net.IP) string {
+	len := 32
+	if ip.To4() == nil {


Is it always guaranteed that ip is a valid IP address? IOW no need to check for nil? Just wondering if all the callers using this are providing valid IP addresses.

The nil check isn't being done as a sanity check, it's being done to determine if the IP address is IPv4 or IPv6.

pmichali

Would suggest adding additional test cases that exercise the new/modified methods directly (versus through some call chain of higher level function test). Especially as you can exercise error cases.

pmichali · 2017-07-07T11:33:50Z