From e33fc46f6bbb734099d8bb6fdd9ef5608c4a9fa6 Mon Sep 17 00:00:00 2001 From: Aggelos Karalias Date: Fri, 5 May 2023 21:48:52 +0300 Subject: [PATCH] fix: #186 token as request param is deprecated Consul now warns aggressively when token is provided as a request param and the X-Consul-Token is required. Also as already mentioned in #186 token request param will be removed in Consul v1.17. By reading the code and related PRs I've seen that in the so called "new http architecture" this is addressed although not all clients fully utilize the new Request class. This PR uses the Request class for both Catalog and KV clients. Should address both #186 and #237. --- .../com/ecwid/consul/v1/ConsulRawClient.java | 1 + .../v1/catalog/CatalogConsulClient.java | 16 ++++- .../v1/catalog/CatalogServiceRequest.java | 4 -- .../v1/catalog/CatalogServicesRequest.java | 4 -- .../consul/v1/kv/KeyValueConsulClient.java | 66 +++++++++++++++---- .../v1/catalog/CatalogConsulClientTest.java | 13 +++- 6 files changed, 79 insertions(+), 25 deletions(-) diff --git a/src/main/java/com/ecwid/consul/v1/ConsulRawClient.java b/src/main/java/com/ecwid/consul/v1/ConsulRawClient.java index f5117f3c..e42326e0 100644 --- a/src/main/java/com/ecwid/consul/v1/ConsulRawClient.java +++ b/src/main/java/com/ecwid/consul/v1/ConsulRawClient.java @@ -170,6 +170,7 @@ public HttpResponse makePutRequest(Request request) { HttpRequest httpRequest = HttpRequest.Builder.newBuilder() .setUrl(url) + .setContent(request.getContent()) // if content is not null then it has priority over binaryContent .setBinaryContent(request.getBinaryContent()) .addHeaders(Utils.createTokenMap(request.getToken())) .build(); diff --git a/src/main/java/com/ecwid/consul/v1/catalog/CatalogConsulClient.java b/src/main/java/com/ecwid/consul/v1/catalog/CatalogConsulClient.java index d1af950a..6b7d913d 100644 --- a/src/main/java/com/ecwid/consul/v1/catalog/CatalogConsulClient.java +++ b/src/main/java/com/ecwid/consul/v1/catalog/CatalogConsulClient.java @@ -140,7 +140,13 @@ public Response>> getCatalogServices(QueryParams queryP @Override public Response>> getCatalogServices(CatalogServicesRequest catalogServicesRequest) { - HttpResponse httpResponse = rawClient.makeGetRequest("/v1/catalog/services", catalogServicesRequest.asUrlParameters()); + Request request = Request.Builder.newBuilder() + .setEndpoint("/v1/catalog/services") + .addUrlParameters(catalogServicesRequest.asUrlParameters()) + .setToken(catalogServicesRequest.getToken()) + .build(); + + HttpResponse httpResponse = rawClient.makeGetRequest(request); if (httpResponse.getStatusCode() == 200) { Map> value = GsonFactory.getGson().fromJson(httpResponse.getContent(), @@ -188,7 +194,13 @@ public Response> getCatal @Override public Response> getCatalogService(String serviceName, CatalogServiceRequest catalogServiceRequest) { - HttpResponse httpResponse = rawClient.makeGetRequest("/v1/catalog/service/" + serviceName, catalogServiceRequest.asUrlParameters()); + Request request = Request.Builder.newBuilder() + .setEndpoint("/v1/catalog/service/" + serviceName) + .addUrlParameters(catalogServiceRequest.asUrlParameters()) + .setToken(catalogServiceRequest.getToken()) + .build(); + + HttpResponse httpResponse = rawClient.makeGetRequest(request); if (httpResponse.getStatusCode() == 200) { List value = GsonFactory.getGson().fromJson(httpResponse.getContent(), diff --git a/src/main/java/com/ecwid/consul/v1/catalog/CatalogServiceRequest.java b/src/main/java/com/ecwid/consul/v1/catalog/CatalogServiceRequest.java index edf9dd27..87f6080e 100644 --- a/src/main/java/com/ecwid/consul/v1/catalog/CatalogServiceRequest.java +++ b/src/main/java/com/ecwid/consul/v1/catalog/CatalogServiceRequest.java @@ -144,10 +144,6 @@ public List asUrlParameters() { params.add(queryParams); } - if (token != null) { - params.add(new SingleUrlParameters("token", token)); - } - return params; } diff --git a/src/main/java/com/ecwid/consul/v1/catalog/CatalogServicesRequest.java b/src/main/java/com/ecwid/consul/v1/catalog/CatalogServicesRequest.java index 30c16b04..32fcd1cf 100644 --- a/src/main/java/com/ecwid/consul/v1/catalog/CatalogServicesRequest.java +++ b/src/main/java/com/ecwid/consul/v1/catalog/CatalogServicesRequest.java @@ -101,10 +101,6 @@ public List asUrlParameters() { params.add(queryParams); } - if (token != null) { - params.add(new SingleUrlParameters("token", token)); - } - return params; } diff --git a/src/main/java/com/ecwid/consul/v1/kv/KeyValueConsulClient.java b/src/main/java/com/ecwid/consul/v1/kv/KeyValueConsulClient.java index 89d5de76..54f0f24e 100644 --- a/src/main/java/com/ecwid/consul/v1/kv/KeyValueConsulClient.java +++ b/src/main/java/com/ecwid/consul/v1/kv/KeyValueConsulClient.java @@ -1,7 +1,5 @@ package com.ecwid.consul.v1.kv; -import java.util.List; - import com.ecwid.consul.ConsulException; import com.ecwid.consul.SingleUrlParameters; import com.ecwid.consul.UrlParameters; @@ -14,6 +12,8 @@ import com.ecwid.consul.v1.kv.model.PutParams; import com.google.gson.reflect.TypeToken; +import java.util.List; + /** * @author Vasily Vasilkov (vgv@ecwid.com) */ @@ -66,8 +66,12 @@ public Response getKVValue(String key, QueryParams queryParams) { @Override public Response getKVValue(String key, String token, QueryParams queryParams) { - UrlParameters tokenParams = token != null ? new SingleUrlParameters("token", token) : null; - HttpResponse httpResponse = rawClient.makeGetRequest("/v1/kv/" + key, tokenParams, queryParams); + Request request = Request.Builder.newBuilder() + .setEndpoint("/v1/kv/" + key) + .addUrlParameter(queryParams) + .setToken(token) + .build(); + HttpResponse httpResponse = rawClient.makeGetRequest(request); if (httpResponse.getStatusCode() == 200) { List value = GsonFactory.getGson().fromJson(httpResponse.getContent(), new TypeToken>() { @@ -104,8 +108,13 @@ public Response getKVBinaryValue(String key, QueryParams queryPa @Override public Response getKVBinaryValue(String key, String token, QueryParams queryParams) { - UrlParameters tokenParams = token != null ? new SingleUrlParameters("token", token) : null; - HttpResponse httpResponse = rawClient.makeGetRequest("/v1/kv/" + key, tokenParams, queryParams); + Request request = Request.Builder.newBuilder() + .setEndpoint("/v1/kv/" + key) + .addUrlParameter(queryParams) + .setToken(token) + .build(); + + HttpResponse httpResponse = rawClient.makeGetRequest(request); if (httpResponse.getStatusCode() == 200) { List value = GsonFactory.getGson().fromJson(httpResponse.getContent(), new TypeToken>() { @@ -143,8 +152,15 @@ public Response> getKVValues(String keyPrefix, QueryParams queryP @Override public Response> getKVValues(String keyPrefix, String token, QueryParams queryParams) { UrlParameters recurseParam = new SingleUrlParameters("recurse"); - UrlParameters tokenParam = token != null ? new SingleUrlParameters("token", token) : null; - HttpResponse httpResponse = rawClient.makeGetRequest("/v1/kv/" + keyPrefix, recurseParam, tokenParam, queryParams); + + Request request = Request.Builder.newBuilder() + .setEndpoint("/v1/kv/" + keyPrefix) + .addUrlParameter(recurseParam) + .addUrlParameter(queryParams) + .setToken(token) + .build(); + + HttpResponse httpResponse = rawClient.makeGetRequest(request); if (httpResponse.getStatusCode() == 200) { List value = GsonFactory.getGson().fromJson(httpResponse.getContent(), new TypeToken>() { @@ -175,8 +191,15 @@ public Response> getKVBinaryValues(String keyPrefix, QueryP @Override public Response> getKVBinaryValues(String keyPrefix, String token, QueryParams queryParams) { UrlParameters recurseParam = new SingleUrlParameters("recurse"); - UrlParameters tokenParam = token != null ? new SingleUrlParameters("token", token) : null; - HttpResponse httpResponse = rawClient.makeGetRequest("/v1/kv/" + keyPrefix, recurseParam, tokenParam, queryParams); + + Request request = Request.Builder.newBuilder() + .setEndpoint("/v1/kv/" + keyPrefix) + .addUrlParameter(recurseParam) + .addUrlParameter(queryParams) + .setToken(token) + .build(); + + HttpResponse httpResponse = rawClient.makeGetRequest(request); if (httpResponse.getStatusCode() == 200) { List value = GsonFactory.getGson().fromJson(httpResponse.getContent(), new TypeToken>() { @@ -208,8 +231,16 @@ public Response> getKVKeysOnly(String keyPrefix, QueryParams queryP public Response> getKVKeysOnly(String keyPrefix, String separator, String token, QueryParams queryParams) { UrlParameters keysParam = new SingleUrlParameters("keys"); UrlParameters separatorParam = separator != null ? new SingleUrlParameters("separator", separator) : null; - UrlParameters tokenParam = token != null ? new SingleUrlParameters("token", token) : null; - HttpResponse httpResponse = rawClient.makeGetRequest("/v1/kv/" + keyPrefix, keysParam, separatorParam, tokenParam, queryParams); + + Request request = Request.Builder.newBuilder() + .setEndpoint("/v1/kv/" + keyPrefix) + .addUrlParameter(keysParam) + .addUrlParameter(separatorParam) + .addUrlParameter(queryParams) + .setToken(token) + .build(); + + HttpResponse httpResponse = rawClient.makeGetRequest(request); if (httpResponse.getStatusCode() == 200) { List value = GsonFactory.getGson().fromJson(httpResponse.getContent(), new TypeToken>() { @@ -250,7 +281,16 @@ public Response setKVValue(String key, String value, PutParams putParam @Override public Response setKVValue(String key, String value, String token, PutParams putParams, QueryParams queryParams) { UrlParameters tokenParam = token != null ? new SingleUrlParameters("token", token) : null; - HttpResponse httpResponse = rawClient.makePutRequest("/v1/kv/" + key, value, putParams, tokenParam, queryParams); + + Request request = Request.Builder.newBuilder() + .setEndpoint("/v1/kv/" + key) + .setContent(value) + .addUrlParameter(putParams) + .addUrlParameter(queryParams) + .setToken(token) + .build(); + + HttpResponse httpResponse = rawClient.makePutRequest(request); if (httpResponse.getStatusCode() == 200) { boolean result = GsonFactory.getGson().fromJson(httpResponse.getContent(), boolean.class); diff --git a/src/test/java/com/ecwid/consul/v1/catalog/CatalogConsulClientTest.java b/src/test/java/com/ecwid/consul/v1/catalog/CatalogConsulClientTest.java index c56a52d3..24a6cba2 100644 --- a/src/test/java/com/ecwid/consul/v1/catalog/CatalogConsulClientTest.java +++ b/src/test/java/com/ecwid/consul/v1/catalog/CatalogConsulClientTest.java @@ -3,7 +3,6 @@ import com.ecwid.consul.ConsulTestConstants; import com.ecwid.consul.v1.Response; import com.ecwid.consul.v1.catalog.model.Node; -import com.ecwid.consul.v1.kv.KeyValueConsulClient; import com.pszymczyk.consul.ConsulProcess; import com.pszymczyk.consul.ConsulStarterBuilder; import com.pszymczyk.consul.infrastructure.Ports; @@ -12,9 +11,10 @@ import org.junit.jupiter.api.Test; import java.util.List; +import java.util.Map; import java.util.Random; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertEquals; class CatalogConsulClientTest { @@ -48,5 +48,14 @@ void testGetCatalogNodes() { assertEquals(1, response.getValue().size()); } + @Test + void testGetCatalogServices() { + CatalogServicesRequest request = CatalogServicesRequest.newBuilder().build(); + Response>> response = consulClient.getCatalogServices(request); + + // We should find only one node – this + assertEquals(1, response.getValue().size()); + } + }