Allow setting of source CIDR for LB rule #78
Conversation
CodeBleu
commented
Jun 9, 2025
CodeBleu
commented
* Annotation added to allow setting of Source CIDR for Load Balancer
rule
There was a problem hiding this comment.
LGTM
Tested by building a docker image based on the pr
Before the fix
After you expose a service, there is no source cidr list populated
k expose deploy/nginx-deployment --port=80 --type=LoadBalancer
After fix, there is source cidr populated on the loadbalancer rule
Tested with normal service.yaml
apiVersion: v1
kind: Service
metadata:
name: nginx-deployment2
namespace: default
annotations:
service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs: "1.2.3.4/32,5.6.7.8/32"
spec:
type: LoadBalancer
selector:
app: nginx
ports:
- port: 80
targetPort: 80
protocol: TCP
nodePort: 30558
externalTrafficPolicy: Cluster
allocateLoadBalancerNodePorts: true
sessionAffinity: None
|
@Pearl1594 Do you mind doing a quick review of this? I believe I need 2 approvals and a test output before I can merge. |
| // Read the source CIDR annotation | ||
| sourceCIDRs, ok := service.Annotations[ServiceAnnotationLoadBalancerSourceCidrs] | ||
| var cidrList []string | ||
| if ok && sourceCIDRs != "" { | ||
| cidrList = strings.Split(sourceCIDRs, ",") | ||
| for i, cidr := range cidrList { | ||
| cidr = strings.TrimSpace(cidr) | ||
| if _, _, err := net.ParseCIDR(cidr); err != nil { | ||
| return nil, fmt.Errorf("invalid CIDR in annotation %s: %s", ServiceAnnotationLoadBalancerSourceCidrs, cidr) | ||
| } | ||
| cidrList[i] = cidr | ||
| } | ||
| } else { | ||
| cidrList = []string{defaultAllowedCIDR} | ||
| } | ||
|
|
||
| // Set the CIDR list in the parameters | ||
| p.SetCidrlist(cidrList) |
There was a problem hiding this comment.
i’d rather see
| // Read the source CIDR annotation | |
| sourceCIDRs, ok := service.Annotations[ServiceAnnotationLoadBalancerSourceCidrs] | |
| var cidrList []string | |
| if ok && sourceCIDRs != "" { | |
| cidrList = strings.Split(sourceCIDRs, ",") | |
| for i, cidr := range cidrList { | |
| cidr = strings.TrimSpace(cidr) | |
| if _, _, err := net.ParseCIDR(cidr); err != nil { | |
| return nil, fmt.Errorf("invalid CIDR in annotation %s: %s", ServiceAnnotationLoadBalancerSourceCidrs, cidr) | |
| } | |
| cidrList[i] = cidr | |
| } | |
| } else { | |
| cidrList = []string{defaultAllowedCIDR} | |
| } | |
| // Set the CIDR list in the parameters | |
| p.SetCidrlist(cidrList) | |
| // Set the CIDR list in the parameters | |
| p.SetCidrlist(readTheSourceCidrAnnotation(service)) |
and
func readTheSourceCidrAnnotation(service *corev1.Service) []string {
// Read the source CIDR annotation
sourceCIDRs, ok := service.Annotations[ServiceAnnotationLoadBalancerSourceCidrs]
var cidrList []string
if ok && sourceCIDRs != "" {
cidrList = strings.Split(sourceCIDRs, ",")
for i, cidr := range cidrList {
cidr = strings.TrimSpace(cidr)
if _, _, err := net.ParseCIDR(cidr); err != nil {
return nil, fmt.Errorf("invalid CIDR in annotation %s: %s", ServiceAnnotationLoadBalancerSourceCidrs, cidr)
}
cidrList[i] = cidr
}
} else {
cidrList = []string{defaultAllowedCIDR}
}
return cidrList
}
(no waranty on the syntax)
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring source CIDR restrictions on CloudStack load balancer rules through a new Kubernetes service annotation. This allows users to restrict which IP ranges can access their load balancers by setting service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs on their Service resources.
Key Changes:
- Added new annotation constant
ServiceAnnotationLoadBalancerSourceCidrsfor specifying allowed source CIDRs - Implemented CIDR validation and parsing logic in
createLoadBalancerRule - Modified function signature to pass the service object for annotation access
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ok && sourceCIDRs != "" { | ||
| cidrList = strings.Split(sourceCIDRs, ",") | ||
| for i, cidr := range cidrList { | ||
| cidr = strings.TrimSpace(cidr) | ||
| if _, _, err := net.ParseCIDR(cidr); err != nil { | ||
| return nil, fmt.Errorf("invalid CIDR in annotation %s: %s", ServiceAnnotationLoadBalancerSourceCidrs, cidr) | ||
| } | ||
| cidrList[i] = cidr | ||
| } | ||
| } else { | ||
| cidrList = []string{defaultAllowedCIDR} | ||
| } |
There was a problem hiding this comment.
[nitpick] The CIDR parsing and validation logic (lines 618-629) is embedded directly in createLoadBalancerRule. Consider extracting this into a helper function (e.g., parseCIDRList(cidrs string) ([]string, error)) to:
- Improve testability - the validation logic could be unit tested independently
- Enable reuse - if firewall or Network ACL rules need to respect this annotation in the future (which they should for security consistency)
- Simplify the
createLoadBalancerRulefunction
This would follow the pattern established by existing helper functions like getStringFromServiceAnnotation and getBoolFromServiceAnnotation.
| for i, cidr := range cidrList { | ||
| cidr = strings.TrimSpace(cidr) | ||
| if _, _, err := net.ParseCIDR(cidr); err != nil { | ||
| return nil, fmt.Errorf("invalid CIDR in annotation %s: %s", ServiceAnnotationLoadBalancerSourceCidrs, cidr) |
There was a problem hiding this comment.
The error message doesn't include the underlying error from net.ParseCIDR(cidr), which could provide valuable debugging information. Consider wrapping the error:
return nil, fmt.Errorf("invalid CIDR %q in annotation %s: %w", cidr, ServiceAnnotationLoadBalancerSourceCidrs, err)This would give users more context about why the CIDR is invalid (e.g., "invalid CIDR notation" vs "CIDR address has host bits set").
| return nil, fmt.Errorf("invalid CIDR in annotation %s: %s", ServiceAnnotationLoadBalancerSourceCidrs, cidr) | |
| return nil, fmt.Errorf("invalid CIDR %q in annotation %s: %w", cidr, ServiceAnnotationLoadBalancerSourceCidrs, err) |
| // Read the source CIDR annotation | ||
| sourceCIDRs, ok := service.Annotations[ServiceAnnotationLoadBalancerSourceCidrs] | ||
| var cidrList []string | ||
| if ok && sourceCIDRs != "" { | ||
| cidrList = strings.Split(sourceCIDRs, ",") | ||
| for i, cidr := range cidrList { | ||
| cidr = strings.TrimSpace(cidr) | ||
| if _, _, err := net.ParseCIDR(cidr); err != nil { | ||
| return nil, fmt.Errorf("invalid CIDR in annotation %s: %s", ServiceAnnotationLoadBalancerSourceCidrs, cidr) | ||
| } | ||
| cidrList[i] = cidr | ||
| } | ||
| } else { | ||
| cidrList = []string{defaultAllowedCIDR} | ||
| } | ||
|
|
||
| // Set the CIDR list in the parameters | ||
| p.SetCidrlist(cidrList) |
There was a problem hiding this comment.
The annotation ServiceAnnotationLoadBalancerSourceCidrs only affects the load balancer rule's CIDR list, but doesn't affect firewall rules (which use service.Spec.LoadBalancerSourceRanges at line 188) or Network ACL rules (which are hardcoded to 0.0.0.0/0 at line 922).
This creates an inconsistent security posture where:
- The load balancer rule may restrict access to specific CIDRs via the annotation
- But the firewall/ACL rules could allow broader access, effectively bypassing the restriction
Recommendation: Consider one of these approaches:
- Option 1 (Preferred): Use a unified CIDR source that checks the annotation first, falls back to
service.Spec.LoadBalancerSourceRanges, then todefaultAllowedCIDR. Apply this to all three security mechanisms (LB rules, firewall rules, and Network ACLs). - Option 2: Document clearly that users must set both the annotation AND
service.Spec.LoadBalancerSourceRangesto the same values for consistent security enforcement. - Option 3: Have the annotation take precedence and automatically apply it to firewall and ACL rules as well.
Option 1 would provide the most intuitive and secure behavior, as it aligns with Kubernetes' standard LoadBalancerSourceRanges field while allowing the CloudStack-specific annotation to override it when needed.
| ServiceAnnotationLoadBalancerProxyProtocol = "service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol" | ||
|
|
||
| ServiceAnnotationLoadBalancerProxyProtocol = "service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol" | ||
| ServiceAnnotationLoadBalancerLoadbalancerHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname" |
There was a problem hiding this comment.
The new annotation ServiceAnnotationLoadBalancerSourceCidrs lacks a documentation comment explaining its purpose and usage. For consistency with ServiceAnnotationLoadBalancerProxyProtocol (lines 41-44), consider adding a comment that describes:
- What the annotation does
- The expected format (comma-separated CIDRs)
- Any version requirements or constraints
| ServiceAnnotationLoadBalancerLoadbalancerHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname" | |
| ServiceAnnotationLoadBalancerLoadbalancerHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname" | |
| // ServiceAnnotationLoadBalancerSourceCidrs is the annotation used on the | |
| // service to specify the allowed source CIDRs for a CloudStack load balancer. | |
| // The value should be a comma-separated list of CIDRs (e.g., "10.0.0.0/8,192.168.1.0/24"). | |
| // If not specified, the default is to allow all sources ("0.0.0.0/0"). | |
| // There are no specific CloudStack version requirements for this annotation. |
|
Code looks good. But it doesn't handle when the CIDR list is updated in the annotation. |
This is because that API didn't exist when the PR was first created. - apache/cloudstack#11568 Can we merge this as is, and maybe create an issue for the update piece as well as some code refactoring suggestions? |
4 participants
