diff --git a/pkg/provider/loadbalancer/accesscontrol.go b/pkg/provider/loadbalancer/accesscontrol.go index 6cbf13bf0d..41fcaedbc1 100644 --- a/pkg/provider/loadbalancer/accesscontrol.go +++ b/pkg/provider/loadbalancer/accesscontrol.go @@ -48,14 +48,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 @@ -63,28 +67,35 @@ 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 := ParseCIDR(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 := ParseCIDR(p) + if err != nil { + errsByKey = append(errsByKey, err) + invalidRanges = append(invalidRanges, p) + } else { + validRanges = append(validRanges, prefix) + } + } + if len(errsByKey) > 0 { + errs = append(errs, fmt.Errorf("invalid service annotation %s:%s: %w", key, value, errors.Join(errsByKey...))) } } + if len(errs) > 0 { - return validRanges, invalidRanges, fmt.Errorf("invalid service annotation %s:%s: %w", 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 @@ -92,32 +103,8 @@ func SourceRanges(svc *v1.Service) ([]netip.Prefix, []string, error) { invalidRanges []string ) // Read from spec - if len(svc.Spec.LoadBalancerSourceRanges) > 0 { - for _, p := range svc.Spec.LoadBalancerSourceRanges { - prefix, err := ParseCIDR(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 := ParseCIDR(p) if err != nil { errs = append(errs, err) @@ -127,7 +114,7 @@ func SourceRanges(svc *v1.Service) ([]netip.Prefix, []string, error) { } } if len(errs) > 0 { - return validRanges, invalidRanges, fmt.Errorf("invalid service annotation %s:%s: %w", 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/accesscontrol_test.go b/pkg/provider/loadbalancer/accesscontrol_test.go index d0ddb01157..6084f0a4c6 100644 --- a/pkg/provider/loadbalancer/accesscontrol_test.go +++ b/pkg/provider/loadbalancer/accesscontrol_test.go @@ -143,7 +143,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 ", }, }, }) @@ -166,7 +166,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, @@ -181,7 +181,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, @@ -196,6 +211,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{ @@ -203,7 +233,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 ", }, }, }) @@ -211,6 +242,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) }) @@ -221,12 +254,13 @@ func TestAllowedIPRanges(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - consts.ServiceAnnotationAllowedIPRanges: "foobar", + consts.ServiceAnnotationAllowedIPRanges: "foobar,10.0.0.1/24", + v1.AnnotationLoadBalancerSourceRangesKey: "barfoo,2002:db8::1/32", }, }, }) assert.Error(t, err) - assert.Equal(t, []string{"foobar"}, invalid) + assert.Equal(t, []string{"foobar", "10.0.0.1/24", "barfoo", "2002:db8::1/32"}, invalid) }) } @@ -255,51 +289,15 @@ 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", func(t *testing.T) { + t.Run("with invalid IP range in spec", func(t *testing.T) { _, invalid, err := SourceRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, - LoadBalancerSourceRanges: []string{"foobar"}, + LoadBalancerSourceRanges: []string{"foobar", "10.0.0.1/24"}, }, }) assert.Error(t, err) - assert.Equal(t, []string{"foobar"}, invalid) + assert.Equal(t, []string{"foobar", "10.0.0.1/24"}, invalid) }) }