From 2a81b163c74f64f3ad2f5b838639d7347aaa6aba Mon Sep 17 00:00:00 2001 From: lukasmetzner Date: Mon, 17 Aug 2026 14:26:50 +0200 Subject: [PATCH 1/5] refactor: use spec model for annotation extraction --- docs/reference/load_balancer_annotations.md | 6 +- hcloud/load_balancers.go | 218 ++--- hcloud/load_balancers_test.go | 86 +- hcloud/testing.go | 8 +- internal/annotation/annotation.go | 163 ++++ internal/annotation/annotation_test.go | 331 +++++++ internal/annotation/load_balancer.go | 88 +- internal/annotation/name.go | 333 ------- internal/annotation/name_test.go | 392 -------- internal/hcops/certificates.go | 11 +- internal/hcops/certificates_test.go | 38 +- internal/hcops/load_balancer.go | 837 +++--------------- internal/hcops/load_balancer_internal_test.go | 136 +-- internal/hcops/load_balancer_test.go | 200 +++-- internal/hcops/mocks.go | 4 +- internal/lbspec/opts.go | 206 +++++ internal/lbspec/opts_test.go | 220 +++++ internal/lbspec/solver.go | 284 ++++++ internal/lbspec/solver_test.go | 416 +++++++++ internal/lbspec/spec.go | 154 ++++ tests/e2e/helper_test.go | 12 +- 21 files changed, 2289 insertions(+), 1854 deletions(-) create mode 100644 internal/annotation/annotation.go create mode 100644 internal/annotation/annotation_test.go delete mode 100644 internal/annotation/name.go delete mode 100644 internal/annotation/name_test.go create mode 100644 internal/lbspec/opts.go create mode 100644 internal/lbspec/opts_test.go create mode 100644 internal/lbspec/solver.go create mode 100644 internal/lbspec/solver_test.go create mode 100644 internal/lbspec/spec.go diff --git a/docs/reference/load_balancer_annotations.md b/docs/reference/load_balancer_annotations.md index 8fd25e85f..9777a9416 100644 --- a/docs/reference/load_balancer_annotations.md +++ b/docs/reference/load_balancer_annotations.md @@ -28,7 +28,7 @@ This page contains all annotations, which can be specified at a Service of type | `load-balancer.hetzner.cloud/node-selector` | `string` | `-` | `No` | Can be set to restrict which Nodes are added as targets to the Load Balancer. It accepts a Kubernetes label selector string, using either the set-based or equality-based formats. If the selector can not be parsed, the targets in the Load Balancer are not updated and an Event is created with the error message. Format: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors | | `load-balancer.hetzner.cloud/uses-proxyprotocol` | `bool` | `false` | `No` | Specifies if the Load Balancer services should use the proxy protocol. | | `load-balancer.hetzner.cloud/http-cookie-name` | `string` | `-` | `No` | Specifies the cookie name when using HTTP or HTTPS as protocol. | -| `load-balancer.hetzner.cloud/http-cookie-lifetime` | `int` | `-` | `No` | Specifies the lifetime of the HTTP cookie. | +| `load-balancer.hetzner.cloud/http-cookie-lifetime` | `duration` | `-` | `No` | Specifies the lifetime of the HTTP cookie. | | `load-balancer.hetzner.cloud/http-timeout-idle` | `duration` | `-` | `No` | Specifies the idle timeout for the client and server side. Must be between 30s and 300s. | | `load-balancer.hetzner.cloud/certificate-type` | `uploaded \| managed` | `uploaded` | `No` | Defines the type of certificate the Load Balancer should use. | | `load-balancer.hetzner.cloud/http-certificates` | `string` | `-` | `No` | A comma separated list of IDs or Names of Certificates assigned to the service. | @@ -38,8 +38,8 @@ This page contains all annotations, which can be specified at a Service of type | `load-balancer.hetzner.cloud/http-sticky-sessions` | `bool` | `false` | `No` | Enables the sticky sessions feature of Hetzner Cloud HTTP Load Balancers. | | `load-balancer.hetzner.cloud/health-check-protocol` | `tcp \| http \| https` | `tcp` | `No` | Sets the protocol the health check should be performed over. | | `load-balancer.hetzner.cloud/health-check-port` | `int` | `-` | `No` | Specifies the port the health check is be performed on. | -| `load-balancer.hetzner.cloud/health-check-interval` | `int` | `-` | `No` | Specifies the interval in which time we perform a health check in seconds. | -| `load-balancer.hetzner.cloud/health-check-timeout` | `int` | `-` | `No` | Specifies the timeout of a single health check. | +| `load-balancer.hetzner.cloud/health-check-interval` | `duration` | `-` | `No` | Specifies the interval in which we perform a health check. | +| `load-balancer.hetzner.cloud/health-check-timeout` | `duration` | `-` | `No` | Specifies the timeout of a single health check. | | `load-balancer.hetzner.cloud/health-check-retries` | `int` | `-` | `No` | Specifies the number of time a health check is retried until a target is marked as unhealthy. | | `load-balancer.hetzner.cloud/health-check-http-domain` | `string` | `-` | `No` | Specifies the domain we try to access when performing the health check. | | `load-balancer.hetzner.cloud/health-check-http-path` | `string` | `-` | `No` | Specifies the path we try to access when performing the health check. | diff --git a/hcloud/load_balancers.go b/hcloud/load_balancers.go index 64d8767f3..bd0c96260 100644 --- a/hcloud/load_balancers.go +++ b/hcloud/load_balancers.go @@ -4,17 +4,18 @@ import ( "context" "errors" "fmt" + "slices" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" - cloudprovider "k8s.io/cloud-provider" "k8s.io/klog/v2" - "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/hcops" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/metrics" "github.com/hetznercloud/hcloud-go/v2/hcloud" + "github.com/hetznercloud/hcloud-go/v2/hcloud/exp/kit/sliceutil" ) // LoadBalancerOps defines the Load Balancer related operations required by @@ -23,7 +24,7 @@ type LoadBalancerOps interface { GetByName(ctx context.Context, name string) (*hcloud.LoadBalancer, error) GetByID(ctx context.Context, id int64) (*hcloud.LoadBalancer, error) GetByK8SServiceUID(ctx context.Context, svc *corev1.Service) (*hcloud.LoadBalancer, error) - Create(ctx context.Context, lbName string, service *corev1.Service) (*hcloud.LoadBalancer, error) + Create(ctx context.Context, service *corev1.Service) (*hcloud.LoadBalancer, error) Delete(ctx context.Context, lb *hcloud.LoadBalancer) error ReconcileHCLB(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) ReconcileHCLBTargets(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service, nodes []*corev1.Node) (bool, error) @@ -42,93 +43,55 @@ func newLoadBalancers(lbOps LoadBalancerOps, lbCfg *config.LoadBalancerConfigura } } -func matchNodeSelector(svc *corev1.Service, nodes []*corev1.Node) ([]*corev1.Node, error) { - var ( - err error - selectedNodes []*corev1.Node - ) - - selector := labels.Everything() - if v, ok := annotation.LBNodeSelector.StringFromService(svc); ok { - selector, err = labels.Parse(v) - if err != nil { - return nil, fmt.Errorf("unable to parse the node-selector annotation: %w", err) - } - } - - for _, n := range nodes { - if selector.Matches(labels.Set(n.GetLabels())) { - selectedNodes = append(selectedNodes, n) - } - } - - return selectedNodes, nil -} - func (l *loadBalancers) GetLoadBalancer( ctx context.Context, _ string, service *corev1.Service, ) (status *corev1.LoadBalancerStatus, exists bool, err error) { const op = "hcloud/loadBalancers.GetLoadBalancer" metrics.OperationCalled.WithLabelValues(op).Inc() - lb, err := l.lbOps.GetByK8SServiceUID(ctx, service) + spec, err := lbspec.Resolve(service, *l.cfg) if err != nil { - if errors.Is(err, hcops.ErrNotFound) { - return nil, false, nil - } return nil, false, fmt.Errorf("%s: %w", op, err) } - if v, ok := annotation.LBHostname.StringFromService(service); ok { - return &corev1.LoadBalancerStatus{ - Ingress: []corev1.LoadBalancerIngress{{Hostname: v}}, - }, true, nil - } - - ingress, err := l.buildLoadBalancerStatusIngress(lb, service) + lb, err := l.lbOps.GetByK8SServiceUID(ctx, service) if err != nil { + if errors.Is(err, hcops.ErrNotFound) { + return nil, false, nil + } return nil, false, fmt.Errorf("%s: %w", op, err) } - return &corev1.LoadBalancerStatus{Ingress: ingress}, true, nil + return &corev1.LoadBalancerStatus{Ingress: l.buildLoadBalancerStatusIngress(lb, spec)}, true, nil } func (l *loadBalancers) GetLoadBalancerName(_ context.Context, _ string, service *corev1.Service) string { - if v, ok := annotation.LBName.StringFromService(service); ok { - return v - } - return cloudprovider.DefaultLoadBalancerName(service) + return lbspec.Name(service) } func (l *loadBalancers) EnsureLoadBalancer( - ctx context.Context, clusterName string, svc *corev1.Service, nodes []*corev1.Node, + ctx context.Context, _ string, svc *corev1.Service, nodes []*corev1.Node, ) (*corev1.LoadBalancerStatus, error) { const op = "hcloud/loadBalancers.EnsureLoadBalancer" metrics.OperationCalled.WithLabelValues(op).Inc() var ( - reload bool - lb *hcloud.LoadBalancer - err error - selectedNodes []*corev1.Node + reload bool + lb *hcloud.LoadBalancer + err error ) - selectedNodes, err = matchNodeSelector(svc, nodes) + spec, err := lbspec.Resolve(svc, *l.cfg) if err != nil { return nil, fmt.Errorf("%s: %w", op, err) } - nodeNames := make([]string, len(selectedNodes)) - for i, n := range selectedNodes { - nodeNames[i] = n.Name - } + selectedNodes := filterNodes(spec.NodeSelector, nodes) + nodeNames := sliceutil.Transform(selectedNodes, func(n *corev1.Node) string { + return n.GetName() + }) klog.InfoS("ensure Load Balancer", "op", op, "service", svc.Name, "nodes", nodeNames) - lb, err = l.lbOps.GetByK8SServiceUID(ctx, svc) - if err != nil && !errors.Is(err, hcops.ErrNotFound) { - return nil, fmt.Errorf("%s: %w", op, err) - } - // Try the load balancer's name if we were not able to find it using the // service UID. This is required for two reasons: // @@ -137,20 +100,23 @@ func (l *loadBalancers) EnsureLoadBalancer( // // 2. Import of load balancers which were created by other means but // should be re-used by the cloud controller manager. - lbName := l.GetLoadBalancerName(ctx, clusterName, svc) + lb, err = l.lbOps.GetByK8SServiceUID(ctx, svc) + if err != nil && !errors.Is(err, hcops.ErrNotFound) { + return nil, fmt.Errorf("%s: %w", op, err) + } + + // Not found by UID label; try name if errors.Is(err, hcops.ErrNotFound) { - lb, err = l.lbOps.GetByName(ctx, lbName) - if err != nil && !errors.Is(err, hcops.ErrNotFound) { - return nil, fmt.Errorf("%s: %w", op, err) - } + lb, err = l.lbOps.GetByName(ctx, spec.Name) } - // If we were still not able to find the load balancer we create it. + // New Load Balancer -> create it if errors.Is(err, hcops.ErrNotFound) { - lb, err = l.lbOps.Create(ctx, lbName, svc) - if err != nil { - return nil, fmt.Errorf("%s: %w", op, err) - } + lb, err = l.lbOps.Create(ctx, svc) + } + + if err != nil { + return nil, fmt.Errorf("%s: %w", op, err) } lbChanged, err := l.lbOps.ReconcileHCLB(ctx, lb, svc) @@ -194,49 +160,34 @@ func (l *loadBalancers) EnsureLoadBalancer( } } - // Either set the Hostname or the IPs (below). - // See: https://github.com/kubernetes/kubernetes/issues/66607 - if v, ok := annotation.LBHostname.StringFromService(svc); ok { - return &corev1.LoadBalancerStatus{ - Ingress: []corev1.LoadBalancerIngress{{Hostname: v}}, - }, nil - } - - ingress, err := l.buildLoadBalancerStatusIngress(lb, svc) - if err != nil { - return nil, fmt.Errorf("%s: %w", op, err) - } - - return &corev1.LoadBalancerStatus{Ingress: ingress}, nil + return &corev1.LoadBalancerStatus{Ingress: l.buildLoadBalancerStatusIngress(lb, spec)}, nil } -func (l *loadBalancers) buildLoadBalancerStatusIngress(lb *hcloud.LoadBalancer, svc *corev1.Service) ([]corev1.LoadBalancerIngress, error) { +// buildLoadBalancerStatusIngress reports the addresses the Service is reachable +// on. A configured hostname replaces the IPs entirely. +// See: https://github.com/kubernetes/kubernetes/issues/66607 +func (l *loadBalancers) buildLoadBalancerStatusIngress(lb *hcloud.LoadBalancer, spec lbspec.Spec) []corev1.LoadBalancerIngress { const op = "hcloud/loadBalancers.getLoadBalancerStatusIngress" metrics.OperationCalled.WithLabelValues(op).Inc() - var ingress []corev1.LoadBalancerIngress - ipMode := corev1.LoadBalancerIPModeVIP - - proxyProtocolEnabled, err := l.getProxyProtocolEnabled(svc) - if err != nil { - return nil, fmt.Errorf("%s: %w", op, err) + if spec.Hostname != "" { + return []corev1.LoadBalancerIngress{{Hostname: spec.Hostname}} } - if proxyProtocolEnabled { + ipMode := corev1.LoadBalancerIPModeVIP + if spec.Service.ProxyProtocol != nil && *spec.Service.ProxyProtocol { ipMode = corev1.LoadBalancerIPModeProxy } + var ingress []corev1.LoadBalancerIngress + if lb.PublicNet.Enabled { ingress = append(ingress, corev1.LoadBalancerIngress{ IP: lb.PublicNet.IPv4.IP.String(), IPMode: &ipMode, }) - ipv6Enabled, err := l.getIPv6Enabled(svc) - if err != nil { - return nil, fmt.Errorf("%s: %w", op, err) - } - if ipv6Enabled { + if spec.IPv6 { ingress = append(ingress, corev1.LoadBalancerIngress{ IP: lb.PublicNet.IPv6.IP.String(), IPMode: &ipMode, @@ -244,12 +195,7 @@ func (l *loadBalancers) buildLoadBalancerStatusIngress(lb *hcloud.LoadBalancer, } } - privateIngressEnabled, err := l.getPrivateIngressEnabled(svc) - if err != nil { - return nil, fmt.Errorf("%s: %w", op, err) - } - - if privateIngressEnabled { + if spec.PrivateIngress { for _, privateNet := range lb.PrivateNet { ingress = append(ingress, corev1.LoadBalancerIngress{ IP: privateNet.IP.String(), @@ -258,62 +204,30 @@ func (l *loadBalancers) buildLoadBalancerStatusIngress(lb *hcloud.LoadBalancer, } } - return ingress, nil -} - -func (l *loadBalancers) getPrivateIngressEnabled(svc *corev1.Service) (bool, error) { - disable, err := annotation.LBDisablePrivateIngress.BoolFromService(svc) - if err == nil { - return !disable, nil - } - if errors.Is(err, annotation.ErrNotSet) { - return l.cfg.PrivateIngressEnabled, nil - } - return true, err -} - -func (l *loadBalancers) getProxyProtocolEnabled(svc *corev1.Service) (bool, error) { - enable, err := annotation.LBSvcProxyProtocol.BoolFromService(svc) - if err == nil { - return enable, nil - } - if errors.Is(err, annotation.ErrNotSet) { - if l.cfg.ProxyProtocolEnabled == nil { - return false, nil - } - return *l.cfg.ProxyProtocolEnabled, nil - } - return false, err -} - -func (l *loadBalancers) getIPv6Enabled(svc *corev1.Service) (bool, error) { - disable, err := annotation.LBIPv6Disabled.BoolFromService(svc) - if err == nil { - return !disable, nil - } - if errors.Is(err, annotation.ErrNotSet) { - return l.cfg.IPv6Enabled, nil - } - return true, err + return ingress } func (l *loadBalancers) UpdateLoadBalancer( - ctx context.Context, clusterName string, svc *corev1.Service, nodes []*corev1.Node, + ctx context.Context, + _ string, + svc *corev1.Service, + nodes []*corev1.Node, ) error { const op = "hcloud/loadBalancers.UpdateLoadBalancer" metrics.OperationCalled.WithLabelValues(op).Inc() var ( - lb *hcloud.LoadBalancer - err error - selectedNodes []*corev1.Node + lb *hcloud.LoadBalancer + err error ) - selectedNodes, err = matchNodeSelector(svc, nodes) + spec, err := lbspec.Resolve(svc, *l.cfg) if err != nil { return fmt.Errorf("%s: %w", op, err) } + selectedNodes := filterNodes(spec.NodeSelector, nodes) + nodeNames := make([]string, len(selectedNodes)) for i, n := range selectedNodes { nodeNames[i] = n.Name @@ -322,15 +236,13 @@ func (l *loadBalancers) UpdateLoadBalancer( lb, err = l.lbOps.GetByK8SServiceUID(ctx, svc) if errors.Is(err, hcops.ErrNotFound) { - lbName := l.GetLoadBalancerName(ctx, clusterName, svc) - - lb, err = l.lbOps.GetByName(ctx, lbName) - if errors.Is(err, hcops.ErrNotFound) { - return nil - } - // further error types handled below + lb, err = l.lbOps.GetByName(ctx, spec.Name) } - if err != nil { + switch { + case errors.Is(err, hcops.ErrNotFound): + // Nothing to do, the Load Balancer does not exist. + return nil + case err != nil: return fmt.Errorf("%s: %w", op, err) } @@ -374,3 +286,9 @@ func (l *loadBalancers) EnsureLoadBalancerDeleted(ctx context.Context, _ string, return nil } + +func filterNodes(selector labels.Selector, nodes []*corev1.Node) []*corev1.Node { + return slices.DeleteFunc(nodes, func(n *corev1.Node) bool { + return !selector.Matches(labels.Set(n.GetLabels())) + }) +} diff --git a/hcloud/load_balancers_test.go b/hcloud/load_balancers_test.go index 201ab92b0..e1e39ffa6 100644 --- a/hcloud/load_balancers_test.go +++ b/hcloud/load_balancers_test.go @@ -11,7 +11,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/hcops" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" "github.com/hetznercloud/hcloud-go/v2/hcloud" ) @@ -29,8 +31,8 @@ func TestLoadBalancers_GetLoadBalancer(t *testing.T) { { Name: "get load balancer without host name IPv6 disabled", ServiceUID: "1", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBIPv6Disabled: "true", + ServiceAnnotations: map[string]string{ + string(annotation.LBIPv6Disabled): "true", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -94,8 +96,8 @@ func TestLoadBalancers_GetLoadBalancer(t *testing.T) { On("GetByK8SServiceUID", tt.Ctx, tt.Service). Return(tt.LB, nil) }, - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBHostname: "hostname", + ServiceAnnotations: map[string]string{ + string(annotation.LBHostname): "hostname", }, Perform: func(t *testing.T, tt *LoadBalancerTestCase) { status, exists, err := tt.LoadBalancers.GetLoadBalancer(tt.Ctx, tt.ClusterName, tt.Service) @@ -231,7 +233,7 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { On("GetByName", tt.Ctx, lbName). Return(nil, hcops.ErrNotFound) tt.LBOps. - On("Create", tt.Ctx, tt.LB.Name, tt.Service). + On("Create", tt.Ctx, tt.Service). Return(tt.LB, nil) tt.LBOps. On("ReconcileHCLB", tt.Ctx, tt.LB, tt.Service). @@ -264,9 +266,9 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { { Name: "public network only no ipv6", ServiceUID: "2", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "pub-net-only-no-ipv6", - annotation.LBIPv6Disabled: "true", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "pub-net-only-no-ipv6", + string(annotation.LBIPv6Disabled): "true", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -295,8 +297,8 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { { Name: "public network only", ServiceUID: "2", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "pub-net-only", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "pub-net-only", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -328,8 +330,8 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { Name: "attach Load Balancer to public and private network", NetworkID: 4711, ServiceUID: "3", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "with-priv-net", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "with-priv-net", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -371,8 +373,8 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { Name: "disable private ingress via default", NetworkID: 4711, ServiceUID: "5", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "with-priv-net-no-priv-ingress", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "with-priv-net-no-priv-ingress", }, UsePrivateIngressDefault: new(false), LB: &hcloud.LoadBalancer{ @@ -414,9 +416,9 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { Name: "disable private ingress via annotation", NetworkID: 4711, ServiceUID: "5", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "with-priv-net-no-priv-ingress", - annotation.LBDisablePrivateIngress: "true", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "with-priv-net-no-priv-ingress", + string(annotation.LBDisablePrivateIngress): "true", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -457,9 +459,9 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { Name: "attach Load Balancer to private network only", NetworkID: 4711, ServiceUID: "6", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "priv-net-only", - annotation.LBDisablePublicNetwork: "true", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "priv-net-only", + string(annotation.LBDisablePublicNetwork): "true", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -484,7 +486,7 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { On("GetByName", tt.Ctx, "priv-net-only"). Return(nil, hcops.ErrNotFound) tt.LBOps. - On("Create", tt.Ctx, tt.LB.Name, tt.Service). + On("Create", tt.Ctx, tt.Service). Return(tt.LB, nil) tt.LBOps. On("ReconcileHCLBTargets", tt.Ctx, tt.LB, tt.Service, tt.Nodes). @@ -511,9 +513,9 @@ func TestLoadBalancers_EnsureLoadBalancer_CreateLoadBalancer(t *testing.T) { Name: "attach Load Balancer to public and private network (with proxy protocol)", NetworkID: 4711, ServiceUID: "3", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "with-priv-net", - annotation.LBSvcProxyProtocol: "true", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "with-priv-net", + string(annotation.LBSvcProxyProtocol): "true", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -561,8 +563,8 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) { { Name: "Load balancer unchanged", ServiceUID: "1", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "test-lb", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "test-lb", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -583,8 +585,8 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) { { Name: "Load balancer changed", ServiceUID: "2", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "test-lb", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "test-lb", }, LB: &hcloud.LoadBalancer{ ID: 2, @@ -606,8 +608,8 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) { { Name: "Load balancer targets changed", ServiceUID: "3", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "test-lb", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "test-lb", }, LB: &hcloud.LoadBalancer{ ID: 3, @@ -629,8 +631,8 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) { { Name: "Load balancer services changed", ServiceUID: "4", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "test-lb", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "test-lb", }, LB: &hcloud.LoadBalancer{ ID: 4, @@ -652,8 +654,8 @@ func TestLoadBalancer_EnsureLoadBalancer_UpdateLoadBalancer(t *testing.T) { { Name: "fall back to load balancer name", ServiceUID: "5", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "pre-existing-lb", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "pre-existing-lb", }, LB: &hcloud.LoadBalancer{ ID: 5, @@ -684,8 +686,8 @@ func TestLoadBalancer_UpdateLoadBalancer(t *testing.T) { { Name: "Load Balancer not found", ServiceUID: "1", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "test-lb", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "test-lb", }, Mock: func(_ *testing.T, tt *LoadBalancerTestCase) { tt.LBOps.On("GetByK8SServiceUID", tt.Ctx, tt.Service).Return(nil, hcops.ErrNotFound) @@ -699,8 +701,8 @@ func TestLoadBalancer_UpdateLoadBalancer(t *testing.T) { { Name: "calls all reconcilement ops", ServiceUID: "2", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "test-lb", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "test-lb", }, LB: &hcloud.LoadBalancer{ ID: 1, @@ -721,8 +723,8 @@ func TestLoadBalancer_UpdateLoadBalancer(t *testing.T) { { Name: "fall back to load balancer name", ServiceUID: "3", - ServiceAnnotations: map[annotation.Name]string{ - annotation.LBName: "previously-created-lb", + ServiceAnnotations: map[string]string{ + string(annotation.LBName): "previously-created-lb", }, LB: &hcloud.LoadBalancer{ ID: 3, @@ -952,11 +954,13 @@ func TestLoadBalancer_matchNodeSelector(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - nodes, err := matchNodeSelector(c.service, c.k8sNodes) + spec, err := lbspec.Resolve(c.service, config.LoadBalancerConfiguration{}) if err != nil { t.Fatal(err) } + nodes := filterNodes(spec.NodeSelector, c.k8sNodes) + if !reflect.DeepEqual(nodes, c.expected) { t.Errorf("expected: %+v got %+v", c.expected, nodes) } diff --git a/hcloud/testing.go b/hcloud/testing.go index f605cb439..0e572111a 100644 --- a/hcloud/testing.go +++ b/hcloud/testing.go @@ -2,13 +2,13 @@ package hcloud import ( "context" + "maps" "testing" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/hcops" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/mocks" @@ -22,7 +22,7 @@ type LoadBalancerTestCase struct { ClusterName string NetworkID int ServiceUID string - ServiceAnnotations map[annotation.Name]string + ServiceAnnotations map[string]string UsePrivateIngressDefault *bool UseIPv6Default *bool Nodes []*corev1.Node @@ -68,9 +68,7 @@ func (tt *LoadBalancerTestCase) run(t *testing.T) { Annotations: map[string]string{}, }, } - for k, v := range tt.ServiceAnnotations { - tt.Service.Annotations[string(k)] = v - } + maps.Copy(tt.Service.Annotations, tt.ServiceAnnotations) if tt.Ctx == nil { tt.Ctx = context.Background() } diff --git a/internal/annotation/annotation.go b/internal/annotation/annotation.go new file mode 100644 index 000000000..edbd8abbb --- /dev/null +++ b/internal/annotation/annotation.go @@ -0,0 +1,163 @@ +// Package annotation defines the Kubernetes annotations that configure the +// resources managed by the cloud controller manager. +// +// Every annotation is declared with the type of its value, so reading one +// yields that type and nothing else: +// +// const LBUsePrivateIP Bool = "load-balancer.hetzner.cloud/use-private-ip" +// +// usePrivateIP, err := LBUsePrivateIP.FromService(svc) +// +// Annotations are declared as constants of a named string type. The reference +// documentation in docs/reference is generated from those declarations by +// tools/doc_generation.go, which expects constants with a string literal. +package annotation + +import ( + "errors" + "fmt" + "net" + "strconv" + "strings" + "time" + + corev1 "k8s.io/api/core/v1" + + "github.com/hetznercloud/hcloud-go/v2/hcloud" +) + +var ErrNotSet = errors.New("not set") + +type Annotation[T any] interface { + FromService(svc *corev1.Service) (T, error) +} + +type ( + String string + Bool string + Int string + Duration string // e.g. "30s" or "1h" + Strings string // comma separated list of strings + IP string + Protocol string // Load Balancer service protocol + AlgorithmType string // Load Balancer algorithm type + // Certificates is an annotation holding a comma separated list of Certificates, + // referenced either by ID or by name. + Certificates string +) + +func (a String) FromService(svc *corev1.Service) (string, error) { + return value(string(a), svc) +} + +func (a Bool) FromService(svc *corev1.Service) (bool, error) { + return parse(string(a), svc, strconv.ParseBool) +} + +func (a Int) FromService(svc *corev1.Service) (int, error) { + return parse(string(a), svc, strconv.Atoi) +} + +func (a Duration) FromService(svc *corev1.Service) (time.Duration, error) { + return parse(string(a), svc, time.ParseDuration) +} + +func (a Strings) FromService(svc *corev1.Service) ([]string, error) { + return parse(string(a), svc, func(v string) ([]string, error) { + return strings.Split(v, ","), nil + }) +} + +func (a IP) FromService(svc *corev1.Service) (net.IP, error) { + return parse(string(a), svc, parseIP) +} + +func (a Protocol) FromService(svc *corev1.Service) (hcloud.LoadBalancerServiceProtocol, error) { + return parse(string(a), svc, parseServiceProtocol) +} + +func (a AlgorithmType) FromService(svc *corev1.Service) (hcloud.LoadBalancerAlgorithmType, error) { + return parse(string(a), svc, parseAlgorithmType) +} + +func (a Certificates) FromService(svc *corev1.Service) ([]*hcloud.Certificate, error) { + return parse(string(a), svc, parseCertificates) +} + +// value returns the raw value of the annotation with name from svc. +func value(name string, svc *corev1.Service) (string, error) { + v, ok := svc.Annotations[name] + if !ok { + return "", fmt.Errorf("%s: %w", name, ErrNotSet) + } + return v, nil +} + +func parse[T any](name string, svc *corev1.Service, convert func(string) (T, error)) (T, error) { + var zero T + + v, err := value(name, svc) + if err != nil { + return zero, err + } + + converted, err := convert(v) + if err != nil { + return zero, fmt.Errorf("%s: %w", name, err) + } + + return converted, nil +} + +func parseIP(v string) (net.IP, error) { + ip := net.ParseIP(v) + if ip == nil { + return nil, fmt.Errorf("invalid ip address: %s", v) + } + return ip, nil +} + +func parseAlgorithmType(v string) (hcloud.LoadBalancerAlgorithmType, error) { + // Lowercase because all our algorithms are lowercase. + algorithm := hcloud.LoadBalancerAlgorithmType(strings.ToLower(v)) + + switch algorithm { + case hcloud.LoadBalancerAlgorithmTypeLeastConnections, + hcloud.LoadBalancerAlgorithmTypeRoundRobin: + return algorithm, nil + default: + return "", fmt.Errorf("invalid: %s", v) + } +} + +func parseServiceProtocol(v string) (hcloud.LoadBalancerServiceProtocol, error) { + // Lowercase because all our protocols are lowercase. + protocol := hcloud.LoadBalancerServiceProtocol(strings.ToLower(v)) + + switch protocol { + case hcloud.LoadBalancerServiceProtocolTCP, + hcloud.LoadBalancerServiceProtocolHTTP, + hcloud.LoadBalancerServiceProtocolHTTPS: + return protocol, nil + default: + return "", fmt.Errorf("invalid: %s", v) + } +} + +func parseCertificates(v string) ([]*hcloud.Certificate, error) { + values := strings.Split(v, ",") + certificates := make([]*hcloud.Certificate, len(values)) + + for i, value := range values { + id, err := strconv.ParseInt(value, 10, 64) + if err != nil { + // If we could not parse the string as an integer we assume it is a + // name, not an id. + certificates[i] = &hcloud.Certificate{Name: value} + continue + } + certificates[i] = &hcloud.Certificate{ID: id} + } + + return certificates, nil +} diff --git a/internal/annotation/annotation_test.go b/internal/annotation/annotation_test.go new file mode 100644 index 000000000..c4e12e6e0 --- /dev/null +++ b/internal/annotation/annotation_test.go @@ -0,0 +1,331 @@ +package annotation_test + +import ( + "errors" + "net" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" + "github.com/hetznercloud/hcloud-go/v2/hcloud" +) + +// Every accessor is exercised through the same annotation name, declared once +// per type. +const annName = "some/annotation" + +const ( + annString annotation.String = annName + annBool annotation.Bool = annName + annInt annotation.Int = annName + annDuration annotation.Duration = annName + annStrings annotation.Strings = annName + annIP annotation.IP = annName + annProtocol annotation.Protocol = annName + annAlgorithmType annotation.AlgorithmType = annName + annCertificates annotation.Certificates = annName +) + +func TestString(t *testing.T) { + tests := []accessorTest{ + { + name: "value as string", + value: "some value", + expected: "some value", + }, + { + // An annotation set to the empty string is set, and some settings + // use that to opt out of a cluster-wide default. + name: "value set to the empty string", + value: "", + expected: "", + }, + { + name: "value not set", + notSet: true, + expected: "", + err: errors.New(annName + ": not set"), + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annString.FromService(svc) + }) + + // The harness always assigns an annotation map, so a Service without any + // annotations is covered separately. + t.Run("Service has no annotations", func(t *testing.T) { + actual, err := annString.FromService(&corev1.Service{}) + + assert.ErrorIs(t, err, annotation.ErrNotSet) + assert.Empty(t, actual) + }) +} + +func TestBool(t *testing.T) { + tests := []accessorTest{ + { + name: "value set to true", + value: "true", + expected: true, + }, + { + name: "value set to false", + value: "false", + expected: false, + }, + { + name: "value not set", + notSet: true, + expected: false, + err: annotation.ErrNotSet, + }, + { + name: "value invalid", + value: "invalid", + expected: false, + err: strconv.ErrSyntax, + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annBool.FromService(svc) + }) +} + +func TestInt(t *testing.T) { + tests := []accessorTest{ + { + name: "value set to 10", + value: "10", + expected: 10, + }, + { + name: "value not set", + notSet: true, + expected: 0, + err: errors.New(annName + ": not set"), + }, + { + name: "value invalid", + value: "invalid", + expected: 0, + err: strconv.ErrSyntax, + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annInt.FromService(svc) + }) +} + +func TestDuration(t *testing.T) { + tests := []accessorTest{ + { + name: "value set", + value: "1h", + expected: time.Hour, + }, + { + name: "value not set", + notSet: true, + err: annotation.ErrNotSet, + }, + { + name: "value invalid", + value: "invalid", + err: errors.New(annName + `: time: invalid duration "invalid"`), + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annDuration.FromService(svc) + }) +} + +func TestStrings(t *testing.T) { + tests := []accessorTest{ + { + name: "value set", + value: "a,b,c", + expected: []string{"a", "b", "c"}, + }, + { + name: "value missing", + notSet: true, + err: annotation.ErrNotSet, + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annStrings.FromService(svc) + }) +} + +func TestIP(t *testing.T) { + tests := []accessorTest{ + { + name: "value set to valid IPv4", + value: "1.2.3.4", + expected: net.ParseIP("1.2.3.4"), + }, + { + name: "value set to valid IPv6", + value: "3c2e:2ef9:a7e9:1a5b:30ba:4912:e3fe:91b2", + expected: net.ParseIP("3c2e:2ef9:a7e9:1a5b:30ba:4912:e3fe:91b2"), + }, + { + name: "value invalid", + value: "invalid", + err: errors.New(annName + ": invalid ip address: invalid"), + }, + { + name: "value not set", + notSet: true, + err: annotation.ErrNotSet, + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annIP.FromService(svc) + }) +} + +func TestProtocol(t *testing.T) { + tests := []accessorTest{ + { + name: "value set", + value: string(hcloud.LoadBalancerServiceProtocolHTTP), + expected: hcloud.LoadBalancerServiceProtocolHTTP, + }, + { + name: "value is uppercased", + value: "HTTPS", + expected: hcloud.LoadBalancerServiceProtocolHTTPS, + }, + { + name: "value not set", + notSet: true, + err: annotation.ErrNotSet, + }, + { + name: "value invalid", + value: "invalid", + err: errors.New(annName + ": invalid: invalid"), + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annProtocol.FromService(svc) + }) +} + +func TestAlgorithmType(t *testing.T) { + tests := []accessorTest{ + { + name: "value set", + value: string(hcloud.LoadBalancerAlgorithmTypeLeastConnections), + expected: hcloud.LoadBalancerAlgorithmTypeLeastConnections, + }, + { + name: "value is uppercased", + value: "ROUND_ROBIN", + expected: hcloud.LoadBalancerAlgorithmTypeRoundRobin, + }, + { + name: "value not set", + notSet: true, + err: annotation.ErrNotSet, + }, + { + name: "value invalid", + value: "invalid", + err: errors.New(annName + ": invalid: invalid"), + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annAlgorithmType.FromService(svc) + }) +} + +func TestCertificates(t *testing.T) { + tests := []accessorTest{ + { + name: "ids set", + value: "3,5", + expected: []*hcloud.Certificate{{ID: 3}, {ID: 5}}, + }, + { + name: "names set", + value: "cert-1,cert-2", + expected: []*hcloud.Certificate{{Name: "cert-1"}, {Name: "cert-2"}}, + }, + { + name: "ids and names mixed", + value: "3,cert-2", + expected: []*hcloud.Certificate{{ID: 3}, {Name: "cert-2"}}, + }, + { + name: "value not set", + notSet: true, + err: annotation.ErrNotSet, + }, + } + + runAllAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { + return annCertificates.FromService(svc) + }) +} + +type accessorTest struct { + name string + // value is put on the Service unless notSet is true. + value string + notSet bool + err error + expected any +} + +func (tt *accessorTest) run(t *testing.T, call func(svc *corev1.Service) (any, error)) { + t.Helper() + + var svc corev1.Service + svc.Annotations = map[string]string{} + + if !tt.notSet { + svc.Annotations[annName] = tt.value + } + + actual, err := call(&svc) + if tt.err != nil { + if errors.Is(err, tt.err) { + return + } + assert.EqualError(t, err, tt.err.Error()) + return + } + assert.NoError(t, err) + // Don't use assert.Equal to compare nil values, as it requires the nil + // values to be casted to the correct type. + if tt.expected == nil && actual == nil { + return + } + assert.Equal(t, tt.expected, actual) +} + +func runAllAccessorTests( + t *testing.T, tests []accessorTest, call func(svc *corev1.Service) (any, error), +) { + t.Helper() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.run(t, call) + }) + } +} diff --git a/internal/annotation/load_balancer.go b/internal/annotation/load_balancer.go index 5ea68931f..88edb7687 100644 --- a/internal/annotation/load_balancer.go +++ b/internal/annotation/load_balancer.go @@ -6,41 +6,41 @@ const ( // // Type: string // Read-only: true - LBPublicIPv4 Name = "load-balancer.hetzner.cloud/ipv4" + LBPublicIPv4 String = "load-balancer.hetzner.cloud/ipv4" // LBPublicIPv4RDNS is the reverse DNS record assigned to the IPv4 address of // the Load Balancer. // // Type: string // Read-only: true - LBPublicIPv4RDNS Name = "load-balancer.hetzner.cloud/ipv4-rdns" + LBPublicIPv4RDNS String = "load-balancer.hetzner.cloud/ipv4-rdns" // LBPublicIPv6 is the public IPv6 address assigned to the Load Balancer by // the backend. // // Type: string // Read-only: true - LBPublicIPv6 Name = "load-balancer.hetzner.cloud/ipv6" + LBPublicIPv6 String = "load-balancer.hetzner.cloud/ipv6" // LBPublicIPv6RDNS is the reverse DNS record assigned to the IPv6 address of // the Load Balancer. // // Type: string // Read-only: true - LBPublicIPv6RDNS Name = "load-balancer.hetzner.cloud/ipv6-rdns" + LBPublicIPv6RDNS String = "load-balancer.hetzner.cloud/ipv6-rdns" // LBIPv6Disabled disables the use of IPv6 for the Load Balancer. // Set this annotation if you use external-dns. // // Type: bool // Default: false - LBIPv6Disabled Name = "load-balancer.hetzner.cloud/ipv6-disabled" + LBIPv6Disabled Bool = "load-balancer.hetzner.cloud/ipv6-disabled" // LBName is the name of the Load Balancer. The name will be visible in // the Hetzner Cloud API console. // // Type: string - LBName Name = "load-balancer.hetzner.cloud/name" + LBName String = "load-balancer.hetzner.cloud/name" // LBDisablePublicNetwork disables the public network of the Hetzner Cloud // Load Balancer. It will still have a public network assigned, but all @@ -48,27 +48,27 @@ const ( // // Type: bool // Default: false - LBDisablePublicNetwork Name = "load-balancer.hetzner.cloud/disable-public-network" + LBDisablePublicNetwork Bool = "load-balancer.hetzner.cloud/disable-public-network" // LBDisablePrivateIngress disables the use of the private network for // ingress. // // Type: bool // Default: false - LBDisablePrivateIngress Name = "load-balancer.hetzner.cloud/disable-private-ingress" + LBDisablePrivateIngress Bool = "load-balancer.hetzner.cloud/disable-private-ingress" // LBUsePrivateIP configures the Load Balancer to use the private IP for // Load Balancer server targets. // // Type: bool // Default: false - LBUsePrivateIP Name = "load-balancer.hetzner.cloud/use-private-ip" + LBUsePrivateIP Bool = "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. // // Type: string - LBPrivateIPv4 Name = "load-balancer.hetzner.cloud/private-ipv4" + LBPrivateIPv4 IP = "load-balancer.hetzner.cloud/private-ipv4" // PrivateSubnetIPRange specifies an existing subnet to which the load balancer will be attached. // The value must be in the CIDR notation. The subnet must belong to the network defined @@ -76,32 +76,32 @@ const ( // See: https://docs.hetzner.cloud/reference/cloud#network-actions-add-a-subnet-to-a-network // // Type: string - PrivateSubnetIPRange Name = "load-balancer.hetzner.cloud/private-subnet-ip-range" + PrivateSubnetIPRange String = "load-balancer.hetzner.cloud/private-subnet-ip-range" // LBHostname specifies the hostname of the Load Balancer. This will be // used as ingress address instead of the Load Balancer IP addresses if // specified. // // Type: string - LBHostname Name = "load-balancer.hetzner.cloud/hostname" + LBHostname String = "load-balancer.hetzner.cloud/hostname" // LBSvcProtocol specifies the protocol of the service. // // Type: tcp | http | https // Default: tcp - LBSvcProtocol Name = "load-balancer.hetzner.cloud/protocol" + LBSvcProtocol Protocol = "load-balancer.hetzner.cloud/protocol" // LBAlgorithmType specifies the algorithm type of the Load Balancer. // // Type: round_robin | least_connections // Default: round_robin - LBAlgorithmType Name = "load-balancer.hetzner.cloud/algorithm-type" + LBAlgorithmType AlgorithmType = "load-balancer.hetzner.cloud/algorithm-type" // LBType specifies the type of the Load Balancer. // // Type: string // Default: lb11 - LBType Name = "load-balancer.hetzner.cloud/type" + LBType String = "load-balancer.hetzner.cloud/type" // LBLocation specifies the location where the Load Balancer will be // created in. @@ -114,7 +114,7 @@ const ( // Mutually exclusive with [LBNetworkZone]. // // Type: string - LBLocation Name = "load-balancer.hetzner.cloud/location" + LBLocation String = "load-balancer.hetzner.cloud/location" // LBNetworkZone specifies the network zone where the Load Balancer will be // created in. @@ -128,7 +128,7 @@ const ( // Mutually exclusive with [LBLocation]. // // Type: string - LBNetworkZone Name = "load-balancer.hetzner.cloud/network-zone" + LBNetworkZone String = "load-balancer.hetzner.cloud/network-zone" // LBNodeSelector can be set to restrict which Nodes are added as targets to the // Load Balancer. It accepts a Kubernetes label selector string, using either the @@ -140,51 +140,51 @@ const ( // Format: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors // // Type: string - LBNodeSelector Name = "load-balancer.hetzner.cloud/node-selector" + LBNodeSelector String = "load-balancer.hetzner.cloud/node-selector" // LBSvcProxyProtocol specifies if the Load Balancer services should // use the proxy protocol. // // Type: bool // Default: false - LBSvcProxyProtocol Name = "load-balancer.hetzner.cloud/uses-proxyprotocol" + LBSvcProxyProtocol Bool = "load-balancer.hetzner.cloud/uses-proxyprotocol" // LBSvcHTTPCookieName specifies the cookie name when using HTTP or HTTPS // as protocol. // // Type: string - LBSvcHTTPCookieName Name = "load-balancer.hetzner.cloud/http-cookie-name" + LBSvcHTTPCookieName String = "load-balancer.hetzner.cloud/http-cookie-name" // LBSvcHTTPCookieLifetime specifies the lifetime of the HTTP cookie. // - // Type: int - LBSvcHTTPCookieLifetime Name = "load-balancer.hetzner.cloud/http-cookie-lifetime" + // Type: duration + LBSvcHTTPCookieLifetime Duration = "load-balancer.hetzner.cloud/http-cookie-lifetime" // LBSvcHTTPTimeoutIdle specifies the idle timeout for the client and // server side. Must be between 30s and 300s. // // Type: duration - LBSvcHTTPTimeoutIdle Name = "load-balancer.hetzner.cloud/http-timeout-idle" + LBSvcHTTPTimeoutIdle Duration = "load-balancer.hetzner.cloud/http-timeout-idle" // LBSvcHTTPCertificateType defines the type of certificate the Load // Balancer should use. // // Type: uploaded | managed // Default: uploaded - LBSvcHTTPCertificateType Name = "load-balancer.hetzner.cloud/certificate-type" + LBSvcHTTPCertificateType String = "load-balancer.hetzner.cloud/certificate-type" // LBSvcHTTPCertificates a comma separated list of IDs or Names of // Certificates assigned to the service. // // Type: string - LBSvcHTTPCertificates Name = "load-balancer.hetzner.cloud/http-certificates" + LBSvcHTTPCertificates Certificates = "load-balancer.hetzner.cloud/http-certificates" // LBSvcHTTPManagedCertificateName contains the name of the managed // certificate to create by the Cloud Controller manager. Ignored if // [LBSvcHTTPCertificateType] is missing or set to "uploaded". // // Type: string - LBSvcHTTPManagedCertificateName Name = "load-balancer.hetzner.cloud/http-managed-certificate-name" + LBSvcHTTPManagedCertificateName String = "load-balancer.hetzner.cloud/http-managed-certificate-name" // LBSvcHTTPManagedCertificateUseACMEStaging tells the cloud controller manager to create // the certificate using Let's Encrypt staging. @@ -196,7 +196,7 @@ const ( // Type: bool // Default: false // Internal: true - LBSvcHTTPManagedCertificateUseACMEStaging Name = "load-balancer.hetzner.cloud/http-managed-certificate-acme-staging" + LBSvcHTTPManagedCertificateUseACMEStaging Bool = "load-balancer.hetzner.cloud/http-managed-certificate-acme-staging" // LBSvcHTTPManagedCertificateDomains contains a comma separated list of the // domain names of the managed certificate. @@ -204,75 +204,75 @@ const ( // All domains are used to create a single managed certificate. // // Type: string - LBSvcHTTPManagedCertificateDomains Name = "load-balancer.hetzner.cloud/http-managed-certificate-domains" + LBSvcHTTPManagedCertificateDomains Strings = "load-balancer.hetzner.cloud/http-managed-certificate-domains" // LBSvcRedirectHTTP create a redirect from HTTP to HTTPS. // // Type: bool // Default: false - LBSvcRedirectHTTP Name = "load-balancer.hetzner.cloud/http-redirect-http" + LBSvcRedirectHTTP Bool = "load-balancer.hetzner.cloud/http-redirect-http" // LBSvcHTTPStickySessions enables the sticky sessions feature of Hetzner // Cloud HTTP Load Balancers. // // Type: bool // Default: false - LBSvcHTTPStickySessions Name = "load-balancer.hetzner.cloud/http-sticky-sessions" + LBSvcHTTPStickySessions Bool = "load-balancer.hetzner.cloud/http-sticky-sessions" // LBSvcHealthCheckProtocol sets the protocol the health check should be // performed over. // // Type: tcp | http | https // Default: tcp - LBSvcHealthCheckProtocol Name = "load-balancer.hetzner.cloud/health-check-protocol" + LBSvcHealthCheckProtocol Protocol = "load-balancer.hetzner.cloud/health-check-protocol" // LBSvcHealthCheckPort specifies the port the health check is be performed // on. // // Type: int - LBSvcHealthCheckPort Name = "load-balancer.hetzner.cloud/health-check-port" + LBSvcHealthCheckPort Int = "load-balancer.hetzner.cloud/health-check-port" - // LBSvcHealthCheckInterval specifies the interval in which time we perform - // a health check in seconds. + // LBSvcHealthCheckInterval specifies the interval in which we perform a + // health check. // - // Type: int - LBSvcHealthCheckInterval Name = "load-balancer.hetzner.cloud/health-check-interval" + // Type: duration + LBSvcHealthCheckInterval Duration = "load-balancer.hetzner.cloud/health-check-interval" // LBSvcHealthCheckTimeout specifies the timeout of a single health check. // - // Type: int - LBSvcHealthCheckTimeout Name = "load-balancer.hetzner.cloud/health-check-timeout" + // Type: duration + LBSvcHealthCheckTimeout Duration = "load-balancer.hetzner.cloud/health-check-timeout" // LBSvcHealthCheckRetries specifies the number of time a health check is // retried until a target is marked as unhealthy. // // Type: int - LBSvcHealthCheckRetries Name = "load-balancer.hetzner.cloud/health-check-retries" + LBSvcHealthCheckRetries Int = "load-balancer.hetzner.cloud/health-check-retries" // LBSvcHealthCheckHTTPDomain specifies the domain we try to access when // performing the health check. // // Type: string - LBSvcHealthCheckHTTPDomain Name = "load-balancer.hetzner.cloud/health-check-http-domain" + LBSvcHealthCheckHTTPDomain String = "load-balancer.hetzner.cloud/health-check-http-domain" // LBSvcHealthCheckHTTPPath specifies the path we try to access when // performing the health check. // // Type: string - LBSvcHealthCheckHTTPPath Name = "load-balancer.hetzner.cloud/health-check-http-path" + LBSvcHealthCheckHTTPPath String = "load-balancer.hetzner.cloud/health-check-http-path" // LBSvcHealthCheckHTTPValidateCertificate specifies whether the health // check should validate the SSL certificate that comes from the target // nodes. // // Type: bool - LBSvcHealthCheckHTTPValidateCertificate Name = "load-balancer.hetzner.cloud/health-check-http-validate-certificate" + LBSvcHealthCheckHTTPValidateCertificate Bool = "load-balancer.hetzner.cloud/health-check-http-validate-certificate" // LBSvcHealthCheckHTTPStatusCodes is a comma separated list of HTTP status // codes which we expect. // // Type: string - LBSvcHealthCheckHTTPStatusCodes Name = "load-balancer.hetzner.cloud/http-status-codes" + LBSvcHealthCheckHTTPStatusCodes Strings = "load-balancer.hetzner.cloud/http-status-codes" // LBID is the ID assigned to the Hetzner Cloud Load Balancer by the // backend. @@ -281,5 +281,5 @@ const ( // // Type: string // Read-only: true - LBID Name = "load-balancer.hetzner.cloud/id" + LBID String = "load-balancer.hetzner.cloud/id" ) diff --git a/internal/annotation/name.go b/internal/annotation/name.go deleted file mode 100644 index 1b0e67432..000000000 --- a/internal/annotation/name.go +++ /dev/null @@ -1,333 +0,0 @@ -package annotation - -import ( - "errors" - "fmt" - "net" - "strconv" - "strings" - "time" - - corev1 "k8s.io/api/core/v1" - - "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/metrics" - "github.com/hetznercloud/hcloud-go/v2/hcloud" -) - -// ErrNotSet signals that an annotation was not set. -var ErrNotSet = errors.New("not set") - -// Name defines the name of a K8S annotation. -type Name string - -// StringFromService retrieves the value belonging to the annotation from svc. -// -// If svc has no value for the annotation the second return value is false. -func (s Name) StringFromService(svc *corev1.Service) (string, bool) { - if svc.Annotations == nil { - return "", false - } - v, ok := svc.Annotations[string(s)] - return v, ok -} - -// StringsFromService retrieves the []string value belonging to the annotation -// from svc. -// -// StringsFromService returns ErrNotSet annotation was not set. -func (s Name) StringsFromService(svc *corev1.Service) ([]string, error) { - const op = "annotation/Name.StringsFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var ss []string - - err := s.applyToValue(op, svc, func(v string) error { - ss = strings.Split(v, ",") - return nil - }) - - return ss, err -} - -// BoolFromService retrieves the boolean value belonging to the annotation from -// svc. -// -// BoolFromService returns an error if the value could not be converted to a -// boolean, or the annotation was not set. In the case of a missing value, the -// error wraps ErrNotSet. -func (s Name) BoolFromService(svc *corev1.Service) (bool, error) { - const op = "annotation/Name.BoolFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - v, ok := s.StringFromService(svc) - if !ok { - return false, fmt.Errorf("%s: %v: %w", op, s, ErrNotSet) - } - b, err := strconv.ParseBool(v) - if err != nil { - return false, fmt.Errorf("%s: %v: %w", op, s, err) - } - return b, nil -} - -// IntFromService retrieves the int value belonging to the annotation from svc. -// -// IntFromService returns an error if the value could not be converted to an -// int, or the annotation was not set. In the case of a missing value, the -// error wraps ErrNotSet. -func (s Name) IntFromService(svc *corev1.Service) (int, error) { - const op = "annotation/Name.IntFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - v, ok := s.StringFromService(svc) - if !ok { - return 0, fmt.Errorf("%s: %v: %w", op, s, ErrNotSet) - } - i, err := strconv.Atoi(v) - if err != nil { - return 0, fmt.Errorf("%s: %v: %w", op, s, err) - } - return i, nil -} - -// IntsFromService retrieves the []int value belonging to the annotation from -// svc. -// -// IntsFromService returns an error if the value could not be converted to a -// []int, or the annotation was not set. In the case of a missing value, the -// error wraps ErrNotSet. -func (s Name) IntsFromService(svc *corev1.Service) ([]int, error) { - const op = "annotation/Name.IntsFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var is []int - - err := s.applyToValue(op, svc, func(v string) error { - ss := strings.Split(v, ",") - is = make([]int, len(ss)) - - for i, s := range ss { - iv, err := strconv.Atoi(s) - if err != nil { - return err - } - is[i] = iv - } - return nil - }) - - return is, err -} - -// IPFromService retrieves the net.IP value belonging to the annotation from -// svc. -// -// IPFromService returns an error if the value could not be converted to a -// net.IP, or the annotation was not set. In the case of a missing value, the -// error wraps ErrNotSet. -func (s Name) IPFromService(svc *corev1.Service) (net.IP, error) { - const op = "annotation/Name.IPFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var ip net.IP - - err := s.applyToValue(op, svc, func(v string) error { - ip = net.ParseIP(v) - if ip == nil { - return fmt.Errorf("invalid ip address: %s", v) - } - return nil - }) - - return ip, err -} - -// DurationFromService retrieves the time.Duration value belonging to the -// annotation from svc. -// -// DurationFromService returns an error if the value could not be converted to -// a time.Duration, or the annotation was not set. In the case of a missing -// value, the error wraps ErrNotSet. -func (s Name) DurationFromService(svc *corev1.Service) (time.Duration, error) { - const op = "annotation/Name.DurationFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var d time.Duration - - err := s.applyToValue(op, svc, func(v string) error { - var err error - - d, err = time.ParseDuration(v) - return err - }) - - return d, err -} - -// LBSvcProtocolFromService retrieves the hcloud.LoadBalancerServiceProtocol -// value belonging to the annotation from svc. -// -// LBSvcProtocolFromService returns an error if the value could not be -// converted to a hcloud.LoadBalancerServiceProtocol, or the annotation was not -// set. In the case of a missing value, the error wraps ErrNotSet. -func (s Name) LBSvcProtocolFromService(svc *corev1.Service) (hcloud.LoadBalancerServiceProtocol, error) { - const op = "annotation/Name.LBSvcProtocolFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var p hcloud.LoadBalancerServiceProtocol - - err := s.applyToValue(op, svc, func(v string) error { - var err error - - p, err = validateServiceProtocol(v) - return err - }) - - return p, err -} - -// LBAlgorithmTypeFromService retrieves the hcloud.LoadBalancerAlgorithmType -// value belonging to the annotation from svc. -// -// LBAlgorithmTypeFromService returns an error if the value could not be -// converted to a hcloud.LoadBalancerAlgorithmType, or the annotation was not -// set. In the case of a missing value, the error wraps ErrNotSet. -func (s Name) LBAlgorithmTypeFromService(svc *corev1.Service) (hcloud.LoadBalancerAlgorithmType, error) { - const op = "annotation/Name.LBAlgorithmTypeFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var alg hcloud.LoadBalancerAlgorithmType - - err := s.applyToValue(op, svc, func(v string) error { - var err error - - alg, err = validateAlgorithmType(v) - return err - }) - - return alg, err -} - -// NetworkZoneFromService retrieves the hcloud.NetworkZone value belonging to -// the annotation from svc. -// -// NetworkZoneFromService returns ErrNotSet if the annotation was not set. -func (s Name) NetworkZoneFromService(svc *corev1.Service) (hcloud.NetworkZone, error) { - const op = "annotation/Name.NetworkZoneFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var nz hcloud.NetworkZone - - err := s.applyToValue(op, svc, func(v string) error { - nz = hcloud.NetworkZone(v) - return nil - }) - - return nz, err -} - -// CertificatesFromService retrieves the []*hcloud.Certificate value belonging -// to the annotation from svc. -// -// CertificatesFromService returns an error if the value could not be converted -// to a []*hcloud.Certificate, or the annotation was not set. In the case of a -// missing value, the error wraps ErrNotSet. -func (s Name) CertificatesFromService(svc *corev1.Service) ([]*hcloud.Certificate, error) { - const op = "annotation/Name.CertificatesFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var cs []*hcloud.Certificate - - err := s.applyToValue(op, svc, func(v string) error { - ss := strings.Split(v, ",") - cs = make([]*hcloud.Certificate, len(ss)) - - for i, s := range ss { - id, err := strconv.ParseInt(s, 10, 64) - if err != nil { - // If we could not parse the string as an integer we assume it - // is a name not an id. - cs[i] = &hcloud.Certificate{Name: s} - continue - } - cs[i] = &hcloud.Certificate{ID: id} - } - - return nil - }) - - return cs, err -} - -// CertificateTypeFromService retrieves the hcloud.CertificateType value -// belonging to the annotation from svc. -// -// CertificateTypeFromService returns an error if the value could not be -// converted to a hcloud.CertificateType. In the case of a missing value, the -// error wraps ErrNotSet. -func (s Name) CertificateTypeFromService(svc *corev1.Service) (hcloud.CertificateType, error) { - const op = "annotation/Name.CertificateTypeFromService" - metrics.OperationCalled.WithLabelValues(op).Inc() - - var ct hcloud.CertificateType - - err := s.applyToValue(op, svc, func(v string) error { - switch strings.ToLower(v) { - case string(hcloud.CertificateTypeUploaded): - ct = hcloud.CertificateTypeUploaded - case string(hcloud.CertificateTypeManaged): - ct = hcloud.CertificateTypeManaged - default: - return fmt.Errorf("%s: unsupported certificate type: %s", op, v) - } - return nil - }) - - return ct, err -} - -func (s Name) applyToValue(op string, svc *corev1.Service, f func(string) error) error { - v, ok := s.StringFromService(svc) - if !ok { - return fmt.Errorf("%s: %v: %w", op, s, ErrNotSet) - } - if err := f(v); err != nil { - return fmt.Errorf("%s: %w", op, err) - } - return nil -} - -func validateAlgorithmType(algorithmType string) (hcloud.LoadBalancerAlgorithmType, error) { - const op = "annotation/validateAlgorithmType" - metrics.OperationCalled.WithLabelValues(op).Inc() - - algorithmType = strings.ToLower(algorithmType) // Lowercase because all our protocols are lowercase - hcloudAlgorithmType := hcloud.LoadBalancerAlgorithmType(algorithmType) - - switch hcloudAlgorithmType { - case hcloud.LoadBalancerAlgorithmTypeLeastConnections: - case hcloud.LoadBalancerAlgorithmTypeRoundRobin: - default: - return "", fmt.Errorf("%s: invalid: %s", op, algorithmType) - } - - return hcloudAlgorithmType, nil -} - -func validateServiceProtocol(protocol string) (hcloud.LoadBalancerServiceProtocol, error) { - const op = "annotation/validateServiceProtocol" - metrics.OperationCalled.WithLabelValues(op).Inc() - - protocol = strings.ToLower(protocol) // Lowercase because all our protocols are lowercase - hcloudProtocol := hcloud.LoadBalancerServiceProtocol(protocol) - switch hcloudProtocol { - case hcloud.LoadBalancerServiceProtocolTCP: - case hcloud.LoadBalancerServiceProtocolHTTPS: - case hcloud.LoadBalancerServiceProtocolHTTP: - // Valid - break - default: - return "", fmt.Errorf("%s: invalid: %s", op, protocol) - } - return hcloudProtocol, nil -} diff --git a/internal/annotation/name_test.go b/internal/annotation/name_test.go deleted file mode 100644 index 1a5cbf7c4..000000000 --- a/internal/annotation/name_test.go +++ /dev/null @@ -1,392 +0,0 @@ -package annotation_test - -import ( - "errors" - "fmt" - "net" - "strconv" - "testing" - "time" - - "github.com/stretchr/testify/assert" - corev1 "k8s.io/api/core/v1" - - "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" - "github.com/hetznercloud/hcloud-go/v2/hcloud" -) - -const ann annotation.Name = "some/annotation" - -func TestName_StringFromService(t *testing.T) { - tests := []struct { - name string - svcAnnotations map[annotation.Name]string - ok bool - expected string - }{ - { - name: "value as string", - svcAnnotations: map[annotation.Name]string{ann: "some value"}, - ok: true, - expected: "some value", - }, - { - name: "Service has no annotations", - }, - { - name: "value not set", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var svc corev1.Service - svc.Annotations = map[string]string{} - - for k, v := range tt.svcAnnotations { - svc.Annotations[string(k)] = v - } - actual, ok := ann.StringFromService(&svc) - assert.Equal(t, tt.ok, ok) - assert.Equal(t, tt.expected, actual) - }) - } -} - -func TestName_StringsFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set", - svcAnnotations: map[annotation.Name]string{ - ann: "a,b,c", - }, - expected: []string{"a", "b", "c"}, - }, - { - name: "value missing", - err: annotation.ErrNotSet, - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.StringsFromService(svc) - }) -} - -func TestName_BoolFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set to true", - svcAnnotations: map[annotation.Name]string{ann: "true"}, - expected: true, - }, - { - name: "value set to false", - svcAnnotations: map[annotation.Name]string{ann: "false"}, - expected: false, - }, - { - name: "value missing", - expected: false, - err: annotation.ErrNotSet, - }, - { - name: "value invalid", - svcAnnotations: map[annotation.Name]string{ann: "invalid"}, - expected: false, - err: strconv.ErrSyntax, - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.BoolFromService(svc) - }) -} - -func TestName_IntFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set to 10", - svcAnnotations: map[annotation.Name]string{ann: "10"}, - expected: 10, - }, - { - name: "value missing", - expected: 0, - err: fmt.Errorf("annotation/Name.IntFromService: %s: %w", ann, annotation.ErrNotSet), - }, - { - name: "value invalid", - svcAnnotations: map[annotation.Name]string{ann: "invalid"}, - expected: 0, - err: strconv.ErrSyntax, - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.IntFromService(svc) - }) -} - -func TestName_IntsFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set", - svcAnnotations: map[annotation.Name]string{ - ann: "5,8", - }, - expected: []int{5, 8}, - }, - { - name: "value missing", - err: annotation.ErrNotSet, - }, - { - name: "value invalid", - svcAnnotations: map[annotation.Name]string{ann: "invalid"}, - err: strconv.ErrSyntax, - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.IntsFromService(svc) - }) -} - -func TestName_IPFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set to valid IPv4", - svcAnnotations: map[annotation.Name]string{ - ann: "1.2.3.4", - }, - expected: net.ParseIP("1.2.3.4"), - }, - { - name: "value set to valid IPv6", - svcAnnotations: map[annotation.Name]string{ - ann: "3c2e:2ef9:a7e9:1a5b:30ba:4912:e3fe:91b2", - }, - expected: net.ParseIP("3c2e:2ef9:a7e9:1a5b:30ba:4912:e3fe:91b2"), - }, - { - name: "value invalid", - svcAnnotations: map[annotation.Name]string{ - ann: "invalid", - }, - err: errors.New("annotation/Name.IPFromService: invalid ip address: invalid"), - }, - { - name: "value not set", - err: annotation.ErrNotSet, - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.IPFromService(svc) - }) -} - -func TestName_DurationFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set", - svcAnnotations: map[annotation.Name]string{ - ann: "1h", - }, - expected: time.Hour, - }, - { - name: "value not set", - err: annotation.ErrNotSet, - }, - { - name: "value invalid", - svcAnnotations: map[annotation.Name]string{ - ann: "invalid", - }, - err: errors.New("annotation/Name.DurationFromService: time: invalid duration \"invalid\""), - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.DurationFromService(svc) - }) -} - -func TestName_LBSvcProtocolFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set", - svcAnnotations: map[annotation.Name]string{ - ann: string(hcloud.LoadBalancerServiceProtocolHTTP), - }, - expected: hcloud.LoadBalancerServiceProtocolHTTP, - }, - { - name: "value not set", - err: annotation.ErrNotSet, - }, - { - name: "value invalid", - svcAnnotations: map[annotation.Name]string{ - ann: "invalid", - }, - err: errors.New("annotation/Name.LBSvcProtocolFromService: annotation/validateServiceProtocol: invalid: invalid"), - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.LBSvcProtocolFromService(svc) - }) -} - -func TestName_LBAlgorithmTypeFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set", - svcAnnotations: map[annotation.Name]string{ - ann: string(hcloud.LoadBalancerAlgorithmTypeLeastConnections), - }, - expected: hcloud.LoadBalancerAlgorithmTypeLeastConnections, - }, - { - name: "value not set", - err: annotation.ErrNotSet, - }, - { - name: "value invalid", - svcAnnotations: map[annotation.Name]string{ - ann: "invalid", - }, - err: errors.New("annotation/Name.LBAlgorithmTypeFromService: annotation/validateAlgorithmType: invalid: invalid"), - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.LBAlgorithmTypeFromService(svc) - }) -} - -func TestName_NetworkZoneFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "value set", - svcAnnotations: map[annotation.Name]string{ - ann: string(hcloud.NetworkZoneEUCentral), - }, - expected: hcloud.NetworkZoneEUCentral, - }, - { - name: "value not set", - err: annotation.ErrNotSet, - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.NetworkZoneFromService(svc) - }) -} - -func TestName_CertificatesFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "ids set", - svcAnnotations: map[annotation.Name]string{ - ann: "3,5", - }, - expected: []*hcloud.Certificate{{ID: 3}, {ID: 5}}, - }, - { - name: "names set", - svcAnnotations: map[annotation.Name]string{ - ann: "cert-1,cert-2", - }, - expected: []*hcloud.Certificate{{Name: "cert-1"}, {Name: "cert-2"}}, - }, - { - name: "value not set", - err: annotation.ErrNotSet, - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.CertificatesFromService(svc) - }) -} - -func TestName_CertificateTypeFromService(t *testing.T) { - tests := []typedAccessorTest{ - { - name: "uploaded certificate", - svcAnnotations: map[annotation.Name]string{ - ann: string(hcloud.CertificateTypeUploaded), - }, - expected: hcloud.CertificateTypeUploaded, - }, - { - name: "managed certificate", - svcAnnotations: map[annotation.Name]string{ - ann: string(hcloud.CertificateTypeManaged), - }, - expected: hcloud.CertificateTypeManaged, - }, - { - name: "unsupported certificate type", - svcAnnotations: map[annotation.Name]string{ - ann: "unsupported type", - }, - err: fmt.Errorf("annotation/Name.CertificateTypeFromService: annotation/Name.CertificateTypeFromService: unsupported certificate type: unsupported type"), - }, - } - - runAllTypedAccessorTests(t, tests, func(svc *corev1.Service) (any, error) { - return ann.CertificateTypeFromService(svc) - }) -} - -type typedAccessorTest struct { - name string - svcAnnotations map[annotation.Name]string - err error - expected any -} - -func (tt *typedAccessorTest) run(t *testing.T, call func(svc *corev1.Service) (any, error)) { - t.Helper() - - var svc corev1.Service - svc.Annotations = map[string]string{} - - for k, v := range tt.svcAnnotations { - svc.Annotations[string(k)] = v - } - - actual, err := call(&svc) - if tt.err != nil { - if errors.Is(err, tt.err) { - return - } - assert.EqualError(t, err, tt.err.Error()) - return - } - assert.NoError(t, err) - // Don't use assert.Equal to compare nil values, as it requires the nil - // values to be casted to the correct type. - if tt.expected == nil && actual == nil { - return - } - assert.Equal(t, tt.expected, actual) -} - -func runAllTypedAccessorTests( - t *testing.T, tests []typedAccessorTest, call func(svc *corev1.Service) (any, error), -) { - t.Helper() - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tt.run(t, call) - }) - } -} diff --git a/internal/hcops/certificates.go b/internal/hcops/certificates.go index 4e9270c1a..446a6b4a8 100644 --- a/internal/hcops/certificates.go +++ b/internal/hcops/certificates.go @@ -55,23 +55,16 @@ func (co *CertificateOps) GetCertificateByLabel(ctx context.Context, label strin return certs[0], nil } -// CreateManagedCertificate creates a managed certificate for domains labeled -// with label. +// CreateManagedCertificate creates the managed certificate described by opts. // // CreateManagedCertificate returns a wrapped ErrAlreadyExists if the // certificate already exists. func (co *CertificateOps) CreateManagedCertificate( - ctx context.Context, name string, domains []string, labels map[string]string, + ctx context.Context, opts hcloud.CertificateCreateOpts, ) error { const op = "hcops/CertificateOps.CreateManagedCertificate" metrics.OperationCalled.WithLabelValues(op).Inc() - opts := hcloud.CertificateCreateOpts{ - Name: name, - Type: hcloud.CertificateTypeManaged, - DomainNames: domains, - Labels: labels, - } result, _, err := co.CertClient.CreateCertificate(ctx, opts) if hcloud.IsError(err, hcloud.ErrorCodeUniquenessError) { return fmt.Errorf("%s: %w", op, ErrAlreadyExists) diff --git a/internal/hcops/certificates_test.go b/internal/hcops/certificates_test.go index ab5255301..a29f5904a 100644 --- a/internal/hcops/certificates_test.go +++ b/internal/hcops/certificates_test.go @@ -130,6 +130,16 @@ func TestCertificateOps_GetCertificateByLabel(t *testing.T) { runCertificateOpsTestCases(t, tests) } +// managedCertificateOpts is the certificate the cases below create. +func managedCertificateOpts() hcloud.CertificateCreateOpts { + return hcloud.CertificateCreateOpts{ + Name: "test-cert", + Type: hcloud.CertificateTypeManaged, + DomainNames: []string{"example.com", "*.example.com"}, + Labels: map[string]string{"key": "value"}, + } +} + func TestCertificateOps_CreateManagedCertificate(t *testing.T) { tests := []certificateOpsTestCase{ { @@ -140,12 +150,7 @@ func TestCertificateOps_CreateManagedCertificate(t *testing.T) { Return(nil, nil, errors.New("test error")) }, Perform: func(t *testing.T, tt *certificateOpsTestCase) { - err := tt.CertOps.CreateManagedCertificate( - tt.Ctx, - "test-cert", - []string{"example.com", "*.example.com"}, - map[string]string{"key": "value"}, - ) + err := tt.CertOps.CreateManagedCertificate(tt.Ctx, managedCertificateOpts()) assert.Error(t, err) assert.True(t, strings.HasSuffix(err.Error(), "test error")) }, @@ -159,12 +164,7 @@ func TestCertificateOps_CreateManagedCertificate(t *testing.T) { Return(nil, nil, err) }, Perform: func(t *testing.T, tt *certificateOpsTestCase) { - err := tt.CertOps.CreateManagedCertificate( - tt.Ctx, - "test-cert", - []string{"example.com", "*.example.com"}, - map[string]string{"key": "value"}, - ) + err := tt.CertOps.CreateManagedCertificate(tt.Ctx, managedCertificateOpts()) assert.ErrorIs(t, err, hcops.ErrAlreadyExists) }, }, @@ -173,22 +173,12 @@ func TestCertificateOps_CreateManagedCertificate(t *testing.T) { Mock: func(_ *testing.T, tt *certificateOpsTestCase) { res := hcloud.CertificateCreateResult{Certificate: &hcloud.Certificate{ID: 1}, Action: &hcloud.Action{ID: 2}} tt.CertClient. - On("CreateCertificate", tt.Ctx, hcloud.CertificateCreateOpts{ - Name: "test-cert", - Type: hcloud.CertificateTypeManaged, - DomainNames: []string{"example.com", "*.example.com"}, - Labels: map[string]string{"key": "value"}, - }). + On("CreateCertificate", tt.Ctx, managedCertificateOpts()). Return(res, nil, nil) tt.ActionClient.On("WaitFor", tt.Ctx, &hcloud.Action{ID: 2}).Return(nil) }, Perform: func(t *testing.T, tt *certificateOpsTestCase) { - err := tt.CertOps.CreateManagedCertificate( - tt.Ctx, - "test-cert", - []string{"example.com", "*.example.com"}, - map[string]string{"key": "value"}, - ) + err := tt.CertOps.CreateManagedCertificate(tt.Ctx, managedCertificateOpts()) assert.NoError(t, err) }, }, diff --git a/internal/hcops/load_balancer.go b/internal/hcops/load_balancer.go index db04c7820..c29f0d5d0 100644 --- a/internal/hcops/load_balancer.go +++ b/internal/hcops/load_balancer.go @@ -4,9 +4,7 @@ import ( "context" "errors" "fmt" - "maps" "net" - "sync" "time" hrobot "github.com/syself/hrobot-go" @@ -17,6 +15,7 @@ import ( "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/cache" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/metrics" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/providerid" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/utils" @@ -29,8 +28,7 @@ const ( // identify a load balancer managed by Hetzner Cloud Cloud Controller Manager. LabelServiceUID = "hcloud-ccm/service-uid" - defaultLoadBalancerType = "lb11" - loadBalancerSubsystem = "load_balancer" + loadBalancerSubsystem = "load_balancer" ) // LoadBalancerOps implements all operations regarding Hetzner Cloud Load Balancers. @@ -118,128 +116,27 @@ func (l *LoadBalancerOps) GetByID(ctx context.Context, id int64) (*hcloud.LoadBa return lb, nil } -func (l *LoadBalancerOps) getType(ctx context.Context, svc *corev1.Service) (*hcloud.LoadBalancerType, bool, error) { - ctx = cache.SetSubsystem(ctx, loadBalancerSubsystem) - var lbTypeName string - var unset bool - - if l.Cfg.LoadBalancer.Type != "" { - lbTypeName = l.Cfg.LoadBalancer.Type - } - - if v, ok := annotation.LBType.StringFromService(svc); ok { - lbTypeName = v - } - - if lbTypeName == "" { - lbTypeName = defaultLoadBalancerType - unset = true - utils.WarnEventLogf( - l.Recorder, - svc, - "LoadBalancerTypeUnconfigured", - "Load Balancer Type unconfigured: this will be required in the future, set it with the annotation %q or cluster-wide with the environment variable %q", - annotation.LBType, - config.HcloudLoadBalancersType, - ) - } - - lbType, err := l.LBTypeCache.ByName(ctx, lbTypeName) - if err != nil { - return nil, unset, err - } - - if lbType == nil { - return nil, unset, fmt.Errorf("load balancer type not found: %s", lbTypeName) - } - - msg, unavailable := deprecationutil.LoadBalancerTypeMessage(lbType) - if unavailable { - return nil, false, errors.New(msg) - } - if msg != "" { - utils.WarnEventLogf( - l.Recorder, - svc, - "LoadBalancerTypeDeprecated", - "%s", msg, - ) - } - - return lbType, unset, nil -} - // Create creates a new Load Balancer using the Hetzner Cloud API. // // It adds annotations identifying the HC Load Balancer to svc. -func (l *LoadBalancerOps) Create( - ctx context.Context, lbName string, svc *corev1.Service, -) (*hcloud.LoadBalancer, error) { +func (l *LoadBalancerOps) Create(ctx context.Context, svc *corev1.Service) (*hcloud.LoadBalancer, error) { const op = "hcops/LoadBalancerOps.Create" metrics.OperationCalled.WithLabelValues(op).Inc() - opts := hcloud.LoadBalancerCreateOpts{ - Name: lbName, - LoadBalancerType: &hcloud.LoadBalancerType{Name: "lb11"}, - Labels: map[string]string{ - LabelServiceUID: string(svc.ObjectMeta.UID), - }, - } - - lbType, _, err := l.getType(ctx, svc) + spec, err := lbspec.Resolve(svc, l.Cfg.LoadBalancer) if err != nil { - return nil, fmt.Errorf("error getting load balancer type: %w", err) - } - opts.LoadBalancerType = lbType - - if l.Cfg.LoadBalancer.Location != "" { - opts.Location = &hcloud.Location{Name: l.Cfg.LoadBalancer.Location} - } - if v, ok := annotation.LBLocation.StringFromService(svc); ok { - if v == "" { - // Allow resetting the location in case someone wants to specify a network zone in an annotation - // and a location as default. - opts.Location = nil - } else { - opts.Location = &hcloud.Location{Name: v} - } - } - opts.NetworkZone = hcloud.NetworkZone(l.Cfg.LoadBalancer.NetworkZone) - if v, ok := annotation.LBNetworkZone.StringFromService(svc); ok { - opts.NetworkZone = hcloud.NetworkZone(v) + return nil, fmt.Errorf("%s: %w", op, err) } - if opts.Location == nil && opts.NetworkZone == "" { + if spec.Location == "" && spec.NetworkZone == "" { return nil, fmt.Errorf("%s: neither %s nor %s set", op, annotation.LBLocation, annotation.LBNetworkZone) } - if opts.Location != nil && opts.NetworkZone != "" { - opts.NetworkZone = "" - } - - algType, err := annotation.LBAlgorithmType.LBAlgorithmTypeFromService(svc) - switch { - case err == nil: - opts.Algorithm = &hcloud.LoadBalancerAlgorithm{Type: algType} - case errors.Is(err, annotation.ErrNotSet): - if l.Cfg.LoadBalancer.AlgorithmType != "" { - opts.Algorithm = &hcloud.LoadBalancerAlgorithm{Type: l.Cfg.LoadBalancer.AlgorithmType} - } - default: - return nil, fmt.Errorf("%s: %w", op, err) - } - disablePubIface, err := annotation.LBDisablePublicNetwork.BoolFromService(svc) - switch { - case err == nil: - opts.PublicInterface = new(!disablePubIface) - case errors.Is(err, annotation.ErrNotSet): - if l.Cfg.LoadBalancer.DisablePublicNetwork != nil { - opts.PublicInterface = new(!*l.Cfg.LoadBalancer.DisablePublicNetwork) - } - default: - return nil, fmt.Errorf("%s: %w", op, err) + lbType, err := l.verifyType(ctx, svc, spec) + if err != nil { + return nil, fmt.Errorf("error getting load balancer type: %w", err) } - result, _, err := l.LBClient.Create(ctx, opts) + result, _, err := l.LBClient.Create(ctx, spec.CreateOpts(lbType)) if err != nil { return nil, fmt.Errorf("%s: %w", op, withInvalidInputFields(err)) } @@ -251,6 +148,7 @@ func (l *LoadBalancerOps) Create( if err != nil { return nil, fmt.Errorf("%s: get Load Balancer: %d: %w", op, result.LoadBalancer.ID, err) } + return lb, nil } @@ -277,49 +175,54 @@ func (l *LoadBalancerOps) ReconcileHCLB(ctx context.Context, lb *hcloud.LoadBala var changed bool - labelSet, err := l.changeHCLBInfo(ctx, lb, svc) + spec, err := lbspec.Resolve(svc, l.Cfg.LoadBalancer) + if err != nil { + return changed, fmt.Errorf("%s: %w", op, err) + } + + labelSet, err := l.changeHCLBInfo(ctx, lb, spec) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } changed = changed || labelSet - ipv4RDNSChanged, err := l.changeIPv4RDNS(ctx, lb, svc) + ipv4RDNSChanged, err := l.changeIPv4RDNS(ctx, lb, spec) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } changed = changed || ipv4RDNSChanged - ipv6RDNSChanged, err := l.changeIPv6RDNS(ctx, lb, svc) + ipv6RDNSChanged, err := l.changeIPv6RDNS(ctx, lb, spec) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } changed = changed || ipv6RDNSChanged - algorithmChanged, err := l.changeAlgorithm(ctx, lb, svc) + algorithmChanged, err := l.changeAlgorithm(ctx, lb, spec) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } changed = changed || algorithmChanged - typeChanged, err := l.changeType(ctx, lb, svc) + typeChanged, err := l.changeType(ctx, lb, svc, spec) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } changed = changed || typeChanged - networkDetached, err := l.detachFromNetwork(ctx, lb, svc) + networkDetached, err := l.detachFromNetwork(ctx, lb, spec) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } changed = changed || networkDetached - networkAttached, err := l.attachToNetwork(ctx, lb, svc) + networkAttached, err := l.attachToNetwork(ctx, lb, spec) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } changed = changed || networkAttached - pubIfaceToggled, err := l.togglePublicInterface(ctx, lb, svc) + pubIfaceToggled, err := l.togglePublicInterface(ctx, lb, spec) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } @@ -334,30 +237,13 @@ func (l *LoadBalancerOps) ReconcileHCLB(ctx context.Context, lb *hcloud.LoadBala // This is implemented in one method as both changes need to be made using // hcloud.LoadBalancerUpdateOpts. Using one method reduces the number of API // requests should more than one change be necessary. -func (l *LoadBalancerOps) changeHCLBInfo(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) { +func (l *LoadBalancerOps) changeHCLBInfo( + ctx context.Context, lb *hcloud.LoadBalancer, spec lbspec.Spec, +) (bool, error) { const op = "hcops/LoadBalancerOps.changeHCLBInfo" metrics.OperationCalled.WithLabelValues(op).Inc() - var ( - update bool - opts hcloud.LoadBalancerUpdateOpts - ) - - if lb.Labels[LabelServiceUID] != string(svc.ObjectMeta.UID) { - // Make a defensive copy of labels. This way we do not modify lb unless - // updating is really successful. - labels := make(map[string]string, len(lb.Labels)+1) - labels[LabelServiceUID] = string(svc.ObjectMeta.UID) - maps.Copy(labels, lb.Labels) - opts.Labels = labels - update = true - } - - if lbName, ok := annotation.LBName.StringFromService(svc); ok && lbName != lb.Name { - opts.Name = lbName - update = true - } - + opts, update := spec.UpdateOpts(lb) if !update { return false, nil } @@ -372,15 +258,15 @@ func (l *LoadBalancerOps) changeHCLBInfo(ctx context.Context, lb *hcloud.LoadBal return true, nil } -func (l *LoadBalancerOps) changeIPv4RDNS(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) { +func (l *LoadBalancerOps) changeIPv4RDNS(ctx context.Context, lb *hcloud.LoadBalancer, spec lbspec.Spec) (bool, error) { const op = "hcops/LoadBalancerOps.changeIPv4RDNS" metrics.OperationCalled.WithLabelValues(op).Inc() - rdns, ok := annotation.LBPublicIPv4RDNS.StringFromService(svc) // If the annotation is not set, no changes are needed - if !ok { + if spec.IPv4RDNS == nil { return false, nil } + rdns := *spec.IPv4RDNS // If the annotation and the actual value match, no changes are needed if rdns == lb.PublicNet.IPv4.DNSPtr { return false, nil @@ -397,15 +283,15 @@ func (l *LoadBalancerOps) changeIPv4RDNS(ctx context.Context, lb *hcloud.LoadBal return true, nil } -func (l *LoadBalancerOps) changeIPv6RDNS(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) { +func (l *LoadBalancerOps) changeIPv6RDNS(ctx context.Context, lb *hcloud.LoadBalancer, spec lbspec.Spec) (bool, error) { const op = "hcops/LoadBalancerOps.changeIPv6RDNS" metrics.OperationCalled.WithLabelValues(op).Inc() - rdns, ok := annotation.LBPublicIPv6RDNS.StringFromService(svc) // If the annotation is not set, no changes are needed - if !ok { + if spec.IPv6RDNS == nil { return false, nil } + rdns := *spec.IPv6RDNS // If the annotation and the actual value match, no changes are needed if rdns == lb.PublicNet.IPv6.DNSPtr { return false, nil @@ -422,26 +308,16 @@ func (l *LoadBalancerOps) changeIPv6RDNS(ctx context.Context, lb *hcloud.LoadBal return true, nil } -func (l *LoadBalancerOps) changeAlgorithm(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) { +func (l *LoadBalancerOps) changeAlgorithm(ctx context.Context, lb *hcloud.LoadBalancer, spec lbspec.Spec) (bool, error) { const op = "hcops/LoadBalancerOps.changeAlgorithm" metrics.OperationCalled.WithLabelValues(op).Inc() - at, err := annotation.LBAlgorithmType.LBAlgorithmTypeFromService(svc) - if err != nil { - if errors.Is(err, annotation.ErrNotSet) { - if l.Cfg.LoadBalancer.AlgorithmType == "" { - return false, nil - } - at = l.Cfg.LoadBalancer.AlgorithmType - } else { - return false, fmt.Errorf("%s: %w", op, err) - } - } - if at == lb.Algorithm.Type { + // An unconfigured algorithm is left alone. + if spec.Algorithm == "" || spec.Algorithm == lb.Algorithm.Type { return false, nil } - opts := hcloud.LoadBalancerChangeAlgorithmOpts{Type: at} + opts := spec.ChangeAlgorithmOpts() action, _, err := l.LBClient.ChangeAlgorithm(ctx, lb, opts) if err != nil { return false, fmt.Errorf("%s: %w", op, withInvalidInputFields(err)) @@ -453,20 +329,22 @@ func (l *LoadBalancerOps) changeAlgorithm(ctx context.Context, lb *hcloud.LoadBa return true, nil } -func (l *LoadBalancerOps) changeType(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) { +func (l *LoadBalancerOps) changeType( + ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service, spec lbspec.Spec, +) (bool, error) { const op = "hcops/LoadBalancerOps.changeType" metrics.OperationCalled.WithLabelValues(op).Inc() opts := hcloud.LoadBalancerChangeTypeOpts{} - lbType, unset, err := l.getType(ctx, svc) + lbType, err := l.verifyType(ctx, svc, spec) if err != nil { return false, fmt.Errorf("error getting load balancer type: %w", err) } // If the user removes the annotation, we do not downgrade the Load Balancer // back to its default value. This could be changed in a next major release. - if unset { + if spec.TypeUnset { return false, nil } @@ -487,18 +365,17 @@ func (l *LoadBalancerOps) changeType(ctx context.Context, lb *hcloud.LoadBalance return true, nil } -func (l *LoadBalancerOps) detachFromNetwork(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) { +func (l *LoadBalancerOps) detachFromNetwork(ctx context.Context, lb *hcloud.LoadBalancer, spec lbspec.Spec) (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 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()) { + if l.NetworkID == lbpn.Network.ID && (spec.PrivateIPv4 == nil || spec.PrivateIPv4.Equal(lbpn.IP)) { continue } klog.InfoS("detach from network", "op", op, "loadBalancerID", lb.ID, "networkID", lbpn.Network.ID, "privateIPv4", lbpn.IP.String()) @@ -516,42 +393,18 @@ func (l *LoadBalancerOps) detachFromNetwork(ctx context.Context, lb *hcloud.Load return changed, nil } -func (l *LoadBalancerOps) attachToNetwork(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) { +func (l *LoadBalancerOps) attachToNetwork(ctx context.Context, lb *hcloud.LoadBalancer, spec lbspec.Spec) (bool, error) { const op = "hcops/LoadBalancerOps.attachToNetwork" metrics.OperationCalled.WithLabelValues(op).Inc() - var err error - - privateIPv4String, privateIPv4configured := annotation.LBPrivateIPv4.StringFromService(svc) - subnetString, subnetConfigured := annotation.PrivateSubnetIPRange.StringFromService(svc) - if !subnetConfigured && l.Cfg.LoadBalancer.PrivateSubnetIPRange != "" { - subnetString = l.Cfg.LoadBalancer.PrivateSubnetIPRange - subnetConfigured = true - } // 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, privateIPv4String) { + if l.NetworkID == 0 || lbAttached(lb, l.NetworkID, spec.PrivateIPv4) { return false, nil } - 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)) - } - } - - var subnet *net.IPNet - if subnetConfigured { - _, subnet, err = net.ParseCIDR(subnetString) - if err != nil { - return false, fmt.Errorf("%s: could not parse private subnet IP range '%s'", op, subnetString) - } - } - - if privateIPv4 != nil { - klog.InfoS("attach to network", "op", op, "loadBalancerID", lb.ID, "networkID", l.NetworkID, "privateIP", privateIPv4) + if spec.PrivateIPv4 != nil { + klog.InfoS("attach to network", "op", op, "loadBalancerID", lb.ID, "networkID", l.NetworkID, "privateIP", spec.PrivateIPv4) } else { klog.InfoS("attach to network", "op", op, "loadBalancerID", lb.ID, "networkID", l.NetworkID) } @@ -568,13 +421,7 @@ func (l *LoadBalancerOps) attachToNetwork(ctx context.Context, lb *hcloud.LoadBa if retryDelay == 0 { retryDelay = time.Second } - opts := hcloud.LoadBalancerAttachToNetworkOpts{Network: nw} - if privateIPv4 != nil { - opts.IP = privateIPv4 - } - if subnet != nil { - opts.IPRange = subnet - } + opts := spec.AttachToNetworkOpts(nw) a, _, err := l.LBClient.AttachToNetwork(ctx, lb, opts) if hcloud.IsError(err, hcloud.ErrorCodeConflict, hcloud.ErrorCodeLocked) { klog.InfoS("retry due to conflict or lock", @@ -594,35 +441,22 @@ func (l *LoadBalancerOps) attachToNetwork(ctx context.Context, lb *hcloud.LoadBa return true, nil } -func (l *LoadBalancerOps) togglePublicInterface(ctx context.Context, lb *hcloud.LoadBalancer, svc *corev1.Service) (bool, error) { +func (l *LoadBalancerOps) togglePublicInterface(ctx context.Context, lb *hcloud.LoadBalancer, spec lbspec.Spec) (bool, error) { const op = "hcops/LoadBalancerOps.togglePublicInterface" metrics.OperationCalled.WithLabelValues(op).Inc() var a *hcloud.Action + var err error - disable, err := annotation.LBDisablePublicNetwork.BoolFromService(svc) - var desiredDisable *bool - switch { - case err == nil: - desiredDisable = new(disable) - case errors.Is(err, annotation.ErrNotSet): - desiredDisable = l.Cfg.LoadBalancer.DisablePublicNetwork - default: - return false, fmt.Errorf("%s: %w", op, err) - } - - if desiredDisable == nil { - return false, nil - } - - if *desiredDisable == !lb.PublicNet.Enabled { + // An unconfigured public interface is left alone. + if spec.PublicInterface == nil || *spec.PublicInterface == lb.PublicNet.Enabled { return false, nil } - if *desiredDisable { - a, _, err = l.LBClient.DisablePublicInterface(ctx, lb) - } else { + if *spec.PublicInterface { a, _, err = l.LBClient.EnablePublicInterface(ctx, lb) + } else { + a, _, err = l.LBClient.DisablePublicInterface(ctx, lb) } if err != nil { return false, fmt.Errorf("%s: %w", op, err) @@ -666,10 +500,12 @@ func (l *LoadBalancerOps) ReconcileHCLBTargets( changed bool ) - privateIPEnabled, err := l.getPrivateIPEnabled(svc) + spec, err := lbspec.Resolve(svc, l.Cfg.LoadBalancer) if err != nil { return changed, fmt.Errorf("%s: %w", op, err) } + + privateIPEnabled := spec.UsePrivateIP if privateIPEnabled && l.NetworkID == 0 { return changed, fmt.Errorf("%s: use private ip: missing network id", op) } @@ -874,10 +710,7 @@ func (l *LoadBalancerOps) ReconcileHCLBTargets( } klog.InfoS("add target", "op", op, "service", svc.ObjectMeta.Name, "targetName", node.Name) - opts := hcloud.LoadBalancerAddServerTargetOpts{ - Server: &hcloud.Server{ID: id}, - UsePrivateIP: &privateIPEnabled, - } + opts := spec.AddServerTargetOpts(id) a, _, err := l.LBClient.AddServerTarget(ctx, lb, opts) if err != nil { if hcloud.IsError(err, hcloud.ErrorCodeResourceLimitExceeded) { @@ -948,17 +781,6 @@ func (l *LoadBalancerOps) emitMaxTargetsReachedError(node *corev1.Node, svc *cor klog.InfoS("cannot add server target because max number of targets have been reached", "op", op, "service", svc.ObjectMeta.Name, "targetName", node.Name) } -func (l *LoadBalancerOps) getPrivateIPEnabled(svc *corev1.Service) (bool, error) { - usePrivateIP, err := annotation.LBUsePrivateIP.BoolFromService(svc) - if err != nil { - if errors.Is(err, annotation.ErrNotSet) { - return l.Cfg.LoadBalancer.PrivateIPEnabled, nil - } - return false, err - } - return usePrivateIP, nil -} - // ReconcileHCLBServices synchronizes services exposed by the Hetzner Cloud // Load Balancer with the kubernetes cluster. func (l *LoadBalancerOps) ReconcileHCLBServices( @@ -969,7 +791,19 @@ func (l *LoadBalancerOps) ReconcileHCLBServices( var changed bool - if err := l.reconcileManagedCertificate(ctx, svc); err != nil { + spec, err := lbspec.Resolve(svc, l.Cfg.LoadBalancer) + if err != nil { + return false, fmt.Errorf("%s: %w", op, err) + } + + if err := l.reconcileManagedCertificate(ctx, spec); err != nil { + return false, fmt.Errorf("%s: %w", op, err) + } + + // Resolved once for the whole Service, as every port uses the same + // certificates. + certificates, err := l.resolveCertificates(ctx, svc, spec) + if err != nil { return false, fmt.Errorf("%s: %w", op, err) } @@ -982,13 +816,7 @@ func (l *LoadBalancerOps) ReconcileHCLBServices( // balancer. Remove the ports from the set of HC Load Balancer listen // ports. for _, port := range svc.Spec.Ports { - var ( - addOpts hcloud.LoadBalancerAddServiceOpts - updOpts hcloud.LoadBalancerUpdateServiceOpts - action *hcloud.Action - - err error - ) + var action *hcloud.Action if port.Protocol != "" && port.Protocol != corev1.ProtocolTCP { utils.WarnEventLogf( @@ -1006,31 +834,19 @@ func (l *LoadBalancerOps) ReconcileHCLBServices( portExists := hclbListenPorts[portNo] delete(hclbListenPorts, portNo) - b := &hclbServiceOptsBuilder{ - Port: port, - Service: svc, - CertOps: l.CertOps, - cfg: l.Cfg.LoadBalancer, - } if portExists { klog.InfoS("update service", "op", op, "port", portNo, "loadBalancerID", lb.ID) - updOpts, err = b.buildUpdateServiceOpts() - if err != nil { - return changed, fmt.Errorf("%s: %w", op, err) - } - action, _, err = l.LBClient.UpdateService(ctx, lb, b.listenPort, updOpts) + opts := spec.Service.UpdateServiceOpts(port, certificates) + action, _, err = l.LBClient.UpdateService(ctx, lb, portNo, opts) if err != nil { return changed, fmt.Errorf("%s: %w", op, withInvalidInputFields(err)) } } else { klog.InfoS("add service", "op", op, "port", portNo, "loadBalancerID", lb.ID) - addOpts, err = b.buildAddServiceOpts() - if err != nil { - return changed, fmt.Errorf("%s: %w", op, err) - } - action, _, err = l.LBClient.AddService(ctx, lb, addOpts) + opts := spec.Service.AddServiceOpts(port, certificates) + action, _, err = l.LBClient.AddService(ctx, lb, opts) if err != nil { return changed, fmt.Errorf("%s: %w", op, withInvalidInputFields(err)) } @@ -1059,31 +875,17 @@ func (l *LoadBalancerOps) ReconcileHCLBServices( return changed, nil } -func (l *LoadBalancerOps) reconcileManagedCertificate(ctx context.Context, svc *corev1.Service) error { +func (l *LoadBalancerOps) reconcileManagedCertificate( + ctx context.Context, spec lbspec.Spec, +) error { const op = "hcops/LoadBalancerOps.reconcileManagedCertificate" metrics.OperationCalled.WithLabelValues(op).Inc() - if typ, ok := annotation.LBSvcHTTPCertificateType.StringFromService(svc); !ok || typ != string(hcloud.CertificateTypeManaged) { + if spec.ManagedCertificate == nil { return nil } - name, ok := annotation.LBSvcHTTPManagedCertificateName.StringFromService(svc) - if !ok || name == "" { - name = fmt.Sprintf("ccm-managed-certificate-%s", svc.ObjectMeta.UID) - } - domains, err := annotation.LBSvcHTTPManagedCertificateDomains.StringsFromService(svc) - if errors.Is(err, annotation.ErrNotSet) { - return fmt.Errorf("%s: no domains for managed certificate", op) - } - labels := map[string]string{ - LabelServiceUID: string(svc.ObjectMeta.UID), - } - // It's ok to ignore the error here. We are only interested if the - // annotation is set and parseable as a truthy boolean. Anything else tells - // us we do not want to use ACME staging. - if ok, _ := annotation.LBSvcHTTPManagedCertificateUseACMEStaging.BoolFromService(svc); ok { - labels["HC-Use-Staging-CA"] = "true" - } - err = l.CertOps.CreateManagedCertificate(ctx, name, domains, labels) + + err := l.CertOps.CreateManagedCertificate(ctx, spec.ManagedCertificate.CreateOpts()) if errors.Is(err, ErrAlreadyExists) { return nil } @@ -1093,454 +895,91 @@ func (l *LoadBalancerOps) reconcileManagedCertificate(ctx context.Context, svc * return nil } -type hclbServiceOptsBuilder struct { - Port corev1.ServicePort - Service *corev1.Service - CertOps *CertificateOps - cfg config.LoadBalancerConfiguration - - listenPort int - destinationPort int - proxyProtocol *bool - protocol hcloud.LoadBalancerServiceProtocol - httpOpts struct { - CookieName *string - CookieLifetime *time.Duration - Certificates []*hcloud.Certificate - RedirectHTTP *bool - StickySessions *bool - TimeoutIdle *time.Duration - } - addHTTP bool - healthCheckOpts struct { - Protocol hcloud.LoadBalancerServiceProtocol - Port *int - Interval *time.Duration - Timeout *time.Duration - Retries *int - httpOpts struct { - Domain *string - Path *string - Response *string - StatusCodes []string - TLS *bool - } - } - addHealthCheck bool - - once sync.Once - err error -} - -func (b *hclbServiceOptsBuilder) extract() { - const op = "hcops/hclbServiceOptsBuilder.extract" +// resolveCertificates turns the certificate references of the Service into +// references the API accepts, which means looking up certificates referenced by +// name. A managed certificate is looked up by the label it was created with. +// +// It returns nil when the Service has no certificates configured. +func (l *LoadBalancerOps) resolveCertificates( + ctx context.Context, svc *corev1.Service, spec lbspec.Spec, +) ([]*hcloud.Certificate, error) { + const op = "hcops/LoadBalancerOps.resolveCertificates" metrics.OperationCalled.WithLabelValues(op).Inc() - b.listenPort = int(b.Port.Port) - b.destinationPort = int(b.Port.NodePort) - - b.do(func() error { - pp, err := annotation.LBSvcProxyProtocol.BoolFromService(b.Service) - if err == nil { - b.proxyProtocol = new(pp) - return nil - } - if errors.Is(err, annotation.ErrNotSet) { - b.proxyProtocol = b.cfg.ProxyProtocolEnabled - return nil - } - return fmt.Errorf("%s: %w", op, err) - }) - - b.protocol = hcloud.LoadBalancerServiceProtocolTCP - b.do(func() error { - p, err := annotation.LBSvcProtocol.LBSvcProtocolFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } + if spec.ManagedCertificate != nil { + cert, err := l.CertOps.GetCertificateByLabel(ctx, fmt.Sprintf("%s=%s", LabelServiceUID, svc.ObjectMeta.UID)) if err != nil { - return fmt.Errorf("%s: %w", op, err) + return nil, fmt.Errorf("%s: %w", op, err) } - b.protocol = p - return nil - }) - - if v, ok := annotation.LBSvcHTTPCookieName.StringFromService(b.Service); ok { - b.httpOpts.CookieName = &v - b.addHTTP = true + return []*hcloud.Certificate{{ID: cert.ID}}, nil } - b.do(func() error { - lt, err := annotation.LBSvcHTTPCookieLifetime.DurationFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.httpOpts.CookieLifetime = < - b.addHTTP = true - return nil - }) - - b.do(func() error { - timeout, err := annotation.LBSvcHTTPTimeoutIdle.DurationFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.httpOpts.TimeoutIdle = &timeout - b.addHTTP = true - return nil - }) - - b.do(func() error { - certtyp, ok := annotation.LBSvcHTTPCertificateType.StringFromService(b.Service) - if ok && certtyp == string(hcloud.CertificateTypeManaged) { - // Continue with managed certificates below - return nil - } - - certs, err := annotation.LBSvcHTTPCertificates.CertificatesFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - - certs, err = b.resolveCertsByNameOrID(ctx, certs) - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.httpOpts.Certificates = certs - b.addHTTP = true - return nil - }) - - b.do(func() error { - certtyp, ok := annotation.LBSvcHTTPCertificateType.StringFromService(b.Service) - if !ok || certtyp != string(hcloud.CertificateTypeManaged) { - // Not a a managed certificate. - return nil - } - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - - svcUID := b.Service.ObjectMeta.UID - cert, err := b.CertOps.GetCertificateByLabel(ctx, fmt.Sprintf("%s=%s", LabelServiceUID, svcUID)) - if err != nil { - return err - } - b.httpOpts.Certificates = []*hcloud.Certificate{{ID: cert.ID}} - b.addHTTP = true - return nil - }) - - b.do(func() error { - redirectHTTP, err := annotation.LBSvcRedirectHTTP.BoolFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.httpOpts.RedirectHTTP = &redirectHTTP - b.addHTTP = true - return nil - }) - - b.do(func() error { - stickySessions, err := annotation.LBSvcHTTPStickySessions.BoolFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.httpOpts.StickySessions = &stickySessions - b.addHTTP = true - return nil - }) - - b.extractHealthCheck() -} - -func (b *hclbServiceOptsBuilder) resolveCertsByNameOrID(ctx context.Context, cs []*hcloud.Certificate) ([]*hcloud.Certificate, error) { - const op = "hcops/hclbServiceOptsBuilder.resolveCertsByNameOrID" - metrics.OperationCalled.WithLabelValues(op).Inc() + if spec.Service.HTTP == nil || len(spec.Service.HTTP.Certificates) == 0 { + return nil, nil + } - resolved := make([]*hcloud.Certificate, len(cs)) - for i, c := range cs { + resolved := make([]*hcloud.Certificate, len(spec.Service.HTTP.Certificates)) + for i, c := range spec.Service.HTTP.Certificates { if c.ID != 0 { resolved[i] = c continue } - c, err := b.CertOps.GetCertificateByNameOrID(ctx, c.Name) + cert, err := l.CertOps.GetCertificateByNameOrID(ctx, c.Name) if err != nil { return nil, fmt.Errorf("%s: %w", op, err) } - resolved[i] = &hcloud.Certificate{ID: c.ID} + resolved[i] = &hcloud.Certificate{ID: cert.ID} } return resolved, nil } -func (b *hclbServiceOptsBuilder) extractHealthCheck() { - const op = "hcops/hclbServiceOptsBuilder.extractHealthCheck" - metrics.OperationCalled.WithLabelValues(op).Inc() - - b.do(func() error { - p, err := annotation.LBSvcHealthCheckProtocol.LBSvcProtocolFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - // Set the service protocol but do not set the addHealthCheck flag. - // This way the health check is configured using the service - // protocol only if at least one health check annotation is - // present. - b.healthCheckOpts.Protocol = b.protocol - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.healthCheckOpts.Protocol = p - b.addHealthCheck = true - return nil - }) - - b.do(func() error { - hcPort, err := annotation.LBSvcHealthCheckPort.IntFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.healthCheckOpts.Port = new(hcPort) - b.addHealthCheck = true - return nil - }) - - b.do(func() error { - hcInterval, err := annotation.LBSvcHealthCheckInterval.DurationFromService(b.Service) - if err == nil { - b.healthCheckOpts.Interval = &hcInterval - b.addHealthCheck = true - return nil - } - if errors.Is(err, annotation.ErrNotSet) { - if b.cfg.HealthCheckInterval != 0 { - b.healthCheckOpts.Interval = &b.cfg.HealthCheckInterval - b.addHealthCheck = true - } - return nil - } - return fmt.Errorf("%s: %w", op, err) - }) - - b.do(func() error { - t, err := annotation.LBSvcHealthCheckTimeout.DurationFromService(b.Service) - if err == nil { - b.healthCheckOpts.Timeout = &t - b.addHealthCheck = true - return nil - } - if errors.Is(err, annotation.ErrNotSet) { - if b.cfg.HealthCheckTimeout != 0 { - b.healthCheckOpts.Timeout = &b.cfg.HealthCheckTimeout - b.addHealthCheck = true - } - return nil - } - return fmt.Errorf("%s: %w", op, err) - }) - - b.do(func() error { - v, err := annotation.LBSvcHealthCheckRetries.IntFromService(b.Service) - if err == nil { - b.healthCheckOpts.Retries = &v - b.addHealthCheck = true - return nil - } - if errors.Is(err, annotation.ErrNotSet) { - if b.cfg.HealthCheckRetries != 0 { - b.healthCheckOpts.Retries = &b.cfg.HealthCheckRetries - b.addHealthCheck = true - } - return nil - } - return fmt.Errorf("%s: %w", op, err) - }) - - if b.healthCheckOpts.Protocol == hcloud.LoadBalancerServiceProtocolTCP { - return - } - - if v, ok := annotation.LBSvcHealthCheckHTTPDomain.StringFromService(b.Service); ok { - b.healthCheckOpts.httpOpts.Domain = &v - } - - if v, ok := annotation.LBSvcHealthCheckHTTPPath.StringFromService(b.Service); ok { - b.healthCheckOpts.httpOpts.Path = &v - } - - b.do(func() error { - tls, err := annotation.LBSvcHealthCheckHTTPValidateCertificate.BoolFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.healthCheckOpts.httpOpts.TLS = &tls - return nil - }) - - b.do(func() error { - scs, err := annotation.LBSvcHealthCheckHTTPStatusCodes.StringsFromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } - if err != nil { - return fmt.Errorf("%s: %w", op, err) - } - b.healthCheckOpts.httpOpts.StatusCodes = scs - return nil - }) -} - -func (b *hclbServiceOptsBuilder) initialize() error { - b.once.Do(b.extract) - return b.err -} - -func (b *hclbServiceOptsBuilder) do(f func() error) { - if b.err != nil { - return - } - b.err = f() -} - -func (b *hclbServiceOptsBuilder) buildAddServiceOpts() (hcloud.LoadBalancerAddServiceOpts, error) { - const op = "hcops/hclbServiceOptsBuilder.buildAddServiceOpts" - metrics.OperationCalled.WithLabelValues(op).Inc() +// verifyType looks up the Load Balancer type requested by spec and warns about a +// type that is unconfigured, deprecated or unavailable. +func (l *LoadBalancerOps) verifyType( + ctx context.Context, svc *corev1.Service, spec lbspec.Spec, +) (*hcloud.LoadBalancerType, error) { + ctx = cache.SetSubsystem(ctx, loadBalancerSubsystem) - if err := b.initialize(); err != nil { - return hcloud.LoadBalancerAddServiceOpts{}, fmt.Errorf("%s: %w", op, err) + if spec.TypeUnset { + utils.WarnEventLogf( + l.Recorder, + svc, + "LoadBalancerTypeUnconfigured", + "Load Balancer Type unconfigured: this will be required in the future, set it with the annotation %q or cluster-wide with the environment variable %q", + annotation.LBType, + config.HcloudLoadBalancersType, + ) } - opts := hcloud.LoadBalancerAddServiceOpts{ - ListenPort: new(b.listenPort), - DestinationPort: new(b.destinationPort), - Protocol: b.protocol, - Proxyprotocol: b.proxyProtocol, - } - if b.addHTTP { - opts.HTTP = &hcloud.LoadBalancerAddServiceOptsHTTP{ - CookieName: b.httpOpts.CookieName, - CookieLifetime: b.httpOpts.CookieLifetime, - Certificates: b.httpOpts.Certificates, - RedirectHTTP: b.httpOpts.RedirectHTTP, - StickySessions: b.httpOpts.StickySessions, - TimeoutIdle: b.httpOpts.TimeoutIdle, - } - } - if b.addHealthCheck { - port := b.healthCheckOpts.Port - if port == nil { - port = new(b.destinationPort) - } - opts.HealthCheck = &hcloud.LoadBalancerAddServiceOptsHealthCheck{ - Protocol: b.healthCheckOpts.Protocol, - Interval: b.healthCheckOpts.Interval, - Port: port, - Retries: b.healthCheckOpts.Retries, - Timeout: b.healthCheckOpts.Timeout, - } - if b.healthCheckOpts.Protocol == hcloud.LoadBalancerServiceProtocolHTTP || - b.healthCheckOpts.Protocol == hcloud.LoadBalancerServiceProtocolHTTPS { - opts.HealthCheck.HTTP = &hcloud.LoadBalancerAddServiceOptsHealthCheckHTTP{ - Domain: b.healthCheckOpts.httpOpts.Domain, - Path: b.healthCheckOpts.httpOpts.Path, - Response: b.healthCheckOpts.httpOpts.Response, - StatusCodes: b.healthCheckOpts.httpOpts.StatusCodes, - TLS: b.healthCheckOpts.httpOpts.TLS, - } - } - } else { - opts.HealthCheck = &hcloud.LoadBalancerAddServiceOptsHealthCheck{ - Protocol: hcloud.LoadBalancerServiceProtocolTCP, - Port: new(b.destinationPort), - } + lbType, err := l.LBTypeCache.ByName(ctx, spec.Type) + if err != nil { + return nil, err } - return opts, nil -} - -func (b *hclbServiceOptsBuilder) buildUpdateServiceOpts() (hcloud.LoadBalancerUpdateServiceOpts, error) { - const op = "hcops/hclbServiceOptsBuilder.buildUpdateServiceOpts" - metrics.OperationCalled.WithLabelValues(op).Inc() - - if err := b.initialize(); err != nil { - return hcloud.LoadBalancerUpdateServiceOpts{}, fmt.Errorf("%s: %w", op, err) + if lbType == nil { + return nil, fmt.Errorf("load balancer type not found: %s", spec.Type) } - opts := hcloud.LoadBalancerUpdateServiceOpts{ - DestinationPort: new(b.destinationPort), - Protocol: b.protocol, - Proxyprotocol: b.proxyProtocol, - } - if b.addHTTP { - opts.HTTP = &hcloud.LoadBalancerUpdateServiceOptsHTTP{ - CookieName: b.httpOpts.CookieName, - CookieLifetime: b.httpOpts.CookieLifetime, - RedirectHTTP: b.httpOpts.RedirectHTTP, - Certificates: b.httpOpts.Certificates, - StickySessions: b.httpOpts.StickySessions, - TimeoutIdle: b.httpOpts.TimeoutIdle, - } + msg, unavailable := deprecationutil.LoadBalancerTypeMessage(lbType) + if unavailable { + return nil, errors.New(msg) } - if b.addHealthCheck { - port := b.healthCheckOpts.Port - if port == nil { - port = new(b.destinationPort) - } - opts.HealthCheck = &hcloud.LoadBalancerUpdateServiceOptsHealthCheck{ - Protocol: b.healthCheckOpts.Protocol, - Interval: b.healthCheckOpts.Interval, - Port: port, - Retries: b.healthCheckOpts.Retries, - Timeout: b.healthCheckOpts.Timeout, - } - if b.healthCheckOpts.Protocol == hcloud.LoadBalancerServiceProtocolHTTP || - b.healthCheckOpts.Protocol == hcloud.LoadBalancerServiceProtocolHTTPS { - opts.HealthCheck.HTTP = &hcloud.LoadBalancerUpdateServiceOptsHealthCheckHTTP{ - Domain: b.healthCheckOpts.httpOpts.Domain, - Path: b.healthCheckOpts.httpOpts.Path, - Response: b.healthCheckOpts.httpOpts.Response, - StatusCodes: b.healthCheckOpts.httpOpts.StatusCodes, - TLS: b.healthCheckOpts.httpOpts.TLS, - } - } - } else { - opts.HealthCheck = &hcloud.LoadBalancerUpdateServiceOptsHealthCheck{ - Protocol: hcloud.LoadBalancerServiceProtocolTCP, - Port: new(b.destinationPort), - } + if msg != "" { + utils.WarnEventLogf( + l.Recorder, + svc, + "LoadBalancerTypeDeprecated", + "%s", msg, + ) } - return opts, nil + return lbType, nil } -func lbAttached(lb *hcloud.LoadBalancer, nwID int64, privateIPv4 string) bool { +func lbAttached(lb *hcloud.LoadBalancer, nwID int64, privateIPv4 net.IP) bool { for _, nw := range lb.PrivateNet { - if nw.Network.ID == nwID && (privateIPv4 == "" || privateIPv4 == nw.IP.String()) { + if nw.Network.ID == nwID && (privateIPv4 == nil || privateIPv4.Equal(nw.IP)) { return true } } diff --git a/internal/hcops/load_balancer_internal_test.go b/internal/hcops/load_balancer_internal_test.go index ebbd75b3b..48397fabb 100644 --- a/internal/hcops/load_balancer_internal_test.go +++ b/internal/hcops/load_balancer_internal_test.go @@ -1,28 +1,35 @@ package hcops import ( + "context" "fmt" + "maps" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/mocks" "github.com/hetznercloud/hcloud-go/v2/hcloud" ) -func TestHCLBServiceOptsBuilder(t *testing.T) { +// TestBuildServiceOpts covers the path from the annotations of a Service to the +// options sent to the API: [lbspec.Resolve], the certificate lookups and the +// two opts builders. +func TestBuildServiceOpts(t *testing.T) { type testCase struct { name string servicePort corev1.ServicePort serviceUID string - serviceAnnotations map[annotation.Name]string + serviceAnnotations map[string]string cfg config.LoadBalancerConfiguration expectedAddOpts hcloud.LoadBalancerAddServiceOpts expectedUpdateOpts hcloud.LoadBalancerUpdateServiceOpts @@ -58,8 +65,8 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { { name: "enable proxy protocol", servicePort: corev1.ServicePort{Port: 81, NodePort: 8081}, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcProxyProtocol: "true", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcProxyProtocol): "true", }, expectedAddOpts: hcloud.LoadBalancerAddServiceOpts{ ListenPort: new(81), @@ -87,8 +94,8 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { cfg: config.LoadBalancerConfiguration{ ProxyProtocolEnabled: new(true), }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcProxyProtocol: "false", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcProxyProtocol): "false", }, expectedAddOpts: hcloud.LoadBalancerAddServiceOpts{ ListenPort: new(86), @@ -113,14 +120,14 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { { name: "select HTTP protocol", servicePort: corev1.ServicePort{Port: 82, NodePort: 8082}, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcProtocol: string(hcloud.LoadBalancerServiceProtocolHTTP), - annotation.LBSvcHTTPCookieName: "my-cookie", - annotation.LBSvcHTTPCookieLifetime: "1h", - annotation.LBSvcHTTPCertificates: "1,3", - annotation.LBSvcRedirectHTTP: "true", - annotation.LBSvcHTTPStickySessions: "true", - annotation.LBSvcHTTPTimeoutIdle: "30s", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcProtocol): string(hcloud.LoadBalancerServiceProtocolHTTP), + string(annotation.LBSvcHTTPCookieName): "my-cookie", + string(annotation.LBSvcHTTPCookieLifetime): "1h", + string(annotation.LBSvcHTTPCertificates): "1,3", + string(annotation.LBSvcRedirectHTTP): "true", + string(annotation.LBSvcHTTPStickySessions): "true", + string(annotation.LBSvcHTTPTimeoutIdle): "30s", }, expectedAddOpts: hcloud.LoadBalancerAddServiceOpts{ ListenPort: new(82), @@ -159,9 +166,9 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { { name: "add certificates by name", servicePort: corev1.ServicePort{Port: 83, NodePort: 8083}, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcProtocol: string(hcloud.LoadBalancerServiceProtocolHTTPS), - annotation.LBSvcHTTPCertificates: "cert-1,cert-2", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcProtocol): string(hcloud.LoadBalancerServiceProtocolHTTPS), + string(annotation.LBSvcHTTPCertificates): "cert-1,cert-2", }, mock: func(_ *testing.T, tt *testCase) { tt.certClient. @@ -231,10 +238,10 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { name: "add managed certificate by service uid label", servicePort: corev1.ServicePort{Port: 83, NodePort: 8083}, serviceUID: "some-service-uid", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcProtocol: string(hcloud.LoadBalancerServiceProtocolHTTPS), - annotation.LBSvcHTTPCertificateType: "managed", - annotation.LBSvcHTTPManagedCertificateDomains: "*.example.com,example.com", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcProtocol): string(hcloud.LoadBalancerServiceProtocolHTTPS), + string(annotation.LBSvcHTTPCertificateType): "managed", + string(annotation.LBSvcHTTPManagedCertificateDomains): "*.example.com,example.com", }, mock: func(_ *testing.T, tt *testCase) { tt.certClient. @@ -272,9 +279,9 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { { name: "add health check with default protocol", servicePort: corev1.ServicePort{Port: 83, NodePort: 8083}, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcProtocol: string(hcloud.LoadBalancerServiceProtocolTCP), - annotation.LBSvcHealthCheckPort: "8084", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcProtocol): string(hcloud.LoadBalancerServiceProtocolTCP), + string(annotation.LBSvcHealthCheckPort): "8084", }, expectedAddOpts: hcloud.LoadBalancerAddServiceOpts{ ListenPort: new(83), @@ -297,12 +304,12 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { { name: "add TCP health check", servicePort: corev1.ServicePort{Port: 83, NodePort: 8083}, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcHealthCheckProtocol: string(hcloud.LoadBalancerServiceProtocolTCP), - annotation.LBSvcHealthCheckPort: "8084", - annotation.LBSvcHealthCheckInterval: "1h", - annotation.LBSvcHealthCheckTimeout: "30s", - annotation.LBSvcHealthCheckRetries: "5", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcHealthCheckProtocol): string(hcloud.LoadBalancerServiceProtocolTCP), + string(annotation.LBSvcHealthCheckPort): "8084", + string(annotation.LBSvcHealthCheckInterval): "1h", + string(annotation.LBSvcHealthCheckTimeout): "30s", + string(annotation.LBSvcHealthCheckRetries): "5", }, expectedAddOpts: hcloud.LoadBalancerAddServiceOpts{ ListenPort: new(83), @@ -331,16 +338,16 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { { name: "add HTTP health check", servicePort: corev1.ServicePort{Port: 84, NodePort: 8084}, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcHealthCheckProtocol: string(hcloud.LoadBalancerServiceProtocolHTTP), - annotation.LBSvcHealthCheckPort: "8085", - annotation.LBSvcHealthCheckInterval: "1h", - annotation.LBSvcHealthCheckTimeout: "30s", - annotation.LBSvcHealthCheckRetries: "5", - annotation.LBSvcHealthCheckHTTPDomain: "example.com", - annotation.LBSvcHealthCheckHTTPPath: "/internal/health", - annotation.LBSvcHealthCheckHTTPValidateCertificate: "true", - annotation.LBSvcHealthCheckHTTPStatusCodes: "200,202", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcHealthCheckProtocol): string(hcloud.LoadBalancerServiceProtocolHTTP), + string(annotation.LBSvcHealthCheckPort): "8085", + string(annotation.LBSvcHealthCheckInterval): "1h", + string(annotation.LBSvcHealthCheckTimeout): "30s", + string(annotation.LBSvcHealthCheckRetries): "5", + string(annotation.LBSvcHealthCheckHTTPDomain): "example.com", + string(annotation.LBSvcHealthCheckHTTPPath): "/internal/health", + string(annotation.LBSvcHealthCheckHTTPValidateCertificate): "true", + string(annotation.LBSvcHealthCheckHTTPStatusCodes): "200,202", }, expectedAddOpts: hcloud.LoadBalancerAddServiceOpts{ ListenPort: new(84), @@ -381,15 +388,15 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { { name: "health check port defaults to node port/destination Port if not specified", servicePort: corev1.ServicePort{Port: 84, NodePort: 8084}, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcHealthCheckProtocol: string(hcloud.LoadBalancerServiceProtocolHTTP), - annotation.LBSvcHealthCheckInterval: "1h", - annotation.LBSvcHealthCheckTimeout: "30s", - annotation.LBSvcHealthCheckRetries: "5", - annotation.LBSvcHealthCheckHTTPDomain: "example.com", - annotation.LBSvcHealthCheckHTTPPath: "/internal/health", - annotation.LBSvcHealthCheckHTTPValidateCertificate: "true", - annotation.LBSvcHealthCheckHTTPStatusCodes: "200,202", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcHealthCheckProtocol): string(hcloud.LoadBalancerServiceProtocolHTTP), + string(annotation.LBSvcHealthCheckInterval): "1h", + string(annotation.LBSvcHealthCheckTimeout): "30s", + string(annotation.LBSvcHealthCheckRetries): "5", + string(annotation.LBSvcHealthCheckHTTPDomain): "example.com", + string(annotation.LBSvcHealthCheckHTTPPath): "/internal/health", + string(annotation.LBSvcHealthCheckHTTPValidateCertificate): "true", + string(annotation.LBSvcHealthCheckHTTPStatusCodes): "200,202", }, expectedAddOpts: hcloud.LoadBalancerAddServiceOpts{ ListenPort: new(84), @@ -440,26 +447,27 @@ func TestHCLBServiceOptsBuilder(t *testing.T) { tt.mock(t, &tt) } - builder := &hclbServiceOptsBuilder{ - Port: tt.servicePort, - Service: &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - UID: types.UID(tt.serviceUID), - Annotations: map[string]string{}, - }, + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + UID: types.UID(tt.serviceUID), + Annotations: map[string]string{}, }, - CertOps: &CertificateOps{ActionClient: tt.actionClient, CertClient: tt.certClient}, - cfg: tt.cfg, } - for k, v := range tt.serviceAnnotations { - builder.Service.Annotations[string(k)] = v + maps.Copy(svc.Annotations, tt.serviceAnnotations) + + spec, err := lbspec.Resolve(svc, tt.cfg) + require.NoError(t, err) + + lbOps := &LoadBalancerOps{ + CertOps: &CertificateOps{ActionClient: tt.actionClient, CertClient: tt.certClient}, } - addOpts, err := builder.buildAddServiceOpts() - assert.NoError(t, err) + certificates, err := lbOps.resolveCertificates(context.Background(), svc, spec) + require.NoError(t, err) + + addOpts := spec.Service.AddServiceOpts(tt.servicePort, certificates) assert.Equal(t, tt.expectedAddOpts, addOpts) - updateOpts, err := builder.buildUpdateServiceOpts() - assert.NoError(t, err) + updateOpts := spec.Service.UpdateServiceOpts(tt.servicePort, certificates) assert.Equal(t, tt.expectedUpdateOpts, updateOpts) }) } diff --git a/internal/hcops/load_balancer_test.go b/internal/hcops/load_balancer_test.go index b21d179f6..0531ce6d2 100644 --- a/internal/hcops/load_balancer_test.go +++ b/internal/hcops/load_balancer_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "maps" "math/rand" "net" "testing" @@ -243,7 +244,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { type testCase struct { name string cfg config.HCCMConfiguration - serviceAnnotations map[annotation.Name]string + serviceAnnotations map[string]string createOpts hcloud.LoadBalancerCreateOpts mock func(t *testing.T, tt *testCase, fx *hcops.LoadBalancerOpsFixture) lb *hcloud.LoadBalancer @@ -257,8 +258,9 @@ func TestLoadBalancerOps_Create(t *testing.T) { Location: "hel1", }, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBLocation: "fsn1", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "some-lb", + string(annotation.LBLocation): "fsn1", }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "some-lb", @@ -279,8 +281,9 @@ func TestLoadBalancerOps_Create(t *testing.T) { NetworkZone: "eu-central", }, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBNetworkZone: "eu-central", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", + string(annotation.LBNetworkZone): "eu-central", }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "another-lb", @@ -299,6 +302,9 @@ func TestLoadBalancerOps_Create(t *testing.T) { Location: "fsn1", }, }, + serviceAnnotations: map[string]string{ + string(annotation.LBName): "some-lb", + }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "some-lb", LoadBalancerType: &hcloud.LoadBalancerType{ID: 1, Name: "lb11"}, @@ -318,6 +324,9 @@ func TestLoadBalancerOps_Create(t *testing.T) { NetworkZone: "eu-central", }, }, + serviceAnnotations: map[string]string{ + string(annotation.LBName): "some-lb", + }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "some-lb", LoadBalancerType: &hcloud.LoadBalancerType{ID: 1, Name: "lb11"}, @@ -335,9 +344,10 @@ func TestLoadBalancerOps_Create(t *testing.T) { Location: "hel1", }, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBLocation: "", - annotation.LBNetworkZone: "eu-central", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", + string(annotation.LBLocation): "", + string(annotation.LBNetworkZone): "eu-central", }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "another-lb", @@ -356,9 +366,10 @@ func TestLoadBalancerOps_Create(t *testing.T) { NetworkZone: "eu-central", }, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBLocation: "fsn1", - annotation.LBNetworkZone: "", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", + string(annotation.LBLocation): "fsn1", + string(annotation.LBNetworkZone): "", }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "another-lb", @@ -372,17 +383,35 @@ func TestLoadBalancerOps_Create(t *testing.T) { }, lb: &hcloud.LoadBalancer{ID: 2}, }, + { + name: "create derives the name from the service uid", + serviceAnnotations: map[string]string{ + string(annotation.LBLocation): "fsn1", + }, + createOpts: hcloud.LoadBalancerCreateOpts{ + Name: "asomelbuid", + LoadBalancerType: &hcloud.LoadBalancerType{ID: 1, Name: "lb11"}, + Location: &hcloud.Location{ + Name: "fsn1", + }, + Labels: map[string]string{ + hcops.LabelServiceUID: "some-lb-uid", + }, + }, + lb: &hcloud.LoadBalancer{ID: 1}, + }, { name: "fails if location and network zone missing", - serviceAnnotations: map[annotation.Name]string{}, + serviceAnnotations: map[string]string{}, err: fmt.Errorf("hcops/LoadBalancerOps.Create: neither %s nor %s set", annotation.LBLocation, annotation.LBNetworkZone), }, { name: "gives preference to location name", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBLocation: "nbg1", - annotation.LBNetworkZone: "eu-central", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", + string(annotation.LBLocation): "nbg1", + string(annotation.LBNetworkZone): "eu-central", }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "another-lb", @@ -396,9 +425,10 @@ func TestLoadBalancerOps_Create(t *testing.T) { }, { name: "set Load Balancer type name", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBType: "lb21", - annotation.LBLocation: "nbg1", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", + string(annotation.LBType): "lb21", + string(annotation.LBLocation): "nbg1", }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "another-lb", @@ -412,9 +442,10 @@ func TestLoadBalancerOps_Create(t *testing.T) { }, { name: "set Load Balancer algorithm type", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBLocation: "nbg1", - annotation.LBAlgorithmType: "least_connections", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", + string(annotation.LBLocation): "nbg1", + string(annotation.LBAlgorithmType): "least_connections", }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "another-lb", @@ -435,6 +466,9 @@ func TestLoadBalancerOps_Create(t *testing.T) { Type: "lb21", }, }, + serviceAnnotations: map[string]string{ + string(annotation.LBName): "lb-default-type", + }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "lb-default-type", LoadBalancerType: &hcloud.LoadBalancerType{ID: 2, Name: "lb21"}, @@ -453,6 +487,9 @@ func TestLoadBalancerOps_Create(t *testing.T) { DisablePublicNetwork: new(true), }, }, + serviceAnnotations: map[string]string{ + string(annotation.LBName): "lb-disable-public", + }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "lb-disable-public", LoadBalancerType: &hcloud.LoadBalancerType{ID: 1, Name: "lb11"}, @@ -466,17 +503,19 @@ func TestLoadBalancerOps_Create(t *testing.T) { }, { name: "fail on invalid Load Balancer algorithm type", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBLocation: "nbg1", - annotation.LBAlgorithmType: "invalidType", + serviceAnnotations: map[string]string{ + string(annotation.LBLocation): "nbg1", + string(annotation.LBAlgorithmType): "invalidType", }, - err: fmt.Errorf("hcops/LoadBalancerOps.Create: annotation/Name.LBAlgorithmTypeFromService: annotation/validateAlgorithmType: invalid: invalidtype"), + err: fmt.Errorf("hcops/LoadBalancerOps.Create: invalid Load Balancer annotations: " + + "load-balancer.hetzner.cloud/algorithm-type: invalid: invalidType"), }, { name: "disable public interface", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBLocation: "nbg1", - annotation.LBDisablePublicNetwork: "true", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "lb-with-priv", + string(annotation.LBLocation): "nbg1", + string(annotation.LBDisablePublicNetwork): "true", }, createOpts: hcloud.LoadBalancerCreateOpts{ Name: "lb-with-priv", @@ -520,11 +559,9 @@ func TestLoadBalancerOps_Create(t *testing.T) { Annotations: map[string]string{}, }, } - for k, v := range tt.serviceAnnotations { - service.Annotations[string(k)] = v - } + maps.Copy(service.Annotations, tt.serviceAnnotations) - lb, err := fx.LBOps.Create(fx.Ctx, tt.createOpts.Name, service) + lb, err := fx.LBOps.Create(fx.Ctx, service) if tt.err != nil { assert.EqualError(t, err, tt.err.Error()) } else { @@ -578,7 +615,7 @@ type LBReconcilementTestCase struct { name string cfg config.HCCMConfiguration serviceUID string - serviceAnnotations map[annotation.Name]string + serviceAnnotations map[string]string servicePorts []corev1.ServicePort k8sNodes []*corev1.Node initialLB *hcloud.LoadBalancer @@ -606,9 +643,7 @@ func (tt *LBReconcilementTestCase) run(t *testing.T) { }, } } - for k, v := range tt.serviceAnnotations { - tt.service.Annotations[string(k)] = v - } + maps.Copy(tt.service.Annotations, tt.serviceAnnotations) if tt.mock != nil { tt.mock(t, tt) } @@ -620,8 +655,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { tests := []LBReconcilementTestCase{ { name: "update algorithm", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBAlgorithmType: string(hcloud.LoadBalancerAlgorithmTypeLeastConnections), + serviceAnnotations: map[string]string{ + string(annotation.LBAlgorithmType): string(hcloud.LoadBalancerAlgorithmTypeLeastConnections), }, initialLB: &hcloud.LoadBalancer{ ID: 1, @@ -649,8 +684,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "update to invalid algorithm", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBAlgorithmType: "invalidType", + serviceAnnotations: map[string]string{ + string(annotation.LBAlgorithmType): "invalidType", }, initialLB: &hcloud.LoadBalancer{ ID: 2, @@ -664,14 +699,15 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { perform: func(t *testing.T, tt *LBReconcilementTestCase) { changed, err := tt.fx.LBOps.ReconcileHCLB(tt.fx.Ctx, tt.initialLB, tt.service) assert.EqualError(t, err, - "hcops/LoadBalancerOps.ReconcileHCLB: hcops/LoadBalancerOps.changeAlgorithm: annotation/Name.LBAlgorithmTypeFromService: annotation/validateAlgorithmType: invalid: invalidtype") + "hcops/LoadBalancerOps.ReconcileHCLB: invalid Load Balancer annotations: "+ + "load-balancer.hetzner.cloud/algorithm-type: invalid: invalidType") assert.False(t, changed) }, }, { name: "don't update unchanged algorithm", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBAlgorithmType: string(hcloud.LoadBalancerAlgorithmTypeRoundRobin), + serviceAnnotations: map[string]string{ + string(annotation.LBAlgorithmType): string(hcloud.LoadBalancerAlgorithmTypeRoundRobin), }, initialLB: &hcloud.LoadBalancer{ ID: 3, @@ -690,8 +726,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "update type", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBType: "lb21", + serviceAnnotations: map[string]string{ + string(annotation.LBType): "lb21", }, initialLB: &hcloud.LoadBalancer{ ID: 1, @@ -754,8 +790,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "don't update unchanged type", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBType: "lb21", + serviceAnnotations: map[string]string{ + string(annotation.LBType): "lb21", }, initialLB: &hcloud.LoadBalancer{ ID: 1, @@ -774,8 +810,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "don't update correct IPv4 RNDS", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBPublicIPv4RDNS: "lb.example.com", + serviceAnnotations: map[string]string{ + string(annotation.LBPublicIPv4RDNS): "lb.example.com", }, initialLB: &hcloud.LoadBalancer{ ID: 6, @@ -794,8 +830,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "update incorrect IPv4 RNDS", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBPublicIPv4RDNS: "new-name-lb.example.com", + serviceAnnotations: map[string]string{ + string(annotation.LBPublicIPv4RDNS): "new-name-lb.example.com", }, initialLB: &hcloud.LoadBalancer{ ID: 6, @@ -821,8 +857,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "don't update correct IPv6 RNDS", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBPublicIPv6RDNS: "lb.example.com", + serviceAnnotations: map[string]string{ + string(annotation.LBPublicIPv6RDNS): "lb.example.com", }, initialLB: &hcloud.LoadBalancer{ ID: 6, @@ -841,8 +877,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "update incorrect IPv6 RNDS", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBPublicIPv6RDNS: "new-name-lb.example.com", + serviceAnnotations: map[string]string{ + string(annotation.LBPublicIPv6RDNS): "new-name-lb.example.com", }, initialLB: &hcloud.LoadBalancer{ ID: 6, @@ -907,8 +943,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { Enabled: true, }, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBPrivateIPv4: "10.10.10.2", + serviceAnnotations: map[string]string{ + string(annotation.LBPrivateIPv4): "10.10.10.2", }, mock: func(_ *testing.T, tt *LBReconcilementTestCase) { nw := &hcloud.Network{ID: 14, Name: "some-network"} @@ -993,8 +1029,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { Enabled: true, }, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBPrivateIPv4: "10.10.10.2", + serviceAnnotations: map[string]string{ + string(annotation.LBPrivateIPv4): "10.10.10.2", }, mock: func(_ *testing.T, tt *LBReconcilementTestCase) { nw := &hcloud.Network{ID: 15, Name: "some-network"} @@ -1105,8 +1141,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "disable enabled public network", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBDisablePublicNetwork: "true", + serviceAnnotations: map[string]string{ + string(annotation.LBDisablePublicNetwork): "true", }, initialLB: &hcloud.LoadBalancer{ ID: 6, @@ -1155,8 +1191,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "keep disabled public interface", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBDisablePublicNetwork: "true", + serviceAnnotations: map[string]string{ + string(annotation.LBDisablePublicNetwork): "true", }, initialLB: &hcloud.LoadBalancer{ ID: 7, @@ -1172,8 +1208,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "enable disabled public interface", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBDisablePublicNetwork: "false", + serviceAnnotations: map[string]string{ + string(annotation.LBDisablePublicNetwork): "false", }, initialLB: &hcloud.LoadBalancer{ ID: 8, @@ -1196,8 +1232,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { }, { name: "keep enabled public interface", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBDisablePublicNetwork: "false", + serviceAnnotations: map[string]string{ + string(annotation.LBDisablePublicNetwork): "false", }, initialLB: &hcloud.LoadBalancer{ ID: 9, @@ -1250,8 +1286,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { { name: "rename load balancer", serviceUID: "11", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBName: "new-name", + serviceAnnotations: map[string]string{ + string(annotation.LBName): "new-name", }, initialLB: &hcloud.LoadBalancer{ ID: 11, @@ -1513,8 +1549,8 @@ func TestLoadBalancerOps_ReconcileHCLBTargets(t *testing.T) { {Spec: corev1.NodeSpec{ProviderID: "hcloud://1"}}, {Spec: corev1.NodeSpec{ProviderID: "hcloud://2"}}, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBUsePrivateIP: "true", + serviceAnnotations: map[string]string{ + string(annotation.LBUsePrivateIP): "true", }, initialLB: &hcloud.LoadBalancer{ ID: 3, @@ -1553,8 +1589,8 @@ func TestLoadBalancerOps_ReconcileHCLBTargets(t *testing.T) { k8sNodes: []*corev1.Node{ {Spec: corev1.NodeSpec{ProviderID: "hcloud://1"}}, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBUsePrivateIP: "false", + serviceAnnotations: map[string]string{ + string(annotation.LBUsePrivateIP): "false", }, initialLB: &hcloud.LoadBalancer{ ID: 4, @@ -1799,8 +1835,8 @@ func TestLoadBalancerOps_ReconcileHCLBServices(t *testing.T) { MaxTargets: 25, }, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcHTTPCertificates: "1", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcHTTPCertificates): "1", }, mock: func(_ *testing.T, tt *LBReconcilementTestCase) { opts := hcloud.LoadBalancerAddServiceOpts{ @@ -1837,8 +1873,8 @@ func TestLoadBalancerOps_ReconcileHCLBServices(t *testing.T) { MaxTargets: 25, }, }, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcHTTPCertificates: "some-cert", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcHTTPCertificates): "some-cert", }, mock: func(_ *testing.T, tt *LBReconcilementTestCase) { cert := &hcloud.Certificate{ID: 1} @@ -1871,9 +1907,9 @@ func TestLoadBalancerOps_ReconcileHCLBServices(t *testing.T) { name: "create managed certificate", servicePorts: []corev1.ServicePort{{Port: 443, NodePort: 8443}}, initialLB: &hcloud.LoadBalancer{ID: 11}, - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcHTTPCertificateType: string(hcloud.CertificateTypeManaged), - annotation.LBSvcHTTPManagedCertificateDomains: "example.com,*.example.com", + serviceAnnotations: map[string]string{ + string(annotation.LBSvcHTTPCertificateType): string(hcloud.CertificateTypeManaged), + string(annotation.LBSvcHTTPManagedCertificateDomains): "example.com,*.example.com", }, serviceUID: "some service uid", mock: func(_ *testing.T, tt *LBReconcilementTestCase) { @@ -1922,8 +1958,8 @@ func TestLoadBalancerOps_ReconcileHCLBServices(t *testing.T) { }, { name: "replace hc Load Balancer services", - serviceAnnotations: map[annotation.Name]string{ - annotation.LBSvcProtocol: string(hcloud.LoadBalancerServiceProtocolHTTP), + serviceAnnotations: map[string]string{ + string(annotation.LBSvcProtocol): string(hcloud.LoadBalancerServiceProtocolHTTP), }, servicePorts: []corev1.ServicePort{ {Port: 81, NodePort: 8081}, diff --git a/internal/hcops/mocks.go b/internal/hcops/mocks.go index 77deff677..1e6776cb2 100644 --- a/internal/hcops/mocks.go +++ b/internal/hcops/mocks.go @@ -25,9 +25,9 @@ func (m *MockLoadBalancerOps) GetByID(ctx context.Context, id int64) (*hcloud.Lo } func (m *MockLoadBalancerOps) Create( - ctx context.Context, lbName string, service *corev1.Service, + ctx context.Context, service *corev1.Service, ) (*hcloud.LoadBalancer, error) { - args := m.Called(ctx, lbName, service) + args := m.Called(ctx, service) return mocks.GetLoadBalancerPtr(args, 0), args.Error(1) } diff --git a/internal/lbspec/opts.go b/internal/lbspec/opts.go new file mode 100644 index 000000000..362e4438f --- /dev/null +++ b/internal/lbspec/opts.go @@ -0,0 +1,206 @@ +package lbspec + +import ( + "maps" + + corev1 "k8s.io/api/core/v1" + + "github.com/hetznercloud/hcloud-go/v2/hcloud" +) + +// labelUseStagingCA tells the backend to issue managed certificates through the +// Let's Encrypt staging environment. +const labelUseStagingCA = "HC-Use-Staging-CA" + +func (s Spec) CreateOpts(lbType *hcloud.LoadBalancerType) hcloud.LoadBalancerCreateOpts { + opts := hcloud.LoadBalancerCreateOpts{ + Name: s.Name, + LoadBalancerType: lbType, + Labels: s.Labels, + NetworkZone: s.NetworkZone, + PublicInterface: s.PublicInterface, + } + + if s.Location != "" { + opts.Location = &hcloud.Location{Name: s.Location} + } + if s.Algorithm != "" { + opts.Algorithm = &hcloud.LoadBalancerAlgorithm{Type: s.Algorithm} + } + + return opts +} + +func (s Spec) UpdateOpts(lb *hcloud.LoadBalancer) (hcloud.LoadBalancerUpdateOpts, bool) { + var ( + opts hcloud.LoadBalancerUpdateOpts + update bool + ) + + if !hasLabels(lb.Labels, s.Labels) { + labels := make(map[string]string, len(lb.Labels)+len(s.Labels)) + maps.Copy(labels, lb.Labels) + maps.Copy(labels, s.Labels) + + opts.Labels = labels + update = true + } + + if !s.NameUnset && s.Name != lb.Name { + opts.Name = s.Name + update = true + } + + return opts, update +} + +func (s Spec) ChangeAlgorithmOpts() hcloud.LoadBalancerChangeAlgorithmOpts { + return hcloud.LoadBalancerChangeAlgorithmOpts{Type: s.Algorithm} +} + +func (s Spec) AttachToNetworkOpts(network *hcloud.Network) hcloud.LoadBalancerAttachToNetworkOpts { + return hcloud.LoadBalancerAttachToNetworkOpts{ + Network: network, + IP: s.PrivateIPv4, + IPRange: s.PrivateSubnetIPRange, + } +} + +func (s Spec) AddServerTargetOpts(serverID int64) hcloud.LoadBalancerAddServerTargetOpts { + return hcloud.LoadBalancerAddServerTargetOpts{ + Server: &hcloud.Server{ID: serverID}, + UsePrivateIP: new(s.UsePrivateIP), + } +} + +func (c ManagedCertificate) CreateOpts() hcloud.CertificateCreateOpts { + labels := c.Labels + if c.UseACMEStaging { + // A copy, so that requesting staging does not leave the label behind + // in the spec. + labels = make(map[string]string, len(c.Labels)+1) + maps.Copy(labels, c.Labels) + labels[labelUseStagingCA] = "true" + } + + return hcloud.CertificateCreateOpts{ + Name: c.Name, + Type: hcloud.CertificateTypeManaged, + DomainNames: c.Domains, + Labels: labels, + } +} + +// hasLabels reports whether actual carries every label in required. +func hasLabels(actual, required map[string]string) bool { + for k, v := range required { + if actual[k] != v { + return false + } + } + return true +} + +func (s ServiceSpec) AddServiceOpts( + port corev1.ServicePort, certificates []*hcloud.Certificate, +) hcloud.LoadBalancerAddServiceOpts { + destinationPort := int(port.NodePort) + + opts := hcloud.LoadBalancerAddServiceOpts{ + ListenPort: new(int(port.Port)), + DestinationPort: new(destinationPort), + Protocol: s.Protocol, + Proxyprotocol: s.ProxyProtocol, + HealthCheck: s.addHealthCheckOpts(destinationPort), + } + + if s.HTTP != nil { + opts.HTTP = &hcloud.LoadBalancerAddServiceOptsHTTP{ + CookieName: s.HTTP.CookieName, + CookieLifetime: s.HTTP.CookieLifetime, + Certificates: certificates, + RedirectHTTP: s.HTTP.RedirectHTTP, + StickySessions: s.HTTP.StickySessions, + TimeoutIdle: s.HTTP.TimeoutIdle, + } + } + + return opts +} + +func (s ServiceSpec) addHealthCheckOpts(destinationPort int) *hcloud.LoadBalancerAddServiceOptsHealthCheck { + if s.HealthCheck == nil { + return &hcloud.LoadBalancerAddServiceOptsHealthCheck{ + Protocol: hcloud.LoadBalancerServiceProtocolTCP, + Port: new(destinationPort), + } + } + + port := s.HealthCheck.Port + if port == nil { + port = new(destinationPort) + } + + opts := &hcloud.LoadBalancerAddServiceOptsHealthCheck{ + Protocol: s.HealthCheck.Protocol, + Interval: s.HealthCheck.Interval, + Port: port, + Retries: s.HealthCheck.Retries, + Timeout: s.HealthCheck.Timeout, + } + + if s.HealthCheck.Protocol == hcloud.LoadBalancerServiceProtocolHTTP || + s.HealthCheck.Protocol == hcloud.LoadBalancerServiceProtocolHTTPS { + opts.HTTP = &hcloud.LoadBalancerAddServiceOptsHealthCheckHTTP{ + Domain: s.HealthCheck.HTTP.Domain, + Path: s.HealthCheck.HTTP.Path, + StatusCodes: s.HealthCheck.HTTP.StatusCodes, + TLS: s.HealthCheck.HTTP.TLS, + } + } + + return opts +} + +func (s ServiceSpec) UpdateServiceOpts( + port corev1.ServicePort, certificates []*hcloud.Certificate, +) hcloud.LoadBalancerUpdateServiceOpts { + add := s.AddServiceOpts(port, certificates) + + opts := hcloud.LoadBalancerUpdateServiceOpts{ + DestinationPort: add.DestinationPort, + Protocol: add.Protocol, + Proxyprotocol: add.Proxyprotocol, + } + + if add.HTTP != nil { + opts.HTTP = &hcloud.LoadBalancerUpdateServiceOptsHTTP{ + CookieName: add.HTTP.CookieName, + CookieLifetime: add.HTTP.CookieLifetime, + Certificates: add.HTTP.Certificates, + RedirectHTTP: add.HTTP.RedirectHTTP, + StickySessions: add.HTTP.StickySessions, + TimeoutIdle: add.HTTP.TimeoutIdle, + } + } + + if add.HealthCheck != nil { + opts.HealthCheck = &hcloud.LoadBalancerUpdateServiceOptsHealthCheck{ + Protocol: add.HealthCheck.Protocol, + Interval: add.HealthCheck.Interval, + Port: add.HealthCheck.Port, + Retries: add.HealthCheck.Retries, + Timeout: add.HealthCheck.Timeout, + } + if add.HealthCheck.HTTP != nil { + opts.HealthCheck.HTTP = &hcloud.LoadBalancerUpdateServiceOptsHealthCheckHTTP{ + Domain: add.HealthCheck.HTTP.Domain, + Path: add.HealthCheck.HTTP.Path, + StatusCodes: add.HealthCheck.HTTP.StatusCodes, + TLS: add.HealthCheck.HTTP.TLS, + } + } + } + + return opts +} diff --git a/internal/lbspec/opts_test.go b/internal/lbspec/opts_test.go new file mode 100644 index 000000000..881c5ecf9 --- /dev/null +++ b/internal/lbspec/opts_test.go @@ -0,0 +1,220 @@ +package lbspec_test + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" + "github.com/hetznercloud/hcloud-go/v2/hcloud" +) + +func TestSpecCreateOpts(t *testing.T) { + lbType := &hcloud.LoadBalancerType{ID: 2, Name: "lb21"} + labels := map[string]string{lbspec.LabelServiceUID: "some-uid"} + + t.Run("in a location", func(t *testing.T) { + spec := lbspec.Spec{Name: "some-lb", Labels: labels, Location: "fsn1"} + + opts := spec.CreateOpts(lbType) + + assert.Equal(t, hcloud.LoadBalancerCreateOpts{ + Name: "some-lb", + LoadBalancerType: lbType, + Labels: labels, + Location: &hcloud.Location{Name: "fsn1"}, + }, opts) + }) + + t.Run("in a network zone", func(t *testing.T) { + spec := lbspec.Spec{Name: "some-lb", Labels: labels, NetworkZone: hcloud.NetworkZoneEUCentral} + + opts := spec.CreateOpts(lbType) + + assert.Nil(t, opts.Location) + assert.Equal(t, hcloud.NetworkZoneEUCentral, opts.NetworkZone) + }) + + t.Run("with an algorithm and a disabled public interface", func(t *testing.T) { + spec := lbspec.Spec{ + Name: "some-lb", + Labels: labels, + Location: "fsn1", + Algorithm: hcloud.LoadBalancerAlgorithmTypeLeastConnections, + PublicInterface: new(false), + } + + opts := spec.CreateOpts(lbType) + + assert.Equal(t, &hcloud.LoadBalancerAlgorithm{ + Type: hcloud.LoadBalancerAlgorithmTypeLeastConnections, + }, opts.Algorithm) + assert.Equal(t, new(false), opts.PublicInterface) + }) + + t.Run("unconfigured settings are left to the API", func(t *testing.T) { + spec := lbspec.Spec{Name: "some-lb", Labels: labels, Location: "fsn1"} + + opts := spec.CreateOpts(lbType) + + assert.Nil(t, opts.Algorithm, "an unconfigured algorithm is not sent") + assert.Nil(t, opts.PublicInterface, "an unconfigured public interface is not sent") + }) +} + +func TestSpecUpdateOpts(t *testing.T) { + required := map[string]string{lbspec.LabelServiceUID: "new-uid"} + + t.Run("nothing to update", func(t *testing.T) { + lb := &hcloud.LoadBalancer{ + Name: "some-lb", + Labels: map[string]string{lbspec.LabelServiceUID: "new-uid"}, + } + + _, update := lbspec.Spec{Name: "some-lb", Labels: required}.UpdateOpts(lb) + + assert.False(t, update) + }) + + t.Run("an unset name leaves the name alone", func(t *testing.T) { + lb := &hcloud.LoadBalancer{ + Name: "imported-lb", + Labels: map[string]string{lbspec.LabelServiceUID: "new-uid"}, + } + + opts, update := lbspec.Spec{Name: "derived-name", NameUnset: true, Labels: required}.UpdateOpts(lb) + + assert.False(t, update) + assert.Empty(t, opts.Name) + }) + + t.Run("renames the Load Balancer", func(t *testing.T) { + lb := &hcloud.LoadBalancer{ + Name: "old-name", + Labels: map[string]string{lbspec.LabelServiceUID: "new-uid"}, + } + + opts, update := lbspec.Spec{Name: "new-name", Labels: required}.UpdateOpts(lb) + + require.True(t, update) + assert.Equal(t, "new-name", opts.Name) + assert.Nil(t, opts.Labels, "the labels are already correct") + }) + + t.Run("adds the required labels to a Load Balancer without any", func(t *testing.T) { + lb := &hcloud.LoadBalancer{Name: "some-lb"} + + opts, update := lbspec.Spec{Name: "some-lb", Labels: required}.UpdateOpts(lb) + + require.True(t, update) + assert.Equal(t, map[string]string{lbspec.LabelServiceUID: "new-uid"}, opts.Labels) + }) + + t.Run("replaces a stale label and keeps the others", func(t *testing.T) { + // A Service that was deleted and recreated keeps its Load Balancer, which + // then carries the UID of the previous Service. The required label has to + // win, or every reconcile writes the stale value back. + lb := &hcloud.LoadBalancer{ + Name: "some-lb", + Labels: map[string]string{ + lbspec.LabelServiceUID: "old-uid", + "team": "keep-me", + }, + } + + opts, update := lbspec.Spec{Name: "some-lb", Labels: required}.UpdateOpts(lb) + + require.True(t, update) + assert.Equal(t, map[string]string{ + lbspec.LabelServiceUID: "new-uid", + "team": "keep-me", + }, opts.Labels) + assert.Equal(t, "old-uid", lb.Labels[lbspec.LabelServiceUID], + "the Load Balancer is only updated once the API call succeeds") + }) +} + +func TestSpecChangeAlgorithmOpts(t *testing.T) { + spec := lbspec.Spec{Algorithm: hcloud.LoadBalancerAlgorithmTypeLeastConnections} + + assert.Equal(t, hcloud.LoadBalancerChangeAlgorithmOpts{ + Type: hcloud.LoadBalancerAlgorithmTypeLeastConnections, + }, spec.ChangeAlgorithmOpts()) +} + +func TestSpecAttachToNetworkOpts(t *testing.T) { + network := &hcloud.Network{ID: 4711} + + t.Run("without an address or subnet", func(t *testing.T) { + opts := lbspec.Spec{}.AttachToNetworkOpts(network) + + assert.Equal(t, network, opts.Network) + assert.Nil(t, opts.IP, "the API picks an address") + assert.Nil(t, opts.IPRange) + }) + + t.Run("with an address and a subnet", func(t *testing.T) { + _, subnet, err := net.ParseCIDR("10.0.1.0/24") + require.NoError(t, err) + + spec := lbspec.Spec{ + PrivateIPv4: net.ParseIP("10.0.1.5"), + PrivateSubnetIPRange: subnet, + } + + opts := spec.AttachToNetworkOpts(network) + + assert.True(t, net.ParseIP("10.0.1.5").Equal(opts.IP)) + assert.Equal(t, subnet, opts.IPRange) + }) +} + +func TestSpecAddServerTargetOpts(t *testing.T) { + for _, usePrivateIP := range []bool{true, false} { + spec := lbspec.Spec{UsePrivateIP: usePrivateIP} + + opts := spec.AddServerTargetOpts(42) + + assert.Equal(t, &hcloud.Server{ID: 42}, opts.Server) + assert.Equal(t, new(usePrivateIP), opts.UsePrivateIP) + } +} + +func TestManagedCertificateCreateOpts(t *testing.T) { + t.Run("certificate for the configured domains", func(t *testing.T) { + cert := lbspec.ManagedCertificate{ + Name: "some-cert", + Labels: map[string]string{lbspec.LabelServiceUID: "some-uid"}, + Domains: []string{"example.com", "*.example.com"}, + } + + opts := cert.CreateOpts() + + assert.Equal(t, hcloud.CertificateCreateOpts{ + Name: "some-cert", + Type: hcloud.CertificateTypeManaged, + DomainNames: []string{"example.com", "*.example.com"}, + Labels: map[string]string{lbspec.LabelServiceUID: "some-uid"}, + }, opts) + }) + + t.Run("ACME staging is requested through a label", func(t *testing.T) { + cert := lbspec.ManagedCertificate{ + Name: "some-cert", + Labels: map[string]string{lbspec.LabelServiceUID: "some-uid"}, + Domains: []string{"example.com"}, + UseACMEStaging: true, + } + + opts := cert.CreateOpts() + + assert.Equal(t, map[string]string{ + lbspec.LabelServiceUID: "some-uid", + "HC-Use-Staging-CA": "true", + }, opts.Labels) + assert.Equal(t, map[string]string{lbspec.LabelServiceUID: "some-uid"}, cert.Labels, + "the labels of the spec are not modified") + }) +} diff --git a/internal/lbspec/solver.go b/internal/lbspec/solver.go new file mode 100644 index 000000000..67ab35edc --- /dev/null +++ b/internal/lbspec/solver.go @@ -0,0 +1,284 @@ +package lbspec + +import ( + "errors" + "fmt" + "net" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + cloudprovider "k8s.io/cloud-provider" + + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config" + "github.com/hetznercloud/hcloud-go/v2/hcloud" +) + +// Name returns the effective Load Balancer name for svc: the requested name if +// the annotation is set, otherwise the name derived from the Service UID. +func Name(svc *corev1.Service) string { + if v, err := annotation.LBName.FromService(svc); err == nil && v != "" { + return v + } + return cloudprovider.DefaultLoadBalancerName(svc) +} + +// Resolve builds the desired state from the Service annotations and the +// cluster-wide Load Balancer configuration. Annotations take precedence over +// the configuration, which takes precedence over the built-in defaults. +// +// All invalid annotations are reported together in the returned error. The +// returned Spec is still fully populated, with the offending settings left at +// their fallback, so callers must not use it when err is non-nil. +func Resolve(svc *corev1.Service, cfg config.LoadBalancerConfiguration) (Spec, error) { + var errs []error + var spec Spec + + spec.Name = resolve(&errs, svc, annotation.LBName, "") + if spec.Name == "" { + spec.Name = cloudprovider.DefaultLoadBalancerName(svc) + spec.NameUnset = true + } + + spec.Hostname = resolve(&errs, svc, annotation.LBHostname, "") + spec.NodeSelector = resolveNodeSelector(&errs, svc) + spec.Labels = map[string]string{ + LabelServiceUID: string(svc.ObjectMeta.UID), + } + + spec.Type = resolve(&errs, svc, annotation.LBType, cfg.Type) + if spec.Type == "" { + spec.Type = DefaultType + spec.TypeUnset = true + } + + spec.Location = resolve(&errs, svc, annotation.LBLocation, cfg.Location) + spec.NetworkZone = hcloud.NetworkZone(resolve(&errs, svc, annotation.LBNetworkZone, cfg.NetworkZone)) + if spec.Location != "" { + spec.NetworkZone = "" + } + + spec.Algorithm = resolve(&errs, svc, annotation.LBAlgorithmType, cfg.AlgorithmType) + spec.PublicInterface = negate(resolvePtr(&errs, svc, annotation.LBDisablePublicNetwork, cfg.DisablePublicNetwork)) + spec.IPv4RDNS = resolvePtr(&errs, svc, annotation.LBPublicIPv4RDNS, nil) + spec.IPv6RDNS = resolvePtr(&errs, svc, annotation.LBPublicIPv6RDNS, nil) + spec.PrivateIPv4 = resolve(&errs, svc, annotation.LBPrivateIPv4, nil) + spec.PrivateSubnetIPRange = resolvePrivateSubnetIPRange(&errs, svc, cfg) + spec.UsePrivateIP = resolve(&errs, svc, annotation.LBUsePrivateIP, cfg.PrivateIPEnabled) + spec.PrivateIngress = !resolve(&errs, svc, annotation.LBDisablePrivateIngress, !cfg.PrivateIngressEnabled) + spec.IPv6 = !resolve(&errs, svc, annotation.LBIPv6Disabled, !cfg.IPv6Enabled) + spec.ManagedCertificate = resolveManagedCertificate(&errs, svc) + spec.Service = resolveService(&errs, svc, cfg, spec.ManagedCertificate != nil) + + if len(errs) > 0 { + return spec, fmt.Errorf("invalid Load Balancer annotations: %w", errors.Join(errs...)) + } + return spec, nil +} + +func resolveNodeSelector(errs *[]error, svc *corev1.Service) labels.Selector { + v := resolvePtr(errs, svc, annotation.LBNodeSelector, nil) + if v == nil { + return labels.Everything() + } + + selector, err := labels.Parse(*v) + if err != nil { + *errs = append(*errs, fmt.Errorf("%s: unable to parse the node-selector annotation: %w", + annotation.LBNodeSelector, err)) + return labels.Everything() + } + + return selector +} + +func resolvePrivateSubnetIPRange(errs *[]error, svc *corev1.Service, cfg config.LoadBalancerConfiguration) *net.IPNet { + value := resolvePtr(errs, svc, annotation.PrivateSubnetIPRange, nil) + if value == nil { + if cfg.PrivateSubnetIPRange == "" { + return nil + } + value = &cfg.PrivateSubnetIPRange + } + + _, subnet, err := net.ParseCIDR(*value) + if err != nil { + *errs = append(*errs, fmt.Errorf("invalid private subnet IP range %q: %w", *value, err)) + return nil + } + + return subnet +} + +func resolveManagedCertificate(errs *[]error, svc *corev1.Service) *ManagedCertificate { + // Compared as a raw string: only the exact value selects managed + // certificates. + if typ, err := annotation.LBSvcHTTPCertificateType.FromService(svc); err != nil || typ != string(hcloud.CertificateTypeManaged) { + return nil + } + + cert := ManagedCertificate{ + Name: fmt.Sprintf("ccm-managed-certificate-%s", svc.ObjectMeta.UID), + Labels: map[string]string{LabelServiceUID: string(svc.ObjectMeta.UID)}, + } + if v, err := annotation.LBSvcHTTPManagedCertificateName.FromService(svc); err == nil && v != "" { + cert.Name = v + } + + domains, err := annotation.LBSvcHTTPManagedCertificateDomains.FromService(svc) + if err != nil { + *errs = append(*errs, fmt.Errorf("%s: no domains for managed certificate", + annotation.LBSvcHTTPManagedCertificateDomains)) + return nil + } + cert.Domains = domains + + // The error is ignored on purpose: we are only interested in whether the + // annotation is set and parses as a truthy boolean. Anything else tells us + // not to use ACME staging. + cert.UseACMEStaging, _ = annotation.LBSvcHTTPManagedCertificateUseACMEStaging.FromService(svc) + + return &cert +} + +func resolveService( + errs *[]error, svc *corev1.Service, cfg config.LoadBalancerConfiguration, hasManagedCertificate bool, +) ServiceSpec { + var spec ServiceSpec + + spec.Protocol = resolve(errs, svc, annotation.LBSvcProtocol, hcloud.LoadBalancerServiceProtocolTCP) + spec.ProxyProtocol = resolvePtr(errs, svc, annotation.LBSvcProxyProtocol, cfg.ProxyProtocolEnabled) + + var http HTTPSpec + var httpConfigured bool + + if v := resolvePtr(errs, svc, annotation.LBSvcHTTPCookieName, nil); v != nil { + http.CookieName = v + httpConfigured = true + } + if v := resolvePtr(errs, svc, annotation.LBSvcHTTPCookieLifetime, nil); v != nil { + http.CookieLifetime = v + httpConfigured = true + } + if v := resolvePtr(errs, svc, annotation.LBSvcHTTPTimeoutIdle, nil); v != nil { + http.TimeoutIdle = v + httpConfigured = true + } + if v := resolvePtr(errs, svc, annotation.LBSvcRedirectHTTP, nil); v != nil { + http.RedirectHTTP = v + httpConfigured = true + } + if v := resolvePtr(errs, svc, annotation.LBSvcHTTPStickySessions, nil); v != nil { + http.StickySessions = v + httpConfigured = true + } + + switch { + case hasManagedCertificate: + // The managed certificate replaces the uploaded certificate list, so + // that annotation is not read at all. The caller fills in the + // certificate it looked up by label. + httpConfigured = true + default: + certs := resolve(errs, svc, annotation.LBSvcHTTPCertificates, nil) + if len(certs) > 0 { + http.Certificates = certs + httpConfigured = true + } + } + + if httpConfigured { + spec.HTTP = &http + } + spec.HealthCheck = resolveHealthCheck(errs, svc, cfg, spec.Protocol) + + return spec +} + +func resolveHealthCheck( + errs *[]error, + svc *corev1.Service, + cfg config.LoadBalancerConfiguration, + serviceProtocol hcloud.LoadBalancerServiceProtocol, +) *HealthCheckSpec { + // Without an explicit health check protocol the service protocol is used, + // but that alone does not configure a health check. + check := HealthCheckSpec{Protocol: serviceProtocol} + var configured bool + + if v := resolvePtr(errs, svc, annotation.LBSvcHealthCheckProtocol, nil); v != nil { + check.Protocol = *v + configured = true + } + if v := resolvePtr(errs, svc, annotation.LBSvcHealthCheckPort, nil); v != nil { + check.Port = v + configured = true + } + // The cluster-wide defaults only apply when set to a non-zero value. + if v := resolvePtr(errs, svc, annotation.LBSvcHealthCheckInterval, nonZero(cfg.HealthCheckInterval)); v != nil { + check.Interval = v + configured = true + } + if v := resolvePtr(errs, svc, annotation.LBSvcHealthCheckTimeout, nonZero(cfg.HealthCheckTimeout)); v != nil { + check.Timeout = v + configured = true + } + if v := resolvePtr(errs, svc, annotation.LBSvcHealthCheckRetries, nonZero(cfg.HealthCheckRetries)); v != nil { + check.Retries = v + configured = true + } + + // A TCP health check has no HTTP options. + if check.Protocol != hcloud.LoadBalancerServiceProtocolTCP { + check.HTTP.Domain = resolvePtr(errs, svc, annotation.LBSvcHealthCheckHTTPDomain, nil) + check.HTTP.Path = resolvePtr(errs, svc, annotation.LBSvcHealthCheckHTTPPath, nil) + check.HTTP.TLS = resolvePtr(errs, svc, annotation.LBSvcHealthCheckHTTPValidateCertificate, nil) + check.HTTP.StatusCodes = resolve(errs, svc, annotation.LBSvcHealthCheckHTTPStatusCodes, nil) + } + + if !configured { + return nil + } + return &check +} + +func resolve[T any](errs *[]error, svc *corev1.Service, a annotation.Annotation[T], fallback T) T { + v, err := a.FromService(svc) + switch { + case err == nil: + return v + case errors.Is(err, annotation.ErrNotSet): + return fallback + default: + *errs = append(*errs, err) + return fallback + } +} + +func resolvePtr[T any](errs *[]error, svc *corev1.Service, a annotation.Annotation[T], fallback *T) *T { + v, err := a.FromService(svc) + switch { + case err == nil: + return &v + case errors.Is(err, annotation.ErrNotSet): + return fallback + default: + *errs = append(*errs, err) + return fallback + } +} + +func nonZero[T comparable](v T) *T { + var zero T + if v == zero { + return nil + } + return &v +} + +func negate(v *bool) *bool { + if v == nil { + return nil + } + return new(!*v) +} diff --git a/internal/lbspec/solver_test.go b/internal/lbspec/solver_test.go new file mode 100644 index 000000000..175533ab0 --- /dev/null +++ b/internal/lbspec/solver_test.go @@ -0,0 +1,416 @@ +package lbspec_test + +import ( + "maps" + "net" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" + "github.com/hetznercloud/hcloud-go/v2/hcloud" +) + +func service(uid string, annotations map[string]string) *corev1.Service { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + UID: types.UID(uid), + Annotations: make(map[string]string, len(annotations)), + }, + } + maps.Copy(svc.Annotations, annotations) + return svc +} + +func TestResolve(t *testing.T) { + tests := []struct { + name string + annotations map[string]string + cfg config.LoadBalancerConfiguration + check func(t *testing.T, spec lbspec.Spec) + }{ + { + name: "defaults without annotations or configuration", + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, "asomeuid", spec.Name, "derived from the service uid") + assert.True(t, spec.NameUnset) + assert.Empty(t, spec.Hostname) + assert.True(t, spec.NodeSelector.Empty(), "selects every Node") + assert.Equal(t, map[string]string{lbspec.LabelServiceUID: "some-uid"}, spec.Labels) + assert.Equal(t, lbspec.DefaultType, spec.Type) + assert.True(t, spec.TypeUnset) + assert.Empty(t, spec.Location) + assert.Empty(t, spec.NetworkZone) + assert.Empty(t, spec.Algorithm, "an unconfigured algorithm is left alone") + assert.Nil(t, spec.PublicInterface, "an unconfigured public interface is left alone") + assert.Nil(t, spec.IPv4RDNS) + assert.Nil(t, spec.IPv6RDNS) + assert.Nil(t, spec.PrivateIPv4) + assert.Nil(t, spec.PrivateSubnetIPRange) + assert.False(t, spec.UsePrivateIP) + assert.Nil(t, spec.ManagedCertificate) + assert.Equal(t, hcloud.LoadBalancerServiceProtocolTCP, spec.Service.Protocol) + assert.Nil(t, spec.Service.ProxyProtocol) + assert.Nil(t, spec.Service.HTTP, "no HTTP block without HTTP options") + assert.Nil(t, spec.Service.HealthCheck, "no health check without health check options") + }, + }, + { + name: "name and hostname are separate settings", + annotations: map[string]string{ + string(annotation.LBName): "my-lb", + string(annotation.LBHostname): "lb.example.com", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, "my-lb", spec.Name) + assert.False(t, spec.NameUnset) + assert.Equal(t, "lb.example.com", spec.Hostname) + }, + }, + { + name: "annotations take precedence over the configuration", + cfg: config.LoadBalancerConfiguration{ + Type: "lb21", + Location: "hel1", + AlgorithmType: hcloud.LoadBalancerAlgorithmTypeRoundRobin, + PrivateIPEnabled: true, + }, + annotations: map[string]string{ + string(annotation.LBType): "lb31", + string(annotation.LBLocation): "fsn1", + string(annotation.LBAlgorithmType): "least_connections", + string(annotation.LBUsePrivateIP): "false", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, "lb31", spec.Type) + assert.False(t, spec.TypeUnset) + assert.Equal(t, "fsn1", spec.Location) + assert.Equal(t, hcloud.LoadBalancerAlgorithmTypeLeastConnections, spec.Algorithm) + assert.False(t, spec.UsePrivateIP) + }, + }, + { + name: "configuration applies without annotations", + cfg: config.LoadBalancerConfiguration{ + Type: "lb21", + NetworkZone: "eu-central", + AlgorithmType: hcloud.LoadBalancerAlgorithmTypeLeastConnections, + DisablePublicNetwork: new(true), + PrivateSubnetIPRange: "10.0.0.0/24", + PrivateIPEnabled: true, + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, "lb21", spec.Type) + assert.False(t, spec.TypeUnset) + assert.Equal(t, hcloud.NetworkZone("eu-central"), spec.NetworkZone) + assert.Equal(t, hcloud.LoadBalancerAlgorithmTypeLeastConnections, spec.Algorithm) + assert.Equal(t, new(false), spec.PublicInterface, "DISABLE_PUBLIC_NETWORK=true means disabled") + assert.Equal(t, "10.0.0.0/24", spec.PrivateSubnetIPRange.String()) + assert.True(t, spec.UsePrivateIP) + }, + }, + { + name: "an empty annotation resets a configured location", + cfg: config.LoadBalancerConfiguration{ + Location: "hel1", + }, + annotations: map[string]string{ + string(annotation.LBLocation): "", + string(annotation.LBNetworkZone): "eu-central", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Empty(t, spec.Location) + assert.Equal(t, hcloud.NetworkZone("eu-central"), spec.NetworkZone) + }, + }, + { + name: "a location wins over a network zone", + annotations: map[string]string{ + string(annotation.LBLocation): "nbg1", + string(annotation.LBNetworkZone): "eu-central", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, "nbg1", spec.Location) + assert.Empty(t, spec.NetworkZone, "the API accepts only one of them") + }, + }, + { + name: "disable annotations are resolved as enabled settings", + cfg: config.LoadBalancerConfiguration{ + PrivateIngressEnabled: true, + IPv6Enabled: true, + }, + annotations: map[string]string{ + string(annotation.LBDisablePublicNetwork): "true", + string(annotation.LBDisablePrivateIngress): "true", + string(annotation.LBIPv6Disabled): "true", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, new(false), spec.PublicInterface) + assert.False(t, spec.PrivateIngress) + assert.False(t, spec.IPv6) + }, + }, + { + name: "reverse DNS records tell an empty value from an unset one", + annotations: map[string]string{ + string(annotation.LBPublicIPv4RDNS): "", + string(annotation.LBPublicIPv6RDNS): "lb.example.com", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, new(""), spec.IPv4RDNS, "an empty record is a value, not an omission") + assert.Equal(t, new("lb.example.com"), spec.IPv6RDNS) + }, + }, + { + name: "private network addressing is parsed", + annotations: map[string]string{ + string(annotation.LBPrivateIPv4): "10.0.1.5", + string(annotation.PrivateSubnetIPRange): "10.0.1.0/24", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.True(t, net.ParseIP("10.0.1.5").Equal(spec.PrivateIPv4)) + assert.Equal(t, "10.0.1.0/24", spec.PrivateSubnetIPRange.String()) + }, + }, + { + name: "node selector is parsed", + annotations: map[string]string{ + string(annotation.LBNodeSelector): "environment=production", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, "environment=production", spec.NodeSelector.String()) + }, + }, + { + name: "HTTP options configure the HTTP block", + annotations: map[string]string{ + string(annotation.LBSvcProtocol): "http", + string(annotation.LBSvcHTTPCookieName): "my-cookie", + string(annotation.LBSvcHTTPCookieLifetime): "1h", + string(annotation.LBSvcHTTPTimeoutIdle): "30s", + string(annotation.LBSvcRedirectHTTP): "true", + string(annotation.LBSvcHTTPStickySessions): "true", + string(annotation.LBSvcHTTPCertificates): "1,some-cert", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Equal(t, hcloud.LoadBalancerServiceProtocolHTTP, spec.Service.Protocol) + require.NotNil(t, spec.Service.HTTP) + assert.Equal(t, new("my-cookie"), spec.Service.HTTP.CookieName) + assert.Equal(t, new(time.Hour), spec.Service.HTTP.CookieLifetime) + assert.Equal(t, new(30*time.Second), spec.Service.HTTP.TimeoutIdle) + assert.Equal(t, new(true), spec.Service.HTTP.RedirectHTTP) + assert.Equal(t, new(true), spec.Service.HTTP.StickySessions) + assert.Equal(t, []*hcloud.Certificate{{ID: 1}, {Name: "some-cert"}}, + spec.Service.HTTP.Certificates, "references are kept as written") + }, + }, + { + name: "a managed certificate defaults its name to the service uid", + annotations: map[string]string{ + string(annotation.LBSvcHTTPCertificateType): "managed", + string(annotation.LBSvcHTTPManagedCertificateDomains): "example.com,*.example.com", + string(annotation.LBSvcHTTPCertificates): "ignored", + }, + check: func(t *testing.T, spec lbspec.Spec) { + require.NotNil(t, spec.ManagedCertificate) + assert.Equal(t, "ccm-managed-certificate-some-uid", spec.ManagedCertificate.Name) + assert.Equal(t, []string{"example.com", "*.example.com"}, spec.ManagedCertificate.Domains) + assert.False(t, spec.ManagedCertificate.UseACMEStaging) + + require.NotNil(t, spec.Service.HTTP, "the certificate is attached to an HTTP service") + assert.Empty(t, spec.Service.HTTP.Certificates, + "the managed certificate is looked up by label, and the list annotation is ignored") + }, + }, + { + name: "a managed certificate can be named and use ACME staging", + annotations: map[string]string{ + string(annotation.LBSvcHTTPCertificateType): "managed", + string(annotation.LBSvcHTTPManagedCertificateName): "my-cert", + string(annotation.LBSvcHTTPManagedCertificateDomains): "example.com", + string(annotation.LBSvcHTTPManagedCertificateUseACMEStaging): "true", + }, + check: func(t *testing.T, spec lbspec.Spec) { + require.NotNil(t, spec.ManagedCertificate) + assert.Equal(t, "my-cert", spec.ManagedCertificate.Name) + assert.True(t, spec.ManagedCertificate.UseACMEStaging) + }, + }, + { + name: "an uploaded certificate type is not a managed certificate", + annotations: map[string]string{ + string(annotation.LBSvcHTTPCertificateType): "uploaded", + string(annotation.LBSvcHTTPCertificates): "1", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Nil(t, spec.ManagedCertificate) + require.NotNil(t, spec.Service.HTTP) + assert.Equal(t, []*hcloud.Certificate{{ID: 1}}, spec.Service.HTTP.Certificates) + }, + }, + { + name: "health check options configure a health check", + annotations: map[string]string{ + string(annotation.LBSvcHealthCheckProtocol): "http", + string(annotation.LBSvcHealthCheckPort): "8080", + string(annotation.LBSvcHealthCheckInterval): "1h", + string(annotation.LBSvcHealthCheckTimeout): "30s", + string(annotation.LBSvcHealthCheckRetries): "5", + }, + check: func(t *testing.T, spec lbspec.Spec) { + require.NotNil(t, spec.Service.HealthCheck) + assert.Equal(t, hcloud.LoadBalancerServiceProtocolHTTP, spec.Service.HealthCheck.Protocol) + assert.Equal(t, new(8080), spec.Service.HealthCheck.Port) + assert.Equal(t, new(time.Hour), spec.Service.HealthCheck.Interval) + assert.Equal(t, new(30*time.Second), spec.Service.HealthCheck.Timeout) + assert.Equal(t, new(5), spec.Service.HealthCheck.Retries) + }, + }, + { + name: "cluster-wide health check defaults configure a health check", + cfg: config.LoadBalancerConfiguration{ + HealthCheckInterval: 30 * time.Second, + HealthCheckTimeout: 5 * time.Second, + HealthCheckRetries: 5, + }, + check: func(t *testing.T, spec lbspec.Spec) { + require.NotNil(t, spec.Service.HealthCheck) + assert.Equal(t, hcloud.LoadBalancerServiceProtocolTCP, spec.Service.HealthCheck.Protocol) + assert.Equal(t, new(30*time.Second), spec.Service.HealthCheck.Interval) + assert.Equal(t, new(5*time.Second), spec.Service.HealthCheck.Timeout) + assert.Equal(t, new(5), spec.Service.HealthCheck.Retries) + }, + }, + { + name: "an unset health check protocol follows the service protocol", + annotations: map[string]string{ + string(annotation.LBSvcProtocol): "https", + string(annotation.LBSvcHealthCheckPort): "8080", + }, + check: func(t *testing.T, spec lbspec.Spec) { + require.NotNil(t, spec.Service.HealthCheck) + assert.Equal(t, hcloud.LoadBalancerServiceProtocolHTTPS, spec.Service.HealthCheck.Protocol) + }, + }, + { + name: "health check HTTP options alone do not configure a health check", + annotations: map[string]string{ + string(annotation.LBSvcHealthCheckHTTPPath): "/healthz", + }, + check: func(t *testing.T, spec lbspec.Spec) { + assert.Nil(t, spec.Service.HealthCheck, + "a TCP health check has no HTTP options, so the path alone is not enough") + }, + }, + { + name: "health check HTTP options are read for HTTP health checks", + annotations: map[string]string{ + string(annotation.LBSvcHealthCheckProtocol): "http", + string(annotation.LBSvcHealthCheckHTTPDomain): "example.com", + string(annotation.LBSvcHealthCheckHTTPPath): "/healthz", + string(annotation.LBSvcHealthCheckHTTPValidateCertificate): "true", + string(annotation.LBSvcHealthCheckHTTPStatusCodes): "200,202", + }, + check: func(t *testing.T, spec lbspec.Spec) { + require.NotNil(t, spec.Service.HealthCheck) + assert.Equal(t, new("example.com"), spec.Service.HealthCheck.HTTP.Domain) + assert.Equal(t, new("/healthz"), spec.Service.HealthCheck.HTTP.Path) + assert.Equal(t, new(true), spec.Service.HealthCheck.HTTP.TLS) + assert.Equal(t, []string{"200", "202"}, spec.Service.HealthCheck.HTTP.StatusCodes) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spec, err := lbspec.Resolve(service("some-uid", tt.annotations), tt.cfg) + require.NoError(t, err) + tt.check(t, spec) + }) + } +} + +func TestResolveReportsEveryInvalidAnnotation(t *testing.T) { + svc := service("some-uid", map[string]string{ + string(annotation.LBAlgorithmType): "sideways", + string(annotation.LBUsePrivateIP): "maybe", + string(annotation.LBSvcHealthCheckRetries): "many", + string(annotation.LBPrivateIPv4): "not-an-ip", + string(annotation.PrivateSubnetIPRange): "not-a-cidr", + }) + + _, err := lbspec.Resolve(svc, config.LoadBalancerConfiguration{}) + + require.Error(t, err) + // One reconcile tells the user about all of their typos, not just the first. + assert.ErrorContains(t, err, "sideways") + assert.ErrorContains(t, err, "maybe") + assert.ErrorContains(t, err, "many") + assert.ErrorContains(t, err, "not-an-ip") + assert.ErrorContains(t, err, "not-a-cidr") +} + +func TestResolveErrors(t *testing.T) { + tests := []struct { + name string + annotations map[string]string + wantErr string + }{ + { + name: "invalid algorithm", + annotations: map[string]string{string(annotation.LBAlgorithmType): "sideways"}, + wantErr: "invalid: sideways", + }, + { + name: "invalid service protocol", + annotations: map[string]string{string(annotation.LBSvcProtocol): "smtp"}, + wantErr: "invalid: smtp", + }, + { + name: "invalid node selector", + annotations: map[string]string{string(annotation.LBNodeSelector): "environment=production=staging"}, + wantErr: "unable to parse the node-selector annotation", + }, + { + name: "invalid duration", + annotations: map[string]string{string(annotation.LBSvcHTTPTimeoutIdle): "30 fortnights"}, + wantErr: "30 fortnights", + }, + { + name: "managed certificate without domains", + annotations: map[string]string{ + string(annotation.LBSvcHTTPCertificateType): "managed", + }, + wantErr: "no domains for managed certificate", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := lbspec.Resolve(service("some-uid", tt.annotations), config.LoadBalancerConfiguration{}) + + require.Error(t, err) + assert.ErrorContains(t, err, "invalid Load Balancer annotations") + assert.ErrorContains(t, err, tt.wantErr) + }) + } +} + +func TestName(t *testing.T) { + t.Run("derived from the service uid", func(t *testing.T) { + assert.Equal(t, "a0000000000000000000000000000000", lbspec.Name(service("0000000-0000-0000-0000-000000000000", nil))) + }) + + t.Run("from the annotation", func(t *testing.T) { + svc := service("some-uid", map[string]string{string(annotation.LBName): "my-lb"}) + assert.Equal(t, "my-lb", lbspec.Name(svc)) + }) +} diff --git a/internal/lbspec/spec.go b/internal/lbspec/spec.go new file mode 100644 index 000000000..fe836dd5a --- /dev/null +++ b/internal/lbspec/spec.go @@ -0,0 +1,154 @@ +// Package lbspec resolves the desired state of a Hetzner Cloud Load Balancer +// from the annotations of a Kubernetes Service and the cluster-wide +// configuration. +// +// Resolution is pure: it makes no API calls, emits no events and reads no +// clock. Anything that needs the Hetzner Cloud API - looking up a Load Balancer +// type by name, turning certificate names into IDs - is left to the caller, +// which receives the parsed intent and nothing else. +// +// Every invalid annotation is reported, not just the first one, so a user with +// two typos does not need two reconciles to learn about both. +package lbspec + +import ( + "net" + "time" + + "k8s.io/apimachinery/pkg/labels" + + "github.com/hetznercloud/hcloud-go/v2/hcloud" +) + +const DefaultType = "lb11" + +// LabelServiceUID is a label added to the Hetzner Cloud backend to uniquely +// identify a load balancer managed by Hetzner Cloud Cloud Controller Manager. +const LabelServiceUID = "hcloud-ccm/service-uid" + +// Spec is the desired state of the Hetzner Cloud Load Balancer for a Service. +// +// Pointer fields distinguish "not configured anywhere" (nil) from a configured +// value, so we never ask the API to change something the user never set. +type Spec struct { + // Name is the effective Load Balancer name, never empty. + Name string + // NameUnset reports that Name is derived from the Service UID because no + // name was requested. An unrequested name is not applied to an existing + // Load Balancer, so that one created by other means keeps its own. + NameUnset bool + // Labels are the labels the Load Balancer must carry. + Labels map[string]string + // Hostname is published as the ingress address instead of the IPs when set. + Hostname string + // NodeSelector restricts which Nodes become targets. Never nil. + NodeSelector labels.Selector + + // Type is the Load Balancer type name, never empty. + Type string + // TypeUnset reports that Type holds [DefaultType] because nothing + // configured it. An unconfigured type is not applied to an existing Load + // Balancer, to avoid downgrading it. + TypeUnset bool + + Location string + NetworkZone hcloud.NetworkZone + + // Algorithm is empty when unconfigured, in which case it is left alone. + Algorithm hcloud.LoadBalancerAlgorithmType + // PublicInterface reports whether the public interface should be enabled. + // Nil when unconfigured, in which case it is left alone. + PublicInterface *bool + // IPv4RDNS and IPv6RDNS are nil when unconfigured. The empty string is a + // valid value and resets the record. + IPv4RDNS *string + IPv6RDNS *string + + // PrivateIPv4 is the address the Load Balancer should have in the private + // network. Nil when unconfigured. + PrivateIPv4 net.IP + // PrivateSubnetIPRange is the existing subnet to attach to. Nil when + // unconfigured. + PrivateSubnetIPRange *net.IPNet + // UsePrivateIP makes server targets use their private IP. + UsePrivateIP bool + // PrivateIngress publishes the private IPs as ingress addresses. + PrivateIngress bool + // IPv6 publishes the public IPv6 address as an ingress address. + IPv6 bool + + // Service applies to every port of the Kubernetes Service. Every HTTP and + // health check annotation is read from the Service, so only the listen and + // destination ports differ per port. + Service ServiceSpec + + // ManagedCertificate is set when the Service asks for a managed + // certificate. The certificate itself is created and looked up by the + // caller. + ManagedCertificate *ManagedCertificate +} + +// ServiceSpec is the desired state of the services exposed by the Load +// Balancer. +type ServiceSpec struct { + Protocol hcloud.LoadBalancerServiceProtocol + ProxyProtocol *bool + + // HTTP is nil unless at least one HTTP option is configured, in which case + // no HTTP block is sent to the API at all. + HTTP *HTTPSpec + // HealthCheck is nil unless at least one health check option is + // configured, in which case a TCP check against the destination port is + // used. + HealthCheck *HealthCheckSpec +} + +// HTTPSpec holds the HTTP options of a Load Balancer service. +type HTTPSpec struct { + CookieName *string + CookieLifetime *time.Duration + TimeoutIdle *time.Duration + RedirectHTTP *bool + StickySessions *bool + + // Certificates reference certificates either by ID or by name, exactly as + // the annotation spelled them. Resolving names to IDs needs the API and is + // left to the caller. Empty for a managed certificate, which the caller + // looks up by label. + Certificates []*hcloud.Certificate +} + +// HealthCheckSpec holds the health check options of a Load Balancer service. +type HealthCheckSpec struct { + // Protocol defaults to the service protocol when no health check protocol + // is configured. + Protocol hcloud.LoadBalancerServiceProtocol + // Port is nil when unconfigured, in which case the destination port is + // checked. + Port *int + Interval *time.Duration + Timeout *time.Duration + Retries *int + + // HTTP is only applied for the HTTP and HTTPS protocols, so it is a value: + // its options can all be unset while the block itself is still sent. + HTTP HealthCheckHTTPSpec +} + +// HealthCheckHTTPSpec holds the HTTP options of a health check. +type HealthCheckHTTPSpec struct { + Domain *string + Path *string + TLS *bool + StatusCodes []string +} + +// ManagedCertificate is a certificate the cloud controller manager creates and +// renews on behalf of the Service. +type ManagedCertificate struct { + Name string + Labels map[string]string + Domains []string + // UseACMEStaging is for Hetzner internal testing only. + UseACMEStaging bool +} diff --git a/tests/e2e/helper_test.go b/tests/e2e/helper_test.go index 03e43fd85..f4f1e7c54 100644 --- a/tests/e2e/helper_test.go +++ b/tests/e2e/helper_test.go @@ -41,14 +41,14 @@ const ( // lbCreateTimeoutFor returns the timeout to use when waiting for the given // Load Balancer service to become ready. func lbCreateTimeoutFor(svc *corev1.Service) time.Duration { - certAnnotations := []annotation.Name{ - annotation.LBSvcHTTPCertificates, - annotation.LBSvcHTTPCertificateType, - annotation.LBSvcHTTPManagedCertificateName, - annotation.LBSvcHTTPManagedCertificateDomains, + certAnnotations := []string{ + string(annotation.LBSvcHTTPCertificates), + string(annotation.LBSvcHTTPCertificateType), + string(annotation.LBSvcHTTPManagedCertificateName), + string(annotation.LBSvcHTTPManagedCertificateDomains), } for _, a := range certAnnotations { - if _, ok := svc.Annotations[string(a)]; ok { + if _, ok := svc.Annotations[a]; ok { return lbCreateTimeoutCert } } From d30be30ce4abf43ffa153204d9221ab76acac357 Mon Sep 17 00:00:00 2001 From: lukasmetzner Date: Tue, 18 Aug 2026 14:47:46 +0200 Subject: [PATCH 2/5] refactor: doc generation type --- internal/annotation/load_balancer.go | 59 ---------------------------- tools/doc_generation.go | 33 +++++++++++++++- 2 files changed, 31 insertions(+), 61 deletions(-) diff --git a/internal/annotation/load_balancer.go b/internal/annotation/load_balancer.go index 88edb7687..03dba0467 100644 --- a/internal/annotation/load_balancer.go +++ b/internal/annotation/load_balancer.go @@ -4,102 +4,83 @@ const ( // LBPublicIPv4 is the public IPv4 address assigned to the Load Balancer by // the backend. // - // Type: string // Read-only: true LBPublicIPv4 String = "load-balancer.hetzner.cloud/ipv4" // LBPublicIPv4RDNS is the reverse DNS record assigned to the IPv4 address of // the Load Balancer. // - // Type: string // Read-only: true LBPublicIPv4RDNS String = "load-balancer.hetzner.cloud/ipv4-rdns" // LBPublicIPv6 is the public IPv6 address assigned to the Load Balancer by // the backend. // - // Type: string // Read-only: true LBPublicIPv6 String = "load-balancer.hetzner.cloud/ipv6" // LBPublicIPv6RDNS is the reverse DNS record assigned to the IPv6 address of // the Load Balancer. // - // Type: string // Read-only: true LBPublicIPv6RDNS String = "load-balancer.hetzner.cloud/ipv6-rdns" // LBIPv6Disabled disables the use of IPv6 for the Load Balancer. // Set this annotation if you use external-dns. // - // Type: bool // Default: false LBIPv6Disabled Bool = "load-balancer.hetzner.cloud/ipv6-disabled" // LBName is the name of the Load Balancer. The name will be visible in // the Hetzner Cloud API console. - // - // Type: string LBName String = "load-balancer.hetzner.cloud/name" // LBDisablePublicNetwork disables the public network of the Hetzner Cloud // Load Balancer. It will still have a public network assigned, but all // traffic is routed over the private network. // - // Type: bool // Default: false LBDisablePublicNetwork Bool = "load-balancer.hetzner.cloud/disable-public-network" // LBDisablePrivateIngress disables the use of the private network for // ingress. // - // Type: bool // Default: false LBDisablePrivateIngress Bool = "load-balancer.hetzner.cloud/disable-private-ingress" // LBUsePrivateIP configures the Load Balancer to use the private IP for // Load Balancer server targets. // - // Type: bool // Default: false LBUsePrivateIP Bool = "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. - // - // Type: string LBPrivateIPv4 IP = "load-balancer.hetzner.cloud/private-ipv4" // PrivateSubnetIPRange specifies an existing subnet to which the load balancer will be attached. // The value must be in the CIDR notation. The subnet must belong to the network defined // in the CCM configuration and must already exist. // See: https://docs.hetzner.cloud/reference/cloud#network-actions-add-a-subnet-to-a-network - // - // Type: string PrivateSubnetIPRange String = "load-balancer.hetzner.cloud/private-subnet-ip-range" // LBHostname specifies the hostname of the Load Balancer. This will be // used as ingress address instead of the Load Balancer IP addresses if // specified. - // - // Type: string LBHostname String = "load-balancer.hetzner.cloud/hostname" // LBSvcProtocol specifies the protocol of the service. // - // Type: tcp | http | https // Default: tcp LBSvcProtocol Protocol = "load-balancer.hetzner.cloud/protocol" // LBAlgorithmType specifies the algorithm type of the Load Balancer. // - // Type: round_robin | least_connections // Default: round_robin LBAlgorithmType AlgorithmType = "load-balancer.hetzner.cloud/algorithm-type" // LBType specifies the type of the Load Balancer. // - // Type: string // Default: lb11 LBType String = "load-balancer.hetzner.cloud/type" @@ -112,8 +93,6 @@ const ( // will lead to the load balancer getting new public IPs assigned. // // Mutually exclusive with [LBNetworkZone]. - // - // Type: string LBLocation String = "load-balancer.hetzner.cloud/location" // LBNetworkZone specifies the network zone where the Load Balancer will be @@ -126,8 +105,6 @@ const ( // assigned. // // Mutually exclusive with [LBLocation]. - // - // Type: string LBNetworkZone String = "load-balancer.hetzner.cloud/network-zone" // LBNodeSelector can be set to restrict which Nodes are added as targets to the @@ -138,32 +115,23 @@ const ( // updated and an Event is created with the error message. // // Format: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors - // - // Type: string LBNodeSelector String = "load-balancer.hetzner.cloud/node-selector" // LBSvcProxyProtocol specifies if the Load Balancer services should // use the proxy protocol. // - // Type: bool // Default: false LBSvcProxyProtocol Bool = "load-balancer.hetzner.cloud/uses-proxyprotocol" // LBSvcHTTPCookieName specifies the cookie name when using HTTP or HTTPS // as protocol. - // - // Type: string LBSvcHTTPCookieName String = "load-balancer.hetzner.cloud/http-cookie-name" // LBSvcHTTPCookieLifetime specifies the lifetime of the HTTP cookie. - // - // Type: duration LBSvcHTTPCookieLifetime Duration = "load-balancer.hetzner.cloud/http-cookie-lifetime" // LBSvcHTTPTimeoutIdle specifies the idle timeout for the client and // server side. Must be between 30s and 300s. - // - // Type: duration LBSvcHTTPTimeoutIdle Duration = "load-balancer.hetzner.cloud/http-timeout-idle" // LBSvcHTTPCertificateType defines the type of certificate the Load @@ -175,15 +143,11 @@ const ( // LBSvcHTTPCertificates a comma separated list of IDs or Names of // Certificates assigned to the service. - // - // Type: string LBSvcHTTPCertificates Certificates = "load-balancer.hetzner.cloud/http-certificates" // LBSvcHTTPManagedCertificateName contains the name of the managed // certificate to create by the Cloud Controller manager. Ignored if // [LBSvcHTTPCertificateType] is missing or set to "uploaded". - // - // Type: string LBSvcHTTPManagedCertificateName String = "load-balancer.hetzner.cloud/http-managed-certificate-name" // LBSvcHTTPManagedCertificateUseACMEStaging tells the cloud controller manager to create @@ -193,7 +157,6 @@ const ( // Users should not use this annotation. There is no guarantee that it // remains or continues to function as it currently functions. // - // Type: bool // Default: false // Internal: true LBSvcHTTPManagedCertificateUseACMEStaging Bool = "load-balancer.hetzner.cloud/http-managed-certificate-acme-staging" @@ -202,76 +165,55 @@ const ( // domain names of the managed certificate. // // All domains are used to create a single managed certificate. - // - // Type: string LBSvcHTTPManagedCertificateDomains Strings = "load-balancer.hetzner.cloud/http-managed-certificate-domains" // LBSvcRedirectHTTP create a redirect from HTTP to HTTPS. // - // Type: bool // Default: false LBSvcRedirectHTTP Bool = "load-balancer.hetzner.cloud/http-redirect-http" // LBSvcHTTPStickySessions enables the sticky sessions feature of Hetzner // Cloud HTTP Load Balancers. // - // Type: bool // Default: false LBSvcHTTPStickySessions Bool = "load-balancer.hetzner.cloud/http-sticky-sessions" // LBSvcHealthCheckProtocol sets the protocol the health check should be // performed over. // - // Type: tcp | http | https // Default: tcp LBSvcHealthCheckProtocol Protocol = "load-balancer.hetzner.cloud/health-check-protocol" // LBSvcHealthCheckPort specifies the port the health check is be performed // on. - // - // Type: int LBSvcHealthCheckPort Int = "load-balancer.hetzner.cloud/health-check-port" // LBSvcHealthCheckInterval specifies the interval in which we perform a // health check. - // - // Type: duration LBSvcHealthCheckInterval Duration = "load-balancer.hetzner.cloud/health-check-interval" // LBSvcHealthCheckTimeout specifies the timeout of a single health check. - // - // Type: duration LBSvcHealthCheckTimeout Duration = "load-balancer.hetzner.cloud/health-check-timeout" // LBSvcHealthCheckRetries specifies the number of time a health check is // retried until a target is marked as unhealthy. - // - // Type: int LBSvcHealthCheckRetries Int = "load-balancer.hetzner.cloud/health-check-retries" // LBSvcHealthCheckHTTPDomain specifies the domain we try to access when // performing the health check. - // - // Type: string LBSvcHealthCheckHTTPDomain String = "load-balancer.hetzner.cloud/health-check-http-domain" // LBSvcHealthCheckHTTPPath specifies the path we try to access when // performing the health check. - // - // Type: string LBSvcHealthCheckHTTPPath String = "load-balancer.hetzner.cloud/health-check-http-path" // LBSvcHealthCheckHTTPValidateCertificate specifies whether the health // check should validate the SSL certificate that comes from the target // nodes. - // - // Type: bool LBSvcHealthCheckHTTPValidateCertificate Bool = "load-balancer.hetzner.cloud/health-check-http-validate-certificate" // LBSvcHealthCheckHTTPStatusCodes is a comma separated list of HTTP status // codes which we expect. - // - // Type: string LBSvcHealthCheckHTTPStatusCodes Strings = "load-balancer.hetzner.cloud/http-status-codes" // LBID is the ID assigned to the Hetzner Cloud Load Balancer by the @@ -279,7 +221,6 @@ const ( // // Deprecated: This annotation is not used. It is reserved for possible future use. // - // Type: string // Read-only: true LBID String = "load-balancer.hetzner.cloud/id" ) diff --git a/tools/doc_generation.go b/tools/doc_generation.go index ef54faaff..7d6b5d6ef 100644 --- a/tools/doc_generation.go +++ b/tools/doc_generation.go @@ -16,6 +16,20 @@ type TemplateData struct { ConstTable string } +// docTypes maps the type an annotation is declared with to the type shown in +// the reference documentation. +var docTypes = map[string]string{ + "String": "string", + "Bool": "bool", + "Int": "int", + "Duration": "duration", + "Strings": "string", + "IP": "string", + "Protocol": "tcp | http | https", + "AlgorithmType": "round_robin | least_connections", + "Certificates": "string", +} + type ConstantDocTable struct { entries map[string]*DocEntry } @@ -23,6 +37,7 @@ type ConstantDocTable struct { type DocEntry struct { rawCommentLines []string constName string + declaredType string pos int Description string @@ -38,10 +53,11 @@ func NewDocTable() *ConstantDocTable { } } -func (t *ConstantDocTable) AddEntry(constValue, constName string, pos int) { +func (t *ConstantDocTable) AddEntry(constValue, constName, declaredType string, pos int) { t.entries[constValue] = &DocEntry{ rawCommentLines: make([]string, 0), constName: constName, + declaredType: declaredType, pos: pos, } } @@ -62,6 +78,14 @@ func (t *ConstantDocTable) FromAST(node ast.Node) (*ConstantDocTable, error) { for constValue, entry := range t.entries { commentBuilder := strings.Builder{} + if entry.declaredType != "" { + docType, ok := docTypes[entry.declaredType] + if !ok { + return nil, fmt.Errorf("unknown type %s for %s", entry.declaredType, constValue) + } + entry.Type = docType + } + for i, line := range entry.rawCommentLines { if val := parseMetadataValue(line, "Type: "); val != "" { entry.Type = val @@ -198,7 +222,12 @@ func (t *ConstantDocTable) visitFunc() func(n ast.Node) bool { constName := valueSpec.Names[0].Name value := strings.ReplaceAll(literal.Value, "\"", "") - t.AddEntry(value, constName, pos) + var declaredType string + if ident, ok := valueSpec.Type.(*ast.Ident); ok { + declaredType = ident.Name + } + + t.AddEntry(value, constName, declaredType, pos) for _, comment := range valueSpec.Doc.List { t.AppendComment(value, comment.Text) From b1178f4043ac21ad782e690d5135b3cde2ae08be Mon Sep 17 00:00:00 2001 From: lukasmetzner Date: Wed, 19 Aug 2026 09:30:08 +0200 Subject: [PATCH 3/5] fix: return new slice when matching selector --- hcloud/load_balancers.go | 6 +++++- hcloud/load_balancers_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/hcloud/load_balancers.go b/hcloud/load_balancers.go index bd0c96260..8d3f6b4d1 100644 --- a/hcloud/load_balancers.go +++ b/hcloud/load_balancers.go @@ -288,7 +288,11 @@ func (l *loadBalancers) EnsureLoadBalancerDeleted(ctx context.Context, _ string, } func filterNodes(selector labels.Selector, nodes []*corev1.Node) []*corev1.Node { - return slices.DeleteFunc(nodes, func(n *corev1.Node) bool { + if selector.Empty() { + return nodes + } + + return slices.DeleteFunc(slices.Clone(nodes), func(n *corev1.Node) bool { return !selector.Matches(labels.Set(n.GetLabels())) }) } diff --git a/hcloud/load_balancers_test.go b/hcloud/load_balancers_test.go index e1e39ffa6..2ab2a0255 100644 --- a/hcloud/load_balancers_test.go +++ b/hcloud/load_balancers_test.go @@ -4,6 +4,7 @@ import ( "errors" "net" "reflect" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -933,6 +934,21 @@ func TestLoadBalancer_matchNodeSelector(t *testing.T) { newNodeSelectorNode("node1", map[string]string{"environment": "production"}), }, }, + { + name: "single node selector to select none", + service: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + string(annotation.LBNodeSelector): "environment=production", + }, + }, + }, + k8sNodes: []*corev1.Node{ + newNodeSelectorNode("node1", map[string]string{"environment": "staging"}), + newNodeSelectorNode("node2", map[string]string{"environment": "staging"}), + }, + expected: []*corev1.Node{}, + }, { name: "multiple node selector to select all", service: &corev1.Service{ @@ -959,11 +975,19 @@ func TestLoadBalancer_matchNodeSelector(t *testing.T) { t.Fatal(err) } + // The service controller keeps a reference to the Nodes it hands + // us, so filtering must leave them untouched. + unfiltered := slices.Clone(c.k8sNodes) + nodes := filterNodes(spec.NodeSelector, c.k8sNodes) if !reflect.DeepEqual(nodes, c.expected) { t.Errorf("expected: %+v got %+v", c.expected, nodes) } + + if !reflect.DeepEqual(c.k8sNodes, unfiltered) { + t.Errorf("filterNodes modified the Nodes passed to it: %+v", c.k8sNodes) + } }) } } From ac9f372f358ef46bed03ff1c56bfea4abc1f5bb4 Mon Sep 17 00:00:00 2001 From: lukasmetzner Date: Wed, 19 Aug 2026 09:44:03 +0200 Subject: [PATCH 4/5] fix: load balancer lookup before annotation parsing --- hcloud/load_balancers.go | 16 +++++++++++----- hcloud/load_balancers_test.go | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/hcloud/load_balancers.go b/hcloud/load_balancers.go index 8d3f6b4d1..97273e1e9 100644 --- a/hcloud/load_balancers.go +++ b/hcloud/load_balancers.go @@ -49,11 +49,12 @@ func (l *loadBalancers) GetLoadBalancer( const op = "hcloud/loadBalancers.GetLoadBalancer" metrics.OperationCalled.WithLabelValues(op).Inc() - spec, err := lbspec.Resolve(service, *l.cfg) - if err != nil { - return nil, false, fmt.Errorf("%s: %w", op, err) - } - + // The lookup comes before resolving the annotations on purpose. The service + // controller calls us before deleting a Load Balancer and only removes its + // finalizer once we returned without an error, so reporting that a Load + // Balancer does not exist must not depend on the annotations being valid. + // Otherwise a Service that never got a Load Balancer because one of its + // annotations is invalid could not be deleted either. lb, err := l.lbOps.GetByK8SServiceUID(ctx, service) if err != nil { if errors.Is(err, hcops.ErrNotFound) { @@ -62,6 +63,11 @@ func (l *loadBalancers) GetLoadBalancer( return nil, false, fmt.Errorf("%s: %w", op, err) } + spec, err := lbspec.Resolve(service, *l.cfg) + if err != nil { + return nil, false, fmt.Errorf("%s: %w", op, err) + } + return &corev1.LoadBalancerStatus{Ingress: l.buildLoadBalancerStatusIngress(lb, spec)}, true, nil } diff --git a/hcloud/load_balancers_test.go b/hcloud/load_balancers_test.go index 2ab2a0255..3698491dd 100644 --- a/hcloud/load_balancers_test.go +++ b/hcloud/load_balancers_test.go @@ -60,6 +60,24 @@ func TestLoadBalancers_GetLoadBalancer(t *testing.T) { assert.Equal(t, tt.LB.PublicNet.IPv4.IP.String(), status.Ingress[0].IP) }, }, + { + Name: "report a missing load balancer despite an invalid annotation", + ServiceUID: "1", + ServiceAnnotations: map[string]string{ + string(annotation.LBSvcHealthCheckInterval): "10", + }, + Mock: func(_ *testing.T, tt *LoadBalancerTestCase) { + tt.LBOps. + On("GetByK8SServiceUID", tt.Ctx, tt.Service). + Return(nil, hcops.ErrNotFound) + }, + Perform: func(t *testing.T, tt *LoadBalancerTestCase) { + status, exists, err := tt.LoadBalancers.GetLoadBalancer(tt.Ctx, tt.ClusterName, tt.Service) + assert.NoError(t, err) + assert.False(t, exists) + assert.Nil(t, status) + }, + }, { Name: "get load balancer without host name", ServiceUID: "1", From 3e61fadfc58f58478249a5a41ce4dbf6fa6f8143 Mon Sep 17 00:00:00 2001 From: lukasmetzner Date: Wed, 19 Aug 2026 10:08:46 +0200 Subject: [PATCH 5/5] cleanups --- internal/hcops/load_balancer.go | 12 ++--- internal/hcops/load_balancer_internal_test.go | 2 +- internal/hcops/load_balancer_test.go | 47 ++++++++++--------- internal/lbspec/solver.go | 5 +- tests/e2e/cloud_test.go | 4 +- tests/e2e/helper_test.go | 4 +- 6 files changed, 34 insertions(+), 40 deletions(-) diff --git a/internal/hcops/load_balancer.go b/internal/hcops/load_balancer.go index c29f0d5d0..28b13650f 100644 --- a/internal/hcops/load_balancer.go +++ b/internal/hcops/load_balancer.go @@ -23,13 +23,7 @@ import ( "github.com/hetznercloud/hcloud-go/v2/hcloud/exp/deprecationutil" ) -const ( - // LabelServiceUID is a label added to the Hetzner Cloud backend to uniquely - // identify a load balancer managed by Hetzner Cloud Cloud Controller Manager. - LabelServiceUID = "hcloud-ccm/service-uid" - - loadBalancerSubsystem = "load_balancer" -) +const loadBalancerSubsystem = "load_balancer" // LoadBalancerOps implements all operations regarding Hetzner Cloud Load Balancers. type LoadBalancerOps struct { @@ -57,7 +51,7 @@ func (l *LoadBalancerOps) GetByK8SServiceUID(ctx context.Context, svc *corev1.Se opts := hcloud.LoadBalancerListOpts{ ListOpts: hcloud.ListOpts{ - LabelSelector: fmt.Sprintf("%s=%s", LabelServiceUID, svc.ObjectMeta.UID), + LabelSelector: fmt.Sprintf("%s=%s", lbspec.LabelServiceUID, svc.ObjectMeta.UID), }, } lbs, err := l.LBClient.AllWithOpts(ctx, opts) @@ -907,7 +901,7 @@ func (l *LoadBalancerOps) resolveCertificates( metrics.OperationCalled.WithLabelValues(op).Inc() if spec.ManagedCertificate != nil { - cert, err := l.CertOps.GetCertificateByLabel(ctx, fmt.Sprintf("%s=%s", LabelServiceUID, svc.ObjectMeta.UID)) + cert, err := l.CertOps.GetCertificateByLabel(ctx, fmt.Sprintf("%s=%s", lbspec.LabelServiceUID, svc.ObjectMeta.UID)) if err != nil { return nil, fmt.Errorf("%s: %w", op, err) } diff --git a/internal/hcops/load_balancer_internal_test.go b/internal/hcops/load_balancer_internal_test.go index 48397fabb..0e69f47a7 100644 --- a/internal/hcops/load_balancer_internal_test.go +++ b/internal/hcops/load_balancer_internal_test.go @@ -247,7 +247,7 @@ func TestBuildServiceOpts(t *testing.T) { tt.certClient. On("AllWithOpts", mock.Anything, hcloud.CertificateListOpts{ ListOpts: hcloud.ListOpts{ - LabelSelector: fmt.Sprintf("%s=%s", LabelServiceUID, "some-service-uid"), + LabelSelector: fmt.Sprintf("%s=%s", lbspec.LabelServiceUID, "some-service-uid"), }, }). Return([]*hcloud.Certificate{{ID: 1}}, nil, nil) diff --git a/internal/hcops/load_balancer_test.go b/internal/hcops/load_balancer_test.go index 0531ce6d2..631cfeca6 100644 --- a/internal/hcops/load_balancer_test.go +++ b/internal/hcops/load_balancer_test.go @@ -19,6 +19,7 @@ import ( "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/hcops" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" "github.com/hetznercloud/hcloud-go/v2/hcloud" ) @@ -205,7 +206,7 @@ func TestGetByK8SServiceUID(t *testing.T) { opts := hcloud.LoadBalancerListOpts{ ListOpts: hcloud.ListOpts{ - LabelSelector: fmt.Sprintf("%s=%s", hcops.LabelServiceUID, tt.uid), + LabelSelector: fmt.Sprintf("%s=%s", lbspec.LabelServiceUID, tt.uid), }, } fx.LBClient. @@ -269,7 +270,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { Name: "fsn1", }, Labels: map[string]string{ - hcops.LabelServiceUID: "some-lb-uid", + lbspec.LabelServiceUID: "some-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 1}, @@ -290,7 +291,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { LoadBalancerType: &hcloud.LoadBalancerType{ID: 1, Name: "lb11"}, NetworkZone: hcloud.NetworkZoneEUCentral, Labels: map[string]string{ - hcops.LabelServiceUID: "another-lb-uid", + lbspec.LabelServiceUID: "another-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 2}, @@ -312,7 +313,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { Name: "fsn1", }, Labels: map[string]string{ - hcops.LabelServiceUID: "some-lb-uid", + lbspec.LabelServiceUID: "some-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 3}, @@ -332,7 +333,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { LoadBalancerType: &hcloud.LoadBalancerType{ID: 1, Name: "lb11"}, NetworkZone: hcloud.NetworkZoneEUCentral, Labels: map[string]string{ - hcops.LabelServiceUID: "some-lb-uid", + lbspec.LabelServiceUID: "some-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 4}, @@ -354,7 +355,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { LoadBalancerType: &hcloud.LoadBalancerType{ID: 1, Name: "lb11"}, NetworkZone: hcloud.NetworkZoneEUCentral, Labels: map[string]string{ - hcops.LabelServiceUID: "another-lb-uid", + lbspec.LabelServiceUID: "another-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 2}, @@ -378,7 +379,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { Name: "fsn1", }, Labels: map[string]string{ - hcops.LabelServiceUID: "another-lb-uid", + lbspec.LabelServiceUID: "another-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 2}, @@ -395,7 +396,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { Name: "fsn1", }, Labels: map[string]string{ - hcops.LabelServiceUID: "some-lb-uid", + lbspec.LabelServiceUID: "some-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 1}, @@ -418,7 +419,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { LoadBalancerType: &hcloud.LoadBalancerType{ID: 1, Name: "lb11"}, Location: &hcloud.Location{Name: "nbg1"}, Labels: map[string]string{ - hcops.LabelServiceUID: "another-lb-uid", + lbspec.LabelServiceUID: "another-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 2}, @@ -435,7 +436,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { LoadBalancerType: &hcloud.LoadBalancerType{ID: 2, Name: "lb21"}, Location: &hcloud.Location{Name: "nbg1"}, Labels: map[string]string{ - hcops.LabelServiceUID: "another-lb-uid", + lbspec.LabelServiceUID: "another-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 3}, @@ -453,7 +454,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { Location: &hcloud.Location{Name: "nbg1"}, Algorithm: &hcloud.LoadBalancerAlgorithm{Type: hcloud.LoadBalancerAlgorithmTypeLeastConnections}, Labels: map[string]string{ - hcops.LabelServiceUID: "another-lb-uid", + lbspec.LabelServiceUID: "another-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 4}, @@ -474,7 +475,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { LoadBalancerType: &hcloud.LoadBalancerType{ID: 2, Name: "lb21"}, Location: &hcloud.Location{Name: "nbg1"}, Labels: map[string]string{ - hcops.LabelServiceUID: "lb-default-type-uid", + lbspec.LabelServiceUID: "lb-default-type-uid", }, }, lb: &hcloud.LoadBalancer{ID: 7}, @@ -496,7 +497,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { Location: &hcloud.Location{Name: "nbg1"}, PublicInterface: new(false), Labels: map[string]string{ - hcops.LabelServiceUID: "lb-disable-public-uid", + lbspec.LabelServiceUID: "lb-disable-public-uid", }, }, lb: &hcloud.LoadBalancer{ID: 8}, @@ -523,7 +524,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { Location: &hcloud.Location{Name: "nbg1"}, PublicInterface: new(false), Labels: map[string]string{ - hcops.LabelServiceUID: "lb-with-priv-uid", + lbspec.LabelServiceUID: "lb-with-priv-uid", }, }, mock: func(_ *testing.T, tt *testCase, fx *hcops.LoadBalancerOpsFixture) { @@ -555,7 +556,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { service := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ - UID: types.UID(tt.createOpts.Labels[hcops.LabelServiceUID]), + UID: types.UID(tt.createOpts.Labels[lbspec.LabelServiceUID]), Annotations: map[string]string{}, }, } @@ -1262,13 +1263,13 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { mock: func(_ *testing.T, tt *LBReconcilementTestCase) { updated := *tt.initialLB updated.Labels = map[string]string{ - hcops.LabelServiceUID: tt.serviceUID, - "some-label": "some-value", + lbspec.LabelServiceUID: tt.serviceUID, + "some-label": "some-value", } opts := hcloud.LoadBalancerUpdateOpts{ Labels: map[string]string{ - hcops.LabelServiceUID: tt.serviceUID, - "some-label": "some-value", + lbspec.LabelServiceUID: tt.serviceUID, + "some-label": "some-value", }, } tt.fx.LBClient. @@ -1279,7 +1280,7 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { changed, err := tt.fx.LBOps.ReconcileHCLB(tt.fx.Ctx, tt.initialLB, tt.service) assert.NoError(t, err) assert.True(t, changed) - assert.Equal(t, tt.serviceUID, tt.initialLB.Labels[hcops.LabelServiceUID]) + assert.Equal(t, tt.serviceUID, tt.initialLB.Labels[lbspec.LabelServiceUID]) assert.Equal(t, "some-value", tt.initialLB.Labels["some-label"]) }, }, @@ -1293,7 +1294,7 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { ID: 11, Name: "old-name", Labels: map[string]string{ - hcops.LabelServiceUID: "11", + lbspec.LabelServiceUID: "11", }, PublicNet: hcloud.LoadBalancerPublicNet{ Enabled: true, @@ -1920,7 +1921,7 @@ func TestLoadBalancerOps_ReconcileHCLBServices(t *testing.T) { Name: "ccm-managed-certificate-some service uid", Type: hcloud.CertificateTypeManaged, DomainNames: []string{"example.com", "*.example.com"}, - Labels: map[string]string{hcops.LabelServiceUID: tt.serviceUID}, + Labels: map[string]string{lbspec.LabelServiceUID: tt.serviceUID}, }). Return(hcloud.CertificateCreateResult{Certificate: cert}, nil, nil) @@ -1930,7 +1931,7 @@ func TestLoadBalancerOps_ReconcileHCLBServices(t *testing.T) { mock.Anything, hcloud.CertificateListOpts{ ListOpts: hcloud.ListOpts{ - LabelSelector: fmt.Sprintf("%s=%s", hcops.LabelServiceUID, tt.serviceUID), + LabelSelector: fmt.Sprintf("%s=%s", lbspec.LabelServiceUID, tt.serviceUID), }, }). Return([]*hcloud.Certificate{cert}, nil, nil) diff --git a/internal/lbspec/solver.go b/internal/lbspec/solver.go index 67ab35edc..c22d9c7f7 100644 --- a/internal/lbspec/solver.go +++ b/internal/lbspec/solver.go @@ -173,13 +173,12 @@ func resolveService( httpConfigured = true } - switch { - case hasManagedCertificate: + if hasManagedCertificate { // The managed certificate replaces the uploaded certificate list, so // that annotation is not read at all. The caller fills in the // certificate it looked up by label. httpConfigured = true - default: + } else { certs := resolve(errs, svc, annotation.LBSvcHTTPCertificates, nil) if len(certs) > 0 { http.Certificates = certs diff --git a/tests/e2e/cloud_test.go b/tests/e2e/cloud_test.go index 0d12c92e1..b56f99dc4 100644 --- a/tests/e2e/cloud_test.go +++ b/tests/e2e/cloud_test.go @@ -18,7 +18,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" - "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/hcops" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/legacydatacenter" "github.com/hetznercloud/hcloud-go/v2/hcloud" ) @@ -140,7 +140,7 @@ func TestServiceLoadBalancersHTTPSWithManagedCertificate(t *testing.T) { certs, err := testCluster.hcloud.Certificate.AllWithOpts(t.Context(), hcloud.CertificateListOpts{ ListOpts: hcloud.ListOpts{ - LabelSelector: fmt.Sprintf("%s=%s", hcops.LabelServiceUID, lbSvc.ObjectMeta.UID), + LabelSelector: fmt.Sprintf("%s=%s", lbspec.LabelServiceUID, lbSvc.ObjectMeta.UID), }, }) assert.NoError(t, err) diff --git a/tests/e2e/helper_test.go b/tests/e2e/helper_test.go index f4f1e7c54..fca6dd8a0 100644 --- a/tests/e2e/helper_test.go +++ b/tests/e2e/helper_test.go @@ -25,7 +25,7 @@ import ( "k8s.io/client-go/tools/clientcmd" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation" - "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/hcops" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/lbspec" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/testsupport" "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/utils" "github.com/hetznercloud/hcloud-go/v2/hcloud" @@ -140,7 +140,7 @@ func (tc *TestCluster) Stop() error { ctx := context.Background() uids := tc.loadBalancers.All() - selector := fmt.Sprintf("%s in (%s)", hcops.LabelServiceUID, strings.Join(uids, ",")) + selector := fmt.Sprintf("%s in (%s)", lbspec.LabelServiceUID, strings.Join(uids, ",")) lbs, err := tc.hcloud.LoadBalancer.AllWithOpts(ctx, hcloud.LoadBalancerListOpts{ ListOpts: hcloud.ListOpts{ LabelSelector: selector,