Merge pull request #94114 from MarcPow/azure-provider-ip-tags

Azure Cloud Provider should support Service annotations that allow for ip-tag control over the public ips created for LoadBalancer Services

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go

@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"math"
 	"reflect"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -82,6 +83,9 @@ const (
 	// ServiceAnnotationPIPName specifies the pip that will be applied to load balancer
 	ServiceAnnotationPIPName = "service.beta.kubernetes.io/azure-pip-name"
 
+	// ServiceAnnotationIPTagsForPublicIP specifies the iptags used when dynamically creating a public ip
+	ServiceAnnotationIPTagsForPublicIP = "service.beta.kubernetes.io/azure-pip-ip-tags"
+
 	// ServiceAnnotationAllowedServiceTag is the annotation used on the service
 	// to specify a list of allowed service tags separated by comma
 	// Refer https://docs.microsoft.com/en-us/azure/virtual-network/security-overview#service-tags for all supported service tags.
@@ -546,6 +550,7 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 		pip.Location = to.StringPtr(az.Location)
 		pip.PublicIPAddressPropertiesFormat = &network.PublicIPAddressPropertiesFormat{
 			PublicIPAllocationMethod: network.Static,
+			IPTags:                   getServiceIPTagRequestForPublicIP(service).IPTags,
 		}
 		pip.Tags = map[string]*string{
 			serviceTagKey:  &serviceName,
@@ -604,6 +609,93 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 	return &pip, nil
 }
 
+type serviceIPTagRequest struct {
+	IPTagsRequestedByAnnotation bool
+	IPTags                      *[]network.IPTag
+}
+
+// Get the ip tag Request for the public ip from service annotations.
+func getServiceIPTagRequestForPublicIP(service *v1.Service) serviceIPTagRequest {
+	if service != nil {
+		if ipTagString, found := service.Annotations[ServiceAnnotationIPTagsForPublicIP]; found {
+			return serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags:                      convertIPTagMapToSlice(getIPTagMap(ipTagString)),
+			}
+		}
+	}
+
+	return serviceIPTagRequest{
+		IPTagsRequestedByAnnotation: false,
+		IPTags:                      nil,
+	}
+}
+
+func getIPTagMap(ipTagString string) map[string]string {
+	outputMap := make(map[string]string)
+	commaDelimitedPairs := strings.Split(strings.TrimSpace(ipTagString), ",")
+	for _, commaDelimitedPair := range commaDelimitedPairs {
+		splitKeyValue := strings.Split(commaDelimitedPair, "=")
+
+		// Include only valid pairs in the return value
+		// Last Write wins.
+		if len(splitKeyValue) == 2 {
+			tagKey := strings.TrimSpace(splitKeyValue[0])
+			tagValue := strings.TrimSpace(splitKeyValue[1])
+
+			outputMap[tagKey] = tagValue
+		}
+	}
+
+	return outputMap
+}
+
+func sortIPTags(ipTags *[]network.IPTag) {
+	if ipTags != nil {
+		sort.Slice(*ipTags, func(i, j int) bool {
+			ipTag := *ipTags
+			return to.String(ipTag[i].IPTagType) < to.String(ipTag[j].IPTagType) ||
+				to.String(ipTag[i].Tag) < to.String(ipTag[j].Tag)
+		})
+	}
+}
+
+func areIPTagsEquivalent(ipTags1 *[]network.IPTag, ipTags2 *[]network.IPTag) bool {
+	sortIPTags(ipTags1)
+	sortIPTags(ipTags2)
+
+	if ipTags1 == nil {
+		ipTags1 = &[]network.IPTag{}
+	}
+
+	if ipTags2 == nil {
+		ipTags2 = &[]network.IPTag{}
+	}
+
+	return reflect.DeepEqual(ipTags1, ipTags2)
+}
+
+func convertIPTagMapToSlice(ipTagMap map[string]string) *[]network.IPTag {
+	if ipTagMap == nil {
+		return nil
+	}
+
+	if len(ipTagMap) == 0 {
+		return &[]network.IPTag{}
+	}
+
+	outputTags := []network.IPTag{}
+	for k, v := range ipTagMap {
+		ipTag := network.IPTag{
+			IPTagType: to.StringPtr(k),
+			Tag:       to.StringPtr(v),
+		}
+		outputTags = append(outputTags, ipTag)
+	}
+
+	return &outputTags
+}
+
 func getDomainNameLabel(pip *network.PublicIPAddress) string {
 	if pip == nil || pip.PublicIPAddressPropertiesFormat == nil || pip.PublicIPAddressPropertiesFormat.DNSSettings == nil {
 		return ""
@@ -1468,10 +1560,34 @@ func deduplicate(collection *[]string) *[]string {
 	return &result
 }
 
+// Determine if we should release existing owned public IPs
+func shouldReleaseExistingOwnedPublicIP(existingPip *network.PublicIPAddress, lbShouldExist bool, lbIsInternal bool, desiredPipName string, ipTagRequest serviceIPTagRequest) bool {
+	// Latch some variables for readability purposes.
+	pipName := *(*existingPip).Name
+
+	// Assume the current IP Tags are empty by default unless properties specify otherwise.
+	currentIPTags := &[]network.IPTag{}
+	pipPropertiesFormat := (*existingPip).PublicIPAddressPropertiesFormat
+	if pipPropertiesFormat != nil {
+		currentIPTags = (*pipPropertiesFormat).IPTags
+	}
+
+	// Release the ip under the following criteria -
+	// #1 - If we don't actually want a load balancer,
+	return !lbShouldExist ||
+		// #2 - If the load balancer is internal, and thus doesn't require public exposure
+		lbIsInternal ||
+		// #3 - If the name of this public ip does not match the desired name,
+		(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))
+}
+
 // This reconciles the PublicIP resources similar to how the LB is reconciled.
 func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbName string, wantLb bool) (*network.PublicIPAddress, error) {
 	isInternal := requiresInternalLoadBalancer(service)
 	serviceName := getServiceName(service)
+	serviceIPTagRequest := getServiceIPTagRequestForPublicIP(service)
 	var lb *network.LoadBalancer
 	var desiredPipName string
 	var err error
@@ -1498,27 +1614,39 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
 		return nil, err
 	}
 
-	var found bool
+	var serviceAnnotationRequestsNamedPublicIP bool = shouldPIPExisted
+	var discoveredDesiredPublicIP bool
+	var deletedDesiredPublicIP bool
 	var pipsToBeDeleted []*network.PublicIPAddress
 	for i := range pips {
 		pip := pips[i]
 		pipName := *pip.Name
-		if serviceOwnsPublicIP(&pip, clusterName, serviceName) {
+
-			// We need to process for pips belong to this service
+		// If we've been told to use a specific public ip by the client, let's track whether or not it actually existed
-			if wantLb && !isInternal && pipName == desiredPipName {
+		// when we inspect the set in Azure.
-				// This is the only case we should preserve the
+		discoveredDesiredPublicIP = discoveredDesiredPublicIP || wantLb && !isInternal && pipName == desiredPipName
-				// Public ip resource with match service tag
+
-				found = true
+		// Now, let's perform additional analysis to determine if we should release the public ips we have found.
-			} else {
+		// We can only let them go if (a) they are owned by this service and (b) they meet the criteria for deletion.
-				pipsToBeDeleted = append(pipsToBeDeleted, &pip)
+		if serviceOwnsPublicIP(&pip, clusterName, serviceName) &&
-			}
+			shouldReleaseExistingOwnedPublicIP(&pip, wantLb, isInternal, desiredPipName, serviceIPTagRequest) {
-		} else if wantLb && !isInternal && pipName == desiredPipName {
+
-			found = true
+			// Then, release the public ip
+			pipsToBeDeleted = append(pipsToBeDeleted, &pip)
+
+			// Flag if we deleted the desired public ip
+			deletedDesiredPublicIP = deletedDesiredPublicIP || pipName == desiredPipName
+
+			// An aside: It would be unusual, but possible, for us to delete a public ip referred to explicitly by name
+			// in Service annotations (which is usually reserved for non-service-owned externals), if that IP is tagged as
+			// having been owned by a particular Kubernetes cluster.
 		}
 	}
-	if !isInternal && shouldPIPExisted && !found && wantLb {
+
+	if !isInternal && serviceAnnotationRequestsNamedPublicIP && !discoveredDesiredPublicIP && wantLb {
 		return nil, fmt.Errorf("reconcilePublicIP for service(%s): pip(%s) not found", serviceName, desiredPipName)
 	}
+
 	var deleteFuncs []func() error
 	for _, pip := range pipsToBeDeleted {
 		pipCopy := *pip
@@ -1536,7 +1664,8 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
 		// Confirm desired public ip resource exists
 		var pip *network.PublicIPAddress
 		domainNameLabel, found := getPublicIPDomainNameLabel(service)
-		if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName, shouldPIPExisted, found); err != nil {
+		errorIfPublicIPDoesNotExist := serviceAnnotationRequestsNamedPublicIP && discoveredDesiredPublicIP && !deletedDesiredPublicIP
+		if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName, errorIfPublicIPDoesNotExist, found); err != nil {
 			return nil, err
 		}
 		return pip, nil

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go

@@ -23,8 +23,10 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
+	"sort"
 	"strconv"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -515,8 +517,8 @@ func TestServiceOwnsPublicIP(t *testing.T) {
 	}
 
 	for i, c := range tests {
-		owns := serviceOwnsPublicIP(c.pip, c.clusterName, c.serviceName)
+		actual := serviceOwnsPublicIP(c.pip, c.clusterName, c.serviceName)
-		assert.Equal(t, owns, c.expected, "TestCase[%d]: %s", i, c.desc)
+		assert.Equal(t, c.expected, actual, "TestCase[%d]: %s", i, c.desc)
 	}
 }
 
@@ -554,6 +556,478 @@ func TestGetPublicIPAddressResourceGroup(t *testing.T) {
 	}
 }
 
+func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
+	existingPipWithTag := network.PublicIPAddress{
+		ID:   to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
+		Name: to.StringPtr("testPIP"),
+		Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+		PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+			PublicIPAddressVersion:   network.IPv4,
+			PublicIPAllocationMethod: network.Static,
+			IPTags: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+			},
+		},
+	}
+
+	existingPipWithNoPublicIPAddressFormatProperties := network.PublicIPAddress{
+		ID:                              to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
+		Name:                            to.StringPtr("testPIP"),
+		Tags:                            map[string]*string{"service": to.StringPtr("default/test1")},
+		PublicIPAddressPropertiesFormat: nil,
+	}
+
+	tests := []struct {
+		desc                  string
+		existingPip           network.PublicIPAddress
+		lbShouldExist         bool
+		lbIsInternal          bool
+		desiredPipName        string
+		ipTagRequest          serviceIPTagRequest
+		expectedShouldRelease bool
+	}{
+		{
+			desc:           "Everything matches, no release",
+			existingPip:    existingPipWithTag,
+			lbShouldExist:  true,
+			lbIsInternal:   false,
+			desiredPipName: *existingPipWithTag.Name,
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags:                      existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags,
+			},
+			expectedShouldRelease: false,
+		},
+		{
+			desc:           "nil tags (none-specified by annotation, some are present on object), no release",
+			existingPip:    existingPipWithTag,
+			lbShouldExist:  true,
+			lbIsInternal:   false,
+			desiredPipName: *existingPipWithTag.Name,
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: false,
+				IPTags:                      nil,
+			},
+			expectedShouldRelease: false,
+		},
+		{
+			desc:           "existing public ip with no format properties (unit test only?), tags required by annotation, no release",
+			existingPip:    existingPipWithNoPublicIPAddressFormatProperties,
+			lbShouldExist:  true,
+			lbIsInternal:   false,
+			desiredPipName: *existingPipWithTag.Name,
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags:                      existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags,
+			},
+			expectedShouldRelease: true,
+		},
+		{
+			desc:           "LB no longer desired, expect release",
+			existingPip:    existingPipWithTag,
+			lbShouldExist:  false,
+			lbIsInternal:   false,
+			desiredPipName: *existingPipWithTag.Name,
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags:                      existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags,
+			},
+			expectedShouldRelease: true,
+		},
+		{
+			desc:           "LB now internal, expect release",
+			existingPip:    existingPipWithTag,
+			lbShouldExist:  true,
+			lbIsInternal:   true,
+			desiredPipName: *existingPipWithTag.Name,
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags:                      existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags,
+			},
+			expectedShouldRelease: true,
+		},
+		{
+			desc:           "Alternate desired name, expect release",
+			existingPip:    existingPipWithTag,
+			lbShouldExist:  true,
+			lbIsInternal:   false,
+			desiredPipName: "otherName",
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags:                      existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags,
+			},
+			expectedShouldRelease: true,
+		},
+		{
+			desc:           "mismatching, expect release",
+			existingPip:    existingPipWithTag,
+			lbShouldExist:  true,
+			lbIsInternal:   false,
+			desiredPipName: *existingPipWithTag.Name,
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags: &[]network.IPTag{
+					{
+						IPTagType: to.StringPtr("tag2"),
+						Tag:       to.StringPtr("tag2value"),
+					},
+				},
+			},
+			expectedShouldRelease: true,
+		},
+	}
+
+	for i, c := range tests {
+
+		actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&c.existingPip, c.lbShouldExist, c.lbIsInternal, c.desiredPipName, c.ipTagRequest)
+		assert.Equal(t, c.expectedShouldRelease, actualShouldRelease, "TestCase[%d]: %s", i, c.desc)
+	}
+}
+
+func TestgetIPTagMap(t *testing.T) {
+	tests := []struct {
+		desc     string
+		input    string
+		expected map[string]string
+	}{
+		{
+			desc:     "empty map should be returned when service has blank annotations",
+			input:    "",
+			expected: map[string]string{},
+		},
+		{
+			desc:  "a single tag should be returned when service has set one tag pair in the annotation",
+			input: "tag1=tagvalue1",
+			expected: map[string]string{
+				"tag1": "tagvalue1",
+			},
+		},
+		{
+			desc:  "a single tag should be returned when service has set one tag pair in the annotation (and spaces are trimmed)",
+			input: " tag1 = tagvalue1 ",
+			expected: map[string]string{
+				"tag1": "tagvalue1",
+			},
+		},
+		{
+			desc:  "a single tag should be returned when service has set two tag pairs in the annotation with the same key (last write wins - according to appearance order in the string)",
+			input: "tag1=tagvalue1,tag1=tagvalue1new",
+			expected: map[string]string{
+				"tag1": "tagvalue1new",
+			},
+		},
+		{
+			desc:  "two tags should be returned when service has set two tag pairs in the annotation",
+			input: "tag1=tagvalue1,tag2=tagvalue2",
+			expected: map[string]string{
+				"tag1": "tagvalue1",
+				"tag2": "tagvalue2",
+			},
+		},
+		{
+			desc:  "two tags should be returned when service has set two tag pairs (and one malformation) in the annotation",
+			input: "tag1=tagvalue1,tag2=tagvalue2,tag3malformed",
+			expected: map[string]string{
+				"tag1": "tagvalue1",
+				"tag2": "tagvalue2",
+			},
+		},
+		{
+			// We may later decide not to support blank values.  The Azure contract is not entirely clear here.
+			desc:  "two tags should be returned when service has set two tag pairs (and one has a blank value) in the annotation",
+			input: "tag1=tagvalue1,tag2=",
+			expected: map[string]string{
+				"tag1": "tagvalue1",
+				"tag2": "",
+			},
+		},
+		{
+			// We may later decide not to support blank keys.  The Azure contract is not entirely clear here.
+			desc:  "two tags should be returned when service has set two tag pairs (and one has a blank key) in the annotation",
+			input: "tag1=tagvalue1,=tag2value",
+			expected: map[string]string{
+				"tag1": "tagvalue1",
+				"":     "tag2value",
+			},
+		},
+	}
+
+	for i, c := range tests {
+		actual := getIPTagMap(c.input)
+		assert.Equal(t, c.expected, actual, "TestCase[%d]: %s", i, c.desc)
+	}
+}
+
+func TestConvertIPTagMapToSlice(t *testing.T) {
+	tests := []struct {
+		desc     string
+		input    map[string]string
+		expected *[]network.IPTag
+	}{
+		{
+			desc:     "nil slice should be returned when the map is nil",
+			input:    nil,
+			expected: nil,
+		},
+		{
+			desc:     "empty slice should be returned when the map is empty",
+			input:    map[string]string{},
+			expected: &[]network.IPTag{},
+		},
+		{
+			desc: "one tag should be returned when the map has one tag",
+			input: map[string]string{
+				"tag1": "tag1value",
+			},
+			expected: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+			},
+		},
+		{
+			desc: "two tags should be returned when the map has two tags",
+			input: map[string]string{
+				"tag1": "tag1value",
+				"tag2": "tag2value",
+			},
+			expected: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+				{
+					IPTagType: to.StringPtr("tag2"),
+					Tag:       to.StringPtr("tag2value"),
+				},
+			},
+		},
+	}
+
+	for i, c := range tests {
+		actual := convertIPTagMapToSlice(c.input)
+
+		// Sort output to provide stability of return from map for test comparison
+		// The order doesn't matter at runtime.
+		if actual != nil {
+			sort.Slice(*actual, func(i, j int) bool {
+				ipTagSlice := *actual
+				return to.String(ipTagSlice[i].IPTagType) < to.String(ipTagSlice[j].IPTagType)
+			})
+		}
+		if c.expected != nil {
+			sort.Slice(*c.expected, func(i, j int) bool {
+				ipTagSlice := *c.expected
+				return to.String(ipTagSlice[i].IPTagType) < to.String(ipTagSlice[j].IPTagType)
+			})
+
+		}
+
+		assert.Equal(t, c.expected, actual, "TestCase[%d]: %s", i, c.desc)
+	}
+}
+
+func TestGetserviceIPTagRequestForPublicIP(t *testing.T) {
+	tests := []struct {
+		desc     string
+		input    *v1.Service
+		expected serviceIPTagRequest
+	}{
+		{
+			desc:  "Annotation should be false when service is absent",
+			input: nil,
+			expected: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: false,
+				IPTags:                      nil,
+			},
+		},
+		{
+			desc: "Annotation should be false when service is present, without annotation",
+			input: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{},
+				},
+			},
+			expected: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: false,
+				IPTags:                      nil,
+			},
+		},
+		{
+			desc: "Annotation should be true, tags slice empty, when annotation blank",
+			input: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						ServiceAnnotationIPTagsForPublicIP: "",
+					},
+				},
+			},
+			expected: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags:                      &[]network.IPTag{},
+			},
+		},
+		{
+			desc: "two tags should be returned when service has set two tag pairs (and one malformation) in the annotation",
+			input: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						ServiceAnnotationIPTagsForPublicIP: "tag1=tag1value,tag2=tag2value,tag3malformed",
+					},
+				},
+			},
+			expected: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags: &[]network.IPTag{
+					{
+						IPTagType: to.StringPtr("tag1"),
+						Tag:       to.StringPtr("tag1value"),
+					},
+					{
+						IPTagType: to.StringPtr("tag2"),
+						Tag:       to.StringPtr("tag2value"),
+					},
+				},
+			},
+		},
+	}
+	for i, c := range tests {
+		actual := getServiceIPTagRequestForPublicIP(c.input)
+
+		// Sort output to provide stability of return from map for test comparison
+		// The order doesn't matter at runtime.
+		if actual.IPTags != nil {
+			sort.Slice(*actual.IPTags, func(i, j int) bool {
+				ipTagSlice := *actual.IPTags
+				return to.String(ipTagSlice[i].IPTagType) < to.String(ipTagSlice[j].IPTagType)
+			})
+		}
+		if c.expected.IPTags != nil {
+			sort.Slice(*c.expected.IPTags, func(i, j int) bool {
+				ipTagSlice := *c.expected.IPTags
+				return to.String(ipTagSlice[i].IPTagType) < to.String(ipTagSlice[j].IPTagType)
+			})
+
+		}
+
+		assert.Equal(t, actual, c.expected, "TestCase[%d]: %s", i, c.desc)
+	}
+}
+
+func TestAreIpTagsEquivalent(t *testing.T) {
+	tests := []struct {
+		desc     string
+		input1   *[]network.IPTag
+		input2   *[]network.IPTag
+		expected bool
+	}{
+		{
+			desc:     "nils should be considered equal",
+			input1:   nil,
+			input2:   nil,
+			expected: true,
+		},
+		{
+			desc:     "nils should be considered to empty arrays (case 1)",
+			input1:   nil,
+			input2:   &[]network.IPTag{},
+			expected: true,
+		},
+		{
+			desc:     "nils should be considered to empty arrays (case 1)",
+			input1:   &[]network.IPTag{},
+			input2:   nil,
+			expected: true,
+		},
+		{
+			desc: "nil should not be considered equal to anything (case 1)",
+			input1: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+				{
+					IPTagType: to.StringPtr("tag2"),
+					Tag:       to.StringPtr("tag2value"),
+				},
+			},
+			input2:   nil,
+			expected: false,
+		},
+		{
+			desc: "nil should not be considered equal to anything (case 2)",
+			input2: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+				{
+					IPTagType: to.StringPtr("tag2"),
+					Tag:       to.StringPtr("tag2value"),
+				},
+			},
+			input1:   nil,
+			expected: false,
+		},
+		{
+			desc: "exactly equal should be treated as equal",
+			input1: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+				{
+					IPTagType: to.StringPtr("tag2"),
+					Tag:       to.StringPtr("tag2value"),
+				},
+			},
+			input2: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+				{
+					IPTagType: to.StringPtr("tag2"),
+					Tag:       to.StringPtr("tag2value"),
+				},
+			},
+			expected: true,
+		},
+		{
+			desc: "equal but out of order should be treated as equal",
+			input1: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+				{
+					IPTagType: to.StringPtr("tag2"),
+					Tag:       to.StringPtr("tag2value"),
+				},
+			},
+			input2: &[]network.IPTag{
+				{
+					IPTagType: to.StringPtr("tag2"),
+					Tag:       to.StringPtr("tag2value"),
+				},
+				{
+					IPTagType: to.StringPtr("tag1"),
+					Tag:       to.StringPtr("tag1value"),
+				},
+			},
+			expected: true,
+		},
+	}
+	for i, c := range tests {
+		actual := areIPTagsEquivalent(c.input1, c.input2)
+		assert.Equal(t, actual, c.expected, "TestCase[%d]: %s", i, c.desc)
+	}
+}
+
 func TestGetServiceTags(t *testing.T) {
 	tests := []struct {
 		desc     string
@@ -2035,17 +2509,19 @@ func TestReconcilePublicIP(t *testing.T) {
 	defer ctrl.Finish()
 
 	testCases := []struct {
-		desc          string
+		desc                        string
-		wantLb        bool
+		wantLb                      bool
-		annotations   map[string]string
+		annotations                 map[string]string
-		existingPIPs  []network.PublicIPAddress
+		existingPIPs                []network.PublicIPAddress
-		expectedID    string
+		expectedID                  string
-		expectedPIP   *network.PublicIPAddress
+		expectedPIP                 *network.PublicIPAddress
-		expectedError bool
+		expectedError               bool
+		expectedCreateOrUpdateCount int
 	}{
 		{
-			desc:   "reconcilePublicIP shall return nil if there's no pip in service",
+			desc:                        "reconcilePublicIP shall return nil if there's no pip in service",
-			wantLb: false,
+			wantLb:                      false,
+			expectedCreateOrUpdateCount: 0,
 		},
 		{
 			desc:   "reconcilePublicIP shall return nil if no pip is owned by service",
@@ -2055,6 +2531,7 @@ func TestReconcilePublicIP(t *testing.T) {
 					Name: to.StringPtr("pip1"),
 				},
 			},
+			expectedCreateOrUpdateCount: 0,
 		},
 		{
 			desc:   "reconcilePublicIP shall delete unwanted pips and create a new one",
@@ -2067,6 +2544,7 @@ func TestReconcilePublicIP(t *testing.T) {
 			},
 			expectedID: "/subscriptions/subscription/resourceGroups/rg/providers/" +
 				"Microsoft.Network/publicIPAddresses/testCluster-atest1",
+			expectedCreateOrUpdateCount: 1,
 		},
 		{
 			desc:        "reconcilePublicIP shall report error if the given PIP name doesn't exist in the resource group",
@@ -2082,7 +2560,8 @@ func TestReconcilePublicIP(t *testing.T) {
 					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
 				},
 			},
-			expectedError: true,
+			expectedError:               true,
+			expectedCreateOrUpdateCount: 0,
 		},
 		{
 			desc:        "reconcilePublicIP shall delete unwanted PIP when given the name of desired PIP",
@@ -2107,6 +2586,85 @@ func TestReconcilePublicIP(t *testing.T) {
 				Name: to.StringPtr("testPIP"),
 				Tags: map[string]*string{"service": to.StringPtr("default/test1")},
 			},
+			expectedCreateOrUpdateCount: 0,
+		},
+		{
+			desc:   "reconcilePublicIP shall delete unwanted pips and existing pips, when the existing pips IP tags do not match",
+			wantLb: true,
+			annotations: map[string]string{
+				ServiceAnnotationPIPName:           "testPIP",
+				ServiceAnnotationIPTagsForPublicIP: "tag1=tag1value",
+			},
+			existingPIPs: []network.PublicIPAddress{
+				{
+					Name: to.StringPtr("pip1"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+				{
+					Name: to.StringPtr("pip2"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+				{
+					Name: to.StringPtr("testPIP"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+			},
+			expectedPIP: &network.PublicIPAddress{
+				ID:   to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
+				Name: to.StringPtr("testPIP"),
+				Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+					PublicIPAddressVersion:   network.IPv4,
+					PublicIPAllocationMethod: network.Static,
+					IPTags: &[]network.IPTag{
+						{
+							IPTagType: to.StringPtr("tag1"),
+							Tag:       to.StringPtr("tag1value"),
+						},
+					},
+				},
+			},
+			expectedCreateOrUpdateCount: 1,
+		},
+		{
+			desc:   "reconcilePublicIP shall preserve existing pips, when the existing pips IP tags do match",
+			wantLb: true,
+			annotations: map[string]string{
+				ServiceAnnotationPIPName:           "testPIP",
+				ServiceAnnotationIPTagsForPublicIP: "tag1=tag1value",
+			},
+			existingPIPs: []network.PublicIPAddress{
+				{
+					Name: to.StringPtr("testPIP"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+					PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+						PublicIPAddressVersion:   network.IPv4,
+						PublicIPAllocationMethod: network.Static,
+						IPTags: &[]network.IPTag{
+							{
+								IPTagType: to.StringPtr("tag1"),
+								Tag:       to.StringPtr("tag1value"),
+							},
+						},
+					},
+				},
+			},
+			expectedPIP: &network.PublicIPAddress{
+				ID:   to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
+				Name: to.StringPtr("testPIP"),
+				Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+					PublicIPAddressVersion:   network.IPv4,
+					PublicIPAllocationMethod: network.Static,
+					IPTags: &[]network.IPTag{
+						{
+							IPTagType: to.StringPtr("tag1"),
+							Tag:       to.StringPtr("tag1value"),
+						},
+					},
+				},
+			},
+			expectedCreateOrUpdateCount: 0,
 		},
 		{
 			desc:        "reconcilePublicIP shall find the PIP by given name and shall not delete the PIP which is not owned by service",
@@ -2128,13 +2686,27 @@ func TestReconcilePublicIP(t *testing.T) {
 				ID:   to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
 				Name: to.StringPtr("testPIP"),
 			},
+			expectedCreateOrUpdateCount: 0,
 		},
 	}
 
 	for i, test := range testCases {
+		deletedPips := make(map[string]bool)
+		savedPips := make(map[string]network.PublicIPAddress)
+		createOrUpdateCount := 0
+		var m sync.Mutex
 		az := GetTestCloud(ctrl)
 		mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
-		mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+		creator := mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).AnyTimes()
+		creator.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, parameters network.PublicIPAddress) *retry.Error {
+			m.Lock()
+			deletedPips[publicIPAddressName] = false
+			savedPips[publicIPAddressName] = parameters
+			createOrUpdateCount++
+			m.Unlock()
+			return nil
+		})
+
 		mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).AnyTimes()
 		if i == 2 {
 			mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", "testCluster-atest1", gomock.Any()).Return(network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound}).Times(1)
@@ -2143,19 +2715,58 @@ func TestReconcilePublicIP(t *testing.T) {
 		service := getTestService("test1", v1.ProtocolTCP, nil, false, 80)
 		service.Annotations = test.annotations
 		for _, pip := range test.existingPIPs {
-			mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *pip.Name, gomock.Any()).Return(pip, nil).AnyTimes()
+			savedPips[*pip.Name] = pip
-			mockPIPsClient.EXPECT().Delete(gomock.Any(), "rg", *pip.Name).Return(nil).AnyTimes()
+			getter := mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *pip.Name, gomock.Any()).AnyTimes()
+			getter.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, expand string) (result network.PublicIPAddress, rerr *retry.Error) {
+				m.Lock()
+				deletedValue, deletedContains := deletedPips[publicIPAddressName]
+				savedPipValue, savedPipContains := savedPips[publicIPAddressName]
+				m.Unlock()
+
+				if (!deletedContains || !deletedValue) && savedPipContains {
+					return savedPipValue, nil
+				}
+
+				return network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound}
+
+			})
+			deleter := mockPIPsClient.EXPECT().Delete(gomock.Any(), "rg", *pip.Name).Return(nil).AnyTimes()
+			deleter.Do(func(ctx context.Context, resourceGroupName string, publicIPAddressName string) *retry.Error {
+				m.Lock()
+				deletedPips[publicIPAddressName] = true
+				m.Unlock()
+				return nil
+			})
+
 			err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", to.String(pip.Name), pip)
 			if err != nil {
 				t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
 			}
+
+			// Clear create or update count to prepare for main execution
+			createOrUpdateCount = 0
 		}
 		pip, err := az.reconcilePublicIP("testCluster", &service, "", test.wantLb)
+		if !test.expectedError {
+			assert.Equal(t, nil, err, "TestCase[%d]: %s", i, test.desc)
+		}
 		if test.expectedID != "" {
 			assert.Equal(t, test.expectedID, to.String(pip.ID), "TestCase[%d]: %s", i, test.desc)
 		} else if test.expectedPIP != nil && test.expectedPIP.Name != nil {
 			assert.Equal(t, *test.expectedPIP.Name, *pip.Name, "TestCase[%d]: %s", i, test.desc)
+
+			if test.expectedPIP.PublicIPAddressPropertiesFormat != nil {
+				sortIPTags(test.expectedPIP.PublicIPAddressPropertiesFormat.IPTags)
+			}
+
+			if pip.PublicIPAddressPropertiesFormat != nil {
+				sortIPTags(pip.PublicIPAddressPropertiesFormat.IPTags)
+			}
+
+			assert.Equal(t, test.expectedPIP.PublicIPAddressPropertiesFormat, pip.PublicIPAddressPropertiesFormat, "TestCase[%d]: %s", i, test.desc)
+
 		}
+		assert.Equal(t, test.expectedCreateOrUpdateCount, createOrUpdateCount, "TestCase[%d]: %s", i, test.desc)
 		assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
 	}
 }
@@ -2169,6 +2780,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 		existingPIPs            []network.PublicIPAddress
 		inputDNSLabel           string
 		foundDNSLabelAnnotation bool
+		additionalAnnotations   map[string]string
 		expectedPIP             *network.PublicIPAddress
 		expectedID              string
 		isIPv6                  bool
@@ -2287,6 +2899,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 	for i, test := range testCases {
 		az := GetTestCloud(ctrl)
 		service := getTestService("test1", v1.ProtocolTCP, nil, test.isIPv6, 80)
+		service.ObjectMeta.Annotations = test.additionalAnnotations
 		mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
 		mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 		mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", "pip1", gomock.Any()).DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, expand string) (network.PublicIPAddress, *retry.Error) {