🌱 Support hrobot:// ProviderID of upstream ccm. (#72)

Author: guettli

README.md

@@ -9,7 +9,14 @@ The Hetzner Cloud controller manager seamlessly integrates your Kubernetes clust
 
 ## About the Fork
 
-In the long run, we (Syself) would like to switch to the [upstream ccm](https://github.com/syself/hetzner-cloud-controller-manager/) again.
+We plan to support the [upstream hcloud
+ccm](https://github.com/hetznercloud/hcloud-cloud-controller-manager/) in
+[CAPH](https://github.com/syself/cluster-api-provider-hetzner/). After that, this fork is no longer
+needed.
+
+## About the Fork (old)
+
+In the long run, we (Syself) would like to switch to the [upstream ccm](https://github.com/hetznercloud/hcloud-cloud-controller-manager/) again.
 
 A lot of changes were made in the upstream fork, and we don't plan to merge them into our fork.
 
@@ -62,10 +69,16 @@ See [CAPH docs](https://syself.com/docs/caph/topics/baremetal/creating-workload-
 
 ## Usage
 
-We recommend to mount the secret `hetzner` as volume and make it available for the container as `/etc/hetzner-secret`.
-Then the credentials are automatically reloaded, when the secret changes.
-When you use hot-reloading, the secret keys must be named `hcloud` (or `token`, for upstream hcloud-ccm compatibility), `robot-user`, and `robot-password`.
-You see an example in the [ccm helm chart](https://github.com/syself/charts/tree/main/charts/ccm-hetzner)
+We recommend to mount the secret `hetzner` as volume and make it available for the container as
+`/etc/hetzner-secret`. Then the credentials are automatically reloaded, when the secret changes.
+
+When you use hot-reloading, the secret keys must be named `hcloud` (or `token`, for upstream
+hcloud-ccm compatibility), `robot-user`, and `robot-password`. You see an example in the [ccm helm
+chart](https://github.com/syself/charts/tree/main/charts/ccm-hetzner)
+
+For bare-metal nodes without an existing ProviderID, you can switch to `hrobot://<id>` via
+`--use-hrobot-provider-id-for-baremetal` (or
+`HCLOUD_USE_HROBOT_PROVIDER_ID_FOR_BAREMETAL=true`).
 
 ## Env Variables
 
@@ -75,6 +88,9 @@ CACHE_TIMEOUT: Timeout of the Robot API Cache. See [ParseDuration](https://pkg.g
 
 HCLOUD_ENDPOINT: Defaults to `https://api.hetzner.cloud/v1`
 
+HCLOUD_USE_HROBOT_PROVIDER_ID_FOR_BAREMETAL: When set to `true`, newly initialized bare-metal
+nodes use `hrobot://<id>` instead of `hcloud://bm-<id>`.
+
 Additional Env Variables are defined at the top of [cloud.go](https://github.com/syself/hetzner-cloud-controller-manager/blob/master/hcloud/cloud.go)
 
 Deprecated (use mounted secret instead):

hcloud/cloud.go

@@ -65,6 +65,7 @@ const (
 	hcloudLoadBalancersUsePrivateIP          = "HCLOUD_LOAD_BALANCERS_USE_PRIVATE_IP"
 	hcloudLoadBalancersDisableIPv6           = "HCLOUD_LOAD_BALANCERS_DISABLE_IPV6"
 	hcloudMetricsEnabledENVVar               = "HCLOUD_METRICS_ENABLED"
+	UseHrobotProviderIDForBaremetalEnvVar    = "HCLOUD_USE_HROBOT_PROVIDER_ID_FOR_BAREMETAL"
 	hcloudMetricsAddress                     = ":8233"
 	providerName                             = "hcloud"
 	hostNamePrefixRobot                      = "bm-"
@@ -247,6 +248,10 @@ func newCloud(_ io.Reader) (cloudprovider.Interface, error) {
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", op, err)
 	}
+	useHrobotProviderIDForBaremetal, err := getEnvBool(UseHrobotProviderIDForBaremetalEnvVar)
+	if err != nil {
+		return nil, fmt.Errorf("%s: %w", op, err)
+	}
 
 	credentialsDir := credentials.GetDirectory(rootDir)
 	_, err = os.Stat(credentialsDir)
@@ -261,7 +266,7 @@ func newCloud(_ io.Reader) (cloudprovider.Interface, error) {
 	return &cloud{
 		hcloudClient: hcloudClient,
 		robotClient:  robotClient,
-		instances:    newInstances(hcloudClient, robotClient, instancesAddressFamily, networkID),
+		instances:    newInstances(hcloudClient, robotClient, instancesAddressFamily, networkID, useHrobotProviderIDForBaremetal),
 		loadBalancer: loadBalancers,
 		routes:       nil,
 		networkID:    networkID,

hcloud/cloud_test.go

@@ -436,7 +436,7 @@ func Test_updateHcloudCredentials(t *testing.T) {
 			},
 		})
 	})
-	i := newInstances(hcloudClient, nil, AddressFamilyIPv4, 0)
+	i := newInstances(hcloudClient, nil, AddressFamilyIPv4, 0, false)
 
 	node := &corev1.Node{
 		Spec: corev1.NodeSpec{ProviderID: "hcloud://1"},

hcloud/instances.go

@@ -40,16 +40,29 @@ const (
 )
 
 type instances struct {
-	client        *hcloud.Client
-	robotClient   robotclient.Client
-	addressFamily addressFamily
-	networkID     int64
+	client              *hcloud.Client
+	robotClient         robotclient.Client
+	addressFamily       addressFamily
+	networkID           int64
+	useHrobotProviderID bool
 }
 
 var errServerNotFound = fmt.Errorf("server not found")
 
-func newInstances(client *hcloud.Client, robotClient robotclient.Client, addressFamily addressFamily, networkID int64) *instances {
-	return &instances{client, robotClient, addressFamily, networkID}
+func newInstances(
+	client *hcloud.Client,
+	robotClient robotclient.Client,
+	addressFamily addressFamily,
+	networkID int64,
+	useHrobotProviderID bool,
+) *instances {
+	return &instances{
+		client:              client,
+		robotClient:         robotClient,
+		addressFamily:       addressFamily,
+		networkID:           networkID,
+		useHrobotProviderID: useHrobotProviderID,
+	}
 }
 
 // lookupServer attempts to locate the corresponding hcloud.Server or models.Server (robot server) for a given v1.Node.
@@ -161,8 +174,13 @@ func (i *instances) InstanceMetadata(ctx context.Context, node *corev1.Node) (me
 		return nil, fmt.Errorf("failed to get instance metadata: no matching bare metal server found for node '%s': %w",
 			node.Name, errServerNotFound)
 	}
+	providerID := providerid.GetBaremetalProviderID(node, bmServer.ServerNumber, i.useHrobotProviderID)
 	return &cloudprovider.InstanceMetadata{
-		ProviderID:    providerid.LegacyFromRobotServerNumber(bmServer.ServerNumber),
+		// By default this returns the legacy format "hcloud://bm-NNNN". When the
+		// --use-hrobot-provider-id-for-baremetal flag is set, it returns "hrobot://NNNN".
+		// We will use the upstream hcloud-ccm in the future. Related PR for caph:
+		// https://github.com/syself/cluster-api-provider-hetzner/pull/1703
+		ProviderID:    providerID,
 		InstanceType:  getInstanceTypeOfRobotServer(bmServer),
 		NodeAddresses: robotNodeAddresses(i.addressFamily, bmServer),
 		Zone:          getZoneOfRobotServer(bmServer),

hcloud/instances_test.go

@@ -89,7 +89,7 @@ func TestInstances_InstanceExists(t *testing.T) {
 		})
 	})
 
-	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0, false)
 
 	tests := []struct {
 		name     string
@@ -111,6 +111,15 @@ func TestInstances_InstanceExists(t *testing.T) {
 				Spec: corev1.NodeSpec{ProviderID: "hcloud://bm-321"},
 			},
 			expected: true,
+		}, {
+			name: "existing robot server by id (hrobot)",
+			node: &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "bm-server1",
+				},
+				Spec: corev1.NodeSpec{ProviderID: "hrobot://321"},
+			},
+			expected: true,
 		}, {
 			name: "missing server by id",
 			node: &corev1.Node{
@@ -126,6 +135,15 @@ func TestInstances_InstanceExists(t *testing.T) {
 				Spec: corev1.NodeSpec{ProviderID: "hcloud://bm-322"},
 			},
 			expected: false,
+		}, {
+			name: "missing robot server by id (hrobot)",
+			node: &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "bm-server2",
+				},
+				Spec: corev1.NodeSpec{ProviderID: "hrobot://322"},
+			},
+			expected: false,
 		}, {
 			name: "existing server by name",
 			node: &corev1.Node{
@@ -206,7 +224,7 @@ func TestInstances_InstanceExistsRobotServerCreatedAfterCacheFill(t *testing.T)
 		t.Fatalf("Unexpected error creating cached robot client: %v", err)
 	}
 
-	instances := newInstances(env.Client, robotClient, AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, robotClient, AddressFamilyIPv4, 0, false)
 
 	// Warm the cache while bm-new does not exist yet.
 	exists, err := instances.InstanceExists(context.TODO(), &corev1.Node{
@@ -278,7 +296,7 @@ func TestInstances_InstanceExistsRobotServerRepeatedMissingNameSkipsSecondForceR
 		t.Fatalf("Unexpected error creating cached robot client: %v", err)
 	}
 
-	instances := newInstances(env.Client, robotClient, AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, robotClient, AddressFamilyIPv4, 0, false)
 	node := &corev1.Node{
 		ObjectMeta: metav1.ObjectMeta{Name: "bm-missing"},
 	}
@@ -348,7 +366,7 @@ func TestInstances_InstanceShutdown(t *testing.T) {
 		})
 	})
 
-	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0, false)
 
 	tests := []struct {
 		name     string
@@ -376,6 +394,15 @@ func TestInstances_InstanceShutdown(t *testing.T) {
 				},
 			},
 			expected: false,
+		}, {
+			name: "bm server (hrobot)",
+			node: &corev1.Node{
+				Spec: corev1.NodeSpec{ProviderID: "hrobot://321"},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "bm-server1",
+				},
+			},
+			expected: false,
 		},
 	}
 
@@ -416,7 +443,7 @@ func TestInstances_InstanceMetadata(t *testing.T) {
 		})
 	})
 
-	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0, false)
 
 	metadata, err := instances.InstanceMetadata(context.TODO(), &corev1.Node{
 		Spec: corev1.NodeSpec{ProviderID: "hcloud://1"},
@@ -457,7 +484,7 @@ func TestInstances_InstanceMetadataRobotServer(t *testing.T) {
 		})
 	})
 
-	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0, false)
 
 	metadata, err := instances.InstanceMetadata(context.TODO(), &corev1.Node{
 		ObjectMeta: metav1.ObjectMeta{
@@ -485,6 +512,86 @@ func TestInstances_InstanceMetadataRobotServer(t *testing.T) {
 	}
 }
 
+func TestInstances_InstanceMetadataRobotServerUsesHrobotProviderIDFlag(t *testing.T) {
+	env := newTestEnv()
+	defer env.Teardown()
+	env.Mux.HandleFunc("/robot/server", func(w http.ResponseWriter, _ *http.Request) {
+		json.NewEncoder(w).Encode([]models.ServerResponse{
+			{
+				Server: models.Server{
+					ServerIP:      "123.123.123.123",
+					ServerIPv6Net: "2a01:f48:111:4221::",
+					ServerNumber:  321,
+					Product:       "bm-product 1",
+					Name:          "bm-server1",
+					Dc:            "NBG1-DC1",
+				},
+			},
+		})
+	})
+
+	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0, true)
+
+	metadata, err := instances.InstanceMetadata(context.TODO(), &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "bm-server1",
+		},
+	})
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+
+	expectedMetadata := &cloudprovider.InstanceMetadata{
+		ProviderID:   "hrobot://321",
+		InstanceType: "bm-product-1",
+		NodeAddresses: []corev1.NodeAddress{
+			{Type: corev1.NodeHostName, Address: "bm-server1"},
+			{Type: corev1.NodeExternalIP, Address: "123.123.123.123"},
+		},
+		Zone:   "nbg1",
+		Region: "eu-central",
+	}
+
+	if !reflect.DeepEqual(metadata, expectedMetadata) {
+		t.Fatalf("Expected metadata %+v but got %+v", *expectedMetadata, *metadata)
+	}
+}
+
+func TestInstances_InstanceMetadataRobotServerKeepsExistingProviderIDWhenFlagIsEnabled(t *testing.T) {
+	env := newTestEnv()
+	defer env.Teardown()
+	env.Mux.HandleFunc("/robot/server/321", func(w http.ResponseWriter, _ *http.Request) {
+		json.NewEncoder(w).Encode(models.ServerResponse{
+			Server: models.Server{
+				ServerIP:      "123.123.123.123",
+				ServerIPv6Net: "2a01:f48:111:4221::",
+				ServerNumber:  321,
+				Product:       "bm-product 1",
+				Name:          "bm-server1",
+				Dc:            "NBG1-DC1",
+			},
+		})
+	})
+
+	instances := newInstances(env.Client, env.RobotClient, AddressFamilyIPv4, 0, true)
+
+	metadata, err := instances.InstanceMetadata(context.TODO(), &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "bm-server1",
+		},
+		// Existing nodes must keep their ProviderID even when the new format is
+		// enabled, otherwise migrations would silently rewrite node identity.
+		Spec: corev1.NodeSpec{ProviderID: "hcloud://bm-321"},
+	})
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+
+	if metadata.ProviderID != "hcloud://bm-321" {
+		t.Fatalf("expected existing provider id to be kept, got %q", metadata.ProviderID)
+	}
+}
+
 func TestNodeAddresses(t *testing.T) {
 	tests := []struct {
 		name           string

internal/providerid/providerid.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
+
+	corev1 "k8s.io/api/core/v1"
 )
 
 const (
@@ -17,6 +19,8 @@ const (
 	//
 	// It MUST not be changed, otherwise existing nodes will not be recognized anymore.
 	prefixRobotLegacy = "hcloud://bm-"
+
+	prefixRobotNew = "hrobot://"
 )
 
 type UnkownPrefixError struct {
@@ -25,9 +29,10 @@ type UnkownPrefixError struct {
 
 func (e *UnkownPrefixError) Error() string {
 	return fmt.Sprintf(
-		"Provider ID does not have one of the the expected prefixes (%s, %s): %s",
+		"Provider ID does not have one of the the expected prefixes (%s, %s, %s): %s",
 		prefixCloud,
 		prefixRobotLegacy,
+		prefixRobotNew,
 		e.ProviderID,
 	)
 }
@@ -40,6 +45,9 @@ func (e *UnkownPrefixError) Error() string {
 func ToServerID(providerID string) (id int64, isCloudServer bool, err error) {
 	idString := ""
 	switch {
+	case strings.HasPrefix(providerID, prefixRobotNew):
+		idString = strings.ReplaceAll(providerID, prefixRobotNew, "")
+
 	case strings.HasPrefix(providerID, prefixRobotLegacy):
 		// This case needs to be before [prefixCloud], as [prefixCloud] is a superset of [prefixRobotLegacy]
 		idString = strings.ReplaceAll(providerID, prefixRobotLegacy, "")
@@ -68,7 +76,14 @@ func FromCloudServerID(serverID int64) string {
 	return fmt.Sprintf("%s%d", prefixCloud, serverID)
 }
 
-// LegacyFromRobotServerNumber generates the canonical ProviderID for a Robot Server.
-func LegacyFromRobotServerNumber(serverNumber int) string {
+// GetBaremetalProviderID creates a ProviderID for baremetal servers. Depending on the
+// configuration, it is either hcloud://bm-NNNN or hrobot://NNNN.
+func GetBaremetalProviderID(node *corev1.Node, serverNumber int, useHrobotProviderID bool) string {
+	if node.Spec.ProviderID != "" {
+		return node.Spec.ProviderID
+	}
+	if useHrobotProviderID {
+		return fmt.Sprintf("%s%d", prefixRobotNew, serverNumber)
+	}
 	return fmt.Sprintf("%s%d", prefixRobotLegacy, serverNumber)
 }