From c87185a96a67dc00599ea347692bf53380cb6493 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 31 May 2024 15:32:04 -0400 Subject: [PATCH] fix(custom-targets): fix bug in transactions, handle more failure cases, include more helpful text with failure responses (#488) --- compose/cryostat.yml | 4 -- compose/reports.yml | 5 +- .../cryostat/discovery/CustomDiscovery.java | 65 ++++++++++++------- 3 files changed, 41 insertions(+), 33 deletions(-) diff --git a/compose/cryostat.yml b/compose/cryostat.yml index bfb853ca1..80dd96fe3 100644 --- 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 index 9e4e96262..29e7da021 100644 --- 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 index 6b8500087..bbcd6d26a 100644 --- 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, 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);