digitalocean / digitalocean-cloud-controller-manager

Kubernetes cloud-controller-manager for DigitalOcean (beta)
Apache License 2.0
527 stars 149 forks source link

IPv6 address missing in nodes status #555

Open sachintiptur opened 1 year ago

sachintiptur commented 1 year ago

We integrated DO CCM into a dual-stack(IPv4 + IPv6) cluster. Nodes are up with both IPv4 and IPv6 addresses but ipv6 addresses are not updated in node status(kubectl describe nodes).

DO CCM flags used:

digitalocean-cloud-controller-manager --kubeconfig=/etc/kubernetes/kubeconfig/kubeconfig --leader-elect=false

Can you let us know whether IPv6 support is available in the version v0.1.40 or is there any specific configuration that we are missing.

timoreimann commented 1 year ago

👋

the CCM implementation fetches v4 addresses only at this point. It should be fairly easy to include v6 ones from the DO API / the droplet struct we're working on already as well here.

The one part I'm not absolutely sure about is how those addresses would need to be reflected in the []v1.NodeAddress slice. Does it suffice to add another set of items of types v1.NodeInternalIP and v1.NodeExternalIP, respectively, to put the v6 addresses in? That is, is Kubernetes able to distinguish the address protocols without any additional tagging in the struct (for which I currently don't see a way to do it, i.e., there's no v1.NodeInternalIPv6 type or something equivalent).

This may need some discovery.

timoreimann commented 1 year ago

(And yeah, additional binary flags might be needed too.)

sachintiptur commented 1 year ago

@timoreimann it can be appended to existing []v1.NodeAddress, no need of new type for IPv6. Also what is the additional flag? is it to enable IPv6 in DO CCM?

timoreimann commented 1 year ago

I might be wrong and we don't necessarily need a new flag. (CCMs like ours mostly inherit flags from kube-controller-manager, so we could check there for any IPv6-ish options.)

I think we'd want a flag or environment variable though to control which IP addresses should be collected (IPv4 as today or IPv4 and IPv6) to make sure that clients expecting the node address fields to be IPv4-populated only do not break.

sachintiptur commented 1 year ago

@timoreimann Ingodo.DropletCreateRequest there is a IPv6 bool, but I think it is for node creation. May be an environment variable or cloud-config ??

timoreimann commented 1 year ago

@sachintiptur CCM does not create VM / droplets but reads IP information from droplets retrieved via the public API. As such, we'd probably want to look at the PublicIPv6() method.

sachintiptur commented 1 year ago

@timoreimann Yeah I got that point, I meant or under assumption that the ipv6 config would be present in droplet while node creation and its being propagated to ccm. Otherwise even I thought about checking for presence of IPv6 Address in Network field. I tried that change locally and it works.

diff --git a/cloud-controller-manager/do/common.go b/cloud-controller-manager/do/common.go
index 1e0538c6..843bed05 100644
--- a/cloud-controller-manager/do/common.go
+++ b/cloud-controller-manager/do/common.go
@@ -141,5 +141,11 @@ func nodeAddresses(droplet *godo.Droplet) ([]v1.NodeAddress, error) {
        }
        addresses = append(addresses, v1.NodeAddress{Type: v1.NodeExternalIP, Address: publicIP})

+       publicIPv6, err := droplet.PublicIPv6()
+       if err != nil || publicIPv6 == "" {
+               return nil, fmt.Errorf("could not get public ipv6: %v", err)
+       }
+       addresses = append(addresses, v1.NodeAddress{Type: v1.NodeExternalIP, Address: publicIPv6})
+
        return addresses, nil
 }
timoreimann commented 1 year ago

Oh gotcha @sachintiptur.

Glad to hear it's working for you. Feel free to submit a PR that adds the logic gated behind an environment variable, e.g., IP_ADDR_PROTOS which defines the IP protocol versions to fetch addresses for and could be a comma-separated list with values 4 and/or 6 (where the empty value defaults to IPv4 still). Let me know what you think.

sachintiptur commented 1 year ago

Yeah that makes sense. Sure will submit a PR.