fix(docker): fix retrofit2 docker client which is replacing ? with %3F causing api failure

clouddriver-docker/clouddriver-docker.gradle

@@ -17,6 +17,7 @@ dependencies {
   implementation "io.spinnaker.fiat:fiat-core:$fiatVersion"
   implementation "io.spinnaker.kork:kork-credentials"
   implementation "io.spinnaker.kork:kork-retrofit"
+  implementation "io.spinnaker.kork:kork-retrofit2"
   implementation "io.spinnaker.kork:kork-exceptions"
   implementation "io.spinnaker.kork:kork-web"
 
@@ -36,6 +37,6 @@ dependencies {
   testImplementation "org.springframework.security:spring-security-test"
   testImplementation "com.github.tomakehurst:wiremock-jre8"
   testImplementation "io.spinnaker.kork:kork-core"
-  implementation "io.spinnaker.kork:kork-retrofit2"
-  implementation "io.spinnaker.kork:kork-web"
+  testImplementation "com.github.tomakehurst:wiremock-jre8-standalone"
+
 }

clouddriver-docker/src/main/groovy/com/netflix/spinnaker/clouddriver/docker/registry/api/v2/auth/DockerBearerTokenService.groovy

@@ -19,6 +19,7 @@ package com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth
 import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.DockerUserAgent
 import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.exception.DockerRegistryAuthenticationException
 import com.netflix.spinnaker.config.DefaultServiceEndpoint
+import com.netflix.spinnaker.kork.annotations.VisibleForTesting
 import com.netflix.spinnaker.kork.client.ServiceClientProvider
 import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall
 import groovy.util.logging.Slf4j
@@ -236,7 +237,8 @@ class DockerBearerTokenService {
     cachedTokens.remove(repository)
   }
 
-  private interface TokenService {
+  @VisibleForTesting
+  interface TokenService {
     @GET("/{path}")
     @Headers([
       "Docker-Distribution-API-Version: registry/2.0"

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

@@ -41,6 +41,7 @@ import retrofit2.http.Header
 import retrofit2.http.Headers
 import retrofit2.http.Path
 import retrofit2.http.Query
+import retrofit2.http.QueryMap
 
 import java.time.Instant
 
@@ -242,7 +243,8 @@ 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)
+    Call<ResponseBody> getTags(@Path(value="repository", encoded=true) String repository, @Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap Map<String, String> queryParams)
+
 
     @GET("/v2/{name}/manifests/{reference}")
     @Headers([
@@ -261,13 +263,15 @@ class DockerRegistryClient {
     @Headers([
         "Docker-Distribution-API-Version: registry/2.0"
     ])
-    Call<ResponseBody> getCatalog(@Query(value="n") int paginateSize, @Header("Authorization") String token, @Header("User-Agent") String agent)
+    Call<ResponseBody> getCatalog(@Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap 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)
+    Call<ResponseBody> get(@Path(value="path", encoded=true) String path, @Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap Map<String, String> queryParams)
+
 
     @GET("/v2/")
     @Headers([
@@ -406,7 +410,7 @@ class DockerRegistryClient {
    * This method will get all repositories available on this registry. It may fail, as some registries
    * don't want you to download their whole catalog (it's potentially a lot of data).
    */
-  public DockerRegistryCatalog getCatalog(String path = null) {
+  public DockerRegistryCatalog getCatalog(String path = null, Map<String, String> queryParams = [:]) {
     if (catalogFile) {
       log.info("Using catalog list at $catalogFile")
       try {
@@ -417,14 +421,15 @@ class DockerRegistryClient {
       }
     }
 
+    queryParams.computeIfAbsent("n", { paginateSize.toString() })
     def response
     try {
       response = request({
-        path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent)) :
-          Retrofit2SyncCall.executeCall(registryService.getCatalog(paginateSize, tokenService.basicAuthHeader, userAgent))
+        path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent, queryParams)) :
+          Retrofit2SyncCall.executeCall(registryService.getCatalog(tokenService.basicAuthHeader, userAgent, queryParams))
       }, { token ->
-        path ? Retrofit2SyncCall.executeCall(registryService.get(path, token, userAgent)) :
-          Retrofit2SyncCall.executeCall(registryService.getCatalog(paginateSize, token, userAgent))
+        path ? Retrofit2SyncCall.executeCall(registryService.get(path, token, userAgent, queryParams)) :
+          Retrofit2SyncCall.executeCall(registryService.getCatalog(token, userAgent, queryParams))
       }, "_catalog")
     } catch (Exception e) {
       log.warn("Error encountered during catalog of $path", e)
@@ -438,33 +443,60 @@ class DockerRegistryClient {
       catalog.repositories = catalog.repositories.findAll { it ==~ repositoriesRegex }
     }
     if (nextPath) {
-      def nextCatalog = getCatalog(nextPath)
+      def nextPathNew
+      (nextPathNew, queryParams) = parseForQueryParams(nextPath)
+      def nextCatalog = getCatalog(nextPathNew, queryParams)
       catalog.repositories.addAll(nextCatalog.repositories)
     }
 
     return catalog
   }
 
-  public DockerRegistryTags getTags(String repository, String path = null) {
+  public DockerRegistryTags getTags(String repository, String path = null, Map<String, String> queryParams = [:]) {
     def response = request({
-      path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent)) :
-        Retrofit2SyncCall.executeCall(registryService.getTags(repository, tokenService.basicAuthHeader, userAgent))
+      path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent, queryParams)) :
+        Retrofit2SyncCall.executeCall(registryService.getTags(repository, tokenService.basicAuthHeader, userAgent, queryParams))
     }, { token ->
-      path ? Retrofit2SyncCall.executeCall(registryService.get(path, token, userAgent)) :
-        Retrofit2SyncCall.executeCall(registryService.getTags(repository, token, userAgent))
+      path ? Retrofit2SyncCall.executeCall(registryService.get(path, token, userAgent, queryParams)) :
+        Retrofit2SyncCall.executeCall(registryService.getTags(repository, token, userAgent, queryParams))
     }, repository)
 
     def nextPath = findNextLink(response?.headers())
     def tags = convertResponseBody(response.body(), DockerRegistryTags)
 
     if (nextPath) {
-      def nextTags = getTags(repository, nextPath)
+      def nextPathNew
+      (nextPathNew, queryParams) = parseForQueryParams(nextPath)
+      def nextTags = getTags(repository, nextPathNew, queryParams)
       tags.tags.addAll(nextTags.tags)
     }
 
     return tags
   }
 
+  /**
+   * This method takes a string that might contain a query string and splits it into the path and the query parameters.
+   * @param nextPath the string that might contain a query string
+   * @return a tuple containing the path (without query string) and a map of query parameters
+   */
+  static Tuple2<String, Map<String, String>> parseForQueryParams(String nextPath) {
+    def nextPathNew
+    def queryParamsString
+    Map<String, String> queryParams = [:]
+    if (nextPath.contains("?")) {
+      (nextPathNew, queryParamsString) = nextPath.split("\\?", 2)
+    } else {
+      nextPathNew = nextPath
+    }
+    if (queryParamsString) {
+      queryParams = queryParamsString.split("&").collectEntries { param ->
+        def (key, value) = param.split("=")
+        [key, value]
+      }
+    }
+    [nextPathNew, queryParams]
+  }
+
   /*
    * This method will hit the /v2/ endpoint of the configured docker registry. If it this endpoint is up,
    * it will return silently. Otherwise, an exception is thrown detailing why the endpoint isn't available.

clouddriver-docker/src/test/groovy/com/netflix/spinnaker/clouddriver/docker/registry/api/v2/client/DockerRegistryClientSpec.groovy

@@ -47,7 +47,7 @@ class DockerRegistryClientSpec extends Specification {
   def stubbedRegistryService = Stub(DockerRegistryClient.DockerRegistryService){
     String tagsJson = "{\"name\":\"library/ubuntu\",\"tags\":[\"latest\",\"xenial\",\"rolling\"]}"
     Response tagsResponse =  Response.success(200, ResponseBody.create(MediaType.parse("application/json"), tagsJson))
-    getTags(_,_,_) >> Calls.response(tagsResponse)
+    getTags(_,_,_,_) >> Calls.response(tagsResponse)
 
     String checkJson = "{}"
     Response checkResponse = Response.success(200, ResponseBody.create(MediaType.parse("application/json"), checkJson))

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

@@ -0,0 +1,205 @@
+/*
+ * Copyright 2025 OpsMx, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+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.urlMatching;
+import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertIterableEquals;
+import static org.mockito.ArgumentMatchers.anyString;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.github.tomakehurst.wiremock.client.WireMock;
+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)
+public class DockerRegistryClientTest {
+
+  @RegisterExtension
+  static WireMockExtension wmDockerRegistry =
+      WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build();
+
+  static DockerRegistryClient.DockerRegistryService dockerRegistryService;
+  @MockBean DockerBearerTokenService dockerBearerTokenService;
+  static DockerRegistryClient dockerRegistryClient;
+  @Autowired ServiceClientProvider serviceClientProvider;
+  ObjectMapper objectMapper = new ObjectMapper();
+  Map<String, Object> tagsResponse;
+  String tagsResponseString;
+  String tagsSecondResponseString;
+  String tagsThirdResponseString;
+  Map<String, Object> catalogResponse;
+  String catalogResponseString;
+  String catalogSecondResponseString;
+  String catalogThirdResponseString;
+
+  @BeforeEach
+  public void init() throws JsonProcessingException {
+    tagsResponse =
+        Map.of(
+            "name",
+            "library/nginx",
+            "tags",
+            new String[] {"1", "1-alpine", "1-alpine-otel", "1-alpine-perl", "1-alpine-slim"});
+    catalogResponse =
+        Map.of(
+            "repositories",
+            new String[] {
+              "library/repo-a-1",
+              "library/repo-b-1",
+              "library/repo-c-1",
+              "library/repo-d-1",
+              "library/repo-e-1"
+            });
+    tagsResponseString = objectMapper.writeValueAsString(tagsResponse);
+    tagsSecondResponseString = tagsResponseString.replaceAll("1", "2");
+    tagsThirdResponseString = tagsResponseString.replaceAll("1", "3");
+
+    catalogResponseString = objectMapper.writeValueAsString(catalogResponse);
+    catalogSecondResponseString = catalogResponseString.replaceAll("1", "2");
+    catalogThirdResponseString = catalogResponseString.replaceAll("1", "3");
+
+    DockerBearerToken bearerToken = new DockerBearerToken();
+    bearerToken.setToken("someToken");
+    bearerToken.setAccess_token("someToken");
+    Mockito.when(dockerBearerTokenService.getToken(anyString())).thenReturn(bearerToken);
+    dockerRegistryService =
+        buildService(DockerRegistryClient.DockerRegistryService.class, wmDockerRegistry.baseUrl());
+    dockerRegistryClient =
+        new DockerRegistryClient(
+            wmDockerRegistry.baseUrl(), 5, "", "", dockerRegistryService, dockerBearerTokenService);
+  }
+
+  private static <T> T buildService(Class<T> type, String baseUrl) {
+    return new Retrofit.Builder()
+        .baseUrl(baseUrl)
+        .client(new OkHttpClient())
+        .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
+        .addConverterFactory(JacksonConverterFactory.create())
+        .build()
+        .create(type);
+  }
+
+  @Test
+  public void getTagsWithoutNextLink() {
+    wmDockerRegistry.stubFor(
+        WireMock.get(urlMatching("/v2/library/nginx/tags/list"))
+            .willReturn(
+                aResponse().withStatus(HttpStatus.OK.value()).withBody(tagsResponseString)));
+
+    DockerRegistryTags dockerRegistryTags = dockerRegistryClient.getTags("library/nginx");
+    String[] tags = (String[]) tagsResponse.get("tags");
+    assertIterableEquals(Arrays.asList(tags), dockerRegistryTags.getTags());
+  }
+
+  @Test
+  public void getTagsWithNextLink() {
+    wmDockerRegistry.stubFor(
+        WireMock.get(urlMatching("/v2/library/nginx/tags/list"))
+            .willReturn(
+                aResponse()
+                    .withStatus(HttpStatus.OK.value())
+                    .withHeader(
+                        "link",
+                        "</v2/library/nginx/tags/list?last=1-alpine-slim&n=5>; rel=\"next\"")
+                    .withBody(tagsResponseString)));
+    wmDockerRegistry.stubFor(
+        WireMock.get(urlMatching("/v2/library/nginx/tags/list\\?last=1-alpine-slim&n=5"))
+            .willReturn(
+                aResponse()
+                    .withStatus(HttpStatus.OK.value())
+                    .withHeader(
+                        "link",
+                        // to test the logic when `?` is not present in the link header
+                        "</v2/library/nginx/tags/list1>; rel=\"next\"")
+                    .withBody(tagsSecondResponseString)));
+    wmDockerRegistry.stubFor(
+        WireMock.get(urlMatching("/v2/library/nginx/tags/list1"))
+            .willReturn(
+                aResponse().withStatus(HttpStatus.OK.value()).withBody(tagsThirdResponseString)));
+
+    DockerRegistryTags dockerRegistryTags = dockerRegistryClient.getTags("library/nginx");
+    assertEquals(15, dockerRegistryTags.getTags().size());
+  }
+
+  @Test
+  public void getCatalogWithNextLink() {
+    wmDockerRegistry.stubFor(
+        WireMock.get(urlMatching("/v2/_catalog\\?n=5"))
+            .willReturn(
+                aResponse()
+                    .withStatus(HttpStatus.OK.value())
+                    .withHeader("link", "</v2/_catalog?last=repo1&n=5>; rel=\"next\"")
+                    .withBody(catalogResponseString)));
+    wmDockerRegistry.stubFor(
+        WireMock.get(urlMatching("/v2/_catalog\\?last=repo1&n=5"))
+            .willReturn(
+                aResponse()
+                    .withStatus(HttpStatus.OK.value())
+                    .withHeader(
+                        "link",
+                        // to test the logic when `?` is not present in the link header
+                        "</v2/_catalog1>; rel=\"next\"")
+                    .withBody(catalogSecondResponseString)));
+    wmDockerRegistry.stubFor(
+        WireMock.get(urlMatching("/v2/_catalog1\\?n=5"))
+            .willReturn(
+                aResponse()
+                    .withStatus(HttpStatus.OK.value())
+                    .withBody(catalogThirdResponseString)));
+
+    DockerRegistryCatalog dockerRegistryCatalog = dockerRegistryClient.getCatalog();
+    assertEquals(15, dockerRegistryCatalog.getRepositories().size());
+  }
+}