From a50d57a4d6651584dc0fe1a8064adb8f1b3c8df0 Mon Sep 17 00:00:00 2001 From: lukasmetzner Date: Wed, 19 Aug 2026 12:07:44 +0200 Subject: [PATCH 1/2] refactor: use a spec model for annotation extraction Introduce internal/lbspec, which resolves a Service and the cluster-wide configuration into a Spec describing the desired state, and which builds the API opts from it. - An invalid annotation now fails the whole reconcilement instead of only the step that happened to read it. A typo in an HTTP annotation therefore also stops node targets from being updated. - The remaining 5 hclbServiceOptsBuilder op label values disappear from hcloud_ccm_operations_total, and resolveCertificates is added. - Certificates are resolved once per Service rather than once per port, and on the reconcile context instead of a detached 5s timeout. - An empty name annotation no longer tries to rename the Load Balancer to the empty string, and an empty hostname annotation no longer publishes an empty hostname as the ingress address. - private-ipv4 is parsed, so the private network address is compared as an address rather than as the literal annotation text. --- hcloud/load_balancers.go | 218 ++--- hcloud/load_balancers_test.go | 52 +- internal/annotation/load_balancer.go | 2 +- internal/hcops/certificates_test.go | 38 +- internal/hcops/load_balancer.go | 860 +++--------------- internal/hcops/load_balancer_internal_test.go | 39 +- internal/hcops/load_balancer_test.go | 104 ++- internal/hcops/mocks.go | 4 +- internal/lbspec/opts.go | 206 +++++ internal/lbspec/opts_test.go | 220 +++++ internal/lbspec/solver.go | 283 ++++++ internal/lbspec/solver_test.go | 416 +++++++++ internal/lbspec/spec.go | 154 ++++ tests/e2e/cloud_test.go | 4 +- tests/e2e/helper_test.go | 4 +- 15 files changed, 1658 insertions(+), 946 deletions(-) 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/hcloud/load_balancers.go b/hcloud/load_balancers.go index 18d5c9e27..97273e1e9 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,33 +43,18 @@ func newLoadBalancers(lbOps LoadBalancerOps, lbCfg *config.LoadBalancerConfigura } } -func matchNodeSelector(svc *corev1.Service, nodes []*corev1.Node) ([]*corev1.Node, error) { - var selectedNodes []*corev1.Node - - selector := labels.Everything() - if v, err := annotation.LBNodeSelector.FromService(svc); err == nil { - parsed, err := labels.Parse(v) - if err != nil { - return nil, fmt.Errorf("unable to parse the node-selector annotation: %w", err) - } - selector = parsed - } - - 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() + // 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) { @@ -77,56 +63,41 @@ func (l *loadBalancers) GetLoadBalancer( return nil, false, fmt.Errorf("%s: %w", op, err) } - if v, err := annotation.LBHostname.FromService(service); err == nil { - return &corev1.LoadBalancerStatus{ - Ingress: []corev1.LoadBalancerIngress{{Hostname: v}}, - }, true, nil - } - - ingress, err := l.buildLoadBalancerStatusIngress(lb, service) + spec, err := lbspec.Resolve(service, *l.cfg) if err != 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, err := annotation.LBName.FromService(service); err == nil { - 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: // @@ -135,20 +106,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) @@ -192,49 +166,34 @@ func (l *loadBalancers) EnsureLoadBalancer( } } - // Either set the Hostname or the IPs (below). - // See: https://github.com/kubernetes/kubernetes/issues/66607 - if v, err := annotation.LBHostname.FromService(svc); err == nil { - 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, @@ -242,12 +201,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(), @@ -256,62 +210,30 @@ func (l *loadBalancers) buildLoadBalancerStatusIngress(lb *hcloud.LoadBalancer, } } - return ingress, nil -} - -func (l *loadBalancers) getPrivateIngressEnabled(svc *corev1.Service) (bool, error) { - disable, err := annotation.LBDisablePrivateIngress.FromService(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.FromService(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.FromService(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 @@ -320,15 +242,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) } @@ -372,3 +292,13 @@ func (l *loadBalancers) EnsureLoadBalancerDeleted(ctx context.Context, _ string, return nil } + +func filterNodes(selector labels.Selector, nodes []*corev1.Node) []*corev1.Node { + 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 99a37834c..3698491dd 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" @@ -11,7 +12,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" ) @@ -57,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", @@ -231,7 +252,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). @@ -484,7 +505,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). @@ -931,6 +952,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{ @@ -952,14 +988,24 @@ 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) } + // 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) + } }) } } diff --git a/internal/annotation/load_balancer.go b/internal/annotation/load_balancer.go index 9fbe08cc0..03dba0467 100644 --- a/internal/annotation/load_balancer.go +++ b/internal/annotation/load_balancer.go @@ -56,7 +56,7 @@ const ( // LBPrivateIPv4 specifies the IPv4 address to assign to the load balancer in the // private network that it's attached to. - LBPrivateIPv4 String = "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 diff --git a/internal/hcops/certificates_test.go b/internal/hcops/certificates_test.go index b5747a27a..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, hcloud.CertificateCreateOpts{ - Name: "test-cert", - Type: hcloud.CertificateTypeManaged, - DomainNames: []string{"example.com", "*.example.com"}, - Labels: 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, hcloud.CertificateCreateOpts{ - Name: "test-cert", - Type: hcloud.CertificateTypeManaged, - DomainNames: []string{"example.com", "*.example.com"}, - Labels: 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, hcloud.CertificateCreateOpts{ - Name: "test-cert", - Type: hcloud.CertificateTypeManaged, - DomainNames: []string{"example.com", "*.example.com"}, - Labels: 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 27ed43515..28b13650f 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" @@ -24,14 +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" - - defaultLoadBalancerType = "lb11" - loadBalancerSubsystem = "load_balancer" -) +const loadBalancerSubsystem = "load_balancer" // LoadBalancerOps implements all operations regarding Hetzner Cloud Load Balancers. type LoadBalancerOps struct { @@ -59,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) @@ -118,128 +110,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, err := annotation.LBType.FromService(svc); err == nil { - 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, err := annotation.LBLocation.FromService(svc); err == nil { - 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, err := annotation.LBNetworkZone.FromService(svc); err == nil { - 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.FromService(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.FromService(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 +142,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 +169,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,31 +231,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. The service UID is set after copying, - // so that it replaces a stale value instead of being overwritten by it. - labels := make(map[string]string, len(lb.Labels)+1) - maps.Copy(labels, lb.Labels) - labels[LabelServiceUID] = string(svc.ObjectMeta.UID) - opts.Labels = labels - update = true - } - - if lbName, err := annotation.LBName.FromService(svc); err == nil && lbName != lb.Name { - opts.Name = lbName - update = true - } - + opts, update := spec.UpdateOpts(lb) if !update { return false, nil } @@ -373,18 +252,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, err := annotation.LBPublicIPv4RDNS.FromService(svc) // If the annotation is not set, no changes are needed - if errors.Is(err, annotation.ErrNotSet) { + if spec.IPv4RDNS == nil { return false, nil } - if err != nil { - return false, fmt.Errorf("%s: %w", op, err) - } + rdns := *spec.IPv4RDNS // If the annotation and the actual value match, no changes are needed if rdns == lb.PublicNet.IPv4.DNSPtr { return false, nil @@ -401,18 +277,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, err := annotation.LBPublicIPv6RDNS.FromService(svc) // If the annotation is not set, no changes are needed - if errors.Is(err, annotation.ErrNotSet) { + if spec.IPv6RDNS == nil { return false, nil } - if err != nil { - return false, fmt.Errorf("%s: %w", op, err) - } + rdns := *spec.IPv6RDNS // If the annotation and the actual value match, no changes are needed if rdns == lb.PublicNet.IPv6.DNSPtr { return false, nil @@ -429,26 +302,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.FromService(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)) @@ -460,20 +323,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 } @@ -494,19 +359,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, err := annotation.LBPrivateIPv4.FromService(svc) - privateIPv4configured := err == nil 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()) @@ -524,42 +387,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() - privateIPv4String, err := annotation.LBPrivateIPv4.FromService(svc) - privateIPv4configured := err == nil - subnetString, err := annotation.PrivateSubnetIPRange.FromService(svc) - subnetConfigured := err == nil - 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) } @@ -576,13 +415,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", @@ -602,35 +435,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.FromService(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) @@ -674,10 +494,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) } @@ -882,10 +704,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) { @@ -956,17 +775,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.FromService(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( @@ -977,7 +785,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) } @@ -990,13 +810,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( @@ -1014,31 +828,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)) } @@ -1067,38 +869,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() - // Compared as a raw string: only the exact value selects managed - // certificates, and an unset annotation reads as the empty string. - if typ, _ := annotation.LBSvcHTTPCertificateType.FromService(svc); typ != string(hcloud.CertificateTypeManaged) { + if spec.ManagedCertificate == nil { return nil } - name, _ := annotation.LBSvcHTTPManagedCertificateName.FromService(svc) - if name == "" { - name = fmt.Sprintf("ccm-managed-certificate-%s", svc.ObjectMeta.UID) - } - domains, err := annotation.LBSvcHTTPManagedCertificateDomains.FromService(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.FromService(svc); ok { - labels["HC-Use-Staging-CA"] = "true" - } - err = l.CertOps.CreateManagedCertificate(ctx, hcloud.CertificateCreateOpts{ - Name: name, - Type: hcloud.CertificateTypeManaged, - DomainNames: domains, - Labels: labels, - }) + + err := l.CertOps.CreateManagedCertificate(ctx, spec.ManagedCertificate.CreateOpts()) if errors.Is(err, ErrAlreadyExists) { return nil } @@ -1108,454 +889,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.FromService(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.FromService(b.Service) - if errors.Is(err, annotation.ErrNotSet) { - return nil - } + if spec.ManagedCertificate != nil { + cert, err := l.CertOps.GetCertificateByLabel(ctx, fmt.Sprintf("%s=%s", lbspec.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, err := annotation.LBSvcHTTPCookieName.FromService(b.Service); err == nil { - b.httpOpts.CookieName = &v - b.addHTTP = true + return []*hcloud.Certificate{{ID: cert.ID}}, nil } - b.do(func() error { - lt, err := annotation.LBSvcHTTPCookieLifetime.FromService(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.FromService(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, _ := annotation.LBSvcHTTPCertificateType.FromService(b.Service) - if certtyp == string(hcloud.CertificateTypeManaged) { - // Continue with managed certificates below - return nil - } - - certs, err := annotation.LBSvcHTTPCertificates.FromService(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, _ := annotation.LBSvcHTTPCertificateType.FromService(b.Service) - if 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.FromService(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.FromService(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.FromService(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.FromService(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.FromService(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.FromService(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.FromService(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, err := annotation.LBSvcHealthCheckHTTPDomain.FromService(b.Service); err == nil { - b.healthCheckOpts.httpOpts.Domain = &v - } - - if v, err := annotation.LBSvcHealthCheckHTTPPath.FromService(b.Service); err == nil { - b.healthCheckOpts.httpOpts.Path = &v - } - - b.do(func() error { - tls, err := annotation.LBSvcHealthCheckHTTPValidateCertificate.FromService(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.FromService(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 f8a25593b..0e69f47a7 100644 --- a/internal/hcops/load_balancer_internal_test.go +++ b/internal/hcops/load_balancer_internal_test.go @@ -1,6 +1,7 @@ package hcops import ( + "context" "fmt" "maps" "testing" @@ -8,17 +9,22 @@ import ( "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 @@ -241,7 +247,7 @@ func TestHCLBServiceOptsBuilder(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) @@ -441,24 +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{}, }, + } + 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}, - cfg: tt.cfg, } - maps.Copy(builder.Service.Annotations, tt.serviceAnnotations) - 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 ac38c68a8..e96231354 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. @@ -259,6 +260,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { }, }, serviceAnnotations: map[string]string{ + string(annotation.LBName): "some-lb", string(annotation.LBLocation): "fsn1", }, createOpts: hcloud.LoadBalancerCreateOpts{ @@ -268,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}, @@ -281,6 +283,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { }, }, serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", string(annotation.LBNetworkZone): "eu-central", }, createOpts: hcloud.LoadBalancerCreateOpts{ @@ -288,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}, @@ -300,6 +303,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"}, @@ -307,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}, @@ -319,12 +325,15 @@ 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"}, NetworkZone: hcloud.NetworkZoneEUCentral, Labels: map[string]string{ - hcops.LabelServiceUID: "some-lb-uid", + lbspec.LabelServiceUID: "some-lb-uid", }, }, lb: &hcloud.LoadBalancer{ID: 4}, @@ -337,6 +346,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { }, }, serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", string(annotation.LBLocation): "", string(annotation.LBNetworkZone): "eu-central", }, @@ -345,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}, @@ -358,6 +368,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { }, }, serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", string(annotation.LBLocation): "fsn1", string(annotation.LBNetworkZone): "", }, @@ -368,11 +379,28 @@ 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}, }, + { + 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{ + lbspec.LabelServiceUID: "some-lb-uid", + }, + }, + lb: &hcloud.LoadBalancer{ID: 1}, + }, { name: "fails if location and network zone missing", serviceAnnotations: map[string]string{}, @@ -382,6 +410,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { { name: "gives preference to location name", serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", string(annotation.LBLocation): "nbg1", string(annotation.LBNetworkZone): "eu-central", }, @@ -390,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}, @@ -398,6 +427,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { { name: "set Load Balancer type name", serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", string(annotation.LBType): "lb21", string(annotation.LBLocation): "nbg1", }, @@ -406,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}, @@ -414,6 +444,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { { name: "set Load Balancer algorithm type", serviceAnnotations: map[string]string{ + string(annotation.LBName): "another-lb", string(annotation.LBLocation): "nbg1", string(annotation.LBAlgorithmType): "least_connections", }, @@ -423,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}, @@ -436,12 +467,15 @@ 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"}, 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}, @@ -454,13 +488,16 @@ 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"}, 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}, @@ -471,11 +508,13 @@ func TestLoadBalancerOps_Create(t *testing.T) { string(annotation.LBLocation): "nbg1", string(annotation.LBAlgorithmType): "invalidType", }, - err: fmt.Errorf("hcops/LoadBalancerOps.Create: load-balancer.hetzner.cloud/algorithm-type: 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[string]string{ + string(annotation.LBName): "lb-with-priv", string(annotation.LBLocation): "nbg1", string(annotation.LBDisablePublicNetwork): "true", }, @@ -485,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) { @@ -517,13 +556,13 @@ 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{}, }, } 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 { @@ -661,7 +700,8 @@ 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: load-balancer.hetzner.cloud/algorithm-type: invalid: invalidType") + "hcops/LoadBalancerOps.ReconcileHCLB: invalid Load Balancer annotations: "+ + "load-balancer.hetzner.cloud/algorithm-type: invalid: invalidType") assert.False(t, changed) }, }, @@ -1223,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. @@ -1240,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"]) }, }, @@ -1250,8 +1290,8 @@ func TestLoadBalancerOps_ReconcileHCLB(t *testing.T) { initialLB: &hcloud.LoadBalancer{ ID: 12, Labels: map[string]string{ - hcops.LabelServiceUID: "stale-uid", - "some-label": "some-value", + lbspec.LabelServiceUID: "stale-uid", + "some-label": "some-value", }, PublicNet: hcloud.LoadBalancerPublicNet{ Enabled: true, @@ -1260,13 +1300,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. @@ -1277,7 +1317,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"]) // The stale label must actually be replaced, otherwise every @@ -1298,7 +1338,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, @@ -1925,7 +1965,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) @@ -1935,7 +1975,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/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..c22d9c7f7 --- /dev/null +++ b/internal/lbspec/solver.go @@ -0,0 +1,283 @@ +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 + } + + 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 + } else { + 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/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, From 48e2cf6d51dce147267bddcf13731adfc59d5e92 Mon Sep 17 00:00:00 2001 From: lukasmetzner Date: Wed, 19 Aug 2026 15:10:45 +0200 Subject: [PATCH 2/2] feat: log annotations separately --- internal/hcops/load_balancer_test.go | 4 +-- internal/lbspec/solver.go | 20 +++++++++++--- internal/lbspec/solver_test.go | 21 ++++++++++----- internal/testsupport/klog.go | 40 ++++++++++++++++++++++++++++ internal/utils/eventlog_test.go | 22 +++------------ 5 files changed, 77 insertions(+), 30 deletions(-) create mode 100644 internal/testsupport/klog.go diff --git a/internal/hcops/load_balancer_test.go b/internal/hcops/load_balancer_test.go index e96231354..1d9b515bb 100644 --- a/internal/hcops/load_balancer_test.go +++ b/internal/hcops/load_balancer_test.go @@ -508,7 +508,7 @@ func TestLoadBalancerOps_Create(t *testing.T) { string(annotation.LBLocation): "nbg1", string(annotation.LBAlgorithmType): "invalidType", }, - err: fmt.Errorf("hcops/LoadBalancerOps.Create: invalid Load Balancer annotations: " + + err: fmt.Errorf("hcops/LoadBalancerOps.Create: invalid Load Balancer annotation: " + "load-balancer.hetzner.cloud/algorithm-type: invalid: invalidType"), }, { @@ -700,7 +700,7 @@ 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: invalid Load Balancer annotations: "+ + "hcops/LoadBalancerOps.ReconcileHCLB: invalid Load Balancer annotation: "+ "load-balancer.hetzner.cloud/algorithm-type: invalid: invalidType") assert.False(t, changed) }, diff --git a/internal/lbspec/solver.go b/internal/lbspec/solver.go index c22d9c7f7..b36c0e1ea 100644 --- a/internal/lbspec/solver.go +++ b/internal/lbspec/solver.go @@ -8,6 +8,7 @@ import ( 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" @@ -70,10 +71,23 @@ func Resolve(svc *corev1.Service, cfg config.LoadBalancerConfiguration) (Spec, e 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...)) + if len(errs) == 0 { + return spec, nil } - return spec, nil + + if len(errs) == 1 { + return spec, fmt.Errorf("invalid Load Balancer annotation: %w", errs[0]) + } + + // Joining errors creates an error message which is hard to read in the + // logs and the Kubernetes events. Log each annotation error separately and + // provide details to check the pod logs, if the user only inspects the + // Kubernetes event. + for _, err := range errs { + klog.ErrorS(err, "invalid Load Balancer annotation", "service", klog.KObj(svc)) + } + + return spec, fmt.Errorf("%d Load Balancer annotation(s) are invalid, see the hcloud-cloud-controller-manager logs for details", len(errs)) } func resolveNodeSelector(errs *[]error, svc *corev1.Service) labels.Selector { diff --git a/internal/lbspec/solver_test.go b/internal/lbspec/solver_test.go index 175533ab0..6543a0ad5 100644 --- a/internal/lbspec/solver_test.go +++ b/internal/lbspec/solver_test.go @@ -15,6 +15,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/lbspec" + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/testsupport" "github.com/hetznercloud/hcloud-go/v2/hcloud" ) @@ -347,15 +348,21 @@ func TestResolveReportsEveryInvalidAnnotation(t *testing.T) { string(annotation.PrivateSubnetIPRange): "not-a-cidr", }) + logs := testsupport.CaptureKlog(t) + _, err := lbspec.Resolve(svc, config.LoadBalancerConfiguration{}) - require.Error(t, err) + // The error only counts the invalid annotations, so that the Kubernetes + // event stays readable. + require.EqualError(t, err, + "5 Load Balancer annotation(s) are invalid, see the hcloud-cloud-controller-manager logs for details") + // 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") + assert.Contains(t, logs.String(), "sideways") + assert.Contains(t, logs.String(), "maybe") + assert.Contains(t, logs.String(), "many") + assert.Contains(t, logs.String(), "not-an-ip") + assert.Contains(t, logs.String(), "not-a-cidr") } func TestResolveErrors(t *testing.T) { @@ -398,7 +405,7 @@ func TestResolveErrors(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, "invalid Load Balancer annotation:") assert.ErrorContains(t, err, tt.wantErr) }) } diff --git a/internal/testsupport/klog.go b/internal/testsupport/klog.go new file mode 100644 index 000000000..5b7d50928 --- /dev/null +++ b/internal/testsupport/klog.go @@ -0,0 +1,40 @@ +package testsupport + +import ( + "bytes" + "testing" + + "k8s.io/klog/v2" +) + +// KlogCapture holds the log output collected by [CaptureKlog]. +type KlogCapture struct { + buf *bytes.Buffer +} + +// String returns everything klog has logged so far. +func (c *KlogCapture) String() string { + // klog buffers its output, flush it so that the caller sees the log + // records that were written up to this point. + klog.Flush() + return c.buf.String() +} + +// CaptureKlog redirects the klog output into a buffer for the duration of the +// test, so that tests can assert on what was logged. The previous klog +// configuration is restored once the test finishes. +// +// klog is configured globally, tests using this helper must not run in +// parallel. +func CaptureKlog(t *testing.T) *KlogCapture { + t.Helper() + + state := klog.CaptureState() + t.Cleanup(state.Restore) + + var buf bytes.Buffer + klog.LogToStderr(false) + klog.SetOutput(&buf) + + return &KlogCapture{buf: &buf} +} diff --git a/internal/utils/eventlog_test.go b/internal/utils/eventlog_test.go index 262bdf9d7..099450f42 100644 --- a/internal/utils/eventlog_test.go +++ b/internal/utils/eventlog_test.go @@ -1,28 +1,15 @@ package utils import ( - "bytes" "testing" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" - "k8s.io/klog/v2" -) - -func captureKlog(t *testing.T) *bytes.Buffer { - t.Helper() - - state := klog.CaptureState() - t.Cleanup(state.Restore) - var buf bytes.Buffer - klog.LogToStderr(false) - klog.SetOutput(&buf) - - return &buf -} + "github.com/hetznercloud/hcloud-cloud-controller-manager/internal/testsupport" +) func TestWarnEventLogf(t *testing.T) { tests := []struct { @@ -56,14 +43,13 @@ func TestWarnEventLogf(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - logs := captureKlog(t) + logs := testsupport.CaptureKlog(t) recorder := record.NewFakeRecorder(1) WarnEventLogf(recorder, &corev1.Node{}, tt.reason, tt.msg, tt.args...) assert.Equal(t, "Warning "+tt.reason+" "+tt.expected, <-recorder.Events) - klog.Flush() // klog prefix `W` for warning assert.Regexp(t, `^W\d`, logs.String()) assert.Contains(t, logs.String(), tt.expected) @@ -72,7 +58,7 @@ func TestWarnEventLogf(t *testing.T) { } func TestWarnEventLogfEventObject(t *testing.T) { - captureKlog(t) + testsupport.CaptureKlog(t) recorder := record.NewFakeRecorder(1) recorder.IncludeObject = true