Repository: hetznercloud/hcloud-cloud-controller-manager
Pull request #768 (2024-10-24 12:09:40)

test: improve load-balancer teardown logic
- Remove panic calls to be able to gather more information when a test fails,
- Cleanup load-balancers when the test cluster is stopped (this ensures that terraform destroy does not hang while trying to destroy a network that still has load-balancer attach to it).
- Improve load-balancer test helpers.

Co-authored by Lukas

https://github.com/hetznercloud/hcloud-cloud-controller-manager/actions/runs/11495092304/job/31993877300?pr=755#step:11:44
https://github.com/hetznercloud/hcloud-cloud-controller-manager/actions/runs/11496754257/job/31998988278#step:11:47

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/768?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud) Report
Attention: Patch coverage is `79.16667%` with `5 lines` in your changes missing coverage. Please review.
> Project coverage is 70.67%. Comparing base [(`c571b0c`)](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/commit/c571b0ca88aa4b709e69442503c8586576062dd0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud) to head [(`b869229`)](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/commit/b8692294f1edb5c29651fdbec47ab27ffaa1d8e7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud).
> Report is 1 commits behind head on main.

| [Files with missing lines](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/768?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud) | Patch % | Lines |
|---|---|---|
| [internal/utils/set.go](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/768?src=pr&el=tree&filepath=internal%2Futils%2Fset.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud#diff-aW50ZXJuYWwvdXRpbHMvc2V0Lmdv) | 79.16% | [5 Missing :warning: ](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/768?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #768      +/-   ##
==========================================
- Coverage   70.77%   70.67%   -0.10%     
==========================================
  Files          31       31              
  Lines        3384     3182     -202     
==========================================
- Hits         2395     2249     -146     
+ Misses        800      760      -40     
+ Partials      189      173      -16     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/768?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud).


Patch:
diff --git a/internal/utils/set.go b/internal/utils/set.go
new file mode 100644
index 00000000..429660e6
--- /dev/null
+++ b/internal/utils/set.go
@@ -0,0 +1,43 @@
+package utils
+
+import (
+	"sync"
+)
+
+type SyncSet[T comparable] struct {
+	lock sync.Mutex
+	data map[T]struct{}
+}
+
+func NewSyncSet[T comparable]() *SyncSet[T] {
+	return &SyncSet[T]{
+		lock: sync.Mutex{},
+		data: make(map[T]struct{}, 0),
+	}
+}
+
+func (s *SyncSet[T]) Add(v T) {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
+	s.data[v] = struct{}{}
+}
+
+func (s *SyncSet[T]) Size() int {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
+	return len(s.data)
+}
+
+func (s *SyncSet[T]) All() []T {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
+	result := make([]T, 0, len(s.data))
+	for value := range s.data {
+		result = append(result, value)
+	}
+
+	return result
+}


Test patch:
diff --git a/internal/utils/set_test.go b/internal/utils/set_test.go
new file mode 100644
index 00000000..1d29f6e2
--- /dev/null
+++ b/internal/utils/set_test.go
@@ -0,0 +1,20 @@
+package utils
+
+import (
+	"sort"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSyncSet(t *testing.T) {
+	o := NewSyncSet[int]()
+
+	o.Add(1)
+	o.Add(2)
+	o.Add(3)
+
+	all := o.All()
+	sort.Ints(all)
+	assert.Equal(t, []int{1, 2, 3}, all)
+}
diff --git a/tests/e2e/cloud_test.go b/tests/e2e/cloud_test.go
index c805b9a2..d62c22f1 100644
--- a/tests/e2e/cloud_test.go
+++ b/tests/e2e/cloud_test.go
@@ -71,9 +71,8 @@ func TestServiceLoadBalancersMinimalSetup(t *testing.T) {
 	t.Parallel()
 
 	lbTest := lbTestHelper{
-		t:         t,
-		K8sClient: testCluster.k8sClient,
-		podName:   "loadbalancer-minimal",
+		t:       t,
+		podName: "loadbalancer-minimal",
 	}
 
 	pod := lbTest.DeployTestPod()
@@ -97,10 +96,9 @@ func TestServiceLoadBalancersHTTPS(t *testing.T) {
 
 	cert := testCluster.CreateTLSCertificate(t, "loadbalancer-https")
 	lbTest := lbTestHelper{
-		t:         t,
-		K8sClient: testCluster.k8sClient,
-		podName:   "loadbalancer-https",
-		port:      443,
+		t:       t,
+		podName: "loadbalancer-https",
+		port:    443,
 	}
 
 	pod := lbTest.DeployTestPod()
@@ -132,10 +130,9 @@ func TestServiceLoadBalancersHTTPSWithManagedCertificate(t *testing.T) {
 
 	domainName := fmt.Sprintf("%d-ccm-test.%s", rand.Int(), testCluster.certDomain)
 	lbTest := lbTestHelper{
-		t:         t,
-		K8sClient: testCluster.k8sClient,
-		podName:   "loadbalancer-https",
-		port:      443,
+		t:       t,
+		podName: "loadbalancer-https",
+		port:    443,
 	}
 
 	pod := lbTest.DeployTestPod()
@@ -161,6 +158,7 @@ func TestServiceLoadBalancersHTTPSWithManagedCertificate(t *testing.T) {
 	assert.Len(t, certs, 1)
 
 	lbTest.TearDown()
+
 	_, err = testCluster.hcloud.Certificate.Delete(ctx, certs[0])
 	assert.NoError(t, err)
 }
@@ -168,7 +166,7 @@ func TestServiceLoadBalancersHTTPSWithManagedCertificate(t *testing.T) {
 func TestServiceLoadBalancersWithPrivateNetwork(t *testing.T) {
 	t.Parallel()
 
-	lbTest := lbTestHelper{t: t, K8sClient: testCluster.k8sClient, podName: "loadbalancer-private-network"}
+	lbTest := lbTestHelper{t: t, podName: "loadbalancer-private-network"}
 
 	pod := lbTest.DeployTestPod()
 
diff --git a/tests/e2e/testing.go b/tests/e2e/helper_test.go
similarity index 78%
rename from tests/e2e/testing.go
rename to tests/e2e/helper_test.go
index 4f58a3de..08b4171d 100644
--- a/tests/e2e/testing.go
+++ b/tests/e2e/helper_test.go
@@ -5,6 +5,7 @@ package e2e
 import (
 	"context"
 	"crypto/tls"
+	"errors"
 	"fmt"
 	"math/rand"
 	"net/http"
@@ -22,7 +23,9 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/clientcmd"
 
+	"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/annotation"
 	"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"
 )
 
@@ -33,13 +36,16 @@ func init() {
 }
 
 type TestCluster struct {
-	hcloud       *hcloud.Client
-	hrobot       hrobot.RobotClient
-	k8sClient    *kubernetes.Clientset
-	certificates []*hcloud.Certificate
+	hcloud    *hcloud.Client
+	hrobot    hrobot.RobotClient
+	k8sClient *kubernetes.Clientset
+
 	// envName is used as the name prefix in all resources
 	envName    string
 	certDomain string
+
+	certificates  *utils.SyncSet[int64]
+	loadBalancers *utils.SyncSet[int64]
 }
 
 func (tc *TestCluster) Start() error {
@@ -89,17 +95,31 @@ func (tc *TestCluster) Start() error {
 	// The domain specified here must be available in Hetzner DNS of the account running the tests.
 	tc.certDomain = os.Getenv("CERT_DOMAIN")
 
+	tc.certificates = utils.NewSyncSet[int64]()
+	tc.loadBalancers = utils.NewSyncSet[int64]()
+
 	return nil
 }
 
 func (tc *TestCluster) Stop() error {
-	for _, c := range tc.certificates {
-		if _, err := tc.hcloud.Certificate.Delete(context.Background(), c); err != nil {
-			fmt.Printf("delete certificate %d failed: %v", c.ID, err)
+	errs := make([]error, 0, tc.loadBalancers.Size()+tc.certificates.Size())
+	ctx := context.Background()
+
+	for _, item := range tc.loadBalancers.All() {
+		fmt.Printf("deleting load balancer %d\n", item)
+		if _, err := tc.hcloud.LoadBalancer.Delete(ctx, &hcloud.LoadBalancer{ID: item}); err != nil {
+			errs = append(errs, fmt.Errorf("delete load balancer %d failed: %w", item, err))
 		}
 	}
 
-	return nil
+	for _, item := range tc.certificates.All() {
+		fmt.Printf("deleting certificate %d\n", item)
+		if _, err := tc.hcloud.Certificate.Delete(ctx, &hcloud.Certificate{ID: item}); err != nil {
+			errs = append(errs, fmt.Errorf("delete certificate %d failed: %w", item, err))
+		}
+	}
+
+	return errors.Join(errs...)
 }
 
 // CreateTLSCertificate creates a TLS certificate used for testing and posts it
@@ -125,7 +145,7 @@ func (tc *TestCluster) CreateTLSCertificate(t *testing.T, baseName string) *hclo
 		t.Fatalf("no certificate created")
 	}
 
-	tc.certificates = append(tc.certificates, cert)
+	tc.certificates.Add(cert.ID)
 
 	return cert
 }
@@ -146,28 +166,30 @@ func (tc *TestCluster) WorkerNodeName(index int) string {
 }
 
 type lbTestHelper struct {
+	t *testing.T
+
+	namespace string
 	podName   string
 	port      int
-	K8sClient *kubernetes.Clientset
-	t         *testing.T
-	namespace string
 }
 
 // DeployTestPod deploys a basic nginx pod within the k8s cluster
 // and waits until it is "ready".
 func (l *lbTestHelper) DeployTestPod() *corev1.Pod {
+	l.t.Helper()
+
 	ctx := context.Background()
 
 	if l.namespace == "" {
 		l.namespace = "hccm-test-" + strconv.Itoa(rand.Int())
 	}
-	_, err := l.K8sClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{
+	_, err := testCluster.k8sClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: l.namespace,
 		},
 	}, metav1.CreateOptions{})
 	if err != nil && !k8serrors.IsAlreadyExists(err) {
-		panic(err)
+		l.t.Fatal(err)
 	}
 
 	podName := fmt.Sprintf("pod-%s", l.podName)
@@ -194,12 +216,12 @@ func (l *lbTestHelper) DeployTestPod() *corev1.Pod {
 		},
 	}
 
-	pod, err := l.K8sClient.CoreV1().Pods(l.namespace).Create(ctx, &testPod, metav1.CreateOptions{})
+	pod, err := testCluster.k8sClient.CoreV1().Pods(l.namespace).Create(ctx, &testPod, metav1.CreateOptions{})
 	if err != nil {
 		l.t.Fatalf("could not create test pod: %s", err)
 	}
 	err = wait.PollUntilContextTimeout(ctx, 1*time.Second, 1*time.Minute, false, func(ctx context.Context) (done bool, err error) {
-		p, err := l.K8sClient.CoreV1().Pods(l.namespace).Get(ctx, podName, metav1.GetOptions{})
+		p, err := testCluster.k8sClient.CoreV1().Pods(l.namespace).Get(ctx, podName, metav1.GetOptions{})
 		if err != nil {
 			return false, err
 		}
@@ -219,6 +241,8 @@ func (l *lbTestHelper) DeployTestPod() *corev1.Pod {
 
 // ServiceDefinition returns a service definition for a Hetzner Cloud Load Balancer (k8s service).
 func (l *lbTestHelper) ServiceDefinition(pod *corev1.Pod, annotations map[string]string) *corev1.Service {
+	l.t.Helper()
+
 	port := l.port
 	if port == 0 {
 		port = 80
@@ -249,6 +273,8 @@ func (l *lbTestHelper) ServiceDefinition(pod *corev1.Pod, annotations map[string
 // CreateService creates a k8s service based on the given service definition
 // and waits until it is "ready".
 func (l *lbTestHelper) CreateService(lbSvc *corev1.Service) (*corev1.Service, error) {
+	l.t.Helper()
+
 	ctx := context.Background()
 
 	// Default is 15s interval, 10s timeout, 3 retries => 45 seconds until up
@@ -258,16 +284,26 @@ func (l *lbTestHelper) CreateService(lbSvc *corev1.Service) (*corev1.Service, er
 	// lbSvc.Annotations[string(annotation.LBSvcHealthCheckRetries)] = "1"
 	// lbSvc.Annotations[string(annotation.LBSvcHealthCheckProtocol)] = "tcp"
 
-	_, err := l.K8sClient.CoreV1().Services(l.namespace).Create(ctx, lbSvc, metav1.CreateOptions{})
+	_, err := testCluster.k8sClient.CoreV1().Services(l.namespace).Create(ctx, lbSvc, metav1.CreateOptions{})
 	if err != nil {
 		return nil, fmt.Errorf("could not create service: %s", err)
 	}
 
 	err = wait.PollUntilContextTimeout(ctx, 1*time.Second, 5*time.Minute, false, func(ctx context.Context) (done bool, err error) {
-		svc, err := l.K8sClient.CoreV1().Services(l.namespace).Get(ctx, lbSvc.Name, metav1.GetOptions{})
+		svc, err := testCluster.k8sClient.CoreV1().Services(l.namespace).Get(ctx, lbSvc.Name, metav1.GetOptions{})
 		if err != nil {
 			return false, err
 		}
+
+		// Save load balancer id for cleanup
+		lbID, err := LoadBalancerIDFromService(lbSvc)
+		if err != nil {
+			l.t.Error(err)
+		}
+		if lbID != 0 {
+			testCluster.loadBalancers.Add(lbID)
+		}
+
 		ingressIPs := svc.Status.LoadBalancer.Ingress
 		if len(ingressIPs) > 0 {
 			lbSvc = svc
@@ -283,15 +319,17 @@ func (l *lbTestHelper) CreateService(lbSvc *corev1.Service) (*corev1.Service, er
 
 // TearDown deletes the created pod and service.
 func (l *lbTestHelper) TearDown() {
+	l.t.Helper()
+
 	err := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) {
-		err := l.K8sClient.CoreV1().Namespaces().Delete(ctx, l.namespace, metav1.DeleteOptions{})
+		err := testCluster.k8sClient.CoreV1().Namespaces().Delete(ctx, l.namespace, metav1.DeleteOptions{})
 		if err != nil && !k8serrors.IsNotFound(err) {
 			return false, err
 		}
 		return k8serrors.IsNotFound(err), nil
 	})
 	if err != nil {
-		panic(err)
+		l.t.Fatal(err)
 	}
 }
 
@@ -333,3 +371,17 @@ func WaitForHTTPAvailable(t *testing.T, ingressIP string, useHTTPS bool) {
 		t.Errorf("%s not available: %s", ingressIP, err)
 	}
 }
+
+func LoadBalancerIDFromService(svc *corev1.Service) (int64, error) {
+	label, ok := svc.Labels[string(annotation.LBID)]
+	if !ok || label == "" {
+		return 0, nil
+	}
+
+	id, err := strconv.ParseInt(label, 10, 64)
+	if err != nil {
+		return 0, err
+	}
+
+	return id, nil
+}
diff --git a/tests/e2e/robot_test.go b/tests/e2e/robot_test.go
index 58de1101..cb5a1578 100644
--- a/tests/e2e/robot_test.go
+++ b/tests/e2e/robot_test.go
@@ -71,9 +71,8 @@ func TestServiceLoadBalancersRobot(t *testing.T) {
 	t.Parallel()
 
 	lbTest := lbTestHelper{
-		t:         t,
-		K8sClient: testCluster.k8sClient,
-		podName:   "loadbalancer-robot-only",
+		t:       t,
+		podName: "loadbalancer-robot-only",
 	}
 
 	pod := lbTest.DeployTestPod()