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 May 19, 2023
1 parent 219610a commit d462f8f
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 86 deletions.
56 changes: 33 additions & 23 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1368,12 +1368,13 @@ func (az *Cloud) isFrontendIPChanged(
if !strings.EqualFold(pointer.StringDeref(config.Name, ""), lbFrontendIPConfigName) {
return false, nil
}
isInternal := requiresInternalLoadBalancer(service)
isIPv6, err := az.isFIPIPv6(service, &config, isInternal)
pipRG := az.getPublicIPAddressResourceGroup(service)
isIPv6, err := az.isFIPIPv6(pipRG, &config)
if err != nil {
return false, err
}
loadBalancerIP := getServiceLoadBalancerIP(service, isIPv6)
isInternal := requiresInternalLoadBalancer(service)
if isInternal {
// Judge subnet
subnetName := getInternalSubnet(service)
Expand All @@ -1392,8 +1393,7 @@ func (az *Cloud) isFrontendIPChanged(
if err != nil {
return false, err
}
pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName, azcache.CacheReadTypeDefault)
pip, existsPip, err := az.getPublicIPAddress(pipRG, pipName, azcache.CacheReadTypeDefault)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -1520,9 +1520,10 @@ func (az *Cloud) findFrontendIPConfigOfService(
isInternal bool,
isIPv6 bool,
) (*network.FrontendIPConfiguration, error) {
pipRG := az.getPublicIPAddressResourceGroup(service)
for _, config := range *fipConfigs {
config := config
fipIsIPv6, err := az.isFIPIPv6(service, &config, isInternal)
fipIsIPv6, err := az.isFIPIPv6(pipRG, &config)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1596,7 +1597,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
}

// update probes/rules
isInternal := requiresInternalLoadBalancer(service)
pipRG := az.getPublicIPAddressResourceGroup(service)
for _, ownedFIPConfig := range ownedFIPConfigs {
if ownedFIPConfig == nil {
continue
Expand All @@ -1605,7 +1606,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
return nil, fmt.Errorf("reconcileLoadBalancer for service (%s)(%t): nil ID for frontend IP config", serviceName, wantLb)
}

isIPv6, err := az.isFIPIPv6(service, ownedFIPConfig, isInternal)
isIPv6, err := az.isFIPIPv6(pipRG, ownedFIPConfig)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1820,31 +1821,53 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
lbFrontEndIPConfigNewConfigs := []network.FrontendIPConfiguration{}
changedTotal := false
var subnet network.Subnet

newConfigs := map[bool][]network.FrontendIPConfiguration{}
pipRG := az.getPublicIPAddressResourceGroup(service)
// Put all LB's FIPs into newConfigs according to IP family
if lb.FrontendIPConfigurations != nil {
for i := range *lb.FrontendIPConfigurations {
fip := (*lb.FrontendIPConfigurations)[i]
fipIsIPv6, err := az.isFIPIPv6(pipRG, &fip)
if err != nil {
return ownedFIPConfigs, toDeleteConfigs, changedTotal, err
}
newConfigs[fipIsIPv6] = append(newConfigs[fipIsIPv6], fip)
}
}

handleFrontendIPConfig := func(isIPv6 bool) error {
ownedFIPConfig, toDeleteConfigsSingleStack, changed, newConfigs, err := az.reconcileFrontendIPConfigsSingleStack(clusterName, service, lb, lbStatus, wantLb, isIPv6, lbFrontendIPConfigName[isIPv6], &subnet)
ownedFIPConfig, toDeleteConfigsSingleStack, changed, newConfigs, err := az.reconcileFrontendIPConfigsSingleStack(clusterName, service, lb, lbStatus, wantLb, isIPv6, newConfigs[isIPv6], lbFrontendIPConfigName[isIPv6], &subnet)
if err != nil {
return err
}

if changed {
lbFrontEndIPConfigNewConfigs = append(lbFrontEndIPConfigNewConfigs, newConfigs...)
changedTotal = true
}
lbFrontEndIPConfigNewConfigs = append(lbFrontEndIPConfigNewConfigs, newConfigs...)
ownedFIPConfigs = append(ownedFIPConfigs, ownedFIPConfig)
toDeleteConfigs = append(toDeleteConfigs, toDeleteConfigsSingleStack...)
return nil
}
// Even if one IP family is not enabled for the Service, we still need to put
// FIPs of that IP family into lbFrontEndIPConfigNewConfigs.
v4Enabled, v6Enabled := getIPFamiliesEnabled(service)
if v4Enabled {
if err := handleFrontendIPConfig(false); err != nil {
return ownedFIPConfigs, toDeleteConfigs, changedTotal, err
}
} else {
lbFrontEndIPConfigNewConfigs = append(lbFrontEndIPConfigNewConfigs, newConfigs[false]...)
}
if v6Enabled {
if err := handleFrontendIPConfig(true); err != nil {
return ownedFIPConfigs, toDeleteConfigs, changedTotal, err
}
} else {
lbFrontEndIPConfigNewConfigs = append(lbFrontEndIPConfigNewConfigs, newConfigs[true]...)
}

if changedTotal {
lb.FrontendIPConfigurations = &lbFrontEndIPConfigNewConfigs
}
Expand All @@ -1857,6 +1880,7 @@ func (az *Cloud) reconcileFrontendIPConfigsSingleStack(
lb *network.LoadBalancer,
status *v1.LoadBalancerStatus,
wantLb, isIPv6 bool,
newConfigs []network.FrontendIPConfiguration,
lbFrontendIPConfigName string,
subnet *network.Subnet,
) (*network.FrontendIPConfiguration, []network.FrontendIPConfiguration, bool, []network.FrontendIPConfiguration, error) {
Expand All @@ -1865,21 +1889,7 @@ func (az *Cloud) reconcileFrontendIPConfigsSingleStack(
serviceName := getServiceName(service)
isInternal := requiresInternalLoadBalancer(service)
dirtyConfigs := false
var newConfigs []network.FrontendIPConfiguration
var toDeleteConfigs []network.FrontendIPConfiguration
if lb.FrontendIPConfigurations != nil {
for i := range *lb.FrontendIPConfigurations {
fip := (*lb.FrontendIPConfigurations)[i]
fipIsIPv6, err := az.isFIPIPv6(service, &fip, isInternal)
if err != nil {
return nil, toDeleteConfigs, false, newConfigs, err
}
if fipIsIPv6 == isIPv6 {
newConfigs = append(newConfigs, fip)
}
}
}

var ownedFIPConfig *network.FrontendIPConfiguration
if !wantLb {
for i := len(newConfigs) - 1; i >= 0; i-- {
Expand Down
128 changes: 106 additions & 22 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6401,17 +6401,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 @@ -6443,17 +6432,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 @@ -6687,6 +6665,112 @@ 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
expectedDirty bool
expectedFIPs []network.FrontendIPConfiguration
}{
{
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,
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(1)
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, true, lbFrontendIPConfigNames)
assert.Nil(t, err)
assert.Equal(t, tc.expectedDirty, dirty)
assert.Equal(t, tc.expectedFIPs, *lb.FrontendIPConfigurations)
})
}
}

func TestReconcileSharedLoadBalancer(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down
3 changes: 2 additions & 1 deletion pkg/provider/azure_privatelinkservice.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ func (az *Cloud) reconcilePrivateLinkService(
wantPLS bool,
) error {
isinternal := requiresInternalLoadBalancer(service)
isIPv6, err := az.isFIPIPv6(service, fipConfig, isinternal)
pipRG := az.getPublicIPAddressResourceGroup(service)
isIPv6, err := az.isFIPIPv6(pipRG, fipConfig)
if err != nil {
klog.Errorf("reconcilePrivateLinkService for service(%s): failed to get FIP IP family: %v", service, err)
return err
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
30 changes: 12 additions & 18 deletions pkg/provider/azure_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,27 +447,23 @@ func getResourceByIPFamily(resource string, isDualStack, isIPv6 bool) string {
}

// isFIPIPv6 checks if the frontend IP configuration is of IPv6.
func (az *Cloud) isFIPIPv6(service *v1.Service, fip *network.FrontendIPConfiguration, isInternal bool) (bool, error) {
if isInternal {
if fip.FrontendIPConfigurationPropertiesFormat != nil {
if fip.FrontendIPConfigurationPropertiesFormat.PrivateIPAddressVersion != "" {
return fip.FrontendIPConfigurationPropertiesFormat.PrivateIPAddressVersion == network.IPv6, nil
}
if fip.FrontendIPConfigurationPropertiesFormat.PrivateIPAddress != nil {
return net.ParseIP(*fip.FrontendIPConfigurationPropertiesFormat.PrivateIPAddress).To4() == nil, nil
}
func (az *Cloud) isFIPIPv6(rg string, fip *network.FrontendIPConfiguration) (bool, error) {
// First check private
if fip.FrontendIPConfigurationPropertiesFormat != nil {
if fip.FrontendIPConfigurationPropertiesFormat.PrivateIPAddressVersion != "" {
return fip.FrontendIPConfigurationPropertiesFormat.PrivateIPAddressVersion == network.IPv6, nil
}
if fip.FrontendIPConfigurationPropertiesFormat.PrivateIPAddress != nil {
return net.ParseIP(*fip.FrontendIPConfigurationPropertiesFormat.PrivateIPAddress).To4() == nil, nil
}
klog.Errorf("Checking IP Family of frontend IP configuration %q of internal Service but "+
"it is not clear. It's considered to be IPv4",
pointer.StringDeref(fip.Name, ""))
return false, nil
}

// Then Check public
var fipPIPID string
if fip.FrontendIPConfigurationPropertiesFormat != nil && fip.FrontendIPConfigurationPropertiesFormat.PublicIPAddress != nil {
fipPIPID = pointer.StringDeref(fip.FrontendIPConfigurationPropertiesFormat.PublicIPAddress.ID, "")
}
pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
pips, err := az.listPIP(pipResourceGroup, azcache.CacheReadTypeDefault)
pips, err := az.listPIP(rg, azcache.CacheReadTypeDefault)
if err != nil {
return false, err
}
Expand All @@ -485,11 +481,9 @@ func (az *Cloud) isFIPIPv6(service *v1.Service, fip *network.FrontendIPConfigura
return net.ParseIP(pointer.StringDeref(pip.PublicIPAddressPropertiesFormat.IPAddress, "")).To4() == nil, nil
}
}
klog.Errorf("Checking IP Family of PIP %q of corresponding frontend IP configuration %q of external Service but "+
"it is not clear. It's considered to be IPv4",
pointer.StringDeref(pip.Name, ""), pointer.StringDeref(fip.Name, ""))
break
}
klog.Errorf("Checking IP Family of frontend IP configuration %q but it is not clear. It's considered to be IPv4", pointer.StringDeref(fip.Name, ""))
return false, nil
}

Expand Down

0 comments on commit d462f8f

Please sign in to comment.