From 862dee928f30c3010b6f0a1946f66fe4468f2f9a Mon Sep 17 00:00:00 2001 From: Daniel Bosscher Date: Thu, 18 Sep 2025 13:56:08 +0000 Subject: [PATCH] fix: update IP tags on managed PIPs instead of recreating them When IP tags specified in service annotations don't match the existing public IP tags, the current logic deletes and recreates the entire PIP. This is inefficient and causes unnecessary downtime. This change: - Removes IP tag mismatch as a reason to delete PIPs in shouldReleaseExistingOwnedPublicIP - Adds ensurePIPIPTagged function to update IP tags in-place - Integrates IP tag updates into the PIP reconciliation flow - Updates existing tests to reflect the new behavior - Adds comprehensive tests for the new IP tag update functionality Benefits: - Improved performance by avoiding PIP recreation - Reduced service downtime during IP tag updates - Consistent handling of IP tags similar to regular tags Fixes: https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/pkg/provider/azure_loadbalancer.go#L3242 --- pkg/provider/azure_loadbalancer.go | 44 +++++-- pkg/provider/azure_loadbalancer_test.go | 164 +++++++++++++++++++++++- 2 files changed, 195 insertions(+), 13 deletions(-) diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go index e89b835bdb..9ca3262b1e 100644 --- a/pkg/provider/azure_loadbalancer.go +++ b/pkg/provider/azure_loadbalancer.go @@ -3270,12 +3270,6 @@ func shouldReleaseExistingOwnedPublicIP( // Latch some variables for readability purposes. pipName := *existingPip.Name - // Assume the current IP Tags are empty by default unless properties specify otherwise. - currentIPTags := []*armnetwork.IPTag{} - if existingPip.Properties != nil { - currentIPTags = existingPip.Properties.IPTags - } - // Check whether the public IP is being referenced by other service. // The owned public IP can be released only when there is not other service using it. // case 1: there is at least one reference when deleting the PIP @@ -3296,9 +3290,9 @@ func shouldReleaseExistingOwnedPublicIP( // #3 - If the name of this public ip does not match the desired name, // NOTICE: For IPv6 Service created with CCM v1.27.1, the created PIP has IPv6 suffix. // We need to recreate such PIP and current logic to delete needs no change. - (pipName != desiredPipName) || - // #4 If the service annotations have specified the ip tags that the public ip must have, but they do not match the ip tags of the existing instance - (ipTagRequest.IPTagsRequestedByAnnotation && !areIPTagsEquivalent(currentIPTags, ipTagRequest.IPTags)) + (pipName != desiredPipName) + // #4 IP tags mismatch should be handled by updating the PIP, not recreating it + // (ipTagRequest.IPTagsRequestedByAnnotation && !areIPTagsEquivalent(currentIPTags, ipTagRequest.IPTags)) } // ensurePIPTagged ensures the public IP of the service is tagged as configured @@ -3345,6 +3339,35 @@ func (az *Cloud) ensurePIPTagged(service *v1.Service, pip *armnetwork.PublicIPAd return changed } +// ensurePIPIPTagged ensures the public IP of the service has correct IP tags as configured +func (az *Cloud) ensurePIPIPTagged(service *v1.Service, pip *armnetwork.PublicIPAddress) bool { + serviceIPTagRequest := getServiceIPTagRequestForPublicIP(service) + + // If no IP tags are requested by annotations, no update needed + if !serviceIPTagRequest.IPTagsRequestedByAnnotation { + return false + } + + // Get current IP tags from the PIP + currentIPTags := []*armnetwork.IPTag{} + if pip.Properties != nil { + currentIPTags = pip.Properties.IPTags + } + + // Compare current and desired IP tags + if areIPTagsEquivalent(currentIPTags, serviceIPTagRequest.IPTags) { + return false // No changes needed + } + + // Update the IP tags + if pip.Properties == nil { + pip.Properties = &armnetwork.PublicIPAddressPropertiesFormat{} + } + pip.Properties.IPTags = serviceIPTagRequest.IPTags + + return true // IP tags were updated +} + // reconcilePublicIPs reconciles the PublicIP resources similar to how the LB is reconciled. func (az *Cloud) reconcilePublicIPs(ctx context.Context, clusterName string, service *v1.Service, lbName string, wantLb bool) ([]*armnetwork.PublicIPAddress, error) { logger := klog.FromContext(ctx).WithName("reconcilePublicIPs"). @@ -3541,6 +3564,9 @@ func (az *Cloud) getPublicIPUpdates( if az.ensurePIPTagged(service, pip) { dirtyPIP = true } + if az.ensurePIPIPTagged(service, pip) { + dirtyPIP = true + } } if shouldReleaseExistingOwnedPublicIP(pip, serviceReferences, wantLb, isInternal, isUserAssignedPIP, desiredPipName, serviceIPTagRequest) { // Then, release the public ip diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go index 3c4408876d..d3fcc15789 100644 --- a/pkg/provider/azure_loadbalancer_test.go +++ b/pkg/provider/azure_loadbalancer_test.go @@ -1305,7 +1305,7 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) { expectedShouldRelease: false, }, { - desc: "existing public ip with no format properties (unit test only?), tags required by annotation, expect release", + desc: "existing public ip with no format properties (unit test only?), tags required by annotation, should NOT release (update instead)", existingPip: existingPipWithNoPublicIPAddressFormatProperties, lbShouldExist: true, lbIsInternal: false, @@ -1314,7 +1314,7 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) { IPTagsRequestedByAnnotation: true, IPTags: existingPipWithTag.Properties.IPTags, }, - expectedShouldRelease: true, + expectedShouldRelease: false, }, { desc: "LB no longer desired, expect release", @@ -1353,7 +1353,7 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) { expectedShouldRelease: true, }, { - desc: "mismatching, expect release", + desc: "mismatching IP tags, should NOT release (update instead)", existingPip: existingPipWithTag, lbShouldExist: true, lbIsInternal: false, @@ -1367,7 +1367,7 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) { }, }, }, - expectedShouldRelease: true, + expectedShouldRelease: false, }, { // This test is for IPv6 PIP created with CCM v1.27.1 and CCM gets upgraded. @@ -6391,6 +6391,162 @@ func TestEnsurePIPTagged(t *testing.T) { }) } +func TestEnsurePIPIPTagged(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + cloud := GetTestCloud(ctrl) + + tests := []struct { + name string + service *v1.Service + pip *armnetwork.PublicIPAddress + expectedChanged bool + expectedIPTags []*armnetwork.IPTag + }{ + { + name: "should update IP tags when they differ", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + consts.ServiceAnnotationIPTagsForPublicIP: "tag1=value1,tag2=value2", + }, + }, + }, + pip: &armnetwork.PublicIPAddress{ + Properties: &armnetwork.PublicIPAddressPropertiesFormat{ + IPTags: []*armnetwork.IPTag{ + { + IPTagType: ptr.To("tag1"), + Tag: ptr.To("oldvalue1"), + }, + }, + }, + }, + expectedChanged: true, + expectedIPTags: []*armnetwork.IPTag{ + { + IPTagType: ptr.To("tag1"), + Tag: ptr.To("value1"), + }, + { + IPTagType: ptr.To("tag2"), + Tag: ptr.To("value2"), + }, + }, + }, + { + name: "should not change when IP tags match", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + consts.ServiceAnnotationIPTagsForPublicIP: "tag1=value1", + }, + }, + }, + pip: &armnetwork.PublicIPAddress{ + Properties: &armnetwork.PublicIPAddressPropertiesFormat{ + IPTags: []*armnetwork.IPTag{ + { + IPTagType: ptr.To("tag1"), + Tag: ptr.To("value1"), + }, + }, + }, + }, + expectedChanged: false, + expectedIPTags: []*armnetwork.IPTag{ + { + IPTagType: ptr.To("tag1"), + Tag: ptr.To("value1"), + }, + }, + }, + { + name: "should not change when no IP tags requested", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + pip: &armnetwork.PublicIPAddress{ + Properties: &armnetwork.PublicIPAddressPropertiesFormat{ + IPTags: []*armnetwork.IPTag{ + { + IPTagType: ptr.To("tag1"), + Tag: ptr.To("value1"), + }, + }, + }, + }, + expectedChanged: false, + expectedIPTags: []*armnetwork.IPTag{ + { + IPTagType: ptr.To("tag1"), + Tag: ptr.To("value1"), + }, + }, + }, + { + name: "should add IP tags to PIP with no properties", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + consts.ServiceAnnotationIPTagsForPublicIP: "tag1=value1", + }, + }, + }, + pip: &armnetwork.PublicIPAddress{ + Properties: nil, + }, + expectedChanged: true, + expectedIPTags: []*armnetwork.IPTag{ + { + IPTagType: ptr.To("tag1"), + Tag: ptr.To("value1"), + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Make a copy to avoid modifying the test data + pip := *test.pip + if test.pip.Properties != nil { + propertiesCopy := *test.pip.Properties + pip.Properties = &propertiesCopy + if test.pip.Properties.IPTags != nil { + pip.Properties.IPTags = make([]*armnetwork.IPTag, len(test.pip.Properties.IPTags)) + for i, tag := range test.pip.Properties.IPTags { + tagCopy := *tag + pip.Properties.IPTags[i] = &tagCopy + } + } + } + + changed := cloud.ensurePIPIPTagged(test.service, &pip) + assert.Equal(t, test.expectedChanged, changed, "unexpected changed result") + + if pip.Properties != nil { + // Sort both actual and expected tags for comparison + if pip.Properties.IPTags != nil { + sortIPTags(&pip.Properties.IPTags) + } + expectedCopy := make([]*armnetwork.IPTag, len(test.expectedIPTags)) + for i, tag := range test.expectedIPTags { + tagCopy := *tag + expectedCopy[i] = &tagCopy + } + sortIPTags(&expectedCopy) + assert.Equal(t, expectedCopy, pip.Properties.IPTags, "unexpected IP tags") + } else { + assert.Empty(t, test.expectedIPTags, "expected no IP tags but PIP has no properties") + } + }) + } +} + func TestEnsureLoadBalancerTagged(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish()