Skip to content

Commit

Permalink
skip invalid LoadBalancerSourceRanges when provisioning nsg rules
Browse files Browse the repository at this point in the history
  • Loading branch information
@jwtty
jwtty committed Mar 19, 2024
1 parent 22d75a8 commit 8539051
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 90 deletions.
21 changes: 12 additions & 9 deletions pkg/provider/loadbalancer/accesscontrol.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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.
Expand Down
68 changes: 51 additions & 17 deletions pkg/provider/loadbalancer/configuration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package loadbalancer

import (
"errors"
"fmt"
"net/netip"
"strings"
Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand Down
39 changes: 25 additions & 14 deletions pkg/provider/loadbalancer/configuration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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",
},
},
})
Expand All @@ -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"},
Expand All @@ -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,
},
Expand All @@ -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"},
Expand All @@ -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",
},
},
})
Expand All @@ -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)
})
}

Expand Down
22 changes: 9 additions & 13 deletions pkg/provider/loadbalancer/iputil/prefix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +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)
}
masked := prefix.Masked()
if prefix.Addr().Compare(masked.Addr()) != 0 {
return nil, fmt.Errorf("invalid CIDR `%s`: not a valid network prefix, should be properly masked like %s", v, masked)
}
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
}

0 comments on commit 8539051

Please sign in to comment.