Fixed review comments

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

Author: Sachin Tiptur

README.md

@@ -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 DO_IP_ADDR_FAMILIES=ipv4 go run main.go \
+REGION=fra1 DO_ACCESS_TOKEN=your_access_token go run main.go \
   --kubeconfig <path to your kubeconfig file>                     \
   --leader-elect=false --v=5 --cloud-provider=digitalocean
 ```
@@ -79,10 +79,10 @@ 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 
-default, if not set or empty.
+The `DO_IP_ADDR_FAMILIES` is used to configure the required IP families and the
+order in which the addresses should be populated in nodes status. The accepted values
+are one of the `"ipv4", "ipv6"` or a comma-separated list of multiple IP address
+families. IPv4 is the default, if not set or empty.
 
 Please note that if you use a Kubernetes cluster created on DigitalOcean, there
 will be a cloud controller manager running in the cluster already, so your local

cloud-controller-manager/do/cloud.go

@@ -62,9 +62,6 @@ const (
 
 var version string
 
-var ipFamilyOpts = []string{"", "ipv4", "ipv6", "ipv4,ipv6", "ipv6,ipv4"}
-var ipFamilies string
-
 type tokenSource struct {
 	AccessToken string
 }
@@ -162,10 +159,8 @@ func newCloud() (cloudprovider.Interface, error) {
 		addr = fmt.Sprintf("%s:%s", addrHost, addrPort)
 	}
 
-	// var ipFamilies string
 	ipf, set := os.LookupEnv(doIPAddrFamiliesEnv)
-	ipFamilies = ipf
-	if set && !containsString(ipFamilyOpts, ipFamilies) {
+	if set && !validateAndSetIPFamilies(ipf) {
 		return nil, fmt.Errorf("invalid value set for environment variable %q", doIPAddrFamiliesEnv)
 	}
 
@@ -292,11 +287,17 @@ func (c *cloud) HasClusterID() bool {
 	return false
 }
 
-func containsString(vals []string, val string) bool {
-	for _, v := range vals {
-		if v == val {
-			return true
+func validateAndSetIPFamilies(ipf string) bool {
+	for _, v := range strings.Split(ipf, ",") {
+		ipf := strings.TrimSpace(v)
+		switch ipf {
+		case "ipv4":
+			ipFamilies = append(ipFamilies, ipv4Family)
+		case "ipv6":
+			ipFamilies = append(ipFamilies, ipv6Family)
+		default:
+			return false
 		}
 	}
-	return false
+	return true
 }

cloud-controller-manager/do/common.go

@@ -20,12 +20,20 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 
 	"github.com/digitalocean/godo"
 	v1 "k8s.io/api/core/v1"
 )
 
+type IPFamily string
+
+var ipFamilies []IPFamily
+
+const (
+	ipv4Family IPFamily = "ipv4"
+	ipv6Family IPFamily = "ipv6"
+)
+
 // apiResultsPerPage is the maximum page size that DigitalOcean's api supports.
 const apiResultsPerPage = 200
 
@@ -130,20 +138,31 @@ func nodeAddresses(droplet *godo.Droplet) ([]v1.NodeAddress, error) {
 	var addresses []v1.NodeAddress
 	addresses = append(addresses, v1.NodeAddress{Type: v1.NodeHostName, Address: droplet.Name})
 
-	for _, i := range strings.Split(ipFamilies, ",") {
-		addr, err := discoverAddress(droplet, i)
+	// default case when DO_IP_ADDR_FAMILIES is not set
+	if ipFamilies == nil {
+		addr, err := discoverAddress(droplet, ipv4Family)
 		if err != nil {
-			return nil, fmt.Errorf("could not get addresses for %s : %v", i, err)
+			return nil, fmt.Errorf("could not get addresses for %s : %v", ipv4Family, err)
 		}
 		addresses = append(addresses, addr...)
+	} else {
+		for _, i := range ipFamilies {
+			addr, err := discoverAddress(droplet, i)
+			if err != nil {
+				return nil, fmt.Errorf("could not get addresses for %s : %v", i, err)
+			}
+			addresses = append(addresses, addr...)
+		}
 	}
 
 	return addresses, nil
 }
 
-func discoverAddress(droplet *godo.Droplet, family string) ([]v1.NodeAddress, error) {
+func discoverAddress(droplet *godo.Droplet, family IPFamily) ([]v1.NodeAddress, error) {
 	var addresses []v1.NodeAddress
-	if family == "ipv4" || family == "" {
+
+	switch family {
+	case ipv4Family:
 		privateIP, err := droplet.PrivateIPv4()
 		if err != nil || privateIP == "" {
 			return nil, fmt.Errorf("could not get private ip: %v", err)
@@ -156,15 +175,13 @@ func discoverAddress(droplet *godo.Droplet, family string) ([]v1.NodeAddress, er
 		}
 		addresses = append(addresses, v1.NodeAddress{Type: v1.NodeExternalIP, Address: publicIP})
 		return addresses, nil
-	}
-	if family == "ipv6" {
+	case ipv6Family:
 		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
 	}
-
 	return addresses, nil
 }

cloud-controller-manager/do/droplets_test.go

@@ -199,11 +199,11 @@ func TestNodeAddresses(t *testing.T) {
 			Address: "99.99.99.99",
 		},
 		{
-			Type:    "public",
+			Type:    v1.NodeExternalIP,
 			Address: "2a01::10",
 		},
 	}
-
+	ipFamilies = []IPFamily{ipv4Family, ipv6Family}
 	addresses, err := instances.NodeAddresses(context.TODO(), "test-droplet")
 
 	if !reflect.DeepEqual(addresses, expectedAddresses) {
@@ -238,12 +238,8 @@ func TestNodeAddressesByProviderID(t *testing.T) {
 			Type:    v1.NodeExternalIP,
 			Address: "99.99.99.99",
 		},
-		{
-			Type:    "public",
-			Address: "2a01::10",
-		},
 	}
-
+	ipFamilies = []IPFamily{ipv4Family}
 	addresses, err := instances.NodeAddressesByProviderID(context.TODO(), "digitalocean://123")
 
 	if !reflect.DeepEqual(addresses, expectedAddresses) {