Skip to content

Commit

Permalink
Merge pull request #5751 from jwtty/cidr-1.28-cp
Browse files Browse the repository at this point in the history 
[release-1.28] feat: skip invalid LoadBalancerSourceRanges network CIDRs when provisioning nsg rules
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Mar 21, 2024
2 parents 2a6e42f + f7e4d62 commit 0f56f1d
Show file tree
Hide file tree
Showing 6 changed files with 181 additions and 78 deletions.
13 changes: 12 additions & 1 deletion pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2859,6 +2859,7 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
sourceRanges = accessControl.SourceRanges()
allowedServiceTags = accessControl.AllowedServiceTags()
allowedIPRanges = accessControl.AllowedIPRanges()
invalidRanges = accessControl.InvalidRanges()
sourceAddressPrefixes = map[bool][]string{
false: accessControl.IPV4Sources(),
true: accessControl.IPV6Sources(),
Expand All @@ -2884,13 +2885,19 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
consts.ServiceAnnotationAllowedIPRanges, consts.ServiceAnnotationAllowedServiceTags,
))
}
if len(invalidRanges) > 0 {
az.Event(service, v1.EventTypeWarning, "InvalidConfiguration", fmt.Sprintf(
"Found invalid LoadBalancerSourceRanges %v, ignoring and adding a default DenyAll rule in security group.",
invalidRanges,
))
}

var expectedSecurityRules []network.SecurityRule
handleSecurityRules := func(isIPv6 bool) error {
expectedSecurityRulesSingleStack, err := az.getExpectedSecurityRules(
wantLb, ports,
sourceAddressPrefixes[isIPv6], service,
destinationIPAddresses[isIPv6], sourceRanges,
destinationIPAddresses[isIPv6], sourceRanges, invalidRanges,
backendIPAddresses[isIPv6], disableFloatingIP, isIPv6,
)
expectedSecurityRules = append(expectedSecurityRules, expectedSecurityRulesSingleStack...)
Expand Down Expand Up @@ -3083,6 +3090,7 @@ func (az *Cloud) getExpectedSecurityRules(
service *v1.Service,
destinationIPAddresses []string,
sourceRanges []netip.Prefix,
invalidRanges []string,
backendIPAddresses []string,
disableFloatingIP, isIPv6 bool,
) ([]network.SecurityRule, error) {
Expand Down Expand Up @@ -3133,6 +3141,9 @@ func (az *Cloud) getExpectedSecurityRules(
shouldAddDenyRule = true
}
}
if len(invalidRanges) > 0 {
shouldAddDenyRule = true
}
if shouldAddDenyRule {
for _, port := range ports {
_, securityProto, _, err := getProtocolsFromKubernetesProtocol(port.Protocol)
Expand Down
32 changes: 32 additions & 0 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4687,6 +4687,38 @@ func TestReconcileSecurityGroupCommon(t *testing.T) {
},
},
},
{
desc: "reconcileSecurityGroup shall create sgs with deny_all rule and not allow internet access when provided allowedIPRanges is invalid",
service: getTestService("svc", v1.ProtocolTCP, map[string]string{
consts.ServiceAnnotationAllowedIPRanges: "10.10.10.1/24",
}, false, 80),
existingSgs: map[string]network.SecurityGroup{"nsg": {
Name: pointer.String("nsg"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{},
}},
lbIPs: &[]string{"10.0.0.1", "10.0.0.2"},
wantLb: true,
expectedSg: &network.SecurityGroup{
Name: pointer.String("nsg"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
SecurityRules: &[]network.SecurityRule{
{
Name: pointer.String("asvc-TCP-80-deny_all"),
SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
Protocol: network.SecurityRuleProtocol("Tcp"),
SourcePortRange: pointer.String("*"),
SourceAddressPrefix: pointer.String("*"),
DestinationPortRange: pointer.String("80"),
DestinationAddressPrefixes: &([]string{"10.0.0.1", "10.0.0.2"}),
Access: network.SecurityRuleAccess("Deny"),
Priority: pointer.Int32(500),
Direction: network.SecurityRuleDirection("Inbound"),
},
},
},
},
},
},
{
desc: "reconcileSecurityGroup shall create sgs while allowedIPRanges and serviceTags annotation is set",
service: getTestService("svc", v1.ProtocolTCP, map[string]string{
Expand Down
109 changes: 83 additions & 26 deletions pkg/provider/loadbalancer/accesscontrol.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ limitations under the License.
package loadbalancer

import (
"errors"
"fmt"
"net/netip"
"strings"

v1 "k8s.io/api/core/v1"
"k8s.io/klog/v2"

"sigs.k8s.io/cloud-provider-azure/pkg/consts"
)
Expand Down Expand Up @@ -50,43 +52,84 @@ 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) {
const Sep = ","
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[consts.ServiceAnnotationAllowedIPRanges]
value, found := svc.Annotations[Key]
if !found {
return nil, nil
return nil, nil, nil
}

rv, err := ParseCIDRs(strings.Split(strings.TrimSpace(value), Sep))
if err != nil {
return nil, fmt.Errorf("invalid service annotation %s:%s: %w", consts.ServiceAnnotationAllowedIPRanges, value, err)
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)
}
}

return rv, nil
if len(errs) > 0 {
return validRanges, invalidRanges, fmt.Errorf("invalid service annotation %s:%s: %w", 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 := ParseCIDRs(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 := 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 rv, nil
return validRanges, invalidRanges, nil
}

const Sep = ","
value, found := svc.Annotations[v1.AnnotationLoadBalancerSourceRangesKey]
// Read from annotation
const (
Sep = ","
Key = v1.AnnotationLoadBalancerSourceRangesKey
)
value, found := svc.Annotations[Key]
if !found {
return nil, nil
return nil, nil, nil
}
rv, err := ParseCIDRs(strings.Split(strings.TrimSpace(value), Sep))
if err != nil {
return nil, fmt.Errorf("invalid service annotation %s:%s: %w", v1.AnnotationLoadBalancerSourceRangesKey, value, err)
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)
}
}
if len(errs) > 0 {
return validRanges, invalidRanges, fmt.Errorf("invalid service annotation %s:%s: %w", Key, value, errors.Join(errs...))
}
return rv, nil
return validRanges, invalidRanges, nil
}

type AccessControl struct {
Expand All @@ -96,16 +139,21 @@ type AccessControl struct {
sourceRanges []netip.Prefix
allowedIPRanges []netip.Prefix
allowedServiceTags []string
invalidRanges []string
}

func NewAccessControl(svc *v1.Service) (*AccessControl, error) {
sourceRanges, err := SourceRanges(svc)
logger := klog.Background().
WithName("LoadBalancer.AccessControl").
WithValues("service-name", svc.Name)

sourceRanges, invalidSourceRanges, err := SourceRanges(svc)
if err != nil {
return nil, err
logger.Error(err, "Failed to parse SourceRange configuration")
}
allowedIPRanges, err := AllowedIPRanges(svc)
allowedIPRanges, invalidAllowedIPRanges, err := AllowedIPRanges(svc)
if err != nil {
return nil, err
logger.Error(err, "Failed to parse AllowedIPRanges configuration")
}
allowedServiceTags, err := AllowedServiceTags(svc)
if err != nil {
Expand All @@ -117,6 +165,7 @@ func NewAccessControl(svc *v1.Service) (*AccessControl, error) {
sourceRanges: sourceRanges,
allowedIPRanges: allowedIPRanges,
allowedServiceTags: allowedServiceTags,
invalidRanges: append(invalidSourceRanges, invalidAllowedIPRanges...),
}, nil
}

Expand All @@ -135,9 +184,14 @@ func (ac *AccessControl) AllowedServiceTags() []string {
return ac.allowedServiceTags
}

// InvalidRanges returns the invalid IP ranges provided by user in sourceRanges and allowedIPRanges.
func (ac *AccessControl) InvalidRanges() []string {
return ac.invalidRanges
}

// 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 @@ -149,6 +203,9 @@ func (ac *AccessControl) IsAllowFromInternet() bool {
if len(ac.allowedIPRanges) > 0 && !IsCIDRsAllowAll(ac.allowedIPRanges) {
return false
}
if len(ac.invalidRanges) > 0 {
return false
}
if IsExternal(ac.svc) {
return true
}
Expand Down
Loading

0 comments on commit 0f56f1d

Please sign in to comment.