Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPv6 address missing in nodes status #555

Open
sachintiptur opened this issue Dec 7, 2022 · 9 comments
Open

IPv6 address missing in nodes status #555

sachintiptur opened this issue Dec 7, 2022 · 9 comments

Comments

@sachintiptur
Copy link

sachintiptur commented Dec 7, 2022

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
Copy link
Collaborator

👋

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
Copy link
Collaborator

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

@sachintiptur
Copy link
Author

sachintiptur commented Dec 8, 2022

@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
Copy link
Collaborator

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
Copy link
Author

sachintiptur commented Dec 8, 2022

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

@timoreimann
Copy link
Collaborator

@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
Copy link
Author

@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
Copy link
Collaborator

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
Copy link
Author

Yeah that makes sense. Sure will submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants