Merge pull request #2531 from k8s-infra-cherrypick-robot/cherry-pick-…

…2524-to-release-1.23 [release-1.23] fix: check internal lb ip in subnet before keeping it
kubernetes-sigs · Oct 15, 2022 · 3bc56ae · 3bc56ae
2 parents bfd3069 + 10ea822
commit 3bc56ae
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 8 deletions.
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"math"
 	"net"
+	"net/netip"
 	"reflect"
 	"sort"
 	"strconv"
@@ -1972,7 +1973,7 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
 				if loadBalancerIP != "" {
 					configProperties.PrivateIPAllocationMethod = network.IPAllocationMethodStatic
 					configProperties.PrivateIPAddress = &loadBalancerIP
-				} else if status != nil && len(status.Ingress) > 0 {
+				} else if status != nil && len(status.Ingress) > 0 && ipInSubnet(status.Ingress[0].IP, &subnet) {
 					klog.V(4).Infof("reconcileFrontendIPConfigs for service (%s): keep the original private IP %s", serviceName, status.Ingress[0].IP)
 					configProperties.PrivateIPAllocationMethod = network.IPAllocationMethodStatic
 					configProperties.PrivateIPAddress = to.StringPtr(status.Ingress[0].IP)
@@ -3392,6 +3393,35 @@ func subnet(service *v1.Service) *string {
 	return nil
 }
 
+func ipInSubnet(ip string, subnet *network.Subnet) bool {
+	if subnet == nil || subnet.SubnetPropertiesFormat == nil {
+		return false
+	}
+	netIP, err := netip.ParseAddr(ip)
+	if err != nil {
+		klog.Errorf("ipInSubnet: failed to parse ip %s: %v", netIP, err)
+		return false
+	}
+	cidrs := make([]string, 0)
+	if subnet.AddressPrefix != nil {
+		cidrs = append(cidrs, *subnet.AddressPrefix)
+	}
+	if subnet.AddressPrefixes != nil {
+		cidrs = append(cidrs, *subnet.AddressPrefixes...)
+	}
+	for _, cidr := range cidrs {
+		network, err := netip.ParsePrefix(cidr)
+		if err != nil {
+			klog.Errorf("ipInSubnet: failed to parse ip cidr %s: %v", cidr, err)
+			continue
+		}
+		if network.Contains(netIP) {
+			return true
+		}
+	}
+	return false
+}
+
 // getServiceLoadBalancerMode parses the mode value.
 // if the value is __auto__ it returns isAuto = TRUE.
 // if anything else it returns the unique VM set names after trimming spaces.

diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
@@ -5198,7 +5198,7 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
 		regionZonesMap            map[string][]string
 		expectedZones             *[]string
 		expectedDirty             bool
-		expectedIP                string
+		expectedIP                *string
 		expectedErr               error
 	}{
 		{
@@ -5272,14 +5272,25 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
 			expectedDirty: true,
 		},
 		{
-			description: "reconcileFrontendIPConfigs should reuse the existing private IP for internal services",
+			description: "reconcileFrontendIPConfigs should reuse the existing private IP for internal services when subnet does not change",
 			service:     getInternalTestService("test", 80),
 			status: &v1.LoadBalancerStatus{
 				Ingress: []v1.LoadBalancerIngress{
 					{IP: "1.2.3.4"},
 				},
 			},
-			expectedIP:    "1.2.3.4",
+			expectedIP:    to.StringPtr("1.2.3.4"),
+			expectedDirty: true,
+		},
+		{
+			description: "reconcileFrontendIPConfigs should not reuse the existing private IP for internal services when subnet changes",
+			service:     getInternalTestService("test", 80),
+			status: &v1.LoadBalancerStatus{
+				Ingress: []v1.LoadBalancerIngress{
+					{IP: "1.2.3.6"},
+				},
+			},
+			expectedIP:    to.StringPtr(""),
 			expectedDirty: true,
 		},
 	} {
@@ -5297,7 +5308,8 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
 			mockPIPClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).MaxTimes(1)
 
 			subnetClient := cloud.SubnetsClient.(*mocksubnetclient.MockInterface)
-			subnetClient.EXPECT().Get(gomock.Any(), "rg", "vnet", "subnet", gomock.Any()).Return(network.Subnet{}, nil).MaxTimes(1)
+			subnetClient.EXPECT().Get(gomock.Any(), "rg", "vnet", "subnet", gomock.Any()).Return(
+				network.Subnet{SubnetPropertiesFormat: &network.SubnetPropertiesFormat{AddressPrefix: to.StringPtr("1.2.3.4/31")}}, nil).MaxTimes(1)
 
 			zoneClient := mockzoneclient.NewMockInterface(ctrl)
 			zoneClient.EXPECT().GetZones(gomock.Any(), gomock.Any()).Return(map[string][]string{}, tc.getZoneError).MaxTimes(1)
@@ -5318,9 +5330,13 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
 				}
 			}
 
-			if tc.expectedIP != "" {
-				assert.Equal(t, network.IPAllocationMethodStatic, (*lb.FrontendIPConfigurations)[0].PrivateIPAllocationMethod)
-				assert.Equal(t, tc.expectedIP, to.String((*lb.FrontendIPConfigurations)[0].PrivateIPAddress))
+			if tc.expectedIP != nil {
+				assert.Equal(t, *tc.expectedIP, to.String((*lb.FrontendIPConfigurations)[0].PrivateIPAddress))
+				if *tc.expectedIP != "" {
+					assert.Equal(t, network.IPAllocationMethodStatic, (*lb.FrontendIPConfigurations)[0].PrivateIPAllocationMethod)
+				} else {
+					assert.Equal(t, network.IPAllocationMethodDynamic, (*lb.FrontendIPConfigurations)[0].PrivateIPAllocationMethod)
+				}
 			}
 		})
 	}