From 7d947867ccce4f1dc64211964301b2cbef04990a Mon Sep 17 00:00:00 2001 From: Wantong Jiang Date: Tue, 9 Apr 2024 10:09:32 +0000 Subject: [PATCH] allow space separated load balancer source ranges --- pkg/provider/loadbalancer/configuration.go | 75 ++++++------- .../loadbalancer/configuration_test.go | 102 +++++++----------- 2 files changed, 71 insertions(+), 106 deletions(-) diff --git a/pkg/provider/loadbalancer/configuration.go b/pkg/provider/loadbalancer/configuration.go index f1f0f28392..04d04d4481 100644 --- a/pkg/provider/loadbalancer/configuration.go +++ b/pkg/provider/loadbalancer/configuration.go @@ -46,14 +46,18 @@ func AllowedServiceTags(svc *v1.Service) ([]string, error) { return nil, nil } - return strings.Split(strings.TrimSpace(value), Sep), nil + tags := strings.Split(strings.TrimSpace(value), Sep) + for i := range tags { + tags[i] = strings.TrimSpace(tags[i]) + } + return tags, nil } -// AllowedIPRanges returns the allowed IP ranges configured by user through AKS custom annotation. +// AllowedIPRanges returns the allowed IP ranges configured by user through AKS custom annotations: +// service.beta.kubernetes.io/azure-allowed-ip-ranges and service.beta.kubernetes.io/load-balancer-source-ranges func AllowedIPRanges(svc *v1.Service) ([]netip.Prefix, []string, error) { const ( Sep = "," - Key = consts.ServiceAnnotationAllowedIPRanges ) var ( errs []error @@ -61,61 +65,44 @@ func AllowedIPRanges(svc *v1.Service) ([]netip.Prefix, []string, error) { invalidRanges []string ) - value, found := svc.Annotations[Key] - if !found { - return nil, nil, nil - } + for _, key := range []string{consts.ServiceAnnotationAllowedIPRanges, v1.AnnotationLoadBalancerSourceRangesKey} { + value, found := svc.Annotations[key] + if !found { + continue + } - for _, p := range strings.Split(strings.TrimSpace(value), Sep) { - prefix, err := iputil.ParsePrefix(p) - if err != nil { - errs = append(errs, err) - invalidRanges = append(invalidRanges, p) - } else { - validRanges = append(validRanges, prefix) + var errsByKey []error + for _, p := range strings.Split(strings.TrimSpace(value), Sep) { + p = strings.TrimSpace(p) + prefix, err := iputil.ParsePrefix(p) + if err != nil { + errsByKey = append(errsByKey, err) + invalidRanges = append(invalidRanges, p) + } else { + validRanges = append(validRanges, prefix) + } + } + if len(errsByKey) > 0 { + errs = append(errs, NewErrAnnotationValue(key, value, errors.Join(errsByKey...))) } } + if len(errs) > 0 { - return validRanges, invalidRanges, NewErrAnnotationValue(Key, value, errors.Join(errs...)) + return validRanges, invalidRanges, errors.Join(errs...) } return validRanges, invalidRanges, nil } -// SourceRanges returns the allowed IP ranges configured by user through `spec.LoadBalancerSourceRanges` and standard annotation. -// If `spec.LoadBalancerSourceRanges` is not set, it will try to parse the annotation. +// SourceRanges returns the allowed IP ranges configured by user through `spec.LoadBalancerSourceRanges`. func SourceRanges(svc *v1.Service) ([]netip.Prefix, []string, error) { var ( errs []error validRanges []netip.Prefix invalidRanges []string ) - // Read from spec - if len(svc.Spec.LoadBalancerSourceRanges) > 0 { - for _, p := range svc.Spec.LoadBalancerSourceRanges { - prefix, err := iputil.ParsePrefix(p) - if err != nil { - errs = append(errs, err) - invalidRanges = append(invalidRanges, p) - } else { - validRanges = append(validRanges, prefix) - } - } - if len(errs) > 0 { - return validRanges, invalidRanges, fmt.Errorf("invalid service.Spec.LoadBalancerSourceRanges [%v]: %w", svc.Spec.LoadBalancerSourceRanges, errors.Join(errs...)) - } - return validRanges, invalidRanges, nil - } - // Read from annotation - const ( - Sep = "," - Key = v1.AnnotationLoadBalancerSourceRangesKey - ) - value, found := svc.Annotations[Key] - if !found { - return nil, nil, nil - } - for _, p := range strings.Split(strings.TrimSpace(value), Sep) { + for _, p := range svc.Spec.LoadBalancerSourceRanges { + p = strings.TrimSpace(p) prefix, err := iputil.ParsePrefix(p) if err != nil { errs = append(errs, err) @@ -125,7 +112,7 @@ func SourceRanges(svc *v1.Service) ([]netip.Prefix, []string, error) { } } if len(errs) > 0 { - return validRanges, invalidRanges, NewErrAnnotationValue(Key, value, errors.Join(errs...)) + return validRanges, invalidRanges, fmt.Errorf("invalid service.Spec.LoadBalancerSourceRanges [%v]: %w", svc.Spec.LoadBalancerSourceRanges, errors.Join(errs...)) } return validRanges, invalidRanges, nil } diff --git a/pkg/provider/loadbalancer/configuration_test.go b/pkg/provider/loadbalancer/configuration_test.go index 1b794c11e5..694149ba2c 100644 --- a/pkg/provider/loadbalancer/configuration_test.go +++ b/pkg/provider/loadbalancer/configuration_test.go @@ -102,7 +102,7 @@ func TestAllowedServiceTags(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - consts.ServiceAnnotationAllowedServiceTags: "Microsoft.ContainerInstance/containerGroups,foo,bar", + consts.ServiceAnnotationAllowedServiceTags: " Microsoft.ContainerInstance/containerGroups, foo, bar ", }, }, }) @@ -125,7 +125,7 @@ func TestAllowedIPRanges(t *testing.T) { assert.Empty(t, actual) assert.Empty(t, invalid) }) - t.Run("with 1 IPv4 range", func(t *testing.T) { + t.Run("with 1 IPv4 range in allowed ip ranges", func(t *testing.T) { actual, invalid, err := AllowedIPRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, @@ -140,7 +140,22 @@ func TestAllowedIPRanges(t *testing.T) { assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("10.10.0.0/24")}, actual) assert.Empty(t, invalid) }) - t.Run("with 1 IPv6 range", func(t *testing.T) { + t.Run("with 1 IPv4 range in load balancer source ranges", func(t *testing.T) { + actual, invalid, err := AllowedIPRanges(&v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + v1.AnnotationLoadBalancerSourceRangesKey: "10.10.0.0/24", + }, + }, + }) + assert.NoError(t, err) + assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("10.10.0.0/24")}, actual) + assert.Empty(t, invalid) + }) + t.Run("with 1 IPv6 range in allowed ip ranges", func(t *testing.T) { actual, invalid, err := AllowedIPRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, @@ -155,6 +170,21 @@ func TestAllowedIPRanges(t *testing.T) { assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("2001:db8::/32")}, actual) assert.Empty(t, invalid) }) + t.Run("with 1 IPv6 range in load balancer source ranges", func(t *testing.T) { + actual, invalid, err := AllowedIPRanges(&v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + v1.AnnotationLoadBalancerSourceRangesKey: "2001:db8::/32", + }, + }, + }) + assert.NoError(t, err) + assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("2001:db8::/32")}, actual) + assert.Empty(t, invalid) + }) t.Run("with multiple IP ranges", func(t *testing.T) { actual, invalid, err := AllowedIPRanges(&v1.Service{ Spec: v1.ServiceSpec{ @@ -162,7 +192,8 @@ func TestAllowedIPRanges(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - consts.ServiceAnnotationAllowedIPRanges: "10.10.0.0/24,2001:db8::/32", + consts.ServiceAnnotationAllowedIPRanges: " 10.10.0.0/24, 2001:db8::/32 ", + v1.AnnotationLoadBalancerSourceRangesKey: " 10.20.0.0/24, 2002:db8::/32 ", }, }, }) @@ -170,6 +201,8 @@ func TestAllowedIPRanges(t *testing.T) { assert.Equal(t, []netip.Prefix{ netip.MustParsePrefix("10.10.0.0/24"), netip.MustParsePrefix("2001:db8::/32"), + netip.MustParsePrefix("10.20.0.0/24"), + netip.MustParsePrefix("2002:db8::/32"), }, actual) assert.Empty(t, invalid) }) @@ -180,7 +213,8 @@ func TestAllowedIPRanges(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - consts.ServiceAnnotationAllowedIPRanges: "foobar,10.0.0.1/24", + consts.ServiceAnnotationAllowedIPRanges: "foobar,10.0.0.1/24", + v1.AnnotationLoadBalancerSourceRangesKey: "barfoo,2002:db8::1/32", }, }, }) @@ -188,8 +222,7 @@ func TestAllowedIPRanges(t *testing.T) { var e *ErrAnnotationValue assert.ErrorAs(t, err, &e) - assert.Equal(t, e.AnnotationKey, consts.ServiceAnnotationAllowedIPRanges) - assert.Equal(t, []string{"foobar", "10.0.0.1/24"}, invalid) + assert.Equal(t, []string{"foobar", "10.0.0.1/24", "barfoo", "2002:db8::1/32"}, invalid) }) } @@ -218,42 +251,6 @@ func TestSourceRanges(t *testing.T) { }, actual) assert.Empty(t, invalid) }) - t.Run("specified in annotation", func(t *testing.T) { - actual, invalid, err := SourceRanges(&v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeLoadBalancer, - }, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - v1.AnnotationLoadBalancerSourceRangesKey: "10.10.0.0/24,2001:db8::/32", - }, - }, - }) - assert.NoError(t, err) - assert.Equal(t, []netip.Prefix{ - netip.MustParsePrefix("10.10.0.0/24"), - netip.MustParsePrefix("2001:db8::/32"), - }, actual) - assert.Empty(t, invalid) - }) - t.Run("specified in both spec and annotation", func(t *testing.T) { - actual, invalid, err := SourceRanges(&v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeLoadBalancer, - LoadBalancerSourceRanges: []string{"10.10.0.0/24"}, - }, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - v1.AnnotationLoadBalancerSourceRangesKey: "2001:db8::/32", - }, - }, - }) - assert.NoError(t, err) - assert.Equal(t, []netip.Prefix{ - netip.MustParsePrefix("10.10.0.0/24"), - }, actual, "spec should take precedence over annotation") - assert.Empty(t, invalid) - }) t.Run("with invalid IP range in spec", func(t *testing.T) { _, invalid, err := SourceRanges(&v1.Service{ Spec: v1.ServiceSpec{ @@ -267,25 +264,6 @@ func TestSourceRanges(t *testing.T) { assert.Error(t, err) assert.Equal(t, []string{"foobar", "10.0.0.1/24"}, invalid) }) - - t.Run("with invalid IP range in annotation", func(t *testing.T) { - _, invalid, err := SourceRanges(&v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeLoadBalancer, - }, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - v1.AnnotationLoadBalancerSourceRangesKey: "foobar,10.0.0.1/24", - }, - }, - }) - assert.Error(t, err) - - var e *ErrAnnotationValue - assert.ErrorAs(t, err, &e) - assert.Equal(t, e.AnnotationKey, v1.AnnotationLoadBalancerSourceRangesKey) - assert.Equal(t, []string{"foobar", "10.0.0.1/24"}, invalid) - }) } func TestAdditionalPublicIPs(t *testing.T) {