Add support for loadBalancerSourceRanges (#731)

This change introduces support for loadBalancerSourceRanges which is the preferred and recommended way to add firewall allow rules and therefore takes presedence over annotation based allow rules. Co-authored-by: J0sh0nat0r <[email protected]> Co-authored-by: Ingo Gottwald <Ingo Gottwald <[email protected]>
digitalocean · Jun 3, 2024 · 3c18263 · 3c18263
1 parent 5c52fbc
commit 3c18263
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## unreleased
 
+* Adding support for specifiying `loadBalancerSourceRanges` in the service spec. Source ranges take precedence over annotation based allow rules (`service.beta.kubernetes.io/do-loadbalancer-allow-rules`).
+
 ## v0.1.51 (beta) - May 28, 2024
 
 * Adjusts load balancer health check behaviour to probe Kubernetes components correctly, ensuring that LB traffic stops

diff --git a/cloud-controller-manager/do/loadbalancers.go b/cloud-controller-manager/do/loadbalancers.go
@@ -34,6 +34,7 @@ import (
 	"k8s.io/cloud-provider/api"
 	servicehelpers "k8s.io/cloud-provider/service/helpers"
 	"k8s.io/klog/v2"
+	utilnet "k8s.io/utils/net"
 
 	"github.com/digitalocean/godo"
 )
@@ -604,6 +605,11 @@ func buildLoadBalancerRequest(ctx context.Context, service *v1.Service, godoClie
 		return nil, err
 	}
 
+	fw, err := buildFirewall(service)
+	if err != nil {
+		return nil, err
+	}
+
 	return &godo.LoadBalancerRequest{
 		Name:                         lbName,
 		SizeSlug:                     sizeSlug,
@@ -617,7 +623,7 @@ func buildLoadBalancerRequest(ctx context.Context, service *v1.Service, godoClie
 		EnableBackendKeepalive:       enableBackendKeepalive,
 		DisableLetsEncryptDNSRecords: &disableLetsEncryptDNSRecords,
 		HTTPIdleTimeoutSeconds:       httpIdleTimeoutSeconds,
-		Firewall:                     buildFirewall(service),
+		Firewall:                     fw,
 		Type:                         lbType,
 	}, nil
 }
@@ -866,11 +872,41 @@ func buildForwardingRule(service *v1.Service, port *v1.ServicePort, protocol, ce
 	return &forwardingRule, nil
 }
 
-func buildFirewall(service *v1.Service) *godo.LBFirewall {
+func buildFirewall(service *v1.Service) (*godo.LBFirewall, error) {
+	var allow []string
+	sourceRanges, err := getSourceRangeRules(service)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get source range rules: %s", err)
+	}
+	if len(sourceRanges) > 0 {
+		// source ranges should take precedence over annotation based allow rules
+		allow = sourceRanges
+	} else {
+		allow = getStrings(service, annDOAllowRules)
+	}
 	return &godo.LBFirewall{
 		Deny:  getStrings(service, annDODenyRules),
-		Allow: getStrings(service, annDOAllowRules),
+		Allow: allow,
+	}, nil
+}
+
+// getSourceRangeRules returns loadBalancerSourceRanges in the format
+// `cidr:{source}`
+func getSourceRangeRules(service *v1.Service) ([]string, error) {
+	if len(service.Spec.LoadBalancerSourceRanges) == 0 {
+		return nil, nil
+	}
+	specs := service.Spec.LoadBalancerSourceRanges
+	ipnets, err := utilnet.ParseIPNets(specs...)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse spec.loadBalancerSourceRanges %q: %s (expected format for an IP address is 10.0.0.0/24)", specs, err)
+	}
+
+	var sourceAllows []string
+	for _, sourceRange := range ipnets {
+		sourceAllows = append(sourceAllows, fmt.Sprintf("cidr:%s", sourceRange))
 	}
+	return sourceAllows, nil
 }
 
 func buildTLSForwardingRule(forwardingRule *godo.ForwardingRule, service *v1.Service, port int32, certificateID string, tlsPassThrough bool) error {

diff --git a/cloud-controller-manager/do/loadbalancers_test.go b/cloud-controller-manager/do/loadbalancers_test.go
@@ -6160,6 +6160,7 @@ func Test_buildFirewall(t *testing.T) {
 		name             string
 		service          *v1.Service
 		expectedFirewall *godo.LBFirewall
+		wantErr          bool
 	}{
 		{
 			name: "annotation not set",
@@ -6220,11 +6221,75 @@ func Test_buildFirewall(t *testing.T) {
 				Allow: []string{"ip:1.2.3.4", "ip:1.2.3.5"},
 			},
 		},
+		{
+			name: "source ranges not set",
+			service: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+					UID:  "abc123",
+				},
+				Spec: v1.ServiceSpec{
+					LoadBalancerSourceRanges: []string{},
+				},
+			},
+			expectedFirewall: &godo.LBFirewall{},
+		},
+		{
+			name: "source ranges set",
+			service: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+					UID:  "abc123",
+				},
+				Spec: v1.ServiceSpec{
+					LoadBalancerSourceRanges: []string{"1.2.0.0/16"},
+				},
+			},
+			expectedFirewall: &godo.LBFirewall{
+				Allow: []string{"cidr:1.2.0.0/16"},
+			},
+		},
+		{
+			name: "source ranges and annotations set",
+			service: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+					UID:  "abc123",
+					Annotations: map[string]string{
+						annDODenyRules:  "cidr:1.2.0.0/16",
+						annDOAllowRules: "ip:1.2.3.4,ip:1.2.3.5",
+					},
+				},
+				Spec: v1.ServiceSpec{
+					LoadBalancerSourceRanges: []string{"1.3.0.0/16"},
+				},
+			},
+			expectedFirewall: &godo.LBFirewall{
+				Deny:  []string{"cidr:1.2.0.0/16"},
+				Allow: []string{"cidr:1.3.0.0/16"},
+			},
+		},
+		{
+			name: "source ranges invalid",
+			service: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+					UID:  "abc123",
+				},
+				Spec: v1.ServiceSpec{
+					LoadBalancerSourceRanges: []string{"1.3.0.12"},
+				},
+			},
+			wantErr: true,
+		},
 	}
 
 	for _, test := range testcases {
 		t.Run(test.name, func(t *testing.T) {
-			firewall := buildFirewall(test.service)
+			firewall, err := buildFirewall(test.service)
+			if test.wantErr != (err != nil) {
+				t.Errorf("got error %q, want error: %t", err, test.wantErr)
+			}
 
 			if test.expectedFirewall == nil && firewall != nil {
 				t.Errorf("expected nil firewall, got %v", firewall)

diff --git a/docs/controllers/services/annotations.md b/docs/controllers/services/annotations.md
@@ -211,16 +211,18 @@ Specifies the HTTP idle timeout configuration in seconds. If not specified, the
 
 ## service.beta.kubernetes.io/do-loadbalancer-deny-rules
 
-Specifies the comma seperated DENY firewall rules for the load-balancer
+Specifies the comma separated DENY firewall rules for the load-balancer
 
 **Note**
 
 Rules must be in the format `{type}:{source}` (ex. `ip:1.2.3.4,cidr:2.3.0.0/16`)
 
 ## service.beta.kubernetes.io/do-loadbalancer-allow-rules
 
-Specifies the comma seperated ALLOW firewall rules for the load-balancer
+Specifies the comma separated ALLOW firewall rules for the load-balancer
 
 **Note**
 
-Rules must be in the format `{type}:{source}` (ex. `ip:1.2.3.4,cidr:2.3.0.0/16`)
+Rules must be in the format `{type}:{source}` (ex. `ip:1.2.3.4,cidr:2.3.0.0/16`).
+
+These rules will be ignored if `LoadBalancerSourceRanges` is set, which is the preferred way to enter allow rules.