Fix PIP tags possibly not ensured issue in ensurePublicIPExists()

Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>
kubernetes-sigs · Jul 18, 2022 · c434fa7 · c434fa7
1 parent 0ad45e4
commit c434fa7
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 14 deletions.
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
@@ -1002,6 +1002,9 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 		}
 		klog.V(2).Infof("ensurePublicIPExists for service(%s): pip(%s) - creating", serviceName, *pip.Name)
 	}
+	if az.ensurePIPTagged(service, &pip) {
+		changed = true
+	}
 
 	if foundDNSLabelAnnotation {
 		updatedDNSSettings, err := reconcileDNSSettings(&pip, domainNameLabel, serviceName, pipName)

diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
@@ -3794,7 +3794,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 		expectedError           bool
 	}{
 		{
-			desc:         "ensurePublicIPExists shall return existed PIP if there is any",
+			desc:         "shall return existed PIP if there is any",
 			existingPIPs: []network.PublicIPAddress{{Name: to.StringPtr("pip1")}},
 			expectedPIP: &network.PublicIPAddress{
 				Name: to.StringPtr("pip1"),
@@ -3804,13 +3804,13 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			shouldPutPIP: true,
 		},
 		{
-			desc: "ensurePublicIPExists shall create a new pip if there is no existed pip",
+			desc: "shall create a new pip if there is no existed pip",
 			expectedID: "/subscriptions/subscription/resourceGroups/rg/providers/" +
 				"Microsoft.Network/publicIPAddresses/pip1",
 			shouldPutPIP: true,
 		},
 		{
-			desc:                    "ensurePublicIPExists shall update existed PIP's dns label",
+			desc:                    "shall update existed PIP's dns label",
 			inputDNSLabel:           "newdns",
 			foundDNSLabelAnnotation: true,
 			existingPIPs: []network.PublicIPAddress{{
@@ -3835,7 +3835,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			shouldPutPIP: true,
 		},
 		{
-			desc:                    "ensurePublicIPExists shall delete DNS from PIP if DNS label is set empty",
+			desc:                    "shall delete DNS from PIP if DNS label is set empty",
 			foundDNSLabelAnnotation: true,
 			existingPIPs: []network.PublicIPAddress{{
 				Name: to.StringPtr("pip1"),
@@ -3857,7 +3857,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			shouldPutPIP: true,
 		},
 		{
-			desc:                    "ensurePublicIPExists shall not delete DNS from PIP if DNS label annotation is not set",
+			desc:                    "shall not delete DNS from PIP if DNS label annotation is not set",
 			foundDNSLabelAnnotation: false,
 			existingPIPs: []network.PublicIPAddress{{
 				Name: to.StringPtr("pip1"),
@@ -3881,7 +3881,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			shouldPutPIP: true,
 		},
 		{
-			desc:                    "ensurePublicIPExists shall update existed PIP's dns label for IPv6",
+			desc:                    "shall update existed PIP's dns label for IPv6",
 			inputDNSLabel:           "newdns",
 			foundDNSLabelAnnotation: true,
 			isIPv6:                  true,
@@ -3908,7 +3908,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			shouldPutPIP: true,
 		},
 		{
-			desc:                    "ensurePublicIPExists shall report an conflict error if the DNS label is conflicted",
+			desc:                    "shall report an conflict error if the DNS label is conflicted",
 			inputDNSLabel:           "test",
 			foundDNSLabelAnnotation: true,
 			existingPIPs: []network.PublicIPAddress{{
@@ -3923,7 +3923,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			expectedError: true,
 		},
 		{
-			desc:          "ensurePublicIPExists shall return the pip without calling PUT API if the tags are good",
+			desc:          "shall return the pip without calling PUT API if the tags are good",
 			inputDNSLabel: "test",
 			existingPIPs: []network.PublicIPAddress{
 				{
@@ -3961,7 +3961,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			},
 		},
 		{
-			desc: "ensurePublicIPExists shall tag the service name to the pip correctly",
+			desc: "shall tag the service name to the pip correctly",
 			existingPIPs: []network.PublicIPAddress{
 				{Name: to.StringPtr("pip1")},
 			},
@@ -3973,7 +3973,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			shouldPutPIP: true,
 		},
 		{
-			desc:   "ensurePublicIPExists shall not call the PUT API for IPV6 pip if it is not necessary",
+			desc:   "shall not call the PUT API for IPV6 pip if it is not necessary",
 			isIPv6: true,
 			useSLB: true,
 			existingPIPs: []network.PublicIPAddress{
@@ -3996,9 +3996,22 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			},
 			shouldPutPIP: true,
 		},
+		{
+			desc:         "shall update pip tags if there is any change",
+			existingPIPs: []network.PublicIPAddress{{Name: to.StringPtr("pip1"), Tags: map[string]*string{"a": to.StringPtr("b")}}},
+			expectedPIP: &network.PublicIPAddress{
+				Name: to.StringPtr("pip1"), Tags: map[string]*string{"a": to.StringPtr("c")},
+				ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg" +
+					"/providers/Microsoft.Network/publicIPAddresses/pip1"),
+			},
+			additionalAnnotations: map[string]string{
+				consts.ServiceAnnotationAzurePIPTags: "a=c",
+			},
+			shouldPutPIP: true,
+		},
 	}
 
-	for i, test := range testCases {
+	for _, test := range testCases {
 		t.Run(test.desc, func(t *testing.T) {
 			az := GetTestCloud(ctrl)
 			if test.useSLB {
@@ -4047,11 +4060,11 @@ func TestEnsurePublicIPExists(t *testing.T) {
 			}).AnyTimes()
 
 			pip, err := az.ensurePublicIPExists(&service, "pip1", test.inputDNSLabel, "", false, test.foundDNSLabelAnnotation)
-			assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s, encountered unexpected error: %v", i, test.desc, err)
+			assert.Equal(t, test.expectedError, err != nil, "unexpectedly encountered (or not) error: %v", err)
 			if test.expectedID != "" {
-				assert.Equal(t, test.expectedID, to.String(pip.ID), "TestCase[%d]: %s", i, test.desc)
+				assert.Equal(t, test.expectedID, to.String(pip.ID))
 			} else {
-				assert.Equal(t, test.expectedPIP, pip, "TestCase[%d]: %s", i, test.desc)
+				assert.Equal(t, test.expectedPIP, pip)
 			}
 		})
 	}