Skip to content

Commit

Permalink
fix: reuse previous private IP address when changing load balancers
Browse files Browse the repository at this point in the history
  • Loading branch information
@nilo19
nilo19 authored and k8s-infra-cherrypick-robot committed Mar 21, 2022
1 parent 8ceff52 commit 26b2519
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 7 deletions.
18 changes: 13 additions & 5 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -723,14 +723,16 @@ func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string,
if isInternalLoadBalancer(&existingLB) != isInternal {
continue
}
status, fipConfig, err := az.getServiceLoadBalancerStatus(service, &existingLB)
var fipConfig *network.FrontendIPConfiguration
status, fipConfig, err = az.getServiceLoadBalancerStatus(service, &existingLB)
if err != nil {
return nil, nil, false, err
}
if status == nil {
// service is not on this load balancer
continue
}
klog.V(4).Infof("getServiceLoadBalancer(%s, %s, %v): current lb ip: %s", service.Name, clusterName, wantLb, status.Ingress[0].IP)

// select another load balancer instead of returning
// the current one if the change is needed
Expand All @@ -756,7 +758,7 @@ func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string,
return nil, nil, false, err
}

return selectedLB, nil, exists, err
return selectedLB, status, exists, err
}

// create a default LB with meta data if not present
Expand Down Expand Up @@ -1520,11 +1522,12 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
return nil, err
}

lb, _, _, err := az.getServiceLoadBalancer(service, clusterName, nodes, wantLb, existingLBs)
lb, lbStatus, _, err := az.getServiceLoadBalancer(service, clusterName, nodes, wantLb, existingLBs)
if err != nil {
klog.Errorf("reconcileLoadBalancer: failed to get load balancer for service %q, error: %v", serviceName, err)
return nil, err
}

lbName := *lb.Name
lbResourceGroup := az.getLoadBalancerResourceGroup()
lbBackendPoolID := az.getBackendPoolID(lbName, az.getLoadBalancerResourceGroup(), getBackendPoolName(clusterName, service))
Expand All @@ -1546,7 +1549,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
}

// reconcile the load balancer's frontend IP configurations.
ownedFIPConfig, changed, err := az.reconcileFrontendIPConfigs(clusterName, service, lb, wantLb, defaultLBFrontendIPConfigName)
ownedFIPConfig, changed, err := az.reconcileFrontendIPConfigs(clusterName, service, lb, lbStatus, wantLb, defaultLBFrontendIPConfigName)
if err != nil {
return lb, err
}
Expand Down Expand Up @@ -1734,7 +1737,7 @@ func (az *Cloud) reconcileLBRules(lb *network.LoadBalancer, service *v1.Service,
return dirtyRules
}

func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Service, lb *network.LoadBalancer, wantLb bool, defaultLBFrontendIPConfigName string) (*network.FrontendIPConfiguration, bool, error) {
func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Service, lb *network.LoadBalancer, status *v1.LoadBalancerStatus, wantLb bool, defaultLBFrontendIPConfigName string) (*network.FrontendIPConfiguration, bool, error) {
var err error
lbName := *lb.Name
serviceName := getServiceName(service)
Expand Down Expand Up @@ -1834,8 +1837,13 @@ 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 {
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)
} else {
// We'll need to call GetLoadBalancer later to retrieve allocated IP.
klog.V(4).Infof("reconcileFrontendIPConfigs for service (%s): dynamically allocate the private IP", serviceName)
configProperties.PrivateIPAllocationMethod = network.IPAllocationMethodDynamic
}

Expand Down
56 changes: 54 additions & 2 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1318,6 +1318,37 @@ func TestGetServiceLoadBalancer(t *testing.T) {
expectedExists: false,
expectedError: false,
},
{
desc: "getServiceLoadBalancer should create a new lb and return the status of the previous lb",
sku: "Basic",
wantLB: true,
service: getTestService("service1", v1.ProtocolTCP, map[string]string{consts.ServiceAnnotationLoadBalancerMode: "as", consts.ServiceAnnotationLoadBalancerInternal: consts.TrueAnnotationValue}, false, 80),
existingLBs: []network.LoadBalancer{
{
Name: to.StringPtr("as-internal"),
Location: to.StringPtr("westus"),
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
FrontendIPConfigurations: &[]network.FrontendIPConfiguration{
{
Name: to.StringPtr("aservice1"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PrivateIPAddress: to.StringPtr("1.2.3.4"),
},
},
},
},
},
},
expectedLB: &network.LoadBalancer{
Name: to.StringPtr("testCluster-internal"),
Location: to.StringPtr("westus"),
Sku: &network.LoadBalancerSku{
Name: network.LoadBalancerSkuNameBasic,
},
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{},
},
expectedStatus: &v1.LoadBalancerStatus{Ingress: []v1.LoadBalancerIngress{{IP: "1.2.3.4", Hostname: ""}}},
},
}

ctrl := gomock.NewController(t)
Expand All @@ -1330,6 +1361,7 @@ func TestGetServiceLoadBalancer(t *testing.T) {
mockLBsClient := mockloadbalancerclient.NewMockInterface(ctrl)
mockLBsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockLBsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingLBs, nil)
mockLBsClient.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
az.LoadBalancerClient = mockLBsClient

for _, existingLB := range test.existingLBs {
Expand All @@ -1338,7 +1370,9 @@ func TestGetServiceLoadBalancer(t *testing.T) {
t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
}
}
test.service.Annotations = test.annotations
if test.annotations != nil {
test.service.Annotations = test.annotations
}
az.LoadBalancerSku = test.sku
lb, status, exists, err := az.getServiceLoadBalancer(&test.service, testClusterName,
clusterResources.nodes, test.wantLB, []network.LoadBalancer{})
Expand Down Expand Up @@ -4601,11 +4635,13 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
service v1.Service
existingFrontendIPConfigs []network.FrontendIPConfiguration
existingPIP network.PublicIPAddress
status *v1.LoadBalancerStatus
getPIPError *retry.Error
getZoneError *retry.Error
regionZonesMap map[string][]string
expectedZones *[]string
expectedDirty bool
expectedIP string
expectedErr error
}{
{
Expand Down Expand Up @@ -4669,6 +4705,17 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
expectedZones: &[]string{"1"},
expectedDirty: true,
},
{
description: "reconcileFrontendIPConfigs should reuse the existing private IP for internal services",
service: getInternalTestService("test", 80),
status: &v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{
{IP: "1.2.3.4"},
},
},
expectedIP: "1.2.3.4",
expectedDirty: true,
},
} {
t.Run(tc.description, func(t *testing.T) {
cloud := GetTestCloud(ctrl)
Expand All @@ -4691,7 +4738,7 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
cloud.ZoneClient = zoneClient

defaultLBFrontendIPConfigName := cloud.getDefaultFrontendIPConfigName(&tc.service)
_, dirty, err := cloud.reconcileFrontendIPConfigs("testCluster", &tc.service, &lb, true, defaultLBFrontendIPConfigName)
_, dirty, err := cloud.reconcileFrontendIPConfigs("testCluster", &tc.service, &lb, tc.status, true, defaultLBFrontendIPConfigName)
if tc.expectedErr == nil {
assert.NoError(t, err)
} else {
Expand All @@ -4704,6 +4751,11 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
assert.Equal(t, tc.expectedZones, fip.Zones)
}
}

if tc.expectedIP != "" {
assert.Equal(t, network.IPAllocationMethodStatic, (*lb.FrontendIPConfigurations)[0].PrivateIPAllocationMethod)
assert.Equal(t, tc.expectedIP, to.String((*lb.FrontendIPConfigurations)[0].PrivateIPAddress))
}
})
}
}
Expand Down

0 comments on commit 26b2519

Please sign in to comment.