Loosen ServiceRef JMXServiceURL coupling (cryostatio#521)

* Remove constructor * Remove another constructor * Loosen JMXServiceURL coupling Use (absolute) URIs in most API touch-points rather than JMXServiceURLs to reduce coupling and ease future explorations for non-JMX connections * Remove last JMXServiceURL-coupled constructor
jan-law · Jun 21, 2021 · 946e959 · 946e959
1 parent 400ff1c
commit 946e959
Show file tree

Hide file tree

Showing 21 changed files with 324 additions and 143 deletions.
diff --git a/src/main/java/io/cryostat/net/web/http/api/v1/TargetRecordingPatchSave.java b/src/main/java/io/cryostat/net/web/http/api/v1/TargetRecordingPatchSave.java
@@ -39,6 +39,7 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.net.URISyntaxException;
 import java.nio.file.Path;
 import java.time.temporal.ChronoUnit;
 import java.util.Map;
@@ -58,6 +59,7 @@
 import io.cryostat.net.TargetConnectionManager;
 import io.cryostat.net.web.http.HttpMimeType;
 import io.cryostat.platform.PlatformClient;
+import io.cryostat.util.URIUtil;
 
 import io.vertx.ext.web.RoutingContext;
 import io.vertx.ext.web.handler.impl.HttpStatusException;
@@ -140,10 +142,12 @@ private String saveRecording(JFRConnection connection, IRecordingDescriptor desc
                                 serviceRef -> {
                                     try {
                                         return serviceRef
-                                                        .getJMXServiceUrl()
-                                                        .equals(connection.getJMXURL())
+                                                        .getServiceUri()
+                                                        .equals(
+                                                                URIUtil.convert(
+                                                                        connection.getJMXURL()))
                                                 && serviceRef.getAlias().isPresent();
-                                    } catch (IOException ioe) {
+                                    } catch (URISyntaxException | IOException ioe) {
                                         return false;
                                     }
                                 })

diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/TargetDeleteHandler.java b/src/main/java/io/cryostat/net/web/http/api/v2/TargetDeleteHandler.java
@@ -38,15 +38,16 @@
 package io.cryostat.net.web.http.api.v2;
 
 import java.io.IOException;
-import java.net.MalformedURLException;
+import java.net.URI;
+import java.net.URISyntaxException;
 
 import javax.inject.Inject;
-import javax.management.remote.JMXServiceURL;
 
 import io.cryostat.net.AuthManager;
 import io.cryostat.net.web.http.HttpMimeType;
 import io.cryostat.net.web.http.api.ApiVersion;
 import io.cryostat.platform.internal.CustomTargetPlatformClient;
+import io.cryostat.util.URIUtil;
 
 import com.google.gson.Gson;
 import io.vertx.core.http.HttpMethod;
@@ -105,13 +106,13 @@ public IntermediateResponse<Void> handle(RequestParameters params) throws ApiExc
             if (StringUtils.isBlank(targetId)) {
                 throw new ApiException(400, "Invalid targetId");
             }
-            JMXServiceURL serviceUrl = new JMXServiceURL(targetId);
-            if (!customTargetPlatformClient.removeTarget(serviceUrl)) {
+            URI uri = URIUtil.createAbsolute(targetId);
+            if (!customTargetPlatformClient.removeTarget(uri)) {
                 throw new ApiException(404);
             }
             return new IntermediateResponse<Void>().body(null);
-        } catch (MalformedURLException mue) {
-            throw new ApiException(400, "Invalid targetId", mue);
+        } catch (URISyntaxException use) {
+            throw new ApiException(400, "Invalid targetId", use);
         } catch (IOException ioe) {
             throw new ApiException(500, "Internal Error", ioe);
         }

diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/TargetsPostHandler.java b/src/main/java/io/cryostat/net/web/http/api/v2/TargetsPostHandler.java
@@ -38,18 +38,19 @@
 package io.cryostat.net.web.http.api.v2;
 
 import java.io.IOException;
-import java.net.MalformedURLException;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.Objects;
 
 import javax.inject.Inject;
-import javax.management.remote.JMXServiceURL;
 
 import io.cryostat.net.AuthManager;
 import io.cryostat.net.web.http.HttpMimeType;
 import io.cryostat.net.web.http.api.ApiVersion;
 import io.cryostat.platform.PlatformClient;
 import io.cryostat.platform.ServiceRef;
 import io.cryostat.platform.internal.CustomTargetPlatformClient;
+import io.cryostat.util.URIUtil;
 
 import com.google.gson.Gson;
 import io.vertx.core.MultiMap;
@@ -121,19 +122,20 @@ public IntermediateResponse<ServiceRef> handle(RequestParameters params) throws
             if (StringUtils.isBlank(alias)) {
                 throw new ApiException(400, "\"alias\" form parameter must be provided");
             }
-            JMXServiceURL jmxServiceURL = new JMXServiceURL(connectUrl);
-            if (platformClient.listDiscoverableServices().stream()
-                    .anyMatch(sr -> Objects.equals(jmxServiceURL, sr.getJMXServiceUrl()))) {
-                throw new ApiException(400, "Duplicate connectUrl");
+            URI uri = URIUtil.createAbsolute(connectUrl);
+            for (ServiceRef serviceRef : platformClient.listDiscoverableServices()) {
+                if (Objects.equals(uri, serviceRef.getServiceUri())) {
+                    throw new ApiException(400, "Duplicate connectUrl");
+                }
             }
-            ServiceRef serviceRef = new ServiceRef(jmxServiceURL, alias);
+            ServiceRef serviceRef = new ServiceRef(uri, alias);
             boolean v = customTargetPlatformClient.addTarget(serviceRef);
             if (!v) {
                 throw new ApiException(400, "Duplicate connectUrl");
             }
             return new IntermediateResponse<ServiceRef>().body(serviceRef);
-        } catch (MalformedURLException mue) {
-            throw new ApiException(400, "Invalid connectUrl", mue);
+        } catch (URISyntaxException use) {
+            throw new ApiException(400, "Invalid connectUrl", use);
         } catch (IOException ioe) {
             throw new ApiException(500, "Internal Error", ioe);
         }

diff --git a/src/main/java/io/cryostat/platform/ServiceRef.java b/src/main/java/io/cryostat/platform/ServiceRef.java
@@ -37,39 +37,26 @@
  */
 package io.cryostat.platform;
 
-import java.net.MalformedURLException;
+import java.net.URI;
 import java.util.Optional;
 
-import javax.management.remote.JMXServiceURL;
-
-import io.cryostat.core.net.JFRConnectionToolkit;
-
 import com.google.gson.annotations.SerializedName;
 import org.apache.commons.lang3.builder.EqualsBuilder;
 import org.apache.commons.lang3.builder.HashCodeBuilder;
 import org.apache.commons.lang3.builder.ToStringBuilder;
 
 public class ServiceRef {
 
-    private final @SerializedName("connectUrl") JMXServiceURL JMXServiceURL;
+    private final @SerializedName("connectUrl") URI serviceUri;
     private final String alias;
 
-    public ServiceRef(JMXServiceURL jmxServiceUrl, String alias) throws MalformedURLException {
-        this.JMXServiceURL = jmxServiceUrl;
+    public ServiceRef(URI uri, String alias) {
+        this.serviceUri = uri;
         this.alias = alias;
     }
 
-    public ServiceRef(JMXServiceURL jmxServiceUrl) throws MalformedURLException {
-        this(jmxServiceUrl, null);
-    }
-
-    public ServiceRef(JFRConnectionToolkit toolkit, String host, int port, String alias)
-            throws MalformedURLException {
-        this(toolkit.createServiceURL(host, port), alias);
-    }
-
-    public JMXServiceURL getJMXServiceUrl() {
-        return JMXServiceURL;
+    public URI getServiceUri() {
+        return serviceUri;
     }
 
     public Optional<String> getAlias() {

diff --git a/src/main/java/io/cryostat/platform/internal/CustomTargetPlatformClient.java b/src/main/java/io/cryostat/platform/internal/CustomTargetPlatformClient.java
@@ -39,6 +39,7 @@
 
 import java.io.IOException;
 import java.io.Reader;
+import java.net.URI;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
 import java.util.ArrayList;
@@ -48,7 +49,6 @@
 import java.util.TreeSet;
 
 import javax.inject.Named;
-import javax.management.remote.JMXServiceURL;
 
 import io.cryostat.MainModule;
 import io.cryostat.core.net.discovery.JvmDiscoveryClient.EventKind;
@@ -74,12 +74,7 @@ public CustomTargetPlatformClient(
             FileSystem fs,
             Gson gson) {
         super(notificationFactory);
-        this.targets =
-                new TreeSet<>(
-                        (u1, u2) ->
-                                u1.getJMXServiceUrl()
-                                        .toString()
-                                        .compareTo(u2.getJMXServiceUrl().toString()));
+        this.targets = new TreeSet<>((u1, u2) -> u1.getServiceUri().compareTo(u2.getServiceUri()));
         this.saveFile = confDir.resolve(SAVEFILE_NAME);
         this.fs = fs;
         this.gson = gson;
@@ -121,10 +116,10 @@ public boolean removeTarget(ServiceRef serviceRef) throws IOException {
         return v;
     }
 
-    public boolean removeTarget(JMXServiceURL connectUrl) throws IOException {
+    public boolean removeTarget(URI connectUrl) throws IOException {
         ServiceRef ref = null;
         for (ServiceRef target : targets) {
-            if (Objects.equals(connectUrl, target.getJMXServiceUrl())) {
+            if (Objects.equals(connectUrl, target.getServiceUri())) {
                 ref = target;
                 break;
             }

diff --git a/src/main/java/io/cryostat/platform/internal/DefaultPlatformClient.java b/src/main/java/io/cryostat/platform/internal/DefaultPlatformClient.java
@@ -39,6 +39,7 @@
 
 import java.io.IOException;
 import java.net.MalformedURLException;
+import java.net.URISyntaxException;
 import java.util.List;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
@@ -48,6 +49,7 @@
 import io.cryostat.core.net.discovery.JvmDiscoveryClient.JvmDiscoveryEvent;
 import io.cryostat.messaging.notifications.NotificationFactory;
 import io.cryostat.platform.ServiceRef;
+import io.cryostat.util.URIUtil;
 
 class DefaultPlatformClient extends AbstractPlatformClient implements Consumer<JvmDiscoveryEvent> {
 
@@ -74,10 +76,10 @@ public void accept(JvmDiscoveryEvent evt) {
         try {
             ServiceRef serviceRef =
                     new ServiceRef(
-                            evt.getJvmDescriptor().getJmxServiceUrl(),
+                            URIUtil.convert(evt.getJvmDescriptor().getJmxServiceUrl()),
                             evt.getJvmDescriptor().getMainClass());
             notifyAsyncTargetDiscovery(evt.getEventKind(), serviceRef);
-        } catch (MalformedURLException e) {
+        } catch (MalformedURLException | URISyntaxException e) {
             logger.warn(e);
         }
     }
@@ -88,8 +90,9 @@ public List<ServiceRef> listDiscoverableServices() {
                 .map(
                         u -> {
                             try {
-                                return new ServiceRef(u.getJmxServiceUrl(), u.getMainClass());
-                            } catch (MalformedURLException e) {
+                                return new ServiceRef(
+                                        URIUtil.convert(u.getJmxServiceUrl()), u.getMainClass());
+                            } catch (MalformedURLException | URISyntaxException e) {
                                 logger.warn(e);
                                 return null;
                             }

diff --git a/src/main/java/io/cryostat/platform/internal/KubeApiPlatformClient.java b/src/main/java/io/cryostat/platform/internal/KubeApiPlatformClient.java
@@ -49,6 +49,7 @@
 import io.cryostat.core.net.discovery.JvmDiscoveryClient.EventKind;
 import io.cryostat.messaging.notifications.NotificationFactory;
 import io.cryostat.platform.ServiceRef;
+import io.cryostat.util.URIUtil;
 
 import dagger.Lazy;
 import io.fabric8.kubernetes.api.model.EndpointPort;
@@ -174,9 +175,11 @@ private List<ServiceRef> createServiceRefs(EndpointSubset subset, EndpointPort p
                         addr -> {
                             try {
                                 return new ServiceRef(
-                                        connectionToolkit.get(),
-                                        addr.getIp(),
-                                        port.getPort(),
+                                        URIUtil.convert(
+                                                connectionToolkit
+                                                        .get()
+                                                        .createServiceURL(
+                                                                addr.getIp(), port.getPort())),
                                         addr.getTargetRef().getName());
                             } catch (Exception e) {
                                 logger.warn(e);

diff --git a/src/main/java/io/cryostat/platform/internal/KubeEnvPlatformClient.java b/src/main/java/io/cryostat/platform/internal/KubeEnvPlatformClient.java
@@ -50,6 +50,7 @@
 import io.cryostat.core.sys.Environment;
 import io.cryostat.platform.PlatformClient;
 import io.cryostat.platform.ServiceRef;
+import io.cryostat.util.URIUtil;
 
 import dagger.Lazy;
 
@@ -87,7 +88,10 @@ private ServiceRef envToServiceRef(Map.Entry<String, String> entry) {
         String alias = matcher.group(1).toLowerCase();
         int port = Integer.parseInt(matcher.group(2));
         try {
-            return new ServiceRef(connectionToolkit.get(), entry.getValue(), port, alias);
+            return new ServiceRef(
+                    URIUtil.convert(
+                            connectionToolkit.get().createServiceURL(entry.getValue(), port)),
+                    alias);
         } catch (Exception e) {
             logger.warn(e);
             return null;

diff --git a/src/main/java/io/cryostat/util/RelativeURIException.java b/src/main/java/io/cryostat/util/RelativeURIException.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright The Cryostat Authors
+ *
+ * The Universal Permissive License (UPL), Version 1.0
+ *
+ * Subject to the condition set forth below, permission is hereby granted to any
+ * person obtaining a copy of this software, associated documentation and/or data
+ * (collectively the "Software"), free of charge and under any and all copyright
+ * rights in the Software, and any and all patent rights owned or freely
+ * licensable by each licensor hereunder covering either (i) the unmodified
+ * Software as contributed to or provided by such licensor, or (ii) the Larger
+ * Works (as defined below), to deal in both
+ *
+ * (a) the Software, and
+ * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if
+ * one is included with the Software (each a "Larger Work" to which the Software
+ * is contributed by such licensors),
+ *
+ * without restriction, including without limitation the rights to copy, create
+ * derivative works of, display, perform, and distribute the Software and make,
+ * use, sell, offer for sale, import, export, have made, and have sold the
+ * Software and the Larger Work(s), and to sublicense the foregoing rights on
+ * either these or other terms.
+ *
+ * This license is subject to the following condition:
+ * The above copyright notice and either this complete permission notice or at
+ * a minimum a reference to the UPL must be included in all copies or
+ * substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+package io.cryostat.util;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+
+@SuppressWarnings("serial")
+public class RelativeURIException extends URISyntaxException {
+    public RelativeURIException(URI u) {
+        super(u.toString(), "Not a valid absolute URI");
+    }
+}