feat(load-balancer): ignore nodes that don't use known provider IDs (#…

…780) With this feature the load balancer is more resilient to hybrid clusters as it skips nodes with unkown provider id prefixes. --------- Co-authored-by: Jonas L. <[email protected]>
hetznercloud · Nov 20, 2024 · 6544740 · 6544740
1 parent 81cc8b2
commit 6544740
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 17 deletions.
diff --git 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
@@ -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/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.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 == "" {

diff --git 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)