Skip to content

Commit

Permalink
Merge pull request #3892 from nilo19/fix/cherry-pick-3877-1.24
Browse files Browse the repository at this point in the history 
[release-1.24] fix: the pip without tags should be user-assigned
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed May 18, 2023
2 parents 275d68c + fbf4539 commit 5027b01
Show file tree
Hide file tree
Showing 7 changed files with 178 additions and 92 deletions.
176 changes: 107 additions & 69 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ func (az *Cloud) reconcileService(ctx context.Context, clusterName string, servi
// EnsureLoadBalancer creates a new load balancer 'name', or updates the existing one. Returns the status of the balancer
func (az *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
// When a client updates the internal load balancer annotation,
// the service may be switched from an internal LB to a public one, or vise versa.
// the service may be switched from an internal LB to a public one, or vice versa.
// Here we'll firstly ensure service do not lie in the opposite LB.

// Serialize service reconcile process
Expand Down Expand Up @@ -543,7 +543,7 @@ func (az *Cloud) safeDeleteLoadBalancer(lb network.LoadBalancer, clusterName, vm
// 1. Using multiple slbs and the vmSet is supposed to share the primary slb.
// 2. When migrating from multiple slbs to single slb mode.
// It also ensures those vmSets are joint the backend pools of the primary SLBs.
// It runs only once everytime the cloud controller manager restarts.
// It runs only once every time the cloud controller manager restarts.
func (az *Cloud) reconcileSharedLoadBalancer(service *v1.Service, clusterName string, nodes []*v1.Node) ([]network.LoadBalancer, error) {
var (
existingLBs []network.LoadBalancer
Expand Down 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.NewString()
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.NewString()
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 te 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

0 comments on commit 5027b01

Please sign in to comment.