Skip to content

Commit 0210189

Browse files
fix(docker): fix retrofit2 docker client which is replacing ? with %3F causing api failure (#6354)
* test(docker): add two test classes, one to demonstrate how real docker registry apis can be accessed using retrofit2 client and another to demonstrate the issue of retrofit2 replacing `?` with `%3F` causing api failure with 400 * fix(docker): fix the issue of retrofit2 replacing `?` with `%3F` causing api failure with 400 * fix(docker): address review comments on the fix to the issue of retrofit2 replacing `?` with `%3F` causing api failure with 400
1 parent 6a65adf commit 0210189

File tree

5 files changed

+259
-19
lines changed

5 files changed

+259
-19
lines changed

clouddriver-docker/clouddriver-docker.gradle

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ dependencies {
1717
implementation "io.spinnaker.fiat:fiat-core:$fiatVersion"
1818
implementation "io.spinnaker.kork:kork-credentials"
1919
implementation "io.spinnaker.kork:kork-retrofit"
20+
implementation "io.spinnaker.kork:kork-retrofit2"
2021
implementation "io.spinnaker.kork:kork-exceptions"
2122
implementation "io.spinnaker.kork:kork-web"
2223

@@ -36,6 +37,6 @@ dependencies {
3637
testImplementation "org.springframework.security:spring-security-test"
3738
testImplementation "com.github.tomakehurst:wiremock-jre8"
3839
testImplementation "io.spinnaker.kork:kork-core"
39-
implementation "io.spinnaker.kork:kork-retrofit2"
40-
implementation "io.spinnaker.kork:kork-web"
40+
testImplementation "com.github.tomakehurst:wiremock-jre8-standalone"
41+
4142
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth
1919
import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.DockerUserAgent
2020
import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.exception.DockerRegistryAuthenticationException
2121
import com.netflix.spinnaker.config.DefaultServiceEndpoint
22+
import com.netflix.spinnaker.kork.annotations.VisibleForTesting
2223
import com.netflix.spinnaker.kork.client.ServiceClientProvider
2324
import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall
2425
import groovy.util.logging.Slf4j
@@ -236,7 +237,8 @@ class DockerBearerTokenService {
236237
cachedTokens.remove(repository)
237238
}
238239

239-
private interface TokenService {
240+
@VisibleForTesting
241+
interface TokenService {
240242
@GET("/{path}")
241243
@Headers([
242244
"Docker-Distribution-API-Version: registry/2.0"

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

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import retrofit2.http.Header
4141
import retrofit2.http.Headers
4242
import retrofit2.http.Path
4343
import retrofit2.http.Query
44+
import retrofit2.http.QueryMap
4445

4546
import java.time.Instant
4647

@@ -242,7 +243,8 @@ class DockerRegistryClient {
242243
@Headers([
243244
"Docker-Distribution-API-Version: registry/2.0"
244245
])
245-
Call<ResponseBody> getTags(@Path(value="repository", encoded=true) String repository, @Header("Authorization") String token, @Header("User-Agent") String agent)
246+
Call<ResponseBody> getTags(@Path(value="repository", encoded=true) String repository, @Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap Map<String, String> queryParams)
247+
246248

247249
@GET("/v2/{name}/manifests/{reference}")
248250
@Headers([
@@ -261,13 +263,15 @@ class DockerRegistryClient {
261263
@Headers([
262264
"Docker-Distribution-API-Version: registry/2.0"
263265
])
264-
Call<ResponseBody> getCatalog(@Query(value="n") int paginateSize, @Header("Authorization") String token, @Header("User-Agent") String agent)
266+
Call<ResponseBody> getCatalog(@Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap Map<String, String> queryParams)
267+
265268

266269
@GET("/{path}")
267270
@Headers([
268271
"Docker-Distribution-API-Version: registry/2.0"
269272
])
270-
Call<ResponseBody> get(@Path(value="path", encoded=true) String path, @Header("Authorization") String token, @Header("User-Agent") String agent)
273+
Call<ResponseBody> get(@Path(value="path", encoded=true) String path, @Header("Authorization") String token, @Header("User-Agent") String agent, @QueryMap Map<String, String> queryParams)
274+
271275

272276
@GET("/v2/")
273277
@Headers([
@@ -406,7 +410,7 @@ class DockerRegistryClient {
406410
* This method will get all repositories available on this registry. It may fail, as some registries
407411
* don't want you to download their whole catalog (it's potentially a lot of data).
408412
*/
409-
public DockerRegistryCatalog getCatalog(String path = null) {
413+
public DockerRegistryCatalog getCatalog(String path = null, Map<String, String> queryParams = [:]) {
410414
if (catalogFile) {
411415
log.info("Using catalog list at $catalogFile")
412416
try {
@@ -417,14 +421,15 @@ class DockerRegistryClient {
417421
}
418422
}
419423

424+
queryParams.computeIfAbsent("n", { paginateSize.toString() })
420425
def response
421426
try {
422427
response = request({
423-
path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent)) :
424-
Retrofit2SyncCall.executeCall(registryService.getCatalog(paginateSize, tokenService.basicAuthHeader, userAgent))
428+
path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent, queryParams)) :
429+
Retrofit2SyncCall.executeCall(registryService.getCatalog(tokenService.basicAuthHeader, userAgent, queryParams))
425430
}, { token ->
426-
path ? Retrofit2SyncCall.executeCall(registryService.get(path, token, userAgent)) :
427-
Retrofit2SyncCall.executeCall(registryService.getCatalog(paginateSize, token, userAgent))
431+
path ? Retrofit2SyncCall.executeCall(registryService.get(path, token, userAgent, queryParams)) :
432+
Retrofit2SyncCall.executeCall(registryService.getCatalog(token, userAgent, queryParams))
428433
}, "_catalog")
429434
} catch (Exception e) {
430435
log.warn("Error encountered during catalog of $path", e)
@@ -438,33 +443,60 @@ class DockerRegistryClient {
438443
catalog.repositories = catalog.repositories.findAll { it ==~ repositoriesRegex }
439444
}
440445
if (nextPath) {
441-
def nextCatalog = getCatalog(nextPath)
446+
def nextPathNew
447+
(nextPathNew, queryParams) = parseForQueryParams(nextPath)
448+
def nextCatalog = getCatalog(nextPathNew, queryParams)
442449
catalog.repositories.addAll(nextCatalog.repositories)
443450
}
444451

445452
return catalog
446453
}
447454

448-
public DockerRegistryTags getTags(String repository, String path = null) {
455+
public DockerRegistryTags getTags(String repository, String path = null, Map<String, String> queryParams = [:]) {
449456
def response = request({
450-
path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent)) :
451-
Retrofit2SyncCall.executeCall(registryService.getTags(repository, tokenService.basicAuthHeader, userAgent))
457+
path ? Retrofit2SyncCall.executeCall(registryService.get(path, tokenService.basicAuthHeader, userAgent, queryParams)) :
458+
Retrofit2SyncCall.executeCall(registryService.getTags(repository, tokenService.basicAuthHeader, userAgent, queryParams))
452459
}, { token ->
453-
path ? Retrofit2SyncCall.executeCall(registryService.get(path, token, userAgent)) :
454-
Retrofit2SyncCall.executeCall(registryService.getTags(repository, token, userAgent))
460+
path ? Retrofit2SyncCall.executeCall(registryService.get(path, token, userAgent, queryParams)) :
461+
Retrofit2SyncCall.executeCall(registryService.getTags(repository, token, userAgent, queryParams))
455462
}, repository)
456463

457464
def nextPath = findNextLink(response?.headers())
458465
def tags = convertResponseBody(response.body(), DockerRegistryTags)
459466

460467
if (nextPath) {
461-
def nextTags = getTags(repository, nextPath)
468+
def nextPathNew
469+
(nextPathNew, queryParams) = parseForQueryParams(nextPath)
470+
def nextTags = getTags(repository, nextPathNew, queryParams)
462471
tags.tags.addAll(nextTags.tags)
463472
}
464473

465474
return tags
466475
}
467476

477+
/**
478+
* This method takes a string that might contain a query string and splits it into the path and the query parameters.
479+
* @param nextPath the string that might contain a query string
480+
* @return a tuple containing the path (without query string) and a map of query parameters
481+
*/
482+
static Tuple2<String, Map<String, String>> parseForQueryParams(String nextPath) {
483+
def nextPathNew
484+
def queryParamsString
485+
Map<String, String> queryParams = [:]
486+
if (nextPath.contains("?")) {
487+
(nextPathNew, queryParamsString) = nextPath.split("\\?", 2)
488+
} else {
489+
nextPathNew = nextPath
490+
}
491+
if (queryParamsString) {
492+
queryParams = queryParamsString.split("&").collectEntries { param ->
493+
def (key, value) = param.split("=")
494+
[key, value]
495+
}
496+
}
497+
[nextPathNew, queryParams]
498+
}
499+
468500
/*
469501
* This method will hit the /v2/ endpoint of the configured docker registry. If it this endpoint is up,
470502
* 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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ class DockerRegistryClientSpec extends Specification {
4747
def stubbedRegistryService = Stub(DockerRegistryClient.DockerRegistryService){
4848
String tagsJson = "{\"name\":\"library/ubuntu\",\"tags\":[\"latest\",\"xenial\",\"rolling\"]}"
4949
Response tagsResponse = Response.success(200, ResponseBody.create(MediaType.parse("application/json"), tagsJson))
50-
getTags(_,_,_) >> Calls.response(tagsResponse)
50+
getTags(_,_,_,_) >> Calls.response(tagsResponse)
5151

5252
String checkJson = "{}"
5353
Response checkResponse = Response.success(200, ResponseBody.create(MediaType.parse("application/json"), checkJson))
Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
1+
/*
2+
* Copyright 2025 OpsMx, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.netflix.spinnaker.clouddriver.docker.registry.api.v2.client;
18+
19+
import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
20+
import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;
21+
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
22+
import static org.junit.jupiter.api.Assertions.assertEquals;
23+
import static org.junit.jupiter.api.Assertions.assertIterableEquals;
24+
import static org.mockito.ArgumentMatchers.anyString;
25+
26+
import com.fasterxml.jackson.core.JsonProcessingException;
27+
import com.fasterxml.jackson.databind.ObjectMapper;
28+
import com.github.tomakehurst.wiremock.client.WireMock;
29+
import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
30+
import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth.DockerBearerToken;
31+
import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth.DockerBearerTokenService;
32+
import com.netflix.spinnaker.config.DefaultServiceClientProvider;
33+
import com.netflix.spinnaker.config.okhttp3.DefaultOkHttpClientBuilderProvider;
34+
import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
35+
import com.netflix.spinnaker.kork.client.ServiceClientProvider;
36+
import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
37+
import com.netflix.spinnaker.kork.retrofit.Retrofit2ServiceFactory;
38+
import com.netflix.spinnaker.kork.retrofit.Retrofit2ServiceFactoryAutoConfiguration;
39+
import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties;
40+
import java.util.Arrays;
41+
import java.util.Map;
42+
import okhttp3.OkHttpClient;
43+
import org.junit.jupiter.api.BeforeEach;
44+
import org.junit.jupiter.api.Test;
45+
import org.junit.jupiter.api.extension.RegisterExtension;
46+
import org.mockito.Mockito;
47+
import org.springframework.beans.factory.annotation.Autowired;
48+
import org.springframework.boot.test.context.SpringBootTest;
49+
import org.springframework.boot.test.mock.mockito.MockBean;
50+
import org.springframework.http.HttpStatus;
51+
import retrofit2.Retrofit;
52+
import retrofit2.converter.jackson.JacksonConverterFactory;
53+
54+
@SpringBootTest(
55+
classes = {
56+
OkHttpClientConfigurationProperties.class,
57+
Retrofit2ServiceFactory.class,
58+
ServiceClientProvider.class,
59+
OkHttpClientProvider.class,
60+
OkHttpClient.class,
61+
DefaultServiceClientProvider.class,
62+
DefaultOkHttpClientBuilderProvider.class,
63+
Retrofit2ServiceFactoryAutoConfiguration.class,
64+
ObjectMapper.class
65+
},
66+
webEnvironment = SpringBootTest.WebEnvironment.NONE)
67+
public class DockerRegistryClientTest {
68+
69+
@RegisterExtension
70+
static WireMockExtension wmDockerRegistry =
71+
WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build();
72+
73+
static DockerRegistryClient.DockerRegistryService dockerRegistryService;
74+
@MockBean DockerBearerTokenService dockerBearerTokenService;
75+
static DockerRegistryClient dockerRegistryClient;
76+
@Autowired ServiceClientProvider serviceClientProvider;
77+
ObjectMapper objectMapper = new ObjectMapper();
78+
Map<String, Object> tagsResponse;
79+
String tagsResponseString;
80+
String tagsSecondResponseString;
81+
String tagsThirdResponseString;
82+
Map<String, Object> catalogResponse;
83+
String catalogResponseString;
84+
String catalogSecondResponseString;
85+
String catalogThirdResponseString;
86+
87+
@BeforeEach
88+
public void init() throws JsonProcessingException {
89+
tagsResponse =
90+
Map.of(
91+
"name",
92+
"library/nginx",
93+
"tags",
94+
new String[] {"1", "1-alpine", "1-alpine-otel", "1-alpine-perl", "1-alpine-slim"});
95+
catalogResponse =
96+
Map.of(
97+
"repositories",
98+
new String[] {
99+
"library/repo-a-1",
100+
"library/repo-b-1",
101+
"library/repo-c-1",
102+
"library/repo-d-1",
103+
"library/repo-e-1"
104+
});
105+
tagsResponseString = objectMapper.writeValueAsString(tagsResponse);
106+
tagsSecondResponseString = tagsResponseString.replaceAll("1", "2");
107+
tagsThirdResponseString = tagsResponseString.replaceAll("1", "3");
108+
109+
catalogResponseString = objectMapper.writeValueAsString(catalogResponse);
110+
catalogSecondResponseString = catalogResponseString.replaceAll("1", "2");
111+
catalogThirdResponseString = catalogResponseString.replaceAll("1", "3");
112+
113+
DockerBearerToken bearerToken = new DockerBearerToken();
114+
bearerToken.setToken("someToken");
115+
bearerToken.setAccess_token("someToken");
116+
Mockito.when(dockerBearerTokenService.getToken(anyString())).thenReturn(bearerToken);
117+
dockerRegistryService =
118+
buildService(DockerRegistryClient.DockerRegistryService.class, wmDockerRegistry.baseUrl());
119+
dockerRegistryClient =
120+
new DockerRegistryClient(
121+
wmDockerRegistry.baseUrl(), 5, "", "", dockerRegistryService, dockerBearerTokenService);
122+
}
123+
124+
private static <T> T buildService(Class<T> type, String baseUrl) {
125+
return new Retrofit.Builder()
126+
.baseUrl(baseUrl)
127+
.client(new OkHttpClient())
128+
.addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
129+
.addConverterFactory(JacksonConverterFactory.create())
130+
.build()
131+
.create(type);
132+
}
133+
134+
@Test
135+
public void getTagsWithoutNextLink() {
136+
wmDockerRegistry.stubFor(
137+
WireMock.get(urlMatching("/v2/library/nginx/tags/list"))
138+
.willReturn(
139+
aResponse().withStatus(HttpStatus.OK.value()).withBody(tagsResponseString)));
140+
141+
DockerRegistryTags dockerRegistryTags = dockerRegistryClient.getTags("library/nginx");
142+
String[] tags = (String[]) tagsResponse.get("tags");
143+
assertIterableEquals(Arrays.asList(tags), dockerRegistryTags.getTags());
144+
}
145+
146+
@Test
147+
public void getTagsWithNextLink() {
148+
wmDockerRegistry.stubFor(
149+
WireMock.get(urlMatching("/v2/library/nginx/tags/list"))
150+
.willReturn(
151+
aResponse()
152+
.withStatus(HttpStatus.OK.value())
153+
.withHeader(
154+
"link",
155+
"</v2/library/nginx/tags/list?last=1-alpine-slim&n=5>; rel=\"next\"")
156+
.withBody(tagsResponseString)));
157+
wmDockerRegistry.stubFor(
158+
WireMock.get(urlMatching("/v2/library/nginx/tags/list\\?last=1-alpine-slim&n=5"))
159+
.willReturn(
160+
aResponse()
161+
.withStatus(HttpStatus.OK.value())
162+
.withHeader(
163+
"link",
164+
// to test the logic when `?` is not present in the link header
165+
"</v2/library/nginx/tags/list1>; rel=\"next\"")
166+
.withBody(tagsSecondResponseString)));
167+
wmDockerRegistry.stubFor(
168+
WireMock.get(urlMatching("/v2/library/nginx/tags/list1"))
169+
.willReturn(
170+
aResponse().withStatus(HttpStatus.OK.value()).withBody(tagsThirdResponseString)));
171+
172+
DockerRegistryTags dockerRegistryTags = dockerRegistryClient.getTags("library/nginx");
173+
assertEquals(15, dockerRegistryTags.getTags().size());
174+
}
175+
176+
@Test
177+
public void getCatalogWithNextLink() {
178+
wmDockerRegistry.stubFor(
179+
WireMock.get(urlMatching("/v2/_catalog\\?n=5"))
180+
.willReturn(
181+
aResponse()
182+
.withStatus(HttpStatus.OK.value())
183+
.withHeader("link", "</v2/_catalog?last=repo1&n=5>; rel=\"next\"")
184+
.withBody(catalogResponseString)));
185+
wmDockerRegistry.stubFor(
186+
WireMock.get(urlMatching("/v2/_catalog\\?last=repo1&n=5"))
187+
.willReturn(
188+
aResponse()
189+
.withStatus(HttpStatus.OK.value())
190+
.withHeader(
191+
"link",
192+
// to test the logic when `?` is not present in the link header
193+
"</v2/_catalog1>; rel=\"next\"")
194+
.withBody(catalogSecondResponseString)));
195+
wmDockerRegistry.stubFor(
196+
WireMock.get(urlMatching("/v2/_catalog1\\?n=5"))
197+
.willReturn(
198+
aResponse()
199+
.withStatus(HttpStatus.OK.value())
200+
.withBody(catalogThirdResponseString)));
201+
202+
DockerRegistryCatalog dockerRegistryCatalog = dockerRegistryClient.getCatalog();
203+
assertEquals(15, dockerRegistryCatalog.getRepositories().size());
204+
}
205+
}

0 commit comments

Comments
 (0)