fix(custom-targets): fix bug in transactions, handle more failure cas…

…es, include more helpful text with failure responses (#488)
cryostatio · May 31, 2024 · c87185a · c87185a
1 parent dd420de
commit c87185a
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 33 deletions.
diff --git a/compose/cryostat.yml b/compose/cryostat.yml
@@ -17,10 +17,6 @@ services:
     user: "1000"
     labels:
       kompose.service.expose: "cryostat3"
-      io.cryostat.discovery: "true"
-      io.cryostat.jmxHost: "localhost"
-      io.cryostat.jmxPort: "0"
-      io.cryostat.jmxUrl: "service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi"
     environment:
       QUARKUS_LOG_LEVEL: ALL
       QUARKUS_HTTP_HOST: "cryostat"

diff --git a/compose/reports.yml b/compose/reports.yml
@@ -16,11 +16,8 @@ services:
       - "10001"
     labels:
       kompose.service.expose: "reports"
-      io.cryostat.discovery: "true"
-      io.cryostat.jmxHost: "reports"
-      io.cryostat.jmxPort: "11224"
     environment:
-      JAVA_OPTS_APPEND: "-Dcom.sun.management.jmxremote.autodiscovery=true -Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.port=11224 -Dcom.sun.management.jmxremote.rmi.port=11224 -Djava.rmi.server.hostname=reports -Dcom.sun.management.jmxremote.authenticate=false -Dcom.sun.management.jmxremote.ssl=false -Dcom.sun.management.jmxremote.local.only=false"
+      JAVA_OPTS_APPEND: "-Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.port=11224 -Dcom.sun.management.jmxremote.rmi.port=11224 -Djava.rmi.server.hostname=reports -Dcom.sun.management.jmxremote.authenticate=false -Dcom.sun.management.jmxremote.ssl=false -Dcom.sun.management.jmxremote.local.only=false"
       QUARKUS_HTTP_PORT: 10001
     healthcheck:
       test: curl --fail http://localhost:10001/ || exit 1

diff --git a/src/main/java/io/cryostat/discovery/CustomDiscovery.java b/src/main/java/io/cryostat/discovery/CustomDiscovery.java
@@ -35,7 +35,6 @@
 import io.cryostat.targets.TargetConnectionManager;
 import io.cryostat.util.URIUtil;
 
-import io.quarkus.narayana.jta.QuarkusTransaction;
 import io.quarkus.runtime.StartupEvent;
 import io.vertx.mutiny.core.eventbus.EventBus;
 import jakarta.annotation.security.RolesAllowed;
@@ -152,12 +151,6 @@ Response doV2Create(
             Optional<Credential> credential,
             boolean dryrun,
             boolean storeCredentials) {
-        var beginTx = !QuarkusTransaction.isActive();
-        if (beginTx) {
-            QuarkusTransaction.begin();
-        } else {
-            // No changes needed for this error
-        }
         try {
             target.connectUrl = sanitizeConnectUrl(target.connectUrl.toString());
             if (!uriUtil.validateUri(target.connectUrl)) {
@@ -170,19 +163,47 @@ Response doV2Create(
                         .build();
             }
 
+            if (Target.find("connectUrl", target.connectUrl).singleResultOptional().isPresent()) {
+                return Response.status(Response.Status.BAD_REQUEST.getStatusCode())
+                        .entity(
+                                V2Response.json(
+                                        Response.Status.BAD_REQUEST, "Duplicate connection URL"))
+                        .build();
+            }
+
             try {
-                target.jvmId =
-                        connectionManager
-                                .executeDirect(
-                                        target,
-                                        credential,
-                                        conn -> conn.getJvmIdentifier().getHash())
-                                .await()
-                                .atMost(timeout);
+                String jvmId =
+                        target.jvmId =
+                                connectionManager
+                                        .executeDirect(
+                                                target,
+                                                credential,
+                                                conn -> conn.getJvmIdentifier().getHash())
+                                        .await()
+                                        .atMost(timeout);
+
+                if (Target.find("jvmId", jvmId).count() > 0) {
+                    return Response.status(Response.Status.BAD_REQUEST.getStatusCode())
+                            .entity(
+                                    V2Response.json(
+                                            Response.Status.BAD_REQUEST,
+                                            String.format(
+                                                    "Target with JVM ID \"%s\" already discovered",
+                                                    jvmId)))
+                            .build();
+                }
             } catch (Exception e) {
                 logger.error("Target connection failed", e);
+                String msg =
+                        connectionManager.isJmxSslFailure(e)
+                                ? "Untrusted JMX SSL/TLS certificate"
+                                : connectionManager.isJmxAuthFailure(e)
+                                        ? "JMX authentication failure"
+                                        : connectionManager.isServiceTypeFailure(e)
+                                                ? "Unexpected service type on port"
+                                                : "Target connection failed";
                 return Response.status(Response.Status.BAD_REQUEST.getStatusCode())
-                        .entity(V2Response.json(Response.Status.BAD_REQUEST, e))
+                        .entity(V2Response.json(Response.Status.BAD_REQUEST, msg))
                         .build();
             }
 
@@ -210,22 +231,16 @@ Response doV2Create(
             node.persist();
             realm.persist();
 
-            if (beginTx) {
-                QuarkusTransaction.commit();
-            }
-
             return Response.created(URI.create("/api/v3/targets/" + target.id))
                     .entity(V2Response.json(Response.Status.CREATED, target))
                     .build();
         } catch (Exception e) {
-            // roll back regardless of whether we initiated this database transaction or a
-            // caller
-            // did
-            QuarkusTransaction.rollback();
             if (ExceptionUtils.indexOfType(e, ConstraintViolationException.class) >= 0) {
                 logger.warn("Invalid target definition", e);
                 return Response.status(Response.Status.BAD_REQUEST.getStatusCode())
-                        .entity(V2Response.json(Response.Status.BAD_REQUEST, e))
+                        .entity(
+                                V2Response.json(
+                                        Response.Status.BAD_REQUEST, "Duplicate connection URL"))
                         .build();
             }
             logger.error("Unknown error", e);