Repository: hetznercloud/hcloud-cloud-controller-manager
Pull request #237 (2021-08-24 21:57:29)

Bring IPv6 flag in line with private ingress flag
To implement my own feature request #233 and inspired by @fynluk's take in #236

Unfortunately #236 doesn't work for me, so here's my version.

This models the HCLOUD_LOAD_BALANCERS_DISABLE_IPV6 feature more closely
after how HCLOUD_LOAD_BALANCERS_DISABLE_PRIVATE_INGRESS is implemented.

@LKaemmerling @fhofherr I noticed EnsureLoadBalancer() returns the private ingress if enabled while GetLoadBalancer() does not. Is this intentional?

LKaemmerling: Hey @mstarostik,

good job! I like the idea. I tested it locally and it works well. I also fixed the style tests.  

For EnsureLoadBalancer:
 I will have a look, on the first view it looks like this is wrong behavior, but as k8s always calls the GetLoadBalancer (even after creation) it shouldn't affect the cluster.

Patch:
diff --git a/hcloud/cloud.go b/hcloud/cloud.go
index 36d34816..57e1a833 100644
--- a/hcloud/cloud.go
+++ b/hcloud/cloud.go
@@ -45,6 +45,7 @@ const (
 	hcloudLoadBalancersNetworkZone           = "HCLOUD_LOAD_BALANCERS_NETWORK_ZONE"
 	hcloudLoadBalancersDisablePrivateIngress = "HCLOUD_LOAD_BALANCERS_DISABLE_PRIVATE_INGRESS"
 	hcloudLoadBalancersUsePrivateIP          = "HCLOUD_LOAD_BALANCERS_USE_PRIVATE_IP"
+	hcloudLoadBalancersDisableIPv6           = "HCLOUD_LOAD_BALANCERS_DISABLE_IPV6"
 	nodeNameENVVar                           = "NODE_NAME"
 	providerName                             = "hcloud"
 	providerVersion                          = "v1.9.1"
@@ -121,7 +122,7 @@ func newCloud(config io.Reader) (cloudprovider.Interface, error) {
 		return nil, fmt.Errorf("%s: %w", op, err)
 	}
 
-	lbOpsDefaults, lbDisablePrivateIngress, err := loadBalancerDefaultsFromEnv()
+	lbOpsDefaults, lbDisablePrivateIngress, lbDisableIPv6, err := loadBalancerDefaultsFromEnv()
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", op, err)
 	}
@@ -137,7 +138,7 @@ func newCloud(config io.Reader) (cloudprovider.Interface, error) {
 		Defaults:      lbOpsDefaults,
 	}
 
-	loadBalancers := newLoadBalancers(lbOps, &client.Action, lbDisablePrivateIngress)
+	loadBalancers := newLoadBalancers(lbOps, &client.Action, lbDisablePrivateIngress, lbDisableIPv6)
 	if os.Getenv(hcloudLoadBalancersEnabledENVVar) == "false" {
 		loadBalancers = nil
 	}
@@ -203,28 +204,33 @@ func (c *cloud) HasClusterID() bool {
 	return false
 }
 
-func loadBalancerDefaultsFromEnv() (hcops.LoadBalancerDefaults, bool, error) {
+func loadBalancerDefaultsFromEnv() (hcops.LoadBalancerDefaults, bool, bool, error) {
 	defaults := hcops.LoadBalancerDefaults{
 		Location:    os.Getenv(hcloudLoadBalancersLocation),
 		NetworkZone: os.Getenv(hcloudLoadBalancersNetworkZone),
 	}
 
 	if defaults.Location != "" && defaults.NetworkZone != "" {
-		return defaults, false, errors.New(
+		return defaults, false, false, errors.New(
 			"HCLOUD_LOAD_BALANCERS_LOCATION/HCLOUD_LOAD_BALANCERS_NETWORK_ZONE: Only one of these can be set")
 	}
 
 	disablePrivateIngress, err := getEnvBool(hcloudLoadBalancersDisablePrivateIngress)
 	if err != nil {
-		return defaults, false, err
+		return defaults, false, false, err
+	}
+
+	disableIPv6, err := getEnvBool(hcloudLoadBalancersDisableIPv6)
+	if err != nil {
+		return defaults, false, false, err
 	}
 
 	defaults.UsePrivateIP, err = getEnvBool(hcloudLoadBalancersUsePrivateIP)
 	if err != nil {
-		return defaults, false, err
+		return defaults, false, false, err
 	}
 
-	return defaults, disablePrivateIngress, nil
+	return defaults, disablePrivateIngress, disableIPv6, nil
 }
 
 // serverIsAttachedToNetwork checks if the server where the master is running on is attached to the configured private network
diff --git a/hcloud/load_balancers.go b/hcloud/load_balancers.go
index 544c3b43..e710ba98 100644
--- a/hcloud/load_balancers.go
+++ b/hcloud/load_balancers.go
@@ -30,13 +30,15 @@ type loadBalancers struct {
 	lbOps                        LoadBalancerOps
 	ac                           hcops.HCloudActionClient // Deprecated: should only be referenced by hcops types
 	disablePrivateIngressDefault bool
+	disableIPv6Default           bool
 }
 
-func newLoadBalancers(lbOps LoadBalancerOps, ac hcops.HCloudActionClient, disablePrivateIngressDefault bool) *loadBalancers {
+func newLoadBalancers(lbOps LoadBalancerOps, ac hcops.HCloudActionClient, disablePrivateIngressDefault bool, disableIPv6Default bool) *loadBalancers {
 	return &loadBalancers{
 		lbOps:                        lbOps,
 		ac:                           ac,
 		disablePrivateIngressDefault: disablePrivateIngressDefault,
+		disableIPv6Default:           disableIPv6Default,
 	}
 }
 
@@ -65,8 +67,8 @@ func (l *loadBalancers) GetLoadBalancer(
 		},
 	}
 
-	disableIPV6, err := annotation.LBIPv6Disabled.BoolFromService(service)
-	if err != nil && !errors.Is(err, annotation.ErrNotSet) {
+	disableIPV6, err := l.getDisableIPv6(service)
+	if err != nil {
 		return nil, false, fmt.Errorf("%s: %v", op, err)
 	}
 	if !disableIPV6 {
@@ -178,9 +180,9 @@ func (l *loadBalancers) EnsureLoadBalancer(
 	if !disablePubNet {
 		ingress = append(ingress, v1.LoadBalancerIngress{IP: lb.PublicNet.IPv4.IP.String()})
 
-		disableIPV6, err := annotation.LBIPv6Disabled.BoolFromService(svc)
-		if err != nil && !errors.Is(err, annotation.ErrNotSet) {
-			return nil, fmt.Errorf("%s: %v", op, err)
+		disableIPV6, err := l.getDisableIPv6(svc)
+		if err != nil {
+			return nil, fmt.Errorf("%s: %w", op, err)
 		}
 		if !disableIPV6 {
 			ingress = append(ingress, v1.LoadBalancerIngress{IP: lb.PublicNet.IPv6.IP.String()})
@@ -211,6 +213,17 @@ func (l *loadBalancers) getDisablePrivateIngress(svc *v1.Service) (bool, error)
 	return false, err
 }
 
+func (l *loadBalancers) getDisableIPv6(svc *v1.Service) (bool, error) {
+	disable, err := annotation.LBIPv6Disabled.BoolFromService(svc)
+	if err == nil {
+		return disable, nil
+	}
+	if errors.Is(err, annotation.ErrNotSet) {
+		return l.disableIPv6Default, nil
+	}
+	return false, err
+}
+
 func (l *loadBalancers) UpdateLoadBalancer(
 	ctx context.Context, clusterName string, svc *v1.Service, nodes []*v1.Node,
 ) error {


Test patch:
diff --git a/hcloud/cloud_test.go b/hcloud/cloud_test.go
index 5e7e1adf..7635f683 100644
--- a/hcloud/cloud_test.go
+++ b/hcloud/cloud_test.go
@@ -265,6 +265,7 @@ func TestLoadBalancerDefaultsFromEnv(t *testing.T) {
 		env                      map[string]string
 		expDefaults              hcops.LoadBalancerDefaults
 		expDisablePrivateIngress bool
+		expDisableIPv6           bool
 		expErr                   string
 	}{
 		{
@@ -280,6 +281,7 @@ func TestLoadBalancerDefaultsFromEnv(t *testing.T) {
 			env: map[string]string{
 				"HCLOUD_LOAD_BALANCERS_LOCATION":                "hel1",
 				"HCLOUD_LOAD_BALANCERS_DISABLE_PRIVATE_INGRESS": "true",
+				"HCLOUD_LOAD_BALANCERS_DISABLE_IPV6":            "true",
 				"HCLOUD_LOAD_BALANCERS_USE_PRIVATE_IP":          "true",
 			},
 			expDefaults: hcops.LoadBalancerDefaults{
@@ -287,6 +289,7 @@ func TestLoadBalancerDefaultsFromEnv(t *testing.T) {
 				UsePrivateIP: true,
 			},
 			expDisablePrivateIngress: true,
+			expDisableIPv6:           true,
 		},
 		{
 			name: "Network zone set",
@@ -312,6 +315,13 @@ func TestLoadBalancerDefaultsFromEnv(t *testing.T) {
 			},
 			expErr: `HCLOUD_LOAD_BALANCERS_DISABLE_PRIVATE_INGRESS: strconv.ParseBool: parsing "invalid": invalid syntax`,
 		},
+		{
+			name: "Invalid DISABLE_IPV6",
+			env: map[string]string{
+				"HCLOUD_LOAD_BALANCERS_DISABLE_IPV6": "invalid",
+			},
+			expErr: `HCLOUD_LOAD_BALANCERS_DISABLE_IPV6: strconv.ParseBool: parsing "invalid": invalid syntax`,
+		},
 		{
 			name: "Invalid USE_PRIVATE_IP",
 			env: map[string]string{
@@ -347,7 +357,7 @@ func TestLoadBalancerDefaultsFromEnv(t *testing.T) {
 				}
 			}()
 
-			defaults, disablePrivateIngress, err := loadBalancerDefaultsFromEnv()
+			defaults, disablePrivateIngress, disableIPv6, err := loadBalancerDefaultsFromEnv()
 
 			if c.expErr != "" {
 				assert.EqualError(t, err, c.expErr)
@@ -356,6 +366,7 @@ func TestLoadBalancerDefaultsFromEnv(t *testing.T) {
 			assert.NoError(t, err)
 			assert.Equal(t, c.expDefaults, defaults)
 			assert.Equal(t, c.expDisablePrivateIngress, disablePrivateIngress)
+			assert.Equal(t, c.expDisableIPv6, disableIPv6)
 		})
 	}
 }
diff --git a/hcloud/testing.go b/hcloud/testing.go
index cd8bc55f..2883e4fc 100644
--- a/hcloud/testing.go
+++ b/hcloud/testing.go
@@ -74,6 +74,7 @@ type LoadBalancerTestCase struct {
 	ServiceUID                   string
 	ServiceAnnotations           map[annotation.Name]interface{}
 	DisablePrivateIngressDefault bool
+	DisableIPv6Default           bool
 	Nodes                        []*v1.Node
 	LB                           *hcloud.LoadBalancer
 	LBCreateResult               *hcloud.LoadBalancerCreateResult
@@ -123,7 +124,7 @@ func (tt *LoadBalancerTestCase) run(t *testing.T) {
 		tt.Mock(t, tt)
 	}
 
-	tt.LoadBalancers = newLoadBalancers(tt.LBOps, tt.ActionClient, tt.DisablePrivateIngressDefault)
+	tt.LoadBalancers = newLoadBalancers(tt.LBOps, tt.ActionClient, tt.DisablePrivateIngressDefault, tt.DisableIPv6Default)
 	tt.Perform(t, tt)
 
 	tt.LBOps.AssertExpectations(t)