Skip to content

Commit

Permalink
[IPv6] Fix reconcileFrontendIPConfigs()
Browse files Browse the repository at this point in the history 
Current logic handles lb.FrontendIPConfigurations according
to Service's IP family, which is incorrect. For an IPv6 cluster,
there're still some IPv4 FIPs due to Azure limitation, which
will be removed. For an IPv4 cluster, all resources are of IPv4,
which is not affected.

Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>
  • Loading branch information
@lzhecheng
lzhecheng committed Jun 5, 2023
1 parent 3b6e917 commit df99c1c
Show file tree
Hide file tree
Showing 8 changed files with 489 additions and 326 deletions.
239 changes: 115 additions & 124 deletions pkg/provider/azure_loadbalancer.go
View file

Large diffs are not rendered by default.

290 changes: 268 additions & 22 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6395,17 +6395,6 @@ func TestRemoveFrontendIPConfigurationFromLoadBalancerDelete(t *testing.T) {
mockPLSClient := cloud.PrivateLinkServiceClient.(*mockprivatelinkserviceclient.MockInterface)
mockPLSClient.EXPECT().List(gomock.Any(), "rg").Return(expectedPLS, nil).MaxTimes(1)
existingLBs := []network.LoadBalancer{{Name: pointer.String("lb")}}
// external Service
existingPIPS := []network.PublicIPAddress{
{
ID: pointer.String("pipID"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
},
},
}
mockPIPsClient := cloud.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(existingPIPS, nil).Times(1)
err := cloud.removeFrontendIPConfigurationFromLoadBalancer(&lb, existingLBs, fip, "testCluster", &service)
assert.NoError(t, err)
})
Expand Down Expand Up @@ -6437,17 +6426,6 @@ func TestRemoveFrontendIPConfigurationFromLoadBalancerUpdate(t *testing.T) {
expectedPLS := make([]network.PrivateLinkService, 0)
mockPLSClient := cloud.PrivateLinkServiceClient.(*mockprivatelinkserviceclient.MockInterface)
mockPLSClient.EXPECT().List(gomock.Any(), "rg").Return(expectedPLS, nil).MaxTimes(1)
// external Service
existingPIPS := []network.PublicIPAddress{
{
ID: pointer.String("pipID"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
},
},
}
mockPIPsClient := cloud.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(existingPIPS, nil).Times(1)
err := cloud.removeFrontendIPConfigurationFromLoadBalancer(&lb, []network.LoadBalancer{}, fip, "testCluster", &service)
assert.NoError(t, err)
})
Expand Down Expand Up @@ -6681,6 +6659,274 @@ func TestReconcileZonesForFrontendIPConfigs(t *testing.T) {
}
}

func TestReconcileFrontendIPConfigs(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

testcases := []struct {
desc string
service v1.Service
existingFIPs []network.FrontendIPConfiguration
existingPIPs []network.PublicIPAddress
status *v1.LoadBalancerStatus
wantLB bool
expectedDirty bool
expectedFIPs []network.FrontendIPConfiguration
expectedErr error
}{
{
desc: "DualStack Service reconciles existing FIPs and does not touch others, not dirty",
service: getTestServiceDualStack("test", v1.ProtocolTCP, nil, 80),
existingFIPs: []network.FrontendIPConfiguration{
{
Name: pointer.String("fipV4"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
Name: pointer.String("pipV4"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
IPAddress: pointer.String("1.2.3.4"),
},
},
},
},
{
Name: pointer.String("fipV6"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
Name: pointer.String("pipV6"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv6,
IPAddress: pointer.String("fe::1"),
},
},
},
},
{
Name: pointer.String("atest"),
ID: pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/frontendIPConfigurations/atest"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
ID: pointer.String("testCluster-atest-id"),
},
},
},
{
Name: pointer.String("atest-IPv6"),
ID: pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/frontendIPConfigurations/atest-IPv6"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
ID: pointer.String("testCluster-atest-id-IPv6"),
},
},
},
},
existingPIPs: []network.PublicIPAddress{
{
Name: pointer.String("testCluster-atest"),
ID: pointer.String("testCluster-atest-id"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
PublicIPAllocationMethod: network.Static,
IPAddress: pointer.String("1.2.3.5"),
},
},
{
Name: pointer.String("testCluster-atest-IPv6"),
ID: pointer.String("testCluster-atest-id-IPv6"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv6,
PublicIPAllocationMethod: network.Static,
IPAddress: pointer.String("fe::2"),
},
},
{
Name: pointer.String("pipV4"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
IPAddress: pointer.String("1.2.3.4"),
},
},
{
Name: pointer.String("pipV6"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv6,
IPAddress: pointer.String("fe::1"),
},
},
},
status: nil,
wantLB: true,
expectedDirty: false,
expectedFIPs: []network.FrontendIPConfiguration{
{
Name: pointer.String("fipV4"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
Name: pointer.String("pipV4"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
IPAddress: pointer.String("1.2.3.4"),
},
},
},
},
{
Name: pointer.String("fipV6"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
Name: pointer.String("pipV6"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv6,
IPAddress: pointer.String("fe::1"),
},
},
},
},
{
Name: pointer.String("atest"),
ID: pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/frontendIPConfigurations/atest"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
ID: pointer.String("testCluster-atest-id"),
},
},
},
{
Name: pointer.String("atest-IPv6"),
ID: pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/frontendIPConfigurations/atest-IPv6"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
ID: pointer.String("testCluster-atest-id-IPv6"),
},
},
},
},
},
{
desc: "DualStack Service reconciles existing FIPs, wantLB == false, but an FIP ID is empty, should return error",
service: getTestServiceDualStack("test", v1.ProtocolTCP, nil, 80),
existingFIPs: []network.FrontendIPConfiguration{
{
Name: pointer.String("atest"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
ID: pointer.String("testCluster-atest-id"),
},
},
},
{
Name: pointer.String("atest-IPv6"),
ID: pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/frontendIPConfigurations/atest-IPv6"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
ID: pointer.String("testCluster-atest-id-IPv6"),
},
},
},
},
status: nil,
wantLB: false,
expectedErr: fmt.Errorf("isFrontendIPConfigUnsafeToDelete: incorrect parameters"),
},
{
desc: "IPv6 Service with existing IPv4 FIP",
service: getTestService("test", v1.ProtocolTCP, nil, true, 80),
existingFIPs: []network.FrontendIPConfiguration{
{
Name: pointer.String("fipV4"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
Name: pointer.String("pipV4"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
IPAddress: pointer.String("1.2.3.4"),
},
},
},
},
},
existingPIPs: []network.PublicIPAddress{
{
Name: pointer.String("testCluster-atest"),
ID: pointer.String("testCluster-atest-id"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv6,
PublicIPAllocationMethod: network.Static,
IPAddress: pointer.String("fe::1"),
},
},
{
Name: pointer.String("pipV4"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
IPAddress: pointer.String("1.2.3.4"),
},
},
},
status: nil,
wantLB: true,
expectedDirty: true,
expectedFIPs: []network.FrontendIPConfiguration{
{
Name: pointer.String("fipV4"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
Name: pointer.String("pipV4"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
IPAddress: pointer.String("1.2.3.4"),
},
},
},
},
{
Name: pointer.String("atest"),
ID: pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lb/frontendIPConfigurations/atest"),
FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
PublicIPAddress: &network.PublicIPAddress{
ID: pointer.String("testCluster-atest-id"),
},
},
},
},
},
}

for _, tc := range testcases {
t.Run(tc.desc, func(t *testing.T) {
cloud := GetTestCloud(ctrl)
cloud.LoadBalancerSku = string(network.LoadBalancerSkuNameStandard)

lb := getTestLoadBalancer(pointer.String("lb"), pointer.String("rg"), pointer.String("testCluster"), pointer.String("testCluster"), tc.service, "standard")
existingFIPs := tc.existingFIPs
lb.FrontendIPConfigurations = &existingFIPs

mockPIPClient := cloud.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPClient.EXPECT().List(gomock.Any(), "rg").Return(tc.existingPIPs, nil).MaxTimes(2)
for _, pip := range tc.existingPIPs {
mockPIPClient.EXPECT().Get(gomock.Any(), "rg", *pip.Name, gomock.Any()).Return(pip, nil).MaxTimes(1)
}

service := tc.service
isDualStack := isServiceDualStack(&service)
defaultLBFrontendIPConfigName := cloud.getDefaultFrontendIPConfigName(&service)
lbFrontendIPConfigNames := map[bool]string{
false: getResourceByIPFamily(defaultLBFrontendIPConfigName, isDualStack, false),
true: getResourceByIPFamily(defaultLBFrontendIPConfigName, isDualStack, true),
}
_, _, dirty, err := cloud.reconcileFrontendIPConfigs("testCluster", &service, &lb, tc.status, tc.wantLB, lbFrontendIPConfigNames)
if tc.expectedErr != nil {
assert.Equal(t, tc.expectedErr, err)
} else {
assert.Nil(t, err)
assert.Equal(t, tc.expectedDirty, dirty)
assert.Equal(t, tc.expectedFIPs, *lb.FrontendIPConfigurations)
}
})
}
}

func TestReconcileIPSettings(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down
16 changes: 12 additions & 4 deletions pkg/provider/azure_privatelinkservice.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,19 @@ func (az *Cloud) reconcilePrivateLinkService(
wantPLS bool,
) error {
isinternal := requiresInternalLoadBalancer(service)
isIPv6, err := az.isFIPIPv6(service, fipConfig, isinternal)
if err != nil {
klog.Errorf("reconcilePrivateLinkService for service(%s): failed to get FIP IP family: %v", service, err)
return err
pipRG := az.getPublicIPAddressResourceGroup(service)
_, _, fipIPVersion := az.serviceOwnsFrontendIP(*fipConfig, service)
var isIPv6 bool
var err error
if fipIPVersion != "" {
isIPv6 = fipIPVersion == network.IPv6
} else {
if isIPv6, err = az.isFIPIPv6(service, pipRG, fipConfig); err != nil {
klog.Errorf("reconcilePrivateLinkService for service(%s): failed to get FIP IP family: %v", service, err)
return err
}
}

if isIPv6 {
klog.V(2).Infof("IPv6 is not supported for private link service, skip reconcilePrivateLinkService for service(%s)", service)
return nil
Expand Down
13 changes: 0 additions & 13 deletions pkg/provider/azure_privatelinkservice_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"k8s.io/utils/pointer"

"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/privatelinkserviceclient/mockprivatelinkserviceclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/publicipclient/mockpublicipclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/subnetclient/mocksubnetclient"
"sigs.k8s.io/cloud-provider-azure/pkg/consts"
"sigs.k8s.io/cloud-provider-azure/pkg/retry"
Expand Down Expand Up @@ -337,18 +336,6 @@ func TestReconcilePrivateLinkService(t *testing.T) {
}
clusterName := testClusterName

if isInternal, ok := test.annotations[consts.ServiceAnnotationLoadBalancerInternal]; !ok || isInternal != "true" {
existingPIPS := []network.PublicIPAddress{
{
ID: pointer.String("pipID"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
PublicIPAddressVersion: network.IPv4,
},
},
}
mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(existingPIPS, nil).Times(1)
}
mockSubnetsClient := az.SubnetsClient.(*mocksubnetclient.MockInterface)
mockPLSsClient := az.PrivateLinkServiceClient.(*mockprivatelinkserviceclient.MockInterface)
if test.expectedSubnetGet {
Expand Down

0 comments on commit df99c1c

Please sign in to comment.