From cb3958dc7b75fe9dacb482a48b1e120e4fc3c3ee Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 14 Apr 2023 12:49:36 +0200 Subject: [PATCH 1/4] Fix all logic surrounding providerID - convert providerID to instanceID where needed - rename provider from external-cloudstack to cloudstack - implement InstanceShutdown and InstanceShutdownByProviderID --- cloudstack.go | 11 +++++-- cloudstack_instances.go | 63 ++++++++++++++++++++++++++++++++--------- util.go | 47 ++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 17 deletions(-) create mode 100644 util.go diff --git a/cloudstack.go b/cloudstack.go index 8d78a861..b7b019a5 100644 --- a/cloudstack.go +++ b/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) } diff --git a/cloudstack_instances.go b/cloudstack_instances.go index 91d65751..e043b2f5 100644 --- a/cloudstack_instances.go +++ b/cloudstack_instances.go @@ -52,8 +52,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 { @@ -124,8 +129,13 @@ func (cs *CSCloud) InstanceType(ctx context.Context, name types.NodeName) (strin // 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 { @@ -150,8 +160,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 +181,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 +210,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: labelInvalidCharsRegex.ReplaceAllString(instance.Serviceofferingname, ``), NodeAddresses: addresses, - Zone: cs.zone, - Region: zone.Region, + Zone: labelInvalidCharsRegex.ReplaceAllString(instance.Zonename, ``), + Region: region, }, nil } diff --git a/util.go b/util.go new file mode 100644 index 00000000..070a6f04 --- /dev/null +++ b/util.go @@ -0,0 +1,47 @@ +/* + * 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 +} From 627cdce2c46db1d2c21620ad08df97b6c882c000 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 14 Apr 2023 16:40:20 +0200 Subject: [PATCH 2/4] No need to log absence of external IP --- cloudstack_instances.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cloudstack_instances.go b/cloudstack_instances.go index e043b2f5..f603ed19 100644 --- a/cloudstack_instances.go +++ b/cloudstack_instances.go @@ -29,7 +29,6 @@ import ( 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]`) @@ -86,10 +85,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 From 1dbe52f50281cd2d5800d6bf37e2813328fd874a Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Mon, 17 Apr 2023 12:47:10 +0200 Subject: [PATCH 3/4] Fix instance label value sanitizing --- cloudstack_instances.go | 11 ++++------- util.go | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/cloudstack_instances.go b/cloudstack_instances.go index f603ed19..a1a73420 100644 --- a/cloudstack_instances.go +++ b/cloudstack_instances.go @@ -23,7 +23,6 @@ import ( "context" "errors" "fmt" - "regexp" "github.com/apache/cloudstack-go/v2/cloudstack" corev1 "k8s.io/api/core/v1" @@ -31,8 +30,6 @@ import ( cloudprovider "k8s.io/cloud-provider" ) -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( @@ -119,7 +116,7 @@ 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. @@ -140,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. @@ -232,9 +229,9 @@ func (cs *CSCloud) InstanceMetadata(ctx context.Context, node *corev1.Node) (*cl return &cloudprovider.InstanceMetadata{ ProviderID: node.Spec.ProviderID, - InstanceType: labelInvalidCharsRegex.ReplaceAllString(instance.Serviceofferingname, ``), + InstanceType: sanitizeLabel(instance.Serviceofferingname), NodeAddresses: addresses, - Zone: labelInvalidCharsRegex.ReplaceAllString(instance.Zonename, ``), + Zone: sanitizeLabel(instance.Zonename), Region: region, }, nil } diff --git a/util.go b/util.go index 070a6f04..d7c3d0c1 100644 --- a/util.go +++ b/util.go @@ -45,3 +45,29 @@ func instanceIDFromProviderID(providerID string) (instanceID string, region stri } 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 +} From 7633e51987030311cc7a8965c2b77dae2c2971d9 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Fri, 7 Jun 2024 08:21:18 +0200 Subject: [PATCH 4/4] Update util.go Co-authored-by: Vishesh --- util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util.go b/util.go index d7c3d0c1..a9adc855 100644 --- a/util.go +++ b/util.go @@ -26,7 +26,7 @@ import ( ) // If Instances.InstanceID or cloudprovider.GetInstanceProviderID is changed, the regexp should be changed too. -var providerIDRegexp = regexp.MustCompile(`^` + ProviderName + `://([^/]*)/([^/]+)$`) +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 ':///'.