Skip to content

Commit

Permalink
Check flipped service in GetLoadBalancer
Browse files Browse the repository at this point in the history
  • Loading branch information
@jwtty
jwtty committed Sep 30, 2022
1 parent cbd6044 commit 4d017dd
Show file tree
Hide file tree
Showing 4 changed files with 263 additions and 58 deletions.
54 changes: 35 additions & 19 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,20 +64,31 @@ func (az *Cloud) GetLoadBalancer(ctx context.Context, clusterName string, servic
return existsPip
}()

_, status, existsLb, err := az.getServiceLoadBalancer(service, clusterName, nil, false, []network.LoadBalancer{})
existingLBs, err := az.ListLB(service)
if err != nil {
return nil, existsPip, err
}

_, status, existsLb, err := az.getServiceLoadBalancer(service, clusterName, nil, false, existingLBs)
if err != nil || existsLb {
return status, existsLb || existsPip, err
}

flippedService := flipServiceInternalAnnotation(service)
_, status, existsLb, err = az.getServiceLoadBalancer(flippedService, clusterName, nil, false, existingLBs)
if err != nil || existsLb {
return status, existsLb || existsPip, err
}

// Return exists = false only if the load balancer and the public IP are not found on Azure
if !existsLb && !existsPip {
serviceName := getServiceName(service)
klog.V(5).Infof("getloadbalancer (cluster:%s) (service:%s) - doesn't exist", clusterName, serviceName)
return nil, false, nil
}

// Return exists = true if either the load balancer or the public IP (or both) exists
return status, true, nil
// Return exists = true if only the public IP exists
return nil, true, nil
}

func getPublicIPDomainNameLabel(service *v1.Service) (string, bool) {
Expand Down Expand Up @@ -241,7 +252,6 @@ func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName stri
defer az.serviceReconcileLock.Unlock()

var err error
isInternal := requiresInternalLoadBalancer(service)
serviceName := getServiceName(service)
mc := metrics.NewMetricContext("services", "ensure_loadbalancer_deleted", az.ResourceGroup, az.getNetworkResourceSubscriptionID(), serviceName)
klog.V(5).InfoS("EnsureLoadBalancerDeleted Start", "service", serviceName, "cluster", clusterName, "service_spec", service)
Expand All @@ -251,7 +261,7 @@ func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName stri
klog.V(5).InfoS("EnsureLoadBalancerDeleted Finish", "service", serviceName, "cluster", clusterName, "service_spec", service, "error", err)
}()

serviceIPToCleanup, err := az.findServiceIPAddress(ctx, clusterName, service, isInternal)
serviceIPToCleanup, err := az.findServiceIPAddress(ctx, clusterName, service)
if err != nil && !retry.HasStatusForbiddenOrIgnoredError(err) {
return err
}
Expand All @@ -267,6 +277,12 @@ func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName stri
return err
}

// check flipped service also
flippedService := flipServiceInternalAnnotation(service)
if _, err := az.reconcileLoadBalancer(clusterName, flippedService, nil, false /* wantLb */); err != nil {
return err
}

_, err = az.reconcilePublicIP(clusterName, service, "", false /* wantLb */)
if err != nil {
return err
Expand Down Expand Up @@ -1009,7 +1025,7 @@ func updateServiceLoadBalancerIP(service *v1.Service, serviceIP string) *v1.Serv
return copyService
}

func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, service *v1.Service, isInternalLb bool) (string, error) {
func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, service *v1.Service) (string, error) {
if len(service.Spec.LoadBalancerIP) > 0 {
return service.Spec.LoadBalancerIP, nil
}
Expand Down Expand Up @@ -2445,13 +2461,25 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
return nil, err
}

destinationIPAddress := ""
if wantLb && lbIP == nil {
return nil, fmt.Errorf("no load balancer IP for setting up security rules for service %s", service.Name)
}
if lbIP != nil {
destinationIPAddress = *lbIP
}

if destinationIPAddress == "" {
destinationIPAddress = "*"
}

disableFloatingIP := false
if consts.IsK8sServiceDisableLoadBalancerFloatingIP(service) {
disableFloatingIP = true
}

backendIPAddresses := make([]string, 0)
if disableFloatingIP {
if wantLb && disableFloatingIP {
lb, exist, err := az.getAzureLoadBalancer(to.String(lbName), azcache.CacheReadTypeDefault)
if err != nil {
return nil, err
Expand All @@ -2466,18 +2494,6 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
}
}

destinationIPAddress := ""
if wantLb && lbIP == nil {
return nil, fmt.Errorf("no load balancer IP for setting up security rules for service %s", service.Name)
}
if lbIP != nil {
destinationIPAddress = *lbIP
}

if destinationIPAddress == "" {
destinationIPAddress = "*"
}

additionalIPs, err := getServiceAdditionalPublicIPs(service)
if err != nil {
return nil, fmt.Errorf("unable to get additional public IPs, error=%v", err)
Expand Down
190 changes: 180 additions & 10 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,132 @@ const LBInUseRawError = `{
}
}`

func TestGetLoadBalancer(t *testing.T) {
lb1 := network.LoadBalancer{
Name: to.StringPtr("testCluster"),
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{},
}
lb2 := network.LoadBalancer{
Name: to.StringPtr("testCluster"),
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
FrontendIPConfigurations: &[]network.FrontendIPConfiguration{
{
Name: to.StringPtr("aservice"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{ID: to.StringPtr("testCluster-aservice")},
},
},
},
},
}
lb3 := network.LoadBalancer{
Name: to.StringPtr("testCluster-internal"),
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
FrontendIPConfigurations: &[]network.FrontendIPConfiguration{
{
Name: to.StringPtr("aservice"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PrivateIPAddress: to.StringPtr("10.0.0.6"),
},
},
},
},
}
tests := []struct {
desc string
service v1.Service
existingLBs []network.LoadBalancer
pipExists bool
expectedGotLB bool
expectedStatus *v1.LoadBalancerStatus
}{
{
desc: "GetLoadBalancer should return true when only public IP exists",
service: getTestService("service", v1.ProtocolTCP, nil, false, 80),
existingLBs: []network.LoadBalancer{lb1},
pipExists: true,
expectedGotLB: true,
expectedStatus: nil,
},
{
desc: "GetLoadBalancer should return false when neither public IP nor LB exists",
service: getTestService("service", v1.ProtocolTCP, nil, false, 80),
existingLBs: []network.LoadBalancer{lb1},
pipExists: false,
expectedGotLB: false,
expectedStatus: nil,
},
{
desc: "GetLoadBalancer should return true when external service finds external LB",
service: getTestService("service", v1.ProtocolTCP, nil, false, 80),
existingLBs: []network.LoadBalancer{lb2},
pipExists: true,
expectedGotLB: true,
expectedStatus: &v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{
{IP: "1.2.3.4"},
},
},
},
{
desc: "GetLoadBalancer should return true when internal service finds internal LB",
service: getInternalTestService("service", 80),
existingLBs: []network.LoadBalancer{lb3},
expectedGotLB: true,
expectedStatus: &v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{
{IP: "10.0.0.6"},
},
},
},
{
desc: "GetLoadBalancer should return true when external service finds previous internal LB",
service: getTestService("service", v1.ProtocolTCP, nil, false, 80),
existingLBs: []network.LoadBalancer{lb3},
expectedGotLB: true,
expectedStatus: &v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{
{IP: "10.0.0.6"},
},
},
},
{
desc: "GetLoadBalancer should return true when external service finds external LB",
service: getInternalTestService("service", 80),
existingLBs: []network.LoadBalancer{lb2},
pipExists: true,
expectedGotLB: true,
expectedStatus: &v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{
{IP: "1.2.3.4"},
},
},
},
}
ctrl := gomock.NewController(t)
defer ctrl.Finish()

for i, c := range tests {
az := GetTestCloud(ctrl)
mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
if c.pipExists {
mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(network.PublicIPAddress{
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: to.StringPtr("1.2.3.4"),
}}, nil)
} else {
mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound})
}
mockLBsClient := az.LoadBalancerClient.(*mockloadbalancerclient.MockInterface)
mockLBsClient.EXPECT().List(gomock.Any(), az.Config.ResourceGroup).Return(c.existingLBs, nil)

status, existsLB, err := az.GetLoadBalancer(context.TODO(), testClusterName, &c.service)
assert.Nil(t, err, "TestCase[%d]: %s", i, c.desc)
assert.Equal(t, c.expectedGotLB, existsLB, "TestCase[%d]: %s", i, c.desc)
assert.Equal(t, c.expectedStatus, status, "TestCase[%d]: %s", i, c.desc)
}
}

func TestFindProbe(t *testing.T) {
tests := []struct {
msg string
Expand Down Expand Up @@ -560,28 +686,40 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) {
isInternalSvc bool
expectCreateError bool
wrongRGAtDelete bool
flipService bool
}{
{
desc: "exteral service then flipped to internal should be created and deleted successfully",
service: getTestService("service1", v1.ProtocolTCP, nil, false, 80),
flipService: true,
},
{
desc: "interal service then flipped to external should be created and deleted successfully",
service: getInternalTestService("service2", 80),
isInternalSvc: true,
flipService: true,
},
{
desc: "external service should be created and deleted successfully",
service: getTestService("service1", v1.ProtocolTCP, nil, false, 80),
service: getTestService("service3", v1.ProtocolTCP, nil, false, 80),
},
{
desc: "internal service should be created and deleted successfully",
service: getInternalTestService("service2", 80),
service: getInternalTestService("service4", 80),
isInternalSvc: true,
},
{
desc: "annotated service with same resourceGroup should be created and deleted successfully",
service: getResourceGroupTestService("service3", "rg", "", 80),
service: getResourceGroupTestService("service5", "rg", "", 80),
},
{
desc: "annotated service with different resourceGroup shouldn't be created but should be deleted successfully",
service: getResourceGroupTestService("service4", "random-rg", "1.2.3.4", 80),
service: getResourceGroupTestService("service6", "random-rg", "1.2.3.4", 80),
expectCreateError: true,
},
{
desc: "annotated service with different resourceGroup shouldn't be created but should be deleted successfully",
service: getResourceGroupTestService("service5", "random-rg", "", 80),
service: getResourceGroupTestService("service7", "random-rg", "", 80),
expectCreateError: true,
wrongRGAtDelete: true,
},
Expand All @@ -596,7 +734,7 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) {
mockLBBackendPool.EXPECT().GetBackendPrivateIPs(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()

clusterResources, expectedInterfaces, expectedVirtualMachines := getClusterResources(az, vmCount, availabilitySetCount)
setMockEnv(az, ctrl, expectedInterfaces, expectedVirtualMachines, 4)
setMockEnv(az, ctrl, expectedInterfaces, expectedVirtualMachines, 5)

for i, c := range tests {

Expand All @@ -616,16 +754,21 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) {
assert.NotNil(t, lbStatus, "TestCase[%d]: %s", i, c.desc)
result, rerr := az.LoadBalancerClient.List(context.TODO(), az.Config.ResourceGroup)
assert.Nil(t, rerr, "TestCase[%d]: %s", i, c.desc)
assert.Equal(t, len(result), 1, "TestCase[%d]: %s", i, c.desc)
assert.Equal(t, len(*result[0].LoadBalancingRules), 1, "TestCase[%d]: %s", i, c.desc)
assert.Equal(t, 1, len(result), "TestCase[%d]: %s", i, c.desc)
assert.Equal(t, 1, len(*result[0].LoadBalancingRules), "TestCase[%d]: %s", i, c.desc)
}

expectedLBs = make([]network.LoadBalancer, 0)
setMockLBs(az, ctrl, &expectedLBs, "service", 1, i+1, c.isInternalSvc)
// finally, delete it.
if c.wrongRGAtDelete {
az.LoadBalancerResourceGroup = "nil"
}
if c.flipService {
flippedService := flipServiceInternalAnnotation(&c.service)
c.service = *flippedService
c.isInternalSvc = !c.isInternalSvc
}
expectedLBs = make([]network.LoadBalancer, 0)
setMockLBs(az, ctrl, &expectedLBs, "service", 1, i+1, c.isInternalSvc)

expectedPLS := make([]network.PrivateLinkService, 0)
mockPLSClient := mockprivatelinkserviceclient.NewMockInterface(ctrl)
Expand Down Expand Up @@ -3127,6 +3270,33 @@ func TestReconcileSecurityGroup(t *testing.T) {
existingSgs: map[string]network.SecurityGroup{"nsg": {}},
expectedSg: &network.SecurityGroup{},
},
{
desc: "reconcileSecurityGroup shall delete unwanted sg if wantLb is false and lbIP is nil",
service: getTestService("test1", v1.ProtocolTCP, nil, false, 80),
existingSgs: map[string]network.SecurityGroup{"nsg": {
Name: to.StringPtr("nsg"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
SecurityRules: &[]network.SecurityRule{
{
Name: to.StringPtr("atest1-toBeDeleted"),
SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
SourceAddressPrefix: to.StringPtr("prefix"),
SourcePortRange: to.StringPtr("range"),
DestinationAddressPrefix: to.StringPtr("desPrefix"),
DestinationPortRange: to.StringPtr("desRange"),
},
},
},
},
}},
wantLb: false,
expectedSg: &network.SecurityGroup{
Name: to.StringPtr("nsg"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
SecurityRules: &[]network.SecurityRule{},
},
},
},
{
desc: "reconcileSecurityGroup shall delete unwanted sgs and create needed ones",
service: getTestService("test1", v1.ProtocolTCP, nil, false, 80),
Expand Down

0 comments on commit 4d017dd

Please sign in to comment.