Skip to content

Commit

Permalink
fix(discovery): plugin registration bugfixes (#1650)
Browse files Browse the repository at this point in the history 
* fix(discovery): delete plugin stored credentials automatically on deregistration/stale prune

* delete any stored credentials on plugin callback ping failure

* bump minimum event loop pool size

* use more specific response code for duplicate matchexpression
  • Loading branch information
@andrewazores
andrewazores authored Sep 13, 2023
1 parent 0d3cbf9 commit 0197f7b
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 35 deletions.
24 changes: 12 additions & 12 deletions smoketest.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@ runDemoApps() {
--env START_DELAY=60 \
--pod cryostat-pod \
--label io.cryostat.discovery="true" \
--label io.cryostat.jmxHost="localhost" \
--label io.cryostat.jmxHost="vertx-fib-demo-0" \
--label io.cryostat.jmxPort="9089" \
--rm -d quay.io/andrewazores/vertx-fib-demo:0.13.0
--rm -d quay.io/andrewazores/vertx-fib-demo:0.13.1

podman run \
--name vertx-fib-demo-1 \
Expand All @@ -142,9 +142,9 @@ runDemoApps() {
--env CRYOSTAT_AGENT_AUTHORIZATION="Basic $(echo user:pass | base64)" \
--pod cryostat-pod \
--label io.cryostat.discovery="true" \
--label io.cryostat.jmxHost="localhost" \
--label io.cryostat.jmxHost="vertx-fib-demo-1" \
--label io.cryostat.jmxPort="9093" \
--rm -d quay.io/andrewazores/vertx-fib-demo:0.13.0
--rm -d quay.io/andrewazores/vertx-fib-demo:0.13.1

podman run \
--name vertx-fib-demo-2 \
Expand All @@ -162,10 +162,10 @@ runDemoApps() {
--env CRYOSTAT_AGENT_AUTHORIZATION="Basic $(echo user:pass | base64)" \
--pod cryostat-pod \
--label io.cryostat.discovery="true" \
--label io.cryostat.jmxHost="localhost" \
--label io.cryostat.jmxHost="vertx-fib-demo-2" \
--label io.cryostat.jmxPort="9094" \
--label io.cryostat.jmxUrl="service:jmx:rmi:///jndi/rmi://localhost:9094/jmxrmi" \
--rm -d quay.io/andrewazores/vertx-fib-demo:0.13.0
--label io.cryostat.jmxUrl="service:jmx:rmi:///jndi/rmi://vertx-fib-demo-2:9094/jmxrmi" \
--rm -d quay.io/andrewazores/vertx-fib-demo:0.13.1

podman run \
--name vertx-fib-demo-3 \
Expand All @@ -184,8 +184,8 @@ runDemoApps() {
--env CRYOSTAT_AGENT_AUTHORIZATION="Basic $(echo user:pass | base64)" \
--pod cryostat-pod \
--label io.cryostat.discovery="true" \
--label io.cryostat.jmxUrl="service:jmx:rmi:///jndi/rmi://localhost:9095/jmxrmi" \
--rm -d quay.io/andrewazores/vertx-fib-demo:0.13.0
--label io.cryostat.jmxUrl="service:jmx:rmi:///jndi/rmi://vertx-fib-demo-3:9095/jmxrmi" \
--rm -d quay.io/andrewazores/vertx-fib-demo:0.13.1

# this config is broken on purpose (missing required env vars) to test the agent's behaviour
# when not properly set up
Expand All @@ -205,7 +205,7 @@ runDemoApps() {
--env JAVA_OPTS="-Dquarkus.http.host=0.0.0.0 -Djava.util.logging.manager=org.jboss.logmanager.LogManager -Dcom.sun.management.jmxremote.port=9097 -Dcom.sun.management.jmxremote.ssl=false -Dcom.sun.management.jmxremote.authenticate=false -javaagent:/deployments/app/cryostat-agent.jar" \
--env QUARKUS_HTTP_PORT=10010 \
--env ORG_ACME_CRYOSTATSERVICE_ENABLED="false" \
--env CRYOSTAT_AGENT_APP_NAME="quarkus-test-agent" \
--env CRYOSTAT_AGENT_APP_NAME="quarkus-test-agent-1" \
--env CRYOSTAT_AGENT_WEBCLIENT_SSL_TRUST_ALL="true" \
--env CRYOSTAT_AGENT_WEBCLIENT_SSL_VERIFY_HOSTNAME="false" \
--env CRYOSTAT_AGENT_WEBSERVER_HOST="localhost" \
Expand All @@ -225,7 +225,7 @@ runDemoApps() {
--env JAVA_OPTS="-Dquarkus.http.host=0.0.0.0 -Djava.util.logging.manager=org.jboss.logmanager.LogManager -javaagent:/deployments/app/cryostat-agent.jar" \
--env QUARKUS_HTTP_PORT=10011 \
--env ORG_ACME_CRYOSTATSERVICE_ENABLED="false" \
--env CRYOSTAT_AGENT_APP_NAME="quarkus-test-agent" \
--env CRYOSTAT_AGENT_APP_NAME="quarkus-test-agent-2" \
--env CRYOSTAT_AGENT_WEBCLIENT_SSL_TRUST_ALL="true" \
--env CRYOSTAT_AGENT_WEBCLIENT_SSL_VERIFY_HOSTNAME="false" \
--env CRYOSTAT_AGENT_WEBSERVER_HOST="localhost" \
Expand All @@ -234,7 +234,7 @@ runDemoApps() {
--env CRYOSTAT_AGENT_BASEURI="${protocol}://localhost:${webPort}/" \
--env CRYOSTAT_AGENT_TRUST_ALL="true" \
--env CRYOSTAT_AGENT_AUTHORIZATION="Basic $(echo user:pass | base64)" \
--env CRYOSTAT_AGENT_REGISTRATION_PREFER_JMX="true" \
--env CRYOSTAT_AGENT_REGISTRATION_PREFER_JMX="false" \
--rm -d quay.io/andrewazores/quarkus-test:latest

# copy a jboss-client.jar into /clientlib first
Expand Down
56 changes: 36 additions & 20 deletions src/main/java/io/cryostat/discovery/DiscoveryStorage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,26 +241,17 @@ private Future<Boolean> ping(HttpMethod mtd, URI uri) {
.ssl("https".equals(uri.getScheme()))
.timeout(1_000)
.followRedirects(true);
String userInfo = uri.getUserInfo();
if (StringUtils.isNotBlank(userInfo) && userInfo.contains(":")) {
String[] parts = userInfo.split(":");
if ("storedcredentials".equals(parts[0])) {
logger.info(
"Using stored credentials id:{} referenced in ping callback userinfo",
parts[1]);
Optional<StoredCredentials> opt =
credentialsManager.get().getById(Integer.parseInt(parts[1]));
if (opt.isEmpty()) {
logger.warn("Could not find such credentials!");
return Future.succeededFuture(false);
}
StoredCredentials credentials = opt.get();
req =
req.authentication(
new UsernamePasswordCredentials(
credentials.getCredentials().getUsername(),
credentials.getCredentials().getPassword()));
}
Optional<StoredCredentials> opt = getStoredCredentials(uri);
if (opt.isPresent()) {
StoredCredentials credentials = opt.get();
logger.info(
"Using stored credentials id:{} referenced in ping callback userinfo",
credentials.getId());
req =
req.authentication(
new UsernamePasswordCredentials(
credentials.getCredentials().getUsername(),
credentials.getCredentials().getPassword()));
}
return req.send()
.onComplete(
Expand All @@ -285,6 +276,29 @@ private Future<Boolean> ping(HttpMethod mtd, URI uri) {
.otherwise(false);
}

private Optional<StoredCredentials> getStoredCredentials(URI uri) {
if (uri == NO_CALLBACK) {
return Optional.empty();
}
String userInfo = uri.getUserInfo();
if (StringUtils.isNotBlank(userInfo) && userInfo.contains(":")) {
String[] parts = userInfo.split(":");
if ("storedcredentials".equals(parts[0])) {
Optional<StoredCredentials> opt =
credentialsManager.get().getById(Integer.parseInt(parts[1]));
if (opt.isEmpty()) {
logger.warn("Could not find stored credentials with id:{} !", parts[1]);
}
return opt;
}
}
return Optional.empty();
}

private void deleteStoredCredentials(URI uri) {
getStoredCredentials(uri).ifPresent(sc -> credentialsManager.get().delete(sc.getId()));
}

private void removePlugin(UUID uuid, Object label) {
deregister(uuid);
logger.info("Stale discovery service {} removed", label);
Expand Down Expand Up @@ -322,6 +336,7 @@ public UUID register(String realm, URI callback) throws RegistrationException {
logger.trace("Discovery Registration: \"{}\" [{}]", realm, id);
return updated.getId();
} catch (Exception e) {
deleteStoredCredentials(callback);
throw new RegistrationException(realm, callback, e, e.getMessage());
}
}
Expand Down Expand Up @@ -393,6 +408,7 @@ public List<? extends AbstractNode> update(

public PluginInfo deregister(UUID id) {
PluginInfo plugin = dao.get(id).orElseThrow(() -> new NotFoundException(id));
deleteStoredCredentials(plugin.getCallback());
dao.delete(id);
findLeavesFrom(gson.fromJson(plugin.getSubtree(), EnvironmentNode.class)).stream()
.map(TargetNode::getTarget)
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/cryostat/net/NetworkModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ static Vertx provideVertx() {
return Vertx.vertx(
new VertxOptions()
.setPreferNativeTransport(true)
.setEventLoopPoolSize(defaults.getEventLoopPoolSize() + 4));
.setEventLoopPoolSize(defaults.getEventLoopPoolSize() + 6));
}

@Provides
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/cryostat/net/web/http/api/beta/MatchExpressionsPostHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public IntermediateResponse<MatchedMatchExpression> handle(RequestParameters par
throw new ApiException(400, "Unable to parse JSON", e);
} catch (RollbackException e) {
if (ExceptionUtils.indexOfType(e, ConstraintViolationException.class) >= 0) {
throw new ApiException(400, "Duplicate matchExpression", e);
throw new ApiException(409, "Duplicate matchExpression", e);
}
throw new ApiException(500, e);
} catch (MatchExpressionValidationException e) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/cryostat/net/web/http/api/v2/CredentialsPostHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public IntermediateResponse<Void> handle(RequestParameters params) throws ApiExc
.body(null);
} catch (RollbackException e) {
if (ExceptionUtils.indexOfType(e, ConstraintViolationException.class) >= 0) {
throw new ApiException(400, "Duplicate matchExpression", e);
throw new ApiException(409, "Duplicate matchExpression", e);
}
throw new ApiException(500, e);
} catch (MatchExpressionValidationException e) {
Expand Down

0 comments on commit 0197f7b

Please sign in to comment.