Skip to content

Commit f430ff5

Browse files
lukasmetznerapricote
lukasmetzner
and
apricote
authored
refactor: remove lb to service annotations (#933)
The cloudprovider interface defines the service object as read-only. Therefore, we should not, and don't need to, copy data from the load balancer to the service annotations. --------- Co-authored-by: Julian Tölle <julian.toelle@hetzner-cloud.de>
1 parent 0f87222 commit f430ff5

11 files changed

Lines changed: 174 additions & 672 deletions

‎hcloud/load_balancers.go‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -196,10 +196,6 @@ func (l *loadBalancers) EnsureLoadBalancer(
196196
}
197197
}
198198

199-
if err := annotation.LBToService(svc, lb); err != nil {
200-
return nil, fmt.Errorf("%s: %w", op, err)
201-
}
202-
203199
// Either set the Hostname or the IPs (below).
204200
// See: https://github.com/kubernetes/kubernetes/issues/66607
205201
if v, ok := annotation.LBHostname.StringFromService(svc); ok {
@@ -232,12 +228,7 @@ func (l *loadBalancers) buildLoadBalancerStatusIngress(lb *hcloud.LoadBalancer,
232228
ipMode = corev1.LoadBalancerIPModeProxy
233229
}
234230

235-
disablePubNet, err := annotation.LBDisablePublicNetwork.BoolFromService(svc)
236-
if err != nil && !errors.Is(err, annotation.ErrNotSet) {
237-
return nil, fmt.Errorf("%s: %w", op, err)
238-
}
239-
240-
if !disablePubNet {
231+
if lb.PublicNet.Enabled {
241232
ingress = append(ingress, corev1.LoadBalancerIngress{
242233
IP: lb.PublicNet.IPv4.IP.String(),
243234
IPMode: &ipMode,

‎hcloud/load_balancers_test.go‎

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,16 @@ func TestLoadBalancers_GetLoadBalancer(t *testing.T) {
2929
{
3030
Name: "get load balancer without host name IPv6 disabled",
3131
ServiceUID: "1",
32-
ServiceAnnotations: map[annotation.Name]interface{}{
33-
annotation.LBIPv6Disabled: true,
32+
ServiceAnnotations: map[annotation.Name]string{
33+
annotation.LBIPv6Disabled: "true",
3434
},
3535
LB: &hcloud.LoadBalancer{
3636
ID: 1,
3737
Name: "no-host-name",
3838
PublicNet: hcloud.LoadBalancerPublicNet{
39-
IPv4: hcloud.LoadBalancerPublicNetIPv4{IP: net.ParseIP("1.2.3.4")},
39+
Enabled: true,
40+
IPv4: hcloud.LoadBalancerPublicNetIPv4{IP: net.ParseIP("1.2.3.4")},
41+
IPv6: hcloud.LoadBalancerPublicNetIPv6{IP: net.ParseIP("fe80::1")},
4042
},
4143
},
4244
Mock: func(_ *testing.T, tt *LoadBalancerTestCase) {
@@ -62,8 +64,9 @@ func TestLoadBalancers_GetLoadBalancer(t *testing.T) {
6264
ID: 1,
6365
Name: "no-host-name",
6466
PublicNet: hcloud.LoadBalancerPublicNet{
65-
IPv4: hcloud.LoadBalancerPublicNetIPv4{IP: net.ParseIP("1.2.3.4")},
66-
IPv6: hcloud.LoadBalancerPublicNetIPv6{IP: net.ParseIP("fe80::1")},
67+
Enabled: true,
68+
IPv4: hcloud.LoadBalancerPublicNetIPv4{IP: net.ParseIP("1.2.3.4")},
69+
IPv6: hcloud.LoadBalancerPublicNetIPv6{IP: net.ParseIP("fe80::1")},
6770
},
6871
},
6972
Mock: func(_ *testing.T, tt *LoadBalancerTestCase) {
@@ -91,7 +94,7 @@ func TestLoadBalancers_GetLoadBalancer(t *testing.T) {
9194
On("GetByK8SServiceUID", tt.Ctx, tt.Service).
9295
Return(tt.LB, nil)
9396
},
94-
ServiceAnnotations: map[annotation.Name]interface{}{
97+
ServiceAnnotations: map[annotation.Name]string{
9598
annotation.LBHostname: "hostname",
9699
},
97100
Perform: func(t *testing.T, tt *LoadBalancerTestCase) {
@@ -261,9 +264,9 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) {
261264
{
262265
Name: "public network only no ipv6",
263266
ServiceUID: "2",
264-
ServiceAnnotations: map[annotation.Name]interface{}{
267+
ServiceAnnotations: map[annotation.Name]string{
265268
annotation.LBName: "pub-net-only-no-ipv6",
266-
annotation.LBIPv6Disabled: true,
269+
annotation.LBIPv6Disabled: "true",
267270
},
268271
LB: &hcloud.LoadBalancer{
269272
ID: 1,
@@ -292,7 +295,7 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) {
292295
{
293296
Name: "public network only",
294297
ServiceUID: "2",
295-
ServiceAnnotations: map[annotation.Name]interface{}{
298+
ServiceAnnotations: map[annotation.Name]string{
296299
annotation.LBName: "pub-net-only",
297300
},
298301
LB: &hcloud.LoadBalancer{
@@ -325,7 +328,7 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) {
325328
Name: "attach Load Balancer to public and private network",
326329
NetworkID: 4711,
327330
ServiceUID: "3",
328-
ServiceAnnotations: map[annotation.Name]interface{}{
331+
ServiceAnnotations: map[annotation.Name]string{
329332
annotation.LBName: "with-priv-net",
330333
},
331334
LB: &hcloud.LoadBalancer{
@@ -368,7 +371,7 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) {
368371
Name: "disable private ingress via default",
369372
NetworkID: 4711,
370373
ServiceUID: "5",
371-
ServiceAnnotations: map[annotation.Name]interface{}{
374+
ServiceAnnotations: map[annotation.Name]string{
372375
annotation.LBName: "with-priv-net-no-priv-ingress",
373376
},
374377
DisablePrivateIngressDefault: true,
@@ -411,9 +414,9 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) {
411414
Name: "disable private ingress via annotation",
412415
NetworkID: 4711,
413416
ServiceUID: "5",
414-
ServiceAnnotations: map[annotation.Name]interface{}{
417+
ServiceAnnotations: map[annotation.Name]string{
415418
annotation.LBName: "with-priv-net-no-priv-ingress",
416-
annotation.LBDisablePrivateIngress: true,
419+
annotation.LBDisablePrivateIngress: "true",
417420
},
418421
LB: &hcloud.LoadBalancer{
419422
ID: 1,
@@ -454,9 +457,9 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) {
454457
Name: "attach Load Balancer to private network only",
455458
NetworkID: 4711,
456459
ServiceUID: "6",
457-
ServiceAnnotations: map[annotation.Name]interface{}{
460+
ServiceAnnotations: map[annotation.Name]string{
458461
annotation.LBName: "priv-net-only",
459-
annotation.LBDisablePublicNetwork: true,
462+
annotation.LBDisablePublicNetwork: "true",
460463
},
461464
LB: &hcloud.LoadBalancer{
462465
ID: 1,
@@ -508,7 +511,7 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) {
508511
Name: "attach Load Balancer to public and private network (with proxy protocol)",
509512
NetworkID: 4711,
510513
ServiceUID: "3",
511-
ServiceAnnotations: map[annotation.Name]interface{}{
514+
ServiceAnnotations: map[annotation.Name]string{
512515
annotation.LBName: "with-priv-net",
513516
annotation.LBSvcProxyProtocol: "true",
514517
},
@@ -558,7 +561,7 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) {
558561
{
559562
Name: "Load balancer unchanged",
560563
ServiceUID: "1",
561-
ServiceAnnotations: map[annotation.Name]interface{}{
564+
ServiceAnnotations: map[annotation.Name]string{
562565
annotation.LBName: "test-lb",
563566
},
564567
LB: &hcloud.LoadBalancer{
@@ -580,7 +583,7 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) {
580583
{
581584
Name: "Load balancer changed",
582585
ServiceUID: "2",
583-
ServiceAnnotations: map[annotation.Name]interface{}{
586+
ServiceAnnotations: map[annotation.Name]string{
584587
annotation.LBName: "test-lb",
585588
},
586589
LB: &hcloud.LoadBalancer{
@@ -603,7 +606,7 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) {
603606
{
604607
Name: "Load balancer targets changed",
605608
ServiceUID: "3",
606-
ServiceAnnotations: map[annotation.Name]interface{}{
609+
ServiceAnnotations: map[annotation.Name]string{
607610
annotation.LBName: "test-lb",
608611
},
609612
LB: &hcloud.LoadBalancer{
@@ -626,7 +629,7 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) {
626629
{
627630
Name: "Load balancer services changed",
628631
ServiceUID: "4",
629-
ServiceAnnotations: map[annotation.Name]interface{}{
632+
ServiceAnnotations: map[annotation.Name]string{
630633
annotation.LBName: "test-lb",
631634
},
632635
LB: &hcloud.LoadBalancer{
@@ -649,7 +652,7 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) {
649652
{
650653
Name: "fall back to load balancer name",
651654
ServiceUID: "5",
652-
ServiceAnnotations: map[annotation.Name]interface{}{
655+
ServiceAnnotations: map[annotation.Name]string{
653656
annotation.LBName: "pre-existing-lb",
654657
},
655658
LB: &hcloud.LoadBalancer{
@@ -681,7 +684,7 @@ func TestLoadBalancer_UpdateLoadBalancer(t *testing.T) {
681684
{
682685
Name: "Load Balancer not found",
683686
ServiceUID: "1",
684-
ServiceAnnotations: map[annotation.Name]interface{}{
687+
ServiceAnnotations: map[annotation.Name]string{
685688
annotation.LBName: "test-lb",
686689
},
687690
Mock: func(_ *testing.T, tt *LoadBalancerTestCase) {
@@ -696,7 +699,7 @@ func TestLoadBalancer_UpdateLoadBalancer(t *testing.T) {
696699
{
697700
Name: "calls all reconcilement ops",
698701
ServiceUID: "2",
699-
ServiceAnnotations: map[annotation.Name]interface{}{
702+
ServiceAnnotations: map[annotation.Name]string{
700703
annotation.LBName: "test-lb",
701704
},
702705
LB: &hcloud.LoadBalancer{
@@ -718,7 +721,7 @@ func TestLoadBalancer_UpdateLoadBalancer(t *testing.T) {
718721
{
719722
Name: "fall back to load balancer name",
720723
ServiceUID: "3",
721-
ServiceAnnotations: map[annotation.Name]interface{}{
724+
ServiceAnnotations: map[annotation.Name]string{
722725
annotation.LBName: "previously-created-lb",
723726
},
724727
LB: &hcloud.LoadBalancer{

‎hcloud/testing.go‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ type LoadBalancerTestCase struct {
2121
ClusterName string
2222
NetworkID int
2323
ServiceUID string
24-
ServiceAnnotations map[annotation.Name]interface{}
24+
ServiceAnnotations map[annotation.Name]string
2525
DisablePrivateIngressDefault bool
2626
DisableIPv6Default bool
2727
Nodes []*corev1.Node
@@ -54,12 +54,13 @@ func (tt *LoadBalancerTestCase) run(t *testing.T) {
5454
tt.ClusterName = "test-cluster"
5555
}
5656
tt.Service = &corev1.Service{
57-
ObjectMeta: metav1.ObjectMeta{UID: types.UID(tt.ServiceUID)},
57+
ObjectMeta: metav1.ObjectMeta{
58+
UID: types.UID(tt.ServiceUID),
59+
Annotations: map[string]string{},
60+
},
5861
}
5962
for k, v := range tt.ServiceAnnotations {
60-
if err := k.AnnotateService(tt.Service, v); err != nil {
61-
t.Fatal(err)
62-
}
63+
tt.Service.Annotations[string(k)] = v
6364
}
6465
if tt.Ctx == nil {
6566
tt.Ctx = context.Background()
@@ -77,7 +78,9 @@ func (tt *LoadBalancerTestCase) run(t *testing.T) {
7778
}
7879

7980
func RunLoadBalancerTests(t *testing.T, tests []LoadBalancerTestCase) {
81+
t.Helper()
82+
8083
for _, tt := range tests {
81-
tt.run(t)
84+
t.Run(tt.Name, func(t *testing.T) { tt.run(t) })
8285
}
8386
}

‎internal/annotation/load_balancer.go‎

Lines changed: 6 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,6 @@
11
package annotation
22

3-
import (
4-
"fmt"
5-
6-
corev1 "k8s.io/api/core/v1"
7-
8-
"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/metrics"
9-
"github.com/hetznercloud/hcloud-go/v2/hcloud"
10-
)
11-
123
const (
13-
// LBID is the ID assigned to the Hetzner Cloud Load Balancer by the
14-
// backend. Read-only.
15-
LBID Name = "load-balancer.hetzner.cloud/id"
16-
174
// LBPublicIPv4 is the public IPv4 address assigned to the Load Balancer by
185
// the backend. Read-only.
196
LBPublicIPv4 Name = "load-balancer.hetzner.cloud/ipv4"
@@ -209,95 +196,10 @@ const (
209196
// LBSvcHealthCheckHTTPStatusCodes is a comma separated list of HTTP status
210197
// codes which we expect.
211198
LBSvcHealthCheckHTTPStatusCodes Name = "load-balancer.hetzner.cloud/http-status-codes"
212-
)
213199

214-
// LBToService sets the relevant annotations on svc to their respective values
215-
// from lb.
216-
func LBToService(svc *corev1.Service, lb *hcloud.LoadBalancer) error {
217-
const op = "annotation/LBToService"
218-
metrics.OperationCalled.WithLabelValues(op).Inc()
219-
220-
sa := &serviceAnnotator{Svc: svc}
221-
222-
sa.Annotate(LBID, lb.ID)
223-
sa.Annotate(LBName, lb.Name)
224-
sa.Annotate(LBType, lb.LoadBalancerType.Name)
225-
sa.Annotate(LBAlgorithmType, lb.Algorithm.Type)
226-
sa.Annotate(LBLocation, lb.Location.Name)
227-
sa.Annotate(LBNetworkZone, lb.Location.NetworkZone)
228-
sa.Annotate(LBPublicIPv4, lb.PublicNet.IPv4.IP)
229-
sa.Annotate(LBPublicIPv6, lb.PublicNet.IPv6.IP)
230-
231-
for _, hclbService := range lb.Services {
232-
var found bool
233-
234-
// Find the HC Load Balancer service that matches our K8S service by
235-
// comparing the port numbers.
236-
for _, p := range svc.Spec.Ports {
237-
if hclbService.ListenPort == int(p.Port) {
238-
found = true
239-
break
240-
}
241-
}
242-
243-
// This hclbService does not match our K8S service. Continue with the
244-
// next one.
245-
if !found {
246-
continue
247-
}
248-
249-
// Once we found a matching service we copy its annotations.
250-
sa.Annotate(LBSvcProtocol, hclbService.Protocol)
251-
sa.Annotate(LBSvcProxyProtocol, hclbService.Proxyprotocol)
252-
253-
if isHTTP(hclbService) || isHTTPS(hclbService) {
254-
sa.Annotate(LBSvcHTTPCookieName, hclbService.HTTP.CookieName)
255-
sa.Annotate(LBSvcHTTPCookieLifetime, hclbService.HTTP.CookieLifetime)
256-
}
257-
258-
if isHTTPS(hclbService) {
259-
sa.Annotate(LBSvcRedirectHTTP, hclbService.HTTP.RedirectHTTP)
260-
sa.Annotate(LBSvcHTTPCertificates, hclbService.HTTP.Certificates)
261-
}
262-
263-
sa.Annotate(LBSvcHealthCheckProtocol, hclbService.HealthCheck.Protocol)
264-
sa.Annotate(LBSvcHealthCheckPort, hclbService.HealthCheck.Port)
265-
sa.Annotate(LBSvcHealthCheckInterval, hclbService.HealthCheck.Interval)
266-
sa.Annotate(LBSvcHealthCheckTimeout, hclbService.HealthCheck.Timeout)
267-
sa.Annotate(LBSvcHealthCheckRetries, hclbService.HealthCheck.Retries)
268-
269-
if isHTTPHealthCheck(hclbService) || isHTTPSHealthCheck(hclbService) {
270-
sa.Annotate(LBSvcHealthCheckHTTPDomain, hclbService.HealthCheck.HTTP.Domain)
271-
sa.Annotate(LBSvcHealthCheckHTTPPath, hclbService.HealthCheck.HTTP.Path)
272-
sa.Annotate(LBSvcHealthCheckHTTPStatusCodes, hclbService.HealthCheck.HTTP.StatusCodes)
273-
}
274-
if isHTTPSHealthCheck(hclbService) {
275-
sa.Annotate(LBSvcHealthCheckHTTPValidateCertificate, hclbService.HealthCheck.HTTP.TLS)
276-
}
277-
278-
// At most one service matches and we've already found it. There
279-
// is no need to bother with the remaining services.
280-
break
281-
}
282-
283-
if sa.Err != nil {
284-
return fmt.Errorf("%s: %w", op, sa.Err)
285-
}
286-
return nil
287-
}
288-
289-
func isHTTP(s hcloud.LoadBalancerService) bool {
290-
return s.Protocol == hcloud.LoadBalancerServiceProtocolHTTP
291-
}
292-
293-
func isHTTPHealthCheck(s hcloud.LoadBalancerService) bool {
294-
return s.HealthCheck.HTTP != nil && s.HealthCheck.Protocol == hcloud.LoadBalancerServiceProtocolHTTP
295-
}
296-
297-
func isHTTPS(s hcloud.LoadBalancerService) bool {
298-
return s.Protocol == hcloud.LoadBalancerServiceProtocolHTTPS
299-
}
300-
301-
func isHTTPSHealthCheck(s hcloud.LoadBalancerService) bool {
302-
return s.HealthCheck.HTTP != nil && s.HealthCheck.Protocol == hcloud.LoadBalancerServiceProtocolHTTPS
303-
}
200+
// LBID is the ID assigned to the Hetzner Cloud Load Balancer by the
201+
// backend. Read-only.
202+
//
203+
// Deprecated: This annotation is not used. It is reserved for possible future use.
204+
LBID Name = "load-balancer.hetzner.cloud/id"
205+
)

0 commit comments

Comments
 (0)