From 98bd3c586943fd11614d979cdafcbb19a72af82e Mon Sep 17 00:00:00 2001 From: Pearl Dsilva Date: Fri, 19 Jan 2024 12:25:47 -0500 Subject: [PATCH 01/10] NSX: (temp fix) Skip adding firewall rules for CKS Clusters on VPC tiers (#56) Currently CKP does not setup NetworkACLs for CKS clusters on VPC tiers, and fails to add Firewall rules - as Firewall isn't supported on VPCs. This is a partial fix, to skip setting up Firewall rules if the network doesn't support the service. --- cloudstack_loadbalancer.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/cloudstack_loadbalancer.go b/cloudstack_loadbalancer.go index b796dfa9..1859da7d 100644 --- a/cloudstack_loadbalancer.go +++ b/cloudstack_loadbalancer.go @@ -163,7 +163,15 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s } } - if lbRule != nil { + network, count, err := lb.Network.GetNetworkByID(lb.networkID, cloudstack.WithProject(lb.projectID)) + if err != nil { + if count == 0 { + return nil, err + } + return nil, err + } + + if lbRule != nil && isFirewallSupported(network.Service) { klog.V(4).Infof("Creating firewall rules for load balancer rule: %v (%v:%v:%v)", lbRuleName, protocol, lbRule.Publicip, port.Port) if _, err := lb.updateFirewallRule(lbRule.Publicipid, int(port.Port), protocol, service.Spec.LoadBalancerSourceRanges); err != nil { return nil, err @@ -244,6 +252,15 @@ func (cs *CSCloud) UpdateLoadBalancer(ctx context.Context, clusterName string, s return nil } +func isFirewallSupported(services []cloudstack.NetworkServiceInternal) bool { + for _, svc := range services { + if svc.Name == "Firewall" { + return true + } + } + return false +} + // EnsureLoadBalancerDeleted deletes the specified load balancer if it exists, returning // nil if the load balancer specified either didn't exist or was successfully deleted. func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName string, service *corev1.Service) error { From a1522b4d40e5741ac1373a5bee149207a1ab2eda Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 14 Apr 2023 12:49:36 +0200 Subject: [PATCH 02/10] 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 | 67 ++++++++++++++++++++++++++++++++--------- util.go | 47 +++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 18 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..1bacfb0d 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 { @@ -103,7 +108,9 @@ func (cs *CSCloud) InstanceID(ctx context.Context, name types.NodeName) (string, return "", fmt.Errorf("error retrieving instance ID: %v", err) } - return instance.Id, nil + // return instance ID with empty region + // in the future, with region support, this should be / + return "/" + instance.Id, nil } // InstanceType returns the type of the specified instance. @@ -124,8 +131,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 +162,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 +183,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 +212,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 40d039ca29adea091f9435e74ae8269e9886026a Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 14 Apr 2023 16:40:20 +0200 Subject: [PATCH 03/10] 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 1bacfb0d..bc123aec 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 779b0ff7e2e7e42781056271bb05c95d310816f8 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Mon, 17 Apr 2023 12:47:10 +0200 Subject: [PATCH 04/10] 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 bc123aec..f3406007 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( @@ -121,7 +118,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. @@ -142,7 +139,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. @@ -234,9 +231,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 28481de2549723234746b987d792b292c7a67e09 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Thu, 4 May 2023 15:49:18 +0200 Subject: [PATCH 05/10] Strip domain from node.Name before matching with vm names Signed-off-by: Hans Rakers --- cloudstack_loadbalancer.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cloudstack_loadbalancer.go b/cloudstack_loadbalancer.go index 1859da7d..0e2484a4 100644 --- a/cloudstack_loadbalancer.go +++ b/cloudstack_loadbalancer.go @@ -349,7 +349,9 @@ func (cs *CSCloud) getLoadBalancer(service *corev1.Service) (*loadBalancer, erro func (cs *CSCloud) verifyHosts(nodes []*corev1.Node) ([]string, string, error) { hostNames := map[string]bool{} for _, node := range nodes { - hostNames[strings.ToLower(node.Name)] = true + // node.Name can be an FQDN as well, and CloudStack VM names aren't + // To match, we need to Split the domain part off here, if present + hostNames[strings.Split(strings.ToLower(node.Name), ".")[0]] = true } p := cs.client.VirtualMachine.NewListVirtualMachinesParams() @@ -379,6 +381,10 @@ func (cs *CSCloud) verifyHosts(nodes []*corev1.Node) ([]string, string, error) { } } + if len(hostIDs) == 0 || len(networkID) == 0 { + return nil, "", fmt.Errorf("none of the hosts matched the list of VMs retrieved from CS API") + } + return hostIDs, networkID, nil } From 6ecc79255c73030d61aec993082c40e4e360bc9c Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 14 Apr 2023 11:19:32 +0200 Subject: [PATCH 06/10] Add hostname annotation to set lb ingress hostname This fixes problems with kube-proxy in ipvs mode considering the lb IP as local to the node See https://github.com/kubernetes/kubernetes/issues/66607 This can also be used to access PROXY proto service from the inside --- cloudstack_loadbalancer.go | 63 +++++++++++++++++++++++++++++++++++--- protocol.go | 18 ++--------- 2 files changed, 62 insertions(+), 19 deletions(-) diff --git a/cloudstack_loadbalancer.go b/cloudstack_loadbalancer.go index 0e2484a4..6bb80492 100644 --- a/cloudstack_loadbalancer.go +++ b/cloudstack_loadbalancer.go @@ -32,9 +32,19 @@ import ( cloudprovider "k8s.io/cloud-provider" ) -// defaultAllowedCIDR is the network range that is allowed on the firewall -// by default when no explicit CIDR list is given on a LoadBalancer. -const defaultAllowedCIDR = "0.0.0.0/0" +const ( + // defaultAllowedCIDR is the network range that is allowed on the firewall + // by default when no explicit CIDR list is given on a LoadBalancer. + defaultAllowedCIDR = "0.0.0.0/0" + + // ServiceAnnotationLoadBalancerProxyProtocol is the annotation used on the + // service to enable the proxy protocol on a CloudStack load balancer. + // Note that this protocol only applies to TCP service ports and + // CloudStack >= 4.6 is required for it to work. + ServiceAnnotationLoadBalancerProxyProtocol = "service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol" + + ServiceAnnotationLoadBalancerLoadbalancerHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname" +) type loadBalancer struct { *cloudstack.CloudStackClient @@ -123,7 +133,7 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s for _, port := range service.Spec.Ports { // Construct the protocol name first, we need it a few times - protocol := ProtocolFromServicePort(port, service.Annotations) + protocol := ProtocolFromServicePort(port, service) if protocol == LoadBalancerProtocolInvalid { return nil, fmt.Errorf("unsupported load balancer protocol: %v", port.Protocol) } @@ -202,6 +212,13 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s } status = &corev1.LoadBalancerStatus{} + // If hostname is explicitly set using service annotation + // Workaround for https://github.com/kubernetes/kubernetes/issues/66607 + if hostname := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerLoadbalancerHostname, ""); hostname != "" { + status.Ingress = []corev1.LoadBalancerIngress{{Hostname: hostname}} + return status, nil + } + // Default to IP status.Ingress = []corev1.LoadBalancerIngress{{IP: lb.ipAddr}} return status, nil @@ -810,3 +827,41 @@ func (lb *loadBalancer) deleteFirewallRule(publicIpId string, publicPort int, pr return deleted, err } + +// getStringFromServiceAnnotation searches a given v1.Service for a specific annotationKey and either returns the annotation's value or a specified defaultSetting +func getStringFromServiceAnnotation(service *corev1.Service, annotationKey string, defaultSetting string) string { + klog.V(4).Infof("getStringFromServiceAnnotation(%s/%s, %v, %v)", service.Namespace, service.Name, annotationKey, defaultSetting) + if annotationValue, ok := service.Annotations[annotationKey]; ok { + //if there is an annotation for this setting, set the "setting" var to it + // annotationValue can be empty, it is working as designed + // it makes possible for instance provisioning loadbalancer without floatingip + klog.V(4).Infof("Found a Service Annotation: %v = %v", annotationKey, annotationValue) + return annotationValue + } + //if there is no annotation, set "settings" var to the value from cloud config + if defaultSetting != "" { + klog.V(4).Infof("Could not find a Service Annotation; falling back on cloud-config setting: %v = %v", annotationKey, defaultSetting) + } + return defaultSetting +} + +// getBoolFromServiceAnnotation searches a given v1.Service for a specific annotationKey and either returns the annotation's boolean value or a specified defaultSetting +func getBoolFromServiceAnnotation(service *corev1.Service, annotationKey string, defaultSetting bool) bool { + klog.V(4).Infof("getBoolFromServiceAnnotation(%s/%s, %v, %v)", service.Namespace, service.Name, annotationKey, defaultSetting) + if annotationValue, ok := service.Annotations[annotationKey]; ok { + returnValue := false + switch annotationValue { + case "true": + returnValue = true + case "false": + returnValue = false + default: + returnValue = defaultSetting + } + + klog.V(4).Infof("Found a Service Annotation: %v = %v", annotationKey, returnValue) + return returnValue + } + klog.V(4).Infof("Could not find a Service Annotation; falling back to default setting: %v = %v", annotationKey, defaultSetting) + return defaultSetting +} diff --git a/protocol.go b/protocol.go index c9142237..07d81748 100644 --- a/protocol.go +++ b/protocol.go @@ -20,7 +20,7 @@ package cloudstack import ( - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" ) // LoadBalancerProtocol represents a specific network protocol supported by the CloudStack load balancer. @@ -35,14 +35,6 @@ const ( LoadBalancerProtocolInvalid ) -// ServiceAnnotationLoadBalancerProxyProtocol is the annotation used on the -// service to enable the proxy protocol on a CloudStack load balancer. -// The value of this annotation is ignored, even if it is seemingly boolean. -// Simple presence of the annotation will enable it. -// Note that this protocol only applies to TCP service ports and -// CloudStack 4.6 is required for it to work. -const ServiceAnnotationLoadBalancerProxyProtocol = "service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol" - // String returns the same value as CSProtocol. func (p LoadBalancerProtocol) String() string { return p.CSProtocol() @@ -89,12 +81,8 @@ func (p LoadBalancerProtocol) IPProtocol() string { // -> "tcp-proxy" (CloudStack 4.6 and later) // // Other values return LoadBalancerProtocolInvalid. -func ProtocolFromServicePort(port v1.ServicePort, annotations map[string]string) LoadBalancerProtocol { - proxy := false - // FIXME this accepts any value as true, even "false", 0 or other falsey stuff - if _, ok := annotations[ServiceAnnotationLoadBalancerProxyProtocol]; ok { - proxy = true - } +func ProtocolFromServicePort(port v1.ServicePort, service *v1.Service) LoadBalancerProtocol { + proxy := getBoolFromServiceAnnotation(service, ServiceAnnotationLoadBalancerProxyProtocol, false) switch port.Protocol { case v1.ProtocolTCP: if proxy { From 115441494fd98be77117dedb3a00be4ff9ded68f Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 14 Jul 2023 17:08:24 +0200 Subject: [PATCH 07/10] Simplify by only implementing InstancesV2 --- cloudstack.go | 107 ++------------------- cloudstack_instances.go | 202 +++++++++++----------------------------- 2 files changed, 59 insertions(+), 250 deletions(-) diff --git a/cloudstack.go b/cloudstack.go index b7b019a5..229ed6dd 100644 --- a/cloudstack.go +++ b/cloudstack.go @@ -20,17 +20,13 @@ package cloudstack import ( - "context" "errors" "fmt" "io" - "os" "github.com/apache/cloudstack-go/v2/cloudstack" "gopkg.in/gcfg.v1" - "k8s.io/apimachinery/pkg/types" cloudprovider "k8s.io/cloud-provider" - "k8s.io/klog/v2" ) // ProviderName is the name of this cloud provider. @@ -113,13 +109,13 @@ func (cs *CSCloud) LoadBalancer() (cloudprovider.LoadBalancer, bool) { // Instances returns an implementation of Instances for CloudStack. func (cs *CSCloud) Instances() (cloudprovider.Instances, bool) { - if cs.client == nil { - return nil, false - } - - return cs, true + return nil, false } +// InstancesV2 is an implementation for instances and should only be implemented by external cloud providers. +// Implementing InstancesV2 is behaviorally identical to Instances but is optimized to significantly reduce +// API calls to the cloud provider when registering and syncing nodes. Implementation of this interface will +// disable calls to the Zones interface. Also returns true if the interface is supported, false otherwise. func (cs *CSCloud) InstancesV2() (cloudprovider.InstancesV2, bool) { if cs.client == nil { return nil, false @@ -130,30 +126,16 @@ func (cs *CSCloud) InstancesV2() (cloudprovider.InstancesV2, bool) { // Zones returns an implementation of Zones for CloudStack. func (cs *CSCloud) Zones() (cloudprovider.Zones, bool) { - if cs.client == nil { - return nil, false - } - - return cs, true + return nil, false } // Clusters returns an implementation of Clusters for CloudStack. func (cs *CSCloud) Clusters() (cloudprovider.Clusters, bool) { - if cs.client == nil { - return nil, false - } - - klog.Warning("This cloud provider doesn't support clusters") return nil, false } // Routes returns an implementation of Routes for CloudStack. func (cs *CSCloud) Routes() (cloudprovider.Routes, bool) { - if cs.client == nil { - return nil, false - } - - klog.Warning("This cloud provider doesn't support routes") return nil, false } @@ -166,80 +148,3 @@ func (cs *CSCloud) ProviderName() string { func (cs *CSCloud) HasClusterID() bool { return true } - -// GetZone returns the Zone containing the region that the program is running in. -func (cs *CSCloud) GetZone(ctx context.Context) (cloudprovider.Zone, error) { - zone := cloudprovider.Zone{} - - if cs.zone == "" { - hostname, err := os.Hostname() - if err != nil { - return zone, fmt.Errorf("failed to get hostname for retrieving the zone: %v", err) - } - - instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName(hostname) - if err != nil { - if count == 0 { - return zone, fmt.Errorf("could not find instance for retrieving the zone: %v", err) - } - return zone, fmt.Errorf("error getting instance for retrieving the zone: %v", err) - } - - cs.zone = instance.Zonename - } - - klog.V(2).Infof("Current zone is %v", cs.zone) - zone.FailureDomain = cs.zone - zone.Region = cs.zone - - return zone, nil -} - -// GetZoneByProviderID returns the Zone, found by using the provider ID. -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( - id, - cloudstack.WithProject(cs.projectID), - ) - if err != nil { - if count == 0 { - return zone, fmt.Errorf("could not find node by ID: %v", id) - } - return zone, fmt.Errorf("error retrieving zone: %v", err) - } - - klog.V(2).Infof("Current zone is %v", cs.zone) - zone.FailureDomain = instance.Zonename - zone.Region = instance.Zonename - - return zone, nil -} - -// GetZoneByNodeName returns the Zone, found by using the node name. -func (cs *CSCloud) GetZoneByNodeName(ctx context.Context, nodeName types.NodeName) (cloudprovider.Zone, error) { - zone := cloudprovider.Zone{} - - instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName( - string(nodeName), - cloudstack.WithProject(cs.projectID), - ) - if err != nil { - if count == 0 { - return zone, fmt.Errorf("could not find node: %v", nodeName) - } - return zone, fmt.Errorf("error retrieving zone: %v", err) - } - - klog.V(2).Infof("Current zone is %v", cs.zone) - zone.FailureDomain = instance.Zonename - zone.Region = instance.Zonename - - return zone, nil -} diff --git a/cloudstack_instances.go b/cloudstack_instances.go index f3406007..88f0cf69 100644 --- a/cloudstack_instances.go +++ b/cloudstack_instances.go @@ -26,47 +26,10 @@ import ( "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" ) -// 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( - string(name), - cloudstack.WithProject(cs.projectID), - ) - if err != nil { - if count == 0 { - return nil, cloudprovider.InstanceNotFound - } - return nil, fmt.Errorf("error retrieving node addresses: %v", err) - } - - return cs.nodeAddresses(instance) -} - -// 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( - id, - cloudstack.WithProject(cs.projectID), - ) - if err != nil { - if count == 0 { - return nil, cloudprovider.InstanceNotFound - } - return nil, fmt.Errorf("error retrieving node addresses: %v", err) - } - - return cs.nodeAddresses(instance) -} - func (cs *CSCloud) nodeAddresses(instance *cloudstack.VirtualMachine) ([]corev1.NodeAddress, error) { if len(instance.Nic) == 0 { return nil, errors.New("instance does not have an internal IP") @@ -87,128 +50,77 @@ func (cs *CSCloud) nodeAddresses(instance *cloudstack.VirtualMachine) ([]corev1. return addresses, nil } -// InstanceID returns the cloud provider ID of the specified instance. -func (cs *CSCloud) InstanceID(ctx context.Context, name types.NodeName) (string, error) { - instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName( - string(name), - cloudstack.WithProject(cs.projectID), - ) - if err != nil { - if count == 0 { - return "", cloudprovider.InstanceNotFound - } - return "", fmt.Errorf("error retrieving instance ID: %v", err) - } - - // return instance ID with empty region - // in the future, with region support, this should be / - return "/" + instance.Id, nil -} - -// InstanceType returns the type of the specified instance. -func (cs *CSCloud) InstanceType(ctx context.Context, name types.NodeName) (string, error) { - instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName( - string(name), - cloudstack.WithProject(cs.projectID), - ) - if err != nil { - if count == 0 { - return "", cloudprovider.InstanceNotFound - } - return "", fmt.Errorf("error retrieving instance type: %v", err) - } - - return sanitizeLabel(instance.Serviceofferingname), nil -} +func (cs *CSCloud) InstanceExists(ctx context.Context, node *corev1.Node) (bool, error) { + _, err := cs.getInstance(ctx, node) -// 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 + if err == cloudprovider.InstanceNotFound { + klog.V(5).Infof("instance not found for node: %s", node.Name) + return false, nil } - instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByID( - id, - cloudstack.WithProject(cs.projectID), - ) - if err != nil { - if count == 0 { - return "", cloudprovider.InstanceNotFound - } - return "", fmt.Errorf("error retrieving instance type: %v", err) - } - - return sanitizeLabel(instance.Serviceofferingname), nil -} - -// AddSSHKeyToAllInstances is currently not implemented. -func (cs *CSCloud) AddSSHKeyToAllInstances(ctx context.Context, user string, keyData []byte) error { - return cloudprovider.NotImplemented -} - -// CurrentNodeName returns the name of the node we are currently running on. -func (cs *CSCloud) CurrentNodeName(ctx context.Context, hostname string) (types.NodeName, error) { - return types.NodeName(hostname), nil -} - -// 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( - id, - cloudstack.WithProject(cs.projectID), - ) - if err != nil { - if count == 0 { - return false, nil - } - return false, fmt.Errorf("error retrieving instance: %v", err) - } - return true, nil } -// InstanceShutdownByProviderID returns true if the instance is in safe state to detach volumes -func (cs *CSCloud) InstanceShutdownByProviderID(ctx context.Context, providerID string) (bool, error) { - id, _, err := instanceIDFromProviderID(providerID) +func (cs *CSCloud) InstanceShutdown(ctx context.Context, node *corev1.Node) (bool, error) { + instance, err := cs.getInstance(ctx, node) 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) { - nodeName := types.NodeName(node.Name) - providerID, err := cs.InstanceID(ctx, nodeName) +func (cs *CSCloud) InstanceMetadata(ctx context.Context, node *corev1.Node) (*cloudprovider.InstanceMetadata, error) { + instance, err := cs.getInstance(ctx, node) if err != nil { - return false, err + return nil, err } - return cs.InstanceExistsByProviderID(ctx, providerID) + addresses, err := cs.nodeAddresses(instance) + if err != nil { + return nil, err + } + + return &cloudprovider.InstanceMetadata{ + ProviderID: getInstanceProviderID(instance), + InstanceType: sanitizeLabel(instance.Serviceofferingname), + NodeAddresses: addresses, + Zone: sanitizeLabel(instance.Zonename), + Region: "", + }, nil } -func (cs *CSCloud) InstanceShutdown(ctx context.Context, node *corev1.Node) (bool, error) { - return cs.InstanceShutdownByProviderID(ctx, node.Spec.ProviderID) +func getInstanceProviderID(instance *cloudstack.VirtualMachine) string { + // TODO: implement region + return fmt.Sprintf("%s:///%s", ProviderName, instance.Id) } -func (cs *CSCloud) InstanceMetadata(ctx context.Context, node *corev1.Node) (*cloudprovider.InstanceMetadata, error) { - id, region, err := instanceIDFromProviderID(node.Spec.ProviderID) +func (cs *CSCloud) getInstance(ctx context.Context, node *corev1.Node) (*cloudstack.VirtualMachine, error) { + if node.Spec.ProviderID == "" { + var err error + klog.V(4).Infof("looking for node by node name %v", node.Name) + instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName( + node.Name, + cloudstack.WithProject(cs.projectID), + ) + if err != nil { + if count == 0 { + return nil, cloudprovider.InstanceNotFound + } + if count > 1 { + return nil, fmt.Errorf("getInstance: multiple instances found") + } + return nil, fmt.Errorf("getInstance: error retrieving instance by name: %v", err) + } + return instance, nil + } + + klog.V(4).Infof("looking for node by provider ID %v", node.Spec.ProviderID) + id, _, err := instanceIDFromProviderID(node.Spec.ProviderID) if err != nil { return nil, err } @@ -221,19 +133,11 @@ func (cs *CSCloud) InstanceMetadata(ctx context.Context, node *corev1.Node) (*cl if count == 0 { return nil, cloudprovider.InstanceNotFound } - return nil, fmt.Errorf("error retrieving instance: %v", err) - } - - addresses, err := cs.nodeAddresses(instance) - if err != nil { - return nil, err + if count > 1 { + return nil, fmt.Errorf("getInstance: multiple instances found") + } + return nil, fmt.Errorf("error retrieving instance by provider ID: %v", err) } - return &cloudprovider.InstanceMetadata{ - ProviderID: node.Spec.ProviderID, - InstanceType: sanitizeLabel(instance.Serviceofferingname), - NodeAddresses: addresses, - Zone: sanitizeLabel(instance.Zonename), - Region: region, - }, nil + return instance, nil } From 77820581e284ae76083bcd2ccd31565df8886ea6 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 14 Jul 2023 17:08:32 +0200 Subject: [PATCH 08/10] Staticcheck fixes --- cloudstack_loadbalancer.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/cloudstack_loadbalancer.go b/cloudstack_loadbalancer.go index 6bb80492..c893d823 100644 --- a/cloudstack_loadbalancer.go +++ b/cloudstack_loadbalancer.go @@ -193,11 +193,11 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s for _, lbRule := range lb.rules { protocol := ProtocolFromLoadBalancer(lbRule.Protocol) if protocol == LoadBalancerProtocolInvalid { - return nil, fmt.Errorf("Error parsing protocol %v: %v", lbRule.Protocol, err) + return nil, fmt.Errorf("error parsing protocol %v: %v", lbRule.Protocol, err) } port, err := strconv.ParseInt(lbRule.Publicport, 10, 32) if err != nil { - return nil, fmt.Errorf("Error parsing port %s: %v", lbRule.Publicport, err) + return nil, fmt.Errorf("error parsing port %s: %v", lbRule.Publicport, err) } klog.V(4).Infof("Deleting firewall rules associated with load balancer rule: %v (%v:%v:%v)", lbRule.Name, protocol, lbRule.Publicip, port) @@ -656,10 +656,7 @@ func compareStringSlice(x, y []string) bool { delete(diff, _y) } } - if len(diff) == 0 { - return true - } - return false + return len(diff) == 0 } func ruleToString(rule *cloudstack.FirewallRule) string { @@ -698,7 +695,7 @@ func rulesToString(rules []*cloudstack.FirewallRule) string { func rulesMapToString(rules map[*cloudstack.FirewallRule]bool) string { ls := &strings.Builder{} first := true - for rule, _ := range rules { + for rule := range rules { if first { first = false } else { @@ -739,7 +736,6 @@ func (lb *loadBalancer) updateFirewallRule(publicIpId string, publicPort int, pr for _, rule := range r.FirewallRules { if rule.Protocol == protocol.IPProtocol() && rule.Startport == publicPort && rule.Endport == publicPort { filtered[rule] = true - } else { } } klog.V(4).Infof("Matching rules for %v: %v", lb.ipAddr, rulesMapToString(filtered)) From bfa3055e732cbe451923011b4be47f8614888257 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Wed, 19 Jul 2023 11:21:07 +0200 Subject: [PATCH 09/10] update comment --- util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util.go b/util.go index d7c3d0c1..ee4f199a 100644 --- a/util.go +++ b/util.go @@ -28,7 +28,7 @@ import ( // 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. +// instanceIDFromProviderID splits a provider's id and returns instanceID and region. // A providerID is build out of '${ProviderName}:///${instance-id}' which contains ':///'. // or '${ProviderName}://${region}/${instance-id}' which contains '://'. // See cloudprovider.GetInstanceProviderID and Instances.InstanceID. From e37253b46eee59277ab06be786c175f9bacdf45e Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Wed, 19 Jul 2023 11:21:25 +0200 Subject: [PATCH 10/10] Add tests --- cloudstack_instances_test.go | 389 +++++++++++++++++++++++++++++++++++ go.mod | 6 +- go.sum | 3 + 3 files changed, 396 insertions(+), 2 deletions(-) create mode 100644 cloudstack_instances_test.go diff --git a/cloudstack_instances_test.go b/cloudstack_instances_test.go new file mode 100644 index 00000000..3b726153 --- /dev/null +++ b/cloudstack_instances_test.go @@ -0,0 +1,389 @@ +package cloudstack + +import ( + "context" + "fmt" + "testing" + + "github.com/apache/cloudstack-go/v2/cloudstack" + "github.com/golang/mock/gomock" + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + cloudprovider "k8s.io/cloud-provider" +) + +func makeInstance(instanceID, privateIP, publicIP string, stateName string) *cloudstack.VirtualMachine { + instance := cloudstack.VirtualMachine{ + Id: instanceID, + Name: "testDummyVM", + Displayname: "testDummyVM", + State: stateName, + Zoneid: "1d8d87d4-1425-459c-8d81-c6f57dca2bd2", + Zonename: "shouldwork", + Hostname: "SimulatedAgent.4234d24b-37fd-42bf-9184-874889001baf", + Serviceofferingid: "498ce071-a077-4237-9492-e55c42553327", + Serviceofferingname: "Very small instance", + Publicip: publicIP, + Nic: []cloudstack.Nic{ + { + Id: "47d79da1-2fe1-4a44-a503-523055714a72", + Ipaddress: privateIP, + }, + }, + Instancename: "i-2-683-QA", + } + + return &instance +} + +func makeNode(nodeName string) *corev1.Node { + providerID := "cloudstack:///915653c4-298b-4d74-bdee-4ced282114f1" + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + Spec: corev1.NodeSpec{ + ProviderID: providerID, + }, + } +} + +func makeNodeWithoutProviderID(nodeName string) *corev1.Node { + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + } +} + +func TestGetInstanceProviderID(t *testing.T) { + testCases := []struct { + name string + instance *cloudstack.VirtualMachine + providerID string + }{ + { + name: "get instance (with public IP) provider ID", + instance: makeInstance("2fda7aaa-24df-11ee-be56-0242ac120002", "192.168.0.1", "1.2.3.4", "Running"), + providerID: "cloudstack:///2fda7aaa-24df-11ee-be56-0242ac120002", + }, + { + name: "get instance (without public IP) provider ID", + instance: makeInstance("89e5fdbc-24df-11ee-be56-0242ac120002", "192.168.0.2", "", "Running"), + providerID: "cloudstack:///89e5fdbc-24df-11ee-be56-0242ac120002", + }, + { + name: "get instance (without private IP) provider ID", + instance: makeInstance("92794b3c-24df-11ee-be56-0242ac120002", "", "1.2.3.4", "Running"), + providerID: "cloudstack:///92794b3c-24df-11ee-be56-0242ac120002", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + providerID := getInstanceProviderID(testCase.instance) + assert.Equal(t, testCase.providerID, providerID) + }) + } +} + +func TestInstanceExists(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + cs := cloudstack.NewMockClient(mockCtrl) + ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) + + fakeInstances := &CSCloud{ + client: cs, + } + + nodeName := "testDummyVM" + + tests := []struct { + name string + node *corev1.Node + mockedCSOutput *cloudstack.VirtualMachine + expectedResult bool + }{ + { + name: "test InstanceExists with running instance", + node: makeNode(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.4", "Running"), + expectedResult: true, + }, + { + name: "test InstanceExists with stopped instance", + node: makeNode(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.4", "Stopped"), + expectedResult: true, + }, + { + name: "test InstanceExists with destroyed instance (node without providerID)", + node: makeNodeWithoutProviderID(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.4", "Destroyed"), + expectedResult: true, + }, + { + name: "test InstanceExists with non existent node without providerID", + node: makeNodeWithoutProviderID("nonExistingVM"), + mockedCSOutput: nil, + expectedResult: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // t.Logf("test node: %v", test.node) + if test.node.Spec.ProviderID == "" { + if test.node.Name == "testDummyVM" { + ms.EXPECT().GetVirtualMachineByName("testDummyVM", gomock.Any()).Return(test.mockedCSOutput, 1, nil) + } else { + ms.EXPECT().GetVirtualMachineByName("nonExistingVM", gomock.Any()).Return(test.mockedCSOutput, 0, fmt.Errorf("No match found for ...")) + } + } else { + ms.EXPECT().GetVirtualMachineByID("915653c4-298b-4d74-bdee-4ced282114f1", gomock.Any()).Return(test.mockedCSOutput, 1, nil) + } + + exists, err := fakeInstances.InstanceExists(context.TODO(), test.node) + + if err != nil { + t.Errorf("InstanceExists failed with node %v: %v", nodeName, err) + } + + if exists != test.expectedResult { + t.Errorf("unexpected result, InstanceExists should return %v", test.expectedResult) + } + }) + } +} + +func TestInstanceShutdown(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + cs := cloudstack.NewMockClient(mockCtrl) + ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) + + fakeInstances := &CSCloud{ + client: cs, + } + + nodeName := "testDummyVM" + + tests := []struct { + name string + node *corev1.Node + mockedCSOutput *cloudstack.VirtualMachine + expectedResult bool + }{ + { + name: "test InstanceShutdown with running instance", + node: makeNode(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.6", "Running"), + expectedResult: false, + }, + { + name: "test InstanceShutdown with running instance (node without providerID)", + node: makeNodeWithoutProviderID(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.6", "Running"), + expectedResult: false, + }, + { + name: "test InstanceShutdown with stopped instance", + node: makeNode(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.6", "Stopped"), + expectedResult: true, + }, + { + name: "test InstanceShutdown with stopped instance (node without providerID)", + node: makeNodeWithoutProviderID(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.6", "Stopped"), + expectedResult: true, + }, + { + name: "test InstanceShutdown with destroyed instance", + node: makeNode(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.6", "Destroyed"), + expectedResult: false, + }, + { + name: "test InstanceShutdown with terminated instance (node without providerID)", + node: makeNodeWithoutProviderID(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.6", "Destroyed"), + expectedResult: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if test.node.Spec.ProviderID == "" { + ms.EXPECT().GetVirtualMachineByName("testDummyVM", gomock.Any()).Return(test.mockedCSOutput, 1, nil) + } else { + ms.EXPECT().GetVirtualMachineByID("915653c4-298b-4d74-bdee-4ced282114f1", gomock.Any()).Return(test.mockedCSOutput, 1, nil) + } + + shutdown, err := fakeInstances.InstanceShutdown(context.TODO(), test.node) + + if err != nil { + t.Logf("InstanceShutdown failed with node %v: %v", nodeName, err) + } + + if shutdown != test.expectedResult { + t.Errorf("unexpected result, InstanceShutdown should return %v", test.expectedResult) + } + }) + } +} + +func TestInstanceMetadata(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + cs := cloudstack.NewMockClient(mockCtrl) + ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) + + fakeInstances := &CSCloud{ + client: cs, + } + + nodeName := "testDummyVM" + + tests := []struct { + name string + node *corev1.Node + expectedResult *cloudprovider.InstanceMetadata + mockedCSOutput *cloudstack.VirtualMachine + }{ + { + name: "test InstanceMetadata with running instance", + node: makeNode(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.6", "Running"), + expectedResult: &cloudprovider.InstanceMetadata{ + ProviderID: "cloudstack:///915653c4-298b-4d74-bdee-4ced282114f1", + InstanceType: "Very_small_instance", + NodeAddresses: []corev1.NodeAddress{ + { + Type: "InternalIP", + Address: "192.168.0.1", + }, + { + Type: "Hostname", + Address: "SimulatedAgent.4234d24b-37fd-42bf-9184-874889001baf", + }, + { + Type: "ExternalIP", + Address: "1.2.3.6", + }, + }, + Zone: "shouldwork", + }, + }, + { + name: "test InstanceMetadata with running instance (node without providerID)", + node: makeNodeWithoutProviderID(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "1.2.3.6", "Running"), + expectedResult: &cloudprovider.InstanceMetadata{ + ProviderID: "cloudstack:///915653c4-298b-4d74-bdee-4ced282114f1", + InstanceType: "Very_small_instance", + NodeAddresses: []corev1.NodeAddress{ + { + Type: "InternalIP", + Address: "192.168.0.1", + }, + { + Type: "Hostname", + Address: "SimulatedAgent.4234d24b-37fd-42bf-9184-874889001baf", + }, + { + Type: "ExternalIP", + Address: "1.2.3.6", + }, + }, + Zone: "shouldwork", + }, + }, + { + name: "test InstanceMetadata with stopped instance", + node: makeNode(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "", "Stopped"), + expectedResult: &cloudprovider.InstanceMetadata{ + ProviderID: "cloudstack:///915653c4-298b-4d74-bdee-4ced282114f1", + InstanceType: "Very_small_instance", + NodeAddresses: []corev1.NodeAddress{ + { + Type: "InternalIP", + Address: "192.168.0.1", + }, + { + Type: "Hostname", + Address: "SimulatedAgent.4234d24b-37fd-42bf-9184-874889001baf", + }, + }, + Zone: "shouldwork", + }, + }, + { + name: "test InstanceMetadata with destroyed instance", + node: makeNode(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "", "Destroyed"), + expectedResult: &cloudprovider.InstanceMetadata{ + ProviderID: "cloudstack:///915653c4-298b-4d74-bdee-4ced282114f1", + InstanceType: "Very_small_instance", + NodeAddresses: []corev1.NodeAddress{ + { + Type: "InternalIP", + Address: "192.168.0.1", + }, + { + Type: "Hostname", + Address: "SimulatedAgent.4234d24b-37fd-42bf-9184-874889001baf", + }, + }, + Zone: "shouldwork", + }, + }, + { + name: "test InstanceMetadata with destroyed instance (node without providerID)", + node: makeNodeWithoutProviderID(nodeName), + mockedCSOutput: makeInstance("915653c4-298b-4d74-bdee-4ced282114f1", "192.168.0.1", "", "Destroyed"), + expectedResult: &cloudprovider.InstanceMetadata{ + ProviderID: "cloudstack:///915653c4-298b-4d74-bdee-4ced282114f1", + InstanceType: "Very_small_instance", + NodeAddresses: []corev1.NodeAddress{ + { + Type: "InternalIP", + Address: "192.168.0.1", + }, + { + Type: "Hostname", + Address: "SimulatedAgent.4234d24b-37fd-42bf-9184-874889001baf", + }, + }, + Zone: "shouldwork", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if test.node.Spec.ProviderID == "" { + ms.EXPECT().GetVirtualMachineByName("testDummyVM", gomock.Any()).Return(test.mockedCSOutput, 1, nil) + } else { + ms.EXPECT().GetVirtualMachineByID("915653c4-298b-4d74-bdee-4ced282114f1", gomock.Any()).Return(test.mockedCSOutput, 1, nil) + } + + metadata, err := fakeInstances.InstanceMetadata(context.TODO(), test.node) + + if err != nil { + t.Logf("InstanceMetadata failed with node %v: %v", nodeName, err) + } + + if !cmp.Equal(metadata, test.expectedResult) { + t.Errorf("unexpected metadata %v, InstanceMetadata should return %v", metadata, test.expectedResult) + } + }) + } +} diff --git a/go.mod b/go.mod index 5bbe6a6e..7c3dd9f8 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,10 @@ go 1.19 require ( github.com/apache/cloudstack-go/v2 v2.15.0 + github.com/golang/mock v1.6.0 + github.com/google/go-cmp v0.5.9 github.com/spf13/pflag v1.0.5 + github.com/stretchr/testify v1.8.0 gopkg.in/gcfg.v1 v1.2.3 k8s.io/api v0.24.12 k8s.io/apimachinery v0.24.12 @@ -34,10 +37,8 @@ require ( github.com/go-openapi/swag v0.19.14 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect - github.com/golang/mock v1.6.0 // indirect github.com/golang/protobuf v1.5.2 // indirect github.com/google/gnostic v0.5.7-v3refs // indirect - github.com/google/go-cmp v0.5.9 // indirect github.com/google/gofuzz v1.1.0 // indirect github.com/google/uuid v1.1.2 // indirect github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect @@ -53,6 +54,7 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pkg/errors v0.9.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_golang v1.14.0 // indirect github.com/prometheus/client_model v0.3.0 // indirect github.com/prometheus/common v0.37.0 // indirect diff --git a/go.sum b/go.sum index 6c3bd3f8..bd144e1e 100644 --- a/go.sum +++ b/go.sum @@ -325,13 +325,16 @@ github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An github.com/stoewer/go-strcase v1.2.0/go.mod h1:IBiWB2sKIp3wVVQ3Y035++gc+knqhUQag1KpM8ahLw8= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/tmc/grpc-websocket-proxy v0.0.0-20201229170055-e5319fda7802 h1:uruHq4dN7GR16kFc5fp3d1RIYzJW5onx8Ybykw2YQFA= github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2 h1:eY9dn8+vbi4tKz5Qo6v2eYzo7kUS51QINcR5jNpbZS8= go.etcd.io/bbolt v1.3.6 h1:/ecaJf0sk1l4l6V4awd65v2C3ILy7MSj+s/x1ADCIMU=