Skip to content

Commit

Permalink
feat(service): Specify private ip for loadbalancer (hetznercloud#724)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@lhp-nemlig
lhp-nemlig committed Sep 3, 2024
1 parent 176990a commit 4801f0e
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 79 deletions.
15 changes: 15 additions & 0 deletions hcloud/load_balancers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions internal/annotation/load_balancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
51 changes: 30 additions & 21 deletions internal/hcops/load_balancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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",
Expand Down Expand Up @@ -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
}
}
Expand Down
123 changes: 65 additions & 58 deletions internal/hcops/load_balancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}{
Expand All @@ -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)
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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},
Expand Down

0 comments on commit 4801f0e

Please sign in to comment.