Skip to content

Allow setting of source CIDR for LB rule #78

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 26, 2025
Merged

Allow setting of source CIDR for LB rule #78

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions cloudstack_loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package cloudstack
import (
"context"
"fmt"
"net"
"strconv"
"strings"

Expand All @@ -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"
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

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
Suggested change
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.

Copilot uses AI. Check for mistakes.
ServiceAnnotationLoadBalancerSourceCidrs = "service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs"
)

type loadBalancer struct {
Expand Down Expand Up @@ -162,7 +163,7 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s
}
} else {
klog.V(4).Infof("Creating load balancer rule: %v", lbRuleName)
lbRule, err = lb.createLoadBalancerRule(lbRuleName, port, protocol)
lbRule, err = lb.createLoadBalancerRule(lbRuleName, port, protocol, service)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -596,7 +597,7 @@ func (lb *loadBalancer) updateLoadBalancerRule(lbRuleName string, protocol LoadB
}

// createLoadBalancerRule creates a new load balancer rule and returns it's ID.
func (lb *loadBalancer) createLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol) (*cloudstack.LoadBalancerRule, error) {
func (lb *loadBalancer) createLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service) (*cloudstack.LoadBalancerRule, error) {
p := lb.LoadBalancer.NewCreateLoadBalancerRuleParams(
lb.algorithm,
lbRuleName,
Expand All @@ -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)
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

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").

Suggested change
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)

Copilot uses AI. Check for mistakes.
}
cidrList[i] = cidr
}
} else {
cidrList = []string{defaultAllowedCIDR}
}
Comment on lines +618 to +629
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

[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.

Copilot uses AI. Check for mistakes.

// Set the CIDR list in the parameters
p.SetCidrlist(cidrList)
Comment on lines +615 to +632
Copy link

@DaanHoogland DaanHoogland Oct 21, 2025

Choose a reason for hiding this comment

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

i’d rather see

Suggested change
// 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)

Comment on lines +615 to +632
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

// Create a new load balancer rule.
r, err := lb.LoadBalancer.CreateLoadBalancerRule(p)
if err != nil {
Expand Down
Loading