Merge pull request #1896 from panslava/rework-source-ranges

Reimplement getting load balancer source ranges

pkg/loadbalancers/l4.go

@@ -482,15 +482,15 @@ func (l4 *L4) ensureIPv4NodesFirewall(nodeNames []string, ipAddress string, resu
 	}()
 
 	// ensure firewalls
-	sourceRanges, err := helpers.GetLoadBalancerSourceRanges(l4.Service)
+	sourceRanges, err := utils.ServiceSourceRanges(l4.Service)
 	if err != nil {
 		result.Error = err
 		return
 	}
 	// Add firewall rule for ILB traffic to nodes
 	nodesFWRParams := firewalls.FirewallParams{
 		PortRanges:        portRanges,
-		SourceRanges:      sourceRanges.StringSlice(),
+		SourceRanges:      sourceRanges,
 		DestinationRanges: []string{ipAddress},
 		Protocol:          string(protocol),
 		Name:              firewallName,

pkg/loadbalancers/l4_test.go

@@ -2155,11 +2155,11 @@ func verifyILBIPv4NodesFirewall(l4 *L4, nodeNames []string) error {
 		return fmt.Errorf("failed to create description for resources, err %w", err)
 	}
 
-	sourceRanges, err := servicehelper.GetLoadBalancerSourceRanges(l4.Service)
+	sourceRanges, err := utils.ServiceSourceRanges(l4.Service)
 	if err != nil {
 		return fmt.Errorf("servicehelper.GetLoadBalancerSourceRanges(%+v) returned error %v, want nil", l4.Service, err)
 	}
-	return verifyFirewall(l4.cloud, nodeNames, fwName, fwDesc, sourceRanges.StringSlice())
+	return verifyFirewall(l4.cloud, nodeNames, fwName, fwDesc, sourceRanges)
 }
 
 func verifyILBIPv6NodesFirewall(l4 *L4, nodeNames []string) error {

pkg/loadbalancers/l4netlb.go

@@ -284,7 +284,7 @@ func (l4netlb *L4NetLB) ensureNodesFirewall(name string, nodeNames []string, ipA
 	}()
 
 	result := &L4NetLBSyncResult{}
-	sourceRanges, err := helpers.GetLoadBalancerSourceRanges(l4netlb.Service)
+	sourceRanges, err := utils.ServiceSourceRanges(l4netlb.Service)
 	if err != nil {
 		result.Error = err
 		return result
@@ -293,7 +293,7 @@ func (l4netlb *L4NetLB) ensureNodesFirewall(name string, nodeNames []string, ipA
 	// Add firewall rule for L4 External LoadBalancer traffic to nodes
 	nodesFWRParams := firewalls.FirewallParams{
 		PortRanges:        portRanges,
-		SourceRanges:      sourceRanges.StringSlice(),
+		SourceRanges:      sourceRanges,
 		DestinationRanges: []string{ipAddress},
 		Protocol:          protocol,
 		Name:              name,

pkg/loadbalancers/l4netlb_test.go

@@ -331,11 +331,11 @@ func verifyNetLBNodesFirewall(l4netlb *L4NetLB, nodeNames []string) error {
 		return fmt.Errorf("failed to create description for resources, err %w", err)
 	}
 
-	sourceRanges, err := servicehelper.GetLoadBalancerSourceRanges(l4netlb.Service)
+	sourceRanges, err := utils.ServiceSourceRanges(l4netlb.Service)
 	if err != nil {
 		return fmt.Errorf("servicehelper.GetLoadBalancerSourceRanges(%+v) returned error %v, want nil", l4netlb.Service, err)
 	}
-	return verifyFirewall(l4netlb.cloud, nodeNames, fwName, fwDesc, sourceRanges.StringSlice())
+	return verifyFirewall(l4netlb.cloud, nodeNames, fwName, fwDesc, sourceRanges)
 }
 
 func verifyNetLBHealthCheckFirewall(l4netlb *L4NetLB, nodeNames []string) error {

pkg/utils/sourceranges.go

@@ -0,0 +1,90 @@
+package utils
+
+import (
+	"fmt"
+	"strings"
+
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/utils/net"
+)
+
+const (
+	allowAllIPv4Range = "0.0.0.0/0"
+)
+
+func ServiceSourceRanges(service *v1.Service) ([]string, error) {
+	ipRanges, err := getAllSourceRanges(service)
+	if err != nil {
+		return nil, err
+	}
+
+	if len(ipRanges) == 0 {
+		return []string{allowAllIPv4Range}, nil
+	}
+	return ipRanges.StringSlice(), nil
+}
+
+func getAllSourceRanges(service *v1.Service) (net.IPNetSet, error) {
+	// if SourceRange field is specified, ignore sourceRange annotation
+	if len(service.Spec.LoadBalancerSourceRanges) > 0 {
+		specs := service.Spec.LoadBalancerSourceRanges
+		for i, spec := range specs {
+			specs[i] = strings.TrimSpace(spec)
+		}
+		ipnets, err := net.ParseIPNets(specs...)
+		if err != nil {
+			return nil, NewInvalidSpecLoadBalancerSourceRangesError(specs, err)
+		}
+		return ipnets, err
+	}
+
+	val, ok := service.Annotations[v1.AnnotationLoadBalancerSourceRangesKey]
+	if !ok {
+		return nil, nil
+	}
+	specs := strings.Split(val, ",")
+	for i, spec := range specs {
+		specs[i] = strings.TrimSpace(spec)
+	}
+	ipnets, err := net.ParseIPNets(specs...)
+	if err != nil {
+		return nil, NewInvalidLoadBalancerSourceRangesAnnotationError(val, err)
+	}
+	return ipnets, nil
+}
+
+// InvalidLoadBalancerSourceRangesSpecError is a struct to define error caused by
+// User misconfiguration of the Service.Spec.LoadBalancerSourceRanges field.
+type InvalidLoadBalancerSourceRangesSpecError struct {
+	LoadBalancerSourceRangesSpec []string
+	ParseErr                     error
+}
+
+func (e *InvalidLoadBalancerSourceRangesSpecError) Error() string {
+	return fmt.Sprintf("service.Spec.LoadBalancerSourceRanges: %v is not valid. Expecting a list of IP ranges. For example, 10.0.0.0/24. Error msg: %v", e.LoadBalancerSourceRangesSpec, e.ParseErr)
+}
+
+func NewInvalidSpecLoadBalancerSourceRangesError(specLoadBalancerSourceRanges []string, err error) *InvalidLoadBalancerSourceRangesSpecError {
+	return &InvalidLoadBalancerSourceRangesSpecError{
+		specLoadBalancerSourceRanges,
+		err,
+	}
+}
+
+// InvalidLoadBalancerSourceRangesAnnotationError is a struct to define error caused by
+// User misconfiguration of the Service.Annotations["service.beta.kubernetes.io/load-balancer-source-ranges"] field.
+type InvalidLoadBalancerSourceRangesAnnotationError struct {
+	LoadBalancerSourceRangesAnnotation string
+	ParseErr                           error
+}
+
+func (e *InvalidLoadBalancerSourceRangesAnnotationError) Error() string {
+	return fmt.Sprintf("Service annotation %s: %s is not valid. Expecting a comma-separated list of source IP ranges. For example, 10.0.0.0/24,192.168.2.0/24", v1.AnnotationLoadBalancerSourceRangesKey, e.LoadBalancerSourceRangesAnnotation)
+}
+
+func NewInvalidLoadBalancerSourceRangesAnnotationError(serviceLoadBalancerSourceRangesAnnotation string, err error) *InvalidLoadBalancerSourceRangesAnnotationError {
+	return &InvalidLoadBalancerSourceRangesAnnotationError{
+		serviceLoadBalancerSourceRangesAnnotation,
+		err,
+	}
+}

pkg/utils/sourceranges_test.go

@@ -0,0 +1,90 @@
+package utils
+
+import (
+	"net"
+	"sort"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestServiceSourceRanges(t *testing.T) {
+	testCases := []struct {
+		desc                 string
+		specRanges           []string
+		annotations          map[string]string
+		expectedSourceRanges []string
+		expectError          error
+	}{
+		{
+			desc:                 "Should return allow all for no specs or annotations",
+			expectedSourceRanges: []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 parse ranges from annotations, if no spec value",
+			annotations: map[string]string{
+				v1.AnnotationLoadBalancerSourceRangesKey: " 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 ignore annotation if spec is present",
+			specRanges: []string{"192.168.0.1/10", "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
+		},
+		{
+			desc:       "Should return special error for invalid spec value",
+			specRanges: []string{"1.0.1.2"}, // wrong CIDR, because no mask
+			expectError: &InvalidLoadBalancerSourceRangesSpecError{
+				LoadBalancerSourceRangesSpec: []string{"1.0.1.2"},
+				ParseErr:                     &net.ParseError{Type: "CIDR address", Text: "1.0.1.2"},
+			},
+		},
+		{
+			desc: "Should return special error for invalid annotation value",
+			annotations: map[string]string{
+				v1.AnnotationLoadBalancerSourceRangesKey: "1.2.3.4 1.2.3.4", // should be comma-separated
+			},
+			expectError: &InvalidLoadBalancerSourceRangesAnnotationError{
+				LoadBalancerSourceRangesAnnotation: "1.2.3.4 1.2.3.4",
+				ParseErr:                           &net.ParseError{Type: "CIDR address", Text: "1.2.3.4 1.2.3.4"},
+			},
+		},
+	}
+
+	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,
+				},
+			}
+
+			sourceRanges, err := ServiceSourceRanges(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)
+			if diff != "" {
+				t.Errorf("Expected source ranges: %v, got ranges %v, diff: %s", tc.expectedSourceRanges, sourceRanges, diff)
+			}
+		})
+	}
+}

pkg/utils/utils.go

@@ -218,11 +218,26 @@ func IsIPConfigurationError(err error) bool {
 	return errors.As(err, &ipConfigError)
 }
 
+// IsInvalidLoadBalancerSourceRangesSpecError checks if wrapped error is an InvalidLoadBalancerSourceRangesSpecError error.
+func IsInvalidLoadBalancerSourceRangesSpecError(err error) bool {
+	var invalidLoadBalancerSourceRangesSpecError *InvalidLoadBalancerSourceRangesSpecError
+	return errors.As(err, &invalidLoadBalancerSourceRangesSpecError)
+}
+
+// IsInvalidLoadBalancerSourceRangesAnnotationError checks if wrapped error is an InvalidLoadBalancerSourceRangesAnnotationError error.
+func IsInvalidLoadBalancerSourceRangesAnnotationError(err error) bool {
+	var invalidLoadBalancerSourceRangesAnnotationError *InvalidLoadBalancerSourceRangesAnnotationError
+	return errors.As(err, &invalidLoadBalancerSourceRangesAnnotationError)
+}
+
 // IsUserError checks if given error is cause by User.
 // Right now User Error might be caused by Network Tier misconfiguration
 // or specifying non-existent or already used IP address.
 func IsUserError(err error) bool {
-	return IsNetworkTierError(err) || IsIPConfigurationError(err)
+	return IsNetworkTierError(err) ||
+		IsIPConfigurationError(err) ||
+		IsInvalidLoadBalancerSourceRangesSpecError(err) ||
+		IsInvalidLoadBalancerSourceRangesAnnotationError(err)
 }
 
 // IsNotFoundError returns true if the resource does not exist