Repository: hetznercloud/hcloud-cloud-controller-manager
Pull request #780 (2024-11-05 13:11:04)

feat(load-balancer): ignore nodes that don't use known provider IDs
With this feature the load balancer is more resilient to hybrid clusters as it skips nodes with unkown provider ids.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/780?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 `3.70370%` with `26 lines` in your changes missing coverage. Please review.
> Project coverage is 69.77%. Comparing base [(`006b0ad`)](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/commit/006b0ad28e4b949dd2b645bff40ecfd792f8a9b4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud) to head [(`3e98ad9`)](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/commit/3e98ad9018a359a6b08e4100ab18d36b674b4219?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/780?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/hcops/load\_balancer.go](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/780?src=pr&el=tree&filepath=internal%2Fhcops%2Fload_balancer.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud#diff-aW50ZXJuYWwvaGNvcHMvbG9hZF9iYWxhbmNlci5nbw==) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/780?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud) |
| [internal/providerid/providerid.go](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/780?src=pr&el=tree&filepath=internal%2Fproviderid%2Fproviderid.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud#diff-aW50ZXJuYWwvcHJvdmlkZXJpZC9wcm92aWRlcmlkLmdv) | 11.11% | [8 Missing :warning: ](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/780?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud) |
| [hcloud/instances.go](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/780?src=pr&el=tree&filepath=hcloud%2Finstances.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hetznercloud#diff-aGNsb3VkL2luc3RhbmNlcy5nbw==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/780?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     #780      +/-   ##
==========================================
- Coverage   70.29%   69.77%   -0.53%     
==========================================
  Files          31       31              
  Lines        3205     3229      +24     
==========================================
  Hits         2253     2253              
- Misses        779      803      +24     
  Partials      173      173              
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/hetznercloud/hcloud-cloud-controller-manager/pull/780?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/hcloud/instances.go b/hcloud/instances.go
index 355cc31b..660a2c44 100644
--- a/hcloud/instances.go
+++ b/hcloud/instances.go
@@ -113,7 +113,13 @@ func (i *instances) lookupServer(
 	}
 
 	if cloudServer != nil && hrobotServer != nil {
-		i.recorder.Eventf(node, corev1.EventTypeWarning, "InstanceLookupFailed", "Node %s could not be uniquely associated with a Cloud or Robot server, as a server with this name exists in both APIs", node.Name)
+		i.recorder.Eventf(
+			node,
+			corev1.EventTypeWarning,
+			"InstanceLookupFailed",
+			"Node %s could not be uniquely associated with a Cloud or Robot server, as a server with this name exists in both APIs",
+			node.Name,
+		)
 		return nil, fmt.Errorf("found both a cloud & robot server for name %q", node.Name)
 	}
 
diff --git a/internal/hcops/load_balancer.go b/internal/hcops/load_balancer.go
index 04b3f59d..938f907e 100644
--- a/internal/hcops/load_balancer.go
+++ b/internal/hcops/load_balancer.go
@@ -613,6 +613,18 @@ func (l *LoadBalancerOps) ReconcileHCLBTargets(
 	for _, node := range nodes {
 		id, isCloudServer, err := providerid.ToServerID(node.Spec.ProviderID)
 		if err != nil {
+			if errors.As(err, new(*providerid.UnkownPrefixError)) {
+				// ProviderID has unknown prefix, cluster might have non-hccm nodes that can not be added to the
+				// Load Balancer. Emitting an event and ignoring that Node in this reconciliation loop.
+				l.Recorder.Eventf(
+					node,
+					corev1.EventTypeWarning,
+					"UnknownProviderIDPrefix",
+					"Node could not be added to Load Balancer for service %s because the provider ID does not match any known format",
+					svc.Name,
+				)
+				continue
+			}
 			return changed, fmt.Errorf("%s: %w", op, err)
 		}
 		if isCloudServer {
diff --git a/internal/providerid/providerid.go b/internal/providerid/providerid.go
index 53892bff..35a7751b 100644
--- a/internal/providerid/providerid.go
+++ b/internal/providerid/providerid.go
@@ -24,6 +24,20 @@ const (
 	prefixRobotLegacy = "hcloud://bm-"
 )
 
+type UnkownPrefixError struct {
+	ProviderID string
+}
+
+func (e *UnkownPrefixError) Error() string {
+	return fmt.Sprintf(
+		"Provider ID does not have one of the the expected prefixes (%s, %s, %s): %s",
+		prefixCloud,
+		prefixRobot,
+		prefixRobotLegacy,
+		e.ProviderID,
+	)
+}
+
 // ToServerID parses the Cloud or Robot Server ID from a ProviderID.
 //
 // This method supports all formats for the ProviderID that were ever used.
@@ -44,7 +58,7 @@ func ToServerID(providerID string) (id int64, isCloudServer bool, err error) {
 		idString = strings.ReplaceAll(providerID, prefixCloud, "")
 
 	default:
-		return 0, false, fmt.Errorf("providerID does not have one of the the expected prefixes (%s, %s, %s): %s", prefixCloud, prefixRobot, prefixRobotLegacy, providerID)
+		return 0, false, &UnkownPrefixError{providerID}
 	}
 
 	if idString == "" {


Test patch:
diff --git a/internal/hcops/load_balancer_test.go b/internal/hcops/load_balancer_test.go
index 487d84a9..df79f58f 100644
--- a/internal/hcops/load_balancer_test.go
+++ b/internal/hcops/load_balancer_test.go
@@ -1286,6 +1286,33 @@ func TestLoadBalancerOps_ReconcileHCLBTargets(t *testing.T) {
 			},
 			cfg: config.HCCMConfiguration{LoadBalancer: config.LoadBalancerConfiguration{DisableIPv6: true}},
 		},
+		{
+			name: "provider id does not have one of the the expected prefixes",
+			k8sNodes: []*corev1.Node{
+				{Spec: corev1.NodeSpec{ProviderID: "hcloud://1"}},
+				{Spec: corev1.NodeSpec{ProviderID: "mycloud://2"}},
+			},
+			initialLB: &hcloud.LoadBalancer{
+				ID: 5,
+				Targets: []hcloud.LoadBalancerTarget{
+					{
+						Type:   hcloud.LoadBalancerTargetTypeServer,
+						Server: &hcloud.LoadBalancerTargetServer{Server: &hcloud.Server{ID: 1}},
+					},
+				},
+				LoadBalancerType: &hcloud.LoadBalancerType{
+					MaxTargets: 2,
+				},
+			},
+			mock: func(_ *testing.T, _ *LBReconcilementTestCase) {
+				// Nothing to mock because no action will be taken besides emitting an event
+			},
+			perform: func(t *testing.T, tt *LBReconcilementTestCase) {
+				changed, err := tt.fx.LBOps.ReconcileHCLBTargets(tt.fx.Ctx, tt.initialLB, tt.service, tt.k8sNodes)
+				assert.NoError(t, err)
+				assert.False(t, changed)
+			},
+		},
 		{
 			name: "enable use of private network via default",
 			cfg: config.HCCMConfiguration{
diff --git a/internal/providerid/providerid_test.go b/internal/providerid/providerid_test.go
index eed44f7a..5bee7cf2 100644
--- a/internal/providerid/providerid_test.go
+++ b/internal/providerid/providerid_test.go
@@ -1,8 +1,11 @@
 package providerid
 
 import (
+	"errors"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestFromCloudServerID(t *testing.T) {
@@ -59,92 +62,101 @@ func TestToServerID(t *testing.T) {
 		providerID        string
 		wantID            int64
 		wantIsCloudServer bool
-		wantErr           bool
+		wantErr           error
 	}{
 		{
 			name:              "[cloud] simple id",
 			providerID:        "hcloud://1234",
 			wantID:            1234,
 			wantIsCloudServer: true,
-			wantErr:           false,
+			wantErr:           nil,
 		},
 		{
 			name:              "[cloud] large id",
 			providerID:        "hcloud://2251799813685247",
 			wantID:            2251799813685247,
 			wantIsCloudServer: true,
-			wantErr:           false,
+			wantErr:           nil,
 		},
 		{
 			name:              "[cloud] invalid id",
 			providerID:        "hcloud://my-cloud",
 			wantID:            0,
 			wantIsCloudServer: false,
-			wantErr:           true,
+			wantErr:           errors.New("unable to parse server id: hcloud://my-cloud"),
 		},
 		{
 			name:              "[cloud] missing id",
 			providerID:        "hcloud://",
 			wantID:            0,
 			wantIsCloudServer: false,
-			wantErr:           true,
+			wantErr:           errors.New("providerID is missing a serverID: hcloud://"),
 		},
 		{
 			name:              "[robot] simple id",
 			providerID:        "hrobot://4321",
 			wantID:            4321,
 			wantIsCloudServer: false,
-			wantErr:           false,
+			wantErr:           nil,
 		},
 		{
 			name:              "[robot] invalid id",
 			providerID:        "hrobot://my-robot",
 			wantID:            0,
 			wantIsCloudServer: false,
-			wantErr:           true,
+			wantErr:           errors.New("unable to parse server id: hrobot://my-robot"),
 		},
 		{
 			name:              "[robot] missing id",
 			providerID:        "hrobot://",
 			wantID:            0,
 			wantIsCloudServer: false,
-			wantErr:           true,
+			wantErr:           errors.New("providerID is missing a serverID: hrobot://"),
 		},
 		{
 			name:              "[robot-syself] simple id",
 			providerID:        "hcloud://bm-4321",
 			wantID:            4321,
 			wantIsCloudServer: false,
-			wantErr:           false,
+			wantErr:           nil,
 		},
 		{
 			name:              "[robot-syself] invalid id",
 			providerID:        "hcloud://bm-my-robot",
 			wantID:            0,
 			wantIsCloudServer: false,
-			wantErr:           true,
+			wantErr:           errors.New("unable to parse server id: hcloud://bm-my-robot"),
 		},
 		{
 			name:              "[robot-syself] missing id",
 			providerID:        "hcloud://bm-",
 			wantID:            0,
 			wantIsCloudServer: false,
-			wantErr:           true,
+			wantErr:           errors.New("providerID is missing a serverID: hcloud://bm-"),
 		},
 		{
 			name:              "unknown format",
 			providerID:        "foobar/321",
 			wantID:            0,
 			wantIsCloudServer: false,
-			wantErr:           true,
+			wantErr:           &UnkownPrefixError{"foobar/321"},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			gotID, gotIsCloudServer, err := ToServerID(tt.providerID)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("ToServerID() error = %v, wantErr %v", err, tt.wantErr)
-				return
+			if tt.wantErr != nil {
+				if err == nil {
+					t.Errorf("ToServerID() expected error = %v, got nil", tt.wantErr)
+					return
+				}
+				if errors.As(tt.wantErr, new(*UnkownPrefixError)) {
+					assert.ErrorAsf(t, err, new(*UnkownPrefixError), "ToServerID() error = %v, wantErr %v", err, tt.wantErr)
+				} else {
+					assert.EqualErrorf(t, err, tt.wantErr.Error(), "ToServerID() error = %v, wantErr %v", err, tt.wantErr)
+				}
+			} else if err != nil {
+				t.Errorf("ToServerID() unexpected error = %v, wantErr nil", err)
 			}
 			if gotID != tt.wantID {
 				t.Errorf("ToServerID() gotID = %v, want %v", gotID, tt.wantID)