diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go index c4a4d2c506..5d25ce124e 100644 --- a/pkg/provider/azure_loadbalancer.go +++ b/pkg/provider/azure_loadbalancer.go @@ -2856,6 +2856,13 @@ func (az *Cloud) reconcileSecurityGroup( consts.ServiceAnnotationAllowedIPRanges, consts.ServiceAnnotationAllowedServiceTags, )) } + + if len(accessControl.InvalidRanges) > 0 { + az.Event(service, v1.EventTypeWarning, "InvalidConfiguration", fmt.Sprintf( + "Found invalid LoadBalancerSourceRanges %v, ignoring and adding a default DenyAll rule in security group.", + accessControl.InvalidRanges, + )) + } } var ( diff --git a/pkg/provider/loadbalancer/accesscontrol.go b/pkg/provider/loadbalancer/accesscontrol.go index 05e393d6dc..fcdb92c92d 100644 --- a/pkg/provider/loadbalancer/accesscontrol.go +++ b/pkg/provider/loadbalancer/accesscontrol.go @@ -46,6 +46,7 @@ type AccessControl struct { // immutable pre-compute states. SourceRanges []netip.Prefix AllowedIPRanges []netip.Prefix + InvalidRanges []string AllowedServiceTags []string securityRuleDestinationPortsByProtocol map[network.SecurityRuleProtocol][]int32 } @@ -82,20 +83,17 @@ func NewAccessControl(svc *v1.Service, sg *network.SecurityGroup, opts ...Access logger.Error(err, "Failed to initialize RuleHelper") return nil, err } - sourceRanges, err := SourceRanges(svc) + sourceRanges, invalidSourceRanges, err := SourceRanges(svc) if err != nil && !options.SkipAnnotationValidation { logger.Error(err, "Failed to parse SourceRange configuration") - return nil, err } - allowedIPRanges, err := AllowedIPRanges(svc) + allowedIPRanges, invalidAllowedIPRanges, err := AllowedIPRanges(svc) if err != nil && !options.SkipAnnotationValidation { logger.Error(err, "Failed to parse AllowedIPRanges configuration") - return nil, err } allowedServiceTags, err := AllowedServiceTags(svc) if err != nil && !options.SkipAnnotationValidation { logger.Error(err, "Failed to parse AllowedServiceTags configuration") - return nil, err } securityRuleDestinationPortsByProtocol, err := securityRuleDestinationPortsByProtocol(svc) if err != nil { @@ -114,13 +112,14 @@ func NewAccessControl(svc *v1.Service, sg *network.SecurityGroup, opts ...Access SourceRanges: sourceRanges, AllowedIPRanges: allowedIPRanges, AllowedServiceTags: allowedServiceTags, + InvalidRanges: append(invalidSourceRanges, invalidAllowedIPRanges...), securityRuleDestinationPortsByProtocol: securityRuleDestinationPortsByProtocol, }, nil } // IsAllowFromInternet returns true if the given service is allowed to be accessed from internet. // To be specific, -// 1. For all types of LB, it returns false if the given service is specified with `service tags` or `not allowed all IP ranges`. +// 1. For all types of LB, it returns false if the given service is specified with `service tags` or `not allowed all IP ranges`, including invalid IP ranges. // 2. For internal LB, it returns true iff the given service is explicitly specified with `allowed all IP ranges`. Refer: https://github.com/kubernetes-sigs/cloud-provider-azure/issues/698 func (ac *AccessControl) IsAllowFromInternet() bool { if len(ac.AllowedServiceTags) > 0 { @@ -132,6 +131,9 @@ func (ac *AccessControl) IsAllowFromInternet() bool { if len(ac.AllowedIPRanges) > 0 && !iputil.IsPrefixesAllowAll(ac.AllowedIPRanges) { return false } + if len(ac.InvalidRanges) > 0 { + return false + } if !IsInternal(ac.svc) { return true } @@ -143,10 +145,11 @@ func (ac *AccessControl) IsAllowFromInternet() bool { // By default, NSG allow traffic from the VNet. func (ac *AccessControl) DenyAllExceptSourceRanges() bool { var ( - annotationEnabled = strings.EqualFold(ac.svc.Annotations[consts.ServiceAnnotationDenyAllExceptLoadBalancerSourceRanges], "true") - sourceRangeSpecified = len(ac.SourceRanges) > 0 || len(ac.AllowedIPRanges) > 0 + annotationEnabled = strings.EqualFold(ac.svc.Annotations[consts.ServiceAnnotationDenyAllExceptLoadBalancerSourceRanges], "true") + sourceRangeSpecified = len(ac.SourceRanges) > 0 || len(ac.AllowedIPRanges) > 0 + invalidRangesSpecified = len(ac.InvalidRanges) > 0 ) - return annotationEnabled && sourceRangeSpecified + return (annotationEnabled && sourceRangeSpecified) || invalidRangesSpecified } // AllowedIPv4Ranges returns the IPv4 ranges that are allowed to access the LoadBalancer. diff --git a/pkg/provider/loadbalancer/accesscontrol_test.go b/pkg/provider/loadbalancer/accesscontrol_test.go index 820e883ee3..6c7960caf6 100644 --- a/pkg/provider/loadbalancer/accesscontrol_test.go +++ b/pkg/provider/loadbalancer/accesscontrol_test.go @@ -53,6 +53,18 @@ func TestAccessControl_IsAllowFromInternet(t *testing.T) { svc: k8sFx.Service().WithInternalEnabled().Build(), expectedOutput: false, }, + { + name: "public LB with invalid access control configuration", + svc: k8sFx.Service().WithLoadBalancerSourceRanges("10.10.10.1/24").Build(), + expectedOutput: false, + }, + { + name: "internal LB with invalid access control configuration", + svc: k8sFx.Service().WithInternalEnabled(). + WithLoadBalancerSourceRanges("10.10.10.1/24"). + Build(), + expectedOutput: false, + }, { name: "public LB with spec.LoadBalancerSourceRanges specified but not allow all", svc: k8sFx.Service().WithLoadBalancerSourceRanges("10.10.10.0/24").Build(), @@ -170,6 +182,11 @@ func TestAccessControl_DenyAllExceptSourceRanges(t *testing.T) { Build(), expectedOutput: true, }, + { + name: "without annotation but invalid allowedIPRanges specified", + svc: k8sFx.Service().WithAllowedIPRanges("10.0.0.1/24").Build(), + expectedOutput: true, + }, } for i := range tests { @@ -223,34 +240,57 @@ func TestAccessControl_AllowedRanges(t *testing.T) { { name: "spec.LoadBalancerSourceRanges = 1 IPv6", svc: k8sFx.Service().WithLoadBalancerSourceRanges( - "2001:db8:85a3::1a2b:3c4d/64", + "2001:db8:85a3::/64", ).Build(), expectedIPv6: []string{ - "2001:db8:85a3::1a2b:3c4d/64", + "2001:db8:85a3::/64", }, }, { name: "spec.LoadBalancerSourceRanges = N IPv6", svc: k8sFx.Service().WithLoadBalancerSourceRanges( - "2001:db8:85a3::1a2b:3c4d/64", - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "fe80:1234::/32", ).Build(), expectedIPv6: []string{ - "2001:db8:85a3::1a2b:3c4d/64", - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "fe80:1234::/32", }, }, { name: "spec.LoadBalancerSourceRanges = N IPv4 and N IPv6", svc: k8sFx.Service().WithLoadBalancerSourceRanges( "10.10.0.0/16", - "2001:db8:85a3::1a2b:3c4d/64", - "fd12:3456:789a::1234:5678/48", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "10.11.0.0/16", + "10.0.0.1/32", + "fe80:1234:abcd::1c2d:3e4f/128", + ).Build(), + expectedIPv4: []string{ + "10.10.0.0/16", + "10.11.0.0/16", + "10.0.0.1/32", + }, + expectedIPv6: []string{ + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", + }, + }, + { + name: "spec.LoadBalancerSourceRanges = N IPv4 and N IPv6 and invalid ranges", + svc: k8sFx.Service().WithLoadBalancerSourceRanges( + "10.10.0.0/16", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", "10.11.0.0/16", "10.0.0.1/32", - "fe80:1234:abcd::1c2d:3e4f/32", + "fe80:1234:abcd::1c2d:3e4f/128", + "20.10.0.1/16", // invalid + "fe80:1234:abcd::1c2d:3e4f/64", // invalid ).Build(), expectedIPv4: []string{ "10.10.0.0/16", @@ -258,9 +298,9 @@ func TestAccessControl_AllowedRanges(t *testing.T) { "10.0.0.1/32", }, expectedIPv6: []string{ - "2001:db8:85a3::1a2b:3c4d/64", - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", }, }, { @@ -287,34 +327,34 @@ func TestAccessControl_AllowedRanges(t *testing.T) { { name: "annotation allowedIPRanges = 1 IPv6", svc: k8sFx.Service().WithAllowedIPRanges( - "2001:db8:85a3::1a2b:3c4d/64", + "2001:db8:85a3::/64", ).Build(), expectedIPv6: []string{ - "2001:db8:85a3::1a2b:3c4d/64", + "2001:db8:85a3::/64", }, }, { name: "annotation allowedIPRanges = N IPv6", svc: k8sFx.Service().WithAllowedIPRanges( - "2001:db8:85a3::1a2b:3c4d/64", - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", ).Build(), expectedIPv6: []string{ - "2001:db8:85a3::1a2b:3c4d/64", - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", }, }, { name: "annotation allowedIPRanges = N IPv4 and N IPv6", svc: k8sFx.Service().WithAllowedIPRanges( "10.10.0.0/16", - "2001:db8:85a3::1a2b:3c4d/64", - "fd12:3456:789a::1234:5678/48", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", "10.11.0.0/16", "10.0.0.1/32", - "fe80:1234:abcd::1c2d:3e4f/32", + "fe80:1234:abcd::1c2d:3e4f/128", ).Build(), expectedIPv4: []string{ "10.10.0.0/16", @@ -322,9 +362,32 @@ func TestAccessControl_AllowedRanges(t *testing.T) { "10.0.0.1/32", }, expectedIPv6: []string{ - "2001:db8:85a3::1a2b:3c4d/64", - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", + }, + }, + { + name: "annotation allowedIPRanges = N IPv4 and N IPv6 with invalid ranges", + svc: k8sFx.Service().WithAllowedIPRanges( + "10.10.0.0/16", + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "10.11.0.0/16", + "10.0.0.1/32", + "fe80:1234:abcd::1c2d:3e4f/128", + "20.10.0.1/16", // invalid + "fe80:1234:abcd::1c2d:3e4f/64", // invalid + ).Build(), + expectedIPv4: []string{ + "10.10.0.0/16", + "10.11.0.0/16", + "10.0.0.1/32", + }, + expectedIPv6: []string{ + "2001:db8:85a3::/64", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", }, }, } @@ -570,8 +633,8 @@ func TestAccessControl_PatchSecurityGroup(t *testing.T) { var ( k8sFx = fixture.NewFixture().Kubernetes() allowedIPRanges = []string{ - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", } svc = k8sFx.Service(). WithAllowedIPRanges(allowedIPRanges...). @@ -616,8 +679,8 @@ func TestAccessControl_PatchSecurityGroup(t *testing.T) { "20.0.0.1/32", } allowedIPv6Ranges = []string{ - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", } svc = k8sFx.Service(). WithAllowedIPRanges(append(allowedIPv4Ranges, allowedIPv6Ranges...)...). @@ -680,8 +743,8 @@ func TestAccessControl_PatchSecurityGroup(t *testing.T) { "20.0.0.1/32", } allowedIPv6Ranges = []string{ - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", } svc = k8sFx.Service(). WithAllowedIPRanges(append(allowedIPv4Ranges, allowedIPv6Ranges...)...). @@ -804,8 +867,8 @@ func TestAccessControl_PatchSecurityGroup(t *testing.T) { "20.0.0.1/32", } allowedIPv6Ranges = []string{ - "fd12:3456:789a::1234:5678/48", - "fe80:1234:abcd::1c2d:3e4f/32", + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", } svc = k8sFx.Service(). WithAllowedIPRanges(append(allowedIPv4Ranges, allowedIPv6Ranges...)...). @@ -929,6 +992,87 @@ func TestAccessControl_PatchSecurityGroup(t *testing.T) { ) runTest(t, svc, originalRules, dstIPv4Addresses, dstIPv6Addresses, true, expectedRules) }) + + t.Run("patch service with invalid allowedIPRanges", func(t *testing.T) { + var ( + k8sFx = fixture.NewFixture().Kubernetes() + inputIPv4Ranges = []string{ + "192.168.0.0/16", + "20.0.0.1/32", + "10.0.0.1/16", // invalid + } + allowedIPv4Ranges = inputIPv4Ranges[:2] + inputIPv6Ranges = []string{ + "fd12:3456:789a::/48", + "fe80:1234:abcd::1c2d:3e4f/128", + "fe80:1234:abcd::3e4f/64", // invalid + } + allowedIPv6Ranges = inputIPv6Ranges[:2] + svc = k8sFx.Service(). + WithAllowedIPRanges(append(inputIPv4Ranges, inputIPv6Ranges...)...). + WithDenyAllExceptLoadBalancerSourceRanges(). + Build() + originalRules = azureFx.NoiseSecurityRules(10) + dstIPv4Addresses = []string{ + "10.0.0.1", + "10.0.0.2", + } + dstIPv6Addresses = []string{ + "2001:db8::1428:57ab", + "2002:fb8::1", + } + expectedRules = testutil.CloneInJSON(originalRules) + ) + expectedRules = append(expectedRules, + + // TCP + IPv4 for 1 IP Range + azureFx. + AllowSecurityRule( + network.SecurityRuleProtocolTCP, iputil.IPv4, allowedIPv4Ranges, k8sFx.Service().TCPPorts(), + ). + WithPriority(500). + WithDestination(dstIPv4Addresses...). + Build(), + + // TCP + IPv6 for 1 IP Range + azureFx. + AllowSecurityRule( + network.SecurityRuleProtocolTCP, iputil.IPv6, allowedIPv6Ranges, k8sFx.Service().TCPPorts(), + ). + WithPriority(501). + WithDestination(dstIPv6Addresses...). + Build(), + + // UDP + IPv4 for 1 IP Range + azureFx. + AllowSecurityRule( + network.SecurityRuleProtocolUDP, iputil.IPv4, allowedIPv4Ranges, k8sFx.Service().UDPPorts(), + ). + WithPriority(502). + WithDestination(dstIPv4Addresses...). + Build(), + + // UDP + IPv6 for 1 IP Range + azureFx. + AllowSecurityRule( + network.SecurityRuleProtocolUDP, iputil.IPv6, allowedIPv6Ranges, k8sFx.Service().UDPPorts(), + ). + WithPriority(503). + WithDestination(dstIPv6Addresses...). + Build(), + + // Deny ALL + azureFx. + DenyAllSecurityRule(iputil.IPv4).WithPriority(4095). + WithDestination(dstIPv4Addresses...). + Build(), + azureFx. + DenyAllSecurityRule(iputil.IPv6).WithPriority(4094). + WithDestination(dstIPv6Addresses...). + Build(), + ) + runTest(t, svc, originalRules, dstIPv4Addresses, dstIPv6Addresses, true, expectedRules) + }) } func TestAccessControl_CleanSecurityGroup(t *testing.T) { diff --git a/pkg/provider/loadbalancer/configuration.go b/pkg/provider/loadbalancer/configuration.go index 436bca4b11..f1f0f28392 100644 --- a/pkg/provider/loadbalancer/configuration.go +++ b/pkg/provider/loadbalancer/configuration.go @@ -17,6 +17,7 @@ limitations under the License. package loadbalancer import ( + "errors" "fmt" "net/netip" "strings" @@ -49,35 +50,60 @@ func AllowedServiceTags(svc *v1.Service) ([]string, error) { } // AllowedIPRanges returns the allowed IP ranges configured by user through AKS custom annotation. -func AllowedIPRanges(svc *v1.Service) ([]netip.Prefix, error) { +func AllowedIPRanges(svc *v1.Service) ([]netip.Prefix, []string, error) { const ( Sep = "," Key = consts.ServiceAnnotationAllowedIPRanges ) + var ( + errs []error + validRanges []netip.Prefix + invalidRanges []string + ) value, found := svc.Annotations[Key] if !found { - return nil, nil + return nil, nil, nil } - rv, err := iputil.ParsePrefixes(strings.Split(strings.TrimSpace(value), Sep)) - if err != nil { - return nil, NewErrAnnotationValue(Key, value, err) + 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) + } } - - return rv, nil + if len(errs) > 0 { + return validRanges, invalidRanges, NewErrAnnotationValue(Key, value, 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. -func SourceRanges(svc *v1.Service) ([]netip.Prefix, error) { +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 { - rv, err := iputil.ParsePrefixes(svc.Spec.LoadBalancerSourceRanges) - if err != nil { - return nil, fmt.Errorf("invalid service.Spec.LoadBalancerSourceRanges [%v]: %w", svc.Spec.LoadBalancerSourceRanges, err) + 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 rv, nil + return validRanges, invalidRanges, nil } // Read from annotation @@ -87,13 +113,21 @@ func SourceRanges(svc *v1.Service) ([]netip.Prefix, error) { ) value, found := svc.Annotations[Key] if !found { - return nil, nil + return nil, nil, nil } - rv, err := iputil.ParsePrefixes(strings.Split(strings.TrimSpace(value), Sep)) - if err != nil { - return nil, NewErrAnnotationValue(Key, value, err) + 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) + } } - return rv, nil + if len(errs) > 0 { + return validRanges, invalidRanges, NewErrAnnotationValue(Key, value, errors.Join(errs...)) + } + return validRanges, invalidRanges, nil } func AdditionalPublicIPs(svc *v1.Service) ([]netip.Addr, error) { diff --git a/pkg/provider/loadbalancer/configuration_test.go b/pkg/provider/loadbalancer/configuration_test.go index 1902512ed1..1b794c11e5 100644 --- a/pkg/provider/loadbalancer/configuration_test.go +++ b/pkg/provider/loadbalancer/configuration_test.go @@ -113,7 +113,7 @@ func TestAllowedServiceTags(t *testing.T) { func TestAllowedIPRanges(t *testing.T) { t.Run("no annotation", func(t *testing.T) { - actual, err := AllowedIPRanges(&v1.Service{ + actual, invalid, err := AllowedIPRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, }, @@ -123,9 +123,10 @@ func TestAllowedIPRanges(t *testing.T) { }) assert.NoError(t, err) assert.Empty(t, actual) + assert.Empty(t, invalid) }) t.Run("with 1 IPv4 range", func(t *testing.T) { - actual, err := AllowedIPRanges(&v1.Service{ + actual, invalid, err := AllowedIPRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, }, @@ -137,9 +138,10 @@ func TestAllowedIPRanges(t *testing.T) { }) 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", func(t *testing.T) { - actual, err := AllowedIPRanges(&v1.Service{ + actual, invalid, err := AllowedIPRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, }, @@ -151,9 +153,10 @@ func TestAllowedIPRanges(t *testing.T) { }) 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, err := AllowedIPRanges(&v1.Service{ + actual, invalid, err := AllowedIPRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, }, @@ -168,15 +171,16 @@ func TestAllowedIPRanges(t *testing.T) { netip.MustParsePrefix("10.10.0.0/24"), netip.MustParsePrefix("2001:db8::/32"), }, actual) + assert.Empty(t, invalid) }) t.Run("with invalid IP range", func(t *testing.T) { - _, err := AllowedIPRanges(&v1.Service{ + _, invalid, err := AllowedIPRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, }, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - consts.ServiceAnnotationAllowedIPRanges: "foobar", + consts.ServiceAnnotationAllowedIPRanges: "foobar,10.0.0.1/24", }, }, }) @@ -185,21 +189,23 @@ 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) }) } func TestSourceRanges(t *testing.T) { t.Run("not specified in spec", func(t *testing.T) { - actual, err := SourceRanges(&v1.Service{ + actual, invalid, err := SourceRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, }, }) assert.NoError(t, err) assert.Empty(t, actual) + assert.Empty(t, invalid) }) t.Run("specified in spec", func(t *testing.T) { - actual, err := SourceRanges(&v1.Service{ + actual, invalid, err := SourceRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, LoadBalancerSourceRanges: []string{"10.10.0.0/24", "2001:db8::/32"}, @@ -210,9 +216,10 @@ func TestSourceRanges(t *testing.T) { netip.MustParsePrefix("10.10.0.0/24"), netip.MustParsePrefix("2001:db8::/32"), }, actual) + assert.Empty(t, invalid) }) t.Run("specified in annotation", func(t *testing.T) { - actual, err := SourceRanges(&v1.Service{ + actual, invalid, err := SourceRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, }, @@ -227,9 +234,10 @@ func TestSourceRanges(t *testing.T) { 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, err := SourceRanges(&v1.Service{ + actual, invalid, err := SourceRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, LoadBalancerSourceRanges: []string{"10.10.0.0/24"}, @@ -244,28 +252,30 @@ func TestSourceRanges(t *testing.T) { 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) { - _, err := SourceRanges(&v1.Service{ + _, invalid, err := SourceRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, - LoadBalancerSourceRanges: []string{"foobar"}, + LoadBalancerSourceRanges: []string{"foobar", "10.0.0.1/24"}, }, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, }, }) 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) { - _, err := SourceRanges(&v1.Service{ + _, invalid, err := SourceRanges(&v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, }, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - v1.AnnotationLoadBalancerSourceRangesKey: "foobar", + v1.AnnotationLoadBalancerSourceRangesKey: "foobar,10.0.0.1/24", }, }, }) @@ -274,6 +284,7 @@ func TestSourceRanges(t *testing.T) { 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) }) } diff --git a/pkg/provider/loadbalancer/iputil/prefix.go b/pkg/provider/loadbalancer/iputil/prefix.go index a5644bfbcd..fdde2d86e8 100644 --- a/pkg/provider/loadbalancer/iputil/prefix.go +++ b/pkg/provider/loadbalancer/iputil/prefix.go @@ -30,14 +30,14 @@ func IsPrefixesAllowAll(prefixes []netip.Prefix) bool { return false } -func ParsePrefixes(vs []string) ([]netip.Prefix, error) { - var rv []netip.Prefix - for _, v := range vs { - prefix, err := netip.ParsePrefix(v) - if err != nil { - return nil, fmt.Errorf("invalid CIDR `%s`: %w", v, err) - } - rv = append(rv, prefix) +func ParsePrefix(v string) (netip.Prefix, error) { + prefix, err := netip.ParsePrefix(v) + if err != nil { + return netip.Prefix{}, fmt.Errorf("invalid CIDR `%s`: %w", v, err) + } + masked := prefix.Masked() + if prefix.Addr().Compare(masked.Addr()) != 0 { + return netip.Prefix{}, fmt.Errorf("invalid CIDR `%s`: not a valid network prefix, should be properly masked like %s", v, masked) } - return rv, nil + return prefix, nil } diff --git a/pkg/provider/loadbalancer/iputil/prefix_test.go b/pkg/provider/loadbalancer/iputil/prefix_test.go index 3afeae6dcf..b341e4a23d 100644 --- a/pkg/provider/loadbalancer/iputil/prefix_test.go +++ b/pkg/provider/loadbalancer/iputil/prefix_test.go @@ -83,52 +83,33 @@ func TestIsPrefixesAllowAll(t *testing.T) { } } -func TestParsePrefixes(t *testing.T) { - t.Run("empty", func(t *testing.T) { - actual, err := ParsePrefixes([]string{}) - assert.NoError(t, err) - assert.Empty(t, actual) - }) +func TestParsePrefix(t *testing.T) { t.Run("1 ipv4 cidr", func(t *testing.T) { - actual, err := ParsePrefixes([]string{ - "10.10.10.0/24", - }) + actual, err := ParsePrefix("10.10.10.0/24") assert.NoError(t, err) - assert.Equal(t, []netip.Prefix{ - netip.MustParsePrefix("10.10.10.0/24"), - }, actual) + assert.Equal(t, netip.MustParsePrefix("10.10.10.0/24"), actual) }) t.Run("1 ipv6 cidr", func(t *testing.T) { - actual, err := ParsePrefixes([]string{ - "2001:db8::/32", - }) - assert.NoError(t, err) - assert.Equal(t, []netip.Prefix{ - netip.MustParsePrefix("2001:db8::/32"), - }, actual) - }) - t.Run("multiple cidrs", func(t *testing.T) { - actual, err := ParsePrefixes([]string{ - "10.10.10.0/24", - "2001:db8::/32", - }) + actual, err := ParsePrefix("2001:db8::/32") assert.NoError(t, err) - assert.Equal(t, []netip.Prefix{ - netip.MustParsePrefix("10.10.10.0/24"), - netip.MustParsePrefix("2001:db8::/32"), - }, actual) + assert.Equal(t, netip.MustParsePrefix("2001:db8::/32"), actual) }) t.Run("invalid cidr", func(t *testing.T) { { - _, err := ParsePrefixes([]string{""}) + _, err := ParsePrefix("") + assert.Error(t, err) + } + { + _, err := ParsePrefix("foo") assert.Error(t, err) } + // below two tests check for valid cidr but not valid network prefix { - _, err := ParsePrefixes([]string{"foo"}) + _, err := ParsePrefix("10.10.10.1/24") assert.Error(t, err) } { - _, err := ParsePrefixes([]string{"10.10.10.0/24", "foo"}) + _, err := ParsePrefix("2001:db8::5/32") assert.Error(t, err) } })