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 2eb2fab commit 20dfd9c
Show file tree
Hide file tree
Showing 5 changed files with 161 additions and 51 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
106 changes: 106 additions & 0 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6687,6 +6687,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
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
17 changes: 8 additions & 9 deletions pkg/provider/azure_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -900,13 +900,12 @@ func TestGetResourceByIPFamily(t *testing.T) {
func TestIsFIPIPv6(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
svc := v1.Service{}

testcases := []struct {
desc string
fip *network.FrontendIPConfiguration
pips []network.PublicIPAddress
isInternal bool
shouldListPIP bool
expectedIsIPv6 bool
}{
{
Expand All @@ -918,7 +917,7 @@ func TestIsFIPIPv6(t *testing.T) {
},
},
[]network.PublicIPAddress{},
true,
false,
false,
},
{
Expand All @@ -929,7 +928,7 @@ func TestIsFIPIPv6(t *testing.T) {
},
},
[]network.PublicIPAddress{},
true,
false,
false,
},
{
Expand Down Expand Up @@ -966,7 +965,7 @@ func TestIsFIPIPv6(t *testing.T) {
},
},
},
false,
true,
true,
},
{
Expand All @@ -993,7 +992,7 @@ func TestIsFIPIPv6(t *testing.T) {
},
},
},
false,
true,
true,
},
{
Expand All @@ -1018,18 +1017,18 @@ func TestIsFIPIPv6(t *testing.T) {
},
},
},
false,
true,
false,
},
}
for _, tc := range testcases {
t.Run(tc.desc, func(t *testing.T) {
az := GetTestCloud(ctrl)
mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
if !tc.isInternal {
if tc.shouldListPIP {
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(tc.pips, nil)
}
isIPv6, err := az.isFIPIPv6(&svc, tc.fip, tc.isInternal)
isIPv6, err := az.isFIPIPv6("rg", tc.fip)
assert.Nil(t, err)
assert.Equal(t, tc.expectedIsIPv6, isIPv6)
})
Expand Down

0 comments on commit 20dfd9c

Please sign in to comment.