-
Notifications
You must be signed in to change notification settings - Fork 32
Associate loadBalancerIP to the network when specified in the Spec #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
c69e916 to
880b75c
Compare
880b75c to
483f3b7
Compare
harikrishna-patnala
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
kiranchavala
left a comment
There was a problem hiding this 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
483f3b7 to
922c946
Compare
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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:
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.setServiceAnnotationmethod incloudstack.go, which uses the Kubernetes client to update service annotations, ensuring that the annotation is set when the controller associates an IP.Improvements to public IP address handling:
getPublicIPAddressto associate an IP if it is found but not yet allocated, and improved error messages for clarity. [1] [2]associatePublicIPAddressto 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:
clientBuilderfield toCSCloudand ensured it is set during initialization, enabling the provider to interact with the Kubernetes API for annotation management. [1] [2] [3]loadBalancerstruct 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.