Fix providerID handling + label sanitizing

cloudstack.go

@@ -34,7 +34,7 @@ import (
 )
 
 // ProviderName is the name of this cloud provider.
-const ProviderName = "external-cloudstack"
+const ProviderName = "cloudstack"
 
 // CSConfig wraps the config for the CloudStack cloud provider.
 type CSConfig struct {
@@ -199,13 +199,18 @@ func (cs *CSCloud) GetZone(ctx context.Context) (cloudprovider.Zone, error) {
 func (cs *CSCloud) GetZoneByProviderID(ctx context.Context, providerID string) (cloudprovider.Zone, error) {
 	zone := cloudprovider.Zone{}
 
+	id, _, err := instanceIDFromProviderID(providerID)
+	if err != nil {
+		return zone, err
+	}
+
 	instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByID(
-		providerID,
+		id,
 		cloudstack.WithProject(cs.projectID),
 	)
 	if err != nil {
 		if count == 0 {
-			return zone, fmt.Errorf("could not find node by ID: %v", providerID)
+			return zone, fmt.Errorf("could not find node by ID: %v", id)
 		}
 		return zone, fmt.Errorf("error retrieving zone: %v", err)
 	}

cloudstack_instances.go

@@ -23,17 +23,13 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"regexp"
 
 	"github.com/apache/cloudstack-go/v2/cloudstack"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	cloudprovider "k8s.io/cloud-provider"
-	"k8s.io/klog/v2"
 )
 
-var labelInvalidCharsRegex *regexp.Regexp = regexp.MustCompile(`([^A-Za-z0-9][^-A-Za-z0-9_.]*)?[^A-Za-z0-9]`)
-
 // NodeAddresses returns the addresses of the specified instance.
 func (cs *CSCloud) NodeAddresses(ctx context.Context, name types.NodeName) ([]corev1.NodeAddress, error) {
 	instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName(
@@ -52,8 +48,13 @@ func (cs *CSCloud) NodeAddresses(ctx context.Context, name types.NodeName) ([]co
 
 // NodeAddressesByProviderID returns the addresses of the specified instance.
 func (cs *CSCloud) NodeAddressesByProviderID(ctx context.Context, providerID string) ([]corev1.NodeAddress, error) {
+	id, _, err := instanceIDFromProviderID(providerID)
+	if err != nil {
+		return nil, err
+	}
+
 	instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByID(
-		providerID,
+		id,
 		cloudstack.WithProject(cs.projectID),
 	)
 	if err != nil {
@@ -81,10 +82,6 @@ func (cs *CSCloud) nodeAddresses(instance *cloudstack.VirtualMachine) ([]corev1.
 
 	if instance.Publicip != "" {
 		addresses = append(addresses, corev1.NodeAddress{Type: corev1.NodeExternalIP, Address: instance.Publicip})
-	} else {
-		// Since there is no sane way to determine the external IP if the host isn't
-		// using static NAT, we will just fire a log message and omit the external IP.
-		klog.V(4).Infof("Could not determine the public IP of host %v (%v)", instance.Name, instance.Id)
 	}
 
 	return addresses, nil
@@ -119,13 +116,18 @@ func (cs *CSCloud) InstanceType(ctx context.Context, name types.NodeName) (strin
 		return "", fmt.Errorf("error retrieving instance type: %v", err)
 	}
 
-	return labelInvalidCharsRegex.ReplaceAllString(instance.Serviceofferingname, ``), nil
+	return sanitizeLabel(instance.Serviceofferingname), nil
 }
 
 // InstanceTypeByProviderID returns the type of the specified instance.
 func (cs *CSCloud) InstanceTypeByProviderID(ctx context.Context, providerID string) (string, error) {
+	id, _, err := instanceIDFromProviderID(providerID)
+	if err != nil {
+		return "", err
+	}
+
 	instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByID(
-		providerID,
+		id,
 		cloudstack.WithProject(cs.projectID),
 	)
 	if err != nil {
@@ -135,7 +137,7 @@ func (cs *CSCloud) InstanceTypeByProviderID(ctx context.Context, providerID stri
 		return "", fmt.Errorf("error retrieving instance type: %v", err)
 	}
 
-	return labelInvalidCharsRegex.ReplaceAllString(instance.Serviceofferingname, ``), nil
+	return sanitizeLabel(instance.Serviceofferingname), nil
 }
 
 // AddSSHKeyToAllInstances is currently not implemented.
@@ -150,8 +152,13 @@ func (cs *CSCloud) CurrentNodeName(ctx context.Context, hostname string) (types.
 
 // InstanceExistsByProviderID returns if the instance still exists.
 func (cs *CSCloud) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {
+	id, _, err := instanceIDFromProviderID(providerID)
+	if err != nil {
+		return false, err
+	}
+
 	_, count, err := cs.client.VirtualMachine.GetVirtualMachineByID(
-		providerID,
+		id,
 		cloudstack.WithProject(cs.projectID),
 	)
 	if err != nil {
@@ -166,7 +173,22 @@ func (cs *CSCloud) InstanceExistsByProviderID(ctx context.Context, providerID st
 
 // InstanceShutdownByProviderID returns true if the instance is in safe state to detach volumes
 func (cs *CSCloud) InstanceShutdownByProviderID(ctx context.Context, providerID string) (bool, error) {
-	return false, cloudprovider.NotImplemented
+	id, _, err := instanceIDFromProviderID(providerID)
+	if err != nil {
+		return false, err
+	}
+
+	instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByID(
+		id,
+		cloudstack.WithProject(cs.projectID),
+	)
+	if err != nil {
+		if count == 0 {
+			return false, cloudprovider.InstanceNotFound
+		}
+		return false, fmt.Errorf("error retrieving instance state: %v", err)
+	}
+	return instance != nil && instance.State == "Stopped", nil
 }
 
 func (cs *CSCloud) InstanceExists(ctx context.Context, node *corev1.Node) (bool, error) {
@@ -180,31 +202,36 @@ func (cs *CSCloud) InstanceExists(ctx context.Context, node *corev1.Node) (bool,
 }
 
 func (cs *CSCloud) InstanceShutdown(ctx context.Context, node *corev1.Node) (bool, error) {
-	return false, cloudprovider.NotImplemented
+	return cs.InstanceShutdownByProviderID(ctx, node.Spec.ProviderID)
 }
 
 func (cs *CSCloud) InstanceMetadata(ctx context.Context, node *corev1.Node) (*cloudprovider.InstanceMetadata, error) {
-
-	instanceType, err := cs.InstanceType(ctx, types.NodeName(node.Name))
+	id, region, err := instanceIDFromProviderID(node.Spec.ProviderID)
 	if err != nil {
 		return nil, err
 	}
 
-	addresses, err := cs.NodeAddresses(ctx, types.NodeName(node.Name))
+	instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByID(
+		id,
+		cloudstack.WithProject(cs.projectID),
+	)
 	if err != nil {
-		return nil, err
+		if count == 0 {
+			return nil, cloudprovider.InstanceNotFound
+		}
+		return nil, fmt.Errorf("error retrieving instance: %v", err)
 	}
 
-	zone, err := cs.GetZone(ctx)
+	addresses, err := cs.nodeAddresses(instance)
 	if err != nil {
 		return nil, err
 	}
 
 	return &cloudprovider.InstanceMetadata{
-		ProviderID:    cs.ProviderName(),
-		InstanceType:  instanceType,
+		ProviderID:    node.Spec.ProviderID,
+		InstanceType:  sanitizeLabel(instance.Serviceofferingname),
 		NodeAddresses: addresses,
-		Zone:          cs.zone,
-		Region:        zone.Region,
+		Zone:          sanitizeLabel(instance.Zonename),
+		Region:        region,
 	}, nil
 }

util.go

@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package cloudstack
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+)
+
+// If Instances.InstanceID or cloudprovider.GetInstanceProviderID is changed, the regexp should be changed too.
+var providerIDRegexp = regexp.MustCompile(`^` + ProviderName + `://([^/]*)/([^/]+)$`)
+
+// instanceIDFromProviderID splits a provider's id and return instanceID.
+// A providerID is build out of '${ProviderName}:///${instance-id}' which contains ':///'.
+// or '${ProviderName}://${region}/${instance-id}' which contains '://'.
+// See cloudprovider.GetInstanceProviderID and Instances.InstanceID.
+func instanceIDFromProviderID(providerID string) (instanceID string, region string, err error) {
+
+	// https://github.com/kubernetes/kubernetes/issues/85731
+	if providerID != "" && !strings.Contains(providerID, "://") {
+		providerID = ProviderName + "://" + providerID
+	}
+
+	matches := providerIDRegexp.FindStringSubmatch(providerID)
+	if len(matches) != 3 {
+		return "", "", fmt.Errorf("ProviderID \"%s\" didn't match expected format \"cloudstack://region/InstanceID\"", providerID)
+	}
+	return matches[2], matches[1], nil
+}
+
+// Sanitize label value so it complies with https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set
+// Anything but [-A-Za-z0-9_.] will get converted to '_'
+func sanitizeLabel(value string) string {
+	fn := func(r rune) rune {
+		if r >= 'a' && r <= 'z' ||
+			r >= 'A' && r <= 'Z' ||
+			r >= '0' && r <= '9' ||
+			r == '-' || r == '_' || r == '.' {
+			return r
+		}
+		return '_'
+	}
+	value = strings.Map(fn, value)
+
+	// Must start & end with alphanumeric char
+	value = strings.Trim(value, "-_.")
+
+	// Strip anything over 63 chars
+	if len(value) > 63 {
+		value = value[:63]
+		value = strings.Trim(value, "-_.")
+	}
+
+	return value
+}