Copilot commented on code in PR #82:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/82#discussion_r2559828323
##########
cloudstack_loadbalancer.go:
##########
@@ -127,6 +133,14 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context,
clusterName string, s
}
}(lb)
}
+
+ // If the controller associated the IP, set the annotation to
persist this information.
+ if lb.ipAssociatedByController && lb.ipAddr ==
service.Spec.LoadBalancerIP {
+ if err := cs.setServiceAnnotation(ctx, service,
ServiceAnnotationLoadBalancerIPAssociatedByController, "true"); err != nil {
+ // Log the error but don't fail - the
annotation is helpful but not critical
+ klog.Warningf("Failed to set annotation on
service %s/%s: %v", service.Namespace, service.Name, err)
+ }
+ }
Review Comment:
[nitpick] The condition `lb.ipAddr == service.Spec.LoadBalancerIP` is subtle
and could benefit from a clarifying comment. This condition is true only when
the user specified a LoadBalancerIP that the controller then associated. When
the user doesn't specify an IP (empty string), the condition is false, which is
correct because the deletion logic at line 372 already handles releasing
controller-allocated IPs when they don't match the spec. Consider adding a
comment explaining this behavior to make the intent clear.
##########
cloudstack_loadbalancer.go:
##########
@@ -353,10 +367,50 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx
context.Context, clusterName st
}
}
- if lb.ipAddr != "" && lb.ipAddr != service.Spec.LoadBalancerIP {
- klog.V(4).Infof("Releasing load balancer IP: %v", lb.ipAddr)
- if err := lb.releaseLoadBalancerIP(); err != nil {
- return err
+ if lb.ipAddr != "" {
+ // If the IP was allocated by the controller (not specified in
service spec), release it.
+ if lb.ipAddr != service.Spec.LoadBalancerIP {
+ klog.V(4).Infof("Releasing load balancer IP: %v",
lb.ipAddr)
+ if err := lb.releaseLoadBalancerIP(); err != nil {
+ return err
+ }
+ } else {
+ // If the IP was specified in service spec, check if it
was associated by the controller.
+ // First, check if there's an annotation indicating the
controller associated it.
+ // If not, check if there are any other load balancer
rules using this IP.
+ shouldDisassociate :=
getBoolFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerIPAssociatedByController, false)
+
+ if shouldDisassociate {
+ // Annotation is set, so check if there are any
other load balancer rules using this IP.
+ // Since we've already deleted all rules for
this service, any remaining rules must belong
+ // to other services. If no other rules exist,
it's safe to disassociate the IP.
+ ip, count, err :=
lb.Address.GetPublicIpAddressByID(lb.ipAddrID)
+ if err != nil {
+ klog.Errorf("Error retrieving IP
address %v for disassociation check: %v", lb.ipAddr, err)
+ } else if count > 0 && ip.Allocated != "" {
+ p :=
lb.LoadBalancer.NewListLoadBalancerRulesParams()
+ p.SetPublicipid(lb.ipAddrID)
+ p.SetListall(true)
+ if lb.projectID != "" {
+ p.SetProjectid(lb.projectID)
+ }
+ otherRules, err :=
lb.LoadBalancer.ListLoadBalancerRules(p)
+ if err != nil {
+ klog.Errorf("Error checking for
other load balancer rules using IP %v: %v", lb.ipAddr, err)
+ } else if otherRules.Count > 0 {
+ // Other load balancer rules
are using this IP (other services are using it),
+ // so don't disassociate.
+ shouldDisassociate = false
+ }
+ }
Review Comment:
The error handling here silently continues even when critical operations
fail. If `GetPublicIpAddressByID` fails (line 387-389) or
`ListLoadBalancerRules` fails (line 397-399), the errors are only logged but
`shouldDisassociate` remains `true`. This means if there's a transient error
checking for other rules, the IP could be incorrectly disassociated even though
other services might be using it. Consider setting `shouldDisassociate = false`
when these error cases occur to fail safe and prevent unintended IP
disassociation.
##########
cloudstack.go:
##########
@@ -238,3 +243,48 @@ func (cs *CSCloud) GetZoneByNodeName(ctx context.Context,
nodeName types.NodeNam
return zone, nil
}
+
+// setServiceAnnotation updates a service annotation using the Kubernetes
client.
+func (cs *CSCloud) setServiceAnnotation(ctx context.Context, service
*corev1.Service, key, value string) error {
+ if cs.clientBuilder == nil {
+ klog.V(4).Infof("Client builder not available, skipping
annotation update for service %s/%s", service.Namespace, service.Name)
+ return nil
+ }
+
+ client, err := cs.clientBuilder.Client("cloud-controller-manager")
+ if err != nil {
+ return fmt.Errorf("failed to get Kubernetes client: %v", err)
+ }
+
+ // Get the current service
+ svc, err := client.CoreV1().Services(service.Namespace).Get(ctx,
service.Name, metav1.GetOptions{})
+ if err != nil {
+ if apierrors.IsNotFound(err) {
+ klog.V(4).Infof("Service %s/%s not found, skipping
annotation update", service.Namespace, service.Name)
+ return nil
+ }
+ return fmt.Errorf("failed to get service: %v", err)
+ }
+
+ // Check if annotation already has the correct value
+ if svc.Annotations != nil {
+ if existingValue, exists := svc.Annotations[key]; exists &&
existingValue == value {
+ klog.V(4).Infof("Annotation %s already set to %s for
service %s/%s", key, value, service.Namespace, service.Name)
+ return nil
+ }
+ }
+
+ // Update the annotation
+ if svc.Annotations == nil {
+ svc.Annotations = make(map[string]string)
+ }
+ svc.Annotations[key] = value
+
+ _, err = client.CoreV1().Services(service.Namespace).Update(ctx, svc,
metav1.UpdateOptions{})
+ if err != nil {
+ return fmt.Errorf("failed to update service annotation: %v",
err)
+ }
+
+ klog.V(4).Infof("Successfully set annotation %s=%s on service %s/%s",
key, value, service.Namespace, service.Name)
+ return nil
Review Comment:
The `setServiceAnnotation` function uses a read-modify-write pattern without
optimistic locking (like `resourceVersion`), which could lead to race
conditions if multiple controllers or operations are updating the service
simultaneously. Consider using a patch operation instead of Get-Update, or
implement retry logic with conflict detection to handle concurrent updates
safely.
##########
cloudstack_loadbalancer.go:
##########
@@ -503,12 +558,16 @@ func (lb *loadBalancer) getPublicIPAddress(loadBalancerIP
string) error {
}
if l.Count != 1 {
- return fmt.Errorf("could not find IP address %v",
loadBalancerIP)
+ return fmt.Errorf("could not find IP address %v. Found %d
addresses", loadBalancerIP, l.Count)
}
lb.ipAddr = l.PublicIpAddresses[0].Ipaddress
lb.ipAddrID = l.PublicIpAddresses[0].Id
+ // If the IP is not allocated, associate it.
+ if l.PublicIpAddresses[0].Allocated == "" {
Review Comment:
[nitpick] When a user specifies a LoadBalancerIP that is already allocated,
the code doesn't verify if the IP belongs to the correct network (matching
`lb.networkID`) or VPC. While CloudStack API permissions might prevent
cross-account issues, an IP allocated to a different network within the same
account could cause load balancer rule creation to fail later. Consider adding
validation to check `l.PublicIpAddresses[0].Associatednetworkid` or
`l.PublicIpAddresses[0].Vpcid` against the expected network to provide a
clearer error message earlier in the process.
```suggestion
ip := l.PublicIpAddresses[0]
// Validate that the IP is associated with the correct network or VPC
if ip.Associatednetworkid != "" && ip.Associatednetworkid !=
lb.networkID {
return fmt.Errorf("IP address %v is associated with network %v,
but expected network %v", loadBalancerIP, ip.Associatednetworkid, lb.networkID)
}
// If the network is in a VPC, also check VPC ID if available
if ip.Vpcid != "" {
network, _, err := lb.Network.GetNetworkByID(lb.networkID,
cloudstack.WithProject(lb.projectID))
if err != nil {
return fmt.Errorf("error retrieving network for VPC
validation: %v", err)
}
if network.Vpcid != "" && ip.Vpcid != network.Vpcid {
return fmt.Errorf("IP address %v is associated with VPC
%v, but expected VPC %v", loadBalancerIP, ip.Vpcid, network.Vpcid)
}
}
lb.ipAddr = ip.Ipaddress
lb.ipAddrID = ip.Id
// If the IP is not allocated, associate it.
if ip.Allocated == "" {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
