Repository: hetznercloud/hcloud-cloud-controller-manager
Pull request #724 (2024-08-19 13:07:22)

feat(service): Specify private ip for loadbalancer
Allows users to specify the IPv4 of the load balancer in the private network via the annotation "load-balancer.hetzner.cloud/private-ipv4"

Removes network logic from main load balancer creation, since the API does not support specifying the IP address there, and moves network logic to exclusively happen in AttachToNetwork and DetachFromNetwork.

I also added an extra LoadBalancer refresh in the main reconciliation loop, since changing the IP address of an SVC object causes network detach and subsequent attach, which in turn needs to cause the targets to be re-added to loadbalancer if they were private network targets.

Unit tests pass and i have tested this in a few scenarios on a cluster of my own.


Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 0aa5f7d0..e48544c5 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -47,7 +47,7 @@ jobs:
 
       - uses: golangci/golangci-lint-action@v6
         with:
-          version: v1.60.1 # renovate: datasource=github-releases depName=golangci/golangci-lint
+          version: v1.60.3 # renovate: datasource=github-releases depName=golangci/golangci-lint
 
           # In general linting is quite fast with warm caches, but a fresh run might take some time.
           args: --timeout 5m
diff --git a/.goreleaser.yml b/.goreleaser.yml
index 21f7beb4..cbfcd8f0 100644
--- a/.goreleaser.yml
+++ b/.goreleaser.yml
@@ -23,6 +23,7 @@ builds:
       - -s
       - -w
       - -X github.com/hetznercloud/hcloud-cloud-controller-manager/hcloud.providerVersion={{ if not .IsSnapshot }}v{{ end }}{{ .Version }}
+      - -X k8s.io/component-base/version.gitVersion={{ if not .IsSnapshot }}v{{ end }}{{ .Version }}
 
 dockers:
   - build_flag_templates: [--platform=linux/amd64]
diff --git a/go.mod b/go.mod
index f31bd450..fb8f08d5 100644
--- a/go.mod
+++ b/go.mod
@@ -6,7 +6,7 @@ toolchain go1.22.6
 
 require (
 	github.com/hetznercloud/hcloud-go/v2 v2.13.1
-	github.com/prometheus/client_golang v1.20.0
+	github.com/prometheus/client_golang v1.20.2
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.9.0
 	github.com/syself/hrobot-go v0.2.5
diff --git a/go.sum b/go.sum
index 7a5659f3..e0b88d35 100644
--- a/go.sum
+++ b/go.sum
@@ -178,6 +178,10 @@ github.com/prometheus/client_golang v1.19.1 h1:wZWJDwK+NameRJuPGDhlnFgx8e8HN3XHQ
 github.com/prometheus/client_golang v1.19.1/go.mod h1:mP78NwGzrVks5S2H6ab8+ZZGJLZUq1hoULYBAYBw1Ho=
 github.com/prometheus/client_golang v1.20.0 h1:jBzTZ7B099Rg24tny+qngoynol8LtVYlA2bqx3vEloI=
 github.com/prometheus/client_golang v1.20.0/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE=
+github.com/prometheus/client_golang v1.20.1 h1:IMJXHOD6eARkQpxo8KkhgEVFlBNm+nkrFUyGlIu7Na8=
+github.com/prometheus/client_golang v1.20.1/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE=
+github.com/prometheus/client_golang v1.20.2 h1:5ctymQzZlyOON1666svgwn3s6IKWgfbjsejTMiXIyjg=
+github.com/prometheus/client_golang v1.20.2/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE=
 github.com/prometheus/client_model v0.5.0 h1:VQw1hfvPvk3Uv6Qf29VrPF32JB6rtbgI6cYPYQjL0Qw=
 github.com/prometheus/client_model v0.5.0/go.mod h1:dTiFglRmd66nLR9Pv9f0mZi7B7fk5Pm3gvsjB5tr+kI=
 github.com/prometheus/client_model v0.6.1 h1:ZKSh/rekM+n3CeS952MLRAdFwIKqeY8b62p8ais2e9E=
diff --git a/hcloud/load_balancers.go b/hcloud/load_balancers.go
index 8393a4e6..7f95a16d 100644
--- a/hcloud/load_balancers.go
+++ b/hcloud/load_balancers.go
@@ -160,6 +160,21 @@ func (l *loadBalancers) EnsureLoadBalancer(
 	}
 	reload = reload || lbChanged
 
+	// Reload early here if reload is true.
+	// If the load balancer private network ip changed,
+	// the load balancer would be detached and re-attached to the network
+	// As a result all of the private network targets would have been
+	// removed and we should make sure the lb state here matches the actual
+	// lb state so that we can re-attach the targets if needed
+	if reload {
+		klog.InfoS("reload HC Load Balancer", "op", op, "loadBalancerID", lb.ID)
+		lb, err = l.lbOps.GetByID(ctx, lb.ID)
+		if err != nil {
+			return nil, fmt.Errorf("%s: %w", op, err)
+		}
+		reload = false
+	}
+
 	servicesChanged, err := l.lbOps.ReconcileHCLBServices(ctx, lb, svc)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", op, err)
diff --git a/internal/annotation/load_balancer.go b/internal/annotation/load_balancer.go
index 0e5fd866..dc5f5452 100644
--- a/internal/annotation/load_balancer.go
+++ b/internal/annotation/load_balancer.go
@@ -54,6 +54,10 @@ const (
 	// Load Balancer server targets.
 	LBUsePrivateIP Name = "load-balancer.hetzner.cloud/use-private-ip"
 
+	// LBPrivateIPv4 specifies the IPv4 address to assign to the load balancer in the
+	// private network that it's attached to.
+	LBPrivateIPv4 Name = "load-balancer.hetzner.cloud/private-ipv4"
+
 	// LBHostname specifies the hostname of the Load Balancer. This will be
 	// used as ingress address instead of the Load Balancer IP addresses if
 	// specified.
diff --git a/internal/hcops/load_balancer.go b/internal/hcops/load_balancer.go
index 5f1a0f68..04b3f59d 100644
--- a/internal/hcops/load_balancer.go
+++ b/internal/hcops/load_balancer.go
@@ -203,16 +203,6 @@ func (l *LoadBalancerOps) Create(
 		opts.Algorithm = &hcloud.LoadBalancerAlgorithm{Type: algType}
 	}
 
-	if l.NetworkID > 0 {
-		nw, _, err := l.NetworkClient.GetByID(ctx, l.NetworkID)
-		if err != nil {
-			return nil, fmt.Errorf("%s: get network %d: %w", op, l.NetworkID, err)
-		}
-		if nw == nil {
-			return nil, fmt.Errorf("%s: get network %d: %w", op, l.NetworkID, ErrNotFound)
-		}
-		opts.Network = nw
-	}
 	disablePubIface, err := annotation.LBDisablePublicNetwork.BoolFromService(svc)
 	if err != nil && !errors.Is(err, annotation.ErrNotSet) {
 		return nil, fmt.Errorf("%s: %w", op, err)
@@ -289,13 +279,13 @@ func (l *LoadBalancerOps) ReconcileHCLB(ctx context.Context, lb *hcloud.LoadBala
 	}
 	changed = changed || typeChanged
 
-	networkDetached, err := l.detachFromNetwork(ctx, lb)
+	networkDetached, err := l.detachFromNetwork(ctx, lb, svc)
 	if err != nil {
 		return changed, fmt.Errorf("%s: %w", op, err)
 	}
 	changed = changed || networkDetached
 
-	networkAttached, err := l.attachToNetwork(ctx, lb)
+	networkAttached, err := l.attachToNetwork(ctx, lb, svc)
 	if err != nil {
 		return changed, fmt.Errorf("%s: %w", op, err)
 	}
@@ -457,19 +447,21 @@ func (l *LoadBalancerOps) changeType(ctx context.Context, lb *hcloud.LoadBalance
 	return true, nil
 }
 
-func (l *LoadBalancerOps) detachFromNetwork(ctx context.Context, lb *hcloud.LoadBalancer) (bool, error) {
+func (l *LoadBalancerOps) detachFromNetwork(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) {
 	const op = "hcops/LoadBalancerOps.detachFromNetwork"
 	metrics.OperationCalled.WithLabelValues(op).Inc()
 
 	var changed bool
 
+	privateIPv4, privateIPv4configured := annotation.LBPrivateIPv4.StringFromService(svc)
 	for _, lbpn := range lb.PrivateNet {
 		// Don't detach the Load Balancer from the network it is supposed to
-		// be attached to.
-		if l.NetworkID == lbpn.Network.ID {
+		// be attached to and the current private IP of the load balancer matches
+		// the one configured by the user, if one is configured.
+		if l.NetworkID == lbpn.Network.ID && (!privateIPv4configured || privateIPv4 == lbpn.IP.String()) {
 			continue
 		}
-		klog.InfoS("detach from network", "op", op, "loadBalancerID", lb.ID, "networkID", lbpn.Network.ID)
+		klog.InfoS("detach from network", "op", op, "loadBalancerID", lb.ID, "networkID", lbpn.Network.ID, "privateIPv4", lbpn.IP.String())
 
 		opts := hcloud.LoadBalancerDetachFromNetworkOpts{Network: lbpn.Network}
 		a, _, err := l.LBClient.DetachFromNetwork(ctx, lb, opts)
@@ -484,16 +476,30 @@ func (l *LoadBalancerOps) detachFromNetwork(ctx context.Context, lb *hcloud.Load
 	return changed, nil
 }
 
-func (l *LoadBalancerOps) attachToNetwork(ctx context.Context, lb *hcloud.LoadBalancer) (bool, error) {
+func (l *LoadBalancerOps) attachToNetwork(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) {
 	const op = "hcops/LoadBalancerOps.attachToNetwork"
 	metrics.OperationCalled.WithLabelValues(op).Inc()
 
+	privateIPv4String, privateIPv4configured := annotation.LBPrivateIPv4.StringFromService(svc)
 	// Don't attach the Load Balancer if network is not set, or the load
 	// balancer is already attached.
-	if l.NetworkID == 0 || lbAttached(lb, l.NetworkID) {
+	if l.NetworkID == 0 || lbAttached(lb, l.NetworkID, privateIPv4String) {
 		return false, nil
 	}
-	klog.InfoS("attach to network", "op", op, "loadBalancerID", lb.ID, "networkID", l.NetworkID)
+
+	var privateIPv4 net.IP
+	if privateIPv4configured {
+		privateIPv4 = net.ParseIP(privateIPv4String)
+		if privateIPv4 == nil {
+			return false, fmt.Errorf("%s: %w", op, fmt.Errorf("could not parse private IPv4 '%s'", privateIPv4))
+		}
+	}
+
+	if privateIPv4 != nil {
+		klog.InfoS("attach to network", "op", op, "loadBalancerID", lb.ID, "networkID", l.NetworkID, "privateIP", privateIPv4)
+	} else {
+		klog.InfoS("attach to network", "op", op, "loadBalancerID", lb.ID, "networkID", l.NetworkID)
+	}
 
 	nw, _, err := l.NetworkClient.GetByID(ctx, l.NetworkID)
 	if err != nil {
@@ -508,6 +514,9 @@ func (l *LoadBalancerOps) attachToNetwork(ctx context.Context, lb *hcloud.LoadBa
 		retryDelay = time.Second
 	}
 	opts := hcloud.LoadBalancerAttachToNetworkOpts{Network: nw}
+	if privateIPv4 != nil {
+		opts.IP = privateIPv4
+	}
 	a, _, err := l.LBClient.AttachToNetwork(ctx, lb, opts)
 	if hcloud.IsError(err, hcloud.ErrorCodeConflict, hcloud.ErrorCodeLocked) {
 		klog.InfoS("retry due to conflict or lock",
@@ -1342,9 +1351,9 @@ func (b *hclbServiceOptsBuilder) buildUpdateServiceOpts() (hcloud.LoadBalancerUp
 	return opts, nil
 }
 
-func lbAttached(lb *hcloud.LoadBalancer, nwID int64) bool {
+func lbAttached(lb *hcloud.LoadBalancer, nwID int64, privateIPv4 string) bool {
 	for _, nw := range lb.PrivateNet {
-		if nw.Network.ID == nwID {
+		if nw.Network.ID == nwID && (privateIPv4 == "" || privateIPv4 == nw.IP.String()) {
 			return true
 		}
 	}


Test patch:
diff --git a/.github/workflows/test_e2e.yml b/.github/workflows/test_e2e.yml
index fbdf631f..91d6de78 100644
--- a/.github/workflows/test_e2e.yml
+++ b/.github/workflows/test_e2e.yml
@@ -54,7 +54,7 @@ jobs:
             skaffold
           helm: v3.15.4 # renovate: datasource=github-releases depName=helm/helm
           kubectl: v1.31.0 # renovate: datasource=github-releases depName=kubernetes/kubernetes
-          skaffold: v2.13.1 # renovate: datasource=github-releases depName=GoogleContainerTools/skaffold
+          skaffold: v2.13.2 # renovate: datasource=github-releases depName=GoogleContainerTools/skaffold
 
       - name: Install k3sup
         run: |
@@ -161,7 +161,7 @@ jobs:
             skaffold
           helm: v3.15.4 # renovate: datasource=github-releases depName=helm/helm
           kubectl: v1.31.0 # renovate: datasource=github-releases depName=kubernetes/kubernetes
-          skaffold: v2.13.1 # renovate: datasource=github-releases depName=GoogleContainerTools/skaffold
+          skaffold: v2.13.2 # renovate: datasource=github-releases depName=GoogleContainerTools/skaffold
 
       - name: Install k3sup
         run: |
diff --git a/internal/hcops/load_balancer_test.go b/internal/hcops/load_balancer_test.go
index 687d7531..8c735729 100644
--- a/internal/hcops/load_balancer_test.go
+++ b/internal/hcops/load_balancer_test.go
@@ -438,56 +438,6 @@ func TestLoadBalancerOps_Create(t *testing.T) {
 			},
 			err: fmt.Errorf("hcops/LoadBalancerOps.Create: annotation/Name.LBAlgorithmTypeFromService: annotation/validateAlgorithmType: invalid: invalidtype"),
 		},
-		{
-			name: "attach Load Balancer to private network",
-			serviceAnnotations: map[annotation.Name]interface{}{
-				annotation.LBLocation: "nbg1",
-			},
-			createOpts: hcloud.LoadBalancerCreateOpts{
-				Name:             "lb-with-priv",
-				LoadBalancerType: &hcloud.LoadBalancerType{Name: "lb11"},
-				Location:         &hcloud.Location{Name: "nbg1"},
-				Network: &hcloud.Network{
-					ID:   4711,
-					Name: "some-network",
-				},
-				Labels: map[string]string{
-					hcops.LabelServiceUID: "lb-with-priv-uid",
-				},
-			},
-			mock: func(_ *testing.T, tt *testCase, fx *hcops.LoadBalancerOpsFixture) {
-				fx.LBOps.NetworkID = tt.createOpts.Network.ID
-				fx.NetworkClient.
-					On("GetByID", fx.Ctx, fx.LBOps.NetworkID).
-					Return(tt.createOpts.Network, nil, nil)
-				action := fx.MockCreate(tt.createOpts, tt.lb, nil)
-				fx.MockGetByID(tt.lb, nil)
-				fx.ActionClient.On("WaitFor", fx.Ctx, action).Return(nil)
-			},
-			lb: &hcloud.LoadBalancer{ID: 5},
-		},
-		{
-			name: "fail if network could not be found",
-			serviceAnnotations: map[annotation.Name]interface{}{
-				annotation.LBLocation: "nbg1",
-			},
-			mock: func(_ *testing.T, _ *testCase, fx *hcops.LoadBalancerOpsFixture) {
-				fx.LBOps.NetworkID = 4711
-				fx.NetworkClient.On("GetByID", fx.Ctx, fx.LBOps.NetworkID).Return(nil, nil, nil)
-			},
-			err: fmt.Errorf("hcops/LoadBalancerOps.Create: get network %d: %w", 4711, hcops.ErrNotFound),
-		},
-		{
-			name: "fail if looking for network returns an error",
-			serviceAnnotations: map[annotation.Name]interface{}{
-				annotation.LBLocation: "nbg1",
-			},
-			mock: func(_ *testing.T, _ *testCase, fx *hcops.LoadBalancerOpsFixture) {
-				fx.LBOps.NetworkID = 4712
-				fx.NetworkClient.On("GetByID", fx.Ctx, fx.LBOps.NetworkID).Return(nil, nil, errTestLbClient)
-			},
-			err: fmt.Errorf("hcops/LoadBalancerOps.Create: get network %d: %w", 4712, errTestLbClient),
-		},
 		{
 			name: "disable public interface",
 			serviceAnnotations: map[annotation.Name]interface{}{
@@ -499,19 +449,11 @@ func TestLoadBalancerOps_Create(t *testing.T) {
 				LoadBalancerType: &hcloud.LoadBalancerType{Name: "lb11"},
 				Location:         &hcloud.Location{Name: "nbg1"},
 				PublicInterface:  hcloud.Ptr(false),
-				Network: &hcloud.Network{
-					ID:   4711,
-					Name: "some-network",
-				},
 				Labels: map[string]string{
 					hcops.LabelServiceUID: "lb-with-priv-uid",
 				},
 			},
 			mock: func(_ *testing.T, tt *testCase, fx *hcops.LoadBalancerOpsFixture) {
-				fx.LBOps.NetworkID = tt.createOpts.Network.ID
-
-				fx.NetworkClient.On("GetByID", fx.Ctx, fx.LBOps.NetworkID).Return(tt.createOpts.Network, nil, nil)
-
 				action := fx.MockCreate(tt.createOpts, tt.lb, nil)
 				fx.MockGetByID(tt.lb, nil)
 				fx.ActionClient.On("WaitFor", fx.Ctx, action).Return(nil)
@@ -868,6 +810,48 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) {
 				assert.True(t, changed)
 			},
 		},
+		{
+			name: "reattach Load Balancer to network because private ipv4 annotation changed",
+			initialLB: &hcloud.LoadBalancer{
+				ID: 4,
+				PrivateNet: []hcloud.LoadBalancerPrivateNet{
+					{
+						Network: &hcloud.Network{ID: 14, Name: "some-network"},
+						IP:      net.ParseIP("10.10.10.3"),
+					},
+				},
+			},
+			serviceAnnotations: map[annotation.Name]interface{}{
+				annotation.LBPrivateIPv4: "10.10.10.2",
+			},
+			mock: func(_ *testing.T, tt *LBReconcilementTestCase) {
+				nw := &hcloud.Network{ID: 14, Name: "some-network"}
+				detachOpts := hcloud.LoadBalancerDetachFromNetworkOpts{
+					Network: nw,
+				}
+
+				tt.fx.LBOps.NetworkID = 14
+
+				attachOpts := hcloud.LoadBalancerAttachToNetworkOpts{
+					Network: nw,
+					IP:      net.ParseIP("10.10.10.2"),
+				}
+
+				detachAction := &hcloud.Action{ID: rand.Int63()}
+				tt.fx.LBClient.On("DetachFromNetwork", tt.fx.Ctx, tt.initialLB, detachOpts).Return(detachAction, nil, nil)
+				tt.fx.ActionClient.On("WaitFor", tt.fx.Ctx, detachAction).Return(nil)
+
+				tt.fx.NetworkClient.On("GetByID", tt.fx.Ctx, nw.ID).Return(nw, nil, nil)
+				attachAction := &hcloud.Action{ID: rand.Int63()}
+				tt.fx.LBClient.On("AttachToNetwork", tt.fx.Ctx, tt.initialLB, attachOpts).Return(attachAction, nil, nil)
+				tt.fx.ActionClient.On("WaitFor", tt.fx.Ctx, attachAction).Return(nil)
+			},
+			perform: func(t *testing.T, tt *LBReconcilementTestCase) {
+				changed, err := tt.fx.LBOps.ReconcileHCLB(tt.fx.Ctx, tt.initialLB, tt.service)
+				assert.NoError(t, err)
+				assert.True(t, changed)
+			},
+		},
 		{
 			name: "don't detach Load Balancer from current network",
 			initialLB: &hcloud.LoadBalancer{
@@ -907,6 +891,29 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) {
 				assert.True(t, changed)
 			},
 		},
+		{
+			name:      "attach Load Balancer to network with specific IP",
+			initialLB: &hcloud.LoadBalancer{ID: 4},
+			serviceAnnotations: map[annotation.Name]interface{}{
+				annotation.LBPrivateIPv4: "10.10.10.2",
+			},
+			mock: func(_ *testing.T, tt *LBReconcilementTestCase) {
+				nw := &hcloud.Network{ID: 15, Name: "some-network"}
+				tt.fx.NetworkClient.On("GetByID", tt.fx.Ctx, nw.ID).Return(nw, nil, nil)
+
+				tt.fx.LBOps.NetworkID = nw.ID
+
+				opts := hcloud.LoadBalancerAttachToNetworkOpts{Network: nw, IP: net.ParseIP("10.10.10.2")}
+				action := &hcloud.Action{ID: rand.Int63()}
+				tt.fx.LBClient.On("AttachToNetwork", tt.fx.Ctx, tt.initialLB, opts).Return(action, nil, nil)
+				tt.fx.ActionClient.On("WaitFor", tt.fx.Ctx, action).Return(nil)
+			},
+			perform: func(t *testing.T, tt *LBReconcilementTestCase) {
+				changed, err := tt.fx.LBOps.ReconcileHCLB(tt.fx.Ctx, tt.initialLB, tt.service)
+				assert.NoError(t, err)
+				assert.True(t, changed)
+			},
+		},
 		{
 			name:      "re-try attach to network on conflict",
 			initialLB: &hcloud.LoadBalancer{ID: 5},