Skip to content

Conversation

@vishesh92
Copy link
Member

Fixes #51

Details

This pull request introduces enhancements to the CloudStack cloud provider for Kubernetes, focusing on better management and tracking of load balancer IP addresses associated by the controller. The main improvements are the addition of a service annotation to track controller-associated IPs, logic to set and use this annotation, and safer handling of public IP allocation and release.

Enhancements to load balancer IP management:

  • Added a new service annotation, service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller, to mark when the controller has associated a load balancer IP. This annotation is used to determine if the IP should be disassociated when the service is deleted.
  • Implemented the setServiceAnnotation method in cloudstack.go, which uses the Kubernetes client to update service annotations, ensuring that the annotation is set when the controller associates an IP.
  • Modified the load balancer lifecycle:
    • When the controller associates an IP, the annotation is set on the Service object.
    • On deletion, the controller checks this annotation to decide whether to disassociate the IP, ensuring that only controller-associated IPs are released. Additional checks prevent disassociation if other load balancer rules still use the IP.

Improvements to public IP address handling:

  • Updated the logic in getPublicIPAddress to associate an IP if it is found but not yet allocated, and improved error messages for clarity. [1] [2]
  • Enhanced associatePublicIPAddress to set the IP address parameter if known and to record when the controller has associated the IP, enabling accurate annotation and cleanup. [1] [2]

Internal API and structure changes:

  • Added a clientBuilder field to CSCloud and ensured it is set during initialization, enabling the provider to interact with the Kubernetes API for annotation management. [1] [2] [3]
  • Extended the loadBalancer struct to track whether the controller associated the IP (ipAssociatedByController).

These changes improve the reliability and correctness of load balancer IP management, particularly in scenarios where IPs are dynamically allocated and need to be safely cleaned up.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the CloudStack Kubernetes provider to support associating user-specified load balancer IP addresses that are unallocated. When a user specifies a LoadBalancerIP in the service spec that exists but is not yet allocated, the controller will now associate it to the appropriate network. The PR also introduces annotation-based tracking to determine whether the controller should disassociate the IP when the service is deleted.

  • Adds logic to detect and associate unallocated IPs specified in service specs
  • Implements annotation tracking to record controller-associated IPs for proper cleanup
  • Enhances deletion logic to check for shared IP usage before disassociation

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
cloudstack_loadbalancer.go Adds new annotation constant for tracking controller-associated IPs, extends loadBalancer struct with ipAssociatedByController field, implements logic to find and associate unallocated IPs, and enhances deletion logic to safely handle IP disassociation based on annotation and shared usage checks
cloudstack.go Adds clientBuilder field to CSCloud struct, implements setServiceAnnotation method to update service annotations via Kubernetes API, and stores clientBuilder during initialization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@vishesh92 vishesh92 added this to the 1.2.0 milestone Nov 25, 2025
@vishesh92 vishesh92 force-pushed the fix-loadbalancer-ip branch 3 times, most recently from c69e916 to 880b75c Compare November 25, 2025 15:02
@vishesh92 vishesh92 marked this pull request as ready for review November 25, 2025 15:05
@vishesh92 vishesh92 force-pushed the fix-loadbalancer-ip branch from 880b75c to 483f3b7 Compare December 3, 2025 08:32
Copy link

@harikrishna-patnala harikrishna-patnala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@kiranchavala kiranchavala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting the following exception in logs

W1209 20:01:33.593888 1 cloudstack_loadbalancer.go:142] Failed to set annotation on service default/nginx-deployment2: failed to get service: services "nginx-deployment2" is forbidden: User "system:serviceaccount:kube-system:cloud-controller-manager" cannot get resource "services" in API group "" in the namespace "default"

used the following yaml

apiVersion: v1
kind: Service
metadata:
  name: nginx-deployment2
  namespace: default
  annotations:
spec:
  type: LoadBalancer
  loadBalancerIP: "10.0.58.17"
  selector:
    app: nginx
  ports:
    - port: 80
      targetPort: 80
      protocol: TCP
      nodePort: 30558
  externalTrafficPolicy: Cluster
  allocateLoadBalancerNodePorts: true
  sessionAffinity: None


@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 91 lines in your changes missing coverage. Please review.
✅ Project coverage is 13.64%. Comparing base (282ef97) to head (922c946).

Files with missing lines Patch % Lines
cloudstack.go 0.00% 53 Missing ⚠️
cloudstack_loadbalancer.go 0.00% 38 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   15.09%   13.64%   -1.46%     
==========================================
  Files           4        4              
  Lines         808      894      +86     
==========================================
  Hits          122      122              
- Misses        684      770      +86     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support static public ip address as a Load balancer IP

4 participants