Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use zones in the pre-existing frontend IP configurations for int… #1070

Merged
merged 2 commits into from
Jan 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/azureclients/interfaceclient/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

const (
// APIVersion is the API version for network.
APIVersion = "2020-08-01"
APIVersion = "2021-02-01"
// AzureStackCloudAPIVersion is the API version for Azure Stack
AzureStackCloudAPIVersion = "2018-11-01"

Expand Down
2 changes: 1 addition & 1 deletion pkg/azureclients/loadbalancerclient/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

const (
// APIVersion is the API version for network.
APIVersion = "2020-08-01"
APIVersion = "2021-02-01"
// AzureStackCloudAPIVersion is the API version for Azure Stack
AzureStackCloudAPIVersion = "2018-11-01"
// AzureStackCloudName is the cloud name of Azure Stack
Expand Down
2 changes: 1 addition & 1 deletion pkg/azureclients/publicipclient/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

const (
// APIVersion is the API version for network.
APIVersion = "2020-08-01"
APIVersion = "2021-02-01"
// AzureStackCloudAPIVersion is the API version for Azure Stack
AzureStackCloudAPIVersion = "2018-11-01"

Expand Down
2 changes: 1 addition & 1 deletion pkg/azureclients/routeclient/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

const (
// APIVersion is the API version for network.
APIVersion = "2020-08-01"
APIVersion = "2021-02-01"
// AzureStackCloudAPIVersion is the API version for Azure Stack
AzureStackCloudAPIVersion = "2018-11-01"
// AzureStackCloudName is the cloud name of Azure Stack
Expand Down
2 changes: 1 addition & 1 deletion pkg/azureclients/routetableclient/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

const (
// APIVersion is the API version for network.
APIVersion = "2020-08-01"
APIVersion = "2021-02-01"
// AzureStackCloudAPIVersion is the API version for Azure Stack
AzureStackCloudAPIVersion = "2018-11-01"
// AzureStackCloudName is the cloud name of Azure Stack
Expand Down
2 changes: 1 addition & 1 deletion pkg/azureclients/securitygroupclient/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

const (
// APIVersion is the API version for network.
APIVersion = "2020-08-01"
APIVersion = "2021-02-01"
// AzureStackCloudAPIVersion is the API version for Azure Stack
AzureStackCloudAPIVersion = "2018-11-01"
// AzureStackCloudName is the cloud name of Azure Stack
Expand Down
47 changes: 38 additions & 9 deletions pkg/provider/azure_loadbalancer.go
Original file line number Diff line number Diff line change
Expand Up @@ -1734,16 +1734,21 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
}
}
} else {
var (
previousZone *[]string
isFipChanged bool
)
for i := len(newConfigs) - 1; i >= 0; i-- {
config := newConfigs[i]
isFipChanged, err := az.isFrontendIPChanged(clusterName, config, service, defaultLBFrontendIPConfigName)
isFipChanged, err = az.isFrontendIPChanged(clusterName, config, service, defaultLBFrontendIPConfigName)
if err != nil {
return nil, false, err
}
if isFipChanged {
klog.V(2).Infof("reconcileLoadBalancer for service (%s)(%t): lb frontendconfig(%s) - dropping", serviceName, wantLb, *config.Name)
newConfigs = append(newConfigs[:i], newConfigs[i+1:]...)
dirtyConfigs = true
previousZone = config.Zones
}
}

Expand Down Expand Up @@ -1810,14 +1815,11 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
FrontendIPConfigurationPropertiesFormat: fipConfigurationProperties,
}

// only add zone information for new internal frontend IP configurations for standard load balancer not deployed to an edge zone.
location := az.Location
zones, err := az.getRegionZonesBackoff(location)
if err != nil {
return nil, false, err
}
if isInternal && az.useStandardLoadBalancer() && len(zones) > 0 && !az.HasExtendedLocation() {
newConfig.Zones = &zones
if isInternal {
if err := az.getFrontendZones(&newConfig, previousZone, isFipChanged, serviceName, defaultLBFrontendIPConfigName); err != nil {
klog.Errorf("reconcileLoadBalancer for service (%s)(%t): failed to getFrontendZones: %s", serviceName, wantLb, err.Error())
return nil, false, err
}
}
newConfigs = append(newConfigs, newConfig)
klog.V(2).Infof("reconcileLoadBalancer for service (%s)(%t): lb frontendconfig(%s) - adding", serviceName, wantLb, defaultLBFrontendIPConfigName)
Expand All @@ -1832,6 +1834,33 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
return ownedFIPConfig, dirtyConfigs, err
}

func (az *Cloud) getFrontendZones(
fipConfig *network.FrontendIPConfiguration,
previousZone *[]string,
isFipChanged bool,
serviceName, defaultLBFrontendIPConfigName string) error {
if !isFipChanged { // fetch zone information from API for new frontends
// only add zone information for new internal frontend IP configurations for standard load balancer not deployed to an edge zone.
location := az.Location
zones, err := az.getRegionZonesBackoff(location)
if err != nil {
return err
}
if az.useStandardLoadBalancer() && len(zones) > 0 && !az.HasExtendedLocation() {
fipConfig.Zones = &zones
}
} else {
if previousZone == nil { // keep the existing zone information for existing frontends
klog.V(2).Infof("getFrontendZones for service (%s): lb frontendconfig(%s): setting zone to nil", serviceName, defaultLBFrontendIPConfigName)
} else {
zoneStr := strings.Join(*previousZone, ",")
klog.V(2).Infof("getFrontendZones for service (%s): lb frontendconfig(%s): setting zone to %s", serviceName, defaultLBFrontendIPConfigName, zoneStr)
}
fipConfig.Zones = previousZone
}
return nil
}

// checkLoadBalancerResourcesConflicts checks if the service is consuming
// ports which conflict with the existing loadBalancer resources,
// including inbound NAT rule, inbound NAT pools and loadBalancing rules
Expand Down
57 changes: 56 additions & 1 deletion pkg/provider/azure_loadbalancer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,16 @@ import (
"github.com/Azure/go-autorest/autorest/to"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"

v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/loadbalancerclient/mockloadbalancerclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/publicipclient/mockpublicipclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/securitygroupclient/mocksecuritygroupclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/subnetclient/mocksubnetclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmssclient/mockvmssclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/zoneclient/mockzoneclient"
"sigs.k8s.io/cloud-provider-azure/pkg/consts"
"sigs.k8s.io/cloud-provider-azure/pkg/retry"
)
Expand Down Expand Up @@ -4356,9 +4359,11 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
existingFrontendIPConfigs []network.FrontendIPConfiguration
existingPIP network.PublicIPAddress
getPIPError *retry.Error
getZoneError *retry.Error
regionZonesMap map[string][]string
expectedZones *[]string
expectedDirty bool
expectedErr error
}{
{
description: "reconcileFrontendIPConfigs should reconcile the zones for the new fip config",
Expand All @@ -4379,6 +4384,48 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
expectedZones: &[]string{"1", "2", "3"},
expectedDirty: true,
},
{
description: "reconcileFrontendIPConfigs should report an error if failed to get zones",
service: getInternalTestService("test", 80),
getZoneError: retry.NewError(false, errors.New("get zone failed")),
expectedErr: errors.New("get zone failed"),
},
{
description: "reconcileFrontendIPConfigs should use the nil zones of the existing frontend",
service: getTestServiceWithAnnotation("test", map[string]string{
consts.ServiceAnnotationLoadBalancerInternalSubnet: "subnet",
consts.ServiceAnnotationLoadBalancerInternal: consts.TrueAnnotationValue}, 80),
existingFrontendIPConfigs: []network.FrontendIPConfiguration{
{
Name: to.StringPtr("atest1"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
Subnet: &network.Subnet{
Name: to.StringPtr("subnet-1"),
},
},
},
},
expectedDirty: true,
},
{
description: "reconcileFrontendIPConfigs should use the non-nil zones of the existing frontend",
service: getTestServiceWithAnnotation("test", map[string]string{
consts.ServiceAnnotationLoadBalancerInternalSubnet: "subnet",
consts.ServiceAnnotationLoadBalancerInternal: consts.TrueAnnotationValue}, 80),
existingFrontendIPConfigs: []network.FrontendIPConfiguration{
{
Name: to.StringPtr("atest1"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
Subnet: &network.Subnet{
Name: to.StringPtr("subnet-1"),
},
},
Zones: &[]string{"1"},
},
},
expectedZones: &[]string{"1"},
expectedDirty: true,
},
} {
t.Run(tc.description, func(t *testing.T) {
cloud := GetTestCloud(ctrl)
Expand All @@ -4396,9 +4443,17 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
subnetClient := cloud.SubnetsClient.(*mocksubnetclient.MockInterface)
subnetClient.EXPECT().Get(gomock.Any(), "rg", "vnet", "subnet", gomock.Any()).Return(network.Subnet{}, nil).MaxTimes(1)

zoneClient := mockzoneclient.NewMockInterface(ctrl)
zoneClient.EXPECT().GetZones(gomock.Any(), gomock.Any()).Return(map[string][]string{}, tc.getZoneError).MaxTimes(1)
cloud.ZoneClient = zoneClient

defaultLBFrontendIPConfigName := cloud.getDefaultFrontendIPConfigName(&tc.service)
_, dirty, err := cloud.reconcileFrontendIPConfigs("testCluster", &tc.service, &lb, true, defaultLBFrontendIPConfigName)
assert.NoError(t, err)
if tc.expectedErr == nil {
assert.NoError(t, err)
} else {
assert.Contains(t, err.Error(), tc.expectedErr.Error())
}
assert.Equal(t, tc.expectedDirty, dirty)

for _, fip := range *lb.FrontendIPConfigurations {
Expand Down
8 changes: 7 additions & 1 deletion pkg/provider/azure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1616,8 +1616,14 @@ func getTestService(identifier string, proto v1.Protocol, annotations map[string
}

func getInternalTestService(identifier string, requestedPorts ...int32) v1.Service {
return getTestServiceWithAnnotation(identifier, map[string]string{consts.ServiceAnnotationLoadBalancerInternal: consts.TrueAnnotationValue}, requestedPorts...)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gofmt the file? there should be an empty line between each functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


func getTestServiceWithAnnotation(identifier string, annotations map[string]string, requestedPorts ...int32) v1.Service {
svc := getTestService(identifier, v1.ProtocolTCP, nil, false, requestedPorts...)
svc.Annotations[consts.ServiceAnnotationLoadBalancerInternal] = consts.TrueAnnotationValue
for k, v := range annotations {
svc.Annotations[k] = v
}
return svc
}

Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/network/ensureloadbalancer.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"strings"
"time"

aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
"github.com/Azure/go-autorest/autorest/to"

v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -572,7 +572,7 @@ func defaultPublicIPAddress(ipName string) aznetwork.PublicIPAddress {
Name: skuName,
},
PublicIPAddressPropertiesFormat: &aznetwork.PublicIPAddressPropertiesFormat{
PublicIPAllocationMethod: aznetwork.Static,
PublicIPAllocationMethod: aznetwork.IPAllocationMethodStatic,
},
}
}
2 changes: 1 addition & 1 deletion tests/e2e/network/network_security_group.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"strings"
"time"

aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
"github.com/Azure/go-autorest/autorest/to"

v1 "k8s.io/api/core/v1"
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/network/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"strings"

"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-07-01/compute"
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/network/service_annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
"strings"
"time"

"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
"github.com/Azure/go-autorest/autorest/to"

appsv1 "k8s.io/api/apps/v1"
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/utils/azure_test_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
azauth "github.com/Azure/azure-sdk-for-go/services/authorization/mgmt/2015-07-01/authorization"
azcompute "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-07-01/compute"
acr "github.com/Azure/azure-sdk-for-go/services/containerregistry/mgmt/2019-05-01/containerregistry"
aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
azresources "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/resources"
"github.com/Azure/go-autorest/autorest"
"github.com/Azure/go-autorest/autorest/azure"
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/utils/network_interface_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"strings"

"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-07-01/compute"
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
)

var (
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/utils/network_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"net"
"strings"

aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
"github.com/Azure/go-autorest/autorest/to"

v1 "k8s.io/api/core/v1"
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/utils/route_table_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"context"
"fmt"

aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
"github.com/Azure/go-autorest/autorest/to"

providerazure "sigs.k8s.io/cloud-provider-azure/pkg/provider"
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/utils/service_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

"sigs.k8s.io/cloud-provider-azure/pkg/consts"

aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"

v1 "k8s.io/api/core/v1"
apierrs "k8s.io/apimachinery/pkg/api/errors"
Expand Down