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()