From 3c18263436069bfcd791336221c0fd864f3be0b6 Mon Sep 17 00:00:00 2001 From: Ingo Gottwald Date: Mon, 3 Jun 2024 21:33:19 +0200 Subject: [PATCH] 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 Co-authored-by: Ingo Gottwald --- CHANGELOG.md | 2 + cloud-controller-manager/do/loadbalancers.go | 42 +++++++++++- .../do/loadbalancers_test.go | 67 ++++++++++++++++++- docs/controllers/services/annotations.md | 8 ++- 4 files changed, 112 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2b596676..aab91ee05 100644 --- 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 index 224277185..aa2664d4c 100644 --- 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 index a9c88f83c..5abd375a2 100644 --- 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 index 94032c4ec..d62885d2f 100644 --- a/docs/controllers/services/annotations.md +++ b/docs/controllers/services/annotations.md @@ -211,7 +211,7 @@ 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** @@ -219,8 +219,10 @@ 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.