fix(docker): resolve the issue of double encoding contained in the ECR's next link header causing the Retrofit2 client to fail due to error code 405 and Message: Method Not Allowed.

Author: kirangodishala

clouddriver-docker/src/main/groovy/com/netflix/spinnaker/clouddriver/docker/registry/api/v2/client/DockerRegistryClient.groovy

@@ -243,7 +243,7 @@ class DockerRegistryClient {
     @Headers([
       "Docker-Distribution-API-Version: registry/2.0"
     ])
-    Call<ResponseBody> getTags(@Path(value="repository", encoded=true) String repository, @Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap Map<String, String> queryParams)
+    Call<ResponseBody> getTags(@Path(value="repository", encoded=true) String repository, @Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap(encoded=true) Map<String, String> queryParams)
 
 
     @GET("/v2/{name}/manifests/{reference}")
@@ -263,14 +263,14 @@ class DockerRegistryClient {
     @Headers([
         "Docker-Distribution-API-Version: registry/2.0"
     ])
-    Call<ResponseBody> getCatalog(@Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap Map<String, String> queryParams)
+    Call<ResponseBody> getCatalog(@Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap(encoded=true) Map<String, String> queryParams)
 
 
     @GET("/{path}")
     @Headers([
         "Docker-Distribution-API-Version: registry/2.0"
     ])
-    Call<ResponseBody> get(@Path(value="path", encoded=true) String path, @Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap Map<String, String> queryParams)
+    Call<ResponseBody> get(@Path(value="path", encoded=true) String path, @Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap(encoded=true) Map<String, String> queryParams)
 
 
     @GET("/v2/")
@@ -394,7 +394,6 @@ class DockerRegistryClient {
     }
 
     def headerValues = caseInsensitiveHeaders["link"]
-    headers.values("link")
 
     // We are at the end of the pagination.
     if (!headerValues || headerValues.size() == 0) {
@@ -453,6 +452,7 @@ class DockerRegistryClient {
   }
 
   public DockerRegistryTags getTags(String repository, String path = null, Map<String, String> queryParams = [:]) {
+    queryParams.computeIfAbsent("n", { paginateSize.toString() })
     def response = request({
       path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent, queryParams)) :
         Retrofit2SyncCall.executeCall(registryService.getTags(repository, tokenService.basicAuthHeader, userAgent, queryParams))

clouddriver-docker/src/test/java/com/netflix/spinnaker/clouddriver/docker/registry/api/v2/client/DockerRegistryClientTest.java

@@ -20,9 +20,9 @@
 import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
 import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;
 import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
+import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertIterableEquals;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.anyString;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
@@ -32,7 +32,6 @@
 import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth.DockerBearerToken;
 import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth.DockerBearerTokenService;
 import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
-import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import java.util.Arrays;
 import java.util.Map;
 import okhttp3.OkHttpClient;
@@ -116,7 +115,7 @@ private static <T> T buildService(Class<T> type, String baseUrl) {
   @Test
   public void getTagsWithoutNextLink() {
     wmDockerRegistry.stubFor(
-        WireMock.get(urlMatching("/v2/library/nginx/tags/list"))
+        WireMock.get(urlMatching("/v2/library/nginx/tags/list\\?n=5"))
             .willReturn(
                 aResponse().withStatus(HttpStatus.OK.value()).withBody(tagsResponseString)));
 
@@ -128,7 +127,7 @@ public void getTagsWithoutNextLink() {
   @Test
   public void getTagsWithNextLink() {
     wmDockerRegistry.stubFor(
-        WireMock.get(urlMatching("/v2/library/nginx/tags/list"))
+        WireMock.get(urlMatching("/v2/library/nginx/tags/list\\?n=5"))
             .willReturn(
                 aResponse()
                     .withStatus(HttpStatus.OK.value())
@@ -147,7 +146,7 @@ public void getTagsWithNextLink() {
                         "</v2/library/nginx/tags/list1>; rel=\"next\"")
                     .withBody(tagsSecondResponseString)));
     wmDockerRegistry.stubFor(
-        WireMock.get(urlMatching("/v2/library/nginx/tags/list1"))
+        WireMock.get(urlMatching("/v2/library/nginx/tags/list1\\?n=5"))
             .willReturn(
                 aResponse().withStatus(HttpStatus.OK.value()).withBody(tagsThirdResponseString)));
 
@@ -190,12 +189,8 @@ public void getTagsWithNextLinkEncryptedAndEncoded() {
     String tagsListEndPointMinusQueryParams = "/v2/library/nginx/tags/list";
     String expectedEncodedParam = "Md1Woj%2FNOhjepFq7kPAr%2FEw%2FYxfcJoH9N52%2Blo3qAQ%3D%3D";
 
-    // notice %252F, %253D which are double encoded characters of /(or %2F) and =(or %3D)
-    String doubleEncodedParam =
-        "Md1Woj%252FNOhjepFq7kPAr%252FEw%252FYxfcJoH9N52%252Blo3qAQ%253D%253D";
-
     wmDockerRegistry.stubFor(
-        WireMock.get(urlMatching(tagsListEndPointMinusQueryParams))
+        WireMock.get(urlMatching(tagsListEndPointMinusQueryParams + "\\?n=5"))
             .willReturn(
                 aResponse()
                     .withStatus(HttpStatus.OK.value())
@@ -211,26 +206,13 @@ public void getTagsWithNextLinkEncryptedAndEncoded() {
             .willReturn(
                 aResponse().withStatus(HttpStatus.OK.value()).withBody(tagsSecondResponseString)));
 
-    wmDockerRegistry.stubFor(
-        WireMock.get(
-                urlMatching(
-                    tagsListEndPointMinusQueryParams + "\\?last=" + doubleEncodedParam + "&n=5"))
-            .willReturn(aResponse().withStatus(405).withBody("Method Not Allowed")));
-
-    // TODO: Fix this issue of retrofit2 encoding the already encoded query parameter
-    assertThrows(
-        SpinnakerHttpException.class,
-        () -> dockerRegistryClient.getTags("library/nginx"),
-        "Status: 405, Method: GET, Message: Method Not Allowed");
+    DockerRegistryTags dockerRegistryTags = dockerRegistryClient.getTags("library/nginx");
+    assertThat(dockerRegistryTags.getTags()).hasSize(10);
 
-    wmDockerRegistry.verify(1, getRequestedFor(urlMatching(tagsListEndPointMinusQueryParams)));
     wmDockerRegistry.verify(
-        1,
-        getRequestedFor(
-            urlMatching(
-                tagsListEndPointMinusQueryParams + "\\?last=" + doubleEncodedParam + "&n=5")));
+        1, getRequestedFor(urlMatching(tagsListEndPointMinusQueryParams + "\\?n=5")));
     wmDockerRegistry.verify(
-        0,
+        1,
         getRequestedFor(
             urlMatching(
                 tagsListEndPointMinusQueryParams + "\\?last=" + expectedEncodedParam + "&n=5")));

clouddriver-docker/src/test/java/com/netflix/spinnaker/clouddriver/docker/registry/security/DockerRegistryNamedAccountCredentialsTest.java

@@ -110,7 +110,7 @@ private static OkHttpClient mockDockerOkClient(
             .build();
     when(mockTagListCall.execute()).thenReturn(tagListResponse);
     when(okHttpClient.newCall(
-            argThat(r -> r.url().toString().equals("https://gcr.io/v2/myrepo/tags/list"))))
+            argThat(r -> r.url().toString().equals("https://gcr.io/v2/myrepo/tags/list?n=100"))))
         .thenReturn(mockTagListCall);
 
     doAnswer(