Skip to content

Commit

Permalink
Merge pull request #3890 from nilo19/fix/cherry-pick-3877-1.26
Browse files Browse the repository at this point in the history 
[release-1.26] fix: the pip without tags should be user-assigned
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed May 18, 2023
2 parents 7932ee3 + 8591dd4 commit 688fd66
Show file tree
Hide file tree
Showing 7 changed files with 176 additions and 90 deletions.
172 changes: 105 additions & 67 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -912,19 +912,36 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service)
}

func (az *Cloud) findMatchedPIPByLoadBalancerIP(service *v1.Service, loadBalancerIP, pipResourceGroup string) (*network.PublicIPAddress, error) {
pips, err := az.listPIP(pipResourceGroup)
pips, err := az.listPIP(pipResourceGroup, azcache.CacheReadTypeDefault)
if err != nil {
return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: failed to listPIP: %w", err)
}

pip, err := getExpectedPIPFromListByIPAddress(pips, loadBalancerIP)
if err != nil {
pips, err = az.listPIP(pipResourceGroup, azcache.CacheReadTypeForceRefresh)
if err != nil {
return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: failed to listPIP force refresh: %w", err)
}

pip, err = getExpectedPIPFromListByIPAddress(pips, loadBalancerIP)
if err != nil {
return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: cannot find public IP with IP address %s in resource group %s", loadBalancerIP, pipResourceGroup)
}
}

return pip, nil
}

func getExpectedPIPFromListByIPAddress(pips []network.PublicIPAddress, ip string) (*network.PublicIPAddress, error) {
for _, pip := range pips {
pip := pip
if pip.PublicIPAddressPropertiesFormat.IPAddress != nil &&
*pip.PublicIPAddressPropertiesFormat.IPAddress == loadBalancerIP {
*pip.PublicIPAddressPropertiesFormat.IPAddress == ip {
return &pip, nil
}
}

return nil, fmt.Errorf("findMatchedPIPByLoadBalancerIP: cannot find public IP with IP address %s in resource group %s", loadBalancerIP, pipResourceGroup)
return nil, fmt.Errorf("getExpectedPIPFromListByIPAddress: cannot find public IP with IP address %s", ip)
}

func flipServiceInternalAnnotation(service *v1.Service) *v1.Service {
Expand Down Expand Up @@ -1088,7 +1105,7 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
}

if foundDNSLabelAnnotation {
updatedDNSSettings, err := reconcileDNSSettings(&pip, domainNameLabel, serviceName, pipName)
updatedDNSSettings, err := reconcileDNSSettings(&pip, domainNameLabel, serviceName, pipName, isUserAssignedPIP)
if err != nil {
return nil, fmt.Errorf("ensurePublicIPExists for service(%s): failed to reconcileDNSSettings: %w", serviceName, err)
}
Expand Down Expand Up @@ -1158,7 +1175,11 @@ func (az *Cloud) reconcileIPSettings(pip *network.PublicIPAddress, service *v1.S
return changed
}

func reconcileDNSSettings(pip *network.PublicIPAddress, domainNameLabel, serviceName, pipName string) (bool, error) {
func reconcileDNSSettings(
pip *network.PublicIPAddress,
domainNameLabel, serviceName, pipName string,
isUserAssignedPIP bool,
) (bool, error) {
var changed bool

if existingServiceName := getServiceFromPIPDNSTags(pip.Tags); existingServiceName != "" && !strings.EqualFold(existingServiceName, serviceName) {
Expand Down Expand Up @@ -1187,8 +1208,10 @@ func reconcileDNSSettings(pip *network.PublicIPAddress, domainNameLabel, service
}

if svc := getServiceFromPIPDNSTags(pip.Tags); svc == "" || !strings.EqualFold(svc, serviceName) {
pip.Tags[consts.ServiceUsingDNSKey] = &serviceName
changed = true
if !isUserAssignedPIP {
pip.Tags[consts.ServiceUsingDNSKey] = &serviceName
changed = true
}
}
}

Expand Down Expand Up @@ -3028,7 +3051,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa

pipResourceGroup := az.getPublicIPAddressResourceGroup(service)

pips, err := az.listPIP(pipResourceGroup)
pips, err := az.listPIP(pipResourceGroup, azcache.CacheReadTypeDefault)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -3122,7 +3145,7 @@ func (az *Cloud) getPublicIPUpdates(
owns, isUserAssignedPIP := serviceOwnsPublicIP(service, &pip, clusterName)
if owns {
var dirtyPIP, toBeDeleted bool
if !wantLb {
if !wantLb && !isUserAssignedPIP {
klog.V(2).Infof("reconcilePublicIP for service(%s): unbinding the service from pip %s", serviceName, *pip.Name)
if err = unbindServiceFromPIP(&pip, service, serviceName, clusterName, isUserAssignedPIP); err != nil {
return false, nil, false, nil, err
Expand Down Expand Up @@ -3167,65 +3190,77 @@ func (az *Cloud) getPublicIPUpdates(
func (az *Cloud) safeDeletePublicIP(service *v1.Service, pipResourceGroup string, pip *network.PublicIPAddress, lb *network.LoadBalancer) error {
// Remove references if pip.IPConfiguration is not nil.
if pip.PublicIPAddressPropertiesFormat != nil &&
pip.PublicIPAddressPropertiesFormat.IPConfiguration != nil &&
lb != nil && lb.LoadBalancerPropertiesFormat != nil &&
lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations != nil {
referencedLBRules := []network.SubResource{}
frontendIPConfigUpdated := false
loadBalancerRuleUpdated := false

// Check whether there are still frontend IP configurations referring to it.
ipConfigurationID := pointer.StringDeref(pip.PublicIPAddressPropertiesFormat.IPConfiguration.ID, "")
if ipConfigurationID != "" {
lbFrontendIPConfigs := *lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations
for i := len(lbFrontendIPConfigs) - 1; i >= 0; i-- {
config := lbFrontendIPConfigs[i]
if strings.EqualFold(ipConfigurationID, pointer.StringDeref(config.ID, "")) {
if config.FrontendIPConfigurationPropertiesFormat != nil &&
config.FrontendIPConfigurationPropertiesFormat.LoadBalancingRules != nil {
referencedLBRules = *config.FrontendIPConfigurationPropertiesFormat.LoadBalancingRules
}
pip.PublicIPAddressPropertiesFormat.IPConfiguration != nil {
// Fetch latest pip to check if the pip in the cache is stale.
// In some cases the public IP to be deleted is still referencing
// the frontend IP config on the LB. This is because the pip is
// stored in the cache and is not up-to-date.
latestPIP, ok, err := az.getPublicIPAddress(pipResourceGroup, *pip.Name, azcache.CacheReadTypeForceRefresh)
if err != nil {
klog.Errorf("safeDeletePublicIP: failed to get latest public IP %s/%s: %s", pipResourceGroup, *pip.Name, err.Error())
return err
}
if ok && latestPIP.PublicIPAddressPropertiesFormat != nil &&
latestPIP.PublicIPAddressPropertiesFormat.IPConfiguration != nil &&
lb != nil && lb.LoadBalancerPropertiesFormat != nil &&
lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations != nil {
referencedLBRules := []network.SubResource{}
frontendIPConfigUpdated := false
loadBalancerRuleUpdated := false

// Check whether there are still frontend IP configurations referring to it.
ipConfigurationID := pointer.StringDeref(pip.PublicIPAddressPropertiesFormat.IPConfiguration.ID, "")
if ipConfigurationID != "" {
lbFrontendIPConfigs := *lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations
for i := len(lbFrontendIPConfigs) - 1; i >= 0; i-- {
config := lbFrontendIPConfigs[i]
if strings.EqualFold(ipConfigurationID, pointer.StringDeref(config.ID, "")) {
if config.FrontendIPConfigurationPropertiesFormat != nil &&
config.FrontendIPConfigurationPropertiesFormat.LoadBalancingRules != nil {
referencedLBRules = *config.FrontendIPConfigurationPropertiesFormat.LoadBalancingRules
}

frontendIPConfigUpdated = true
lbFrontendIPConfigs = append(lbFrontendIPConfigs[:i], lbFrontendIPConfigs[i+1:]...)
break
frontendIPConfigUpdated = true
lbFrontendIPConfigs = append(lbFrontendIPConfigs[:i], lbFrontendIPConfigs[i+1:]...)
break
}
}
}

if frontendIPConfigUpdated {
lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations = &lbFrontendIPConfigs
if frontendIPConfigUpdated {
lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations = &lbFrontendIPConfigs
}
}
}

// Check whether there are still load balancer rules referring to it.
if len(referencedLBRules) > 0 {
referencedLBRuleIDs := sets.New[string]()
for _, refer := range referencedLBRules {
referencedLBRuleIDs.Insert(pointer.StringDeref(refer.ID, ""))
}
// Check whether there are still load balancer rules referring to it.
if len(referencedLBRules) > 0 {
referencedLBRuleIDs := sets.New[string]()
for _, refer := range referencedLBRules {
referencedLBRuleIDs.Insert(pointer.StringDeref(refer.ID, ""))
}

if lb.LoadBalancerPropertiesFormat.LoadBalancingRules != nil {
lbRules := *lb.LoadBalancerPropertiesFormat.LoadBalancingRules
for i := len(lbRules) - 1; i >= 0; i-- {
ruleID := pointer.StringDeref(lbRules[i].ID, "")
if ruleID != "" && referencedLBRuleIDs.Has(ruleID) {
loadBalancerRuleUpdated = true
lbRules = append(lbRules[:i], lbRules[i+1:]...)
if lb.LoadBalancerPropertiesFormat.LoadBalancingRules != nil {
lbRules := *lb.LoadBalancerPropertiesFormat.LoadBalancingRules
for i := len(lbRules) - 1; i >= 0; i-- {
ruleID := pointer.StringDeref(lbRules[i].ID, "")
if ruleID != "" && referencedLBRuleIDs.Has(ruleID) {
loadBalancerRuleUpdated = true
lbRules = append(lbRules[:i], lbRules[i+1:]...)
}
}
}

if loadBalancerRuleUpdated {
lb.LoadBalancerPropertiesFormat.LoadBalancingRules = &lbRules
if loadBalancerRuleUpdated {
lb.LoadBalancerPropertiesFormat.LoadBalancingRules = &lbRules
}
}
}
}

// Update load balancer when frontendIPConfigUpdated or loadBalancerRuleUpdated.
if frontendIPConfigUpdated || loadBalancerRuleUpdated {
err := az.CreateOrUpdateLB(service, *lb)
if err != nil {
klog.Errorf("safeDeletePublicIP for service(%s) failed with error: %v", getServiceName(service), err)
return err
// Update load balancer when frontendIPConfigUpdated or loadBalancerRuleUpdated.
if frontendIPConfigUpdated || loadBalancerRuleUpdated {
err := az.CreateOrUpdateLB(service, *lb)
if err != nil {
klog.Errorf("safeDeletePublicIP for service(%s) failed with error: %v", getServiceName(service), err)
return err
}
}
}
}
Expand Down Expand Up @@ -3490,7 +3525,7 @@ func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clus

// if there is no service tag on the pip, it is user-created pip
if serviceTag == "" {
return strings.EqualFold(pointer.StringDeref(pip.IPAddress, ""), getServiceLoadBalancerIP(service)), true
return isServiceLoadBalancerIPMatchesPIP(service, pip), true
}

// if there is service tag on the pip, it is system-created pip
Expand All @@ -3502,17 +3537,20 @@ func serviceOwnsPublicIP(service *v1.Service, pip *network.PublicIPAddress, clus
}

// If cluster name tag is set, then return true if it matches.
if clusterTag == clusterName {
return true, false
}
} else {
// if the service is not included in the tags of the system-created pip, check the ip address
// this could happen for secondary services
return strings.EqualFold(pointer.StringDeref(pip.IPAddress, ""), getServiceLoadBalancerIP(service)), false
return strings.EqualFold(clusterTag, clusterName), false
}

// if the service is not included in the tags of the system-created pip, check the ip address
// this could happen for secondary services
return isServiceLoadBalancerIPMatchesPIP(service, pip), false
}

return false, false
// if the pip has no tags, it should be user-created
return isServiceLoadBalancerIPMatchesPIP(service, pip), true
}

func isServiceLoadBalancerIPMatchesPIP(service *v1.Service, pip *network.PublicIPAddress) bool {
return strings.EqualFold(pointer.StringDeref(pip.IPAddress, ""), getServiceLoadBalancerIP(service))
}

func isSVCNameInPIPTag(tag, svcName string) bool {
Expand Down
77 changes: 62 additions & 15 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -959,6 +959,17 @@ func TestServiceOwnsPublicIP(t *testing.T) {
serviceLBIP: "1.1.1.1",
expectedOwns: true,
},
{
desc: "should be user-assigned pip if it has no tags",
pip: &network.PublicIPAddress{
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPAddress: pointer.String("1.2.3.4"),
},
},
serviceLBIP: "1.2.3.4",
expectedOwns: true,
expectedUserAssignedPIP: true,
},
}

for i, c := range tests {
Expand Down Expand Up @@ -2059,20 +2070,28 @@ func TestFindMatchedPIPByLoadBalancerIP(t *testing.T) {
},
}
testCases := []struct {
desc string
pips []network.PublicIPAddress
expectedPIP *network.PublicIPAddress
expectedError bool
desc string
pips []network.PublicIPAddress
shouldRefreshCache bool
expectedPIP *network.PublicIPAddress
expectedError bool
}{
{
desc: "findMatchedPIPByLoadBalancerIP shall return the matched ip",
pips: []network.PublicIPAddress{testPIP},
expectedPIP: &testPIP,
},
{
desc: "findMatchedPIPByLoadBalancerIP shall return error if ip is not found",
pips: []network.PublicIPAddress{},
expectedError: true,
desc: "findMatchedPIPByLoadBalancerIP shall return error if ip is not found",
pips: []network.PublicIPAddress{},
shouldRefreshCache: true,
expectedError: true,
},
{
desc: "findMatchedPIPByLoadBalancerIP should refresh cache if no matched ip is found",
pips: []network.PublicIPAddress{testPIP},
shouldRefreshCache: true,
expectedPIP: &testPIP,
},
}
for _, test := range testCases {
Expand All @@ -2082,6 +2101,9 @@ func TestFindMatchedPIPByLoadBalancerIP(t *testing.T) {
setServiceLoadBalancerIP(&service, "1.2.3.4")

mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
if test.shouldRefreshCache {
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return([]network.PublicIPAddress{}, nil)
}
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.pips, nil)
pip, err := az.findMatchedPIPByLoadBalancerIP(&service, "1.2.3.4", "rg")
assert.Equal(t, test.expectedPIP, pip)
Expand Down Expand Up @@ -2137,7 +2159,7 @@ func TestDeterminePublicIPName(t *testing.T) {
setServiceLoadBalancerIP(&service, test.loadBalancerIP)

mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).MaxTimes(1)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).MaxTimes(2)
mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
for _, existingPIP := range test.existingPIPs {
mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *existingPIP.Name, gomock.Any()).Return(existingPIP, nil).AnyTimes()
Expand Down Expand Up @@ -3859,7 +3881,8 @@ func TestSafeDeletePublicIP(t *testing.T) {
desc string
pip *network.PublicIPAddress
lb *network.LoadBalancer
expectedError bool
listError *retry.Error
expectedError error
}{
{
desc: "safeDeletePublicIP shall delete corresponding ip configurations and lb rules",
Expand All @@ -3886,12 +3909,30 @@ func TestSafeDeletePublicIP(t *testing.T) {
},
},
},
{
desc: "safeDeletePublicIP should return error if failed to list pip",
pip: &network.PublicIPAddress{
Name: pointer.String("pip1"),
PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
IPConfiguration: &network.IPConfiguration{
ID: pointer.String("id1"),
},
},
},
listError: retry.NewError(false, errors.New("error")),
expectedError: retry.NewError(false, errors.New("error")).Error(),
},
}

for _, test := range testCases {
t.Run(test.desc, func(t *testing.T) {
az := GetTestCloud(ctrl)
mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
if test.pip != nil &&
test.pip.PublicIPAddressPropertiesFormat != nil &&
test.pip.IPConfiguration != nil {
mockPIPsClient.EXPECT().List(gomock.Any(), gomock.Any()).Return([]network.PublicIPAddress{*test.pip}, test.listError)
}
mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", "pip1", gomock.Any()).Return(nil).AnyTimes()
mockPIPsClient.EXPECT().Delete(gomock.Any(), "rg", "pip1").Return(nil).AnyTimes()
err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", "pip1", network.PublicIPAddress{
Expand All @@ -3904,13 +3945,19 @@ func TestSafeDeletePublicIP(t *testing.T) {
})
assert.NoError(t, err.Error())
service := getTestService("test1", v1.ProtocolTCP, nil, false, 80)
mockLBsClient := mockloadbalancerclient.NewMockInterface(ctrl)
mockLBsClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
az.LoadBalancerClient = mockLBsClient
if test.listError == nil {
mockLBsClient := mockloadbalancerclient.NewMockInterface(ctrl)
mockLBsClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
az.LoadBalancerClient = mockLBsClient
}
rerr := az.safeDeletePublicIP(&service, "rg", test.pip, test.lb)
assert.Equal(t, 0, len(*test.lb.FrontendIPConfigurations))
assert.Equal(t, 0, len(*test.lb.LoadBalancingRules))
assert.Equal(t, test.expectedError, rerr != nil)
if test.expectedError == nil {
assert.Equal(t, 0, len(*test.lb.FrontendIPConfigurations))
assert.Equal(t, 0, len(*test.lb.LoadBalancingRules))
assert.NoError(t, rerr)
} else {
assert.Equal(t, rerr.Error(), test.listError.Error().Error())
}
})
}
}
Expand Down

0 comments on commit 688fd66

Please sign in to comment.