Fix providerID & getZone Implementation #85
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #58 by correcting the providerID format and improving the GetZone implementation for CloudStack cloud provider integration with Kubernetes. The changes address two key problems: (1) providerID was being set to just the provider name instead of the full "provider://instanceID" format, and (2) GetZone was using os.Hostname() which returns the pod name in containerized environments rather than the actual node name.
- Added helper functions to convert between provider IDs and instance IDs in the correct "provider://instanceID" format
- Modified GetZone to query the Kubernetes API for the node name when the cloud controller manager runs in a pod
- Added RBAC permissions for the cloud controller manager to read pod information
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| deployment.yaml | Adds RBAC permissions (list, get) for pods resource to allow the cloud controller manager to query pod information |
| cloudstack_instances.go | Implements helper functions for providerID format conversion and updates all *ByProviderID methods to use the correct instance ID extraction |
| cloudstack.go | Adds clientBuilder field, implements getNodeNameFromPod to retrieve node name from pod spec via Kubernetes API, and updates GetZone to use this new method |
Comments suppressed due to low confidence (1)
cloudstack.go:196
- The
cs.zonefield is being read and written without synchronization in theGetZonemethod. IfGetZoneis called concurrently from multiple goroutines whencs.zoneis empty, there's a race condition where multiple goroutines could simultaneously query the CloudStack API and overwritecs.zone. Consider using a sync.Once or mutex to ensure thread-safe initialization of the zone field.
if cs.zone == "" {
// In Kubernetes pods, os.Hostname() returns the pod name, not the node hostname.
// We need to get the node name from the pod's spec.nodeName using the Kubernetes API.
nodeName, err := cs.getNodeNameFromPod(ctx)
if err != nil {
return zone, fmt.Errorf("failed to get node name for retrieving the zone: %v", err)
}
instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName(
nodeName,
cloudstack.WithProject(cs.projectID),
)
if err != nil {
if count == 0 {
return zone, fmt.Errorf("could not find CloudStack instance with name %s for retrieving the zone: %v", nodeName, err)
}
return zone, fmt.Errorf("error getting instance for retrieving the zone: %v", err)
}
cs.zone = instance.Zonename
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parts := strings.Split(providerID, "://") | ||
| if len(parts) == 1 { | ||
| return providerID | ||
| } | ||
| return parts[1] |
There was a problem hiding this comment.
The getInstanceIDFromProviderID function could produce incorrect results if the providerID contains multiple "://" separators. The current implementation using strings.Split would create more than 2 parts, and parts[1] would only return the segment between the first and second "://" rather than everything after the first one. Use strings.SplitN(providerID, "://", 2) instead to split only on the first occurrence of "://".
| if data, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil { | ||
| namespace = string(data) |
There was a problem hiding this comment.
The namespace trimming logic doesn't handle potential whitespace. When reading from /var/run/secrets/kubernetes.io/serviceaccount/namespace, the data might include trailing newlines or whitespace. Consider using strings.TrimSpace(string(data)) instead of just string(data) to ensure clean namespace values.
| func (cs *CSCloud) getNodeNameFromPod(ctx context.Context) (string, error) { | ||
| if cs.clientBuilder == nil { | ||
| return "", fmt.Errorf("clientBuilder not initialized, cannot query Kubernetes API") | ||
| } | ||
|
|
||
| client, err := cs.clientBuilder.Client("cloud-controller-manager") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get Kubernetes client: %v", err) | ||
| } | ||
|
|
||
| // Get pod name and namespace | ||
| // In Kubernetes, the pod name is available as HOSTNAME environment variable | ||
| // or we can use os.Hostname() which returns the pod name | ||
| podName := os.Getenv("HOSTNAME") | ||
| if podName == "" { | ||
| var err error | ||
| podName, err = os.Hostname() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get pod name: %v", err) | ||
| } | ||
| } | ||
|
|
||
| // Get namespace from environment variable or default to kube-system for CCM | ||
| namespace := os.Getenv("POD_NAMESPACE") | ||
| if namespace == "" { | ||
| // Try reading from service account namespace file (available in pods) | ||
| if data, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil { | ||
| namespace = string(data) | ||
| } else { | ||
| // Default namespace for cloud controller manager | ||
| namespace = "kube-system" | ||
| } | ||
| } | ||
|
|
||
| // Get the pod object from Kubernetes API | ||
| pod, err := client.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get pod %s/%s from Kubernetes API: %v", namespace, podName, err) | ||
| } | ||
|
|
||
| if pod.Spec.NodeName == "" { | ||
| return "", fmt.Errorf("pod %s/%s does not have a nodeName assigned yet", namespace, podName) | ||
| } | ||
|
|
||
| klog.V(4).Infof("found node name %s for pod %s/%s", pod.Spec.NodeName, namespace, podName) | ||
| return pod.Spec.NodeName, nil | ||
| } |
There was a problem hiding this comment.
The new getNodeNameFromPod function, which queries the Kubernetes API to get pod information, lacks test coverage. Since other parts of this codebase have test coverage (see cloudstack_test.go), this new functionality should also include tests to verify correct behavior in various scenarios (e.g., missing HOSTNAME env var, missing namespace file, pod not scheduled yet).
| func (cs *CSCloud) getProviderIDFromInstanceID(instanceID string) string { | ||
| return fmt.Sprintf("%s://%s", cs.ProviderName(), instanceID) | ||
| } | ||
|
|
||
| func (cs *CSCloud) getInstanceIDFromProviderID(providerID string) string { | ||
| parts := strings.Split(providerID, "://") | ||
| if len(parts) == 1 { | ||
| return providerID | ||
| } | ||
| return parts[1] | ||
| } |
There was a problem hiding this comment.
The new helper functions getProviderIDFromInstanceID and getInstanceIDFromProviderID lack test coverage. These are critical functions for parsing and constructing provider IDs. Since the codebase has existing test coverage (see cloudstack_test.go), these utilities should include unit tests to verify correct behavior with various input formats (e.g., with "://", without "://", empty strings, etc.).
| func (cs *CSCloud) getProviderIDFromInstanceID(instanceID string) string { | ||
| return fmt.Sprintf("%s://%s", cs.ProviderName(), instanceID) | ||
| } | ||
|
|
||
| func (cs *CSCloud) getInstanceIDFromProviderID(providerID string) string { | ||
| parts := strings.Split(providerID, "://") | ||
| if len(parts) == 1 { | ||
| return providerID | ||
| } | ||
| return parts[1] | ||
| } |
There was a problem hiding this comment.
The new helper functions getProviderIDFromInstanceID and getInstanceIDFromProviderID are missing documentation comments. Following Go conventions, exported or internal helper functions should have comments explaining their purpose, parameters, and return values. Add documentation comments to clarify what these functions do and their expected input/output formats.
1 participant
Fix #58