Copilot commented on code in PR #78:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/78#discussion_r2555492827
##########
cloudstack_loadbalancer.go:
##########
@@ -606,12 +607,30 @@ func (lb *loadBalancer) createLoadBalancerRule(lbRuleName
string, port corev1.Se
p.SetNetworkid(lb.networkID)
p.SetPublicipid(lb.ipAddrID)
-
p.SetProtocol(protocol.CSProtocol())
// Do not open the firewall implicitly, we always create explicit
firewall rules
p.SetOpenfirewall(false)
+ // 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}
+ }
Review 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:
1. Improve testability - the validation logic could be unit tested
independently
2. Enable reuse - if firewall or Network ACL rules need to respect this
annotation in the future (which they should for security consistency)
3. Simplify the `createLoadBalancerRule` function
This would follow the pattern established by existing helper functions like
`getStringFromServiceAnnotation` and `getBoolFromServiceAnnotation`.
##########
cloudstack_loadbalancer.go:
##########
@@ -606,12 +607,30 @@ func (lb *loadBalancer) createLoadBalancerRule(lbRuleName
string, port corev1.Se
p.SetNetworkid(lb.networkID)
p.SetPublicipid(lb.ipAddrID)
-
p.SetProtocol(protocol.CSProtocol())
// Do not open the firewall implicitly, we always create explicit
firewall rules
p.SetOpenfirewall(false)
+ // 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)
Review Comment:
The error message doesn't include the underlying error from
`net.ParseCIDR(cidr)`, which could provide valuable debugging information.
Consider wrapping the error:
```go
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").
```suggestion
return nil, fmt.Errorf("invalid CIDR %q in
annotation %s: %w", cidr, ServiceAnnotationLoadBalancerSourceCidrs, err)
```
##########
cloudstack_loadbalancer.go:
##########
@@ -41,9 +42,9 @@ const (
// service to enable the proxy protocol on a CloudStack load balancer.
// Note that this protocol only applies to TCP service ports and
// CloudStack >= 4.6 is required for it to work.
- 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"
Review 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
```suggestion
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.
```
##########
cloudstack_loadbalancer.go:
##########
@@ -606,12 +607,30 @@ func (lb *loadBalancer) createLoadBalancerRule(lbRuleName
string, port corev1.Se
p.SetNetworkid(lb.networkID)
p.SetPublicipid(lb.ipAddrID)
-
p.SetProtocol(protocol.CSProtocol())
// Do not open the firewall implicitly, we always create explicit
firewall rules
p.SetOpenfirewall(false)
+ // 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)
Review 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:
1. The load balancer rule may restrict access to specific CIDRs via the
annotation
2. 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
to `defaultAllowedCIDR`. 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.LoadBalancerSourceRanges` to 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.
--
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]
