Add support for returning IPv6 node addresses

[hakman]

What type of PR is this?
/kind feature
/priority important-longterm

What this PR does / why we need it:
IPv6 support has been around for some time now both in AWS and Kubernetes. Dual-Stack is also one of the major themes in Kubernetes 1.21.
I am trying to add IPv6 support for kOps and bumping into various issues because the IPv6 addresses are not returned by the cloud provider implementation.
xRef: kubernetes/kops#8432

I am not sure if there was any reason for not implementing this until now, but this PR should be a good conversation starter.

Which issue(s) this PR fixes:
n/a

Special notes for your reviewer:
I added the IPv6 addresses to the internal address list as it seems to be the convention in the related feature docs.
I also considered that adding the IPv6 addresses after all internal IPv4 would best preserve current behavior.

Does this PR introduce a user-facing change?:

Added support for returning IPv6 node addresses

/cc @nckturner @wongma7 @justinsb @aojea

[anguslees]

I think we should add only the 'first' address (from each ENI).

In IPv4, AWS has a strong distinction between 'primary' IPv4 and all the 'secondary' addresses (per ENI). In IPv6, the AWS API does not make a distinction, and it's just a single list of all IPv6 addresses (per ENI).

So if/when we want to add multiple IPv6 addresses to an ENI, eg so we have VPC addresses for pods, then they will all turn up in this ipv6s list. I think we need to be less greedy here :) and only use the first address, so the 'rest' can be left for other purposes. From my testing ordering is preserved, so first means naive ipv6s[0].

[hakman]

Sounds good to me. Done, also added a test for this case.

[anguslees]

Should we should use it only if ip.IsGlobalUnicast()

Fwiw, I think we should use whatever address(es?) are reported by VPC (in metadata). If VPC is giving us ULA addresses (for example), then there's a very good reason, and the code here should trust the VPC address is relevant and use it.

I have no reason to believe this will ever happen to be clear! but if VPC is reporting something 'weird', then I think there's a very good chance that VPC is correct for that situation and our if IsGlobalUnicast() logic is going to be incorrect.

[aojea]

ULA are IsGlobalUnicast() , this just removes link-local, multicast and loopback
But agreed, this validation shouldn't be here and VPC is the source of truth

[hakman]

Thanks for the review @anguslees. I think I addressed all the comments.

[anguslees]

/lgtm

[hakman]

Good news! I built a custom image to test this change and managed to get Conformance tests passing using an IPv6-only cluster created with kOps:
https://testgrid.k8s.io/kops-ipv6#kops-grid-scenario-ipv6-conformance

[hakman]

/assign @nckturner

[nckturner]

Sorry for the delay, I was on vacation. This looks good to me.
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, hakman, nckturner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [nckturner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hakman]

/hold

[hakman]

@nckturner One final Q from me, if you don't mind. While playing with IPv6 clusters I noticed that k8s is picky about the order of the addresses. Would it make sense to move the IPv6 addresses first as they are optional and there is no reason to enable them unless someone wants to use them?

[nckturner]

I do remember that address order being important, would you mind adding some more detail though? Also I see the gce provider adding ipv6 node addresses here and it looks like they are not first.

[hakman]

As the fix is now, conformance tests pass on IPv6 only clusters, so this is not a huge issue.
Kubernetes reports node addresses as an ordered list and kubectl shows node IP as the first address (mostly aesthetic), though pods that run on host network report that address as their main address too.
Not sure if most tools support nodes and pods with multiple IPs so, ideally, there should be some preference.
With kube-controller-manager, kubelet allowed --node-ip=:: to specify family preference, but with external CCM this is no longer possible:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/nodestatus/setters.go#L148

[nckturner]

Spoke to @justinsb and he thinks it will be less likely to break users if we keep the ipv6 addresses at the end for now, with the potential of adding a configuration to return ipv6 first/only return ipv6. I don't have a good idea of what will happen when we have dual stack configured for nodes, but I'm wondering about this PR:

On nodes with dual-stack node.status.addresses (regardless of how they got them), hostNetwork pods will now have dual-stack pod.status.podIPs, allowing the pod to be the target of either a single-stack IPv4 or single-stack IPv6 Service (or, once dual stack services #91824 merges, a dual-stack Service).

It seems that PR adds pod.status.podIPs, which can be set by the cloud provider node addresses in the case of hostNetwork pods. Am I understanding that correctly?

[aojea]

It seems that PR adds pod.status.podIPs, which can be set by the cloud provider node addresses in the case of hostNetwork pods. Am I understanding that correctly?

hostNetwork pods use the IPs of the Host, until recently there was only one Node.Status.Address, hence hostNetwork pods always have only one PodIP.
Since that PR, hostNetwork pods in dual-stack clusters, will use one IP of each family from node.Status.Addresses for its PodIPs.
Important, node.status.Addresses order matters and defines the "primary" ip family, i.e., a dual-stack cluster with IPv4,IPv6 has IPv4 as primary ip family.

[hakman]

Sorry for my late reply, last week was pretty hectic for me. I think this is a good start and will get more eyes on IPv6 with AWS. We can adjust how it works in future PRs. Thanks @nckturner & @aojea.
/hold cancel

[anguslees]

I do remember that address order being important, would you mind adding some more detail though?

Just for the archives, order is extremely important here. Basically this whole Addresses array is not (and never has been) used for anything at all :P Kubernetes only uses the first InternalIP address in this array (modifyable via apiserver --kubelet-preferred-address-types, but it's always only the first of each address type) - so adding the v6 addresses after the v4 address means they're unused in all but a v6-only scenario.

[aojea]

with dual stack , addresses has a meaning, they define the order of the cluster ipv4 - ipv6 or ipv4 - ipv6