diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index 1c4080fd90..09ed9dd932 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -482,7 +482,7 @@ func (l4 *L4) ensureIPv4NodesFirewall(nodeNames []string, ipAddress string, resu }() // ensure firewalls - sourceRanges, err := utils.ServiceSourceRanges(l4.Service) + ipv4SourceRanges, err := utils.IPv4ServiceSourceRanges(l4.Service) if err != nil { result.Error = err return @@ -490,7 +490,7 @@ func (l4 *L4) ensureIPv4NodesFirewall(nodeNames []string, ipAddress string, resu // Add firewall rule for ILB traffic to nodes nodesFWRParams := firewalls.FirewallParams{ PortRanges: portRanges, - SourceRanges: sourceRanges, + SourceRanges: ipv4SourceRanges, DestinationRanges: []string{ipAddress}, Protocol: string(protocol), Name: firewallName, diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 44d86dbb07..403ba68e44 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -2155,9 +2155,9 @@ func verifyILBIPv4NodesFirewall(l4 *L4, nodeNames []string) error { return fmt.Errorf("failed to create description for resources, err %w", err) } - sourceRanges, err := utils.ServiceSourceRanges(l4.Service) + sourceRanges, err := utils.IPv4ServiceSourceRanges(l4.Service) if err != nil { - return fmt.Errorf("servicehelper.GetLoadBalancerSourceRanges(%+v) returned error %v, want nil", l4.Service, err) + return fmt.Errorf("utils.IPv4ServiceSourceRanges(%+v) returned error %v, want nil", l4.Service, err) } return verifyFirewall(l4.cloud, nodeNames, fwName, fwDesc, sourceRanges) } @@ -2170,7 +2170,11 @@ func verifyILBIPv6NodesFirewall(l4 *L4, nodeNames []string) error { return fmt.Errorf("failed to create description for resources, err %w", err) } - return verifyFirewall(l4.cloud, nodeNames, ipv6FirewallName, fwDesc, []string{"0::0/0"}) + sourceRanges, err := utils.IPv6ServiceSourceRanges(l4.Service) + if err != nil { + return fmt.Errorf("utils.IPv6ServiceSourceRanges(%+v) returned error %v, want nil", l4.Service, err) + } + return verifyFirewall(l4.cloud, nodeNames, ipv6FirewallName, fwDesc, sourceRanges) } func verifyILBIPv4HealthCheckFirewall(l4 *L4, nodeNames []string) error { diff --git a/pkg/loadbalancers/l4ipv6.go b/pkg/loadbalancers/l4ipv6.go index 11a2b476fc..e5267ec94a 100644 --- a/pkg/loadbalancers/l4ipv6.go +++ b/pkg/loadbalancers/l4ipv6.go @@ -130,10 +130,16 @@ func (l4 *L4) ensureIPv6NodesFirewall(forwardingRule *composite.ForwardingRule, klog.V(2).Infof("Finished ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, time taken: %v", l4.Service.Namespace, l4.Service.Name, firewallName, time.Since(start)) }() + // ensure firewalls + ipv6SourceRanges, err := utils.IPv6ServiceSourceRanges(l4.Service) + if err != nil { + result.Error = err + return + } + ipv6nodesFWRParams := firewalls.FirewallParams{ - PortRanges: portRanges, - // TODO(panslava): support .spec.loadBalancerSourceRanges - SourceRanges: []string{"0::0/0"}, + PortRanges: portRanges, + SourceRanges: ipv6SourceRanges, DestinationRanges: []string{forwardingRule.IPAddress}, Protocol: string(protocol), Name: firewallName, @@ -141,7 +147,7 @@ func (l4 *L4) ensureIPv6NodesFirewall(forwardingRule *composite.ForwardingRule, L4Type: utils.ILB, } - err := firewalls.EnsureL4LBFirewallForNodes(l4.Service, &ipv6nodesFWRParams, l4.cloud, l4.recorder) + err = firewalls.EnsureL4LBFirewallForNodes(l4.Service, &ipv6nodesFWRParams, l4.cloud, l4.recorder) if err != nil { result.GCEResourceInError = annotations.FirewallRuleIPv6Resource result.Error = err diff --git a/pkg/loadbalancers/l4netlb.go b/pkg/loadbalancers/l4netlb.go index fee51af1f4..9d8e3e7ce4 100644 --- a/pkg/loadbalancers/l4netlb.go +++ b/pkg/loadbalancers/l4netlb.go @@ -284,7 +284,7 @@ func (l4netlb *L4NetLB) ensureNodesFirewall(name string, nodeNames []string, ipA }() result := &L4NetLBSyncResult{} - sourceRanges, err := utils.ServiceSourceRanges(l4netlb.Service) + sourceRanges, err := utils.IPv4ServiceSourceRanges(l4netlb.Service) if err != nil { result.Error = err return result diff --git a/pkg/loadbalancers/l4netlb_test.go b/pkg/loadbalancers/l4netlb_test.go index 428533ecdf..30647f375a 100644 --- a/pkg/loadbalancers/l4netlb_test.go +++ b/pkg/loadbalancers/l4netlb_test.go @@ -331,7 +331,7 @@ func verifyNetLBNodesFirewall(l4netlb *L4NetLB, nodeNames []string) error { return fmt.Errorf("failed to create description for resources, err %w", err) } - sourceRanges, err := utils.ServiceSourceRanges(l4netlb.Service) + sourceRanges, err := utils.IPv4ServiceSourceRanges(l4netlb.Service) if err != nil { return fmt.Errorf("servicehelper.GetLoadBalancerSourceRanges(%+v) returned error %v, want nil", l4netlb.Service, err) } diff --git a/pkg/utils/sourceranges.go b/pkg/utils/sourceranges.go index 6579efdd5a..bd30149934 100644 --- a/pkg/utils/sourceranges.go +++ b/pkg/utils/sourceranges.go @@ -10,18 +10,43 @@ import ( const ( allowAllIPv4Range = "0.0.0.0/0" + allowAllIPv6Range = "0::0/0" ) -func ServiceSourceRanges(service *v1.Service) ([]string, error) { - ipRanges, err := getAllSourceRanges(service) +func IPv4ServiceSourceRanges(service *v1.Service) ([]string, error) { + allRanges, err := getAllSourceRanges(service) if err != nil { return nil, err } - if len(ipRanges) == 0 { + var ipv4Ranges []string + for _, ip := range allRanges { + if net.IsIPv4CIDR(ip) { + ipv4Ranges = append(ipv4Ranges, ip.String()) + } + } + if len(ipv4Ranges) == 0 { return []string{allowAllIPv4Range}, nil } - return ipRanges.StringSlice(), nil + return ipv4Ranges, nil +} + +func IPv6ServiceSourceRanges(service *v1.Service) ([]string, error) { + allRanges, err := getAllSourceRanges(service) + if err != nil { + return nil, err + } + + var ipv6Ranges []string + for _, ip := range allRanges { + if net.IsIPv6CIDR(ip) { + ipv6Ranges = append(ipv6Ranges, ip.String()) + } + } + if len(ipv6Ranges) == 0 { + return []string{allowAllIPv6Range}, nil + } + return ipv6Ranges, nil } func getAllSourceRanges(service *v1.Service) (net.IPNetSet, error) { diff --git a/pkg/utils/sourceranges_test.go b/pkg/utils/sourceranges_test.go index e901c205c7..5ffde5fa1d 100644 --- a/pkg/utils/sourceranges_test.go +++ b/pkg/utils/sourceranges_test.go @@ -10,37 +10,42 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestServiceSourceRanges(t *testing.T) { +func TestIPv4ServiceSourceRanges(t *testing.T) { testCases := []struct { - desc string - specRanges []string - annotations map[string]string - expectedSourceRanges []string - expectError error + desc string + specRanges []string + annotations map[string]string + expectedIPv4SourceRanges []string + expectError error }{ { - desc: "Should return allow all for no specs or annotations", - expectedSourceRanges: []string{"0.0.0.0/0"}, + desc: "Should return allow all for no specs or annotations", + expectedIPv4SourceRanges: []string{"0.0.0.0/0"}, }, { - desc: "Should parse ranges from spec", - specRanges: []string{" 192.168.0.1/10", " 132.8.0.1/8 "}, - expectedSourceRanges: []string{"192.128.0.0/10", "132.0.0.0/8"}, // only significant bits are left, spaces are trimmed + desc: "Should return allow all if only IPv6 CIDRs in Spec", + specRanges: []string{" 0::0/0 ", "2001:db8:3333:4444:5555:6666:7777:8888/32"}, + expectedIPv4SourceRanges: []string{"0.0.0.0/0"}, }, { - desc: "Should parse ranges from annotations, if no spec value", + desc: "Should get IPv4 addresses only, if mixed with IPv6", + specRanges: []string{"0::0/0", " 192.168.0.1/10 ", "2001:db8:3333:4444:5555:6666:7777:8888/32", " 132.8.0.1/8"}, + expectedIPv4SourceRanges: []string{"192.128.0.0/10", "132.0.0.0/8"}, // only significant bits are left, space are trimmed + }, + { + desc: "Should get IPv4 addresses from annotation, if mixed with IPv6 and no spec ranges", annotations: map[string]string{ - v1.AnnotationLoadBalancerSourceRangesKey: " 192.168.0.1/10 , 132.8.0.1/8 ", + v1.AnnotationLoadBalancerSourceRangesKey: "0::0/0, 192.168.0.1/10 ,2001:db8:3333:4444:5555:6666:7777:8888/32, 132.8.0.1/8 ", }, - expectedSourceRanges: []string{"192.128.0.0/10", "132.0.0.0/8"}, // only significant bits are left, spaces are trimmed + expectedIPv4SourceRanges: []string{"192.128.0.0/10", "132.0.0.0/8"}, // only significant bits are left, spaces are trimmed }, { desc: "Should ignore annotation if spec is present", - specRanges: []string{"192.168.0.1/10", "132.8.0.1/8"}, + specRanges: []string{"0::0/0", " 192.168.0.1/10 ", "2001:db8:3333:4444:5555:6666:7777:8888/32 ", " 132.8.0.1/8"}, annotations: map[string]string{ v1.AnnotationLoadBalancerSourceRangesKey: "1.2.3 1.2.3", // should not return error, even if annotation is invalid }, - expectedSourceRanges: []string{"192.128.0.0/10", "132.0.0.0/8"}, // only significant bits are left + expectedIPv4SourceRanges: []string{"192.128.0.0/10", "132.0.0.0/8"}, // only significant bits are left, spaces are trimmed }, { desc: "Should return special error for invalid spec value", @@ -73,17 +78,101 @@ func TestServiceSourceRanges(t *testing.T) { }, } - sourceRanges, err := ServiceSourceRanges(svc) + ipv4Ranges, err := IPv4ServiceSourceRanges(svc) + errDiff := cmp.Diff(tc.expectError, err) + if errDiff != "" { + t.Errorf("Expected error %v, got %v, diff: %v", tc.expectError, err, errDiff) + } + + sort.Strings(tc.expectedIPv4SourceRanges) + sort.Strings(ipv4Ranges) + diff := cmp.Diff(tc.expectedIPv4SourceRanges, ipv4Ranges) + if diff != "" { + t.Errorf("Expected IPv4 ranges: %v, got ranges %v, diff: %s", tc.expectedIPv4SourceRanges, ipv4Ranges, diff) + } + }) + } +} + +func TestIPv6ServiceSourceRanges(t *testing.T) { + testCases := []struct { + desc string + specRanges []string + annotations map[string]string + expectedIPv6SourceRanges []string + expectError error + }{ + { + desc: "Should return allow all for no specs or annotations", + expectedIPv6SourceRanges: []string{"0::0/0"}, + }, + { + desc: "Should return allow all if only IPv4 CIDRs in Spec", + specRanges: []string{" 1.2.3.4/5 ", " 192.168.0.1/10 "}, + expectedIPv6SourceRanges: []string{"0::0/0"}, + }, + { + desc: "Should get IPv6 addresses only, if mixed with IPv4", + specRanges: []string{" 2001:db8:4444:4444:5555:6666:7777:8888/10 ", " 192.168.0.1/10 ", "2001:db8:3333:4444:5555:6666:7777:8888/32", " 132.8.0.1/8"}, + expectedIPv6SourceRanges: []string{"2000::/10", "2001:db8::/32"}, // only significant bits are left, space are trimmed + }, + { + desc: "Should get IPv6 addresses from annotation, if mixed with IPv4 and no spec ranges", + annotations: map[string]string{ + v1.AnnotationLoadBalancerSourceRangesKey: " 2001:db8:4444:4444:5555:6666:7777:8888/10 , 192.168.0.1/10 ,2001:db8:3333:4444:5555:6666:7777:8888/32, 132.8.0.1/8 ", + }, + expectedIPv6SourceRanges: []string{"2000::/10", "2001:db8::/32"}, // only significant bits are left, space are trimmed + }, + { + desc: "Should ignore annotation if spec is present", + specRanges: []string{" 2001:db8:4444:4444:5555:6666:7777:8888/10 ", " 192.168.0.1/10 ", "2001:db8:3333:4444:5555:6666:7777:8888/32 ", " 132.8.0.1/8"}, + annotations: map[string]string{ + v1.AnnotationLoadBalancerSourceRangesKey: "2001:db8:3333:4444:5555:6666:7777:8888 1.2.3", // should not return error, even if annotation is invalid + }, + expectedIPv6SourceRanges: []string{"2000::/10", "2001:db8::/32"}, // only significant bits are left, space are trimmed + }, + { + desc: "Should return special error for invalid spec value", + specRanges: []string{"2001:db8:4444:4444:5555:6666:7777:8888"}, // no mask + expectError: &InvalidLoadBalancerSourceRangesSpecError{ + LoadBalancerSourceRangesSpec: []string{"2001:db8:4444:4444:5555:6666:7777:8888"}, + ParseErr: &net.ParseError{Type: "CIDR address", Text: "2001:db8:4444:4444:5555:6666:7777:8888"}, + }, + }, + { + desc: "Should return special error for invalid annotation value", + annotations: map[string]string{ + v1.AnnotationLoadBalancerSourceRangesKey: "2001:db8:4444:4444:5555:6666:7777:8888/14 2001:db8:4444:4444:5555:6666:7777:8888/15", // should be comma-separated + }, + expectError: &InvalidLoadBalancerSourceRangesAnnotationError{ + LoadBalancerSourceRangesAnnotation: "2001:db8:4444:4444:5555:6666:7777:8888/14 2001:db8:4444:4444:5555:6666:7777:8888/15", + ParseErr: &net.ParseError{Type: "CIDR address", Text: "2001:db8:4444:4444:5555:6666:7777:8888/14 2001:db8:4444:4444:5555:6666:7777:8888/15"}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + svc := &v1.Service{ + Spec: v1.ServiceSpec{ + LoadBalancerSourceRanges: tc.specRanges, + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: tc.annotations, + }, + } + + ipv6Ranges, err := IPv6ServiceSourceRanges(svc) errDiff := cmp.Diff(tc.expectError, err) if errDiff != "" { t.Errorf("Expected error %v, got %v, diff: %v", tc.expectError, err, errDiff) } - sort.Strings(tc.expectedSourceRanges) - sort.Strings(sourceRanges) - diff := cmp.Diff(tc.expectedSourceRanges, sourceRanges) + sort.Strings(tc.expectedIPv6SourceRanges) + sort.Strings(ipv6Ranges) + diff := cmp.Diff(tc.expectedIPv6SourceRanges, ipv6Ranges) if diff != "" { - t.Errorf("Expected source ranges: %v, got ranges %v, diff: %s", tc.expectedSourceRanges, sourceRanges, diff) + t.Errorf("Expected IPv6 ranges: %v, got ranges %v, diff: %s", tc.expectedIPv6SourceRanges, ipv6Ranges, diff) } }) }