Skip to content

Commit 5e32a8a

Browse files
kirangodishalakirangodishala
committed
fix(docker): fix the issue of retrofit2 replacing ? with %3F causing api failure with 400
1 parent 0f7e209 commit 5e32a8a

File tree

4 files changed

+133
-27
lines changed

4 files changed

+133
-27
lines changed

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

Lines changed: 42 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,55 @@ 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+
static def parseForQueryParams(String nextPath) {
478+
def nextPathNew
479+
def queryParamsString
480+
Map<String, String> queryParams = [:]
481+
if (nextPath.contains("?")) {
482+
(nextPathNew, queryParamsString) = nextPath.split("\\?", 2)
483+
} else {
484+
nextPathNew = nextPath
485+
}
486+
if (queryParamsString) {
487+
queryParams = queryParamsString.split("&").collectEntries { param ->
488+
def (key, value) = param.split("=")
489+
[key, value]
490+
}
491+
}
492+
[nextPathNew, queryParams]
493+
}
494+
468495
/*
469496
* This method will hit the /v2/ endpoint of the configured docker registry. If it this endpoint is up,
470497
* 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))

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

Lines changed: 73 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
2020
import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;
2121
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
22+
import static org.junit.jupiter.api.Assertions.assertEquals;
2223
import static org.junit.jupiter.api.Assertions.assertIterableEquals;
2324
import static org.mockito.ArgumentMatchers.anyString;
2425

@@ -35,12 +36,10 @@
3536
import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
3637
import com.netflix.spinnaker.kork.retrofit.Retrofit2ServiceFactory;
3738
import com.netflix.spinnaker.kork.retrofit.Retrofit2ServiceFactoryAutoConfiguration;
38-
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
3939
import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties;
4040
import java.util.Arrays;
4141
import java.util.Map;
4242
import okhttp3.OkHttpClient;
43-
import org.junit.jupiter.api.Assertions;
4443
import org.junit.jupiter.api.BeforeEach;
4544
import org.junit.jupiter.api.Test;
4645
import org.junit.jupiter.api.extension.RegisterExtension;
@@ -78,7 +77,12 @@ public class DockerRegistryClientTest {
7877
ObjectMapper objectMapper = new ObjectMapper();
7978
Map<String, Object> tagsResponse;
8079
String tagsResponseString;
81-
String nextLink = "</v2/library/nginx/tags/list?last=1-alpine-slim&n=5>; rel=\"next\"";
80+
String tagsSecondResponseString;
81+
String tagsThirdResponseString;
82+
Map<String, Object> catalogResponse;
83+
String catalogResponseString;
84+
String catalogSecondResponseString;
85+
String catalogThirdResponseString;
8286

8387
@BeforeEach
8488
public void init() throws JsonProcessingException {
@@ -88,7 +92,23 @@ public void init() throws JsonProcessingException {
8892
"library/nginx",
8993
"tags",
9094
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+
});
91105
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");
92112

93113
DockerBearerToken bearerToken = new DockerBearerToken();
94114
bearerToken.setToken("someToken");
@@ -130,12 +150,56 @@ public void getTagsWithNextLink() {
130150
.willReturn(
131151
aResponse()
132152
.withStatus(HttpStatus.OK.value())
133-
.withHeader("link", nextLink)
153+
.withHeader(
154+
"link",
155+
"</v2/library/nginx/tags/list?last=1-alpine-slim&n=5>; rel=\"next\"")
134156
.withBody(tagsResponseString)));
135-
// TODO: Fix the below error occurring due to retrofit2 replacing `?` with `%3F`
136-
Assertions.assertThrows(
137-
SpinnakerHttpException.class,
138-
() -> dockerRegistryClient.getTags("library/nginx"),
139-
"Status: 404, Method: GET, URL: http://<baseUrl>/v2/library/nginx/tags/list%3Flast=1-alpine-slim&n=5, Message: Not Found");
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());
140204
}
141205
}

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,14 @@
2323
import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall;
2424
import java.io.IOException;
2525
import java.time.Instant;
26+
import java.util.HashMap;
27+
import java.util.Map;
2628
import okhttp3.OkHttpClient;
2729
import okhttp3.ResponseBody;
2830
import org.junit.jupiter.api.BeforeAll;
2931
import org.junit.jupiter.api.Test;
3032
import retrofit2.Call;
33+
import retrofit2.Response;
3134
import retrofit2.Retrofit;
3235
import retrofit2.converter.jackson.JacksonConverterFactory;
3336
import retrofit2.http.GET;
@@ -46,6 +49,7 @@ public class DockerRegistryServiceTest {
4649
String tagsPath = "v2/library/nginx/tags/list";
4750
private Instant expiryTime = Instant.now();
4851
static String bearerToken;
52+
static Map<String, String> queryMap = new HashMap<>();
4953

5054
@BeforeAll
5155
public static void setup() {
@@ -66,7 +70,7 @@ void getToken() {
6670
void getTagsWithToken() throws IOException {
6771
try (ResponseBody response =
6872
Retrofit2SyncCall.execute(
69-
dockerRegistryService.getTags(repository, getBearerToken(), "Spinnaker"))) {
73+
dockerRegistryService.getTags(repository, getBearerToken(), "Spinnaker", Map.of()))) {
7074
assertNotNull(response.string());
7175
}
7276
}
@@ -75,11 +79,22 @@ void getTagsWithToken() throws IOException {
7579
void getTagsWithPathSupplied() throws IOException {
7680
try (ResponseBody response =
7781
Retrofit2SyncCall.execute(
78-
dockerRegistryService.get(tagsPath, getBearerToken(), "Spinnaker"))) {
82+
dockerRegistryService.get(tagsPath, getBearerToken(), "Spinnaker", Map.of()))) {
7983
assertNotNull(response.string());
8084
}
8185
}
8286

87+
@Test
88+
void getTagsWithQueryParams() {
89+
queryMap.put("n", "5");
90+
queryMap.put("last", "1");
91+
Response<ResponseBody> response =
92+
Retrofit2SyncCall.executeCall(
93+
dockerRegistryService.get(tagsPath, getBearerToken(), "Spinnaker", queryMap));
94+
String nextLink = response.headers().get("link");
95+
assert nextLink != null;
96+
}
97+
8398
private String getBearerToken() {
8499
if (bearerToken == null || Instant.now().isAfter(expiryTime)) {
85100
DockerBearerToken token =

0 commit comments

Comments
 (0)