fix(docker): resolve the issue of double encoding while making the next call to ECR's tags list (#6357) (#6358)

* test(docker): add test to demonstrate the issue of double encoding of query parameter when the next link from a docker registry like ECR responds with an encoded url.

* 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`.

(cherry picked from commit 06c98c0)

Co-authored-by: Kiran Godishala <[email protected]>

Author: mergify[bot]

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

@@ -17,8 +17,10 @@
 package com.netflix.spinnaker.clouddriver.docker.registry.api.v2.client;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+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.mockito.ArgumentMatchers.anyString;
@@ -29,41 +31,21 @@
 import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
 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.config.DefaultServiceClientProvider;
-import com.netflix.spinnaker.config.okhttp3.DefaultOkHttpClientBuilderProvider;
-import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
-import com.netflix.spinnaker.kork.client.ServiceClientProvider;
 import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
-import com.netflix.spinnaker.kork.retrofit.Retrofit2ServiceFactory;
-import com.netflix.spinnaker.kork.retrofit.Retrofit2ServiceFactoryAutoConfiguration;
-import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties;
 import java.util.Arrays;
 import java.util.Map;
 import okhttp3.OkHttpClient;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.mockito.Mockito;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.boot.test.mock.mockito.MockBean;
 import org.springframework.http.HttpStatus;
 import retrofit2.Retrofit;
 import retrofit2.converter.jackson.JacksonConverterFactory;
 
-@SpringBootTest(
-    classes = {
-      OkHttpClientConfigurationProperties.class,
-      Retrofit2ServiceFactory.class,
-      ServiceClientProvider.class,
-      OkHttpClientProvider.class,
-      OkHttpClient.class,
-      DefaultServiceClientProvider.class,
-      DefaultOkHttpClientBuilderProvider.class,
-      Retrofit2ServiceFactoryAutoConfiguration.class,
-      ObjectMapper.class
-    },
-    webEnvironment = SpringBootTest.WebEnvironment.NONE)
+@SpringBootTest(classes = {DockerBearerTokenService.class})
 public class DockerRegistryClientTest {
 
   @RegisterExtension
@@ -73,7 +55,6 @@ public class DockerRegistryClientTest {
   static DockerRegistryClient.DockerRegistryService dockerRegistryService;
   @MockBean DockerBearerTokenService dockerBearerTokenService;
   static DockerRegistryClient dockerRegistryClient;
-  @Autowired ServiceClientProvider serviceClientProvider;
   ObjectMapper objectMapper = new ObjectMapper();
   Map<String, Object> tagsResponse;
   String tagsResponseString;
@@ -134,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)));
 
@@ -146,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())
@@ -165,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)));
 
@@ -202,4 +183,38 @@ public void getCatalogWithNextLink() {
     DockerRegistryCatalog dockerRegistryCatalog = dockerRegistryClient.getCatalog();
     assertEquals(15, dockerRegistryCatalog.getRepositories().size());
   }
+
+  @Test
+  public void getTagsWithNextLinkEncryptedAndEncoded() {
+    String tagsListEndPointMinusQueryParams = "/v2/library/nginx/tags/list";
+    String expectedEncodedParam = "Md1Woj%2FNOhjepFq7kPAr%2FEw%2FYxfcJoH9N52%2Blo3qAQ%3D%3D";
+
+    wmDockerRegistry.stubFor(
+        WireMock.get(urlMatching(tagsListEndPointMinusQueryParams + "\\?n=5"))
+            .willReturn(
+                aResponse()
+                    .withStatus(HttpStatus.OK.value())
+                    .withHeader(
+                        "link",
+                        "</v2/library/nginx/tags/list?last=Md1Woj%2FNOhjepFq7kPAr%2FEw%2FYxfcJoH9N52%2Blo3qAQ%3D%3D&n=5>; rel=\"next\"")
+                    .withBody(tagsResponseString)));
+
+    wmDockerRegistry.stubFor(
+        WireMock.get(
+                urlMatching(
+                    tagsListEndPointMinusQueryParams + "\\?last=" + expectedEncodedParam + "&n=5"))
+            .willReturn(
+                aResponse().withStatus(HttpStatus.OK.value()).withBody(tagsSecondResponseString)));
+
+    DockerRegistryTags dockerRegistryTags = dockerRegistryClient.getTags("library/nginx");
+    assertThat(dockerRegistryTags.getTags()).hasSize(10);
+
+    wmDockerRegistry.verify(
+        1, getRequestedFor(urlMatching(tagsListEndPointMinusQueryParams + "\\?n=5")));
+    wmDockerRegistry.verify(
+        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(