Merge pull request #1955 from lzhecheng/cp-r-1.23-1400

[release-1.23] Improve the error handling for ARM APIs
kubernetes-sigs · Jun 30, 2022 · e421603 · e421603
2 parents 2aa7b8b + 9b05bf6
commit e421603
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 45 deletions.
diff --git a/pkg/azureclients/armclient/azure_armclient.go b/pkg/azureclients/armclient/azure_armclient.go
@@ -126,7 +126,7 @@ func NormalizeAzureRegion(name string) string {
 	return strings.ToLower(region)
 }
 
-// DoExponentialBackoffRetry returns an autorest.SendDecorator which performs retry with customizable backoff policy.
+// DoHackRegionalRetryDecorator returns an autorest.SendDecorator which performs retry with customizable backoff policy.
 func DoHackRegionalRetryDecorator(c *Client) autorest.SendDecorator {
 	return func(s autorest.Sender) autorest.Sender {
 		return autorest.SenderFunc(func(request *http.Request) (*http.Response, error) {
@@ -135,28 +135,35 @@ func DoHackRegionalRetryDecorator(c *Client) autorest.SendDecorator {
 				klog.V(2).Infof("response is empty")
 				return response, rerr
 			}
-			if rerr == nil || response.StatusCode == http.StatusNotFound || c.regionalEndpoint == "" {
-				return response, rerr
-			}
-			// Hack: retry the regional ARM endpoint in case of ARM traffic split and arm resource group replication is too slow
+
 			bodyBytes, _ := ioutil.ReadAll(response.Body)
 			defer func() {
 				response.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes))
 			}()
 
 			bodyString := string(bodyBytes)
-			var body map[string]interface{}
-			if e := json.Unmarshal(bodyBytes, &body); e != nil {
-				klog.Errorf("Send.sendRequest: error in parsing response body string: %s, Skip retrying regional host", e.Error())
-				return response, rerr
-			}
-			klog.V(5).Infof("Send.sendRequest original response: %s", bodyString)
+			trimmed := strings.TrimSpace(bodyString)
+			// Hack: retry the regional ARM endpoint in case of ARM traffic split and arm resource group replication is too slow
+			// Empty content and 2xx http status code are returned in this case.
+			// Issue: https://github.com/kubernetes-sigs/cloud-provider-azure/issues/1296
+			emptyResp := (response.ContentLength == 0 || trimmed == "" || trimmed == "{}") && response.StatusCode >= 200 && response.StatusCode < 300
+			if !emptyResp {
+				if rerr == nil || response.StatusCode == http.StatusNotFound || c.regionalEndpoint == "" {
+					return response, rerr
+				}
 
-			if err, ok := body["error"].(map[string]interface{}); !ok ||
-				err["code"] == nil ||
-				!strings.EqualFold(err["code"].(string), "ResourceGroupNotFound") {
-				klog.V(5).Infof("Send.sendRequest: response body does not contain ResourceGroupNotFound error code. Skip retrying regional host")
-				return response, rerr
+				var body map[string]interface{}
+				if e := json.Unmarshal(bodyBytes, &body); e != nil {
+					klog.Errorf("Send.sendRequest: error in parsing response body string: %s, Skip retrying regional host", e.Error())
+					return response, rerr
+				}
+				klog.V(5).Infof("Send.sendRequest original response: %s", bodyString)
+
+				err, ok := body["error"].(map[string]interface{})
+				if !ok || err["code"] == nil || !strings.EqualFold(err["code"].(string), "ResourceGroupNotFound") {
+					klog.V(5).Infof("Send.sendRequest: response body does not contain ResourceGroupNotFound error code. Skip retrying regional host")
+					return response, rerr
+				}
 			}
 
 			currentHost := request.URL.Host

diff --git a/pkg/azureclients/armclient/azure_armclient_test.go b/pkg/azureclients/armclient/azure_armclient_test.go
@@ -75,39 +75,61 @@ func TestSend(t *testing.T) {
 	assert.Equal(t, http.StatusOK, response.StatusCode)
 }
 func TestSendFailureRegionalRetry(t *testing.T) {
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		assert.Equal(t, "GET", r.Method)
-		w.WriteHeader(http.StatusOK)
-		_, err := w.Write([]byte("{}"))
-		assert.NoError(t, err)
-	}))
-	globalServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		http.Error(w, "{\"error\":{\"code\":\"ResourceGroupNotFound\"}}", http.StatusInternalServerError)
-	}))
-
-	azConfig := azureclients.ClientConfig{Backoff: &retry.Backoff{Steps: 3}, UserAgent: "test", Location: "eastus"}
-	armClient := New(nil, azConfig, server.URL, "2019-01-01")
-	targetURL, _ := url.Parse(server.URL)
-	armClient.regionalEndpoint = targetURL.Host
-	pathParameters := map[string]interface{}{
-		"resourceGroupName": autorest.Encode("path", "testgroup"),
-		"subscriptionId":    autorest.Encode("path", "testid"),
-		"resourceName":      autorest.Encode("path", "testname"),
+	testcases := []struct {
+		description        string
+		globalServerErrMsg string
+		globalServerCode   int
+	}{
+		{
+			"RegionalRetry",
+			"{\"error\":{\"code\":\"ResourceGroupNotFound\"}}",
+			http.StatusInternalServerError,
+		},
+		{
+			"ReplicationLatency",
+			"{}",
+			http.StatusOK,
+		},
 	}
 
-	decorators := []autorest.PrepareDecorator{
-		autorest.WithPathParameters(
-			"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/vNets/{resourceName}", pathParameters),
-		autorest.WithBaseURL(globalServer.URL),
-	}
+	for _, tc := range testcases {
+		t.Run(tc.description, func(t *testing.T) {
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				assert.Equal(t, "GET", r.Method)
+				w.WriteHeader(http.StatusOK)
+				_, err := w.Write([]byte("{}"))
+				assert.NoError(t, err)
+			}))
 
-	ctx := context.Background()
-	request, err := armClient.PrepareGetRequest(ctx, decorators...)
-	assert.NoError(t, err)
+			globalServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				http.Error(w, tc.globalServerErrMsg, tc.globalServerCode)
+			}))
+			azConfig := azureclients.ClientConfig{Backoff: &retry.Backoff{Steps: 3}, UserAgent: "test", Location: "eastus"}
+			armClient := New(nil, azConfig, server.URL, "2019-01-01")
+			targetURL, _ := url.Parse(server.URL)
+			armClient.regionalEndpoint = targetURL.Host
+			pathParameters := map[string]interface{}{
+				"resourceGroupName": autorest.Encode("path", "testgroup"),
+				"subscriptionId":    autorest.Encode("path", "testid"),
+				"resourceName":      autorest.Encode("path", "testname"),
+			}
+
+			decorators := []autorest.PrepareDecorator{
+				autorest.WithPathParameters(
+					"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/vNets/{resourceName}", pathParameters),
+				autorest.WithBaseURL(globalServer.URL),
+			}
 
-	response, rerr := armClient.Send(ctx, request)
-	assert.Nil(t, rerr)
-	assert.Equal(t, http.StatusOK, response.StatusCode)
+			ctx := context.Background()
+			request, err := armClient.PrepareGetRequest(ctx, decorators...)
+			assert.NoError(t, err)
+
+			response, rerr := armClient.Send(ctx, request)
+			assert.Nil(t, rerr)
+			assert.Equal(t, http.StatusOK, response.StatusCode)
+			assert.Equal(t, targetURL.Host, response.Request.URL.Host)
+		})
+	}
 }
 
 func TestSendFailure(t *testing.T) {