Merge pull request #15434 from cdapio/bugfix/CDAP-20900-handle-url-en…

…coded-params-in-router

[CDAP-20900] Handle URL encoded data in Internal Router Service

cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/monitor/AbstractServiceRoutingHandler.java

@@ -170,11 +170,10 @@ public void handleError(Throwable throwable) {
   }
 
   /**
-   * Opens a {@link HttpURLConnection} to the given service for the given
-   * program run.
+   * Opens a {@link HttpURLConnection} to the given service.
    *
    * @throws BadRequestException if the request for service routing is not
-   *                             valid
+   *                             valid.
    */
   private HttpURLConnection openConnection(HttpRequest request, String service,
       String path) throws BadRequestException {
@@ -184,7 +183,7 @@ private HttpURLConnection openConnection(HttpRequest request, String service,
       throw new ServiceUnavailableException(service);
     }
 
-    URI uri = URIScheme.createURI(discoverable, path);
+    URI uri = URIScheme.createURI(discoverable, "%s", path);
     LOG.trace("Routing request for service '{}' to uri '{}'.", service, uri);
     try {
       URL url = uri.toURL();

cdap-app-fabric/src/test/java/io/cdap/cdap/internal/app/runtime/monitor/InternalServiceRoutingHandlerTest.java

@@ -200,4 +200,26 @@ public void testServerDisabled() throws IOException, UnauthorizedException {
         .createRemoteClient(MOCK_SERVICE,
             DefaultHttpRequestConfig.DEFAULT, "");
   }
+
+
+  @Test
+  public void testUrlEncodedGetQueryParam()
+      throws IOException, UnauthorizedException {
+    RemoteClient remoteClient = injector.getInstance(RemoteClientFactory.class)
+        .createRemoteClient(
+            Constants.Service.INTERNAL_ROUTER,
+            DefaultHttpRequestConfig.DEFAULT,
+            Constants.Gateway.INTERNAL_API_VERSION_3 + "/router");
+    // We add a %20d at the end of the URL to ensure it that the router
+    // handles URL encoded string correctly.
+    HttpMethod method = HttpMethod.GET;
+    HttpRequest request = remoteClient.requestBuilder(method,
+            String.format("services/%s/mock/%s/%d",
+                MOCK_SERVICE, method.name().toLowerCase(), 200) +
+            "?queryParam=abc%20d")
+        .build();
+
+    HttpResponse response = remoteClient.execute(request);
+    Assert.assertTrue(response.getResponseBodyAsString().contains("abc d"));
+  }
 }

cdap-app-fabric/src/test/java/io/cdap/cdap/internal/app/runtime/monitor/MockServiceHandler.java

@@ -28,6 +28,7 @@
 import javax.ws.rs.PUT;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
 
 /**
  * A mock handler for testing service routing. It provides service endpoints for
@@ -39,9 +40,9 @@ public final class MockServiceHandler extends AbstractHttpHandler {
   @Path("/get/{status}")
   @GET
   public void get(HttpRequest request, HttpResponder responder,
-      @PathParam("status") int statusCode) {
+      @PathParam("status") int statusCode, @QueryParam("queryParam") String queryParam) {
     responder.sendString(HttpResponseStatus.valueOf(statusCode),
-        "Status is " + statusCode);
+        "Status is " + statusCode + " and query param is " + queryParam);
   }
 
   @Path("/delete/{status}")

cdap-app-fabric/src/test/java/io/cdap/cdap/internal/app/services/http/AppFabricTestBase.java

@@ -375,8 +375,7 @@ protected static TransactionSystemClient getTxClient() {
   protected static URI getEndPoint(String path) {
     Discoverable discoverable = appFabricEndpointStrategy.pick(5, TimeUnit.SECONDS);
     Assert.assertNotNull("AppFabric endpoint is missing, service may not be running.", discoverable);
-    // The path is literal and we need to escape "%" before passing to createURI, which takes a format string.
-    return URIScheme.createURI(discoverable, path.replace("%", "%%"));
+    return URIScheme.createURI(discoverable, "%s", path);
   }
 
   protected static HttpResponse doGet(String resource) throws Exception {

cdap-common/src/main/java/io/cdap/cdap/common/discovery/URIScheme.java

@@ -23,21 +23,26 @@
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import org.apache.twill.discovery.Discoverable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Enum representing URI scheme.
  */
 public enum URIScheme {
 
-  HTTP("http", new byte[0], 80),
-  HTTPS("https", "https://".getBytes(StandardCharsets.UTF_8), 443);
+  HTTP("http", new byte[0], 80), HTTPS("https",
+      "https://".getBytes(StandardCharsets.UTF_8), 443);
+
+  private static final Logger LOG = LoggerFactory.getLogger(URIScheme.class);
 
   private final String scheme;
   private final byte[] discoverablePayload;
   private final int defaultPort;
 
   /**
-   * Returns the {@link URIScheme} based on the given {@link Discoverable} payload.
+   * Returns the {@link URIScheme} based on the given {@link Discoverable}
+   * payload.
    */
   public static URIScheme getScheme(Discoverable discoverable) {
     for (URIScheme scheme : values()) {
@@ -77,14 +82,21 @@ public static Discoverable createDiscoverable(String serviceName, NettyHttpServi
    * Creates a {@link URI} based on the scheme from the given {@link Discoverable}.
    */
   public static URI createURI(Discoverable discoverable, String pathFmt, Object... objs) {
+    if (objs.length == 0) {
+      LOG.warn("Received 0 arguments for substitution in the path format. "
+               + "If no substitutions are required, use '%s' as the path format "
+               + "and provide the literal path as as a substitution argument to avoid issues "
+               + "with url encoded strings.");
+    }
     String scheme = getScheme(discoverable).scheme;
     InetSocketAddress address = discoverable.getSocketAddress();
     String path = String.format(pathFmt, objs);
     if (path.startsWith("/")) {
       path = path.substring(1);
     }
     return URI.create(
-        String.format("%s://%s:%d/%s", scheme, address.getHostName(), address.getPort(), path));
+        String.format("%s://%s:%d/%s", scheme, address.getHostName(),
+            address.getPort(), path));
   }
 
 

cdap-common/src/main/java/io/cdap/cdap/common/twill/AbstractMasterServiceManager.java

@@ -198,7 +198,7 @@ public void resetServiceLogLevels(Set<String> loggerNames) {
 
   private boolean isEndpointAlive(Discoverable discoverable) {
     try {
-      URL url = URIScheme.createURI(discoverable, "/ping").toURL();
+      URL url = URIScheme.createURI(discoverable, "%s", "/ping").toURL();
       int responseCode = HttpRequests.execute(HttpRequest.get(url).build(), httpRequestConfig)
           .getResponseCode();
       if (responseCode == HttpURLConnection.HTTP_OK) {

cdap-gateway/src/test/java/io/cdap/cdap/gateway/handlers/log/LogHttpHandlerTest.java

@@ -836,8 +836,7 @@ private HttpResponse doGet(String path) throws IOException {
       () -> discoveryServiceClient.discover(Constants.Service.LOG_QUERY)).pick(10, TimeUnit.SECONDS);
     Assert.assertNotNull(discoverable);
 
-    // Path is literal, hence replacing the "%" with "%%" for formatter
-    URL url = URIScheme.createURI(discoverable, path.replace("%", "%%")).toURL();
+    URL url = URIScheme.createURI(discoverable, "%s", path).toURL();
     return HttpRequests.execute(HttpRequest.get(url).build(), new DefaultHttpRequestConfig(false));
   }
 }

cdap-gateway/src/test/java/io/cdap/cdap/gateway/handlers/metrics/MetricsSuiteTestBase.java

@@ -211,8 +211,7 @@ public static void stopMetricsService(CConfiguration conf) {
   }
 
   public static URI getEndPoint(String path) {
-    // Replace "%" with "%%" for literal path
-    return URIScheme.createURI(discoverable, path.replace("%", "%%"));
+    return URIScheme.createURI(discoverable, "%s", path);
   }
 
   public static HttpResponse doGet(String resource) throws Exception {

cdap-master/src/test/java/io/cdap/cdap/master/environment/k8s/LogsServiceMainTest.java

@@ -116,7 +116,7 @@ private HttpResponse doGet(String path, DiscoveryServiceClient discoveryServiceC
     Discoverable discoverable = new RandomEndpointStrategy(
       () -> discoveryServiceClient.discover(serviceName)).pick(10, TimeUnit.SECONDS);
     Assert.assertNotNull(discoverable);
-    URL url = URIScheme.createURI(discoverable, path).toURL();
+    URL url = URIScheme.createURI(discoverable, "%s", path).toURL();
     return HttpRequests.execute(HttpRequest.get(url).build(), new DefaultHttpRequestConfig(false));
   }
 

cdap-support-bundle/src/test/java/io/cdap/cdap/SupportBundleTestBase.java

@@ -269,8 +269,7 @@ protected static Injector getInjector() {
   protected static URI getEndPoint(String path) {
     Discoverable discoverable = appFabricEndpointStrategy.pick(5, TimeUnit.SECONDS);
     Assert.assertNotNull("SupportBundle endpoint is missing, service may not be running.", discoverable);
-    // The path is literal and we need to escape "%" before passing to createURI, which takes a format string.
-    return URIScheme.createURI(discoverable, path.replace("%", "%%"));
+    return URIScheme.createURI(discoverable,"%s", path);
   }
 
   protected static HttpResponse doGet(String resource) throws Exception {

cdap-support-bundle/src/test/java/io/cdap/cdap/support/handlers/SupportBundleHttpHandlerTest.java

@@ -125,7 +125,7 @@ private String requestBundle(Map<String, String> params)
 
     String path = String.format("%s/support/bundles%s", Constants.Gateway.API_VERSION_3, queryBuilder);
 
-    HttpRequest request = HttpRequest.post(URIScheme.createURI(discoverable, path).toURL()).build();
+    HttpRequest request = HttpRequest.post(URIScheme.createURI(discoverable, "%s", path).toURL()).build();
     AtomicReference<HttpResponse> response =
       new AtomicReference<>(HttpRequests.execute(request, new DefaultHttpRequestConfig(false)));
 
@@ -155,7 +155,7 @@ private HttpResponse requestExportBundle(String bundleId) throws IOException {
 
     String path = String.format("%s/support/bundles/%s", Constants.Gateway.API_VERSION_3, bundleId);
 
-    HttpRequest request = HttpRequest.post(URIScheme.createURI(discoverable, path).toURL()).build();
+    HttpRequest request = HttpRequest.post(URIScheme.createURI(discoverable, "%s", path).toURL()).build();
     return HttpRequests.execute(request, new DefaultHttpRequestConfig(false));
   }
 }