diff --git a/hcloud/instances.go b/hcloud/instances.go index 355cc31b7..660a2c442 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 04b3f59d6..938f907e0 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/hcops/load_balancer_test.go b/internal/hcops/load_balancer_test.go index 487d84a94..df79f58fd 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.go b/internal/providerid/providerid.go index 53892bff9..35a7751ba 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 == "" { diff --git a/internal/providerid/providerid_test.go b/internal/providerid/providerid_test.go index eed44f7a4..5bee7cf29 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)