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 support in node addresses #561

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sachintiptur
Copy link

@sachintiptur sachintiptur commented Dec 12, 2022

Signed-off-by: Sachin Tiptur [email protected]

This PR adds a new env DO_IP_ADDR_FAMILIES to configure IP families required to add IP addresses in node's status.
This also populates the nodes address in the same order as DO_IP_ADDR_FAMILIES is configured.
This fixes the issue #555

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, appreciated.

It'd be great if we could extend the README / documentation as well.

@@ -128,18 +130,28 @@ func allLoadBalancerList(ctx context.Context, client *godo.Client) ([]godo.LoadB
func nodeAddresses(droplet *godo.Droplet) ([]v1.NodeAddress, error) {
var addresses []v1.NodeAddress
addresses = append(addresses, v1.NodeAddress{Type: v1.NodeHostName, Address: droplet.Name})
ipFamilies := os.Getenv(doIPAddrFamiliesEnv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be great if we parsed the env var during the startup phase only (and use the opportunity to error out if something unsupported is passed in). The logic processing the parameters (i.e., this function) can then work off of more strongly typed data (say, a typed string with two constant values).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

cloud-controller-manager/do/common.go Outdated Show resolved Hide resolved
Sachin Tiptur added 2 commits December 13, 2022 11:18
Signed-off-by: Sachin Tiptur <[email protected]>
Signed-off-by: Sachin Tiptur <[email protected]>
README.md Outdated
@@ -62,7 +62,7 @@ package-hosted directory like this:

```bash
cd cloud-controller-manager/cmd/digitalocean-cloud-controller-manager
REGION=fra1 DO_ACCESS_TOKEN=your_access_token go run main.go \
REGION=fra1 DO_ACCESS_TOKEN=your_access_token DO_IP_ADDR_FAMILIES=ipv4 go run main.go \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd omit the usage from this example since it's not strictly needed when running locally.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

README.md Outdated
@@ -79,6 +79,11 @@ You might also need to provide your DigitalOcean access token in
the cloud controller to start, but in that case, you will not be able to
validate integration with DigitalOcean API.

The `DO_IP_ADDR_FAMILIES` is used to configure the required IP familes and the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: families

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

README.md Outdated
@@ -79,6 +79,11 @@ You might also need to provide your DigitalOcean access token in
the cloud controller to start, but in that case, you will not be able to
validate integration with DigitalOcean API.

The `DO_IP_ADDR_FAMILIES` is used to configure the required IP familes and the
order in which address should be populated in nodes status. The accepted values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
order in which address should be populated in nodes status. The accepted values
order in which the addresses should be populated in nodes status. The accepted values

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

cloud-controller-manager/do/cloud.go Outdated Show resolved Hide resolved
cloud-controller-manager/do/cloud.go Outdated Show resolved Hide resolved
README.md Outdated
@@ -79,6 +79,11 @@ You might also need to provide your DigitalOcean access token in
the cloud controller to start, but in that case, you will not be able to
validate integration with DigitalOcean API.

The `DO_IP_ADDR_FAMILIES` is used to configure the required IP familes and the
order in which address should be populated in nodes status. The accepted values
are one of the `{"", "ipv4", "ipv6", "ipv4,ipv6", "ipv6,ipv4"}`.IPv4 is the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have curly braces here? It almost looks like JSON but we don't require JSON, do we?

Can we rephrase this in a more user friendly manner, e.g., say one of "ipv4", "ipv6", or a comma-separated list of multiple IP address families?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

cloud-controller-manager/do/common.go Outdated Show resolved Hide resolved
Signed-off-by: Sachin Tiptur <[email protected]>
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

Successfully merging this pull request may close these issues.

2 participants