From 79cbaa8b667203db1bccca5e4f7d5cc4d8bcf18f Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Apr 2026 13:17:12 -0400 Subject: [PATCH 01/24] fix(discovery): tolerate temporary plugin discovery failures --- .../java/io/cryostat/ConfigProperties.java | 2 ++ .../java/io/cryostat/discovery/Discovery.java | 30 +++++++++++++++++-- .../cryostat/discovery/DiscoveryPlugin.java | 8 +++++ src/main/resources/application.properties | 1 + .../db/migration/V4.2.0__cryostat.sql | 5 ++++ 5 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/cryostat/ConfigProperties.java b/src/main/java/io/cryostat/ConfigProperties.java index 163ac15c0..9916498e0 100644 --- a/src/main/java/io/cryostat/ConfigProperties.java +++ b/src/main/java/io/cryostat/ConfigProperties.java @@ -50,6 +50,8 @@ public class ConfigProperties { public static final String CONTAINERS_POLL_PERIOD = "cryostat.discovery.containers.poll-period"; public static final String CONTAINERS_REQUEST_TIMEOUT = "cryostat.discovery.containers.request-timeout"; + public static final String DISCOVERY_PLUGINS_MAX_FAILURES = + "cryostat.discovery.plugins.max-failures"; public static final String CONNECTIONS_TTL = "cryostat.connections.ttl"; public static final String CONNECTIONS_FAILED_BACKOFF = "cryostat.connections.failed-backoff"; diff --git a/src/main/java/io/cryostat/discovery/Discovery.java b/src/main/java/io/cryostat/discovery/Discovery.java index a9e23a409..56d7f56bd 100644 --- a/src/main/java/io/cryostat/discovery/Discovery.java +++ b/src/main/java/io/cryostat/discovery/Discovery.java @@ -118,6 +118,9 @@ public class Discovery { @ConfigProperty(name = "cryostat.discovery.plugins.ping-period") Duration discoveryPingPeriod; + @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_MAX_FAILURES) + int maxConsecutiveFailures; + @ConfigProperty(name = ConfigProperties.AGENT_TLS_REQUIRED) boolean agentTlsRequired; @@ -816,6 +819,9 @@ private static record NodeContext( static class RefreshPluginJob implements Job { @Inject Logger logger; + @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_MAX_FAILURES) + int maxConsecutiveFailures; + @Override @Transactional public void execute(JobExecutionContext context) throws JobExecutionException { @@ -836,6 +842,10 @@ public void execute(JobExecutionContext context) throws JobExecutionException { logger.debugv( "Retained discovery plugin: {0} @ {1}", plugin.realm, plugin.callback); } + + plugin.consecutiveFailures = 0; + plugin.lastSuccessfulPing = Instant.now(); + plugin.persist(); } catch (NoResultException e) { logger.debugv( e, @@ -846,9 +856,23 @@ public void execute(JobExecutionContext context) throws JobExecutionException { throw ex; } catch (Exception e) { if (plugin != null) { - logger.debugv( - e, "Pruned discovery plugin: {0} @ {1}", plugin.realm, plugin.callback); - plugin.delete(); + plugin.consecutiveFailures++; + plugin.persist(); + + if (plugin.consecutiveFailures >= maxConsecutiveFailures) { + logger.warnv( + "Pruning discovery plugin after {0} consecutive failures: {1} @" + + " {2}", + plugin.consecutiveFailures, plugin.realm.name, plugin.callback); + plugin.delete(); + } else { + logger.debugv( + "Discovery plugin ping failed ({0}/{1}): {2} @ {3}", + plugin.consecutiveFailures, + maxConsecutiveFailures, + plugin.realm.name, + plugin.callback); + } } else { var ex = new JobExecutionException(e); ex.setUnscheduleFiringTrigger(true); diff --git a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java index 2a0df0ae7..3fc31c35a 100644 --- a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java +++ b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java @@ -17,6 +17,7 @@ import java.net.URI; import java.net.URISyntaxException; +import java.time.Instant; import java.util.UUID; import java.util.function.Supplier; @@ -109,6 +110,13 @@ public class DiscoveryPlugin extends PanacheEntityBase { @JsonProperty(access = JsonProperty.Access.READ_ONLY) public boolean builtin; + @Column(nullable = false) + public int consecutiveFailures = 0; + + @Column(nullable = true) + @Convert(converter = InstantConverter.class) + public Instant lastSuccessfulPing; + @ApplicationScoped static class Listener { diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 257895cae..cb17b8675 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -23,6 +23,7 @@ cryostat.discovery.containers.request-timeout=2s cryostat.discovery.podman.enabled=false cryostat.discovery.docker.enabled=false cryostat.discovery.plugins.ping-period=5m +cryostat.discovery.plugins.max-failures=3 cryostat.discovery.plugins.jwt.secret.algorithm=AES cryostat.discovery.plugins.jwt.secret.keysize=256 cryostat.discovery.plugins.jwt.signature.algorithm=HS256 diff --git a/src/main/resources/db/migration/V4.2.0__cryostat.sql b/src/main/resources/db/migration/V4.2.0__cryostat.sql index 42abb96aa..0d26ee137 100644 --- a/src/main/resources/db/migration/V4.2.0__cryostat.sql +++ b/src/main/resources/db/migration/V4.2.0__cryostat.sql @@ -299,6 +299,8 @@ CREATE TABLE DiscoveryPlugin_AUD ( callback TEXT, credential_id BIGINT, builtin BOOLEAN, + consecutiveFailures INTEGER, + lastSuccessfulPing BIGINT, PRIMARY KEY (id, REV), FOREIGN KEY (REV) REFERENCES REVINFO (REV), FOREIGN KEY (REVEND) REFERENCES REVINFO (REV) @@ -639,4 +641,7 @@ CREATE INDEX IDX_ASYNCPROFILERRECORDING_AUD_REV ON AsyncProfilerRecording_AUD (R CREATE INDEX IDX_ASYNCPROFILERRECORDING_AUD_REVTYPE ON AsyncProfilerRecording_AUD (REVTYPE); CREATE INDEX IDX_ASYNCPROFILERRECORDING_AUD_REVEND ON AsyncProfilerRecording_AUD (REVEND); +ALTER TABLE DiscoveryPlugin ADD COLUMN consecutiveFailures INTEGER NOT NULL DEFAULT 0; +ALTER TABLE DiscoveryPlugin ADD COLUMN lastSuccessfulPing BIGINT; + COMMIT; From b6d79172869f0e40e72b7bf57c86574de97fba37 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Apr 2026 13:17:52 -0400 Subject: [PATCH 02/24] chore(discovery): refactor to extract PluginCallbackFactory --- .../java/io/cryostat/discovery/Discovery.java | 8 +++-- .../cryostat/discovery/DiscoveryPlugin.java | 3 +- .../discovery/PluginCallbackFactory.java | 34 +++++++++++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) create mode 100644 src/main/java/io/cryostat/discovery/PluginCallbackFactory.java diff --git a/src/main/java/io/cryostat/discovery/Discovery.java b/src/main/java/io/cryostat/discovery/Discovery.java index 56d7f56bd..a49f68597 100644 --- a/src/main/java/io/cryostat/discovery/Discovery.java +++ b/src/main/java/io/cryostat/discovery/Discovery.java @@ -39,7 +39,6 @@ import io.cryostat.ConfigProperties; import io.cryostat.credentials.Credential; -import io.cryostat.discovery.DiscoveryPlugin.PluginCallback; import io.cryostat.discovery.KubeEndpointSlicesDiscovery.KubeDiscoveryNodeType; import io.cryostat.discovery.NodeType.BaseNodeType; import io.cryostat.targets.TargetConnectionManager; @@ -133,6 +132,7 @@ public class Discovery { @Inject Scheduler scheduler; @Inject URIUtil uriUtil; @Inject KubeEndpointSlicesDiscovery k8sDiscovery; + @Inject PluginCallbackFactory callbackFactory; void onStart(@Observes StartupEvent evt) { QuarkusTransaction.requiringNew() @@ -360,7 +360,7 @@ public PluginRegistration register(@Context RoutingContext ctx, JsonObject body) .ifPresent( p -> { try { - var cb = PluginCallback.create(p); + var cb = callbackFactory.create(p); cb.ping(); throw new DuplicatePluginException( String.format( @@ -822,6 +822,8 @@ static class RefreshPluginJob implements Job { @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_MAX_FAILURES) int maxConsecutiveFailures; + @Inject PluginCallbackFactory callbackFactory; + @Override @Transactional public void execute(JobExecutionContext context) throws JobExecutionException { @@ -832,7 +834,7 @@ public void execute(JobExecutionContext context) throws JobExecutionException { DiscoveryPlugin.find( "id", context.getMergedJobDataMap().get(PLUGIN_ID_MAP_KEY)) .singleResult(); - var cb = PluginCallback.create(plugin); + var cb = callbackFactory.create(plugin); if (refresh) { cb.refresh(); logger.debugv( diff --git a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java index 3fc31c35a..840cb0b6b 100644 --- a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java +++ b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java @@ -121,6 +121,7 @@ public class DiscoveryPlugin extends PanacheEntityBase { static class Listener { @Inject Logger logger; + @Inject PluginCallbackFactory callbackFactory; @PrePersist @Transactional @@ -140,7 +141,7 @@ public void prePersist(DiscoveryPlugin plugin) { logger.debugv( "Testing discovery plugin callback: {0} @ {1}", plugin.realm.name, plugin.callback); - PluginCallback.create(plugin).ping(); + callbackFactory.create(plugin).ping(); logger.debugv( "Registered discovery plugin: {0} @ {1}", plugin.realm.name, plugin.callback); diff --git a/src/main/java/io/cryostat/discovery/PluginCallbackFactory.java b/src/main/java/io/cryostat/discovery/PluginCallbackFactory.java new file mode 100644 index 000000000..f49311e1d --- /dev/null +++ b/src/main/java/io/cryostat/discovery/PluginCallbackFactory.java @@ -0,0 +1,34 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.discovery; + +import java.net.URISyntaxException; + +import io.cryostat.discovery.DiscoveryPlugin.PluginCallback; + +import jakarta.enterprise.context.ApplicationScoped; + +/** + * Factory for creating PluginCallback instances. This abstraction allows tests to inject mock + * callbacks without making real network connections. + */ +@ApplicationScoped +public class PluginCallbackFactory { + + public PluginCallback create(DiscoveryPlugin plugin) throws URISyntaxException { + return PluginCallback.create(plugin); + } +} From 24b95c18325ccd8907ba8f955092a24ec5b10e9d Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Apr 2026 13:18:00 -0400 Subject: [PATCH 03/24] fixup! fix(discovery): tolerate temporary plugin discovery failures --- .../cryostat/discovery/InstantConverter.java | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 src/main/java/io/cryostat/discovery/InstantConverter.java diff --git a/src/main/java/io/cryostat/discovery/InstantConverter.java b/src/main/java/io/cryostat/discovery/InstantConverter.java new file mode 100644 index 000000000..2a017abba --- /dev/null +++ b/src/main/java/io/cryostat/discovery/InstantConverter.java @@ -0,0 +1,39 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.discovery; + +import java.time.Instant; + +import jakarta.persistence.AttributeConverter; + +public class InstantConverter implements AttributeConverter { + + @Override + public Long convertToDatabaseColumn(Instant attribute) { + if (attribute == null) { + return null; + } + return attribute.toEpochMilli(); + } + + @Override + public Instant convertToEntityAttribute(Long dbData) { + if (dbData == null) { + return null; + } + return Instant.ofEpochMilli(dbData); + } +} From 10efe5ad86fe2f8ef1e673b78d5b02ee1b6ce1dd Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Apr 2026 13:18:32 -0400 Subject: [PATCH 04/24] test(discovery): add tests for temporary plugin failures --- .../DiscoveryPluginGracePeriodTest.java | 388 ++++++++++++++++++ 1 file changed, 388 insertions(+) create mode 100644 src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java diff --git a/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java b/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java new file mode 100644 index 000000000..2010cef7e --- /dev/null +++ b/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java @@ -0,0 +1,388 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.discovery; + +import static io.restassured.RestAssured.given; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.net.URI; +import java.time.Instant; +import java.util.Map; +import java.util.UUID; + +import io.cryostat.AbstractTransactionalTestBase; +import io.cryostat.ConfigProperties; +import io.cryostat.discovery.DiscoveryPlugin.PluginCallback; + +import io.quarkus.narayana.jta.QuarkusTransaction; +import io.quarkus.test.InjectMock; +import io.quarkus.test.junit.QuarkusTest; +import io.restassured.http.ContentType; +import jakarta.inject.Inject; +import jakarta.transaction.Transactional; +import jakarta.ws.rs.ProcessingException; +import org.eclipse.microprofile.config.inject.ConfigProperty; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.quartz.JobDataMap; +import org.quartz.JobExecutionContext; + +@QuarkusTest +public class DiscoveryPluginGracePeriodTest extends AbstractTransactionalTestBase { + + @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_MAX_FAILURES) + int maxConsecutiveFailures; + + @Inject Discovery.RefreshPluginJob refreshPluginJob; + + @InjectMock PluginCallbackFactory callbackFactory; + + private static final String PLUGIN_ID_MAP_KEY = "pluginId"; + private static final String REFRESH_MAP_KEY = "refresh"; + + private PluginCallback mockCallback; + + @BeforeEach + public void setupMocks() throws Exception { + mockCallback = mock(PluginCallback.class); + Mockito.reset(callbackFactory); + // By default, return mockCallback for any plugin + when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); + } + + @Test + @Transactional + public void testConsecutiveFailuresIncrement() throws Exception { + // Create credentials first + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl == 'http://localhost:9999/nonexistent'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + // Create a plugin manually for testing + var realm = new DiscoveryNode(); + realm.name = "test_failure_realm"; + realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); + realm.persist(); + + var plugin = new DiscoveryPlugin(); + plugin.realm = realm; + plugin.callback = + URI.create( + String.format( + "http://storedcredentials:%d@localhost:9999/nonexistent", + credentialId)); + plugin.builtin = false; + plugin.consecutiveFailures = 0; + plugin.persist(); + + UUID pluginId = plugin.id; + + assertNotNull(plugin); + assertEquals(0, plugin.consecutiveFailures); + assertNull(plugin.lastSuccessfulPing); + + // Mock the callback to throw an exception simulating network failure + doThrow(new ProcessingException("Connection refused")).when(mockCallback).ping(); + when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); + + // Simulate ping failures + var context = mock(JobExecutionContext.class); + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, pluginId); + dataMap.put(REFRESH_MAP_KEY, false); + when(context.getMergedJobDataMap()).thenReturn(dataMap); + + // First failure + try { + refreshPluginJob.execute(context); + } catch (Exception e) { + // Expected to fail + } + + var updatedPlugin1 = DiscoveryPlugin.findById(pluginId); + assertEquals(1, updatedPlugin1.consecutiveFailures); + assertNull(updatedPlugin1.lastSuccessfulPing); + + // Second failure + try { + refreshPluginJob.execute(context); + } catch (Exception e) { + // Expected to fail + } + + var updatedPlugin2 = DiscoveryPlugin.findById(pluginId); + assertEquals(2, updatedPlugin2.consecutiveFailures); + assertNull(updatedPlugin2.lastSuccessfulPing); + } + + @Test + @Transactional + public void testPluginDeletedAfterMaxFailures() throws Exception { + // Create credentials first + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl == 'http://localhost:9999/nonexistent'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + // Create a plugin manually for testing + var realm = new DiscoveryNode(); + realm.name = "test_max_failures_realm"; + realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); + realm.persist(); + + var plugin = new DiscoveryPlugin(); + plugin.realm = realm; + plugin.callback = + URI.create( + String.format( + "http://storedcredentials:%d@localhost:9999/nonexistent", + credentialId)); + plugin.builtin = false; + plugin.consecutiveFailures = maxConsecutiveFailures - 1; + plugin.persist(); + + UUID pluginId = plugin.id; + + // Mock the callback to throw an exception simulating network failure + doThrow(new ProcessingException("Connection refused")).when(mockCallback).ping(); + when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); + + var context = mock(JobExecutionContext.class); + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, pluginId); + dataMap.put(REFRESH_MAP_KEY, false); + when(context.getMergedJobDataMap()).thenReturn(dataMap); + + // This failure should trigger deletion + try { + refreshPluginJob.execute(context); + } catch (Exception e) { + // Expected to fail + } + + var deletedPlugin = DiscoveryPlugin.findById(pluginId); + assertNull(deletedPlugin, "Plugin should be deleted after max consecutive failures"); + } + + @Test + public void testConsecutiveFailuresResetOnSuccess() throws Exception { + // For this test, use real callback creation since we're testing against real endpoint + doCallRealMethod().when(callbackFactory).create(any(DiscoveryPlugin.class)); + + // Create a plugin with a valid callback + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl ==" + + " 'http://localhost:8081/health/liveness'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + var callback = + String.format( + "http://storedcredentials:%d@localhost:8081/health/liveness", credentialId); + + var registration = + given().log() + .all() + .when() + .body(Map.of("realm", "test_reset_realm", "callback", callback)) + .contentType(ContentType.JSON) + .post("/api/v4/discovery") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(200) + .contentType(ContentType.JSON) + .extract() + .jsonPath(); + + var pluginId = UUID.fromString(registration.getString("id")); + + // Manually set some consecutive failures + QuarkusTransaction.requiringNew() + .run( + () -> { + var plugin = + DiscoveryPlugin.find("id", pluginId) + .firstResult(); + plugin.consecutiveFailures = 2; + plugin.persist(); + }); + + var context = mock(JobExecutionContext.class); + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, pluginId); + dataMap.put(REFRESH_MAP_KEY, false); + when(context.getMergedJobDataMap()).thenReturn(dataMap); + + // Execute successful ping + refreshPluginJob.execute(context); + + DiscoveryPlugin updatedPlugin = + QuarkusTransaction.requiringNew() + .call( + () -> + DiscoveryPlugin.find("id", pluginId) + .firstResult()); + + assertEquals(0, updatedPlugin.consecutiveFailures, "Consecutive failures should be reset"); + assertNotNull(updatedPlugin.lastSuccessfulPing, "Last successful ping should be set"); + assertTrue( + updatedPlugin.lastSuccessfulPing.isBefore(Instant.now().plusSeconds(1)), + "Last successful ping should be recent"); + } + + @Test + @Transactional + public void testPluginNotDeletedBeforeMaxFailures() throws Exception { + // Create credentials first + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl == 'http://localhost:9999/nonexistent'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + // Create a plugin manually for testing + var realm = new DiscoveryNode(); + realm.name = "test_not_deleted_realm"; + realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); + realm.persist(); + + var plugin = new DiscoveryPlugin(); + plugin.realm = realm; + plugin.callback = + URI.create( + String.format( + "http://storedcredentials:%d@localhost:9999/nonexistent", + credentialId)); + plugin.builtin = false; + plugin.consecutiveFailures = 0; + plugin.persist(); + + UUID pluginId = plugin.id; + + // Mock the callback to throw an exception simulating network failure + doThrow(new ProcessingException("Connection refused")).when(mockCallback).ping(); + when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); + + var context = mock(JobExecutionContext.class); + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, pluginId); + dataMap.put(REFRESH_MAP_KEY, false); + when(context.getMergedJobDataMap()).thenReturn(dataMap); + + // Execute failures up to max-1 + for (int i = 0; i < maxConsecutiveFailures - 1; i++) { + try { + refreshPluginJob.execute(context); + } catch (Exception e) { + // Expected to fail + } + } + + var updatedPlugin = DiscoveryPlugin.findById(pluginId); + + assertNotNull( + updatedPlugin, "Plugin should not be deleted before max consecutive failures"); + assertEquals( + maxConsecutiveFailures - 1, + updatedPlugin.consecutiveFailures, + "Consecutive failures should be tracked"); + } +} From 11665e388122ded98fc160f19492c6ed7342d3a5 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Apr 2026 13:42:20 -0400 Subject: [PATCH 05/24] chore(db): configure default datasource size and timeouts, add monitoring --- .../monitoring/ConnectionPoolMonitor.java | 98 +++++++++++ src/main/resources/application.properties | 10 ++ .../monitoring/ConnectionPoolMonitorTest.java | 157 ++++++++++++++++++ 3 files changed, 265 insertions(+) create mode 100644 src/main/java/io/cryostat/monitoring/ConnectionPoolMonitor.java create mode 100644 src/test/java/io/cryostat/monitoring/ConnectionPoolMonitorTest.java diff --git a/src/main/java/io/cryostat/monitoring/ConnectionPoolMonitor.java b/src/main/java/io/cryostat/monitoring/ConnectionPoolMonitor.java new file mode 100644 index 000000000..8729370f2 --- /dev/null +++ b/src/main/java/io/cryostat/monitoring/ConnectionPoolMonitor.java @@ -0,0 +1,98 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.monitoring; + +import io.agroal.api.AgroalDataSource; +import io.quarkus.scheduler.Scheduled; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.jboss.logging.Logger; + +/** + * Monitors database connection pool health and logs metrics periodically. + * + *

This monitor tracks active, available, waiting, max used, and leak detection counts to help + * identify connection pool exhaustion and potential leaks. Part of Risk 10 Phase 2 mitigation. + */ +@ApplicationScoped +public class ConnectionPoolMonitor { + + @Inject AgroalDataSource dataSource; + @Inject Logger log; + + /** + * Logs connection pool statistics every 30 seconds. + * + *

Logs at debug level for normal operation. Logs at warn level with full metrics if + * connections are waiting (possible exhaustion) or leaks are detected. + */ + @Scheduled(every = "30s") + void logPoolStats() { + try { + var metrics = dataSource.getMetrics(); + + long active = metrics.activeCount(); + long available = metrics.availableCount(); + long waiting = metrics.awaitingCount(); + long maxUsed = metrics.maxUsedCount(); + long leakDetection = metrics.leakDetectionCount(); + + boolean hasIssues = waiting > 0 || leakDetection > 0; + + if (hasIssues) { + log.warnf( + "Connection pool issue detected: active=%d, available=%d, waiting=%d," + + " maxUsed=%d, leakDetection=%d", + active, available, waiting, maxUsed, leakDetection); + } else { + log.debugf( + "Connection pool: active=%d, available=%d, waiting=%d, maxUsed=%d," + + " leakDetection=%d", + active, available, waiting, maxUsed, leakDetection); + } + } catch (Exception e) { + log.errorf(e, "Failed to retrieve connection pool metrics"); + } + } + + /** + * Gets the current number of active connections. + * + * @return the number of active connections + */ + public long getActiveCount() { + try { + return dataSource.getMetrics().activeCount(); + } catch (Exception e) { + log.errorf(e, "Failed to retrieve active connection count"); + return -1; + } + } + + /** + * Gets the current number of connections waiting for availability. + * + * @return the number of waiting connections + */ + public long getAwaitingCount() { + try { + return dataSource.getMetrics().awaitingCount(); + } catch (Exception e) { + log.errorf(e, "Failed to retrieve awaiting connection count"); + return -1; + } + } +} diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index cb17b8675..29df73d53 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -3,6 +3,16 @@ quarkus.flyway.baseline-at-start=true quarkus.flyway.baseline-on-migrate=true quarkus.flyway.validate-migration-naming=true +quarkus.datasource.metrics.enabled=true +quarkus.datasource.jdbc.min-size=10 +quarkus.datasource.jdbc.max-size=50 +quarkus.datasource.jdbc.acquisition-timeout=PT10S +quarkus.datasource.jdbc.idle-removal-interval=PT5M +quarkus.datasource.jdbc.max-lifetime=PT30M +quarkus.datasource.jdbc.leak-detection-interval=PT2M + +quarkus.transaction-manager.default-transaction-timeout=PT30S + quarkus.hibernate-envers.audit-strategy=org.hibernate.envers.strategy.ValidityAuditStrategy quarkus.quartz.store-type=jdbc-cmt diff --git a/src/test/java/io/cryostat/monitoring/ConnectionPoolMonitorTest.java b/src/test/java/io/cryostat/monitoring/ConnectionPoolMonitorTest.java new file mode 100644 index 000000000..0f0969584 --- /dev/null +++ b/src/test/java/io/cryostat/monitoring/ConnectionPoolMonitorTest.java @@ -0,0 +1,157 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.monitoring; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import io.agroal.api.AgroalDataSource; +import io.agroal.api.AgroalDataSourceMetrics; +import org.jboss.logging.Logger; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class ConnectionPoolMonitorTest { + + ConnectionPoolMonitor monitor; + + @Mock AgroalDataSource dataSource; + @Mock Logger logger; + + @BeforeEach + void setup() { + monitor = new ConnectionPoolMonitor(); + monitor.dataSource = dataSource; + monitor.log = logger; + } + + @Test + void testLogPoolStatsWithNoIssues() { + AgroalDataSourceMetrics metrics = mock(AgroalDataSourceMetrics.class); + when(dataSource.getMetrics()).thenReturn(metrics); + when(metrics.activeCount()).thenReturn(5L); + when(metrics.availableCount()).thenReturn(15L); + when(metrics.awaitingCount()).thenReturn(0L); + when(metrics.maxUsedCount()).thenReturn(8L); + when(metrics.leakDetectionCount()).thenReturn(0L); + + monitor.logPoolStats(); + + verify(logger) + .debugf( + "Connection pool: active=%d, available=%d, waiting=%d, maxUsed=%d," + + " leakDetection=%d", + 5L, 15L, 0L, 8L, 0L); + } + + @Test + void testLogPoolStatsWithWaiting() { + AgroalDataSourceMetrics metrics = mock(AgroalDataSourceMetrics.class); + when(dataSource.getMetrics()).thenReturn(metrics); + when(metrics.activeCount()).thenReturn(10L); + when(metrics.availableCount()).thenReturn(0L); + when(metrics.awaitingCount()).thenReturn(3L); + when(metrics.maxUsedCount()).thenReturn(10L); + when(metrics.leakDetectionCount()).thenReturn(0L); + + monitor.logPoolStats(); + + verify(logger) + .warnf( + "Connection pool issue detected: active=%d, available=%d, waiting=%d," + + " maxUsed=%d, leakDetection=%d", + 10L, 0L, 3L, 10L, 0L); + } + + @Test + void testLogPoolStatsWithLeakDetection() { + AgroalDataSourceMetrics metrics = mock(AgroalDataSourceMetrics.class); + when(dataSource.getMetrics()).thenReturn(metrics); + when(metrics.activeCount()).thenReturn(8L); + when(metrics.availableCount()).thenReturn(12L); + when(metrics.awaitingCount()).thenReturn(0L); + when(metrics.maxUsedCount()).thenReturn(10L); + when(metrics.leakDetectionCount()).thenReturn(2L); + + monitor.logPoolStats(); + + verify(logger) + .warnf( + "Connection pool issue detected: active=%d, available=%d, waiting=%d," + + " maxUsed=%d, leakDetection=%d", + 8L, 12L, 0L, 10L, 2L); + } + + @Test + void testLogPoolStatsWithBothIssues() { + AgroalDataSourceMetrics metrics = mock(AgroalDataSourceMetrics.class); + when(dataSource.getMetrics()).thenReturn(metrics); + when(metrics.activeCount()).thenReturn(10L); + when(metrics.availableCount()).thenReturn(0L); + when(metrics.awaitingCount()).thenReturn(5L); + when(metrics.maxUsedCount()).thenReturn(10L); + when(metrics.leakDetectionCount()).thenReturn(3L); + + monitor.logPoolStats(); + + verify(logger) + .warnf( + "Connection pool issue detected: active=%d, available=%d, waiting=%d," + + " maxUsed=%d, leakDetection=%d", + 10L, 0L, 5L, 10L, 3L); + } + + @Test + void testLogPoolStatsHandlesException() { + when(dataSource.getMetrics()).thenThrow(new RuntimeException("Test exception")); + + monitor.logPoolStats(); + + verify(logger) + .errorf( + org.mockito.ArgumentMatchers.any(Exception.class), + org.mockito.ArgumentMatchers.eq( + "Failed to retrieve connection pool metrics")); + } + + @Test + void testGetActiveCount() { + AgroalDataSourceMetrics metrics = mock(AgroalDataSourceMetrics.class); + when(dataSource.getMetrics()).thenReturn(metrics); + when(metrics.activeCount()).thenReturn(7L); + + long result = monitor.getActiveCount(); + + Assertions.assertEquals(7L, result); + } + + @Test + void testGetAwaitingCount() { + AgroalDataSourceMetrics metrics = mock(AgroalDataSourceMetrics.class); + when(dataSource.getMetrics()).thenReturn(metrics); + when(metrics.awaitingCount()).thenReturn(2L); + + long result = monitor.getAwaitingCount(); + + Assertions.assertEquals(2L, result); + } +} From 6d97c507737951ffb1256d42d6d78e05c339f2a4 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Apr 2026 16:40:12 -0400 Subject: [PATCH 06/24] fix(discovery): exponential backoff of failed ping, improve failed ping cleanup --- .../java/io/cryostat/ConfigProperties.java | 4 + .../java/io/cryostat/discovery/Discovery.java | 147 ++++-- .../discovery/DiscoveryJwtFactory.java | 4 +- .../cryostat/discovery/DiscoveryPlugin.java | 19 + src/main/resources/application.properties | 1 + .../db/migration/V4.2.0__cryostat.sql | 6 + .../DiscoveryPluginGracePeriodTest.java | 433 +++++++++++++++--- 7 files changed, 510 insertions(+), 104 deletions(-) diff --git a/src/main/java/io/cryostat/ConfigProperties.java b/src/main/java/io/cryostat/ConfigProperties.java index 9916498e0..8c3a59b47 100644 --- a/src/main/java/io/cryostat/ConfigProperties.java +++ b/src/main/java/io/cryostat/ConfigProperties.java @@ -52,6 +52,10 @@ public class ConfigProperties { "cryostat.discovery.containers.request-timeout"; public static final String DISCOVERY_PLUGINS_MAX_FAILURES = "cryostat.discovery.plugins.max-failures"; + public static final String DISCOVERY_PLUGINS_PING_PERIOD = + "cryostat.discovery.plugins.ping-period"; + public static final String DISCOVERY_PLUGINS_MAX_BACKOFF_MULTIPLIER = + "cryostat.discovery.plugins.max-backoff-multiplier"; public static final String CONNECTIONS_TTL = "cryostat.connections.ttl"; public static final String CONNECTIONS_FAILED_BACKOFF = "cryostat.connections.failed-backoff"; diff --git a/src/main/java/io/cryostat/discovery/Discovery.java b/src/main/java/io/cryostat/discovery/Discovery.java index a49f68597..e93ab78f4 100644 --- a/src/main/java/io/cryostat/discovery/Discovery.java +++ b/src/main/java/io/cryostat/discovery/Discovery.java @@ -60,6 +60,7 @@ import jakarta.annotation.security.RolesAllowed; import jakarta.enterprise.event.Observes; import jakarta.inject.Inject; +import jakarta.persistence.EntityManager; import jakarta.persistence.NoResultException; import jakarta.transaction.Transactional; import jakarta.ws.rs.BadRequestException; @@ -114,7 +115,7 @@ public class Discovery { private static final String PLUGIN_ID_MAP_KEY = "pluginId"; private static final String REFRESH_MAP_KEY = "refresh"; - @ConfigProperty(name = "cryostat.discovery.plugins.ping-period") + @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_PING_PERIOD) Duration discoveryPingPeriod; @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_MAX_FAILURES) @@ -814,7 +815,6 @@ private static record NodeContext( * pings plugins to ensure they are still alive/reachable and to prompt them to request a fresh * token if their token will be expiring soon. */ - @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE") @DisallowConcurrentExecution static class RefreshPluginJob implements Job { @Inject Logger logger; @@ -822,32 +822,65 @@ static class RefreshPluginJob implements Job { @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_MAX_FAILURES) int maxConsecutiveFailures; + @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_PING_PERIOD) + Duration basePingPeriod; + + @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_MAX_BACKOFF_MULTIPLIER) + int maxBackoffMultiplier; + @Inject PluginCallbackFactory callbackFactory; + @Inject EntityManager entityManager; + @Override - @Transactional public void execute(JobExecutionContext context) throws JobExecutionException { - DiscoveryPlugin plugin = null; - try { - boolean refresh = context.getMergedJobDataMap().getBoolean(REFRESH_MAP_KEY); - plugin = - DiscoveryPlugin.find( - "id", context.getMergedJobDataMap().get(PLUGIN_ID_MAP_KEY)) - .singleResult(); - var cb = callbackFactory.create(plugin); - if (refresh) { - cb.refresh(); - logger.debugv( - "Refreshed discovery plugin: {0} @ {1}", plugin.realm, plugin.callback); - } else { - cb.ping(); - logger.debugv( - "Retained discovery plugin: {0} @ {1}", plugin.realm, plugin.callback); - } + boolean refresh = context.getMergedJobDataMap().getBoolean(REFRESH_MAP_KEY); + UUID pluginId = (UUID) context.getMergedJobDataMap().get(PLUGIN_ID_MAP_KEY); - plugin.consecutiveFailures = 0; - plugin.lastSuccessfulPing = Instant.now(); - plugin.persist(); + try { + QuarkusTransaction.requiringNew() + .run( + () -> { + try { + var p = DiscoveryPlugin.findById(pluginId); + + if (p == null) { + throw new NoResultException( + "Plugin not found: " + pluginId); + } + + if (p.nextPingAt != null + && Instant.now().isBefore(p.nextPingAt)) { + logger.debugv( + "Skipping ping due to backoff: {0} @ {1}", + p.realm.name, p.callback); + return; + } + + var cb = callbackFactory.create(p); + if (refresh) { + cb.refresh(); + logger.debugv( + "Refreshed discovery plugin: {0} @ {1}", + p.realm.name, p.callback); + } else { + cb.ping(); + logger.debugv( + "Retained discovery plugin: {0} @ {1}", + p.realm.name, p.callback); + } + + p.consecutiveFailures = 0; + p.lastSuccessfulPing = Instant.now(); + p.backoffMultiplier = 1; + p.nextPingAt = null; + p.persist(); + } catch (NoResultException e) { + throw e; + } catch (Exception e) { + throw new RuntimeException(e); + } + }); } catch (NoResultException e) { logger.debugv( e, @@ -857,24 +890,56 @@ public void execute(JobExecutionContext context) throws JobExecutionException { ex.setUnscheduleFiringTrigger(true); throw ex; } catch (Exception e) { - if (plugin != null) { - plugin.consecutiveFailures++; - plugin.persist(); - - if (plugin.consecutiveFailures >= maxConsecutiveFailures) { - logger.warnv( - "Pruning discovery plugin after {0} consecutive failures: {1} @" - + " {2}", - plugin.consecutiveFailures, plugin.realm.name, plugin.callback); - plugin.delete(); - } else { - logger.debugv( - "Discovery plugin ping failed ({0}/{1}): {2} @ {3}", - plugin.consecutiveFailures, - maxConsecutiveFailures, - plugin.realm.name, - plugin.callback); - } + // Unwrap RuntimeException wrappers to get the actual cause + Throwable cause = e; + while (cause instanceof RuntimeException && cause.getCause() != null) { + cause = cause.getCause(); + } + + if (pluginId != null) { + final Throwable finalCause = cause; + QuarkusTransaction.requiringNew() + .run( + () -> { + var p = DiscoveryPlugin.findById(pluginId); + if (p != null) { + p.consecutiveFailures++; + p.lastFailedPing = Instant.now(); + p.backoffMultiplier = + Math.min( + p.backoffMultiplier * 2, + maxBackoffMultiplier); + Duration backoffPeriod = + basePingPeriod.multipliedBy( + p.backoffMultiplier); + p.nextPingAt = Instant.now().plus(backoffPeriod); + p.persist(); + + if (p.consecutiveFailures >= maxConsecutiveFailures) { + logger.warnv( + "Pruning discovery plugin after {0}" + + " consecutive failures: {1} @" + + " {2}", + p.consecutiveFailures, + p.realm.name, + p.callback); + p.delete(); + } else { + logger.warnv( + finalCause, + "Plugin ping failed ({0}/{1}), backing off" + + " for {2}: {3} @ {4}", + p.consecutiveFailures, + maxConsecutiveFailures, + backoffPeriod, + p.realm.name, + p.callback); + } + } + }); + + var ex = new JobExecutionException(e); + throw ex; } else { var ex = new JobExecutionException(e); ex.setUnscheduleFiringTrigger(true); diff --git a/src/main/java/io/cryostat/discovery/DiscoveryJwtFactory.java b/src/main/java/io/cryostat/discovery/DiscoveryJwtFactory.java index 05cea1afd..de4edf60f 100644 --- a/src/main/java/io/cryostat/discovery/DiscoveryJwtFactory.java +++ b/src/main/java/io/cryostat/discovery/DiscoveryJwtFactory.java @@ -35,6 +35,8 @@ import javax.crypto.KeyGenerator; import javax.crypto.SecretKey; +import io.cryostat.ConfigProperties; + import com.nimbusds.jose.EncryptionMethod; import com.nimbusds.jose.JOSEException; import com.nimbusds.jose.JWEAlgorithm; @@ -71,7 +73,7 @@ public class DiscoveryJwtFactory { static final String DISCOVERY_V4_API_PATH = "/api/v4/discovery/"; static final String DISCOVERY_V4_2_API_PATH = "/api/v4.2/discovery/"; - @ConfigProperty(name = "cryostat.discovery.plugins.ping-period") + @ConfigProperty(name = ConfigProperties.DISCOVERY_PLUGINS_PING_PERIOD) Duration discoveryPingPeriod; @ConfigProperty(name = "cryostat.discovery.plugins.jwt.signature.algorithm") diff --git a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java index 840cb0b6b..90cce1548 100644 --- a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java +++ b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java @@ -117,6 +117,17 @@ public class DiscoveryPlugin extends PanacheEntityBase { @Convert(converter = InstantConverter.class) public Instant lastSuccessfulPing; + @Column(nullable = true) + @Convert(converter = InstantConverter.class) + public Instant lastFailedPing; + + @Column(nullable = false) + public int backoffMultiplier = 1; + + @Column(nullable = true) + @Convert(converter = InstantConverter.class) + public Instant nextPingAt; + @ApplicationScoped static class Listener { @@ -137,6 +148,14 @@ public void prePersist(DiscoveryPlugin plugin) { plugin.credential = credential; plugin.callback = UriBuilder.fromUri(plugin.callback).userInfo(null).build(); } + if (plugin.nextPingAt != null + || plugin.lastFailedPing != null + || plugin.lastSuccessfulPing != null) { + logger.debugv( + "Skipping prePersist ping for plugin with existing state: {0} @ {1}", + plugin.realm.name, plugin.callback); + return; + } try { logger.debugv( "Testing discovery plugin callback: {0} @ {1}", diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 29df73d53..bcce8a9ca 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -34,6 +34,7 @@ cryostat.discovery.podman.enabled=false cryostat.discovery.docker.enabled=false cryostat.discovery.plugins.ping-period=5m cryostat.discovery.plugins.max-failures=3 +cryostat.discovery.plugins.max-backoff-multiplier=8 cryostat.discovery.plugins.jwt.secret.algorithm=AES cryostat.discovery.plugins.jwt.secret.keysize=256 cryostat.discovery.plugins.jwt.signature.algorithm=HS256 diff --git a/src/main/resources/db/migration/V4.2.0__cryostat.sql b/src/main/resources/db/migration/V4.2.0__cryostat.sql index 0d26ee137..6701f9e35 100644 --- a/src/main/resources/db/migration/V4.2.0__cryostat.sql +++ b/src/main/resources/db/migration/V4.2.0__cryostat.sql @@ -301,6 +301,9 @@ CREATE TABLE DiscoveryPlugin_AUD ( builtin BOOLEAN, consecutiveFailures INTEGER, lastSuccessfulPing BIGINT, + lastFailedPing BIGINT, + backoffMultiplier INTEGER, + nextPingAt BIGINT, PRIMARY KEY (id, REV), FOREIGN KEY (REV) REFERENCES REVINFO (REV), FOREIGN KEY (REVEND) REFERENCES REVINFO (REV) @@ -643,5 +646,8 @@ CREATE INDEX IDX_ASYNCPROFILERRECORDING_AUD_REVEND ON AsyncProfilerRecording_AUD ALTER TABLE DiscoveryPlugin ADD COLUMN consecutiveFailures INTEGER NOT NULL DEFAULT 0; ALTER TABLE DiscoveryPlugin ADD COLUMN lastSuccessfulPing BIGINT; +ALTER TABLE DiscoveryPlugin ADD COLUMN lastFailedPing BIGINT; +ALTER TABLE DiscoveryPlugin ADD COLUMN backoffMultiplier INTEGER NOT NULL DEFAULT 1; +ALTER TABLE DiscoveryPlugin ADD COLUMN nextPingAt BIGINT; COMMIT; diff --git a/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java b/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java index 2010cef7e..a1b5a6829 100644 --- a/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java +++ b/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java @@ -33,7 +33,6 @@ import io.quarkus.test.junit.QuarkusTest; import io.restassured.http.ContentType; import jakarta.inject.Inject; -import jakarta.transaction.Transactional; import jakarta.ws.rs.ProcessingException; import org.eclipse.microprofile.config.inject.ConfigProperty; import org.junit.jupiter.api.BeforeEach; @@ -61,12 +60,35 @@ public class DiscoveryPluginGracePeriodTest extends AbstractTransactionalTestBas public void setupMocks() throws Exception { mockCallback = mock(PluginCallback.class); Mockito.reset(callbackFactory); - // By default, return mockCallback for any plugin when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); } + private UUID createPluginInCommittedTransaction( + long credentialId, String realmName, int consecutiveFailures) { + return QuarkusTransaction.requiringNew() + .call( + () -> { + var realm = new DiscoveryNode(); + realm.name = realmName; + realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); + realm.persist(); + + var plugin = new DiscoveryPlugin(); + plugin.realm = realm; + plugin.callback = + URI.create( + String.format( + "http://storedcredentials:%d@localhost:9999/nonexistent", + credentialId)); + plugin.builtin = false; + plugin.consecutiveFailures = consecutiveFailures; + plugin.persist(); + + return plugin.id; + }); + } + @Test - @Transactional public void testConsecutiveFailuresIncrement() throws Exception { // Create credentials first var credentialId = @@ -94,30 +116,14 @@ public void testConsecutiveFailuresIncrement() throws Exception { .jsonPath() .getLong("id"); - // Create a plugin manually for testing - var realm = new DiscoveryNode(); - realm.name = "test_failure_realm"; - realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); - realm.persist(); - - var plugin = new DiscoveryPlugin(); - plugin.realm = realm; - plugin.callback = - URI.create( - String.format( - "http://storedcredentials:%d@localhost:9999/nonexistent", - credentialId)); - plugin.builtin = false; - plugin.consecutiveFailures = 0; - plugin.persist(); - - UUID pluginId = plugin.id; + // Create a plugin in a committed transaction so the job can see it + UUID pluginId = createPluginInCommittedTransaction(credentialId, "test_failure_realm", 0); + var plugin = DiscoveryPlugin.findById(pluginId); assertNotNull(plugin); assertEquals(0, plugin.consecutiveFailures); assertNull(plugin.lastSuccessfulPing); - // Mock the callback to throw an exception simulating network failure doThrow(new ProcessingException("Connection refused")).when(mockCallback).ping(); when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); @@ -135,10 +141,23 @@ public void testConsecutiveFailuresIncrement() throws Exception { // Expected to fail } - var updatedPlugin1 = DiscoveryPlugin.findById(pluginId); + var updatedPlugin1 = + QuarkusTransaction.requiringNew() + .call(() -> DiscoveryPlugin.findById(pluginId)); assertEquals(1, updatedPlugin1.consecutiveFailures); assertNull(updatedPlugin1.lastSuccessfulPing); + // Clear backoff to allow second ping attempt + QuarkusTransaction.requiringNew() + .run( + () -> { + var p = DiscoveryPlugin.findById(pluginId); + if (p != null) { + p.nextPingAt = null; + p.persist(); + } + }); + // Second failure try { refreshPluginJob.execute(context); @@ -146,13 +165,14 @@ public void testConsecutiveFailuresIncrement() throws Exception { // Expected to fail } - var updatedPlugin2 = DiscoveryPlugin.findById(pluginId); + var updatedPlugin2 = + QuarkusTransaction.requiringNew() + .call(() -> DiscoveryPlugin.findById(pluginId)); assertEquals(2, updatedPlugin2.consecutiveFailures); assertNull(updatedPlugin2.lastSuccessfulPing); } @Test - @Transactional public void testPluginDeletedAfterMaxFailures() throws Exception { // Create credentials first var credentialId = @@ -180,24 +200,10 @@ public void testPluginDeletedAfterMaxFailures() throws Exception { .jsonPath() .getLong("id"); - // Create a plugin manually for testing - var realm = new DiscoveryNode(); - realm.name = "test_max_failures_realm"; - realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); - realm.persist(); - - var plugin = new DiscoveryPlugin(); - plugin.realm = realm; - plugin.callback = - URI.create( - String.format( - "http://storedcredentials:%d@localhost:9999/nonexistent", - credentialId)); - plugin.builtin = false; - plugin.consecutiveFailures = maxConsecutiveFailures - 1; - plugin.persist(); - - UUID pluginId = plugin.id; + // Create a plugin in a committed transaction + UUID pluginId = + createPluginInCommittedTransaction( + credentialId, "test_max_failures_realm", maxConsecutiveFailures - 1); // Mock the callback to throw an exception simulating network failure doThrow(new ProcessingException("Connection refused")).when(mockCallback).ping(); @@ -310,7 +316,6 @@ public void testConsecutiveFailuresResetOnSuccess() throws Exception { } @Test - @Transactional public void testPluginNotDeletedBeforeMaxFailures() throws Exception { // Create credentials first var credentialId = @@ -338,24 +343,9 @@ public void testPluginNotDeletedBeforeMaxFailures() throws Exception { .jsonPath() .getLong("id"); - // Create a plugin manually for testing - var realm = new DiscoveryNode(); - realm.name = "test_not_deleted_realm"; - realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); - realm.persist(); - - var plugin = new DiscoveryPlugin(); - plugin.realm = realm; - plugin.callback = - URI.create( - String.format( - "http://storedcredentials:%d@localhost:9999/nonexistent", - credentialId)); - plugin.builtin = false; - plugin.consecutiveFailures = 0; - plugin.persist(); - - UUID pluginId = plugin.id; + // Create a plugin in a committed transaction + UUID pluginId = + createPluginInCommittedTransaction(credentialId, "test_not_deleted_realm", 0); // Mock the callback to throw an exception simulating network failure doThrow(new ProcessingException("Connection refused")).when(mockCallback).ping(); @@ -374,6 +364,19 @@ public void testPluginNotDeletedBeforeMaxFailures() throws Exception { } catch (Exception e) { // Expected to fail } + + // Clear backoff to allow next ping attempt + if (i < maxConsecutiveFailures - 2) { + QuarkusTransaction.requiringNew() + .run( + () -> { + var p = DiscoveryPlugin.findById(pluginId); + if (p != null) { + p.nextPingAt = null; + p.persist(); + } + }); + } } var updatedPlugin = DiscoveryPlugin.findById(pluginId); @@ -385,4 +388,310 @@ public void testPluginNotDeletedBeforeMaxFailures() throws Exception { updatedPlugin.consecutiveFailures, "Consecutive failures should be tracked"); } + + @Test + public void testLastFailedPingTracked() throws Exception { + // Create credentials first + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl == 'http://localhost:9999/nonexistent'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + // Create a plugin in a committed transaction + UUID pluginId = + createPluginInCommittedTransaction(credentialId, "test_failure_tracking_realm", 0); + + // Mock the callback to throw an exception simulating network failure + doThrow(new ProcessingException("Connection refused")).when(mockCallback).ping(); + when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); + + var context = mock(JobExecutionContext.class); + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, pluginId); + dataMap.put(REFRESH_MAP_KEY, false); + when(context.getMergedJobDataMap()).thenReturn(dataMap); + + // Execute failure + try { + refreshPluginJob.execute(context); + } catch (Exception e) { + // Expected to fail + } + + var updatedPlugin = DiscoveryPlugin.findById(pluginId); + assertNotNull(updatedPlugin.lastFailedPing, "Last failed ping should be set"); + assertTrue( + updatedPlugin.lastFailedPing.isBefore(Instant.now().plusSeconds(1)), + "Last failed ping should be recent"); + } + + @Test + public void testBackoffMultiplierIncreasesOnFailure() throws Exception { + // Create credentials first + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl == 'http://localhost:9999/nonexistent'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + // Create a plugin in a committed transaction + UUID pluginId = createPluginInCommittedTransaction(credentialId, "test_backoff_realm", 0); + + var plugin = DiscoveryPlugin.findById(pluginId); + assertEquals(1, plugin.backoffMultiplier); + assertNull(plugin.nextPingAt); + + // Mock the callback to throw an exception simulating network failure + doThrow(new ProcessingException("Connection refused")).when(mockCallback).ping(); + when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); + + var context = mock(JobExecutionContext.class); + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, pluginId); + dataMap.put(REFRESH_MAP_KEY, false); + when(context.getMergedJobDataMap()).thenReturn(dataMap); + + // First failure - backoff should double to 2 + try { + refreshPluginJob.execute(context); + } catch (Exception e) { + // Expected to fail + } + + var updatedPlugin1 = + QuarkusTransaction.requiringNew() + .call(() -> DiscoveryPlugin.findById(pluginId)); + assertEquals(2, updatedPlugin1.backoffMultiplier, "Backoff multiplier should double"); + assertNotNull(updatedPlugin1.nextPingAt, "Next ping time should be set"); + + // Clear backoff to allow second ping attempt + QuarkusTransaction.requiringNew() + .run( + () -> { + var p = DiscoveryPlugin.findById(pluginId); + if (p != null) { + p.nextPingAt = null; + p.persist(); + } + }); + + // Second failure - backoff should double to 4 + try { + refreshPluginJob.execute(context); + } catch (Exception e) { + // Expected to fail + } + + var updatedPlugin2 = + QuarkusTransaction.requiringNew() + .call(() -> DiscoveryPlugin.findById(pluginId)); + assertEquals(4, updatedPlugin2.backoffMultiplier, "Backoff multiplier should double again"); + } + + @Test + public void testBackoffSkipsPingWhenNotReady() throws Exception { + // Create credentials first + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl == 'http://localhost:9999/nonexistent'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + // Create a plugin in a committed transaction with backoff state + UUID pluginId = + QuarkusTransaction.requiringNew() + .call( + () -> { + var realm = new DiscoveryNode(); + realm.name = "test_backoff_skip_realm"; + realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); + realm.persist(); + + var plugin = new DiscoveryPlugin(); + plugin.realm = realm; + plugin.callback = + URI.create( + String.format( + "http://storedcredentials:%d@localhost:9999/nonexistent", + credentialId)); + plugin.builtin = false; + plugin.consecutiveFailures = 1; + plugin.backoffMultiplier = 2; + plugin.nextPingAt = + Instant.now().plusSeconds(3600); // 1 hour in future + plugin.persist(); + + return plugin.id; + }); + + // Mock the callback - it should NOT be called due to backoff + doThrow(new ProcessingException("Should not be called")).when(mockCallback).ping(); + when(callbackFactory.create(any(DiscoveryPlugin.class))).thenReturn(mockCallback); + + var context = mock(JobExecutionContext.class); + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, pluginId); + dataMap.put(REFRESH_MAP_KEY, false); + when(context.getMergedJobDataMap()).thenReturn(dataMap); + + // Execute - should skip ping due to backoff + refreshPluginJob.execute(context); + + // Verify ping was never called + verify(mockCallback, never()).ping(); + + var updatedPlugin = DiscoveryPlugin.findById(pluginId); + assertEquals( + 1, + updatedPlugin.consecutiveFailures, + "Consecutive failures should not change when ping is skipped"); + } + + @Test + public void testBackoffResetsOnSuccess() throws Exception { + // For this test, use real callback creation since we're testing against real endpoint + doCallRealMethod().when(callbackFactory).create(any(DiscoveryPlugin.class)); + + // Create a plugin with a valid callback + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl ==" + + " 'http://localhost:8081/health/liveness'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + var callback = + String.format( + "http://storedcredentials:%d@localhost:8081/health/liveness", credentialId); + + var registration = + given().log() + .all() + .when() + .body(Map.of("realm", "test_backoff_reset_realm", "callback", callback)) + .contentType(ContentType.JSON) + .post("/api/v4/discovery") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(200) + .contentType(ContentType.JSON) + .extract() + .jsonPath(); + + var pluginId = UUID.fromString(registration.getString("id")); + + // Manually set backoff state + QuarkusTransaction.requiringNew() + .run( + () -> { + var plugin = + DiscoveryPlugin.find("id", pluginId) + .firstResult(); + plugin.consecutiveFailures = 2; + plugin.backoffMultiplier = 4; + plugin.nextPingAt = Instant.now().minusSeconds(1); // In the past + plugin.persist(); + }); + + var context = mock(JobExecutionContext.class); + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, pluginId); + dataMap.put(REFRESH_MAP_KEY, false); + when(context.getMergedJobDataMap()).thenReturn(dataMap); + + // Execute successful ping + refreshPluginJob.execute(context); + + DiscoveryPlugin updatedPlugin = + QuarkusTransaction.requiringNew() + .call( + () -> + DiscoveryPlugin.find("id", pluginId) + .firstResult()); + + assertEquals(0, updatedPlugin.consecutiveFailures, "Consecutive failures should be reset"); + assertEquals(1, updatedPlugin.backoffMultiplier, "Backoff multiplier should be reset"); + assertNull(updatedPlugin.nextPingAt, "Next ping time should be cleared"); + assertNotNull(updatedPlugin.lastSuccessfulPing, "Last successful ping should be set"); + } } From 612bc681e1173098f5d6c22032a6efbaf3b4e392 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Apr 2026 16:50:43 -0400 Subject: [PATCH 07/24] fix(discovery): improve handling of stale plugins in deletion --- .../java/io/cryostat/discovery/Discovery.java | 93 ++++++++++++++----- 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/src/main/java/io/cryostat/discovery/Discovery.java b/src/main/java/io/cryostat/discovery/Discovery.java index e93ab78f4..0a02a0b9d 100644 --- a/src/main/java/io/cryostat/discovery/Discovery.java +++ b/src/main/java/io/cryostat/discovery/Discovery.java @@ -62,6 +62,7 @@ import jakarta.inject.Inject; import jakarta.persistence.EntityManager; import jakarta.persistence.NoResultException; +import jakarta.persistence.PersistenceException; import jakarta.transaction.Transactional; import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.Consumes; @@ -83,6 +84,7 @@ import org.eclipse.microprofile.faulttolerance.Bulkhead; import org.eclipse.microprofile.openapi.annotations.Operation; import org.eclipse.microprofile.openapi.annotations.tags.Tag; +import org.hibernate.exception.ConstraintViolationException; import org.jboss.logging.Logger; import org.jboss.resteasy.reactive.RestForm; import org.jboss.resteasy.reactive.RestHeader; @@ -371,31 +373,80 @@ public PluginRegistration register(@Context RoutingContext ctx, JsonObject body) } catch (Exception e) { if (!(e instanceof DuplicatePluginException)) { logger.debug(e); - QuarkusTransaction.joiningExisting().run(p::delete); + UUID oldPluginId = p.id; + QuarkusTransaction.joiningExisting() + .run( + () -> { + try { + var existing = + DiscoveryPlugin + . + findById( + oldPluginId); + if (existing != null) { + existing.delete(); + logger.debugv( + "Deleted unreachable" + + " plugin: {0}", + oldPluginId); + } else { + logger.debugv( + "Plugin already deleted" + + " (concurrent" + + " cleanup): {0}", + oldPluginId); + } + } catch (Exception deleteEx) { + logger.debugv( + deleteEx, + "Failed to delete" + + " unreachable plugin" + + " (may already be" + + " deleted): {0}", + oldPluginId); + } + }); } } }); - // new plugin registration - plugin = - QuarkusTransaction.joiningExisting() - .call( - () -> { - DiscoveryPlugin p = new DiscoveryPlugin(); - p.callback = callbackUri; - p.realm = - DiscoveryNode.environment( - requireNonBlank(realmName, "realm"), - NodeType.BaseNodeType.REALM); - p.builtin = false; - - var universe = DiscoveryNode.getUniverse(); - p.realm.parent = universe; - p.persist(); - universe.children.add(p.realm); - universe.persist(); - return p; - }); + try { + plugin = + QuarkusTransaction.joiningExisting() + .call( + () -> { + DiscoveryPlugin p = new DiscoveryPlugin(); + p.callback = callbackUri; + p.realm = + DiscoveryNode.environment( + requireNonBlank(realmName, "realm"), + NodeType.BaseNodeType.REALM); + p.builtin = false; + + var universe = DiscoveryNode.getUniverse(); + p.realm.parent = universe; + p.persist(); + universe.children.add(p.realm); + universe.persist(); + return p; + }); + } catch (PersistenceException e) { + if (e.getCause() instanceof ConstraintViolationException) { + logger.warnv( + "Duplicate callback detected during registration, using existing" + + " plugin: {0}", + callbackUri); + plugin = + QuarkusTransaction.joiningExisting() + .call( + () -> + DiscoveryPlugin.find( + "callback", callbackUri) + .singleResult()); + } else { + throw e; + } + } try { locations = jwtFactory.getPluginLocations(plugin); From 47e3a24ea50001b2725e5db7d263c197d0509f79 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Apr 2026 17:43:20 -0400 Subject: [PATCH 08/24] fix(discovery): atomic child node list updates --- .../cryostat/discovery/CustomDiscovery.java | 11 +- .../java/io/cryostat/discovery/Discovery.java | 40 ++++- .../KubeEndpointSlicesDiscovery.java | 22 ++- .../DiscoveryTreeConsistencyTest.java | 152 ++++++++++++++++++ 4 files changed, 213 insertions(+), 12 deletions(-) create mode 100644 src/test/java/io/cryostat/discovery/DiscoveryTreeConsistencyTest.java diff --git a/src/main/java/io/cryostat/discovery/CustomDiscovery.java b/src/main/java/io/cryostat/discovery/CustomDiscovery.java index 0464af1c7..a99d5d965 100644 --- a/src/main/java/io/cryostat/discovery/CustomDiscovery.java +++ b/src/main/java/io/cryostat/discovery/CustomDiscovery.java @@ -39,6 +39,8 @@ import jakarta.annotation.security.RolesAllowed; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; +import jakarta.persistence.EntityManager; +import jakarta.persistence.LockModeType; import jakarta.transaction.Transactional; import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.Consumes; @@ -78,6 +80,7 @@ public class CustomDiscovery { @Inject Logger logger; @Inject EventBus bus; + @Inject EntityManager entityManager; @Inject TargetConnectionManager connectionManager; @Inject URIUtil uriUtil; @@ -200,11 +203,16 @@ RestResponse doCreate( DiscoveryNode.target(target, NodeType.BaseNodeType.JVM); target.discoveryNode = node; DiscoveryNode realm = DiscoveryNode.getRealm(REALM).orElseThrow(); + realm = + entityManager.find( + DiscoveryNode.class, + realm.id, + LockModeType.PESSIMISTIC_WRITE); - realm.children.add(node); node.parent = realm; target.persist(); node.persist(); + realm.children.add(node); realm.persist(); return ResponseBuilder.created( @@ -239,6 +247,7 @@ RestResponse doCreate( public void delete(@RestPath long id) throws URISyntaxException { Target target = Target.find("id", id).singleResult(); DiscoveryNode realm = DiscoveryNode.getRealm(REALM).orElseThrow(); + realm = entityManager.find(DiscoveryNode.class, realm.id, LockModeType.PESSIMISTIC_WRITE); boolean withinRealm = realm.children.remove(target.discoveryNode); if (!withinRealm) { throw new BadRequestException(); diff --git a/src/main/java/io/cryostat/discovery/Discovery.java b/src/main/java/io/cryostat/discovery/Discovery.java index 0a02a0b9d..5706c830b 100644 --- a/src/main/java/io/cryostat/discovery/Discovery.java +++ b/src/main/java/io/cryostat/discovery/Discovery.java @@ -61,7 +61,9 @@ import jakarta.enterprise.event.Observes; import jakarta.inject.Inject; import jakarta.persistence.EntityManager; +import jakarta.persistence.LockModeType; import jakarta.persistence.NoResultException; +import jakarta.persistence.OptimisticLockException; import jakarta.persistence.PersistenceException; import jakarta.transaction.Transactional; import jakarta.ws.rs.BadRequestException; @@ -136,6 +138,7 @@ public class Discovery { @Inject URIUtil uriUtil; @Inject KubeEndpointSlicesDiscovery k8sDiscovery; @Inject PluginCallbackFactory callbackFactory; + @Inject EntityManager entityManager; void onStart(@Observes StartupEvent evt) { QuarkusTransaction.requiringNew() @@ -544,6 +547,9 @@ public void publishWithContext( @RestHeader("Cryostat-Discovery-Authentication") String token, DiscoveryPublication body) { DiscoveryPlugin plugin = DiscoveryPlugin.find("id", id).singleResult(); + DiscoveryNode realm = + entityManager.find( + DiscoveryNode.class, plugin.realm.id, LockModeType.PESSIMISTIC_WRITE); try { jwtValidator.validateJwt(ctx, plugin, token, true); } catch (MalformedURLException @@ -559,12 +565,12 @@ public void publishWithContext( validatePublishedNode(n); } - plugin.realm.children.clear(); + List replacementChildren = new ArrayList<>(); for (var n : body.nodes) { n.target.discoveryNode = n; } - body.fillStrategy.ifPresent( + body.fillStrategy.ifPresentOrElse( algo -> { Map pubCtx = body.context.orElse(Map.of()); switch (algo) { @@ -603,7 +609,6 @@ public void publishWithContext( nsNode.labels = new HashMap<>(); nsNode.children = new ArrayList<>(); nsNode.target = null; - nsNode.parent = plugin.realm; nsNode.children.add(lineage); lineage.parent = nsNode; @@ -618,19 +623,26 @@ public void publishWithContext( } nsNode.persist(); - plugin.realm.children.add(nsNode); + replacementChildren.add(nsNode); break; default: - plugin.realm.children.addAll(body.nodes); + replacementChildren.addAll(body.nodes); for (var n : body.nodes) { - n.parent = plugin.realm; + n.parent = realm; n.persist(); } break; } + }, + () -> { + replacementChildren.addAll(body.nodes); + for (var n : body.nodes) { + n.parent = realm; + n.persist(); + } }); - plugin.realm.persist(); + replaceChildren(realm, replacementChildren); plugin.persist(); } @@ -743,6 +755,20 @@ private void validatePublishedNode(DiscoveryNode currentNode) { } } + private void replaceChildren(DiscoveryNode parent, List replacementChildren) { + List existingChildren = new ArrayList<>(parent.children); + parent.children.clear(); + existingChildren.forEach(child -> child.parent = null); + replacementChildren.forEach(child -> child.parent = parent); + parent.children.addAll(replacementChildren); + try { + parent.persist(); + entityManager.flush(); + } catch (OptimisticLockException e) { + throw new BadRequestException("Discovery tree update conflict", e); + } + } + @SuppressFBWarnings("DLS_DEAD_LOCAL_STORE") private DiscoveryNode mergeRealms() { DiscoveryNode universe = DiscoveryNode.getUniverse(); diff --git a/src/main/java/io/cryostat/discovery/KubeEndpointSlicesDiscovery.java b/src/main/java/io/cryostat/discovery/KubeEndpointSlicesDiscovery.java index ebd3ddef9..39bd202be 100644 --- a/src/main/java/io/cryostat/discovery/KubeEndpointSlicesDiscovery.java +++ b/src/main/java/io/cryostat/discovery/KubeEndpointSlicesDiscovery.java @@ -60,6 +60,8 @@ import jakarta.enterprise.context.ApplicationScoped; import jakarta.enterprise.event.Observes; import jakarta.inject.Inject; +import jakarta.persistence.EntityManager; +import jakarta.persistence.LockModeType; import jakarta.transaction.Transactional; import jakarta.transaction.Transactional.TxType; import org.apache.commons.lang3.StringUtils; @@ -115,6 +117,8 @@ public class KubeEndpointSlicesDiscovery implements ResourceEventHandler n.name.equals(namespace)) - .orElse( - DiscoveryNode.environment( - namespace, KubeDiscoveryNodeType.NAMESPACE)); + DiscoveryNode.getChild(lockedRealm, n -> n.name.equals(namespace)) + .orElseGet( + () -> { + DiscoveryNode created = + DiscoveryNode.environment( + namespace, KubeDiscoveryNodeType.NAMESPACE); + created.parent = lockedRealm; + return created; + }); if (evt.eventKind == EventKind.FOUND) { persistOwnerChain(nsNode, evt.target, evt.objRef); @@ -402,6 +413,7 @@ public void handleEndpointEvent(EndpointDiscoveryEvent evt) { realm.children.add(nsNode); nsNode.parent = realm; } + entityManager.flush(); realm.persist(); } @@ -718,6 +730,7 @@ private void pruneOwnerChain(DiscoveryNode nsNode, Target target) { child = parent; } + entityManager.flush(); nsNode.persist(); target.delete(); } @@ -807,6 +820,7 @@ private void persistNodeChain(DiscoveryNode nsNode, Target target, DiscoveryNode } // Finally persist the namespace node + entityManager.flush(); nsNode.persist(); } diff --git a/src/test/java/io/cryostat/discovery/DiscoveryTreeConsistencyTest.java b/src/test/java/io/cryostat/discovery/DiscoveryTreeConsistencyTest.java new file mode 100644 index 000000000..f79092ae4 --- /dev/null +++ b/src/test/java/io/cryostat/discovery/DiscoveryTreeConsistencyTest.java @@ -0,0 +1,152 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.discovery; + +import static org.junit.jupiter.api.Assertions.*; + +import java.net.URI; +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; + +import io.cryostat.AbstractTransactionalTestBase; +import io.cryostat.targets.Target; + +import io.quarkus.narayana.jta.QuarkusTransaction; +import io.quarkus.test.junit.QuarkusTest; +import jakarta.inject.Inject; +import jakarta.persistence.EntityManager; +import jakarta.persistence.LockModeType; +import org.junit.jupiter.api.Test; + +@QuarkusTest +class DiscoveryTreeConsistencyTest extends AbstractTransactionalTestBase { + + @Inject EntityManager entityManager; + + @Test + void shouldReplaceRealmChildrenAtomically() { + UUID pluginId = QuarkusTransaction.requiringNew().call(() -> createPlugin("atomic-realm")); + + QuarkusTransaction.requiringNew() + .run( + () -> { + DiscoveryPlugin plugin = + entityManager.find(DiscoveryPlugin.class, pluginId); + DiscoveryNode realm = plugin.realm; + DiscoveryNode stale = + targetNode( + "service:jmx:rmi:///jndi/rmi://127.0.0.1:9091/jmxrmi"); + realm.children.add(stale); + stale.parent = realm; + entityManager.persist(stale); + entityManager.flush(); + }); + + QuarkusTransaction.requiringNew() + .run( + () -> { + DiscoveryPlugin plugin = + entityManager.find(DiscoveryPlugin.class, pluginId); + DiscoveryNode realm = plugin.realm; + List replacement = new ArrayList<>(); + replacement.add( + targetNode( + "service:jmx:rmi:///jndi/rmi://127.0.0.2:9091/jmxrmi")); + replacement.add( + targetNode( + "service:jmx:rmi:///jndi/rmi://127.0.0.3:9091/jmxrmi")); + List staleChildren = new ArrayList<>(realm.children); + realm.children.clear(); + staleChildren.forEach(node -> node.parent = null); + for (DiscoveryNode node : replacement) { + node.parent = realm; + realm.children.add(node); + entityManager.persist(node); + } + entityManager.flush(); + }); + + QuarkusTransaction.requiringNew() + .run( + () -> { + DiscoveryPlugin plugin = + entityManager.find(DiscoveryPlugin.class, pluginId); + DiscoveryNode realm = plugin.realm; + assertEquals(2, realm.children.size()); + assertEquals( + List.of( + "service:jmx:rmi:///jndi/rmi://127.0.0.2:9091/jmxrmi", + "service:jmx:rmi:///jndi/rmi://127.0.0.3:9091/jmxrmi"), + realm.children.stream() + .map(n -> n.target.connectUrl.toString()) + .sorted() + .toList()); + }); + } + + @Test + void shouldAcquirePessimisticWriteLockForRealmMutation() { + UUID pluginId = QuarkusTransaction.requiringNew().call(() -> createPlugin("locked-realm")); + + QuarkusTransaction.requiringNew() + .run( + () -> { + DiscoveryPlugin plugin = + entityManager.find(DiscoveryPlugin.class, pluginId); + DiscoveryNode lockedRealm = + entityManager.find( + DiscoveryNode.class, + plugin.realm.id, + LockModeType.PESSIMISTIC_WRITE); + assertNotNull(lockedRealm); + assertEquals(plugin.realm.id, lockedRealm.id); + }); + } + + private UUID createPlugin(String realmName) { + DiscoveryNode realm = new DiscoveryNode(); + realm.name = realmName; + realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); + realm.persist(); + + DiscoveryPlugin plugin = new DiscoveryPlugin(); + plugin.realm = realm; + plugin.callback = URI.create("http://storedcredentials:1@localhost:8181/callback"); + plugin.credential = null; + plugin.builtin = true; + plugin.persist(); + + return plugin.id; + } + + private DiscoveryNode targetNode(String connectUrl) { + Target target = new Target(); + target.alias = connectUrl; + target.connectUrl = URI.create(connectUrl); + target.jvmId = UUID.randomUUID().toString(); + target.labels = new java.util.HashMap<>(); + target.activeRecordings = new ArrayList<>(); + + DiscoveryNode node = new DiscoveryNode(); + node.name = connectUrl; + node.nodeType = NodeType.BaseNodeType.JVM.getKind(); + node.target = target; + node.children = new ArrayList<>(); + target.discoveryNode = node; + return node; + } +} From 35cc3c971d74ffa30689dd45991ff4e04093c48a Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 10:28:12 -0400 Subject: [PATCH 09/24] fix(discovery): idempotent plugin registration on (callback, realmName) only start plugin refresh job for new registrations --- .../java/io/cryostat/discovery/Discovery.java | 226 +++++++++--------- .../cryostat/discovery/DiscoveryPlugin.java | 15 +- .../db/migration/V4.2.0__cryostat.sql | 3 + .../discovery/DiscoveryPluginTest.java | 117 +++++++++ 4 files changed, 247 insertions(+), 114 deletions(-) diff --git a/src/main/java/io/cryostat/discovery/Discovery.java b/src/main/java/io/cryostat/discovery/Discovery.java index 5706c830b..dff4ec813 100644 --- a/src/main/java/io/cryostat/discovery/Discovery.java +++ b/src/main/java/io/cryostat/discovery/Discovery.java @@ -64,7 +64,6 @@ import jakarta.persistence.LockModeType; import jakarta.persistence.NoResultException; import jakarta.persistence.OptimisticLockException; -import jakarta.persistence.PersistenceException; import jakarta.transaction.Transactional; import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.Consumes; @@ -86,7 +85,6 @@ import org.eclipse.microprofile.faulttolerance.Bulkhead; import org.eclipse.microprofile.openapi.annotations.Operation; import org.eclipse.microprofile.openapi.annotations.tags.Tag; -import org.hibernate.exception.ConstraintViolationException; import org.jboss.logging.Logger; import org.jboss.resteasy.reactive.RestForm; import org.jboss.resteasy.reactive.RestHeader; @@ -356,100 +354,99 @@ public PluginRegistration register(@Context RoutingContext ctx, JsonObject body) throw new InternalServerErrorException(e); } } else { - // check if a plugin record with the same callback already exists. If it does, - // ping it: + // check if a plugin record with the same callback and realm-name already exists. If it + // does, ping it: // - if it's still there reject this request as a duplicate // - otherwise delete the previous record and accept this new one as a replacement - QuarkusTransaction.joiningExisting() - .call(() -> DiscoveryPlugin.find("callback", unauthCallback)) - .singleResultOptional() - .ifPresent( - p -> { - try { - var cb = callbackFactory.create(p); - cb.ping(); - throw new DuplicatePluginException( - String.format( - "Plugin with callback %s already exists and is" - + " still reachable", - unauthCallback)); - } catch (Exception e) { - if (!(e instanceof DuplicatePluginException)) { - logger.debug(e); - UUID oldPluginId = p.id; - QuarkusTransaction.joiningExisting() - .run( - () -> { - try { - var existing = - DiscoveryPlugin - . - findById( - oldPluginId); - if (existing != null) { - existing.delete(); - logger.debugv( - "Deleted unreachable" - + " plugin: {0}", - oldPluginId); - } else { - logger.debugv( - "Plugin already deleted" - + " (concurrent" - + " cleanup): {0}", - oldPluginId); - } - } catch (Exception deleteEx) { - logger.debugv( - deleteEx, - "Failed to delete" - + " unreachable plugin" - + " (may already be" - + " deleted): {0}", - oldPluginId); - } - }); - } - } - }); + boolean isNewPlugin = false; + plugin = + QuarkusTransaction.joiningExisting() + .call( + () -> { + Optional existing = + DiscoveryPlugin.findByCallbackAndRealmName( + unauthCallback, realmName); - try { - plugin = - QuarkusTransaction.joiningExisting() - .call( - () -> { - DiscoveryPlugin p = new DiscoveryPlugin(); - p.callback = callbackUri; - p.realm = - DiscoveryNode.environment( - requireNonBlank(realmName, "realm"), - NodeType.BaseNodeType.REALM); - p.builtin = false; - - var universe = DiscoveryNode.getUniverse(); - p.realm.parent = universe; - p.persist(); - universe.children.add(p.realm); - universe.persist(); - return p; - }); - } catch (PersistenceException e) { - if (e.getCause() instanceof ConstraintViolationException) { - logger.warnv( - "Duplicate callback detected during registration, using existing" - + " plugin: {0}", - callbackUri); - plugin = - QuarkusTransaction.joiningExisting() - .call( - () -> - DiscoveryPlugin.find( - "callback", callbackUri) - .singleResult()); - } else { - throw e; - } - } + if (existing.isPresent()) { + logger.debugv( + "Reusing existing plugin: {0}", + existing.get().id); + return existing.get(); + } + + // Check for plugin with same callback, different realm + Optional byCallback = + DiscoveryPlugin.find( + "callback", unauthCallback) + .singleResultOptional(); + + if (byCallback.isPresent()) { + DiscoveryPlugin p = byCallback.get(); + try { + var cb = callbackFactory.create(p); + cb.ping(); + // Plugin is reachable but has different realm + throw new DuplicatePluginException( + String.format( + "Plugin with callback %s already" + + " exists and is still" + + " reachable", + unauthCallback)); + } catch (Exception e) { + if (!(e instanceof DuplicatePluginException)) { + // Plugin unreachable, delete and create new + logger.debug(e); + UUID oldPluginId = p.id; + try { + var toDelete = + DiscoveryPlugin + .findById( + oldPluginId); + if (toDelete != null) { + toDelete.delete(); + logger.debugv( + "Deleted unreachable plugin:" + + " {0}", + oldPluginId); + } else { + logger.debugv( + "Plugin already deleted" + + " (concurrent cleanup):" + + " {0}", + oldPluginId); + } + } catch (Exception deleteEx) { + logger.debugv( + deleteEx, + "Failed to delete unreachable" + + " plugin (may already be" + + " deleted): {0}", + oldPluginId); + } + } else { + throw e; + } + } + } + + // Create new plugin + DiscoveryPlugin p = new DiscoveryPlugin(); + p.callback = callbackUri; + p.realm = + DiscoveryNode.environment( + requireNonBlank(realmName, "realm"), + NodeType.BaseNodeType.REALM); + p.builtin = false; + + var universe = DiscoveryNode.getUniverse(); + p.realm.parent = universe; + p.persist(); + universe.children.add(p.realm); + universe.persist(); + + logger.debugv("Created new plugin: {0}", p.id); + return p; + }); try { locations = jwtFactory.getPluginLocations(plugin); @@ -457,26 +454,29 @@ public PluginRegistration register(@Context RoutingContext ctx, JsonObject body) throw new BadRequestException(e); } - var dataMap = new JobDataMap(); - dataMap.put(PLUGIN_ID_MAP_KEY, plugin.id); - dataMap.put(REFRESH_MAP_KEY, true); - JobDetail jobDetail = - JobBuilder.newJob(RefreshPluginJob.class) - .withIdentity(plugin.id.toString(), JOB_PERIODIC) - .usingJobData(dataMap) - .build(); - var trigger = - TriggerBuilder.newTrigger() - .withIdentity( - jobDetail.getKey().getName(), jobDetail.getKey().getGroup()) - .startAt(Date.from(Instant.now().plus(discoveryPingPeriod))) - .withSchedule( - SimpleScheduleBuilder.simpleSchedule() - .repeatForever() - .withIntervalInSeconds( - (int) discoveryPingPeriod.toSeconds())) - .build(); - scheduler.scheduleJob(jobDetail, trigger); + isNewPlugin = !scheduler.checkExists(JobKey.jobKey(plugin.id.toString(), JOB_PERIODIC)); + if (isNewPlugin) { + var dataMap = new JobDataMap(); + dataMap.put(PLUGIN_ID_MAP_KEY, plugin.id); + dataMap.put(REFRESH_MAP_KEY, true); + JobDetail jobDetail = + JobBuilder.newJob(RefreshPluginJob.class) + .withIdentity(plugin.id.toString(), JOB_PERIODIC) + .usingJobData(dataMap) + .build(); + var trigger = + TriggerBuilder.newTrigger() + .withIdentity( + jobDetail.getKey().getName(), jobDetail.getKey().getGroup()) + .startAt(Date.from(Instant.now().plus(discoveryPingPeriod))) + .withSchedule( + SimpleScheduleBuilder.simpleSchedule() + .repeatForever() + .withIntervalInSeconds( + (int) discoveryPingPeriod.toSeconds())) + .build(); + scheduler.scheduleJob(jobDetail, trigger); + } } String token; diff --git a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java index 90cce1548..bb8a42613 100644 --- a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java +++ b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java @@ -18,6 +18,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.time.Instant; +import java.util.Optional; import java.util.UUID; import java.util.function.Supplier; @@ -75,7 +76,12 @@ @NamedQueries({ @NamedQuery( name = "DiscoveryPlugin.getBuiltinRealmIds", - query = "SELECT p.realm.id FROM DiscoveryPlugin p WHERE p.builtin = true") + query = "SELECT p.realm.id FROM DiscoveryPlugin p WHERE p.builtin = true"), + @NamedQuery( + name = "DiscoveryPlugin.findByCallbackAndRealmName", + query = + "SELECT p FROM DiscoveryPlugin p JOIN FETCH p.realm r WHERE p.callback = ?1" + + " AND r.name = ?2") }) public class DiscoveryPlugin extends PanacheEntityBase { @@ -128,6 +134,13 @@ public class DiscoveryPlugin extends PanacheEntityBase { @Convert(converter = InstantConverter.class) public Instant nextPingAt; + public static Optional findByCallbackAndRealmName( + URI callback, String realmName) { + return DiscoveryPlugin.find( + "#DiscoveryPlugin.findByCallbackAndRealmName", callback, realmName) + .singleResultOptional(); + } + @ApplicationScoped static class Listener { diff --git a/src/main/resources/db/migration/V4.2.0__cryostat.sql b/src/main/resources/db/migration/V4.2.0__cryostat.sql index 6701f9e35..2d182380e 100644 --- a/src/main/resources/db/migration/V4.2.0__cryostat.sql +++ b/src/main/resources/db/migration/V4.2.0__cryostat.sql @@ -650,4 +650,7 @@ ALTER TABLE DiscoveryPlugin ADD COLUMN lastFailedPing BIGINT; ALTER TABLE DiscoveryPlugin ADD COLUMN backoffMultiplier INTEGER NOT NULL DEFAULT 1; ALTER TABLE DiscoveryPlugin ADD COLUMN nextPingAt BIGINT; +-- Add unique constraint on DiscoveryNode name for Realm nodes to enforce realm name uniqueness +CREATE UNIQUE INDEX uk_discovery_node_realm_name ON DiscoveryNode (name) WHERE (nodeType = 'Realm'); + COMMIT; diff --git a/src/test/java/io/cryostat/discovery/DiscoveryPluginTest.java b/src/test/java/io/cryostat/discovery/DiscoveryPluginTest.java index 6688fc774..503680f75 100644 --- a/src/test/java/io/cryostat/discovery/DiscoveryPluginTest.java +++ b/src/test/java/io/cryostat/discovery/DiscoveryPluginTest.java @@ -474,6 +474,123 @@ void testPublishHierarchicalNodeList() { // subtree } + @Test + void testIdempotentRegistration() { + // store credentials + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl ==" + + " 'http://localhost:8081/health/liveness'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + // register first time + var realmName = "idempotent_test_realm"; + var callback = + String.format( + "http://storedcredentials:%d@localhost:8081/health/liveness", credentialId); + var registration1 = + given().log() + .all() + .when() + .body(Map.of("realm", realmName, "callback", callback)) + .contentType(ContentType.JSON) + .post("/api/v4/discovery") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(200) + .contentType(ContentType.JSON) + .extract() + .jsonPath(); + var pluginId1 = registration1.getString("id"); + var pluginToken1 = registration1.getString("token"); + MatcherAssert.assertThat( + pluginId1, Matchers.is(Matchers.not(Matchers.emptyOrNullString()))); + MatcherAssert.assertThat( + pluginToken1, Matchers.is(Matchers.not(Matchers.emptyOrNullString()))); + + // register second time with same callback and realm - should be idempotent + var registration2 = + given().log() + .all() + .when() + .body(Map.of("realm", realmName, "callback", callback)) + .contentType(ContentType.JSON) + .post("/api/v4/discovery") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(200) + .contentType(ContentType.JSON) + .extract() + .jsonPath(); + var pluginId2 = registration2.getString("id"); + var pluginToken2 = registration2.getString("token"); + + // Should return the same plugin ID (idempotent) + MatcherAssert.assertThat(pluginId2, Matchers.equalTo(pluginId1)); + // Token should be different (refreshed) + MatcherAssert.assertThat(pluginToken2, Matchers.not(Matchers.equalTo(pluginToken1))); + + // register third time - verify still idempotent + var registration3 = + given().log() + .all() + .when() + .body(Map.of("realm", realmName, "callback", callback)) + .contentType(ContentType.JSON) + .post("/api/v4/discovery") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(200) + .contentType(ContentType.JSON) + .extract() + .jsonPath(); + var pluginId3 = registration3.getString("id"); + + MatcherAssert.assertThat(pluginId3, Matchers.equalTo(pluginId1)); + + // cleanup + given().log() + .all() + .when() + .header(DISCOVERY_HEADER, pluginToken2) + .delete(String.format("/api/v4/discovery/%s", pluginId1)) + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(204); + } + record Node(String name, String nodeType, Target target, List children) { Node(String name, String nodeType, Target target) { this(name, nodeType, target, null); From 0fafc7b2841e39908a2f709798c2eb68335394db Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 11:05:04 -0400 Subject: [PATCH 10/24] schema update --- schema/openapi.yaml | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/schema/openapi.yaml b/schema/openapi.yaml index 6d2e7edde..54de73ff5 100644 --- a/schema/openapi.yaml +++ b/schema/openapi.yaml @@ -336,14 +336,26 @@ components: type: object DiscoveryPlugin: properties: + backoffMultiplier: + format: int32 + type: integer builtin: readOnly: true type: boolean callback: format: uri type: string + consecutiveFailures: + format: int32 + type: integer id: $ref: '#/components/schemas/UUID' + lastFailedPing: + $ref: '#/components/schemas/Instant' + lastSuccessfulPing: + $ref: '#/components/schemas/Instant' + nextPingAt: + $ref: '#/components/schemas/Instant' realm: $ref: '#/components/schemas/DiscoveryNode' required: @@ -352,14 +364,26 @@ components: type: object DiscoveryPlugin_Flat: properties: + backoffMultiplier: + format: int32 + type: integer builtin: readOnly: true type: boolean callback: format: uri type: string + consecutiveFailures: + format: int32 + type: integer id: $ref: '#/components/schemas/UUID' + lastFailedPing: + $ref: '#/components/schemas/Instant' + lastSuccessfulPing: + $ref: '#/components/schemas/Instant' + nextPingAt: + $ref: '#/components/schemas/Instant' realm: $ref: '#/components/schemas/DiscoveryNode_Flat' required: @@ -489,6 +513,11 @@ components: statusMessage: type: string type: object + Instant: + examples: + - 2022-03-10T16:15:50Z + format: date-time + type: string JniInfo: properties: globalRefs: From dea5037c702c11628f7d2b8e6e693cb6e4badb09 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 11:15:37 -0400 Subject: [PATCH 11/24] fix(discovery): plugin-associated credentials expire after 1 hour of non-use --- .../java/io/cryostat/ConfigProperties.java | 1 + .../io/cryostat/credentials/Credential.java | 44 ++++++++ .../credentials/CredentialExpirationJob.java | 50 +++++++++ .../cryostat/discovery/DiscoveryPlugin.java | 10 ++ .../java/io/cryostat/targets/AgentClient.java | 5 +- src/main/resources/application.properties | 1 + .../db/migration/V4.2.0__cryostat.sql | 2 + .../CredentialExpirationCleanupTest.java | 106 ++++++++++++++++++ .../cryostat/credentials/CredentialsTest.java | 68 +++++++++++ .../DiscoveryPluginGracePeriodTest.java | 67 +++++++++++ 10 files changed, 353 insertions(+), 1 deletion(-) create mode 100644 src/main/java/io/cryostat/credentials/CredentialExpirationJob.java create mode 100644 src/test/java/io/cryostat/credentials/CredentialExpirationCleanupTest.java diff --git a/src/main/java/io/cryostat/ConfigProperties.java b/src/main/java/io/cryostat/ConfigProperties.java index 8c3a59b47..23adfc957 100644 --- a/src/main/java/io/cryostat/ConfigProperties.java +++ b/src/main/java/io/cryostat/ConfigProperties.java @@ -108,6 +108,7 @@ public class ConfigProperties { public static final String URI_RANGE = "cryostat.target.uri-range"; public static final String AGENT_TLS_REQUIRED = "cryostat.agent.tls.required"; + public static final String CREDENTIAL_EXPIRATION = "cryostat.credentials.expiration"; public static final String DECLARATIVE_CONFIG_RESOLVE_SYMLINKS = "cryostat.declarative-configuration.symlinks.resolve"; diff --git a/src/main/java/io/cryostat/credentials/Credential.java b/src/main/java/io/cryostat/credentials/Credential.java index 4231d9532..ced286e48 100644 --- a/src/main/java/io/cryostat/credentials/Credential.java +++ b/src/main/java/io/cryostat/credentials/Credential.java @@ -15,8 +15,14 @@ */ package io.cryostat.credentials; +import java.time.Duration; +import java.time.Instant; +import java.util.List; + +import io.cryostat.ConfigProperties; import io.cryostat.credentials.events.CredentialEvents; import io.cryostat.discovery.DiscoveryPlugin; +import io.cryostat.discovery.InstantConverter; import io.cryostat.expressions.MatchExpression; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -29,16 +35,22 @@ import jakarta.persistence.Cacheable; import jakarta.persistence.CascadeType; import jakarta.persistence.Column; +import jakarta.persistence.Convert; import jakarta.persistence.Entity; import jakarta.persistence.EntityListeners; import jakarta.persistence.FetchType; import jakarta.persistence.JoinColumn; +import jakarta.persistence.NamedQueries; +import jakarta.persistence.NamedQuery; import jakarta.persistence.OneToOne; import jakarta.persistence.PostPersist; import jakarta.persistence.PostRemove; import jakarta.persistence.PostUpdate; +import jakarta.persistence.PrePersist; +import jakarta.persistence.PreUpdate; import jakarta.validation.constraints.NotBlank; import jakarta.validation.constraints.NotNull; +import org.eclipse.microprofile.config.inject.ConfigProperty; import org.hibernate.annotations.Cache; import org.hibernate.annotations.CacheConcurrencyStrategy; import org.hibernate.annotations.ColumnTransformer; @@ -63,6 +75,11 @@ @Entity @EntityListeners(Credential.Listener.class) @Cacheable +@NamedQueries({ + @NamedQuery( + name = "Credential.findExpired", + query = "SELECT c FROM Credential c WHERE c.expiresAt IS NOT NULL AND c.expiresAt < ?1") +}) public class Credential extends PanacheEntity { public static final String CREDENTIALS_STORED = "CredentialsStored"; @@ -129,12 +146,39 @@ public class Credential extends PanacheEntity { @Nullable public DiscoveryPlugin discoveryPlugin; + @Column(nullable = true) + @Convert(converter = InstantConverter.class) + public Instant expiresAt; + + @Column(nullable = true) + @Convert(converter = InstantConverter.class) + public Instant lastUsedAt; + + public static List findExpired() { + return findExpired(Instant.now()); + } + + public static List findExpired(Instant cutoff) { + return Credential.find("#Credential.findExpired", cutoff).list(); + } + @ApplicationScoped static class Listener { + @ConfigProperty(name = ConfigProperties.CREDENTIAL_EXPIRATION) + Duration credentialExpiration; + @Inject Event credentialCreatedEvent; @Inject Event credentialUpdatedEvent; @Inject Event credentialDeletedEvent; + @PrePersist + @PreUpdate + public void applyDiscoveryPluginExpiry(Credential credential) { + if (credential.discoveryPlugin != null && credential.expiresAt == null) { + credential.expiresAt = Instant.now().plus(credentialExpiration); + } + } + @PostPersist public void postPersist(Credential credential) { CredentialEvents.CredentialSnapshot snapshot = createSnapshot(credential); diff --git a/src/main/java/io/cryostat/credentials/CredentialExpirationJob.java b/src/main/java/io/cryostat/credentials/CredentialExpirationJob.java new file mode 100644 index 000000000..4843d6a87 --- /dev/null +++ b/src/main/java/io/cryostat/credentials/CredentialExpirationJob.java @@ -0,0 +1,50 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.credentials; + +import java.util.List; + +import io.cryostat.ConfigProperties; +import io.cryostat.discovery.DiscoveryPlugin; + +import io.quarkus.scheduler.Scheduled; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import jakarta.transaction.Transactional; +import org.eclipse.microprofile.config.inject.ConfigProperty; +import org.jboss.logging.Logger; + +@ApplicationScoped +public class CredentialExpirationJob { + + @Inject Logger logger; + + @ConfigProperty(name = ConfigProperties.CREDENTIAL_EXPIRATION) + String credentialExpiration; + + @Scheduled(every = "${" + ConfigProperties.CREDENTIAL_EXPIRATION + "}") + @Transactional + void cleanupExpiredCredentials() { + List expired = Credential.findExpired(); + for (Credential credential : expired) { + long pluginCount = DiscoveryPlugin.count("credential", credential); + if (pluginCount == 0) { + logger.infov("Deleting expired credential {0}", credential.id); + credential.delete(); + } + } + } +} diff --git a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java index bb8a42613..111446895 100644 --- a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java +++ b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java @@ -17,11 +17,13 @@ import java.net.URI; import java.net.URISyntaxException; +import java.time.Duration; import java.time.Instant; import java.util.Optional; import java.util.UUID; import java.util.function.Supplier; +import io.cryostat.ConfigProperties; import io.cryostat.credentials.Credential; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -57,6 +59,7 @@ import jakarta.ws.rs.core.MultivaluedMap; import jakarta.ws.rs.core.UriBuilder; import org.apache.commons.lang3.StringUtils; +import org.eclipse.microprofile.config.inject.ConfigProperty; import org.hibernate.annotations.Cache; import org.hibernate.annotations.CacheConcurrencyStrategy; import org.hibernate.annotations.UuidGenerator; @@ -147,6 +150,9 @@ static class Listener { @Inject Logger logger; @Inject PluginCallbackFactory callbackFactory; + @ConfigProperty(name = ConfigProperties.CREDENTIAL_EXPIRATION) + Duration credentialExpiration; + @PrePersist @Transactional public void prePersist(DiscoveryPlugin plugin) { @@ -159,6 +165,10 @@ public void prePersist(DiscoveryPlugin plugin) { if (plugin.credential == null) { var credential = getCredential(plugin); plugin.credential = credential; + credential.discoveryPlugin = plugin; + if (credential.expiresAt == null) { + credential.expiresAt = Instant.now().plus(credentialExpiration); + } plugin.callback = UriBuilder.fromUri(plugin.callback).userInfo(null).build(); } if (plugin.nextPingAt != null diff --git a/src/main/java/io/cryostat/targets/AgentClient.java b/src/main/java/io/cryostat/targets/AgentClient.java index b1ac86622..744a321b0 100644 --- a/src/main/java/io/cryostat/targets/AgentClient.java +++ b/src/main/java/io/cryostat/targets/AgentClient.java @@ -21,6 +21,7 @@ import java.io.InputStream; import java.net.URI; import java.time.Duration; +import java.time.Instant; import java.util.Arrays; import java.util.Collection; import java.util.HashMap; @@ -117,7 +118,7 @@ Duration getTimeout() { return httpTimeout; } - Uni ping() { + public Uni ping() { return agentRestClient .ping() .invoke(Response::close) @@ -645,6 +646,8 @@ public AgentClient create(Target target) { if (credential == null) { throw new ConnectionException(NULL_CREDENTIALS); } + credential.lastUsedAt = Instant.now(); + credential.persist(); return new UsernamePasswordCredentials( credential.username, credential.password); }); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index bcce8a9ca..ca8170857 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -91,6 +91,7 @@ cryostat.http.proxy.path=/ cryostat.target.uri-range=PUBLIC cryostat.agent.tls.required=true +cryostat.credentials.expiration=1h # these rest-client urls should always be overridden quarkus.rest-client.reports.url=http://localhost/ diff --git a/src/main/resources/db/migration/V4.2.0__cryostat.sql b/src/main/resources/db/migration/V4.2.0__cryostat.sql index 2d182380e..719e1972b 100644 --- a/src/main/resources/db/migration/V4.2.0__cryostat.sql +++ b/src/main/resources/db/migration/V4.2.0__cryostat.sql @@ -649,6 +649,8 @@ ALTER TABLE DiscoveryPlugin ADD COLUMN lastSuccessfulPing BIGINT; ALTER TABLE DiscoveryPlugin ADD COLUMN lastFailedPing BIGINT; ALTER TABLE DiscoveryPlugin ADD COLUMN backoffMultiplier INTEGER NOT NULL DEFAULT 1; ALTER TABLE DiscoveryPlugin ADD COLUMN nextPingAt BIGINT; +ALTER TABLE Credential ADD COLUMN expiresAt BIGINT; +ALTER TABLE Credential ADD COLUMN lastUsedAt BIGINT; -- Add unique constraint on DiscoveryNode name for Realm nodes to enforce realm name uniqueness CREATE UNIQUE INDEX uk_discovery_node_realm_name ON DiscoveryNode (name) WHERE (nodeType = 'Realm'); diff --git a/src/test/java/io/cryostat/credentials/CredentialExpirationCleanupTest.java b/src/test/java/io/cryostat/credentials/CredentialExpirationCleanupTest.java new file mode 100644 index 000000000..729c3cafe --- /dev/null +++ b/src/test/java/io/cryostat/credentials/CredentialExpirationCleanupTest.java @@ -0,0 +1,106 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.credentials; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.net.URI; +import java.time.Instant; + +import io.cryostat.AbstractTransactionalTestBase; +import io.cryostat.discovery.DiscoveryNode; +import io.cryostat.discovery.DiscoveryPlugin; +import io.cryostat.discovery.NodeType; +import io.cryostat.expressions.MatchExpression; + +import io.quarkus.narayana.jta.QuarkusTransaction; +import io.quarkus.test.junit.QuarkusTest; +import jakarta.inject.Inject; +import org.junit.jupiter.api.Test; + +@QuarkusTest +public class CredentialExpirationCleanupTest extends AbstractTransactionalTestBase { + + @Inject CredentialExpirationJob credentialExpirationJob; + + @Test + void deletesExpiredUnassociatedCredentials() { + long credentialId = + QuarkusTransaction.requiringNew() + .call( + () -> { + var expr = new MatchExpression("true"); + expr.persist(); + + var credential = new Credential(); + credential.matchExpression = expr; + credential.username = "user"; + credential.password = "pass"; + credential.expiresAt = Instant.now().minusSeconds(10); + credential.persist(); + + return credential.id; + }); + + credentialExpirationJob.cleanupExpiredCredentials(); + + var deleted = + QuarkusTransaction.requiringNew() + .call(() -> Credential.findById(credentialId)); + assertNull(deleted); + } + + @Test + void doesNotDeleteExpiredCredentialsStillAssociatedWithPlugin() { + long credentialId = + QuarkusTransaction.requiringNew() + .call( + () -> { + var expr = new MatchExpression("true"); + expr.persist(); + + var credential = new Credential(); + credential.matchExpression = expr; + credential.username = "user"; + credential.password = "pass"; + credential.expiresAt = Instant.now().minusSeconds(10); + credential.persist(); + + var realm = new DiscoveryNode(); + realm.name = "cleanup_test_realm"; + realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); + realm.persist(); + + var plugin = new DiscoveryPlugin(); + plugin.realm = realm; + plugin.callback = + URI.create("http://localhost:8081/health/liveness"); + plugin.builtin = false; + plugin.credential = credential; + plugin.persist(); + + return credential.id; + }); + + credentialExpirationJob.cleanupExpiredCredentials(); + + var retained = + QuarkusTransaction.requiringNew() + .call(() -> Credential.findById(credentialId)); + assertNotNull(retained); + } +} diff --git a/src/test/java/io/cryostat/credentials/CredentialsTest.java b/src/test/java/io/cryostat/credentials/CredentialsTest.java index c6b1028c3..50e7a8ec7 100644 --- a/src/test/java/io/cryostat/credentials/CredentialsTest.java +++ b/src/test/java/io/cryostat/credentials/CredentialsTest.java @@ -16,15 +16,19 @@ package io.cryostat.credentials; import static io.restassured.RestAssured.given; +import static org.junit.jupiter.api.Assertions.assertNull; +import java.time.Instant; import java.util.List; import io.cryostat.AbstractTransactionalTestBase; +import io.quarkus.narayana.jta.QuarkusTransaction; import io.quarkus.test.common.http.TestHTTPEndpoint; import io.quarkus.test.junit.QuarkusTest; import io.restassured.http.ContentType; import org.hamcrest.Matchers; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @QuarkusTest @@ -158,6 +162,70 @@ public void testCredentialCheck() { Matchers.matchesRegex("[\\s]*\"NA\"[\\s]*")); } + @Test + void testCreatedCredentialsDoNotExpireByDefault() { + int id = createTestCredential(); + + var credential = + QuarkusTransaction.requiringNew() + .call(() -> Credential.findById((long) id)); + + assertNull(credential.expiresAt); + assertNull(credential.lastUsedAt); + } + + @Test + void testPluginCredentialGetsDefaultExpiryWhenAssociated() { + var credentialId = + given().log() + .all() + .contentType(ContentType.URLENC) + .formParam( + "matchExpression", + "target.connectUrl == 'http://localhost:8081/health/liveness'") + .formParam("username", "user") + .formParam("password", "pass") + .when() + .post() + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + var callback = + String.format( + "http://storedcredentials:%d@localhost:8081/health/liveness", credentialId); + + given().log() + .all() + .when() + .body(java.util.Map.of("realm", "expiry_test_realm", "callback", callback)) + .contentType(ContentType.JSON) + .post("http://localhost:8081/api/v4/discovery") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(200); + + var credential = + QuarkusTransaction.requiringNew() + .call(() -> Credential.findById(credentialId)); + + Assertions.assertNotNull(credential.expiresAt); + Assertions.assertTrue( + !credential.expiresAt.isBefore(Instant.now().plusSeconds(3500)) + && !credential.expiresAt.isAfter(Instant.now().plusSeconds(3700))); + assertNull(credential.lastUsedAt); + } + private int createTestCredential() { return given().log() .all() diff --git a/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java b/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java index a1b5a6829..572e5a2ef 100644 --- a/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java +++ b/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java @@ -26,7 +26,9 @@ import io.cryostat.AbstractTransactionalTestBase; import io.cryostat.ConfigProperties; +import io.cryostat.credentials.Credential; import io.cryostat.discovery.DiscoveryPlugin.PluginCallback; +import io.cryostat.targets.AgentClient; import io.quarkus.narayana.jta.QuarkusTransaction; import io.quarkus.test.InjectMock; @@ -48,6 +50,7 @@ public class DiscoveryPluginGracePeriodTest extends AbstractTransactionalTestBas int maxConsecutiveFailures; @Inject Discovery.RefreshPluginJob refreshPluginJob; + @Inject AgentClient.Factory agentClientFactory; @InjectMock PluginCallbackFactory callbackFactory; @@ -694,4 +697,68 @@ public void testBackoffResetsOnSuccess() throws Exception { assertNull(updatedPlugin.nextPingAt, "Next ping time should be cleared"); assertNotNull(updatedPlugin.lastSuccessfulPing, "Last successful ping should be set"); } + + @Test + public void testAgentCredentialUseUpdatesLastUsedAt() { + var credentialId = + given().log() + .all() + .when() + .formParams( + Map.of( + "username", + "user", + "password", + "pass", + "matchExpression", + "target.connectUrl ==" + + " 'http://localhost:8081/health/liveness'")) + .contentType(ContentType.URLENC) + .post("/api/v4/credentials") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(201) + .contentType(ContentType.JSON) + .extract() + .jsonPath() + .getLong("id"); + + var callback = + String.format( + "http://storedcredentials:%d@localhost:8081/health/liveness", credentialId); + + given().log() + .all() + .when() + .body(Map.of("realm", "test_last_used_realm", "callback", callback)) + .contentType(ContentType.JSON) + .post("/api/v4/discovery") + .then() + .log() + .all() + .and() + .assertThat() + .statusCode(200); + + var beforeUse = + QuarkusTransaction.requiringNew() + .call(() -> Credential.findById(credentialId)); + assertNull(beforeUse.lastUsedAt); + + var target = new io.cryostat.targets.Target(); + target.connectUrl = URI.create("http://localhost:8081/health/liveness"); + + var client = agentClientFactory.create(target); + + assertDoesNotThrow(() -> client.ping().await().indefinitely()); + + var afterUse = + QuarkusTransaction.requiringNew() + .call(() -> Credential.findById(credentialId)); + assertNotNull(afterUse.lastUsedAt); + assertTrue(afterUse.lastUsedAt.isBefore(Instant.now().plusSeconds(10))); + } } From c77142a5cd8e5b3cb42110076685e16f831b4d1a Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 11:17:16 -0400 Subject: [PATCH 12/24] schema update --- schema/notifications.yaml | 18 ++++++++++++++++++ schema/openapi.yaml | 4 ++++ 2 files changed, 22 insertions(+) diff --git a/schema/notifications.yaml b/schema/notifications.yaml index ba7259c17..a6e3739f8 100644 --- a/schema/notifications.yaml +++ b/schema/notifications.yaml @@ -341,6 +341,12 @@ components: properties: script: type: string + expiresAt: + type: object + description: Payload of type Instant + lastUsedAt: + type: object + description: Payload of type Instant required: - meta - message @@ -371,6 +377,12 @@ components: properties: script: type: string + expiresAt: + type: object + description: Payload of type Instant + lastUsedAt: + type: object + description: Payload of type Instant required: - meta - message @@ -401,6 +413,12 @@ components: properties: script: type: string + expiresAt: + type: object + description: Payload of type Instant + lastUsedAt: + type: object + description: Payload of type Instant required: - meta - message diff --git a/schema/openapi.yaml b/schema/openapi.yaml index 54de73ff5..44e6af97e 100644 --- a/schema/openapi.yaml +++ b/schema/openapi.yaml @@ -202,9 +202,13 @@ components: type: string Credential: properties: + expiresAt: + $ref: '#/components/schemas/Instant' id: format: int64 type: integer + lastUsedAt: + $ref: '#/components/schemas/Instant' matchExpression: $ref: '#/components/schemas/MatchExpression' password: From a12995f563176e72d8d4ce732dc052f2a629b5fd Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 12:44:59 -0400 Subject: [PATCH 13/24] fixup! fix(discovery): plugin-associated credentials expire after 1 hour of non-use --- src/main/resources/db/migration/V4.2.0__cryostat.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/resources/db/migration/V4.2.0__cryostat.sql b/src/main/resources/db/migration/V4.2.0__cryostat.sql index 719e1972b..5d7befa00 100644 --- a/src/main/resources/db/migration/V4.2.0__cryostat.sql +++ b/src/main/resources/db/migration/V4.2.0__cryostat.sql @@ -333,6 +333,8 @@ CREATE TABLE Credential_AUD ( matchExpression BIGINT, username BYTEA, password BYTEA, + expiresAt BIGINT, + lastUsedAt BIGINT, PRIMARY KEY (id, REV), FOREIGN KEY (REV) REFERENCES REVINFO (REV), FOREIGN KEY (REVEND) REFERENCES REVINFO (REV) From 6b1059c795d8d6e5ed2afdae518eb1d8c7076260 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 15:23:34 -0400 Subject: [PATCH 14/24] JsonIgnore new properties --- src/main/java/io/cryostat/credentials/Credential.java | 2 ++ src/main/java/io/cryostat/discovery/DiscoveryPlugin.java | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/src/main/java/io/cryostat/credentials/Credential.java b/src/main/java/io/cryostat/credentials/Credential.java index ced286e48..d2034ceca 100644 --- a/src/main/java/io/cryostat/credentials/Credential.java +++ b/src/main/java/io/cryostat/credentials/Credential.java @@ -148,10 +148,12 @@ public class Credential extends PanacheEntity { @Column(nullable = true) @Convert(converter = InstantConverter.class) + @JsonIgnore public Instant expiresAt; @Column(nullable = true) @Convert(converter = InstantConverter.class) + @JsonIgnore public Instant lastUsedAt; public static List findExpired() { diff --git a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java index 111446895..21d5f7eb0 100644 --- a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java +++ b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java @@ -120,21 +120,26 @@ public class DiscoveryPlugin extends PanacheEntityBase { public boolean builtin; @Column(nullable = false) + @JsonIgnore public int consecutiveFailures = 0; @Column(nullable = true) @Convert(converter = InstantConverter.class) + @JsonIgnore public Instant lastSuccessfulPing; @Column(nullable = true) @Convert(converter = InstantConverter.class) + @JsonIgnore public Instant lastFailedPing; @Column(nullable = false) + @JsonIgnore public int backoffMultiplier = 1; @Column(nullable = true) @Convert(converter = InstantConverter.class) + @JsonIgnore public Instant nextPingAt; public static Optional findByCallbackAndRealmName( From 5298cd13aa9aaa8ebae2cbe97c7b12f4850065d9 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 15:37:12 -0400 Subject: [PATCH 15/24] update schema --- schema/notifications.yaml | 18 ------------------ schema/openapi.yaml | 33 --------------------------------- 2 files changed, 51 deletions(-) diff --git a/schema/notifications.yaml b/schema/notifications.yaml index a6e3739f8..ba7259c17 100644 --- a/schema/notifications.yaml +++ b/schema/notifications.yaml @@ -341,12 +341,6 @@ components: properties: script: type: string - expiresAt: - type: object - description: Payload of type Instant - lastUsedAt: - type: object - description: Payload of type Instant required: - meta - message @@ -377,12 +371,6 @@ components: properties: script: type: string - expiresAt: - type: object - description: Payload of type Instant - lastUsedAt: - type: object - description: Payload of type Instant required: - meta - message @@ -413,12 +401,6 @@ components: properties: script: type: string - expiresAt: - type: object - description: Payload of type Instant - lastUsedAt: - type: object - description: Payload of type Instant required: - meta - message diff --git a/schema/openapi.yaml b/schema/openapi.yaml index 44e6af97e..6d2e7edde 100644 --- a/schema/openapi.yaml +++ b/schema/openapi.yaml @@ -202,13 +202,9 @@ components: type: string Credential: properties: - expiresAt: - $ref: '#/components/schemas/Instant' id: format: int64 type: integer - lastUsedAt: - $ref: '#/components/schemas/Instant' matchExpression: $ref: '#/components/schemas/MatchExpression' password: @@ -340,26 +336,14 @@ components: type: object DiscoveryPlugin: properties: - backoffMultiplier: - format: int32 - type: integer builtin: readOnly: true type: boolean callback: format: uri type: string - consecutiveFailures: - format: int32 - type: integer id: $ref: '#/components/schemas/UUID' - lastFailedPing: - $ref: '#/components/schemas/Instant' - lastSuccessfulPing: - $ref: '#/components/schemas/Instant' - nextPingAt: - $ref: '#/components/schemas/Instant' realm: $ref: '#/components/schemas/DiscoveryNode' required: @@ -368,26 +352,14 @@ components: type: object DiscoveryPlugin_Flat: properties: - backoffMultiplier: - format: int32 - type: integer builtin: readOnly: true type: boolean callback: format: uri type: string - consecutiveFailures: - format: int32 - type: integer id: $ref: '#/components/schemas/UUID' - lastFailedPing: - $ref: '#/components/schemas/Instant' - lastSuccessfulPing: - $ref: '#/components/schemas/Instant' - nextPingAt: - $ref: '#/components/schemas/Instant' realm: $ref: '#/components/schemas/DiscoveryNode_Flat' required: @@ -517,11 +489,6 @@ components: statusMessage: type: string type: object - Instant: - examples: - - 2022-03-10T16:15:50Z - format: date-time - type: string JniInfo: properties: globalRefs: From 5ff7f9e880a823b91e704ff39aed3a296a682aa9 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 15:38:22 -0400 Subject: [PATCH 16/24] reset submodule --- src/main/webui | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/webui b/src/main/webui index cffcbf6ae..a2acc1df1 160000 --- a/src/main/webui +++ b/src/main/webui @@ -1 +1 @@ -Subproject commit cffcbf6ae80cbb82da711dbf21a97a0606721f3a +Subproject commit a2acc1df1ddf60ed6431b43fba4a6e6db441fdbc From beaa44193bea2d483d9b99740920d2fd33acd824 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 22 Apr 2026 16:06:10 -0400 Subject: [PATCH 17/24] do not emit notifications on JsonIgnore'd or write-only attributes --- .../io/cryostat/credentials/Credential.java | 10 +- .../events/NotificationDirtyCheckSupport.java | 178 +++++++++++ .../CredentialNotificationTest.java | 298 ++++++++++++++++++ 3 files changed, 483 insertions(+), 3 deletions(-) create mode 100644 src/main/java/io/cryostat/events/NotificationDirtyCheckSupport.java create mode 100644 src/test/java/io/cryostat/credentials/CredentialNotificationTest.java diff --git a/src/main/java/io/cryostat/credentials/Credential.java b/src/main/java/io/cryostat/credentials/Credential.java index d2034ceca..c380839bc 100644 --- a/src/main/java/io/cryostat/credentials/Credential.java +++ b/src/main/java/io/cryostat/credentials/Credential.java @@ -23,6 +23,7 @@ import io.cryostat.credentials.events.CredentialEvents; import io.cryostat.discovery.DiscoveryPlugin; import io.cryostat.discovery.InstantConverter; +import io.cryostat.events.NotificationDirtyCheckSupport; import io.cryostat.expressions.MatchExpression; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -172,6 +173,7 @@ static class Listener { @Inject Event credentialCreatedEvent; @Inject Event credentialUpdatedEvent; @Inject Event credentialDeletedEvent; + @Inject NotificationDirtyCheckSupport dirtyCheckSupport; @PrePersist @PreUpdate @@ -190,9 +192,11 @@ public void postPersist(Credential credential) { @PostUpdate public void postUpdate(Credential credential) { - CredentialEvents.CredentialSnapshot snapshot = createSnapshot(credential); - credentialUpdatedEvent.fire( - new CredentialEvents.CredentialUpdated(credential.id, snapshot)); + if (dirtyCheckSupport.hasRelevantDirtyProperties(credential)) { + CredentialEvents.CredentialSnapshot snapshot = createSnapshot(credential); + credentialUpdatedEvent.fire( + new CredentialEvents.CredentialUpdated(credential.id, snapshot)); + } } @PostRemove diff --git a/src/main/java/io/cryostat/events/NotificationDirtyCheckSupport.java b/src/main/java/io/cryostat/events/NotificationDirtyCheckSupport.java new file mode 100644 index 000000000..d0f491267 --- /dev/null +++ b/src/main/java/io/cryostat/events/NotificationDirtyCheckSupport.java @@ -0,0 +1,178 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.events; + +import java.lang.reflect.Field; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonProperty; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import jakarta.persistence.EntityManager; +import org.hibernate.engine.spi.EntityEntry; +import org.hibernate.engine.spi.SessionImplementor; +import org.hibernate.persister.entity.EntityPersister; +import org.jboss.logging.Logger; + +/** + * Utility class for checking which entity properties have been modified (dirty) in the current + * Hibernate session. This is used to determine whether entity updates should trigger WebSocket + * notifications based on which specific fields were changed. + * + *

This class provides methods to: + * + *

    + *
  • Check if any notification-relevant properties are dirty using Jackson annotation reflection + *
  • Get all dirty property names for debugging purposes + *
+ * + *

The dirty-checking mechanism uses Hibernate's internal APIs to access the entity's dirty state + * metadata from the persistence context. Fields are considered notification-relevant unless they + * are marked with {@code @JsonIgnore} or {@code @JsonProperty(access = + * JsonProperty.Access.WRITE_ONLY)}. This approach automatically adapts to field additions, + * removals, and renames without requiring manual updates to property name lists. + */ +@ApplicationScoped +public class NotificationDirtyCheckSupport { + + @Inject EntityManager entityManager; + @Inject Logger logger; + + /** + * Checks if any notification-relevant properties are dirty (modified) for the given entity. A + * property is considered notification-relevant unless it is marked with {@code @JsonIgnore} or + * {@code @JsonProperty(access = JsonProperty.Access.WRITE_ONLY)}. + * + * @param entity the entity to check for dirty properties + * @return true if any notification-relevant property is dirty, false otherwise or if dirty + * checking is unavailable + */ + public boolean hasRelevantDirtyProperties(Object entity) { + Set dirtyProperties = getDirtyPropertyNames(entity); + + if (dirtyProperties.isEmpty()) { + return false; + } + + Set ignoredProperties = getIgnoredPropertyNames(entity.getClass()); + for (String dirtyProperty : dirtyProperties) { + if (!ignoredProperties.contains(dirtyProperty)) { + logger.tracev( + "Entity {0} has notification-relevant dirty property: {1}", + entity.getClass().getSimpleName(), dirtyProperty); + return true; + } + } + + logger.tracev( + "Entity {0} has dirty properties {1} but none are notification-relevant (all" + + " ignored: {2})", + entity.getClass().getSimpleName(), dirtyProperties, ignoredProperties); + return false; + } + + /** + * Gets the set of property names that should be ignored for notification purposes. Properties + * are ignored if they have {@code @JsonIgnore} or {@code @JsonProperty(access = + * JsonProperty.Access.WRITE_ONLY)} annotations. + * + * @param entityClass the entity class to inspect + * @return set of property names to ignore for notifications + */ + private Set getIgnoredPropertyNames(Class entityClass) { + Set ignoredProperties = new HashSet<>(); + + // Inspect all declared fields in the entity class + for (Field field : entityClass.getDeclaredFields()) { + if (field.isAnnotationPresent(JsonIgnore.class)) { + ignoredProperties.add(field.getName()); + continue; + } + + JsonProperty jsonProperty = field.getAnnotation(JsonProperty.class); + if (jsonProperty != null && jsonProperty.access() == JsonProperty.Access.WRITE_ONLY) { + ignoredProperties.add(field.getName()); + } + } + + return ignoredProperties; + } + + /** + * Gets all dirty (modified) property names for the given entity. This is useful for debugging + * and logging purposes. + * + * @param entity the entity to check for dirty properties + * @return set of dirty property names, or empty set if entity is not managed or dirty checking + * is unavailable + */ + public Set getDirtyPropertyNames(Object entity) { + try { + SessionImplementor session = entityManager.unwrap(SessionImplementor.class); + + EntityEntry entityEntry = session.getPersistenceContext().getEntry(entity); + + if (entityEntry == null) { + logger.debugv( + "Entity {0} is not managed, cannot determine dirty properties", + entity.getClass().getSimpleName()); + return Collections.emptySet(); + } + + EntityPersister persister = entityEntry.getPersister(); + String[] propertyNames = persister.getPropertyNames(); + + // Get current and loaded state to determine which properties are dirty + Object[] currentState = persister.getValues(entity); + Object[] loadedState = entityEntry.getLoadedState(); + + if (loadedState == null) { + // No loaded state available (e.g., new entity) + return Collections.emptySet(); + } + + // Build set of dirty property names by comparing current and loaded state + Set dirtyProperties = new HashSet<>(); + for (int i = 0; + i < propertyNames.length && i < currentState.length && i < loadedState.length; + i++) { + Object currentValue = currentState[i]; + Object loadedValue = loadedState[i]; + + // Check if the property value has changed + boolean isDirty = + (currentValue == null && loadedValue != null) + || (currentValue != null && !currentValue.equals(loadedValue)); + + if (isDirty) { + dirtyProperties.add(propertyNames[i]); + } + } + + return dirtyProperties; + + } catch (Exception e) { + logger.warnv( + e, + "Failed to determine dirty properties for entity {0}", + entity.getClass().getSimpleName()); + return Collections.emptySet(); + } + } +} diff --git a/src/test/java/io/cryostat/credentials/CredentialNotificationTest.java b/src/test/java/io/cryostat/credentials/CredentialNotificationTest.java new file mode 100644 index 000000000..0a343fe16 --- /dev/null +++ b/src/test/java/io/cryostat/credentials/CredentialNotificationTest.java @@ -0,0 +1,298 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.credentials; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.time.Duration; +import java.time.Instant; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +import io.cryostat.AbstractTransactionalTestBase; +import io.cryostat.credentials.events.CredentialEvents; +import io.cryostat.expressions.MatchExpression; + +import io.quarkus.narayana.jta.QuarkusTransaction; +import io.quarkus.test.junit.QuarkusTest; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.event.Observes; +import jakarta.enterprise.event.TransactionPhase; +import jakarta.inject.Inject; +import jakarta.transaction.Transactional; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Tests for Credential entity dirty-field filtering implementation. Verifies that only relevant + * field updates trigger notifications. + * + *

The Credential entity uses the NotificationDirtyCheckSupport to filter out updates to fields + * marked with @JsonIgnore or @JsonProperty(access = WRITE_ONLY). This prevents unnecessary + * notifications when only internal tracking fields like expiresAt or lastUsedAt are updated. + */ +@QuarkusTest +class CredentialNotificationTest extends AbstractTransactionalTestBase { + + @Inject CredentialTestService credentialService; + @Inject CredentialEventObserver observer; + + @BeforeEach + void resetObserver() { + observer.reset(); + } + + @Test + void testIgnoredFieldUpdateSuppressed_expiresAt() { + Long credentialId = credentialService.createCredential("target.alias == 'test'"); + + observer.reset(); + + credentialService.updateExpiresAt(credentialId, Instant.now().plusSeconds(3600)); + + assertThrows( + TimeoutException.class, + () -> observer.awaitUpdate(credentialId, Duration.ofSeconds(2)), + "Update notification should not be fired when only expiresAt is changed"); + } + + @Test + void testIgnoredFieldUpdateSuppressed_lastUsedAt() { + Long credentialId = credentialService.createCredential("target.alias == 'test'"); + + observer.reset(); + + credentialService.updateLastUsedAt(credentialId, Instant.now()); + + assertThrows( + TimeoutException.class, + () -> observer.awaitUpdate(credentialId, Duration.ofSeconds(2)), + "Update notification should not be fired when only lastUsedAt is changed"); + } + + @Test + void testMultipleIgnoredFieldUpdatesStillSuppressed() { + Long credentialId = credentialService.createCredential("target.alias == 'test'"); + + observer.reset(); + + credentialService.updateExpiresAtAndLastUsedAt( + credentialId, Instant.now().plusSeconds(3600), Instant.now()); + + assertThrows( + TimeoutException.class, + () -> observer.awaitUpdate(credentialId, Duration.ofSeconds(2)), + "Update notification should not be fired when only ignored fields are changed"); + } + + @Test + void testCreateNotificationUnchanged() throws Exception { + observer.reset(); + + Long credentialId = credentialService.createCredential("target.alias == 'create-test'"); + + CredentialEventObserver.EventCapture capture = + observer.awaitCreate(credentialId, Duration.ofSeconds(5)); + + assertThat(capture, notNullValue()); + assertThat(capture.credentialId(), equalTo(credentialId)); + assertThat(capture.eventType(), equalTo("CREATE")); + } + + @Test + void testDeleteNotificationUnchanged() throws Exception { + Long credentialId = credentialService.createCredential("target.alias == 'delete-test'"); + + observer.reset(); + + credentialService.deleteCredential(credentialId); + + CredentialEventObserver.EventCapture capture = + observer.awaitDelete(credentialId, Duration.ofSeconds(5)); + + assertThat(capture, notNullValue()); + assertThat(capture.credentialId(), equalTo(credentialId)); + assertThat(capture.eventType(), equalTo("DELETE")); + } + + @Test + void testUpdateNotificationNotFiredOnRollback() { + Long credentialId = credentialService.createCredential("target.alias == 'test'"); + + observer.reset(); + + assertThrows( + RuntimeException.class, + () -> + credentialService.updateExpiresAtAndRollback( + credentialId, Instant.now().plusSeconds(3600))); + + assertThrows( + TimeoutException.class, + () -> observer.awaitUpdate(credentialId, Duration.ofSeconds(2)), + "Update notification should not be fired when transaction is rolled back"); + + Instant expiresAt = + QuarkusTransaction.requiringNew() + .call( + () -> { + Credential credential = Credential.findById(credentialId); + return credential.expiresAt; + }); + assertThat("expiresAt should not have changed after rollback", expiresAt, equalTo(null)); + } + + @ApplicationScoped + static class CredentialTestService { + + @Transactional + public Long createCredential(String matchExpressionScript) { + MatchExpression matchExpression = new MatchExpression(matchExpressionScript); + matchExpression.persist(); + + Credential credential = new Credential(); + credential.matchExpression = matchExpression; + credential.username = "testuser"; + credential.password = "testpass"; + credential.persist(); + + return credential.id; + } + + @Transactional + public void updateExpiresAt(Long credentialId, Instant expiresAt) { + Credential credential = Credential.findById(credentialId); + if (credential == null) { + throw new IllegalArgumentException("Credential not found: " + credentialId); + } + + credential.expiresAt = expiresAt; + credential.persist(); + } + + @Transactional + public void updateLastUsedAt(Long credentialId, Instant lastUsedAt) { + Credential credential = Credential.findById(credentialId); + if (credential == null) { + throw new IllegalArgumentException("Credential not found: " + credentialId); + } + + credential.lastUsedAt = lastUsedAt; + credential.persist(); + } + + @Transactional + public void updateExpiresAtAndLastUsedAt( + Long credentialId, Instant expiresAt, Instant lastUsedAt) { + Credential credential = Credential.findById(credentialId); + if (credential == null) { + throw new IllegalArgumentException("Credential not found: " + credentialId); + } + + credential.expiresAt = expiresAt; + credential.lastUsedAt = lastUsedAt; + credential.persist(); + } + + @Transactional + public void deleteCredential(Long credentialId) { + Credential credential = Credential.findById(credentialId); + if (credential == null) { + throw new IllegalArgumentException("Credential not found: " + credentialId); + } + + credential.delete(); + } + + @Transactional + public void updateExpiresAtAndRollback(Long credentialId, Instant expiresAt) { + Credential credential = Credential.findById(credentialId); + if (credential == null) { + throw new IllegalArgumentException("Credential not found: " + credentialId); + } + + credential.expiresAt = expiresAt; + credential.persist(); + + throw new RuntimeException("force rollback"); + } + } + + @ApplicationScoped + static class CredentialEventObserver { + + private volatile CompletableFuture createFuture = new CompletableFuture<>(); + private volatile CompletableFuture updateFuture = new CompletableFuture<>(); + private volatile CompletableFuture deleteFuture = new CompletableFuture<>(); + + void onCredentialCreated( + @Observes(during = TransactionPhase.AFTER_SUCCESS) + CredentialEvents.CredentialCreated event) { + createFuture.complete(new EventCapture(event.getEntityId(), "CREATE")); + } + + void onCredentialUpdated( + @Observes(during = TransactionPhase.AFTER_SUCCESS) + CredentialEvents.CredentialUpdated event) { + updateFuture.complete(new EventCapture(event.getEntityId(), "UPDATE")); + } + + void onCredentialDeleted( + @Observes(during = TransactionPhase.AFTER_SUCCESS) + CredentialEvents.CredentialDeleted event) { + deleteFuture.complete(new EventCapture(event.getEntityId(), "DELETE")); + } + + void reset() { + createFuture = new CompletableFuture<>(); + updateFuture = new CompletableFuture<>(); + deleteFuture = new CompletableFuture<>(); + } + + EventCapture awaitCreate(Long expectedId, Duration timeout) throws Exception { + EventCapture capture = createFuture.get(timeout.toMillis(), TimeUnit.MILLISECONDS); + if (!expectedId.equals(capture.credentialId())) { + throw new TimeoutException( + "Observed unexpected credential ID " + capture.credentialId()); + } + return capture; + } + + EventCapture awaitUpdate(Long expectedId, Duration timeout) throws Exception { + EventCapture capture = updateFuture.get(timeout.toMillis(), TimeUnit.MILLISECONDS); + if (!expectedId.equals(capture.credentialId())) { + throw new TimeoutException( + "Observed unexpected credential ID " + capture.credentialId()); + } + return capture; + } + + EventCapture awaitDelete(Long expectedId, Duration timeout) throws Exception { + EventCapture capture = deleteFuture.get(timeout.toMillis(), TimeUnit.MILLISECONDS); + if (!expectedId.equals(capture.credentialId())) { + throw new TimeoutException( + "Observed unexpected credential ID " + capture.credentialId()); + } + return capture; + } + + record EventCapture(Long credentialId, String eventType) {} + } +} From 4592e4404647885b2572315a877f8504537bcb7c Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Thu, 23 Apr 2026 12:02:42 -0400 Subject: [PATCH 18/24] unschedule ActiveRecordingUpdateJob on NoResultException --- .../targets/ActiveRecordingUpdateJob.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/cryostat/targets/ActiveRecordingUpdateJob.java b/src/main/java/io/cryostat/targets/ActiveRecordingUpdateJob.java index da9e4ee8f..9a04980f1 100644 --- a/src/main/java/io/cryostat/targets/ActiveRecordingUpdateJob.java +++ b/src/main/java/io/cryostat/targets/ActiveRecordingUpdateJob.java @@ -19,8 +19,10 @@ import io.cryostat.recordings.RecordingHelper; import jakarta.inject.Inject; +import jakarta.persistence.NoResultException; import jakarta.persistence.PersistenceException; import jakarta.transaction.Transactional; +import org.hibernate.ObjectDeletedException; import org.jboss.logging.Logger; import org.quartz.DisallowConcurrentExecution; import org.quartz.Job; @@ -49,10 +51,17 @@ public void execute(JobExecutionContext context) throws JobExecutionException { Target target; try { target = Target.getTargetById(recording.target.id); - } catch (PersistenceException e) { - // the target was lost in the meantime, so we can stop worrying about this update + } catch (NoResultException | ObjectDeletedException e) { + // target disappeared in the meantime. No big deal. logger.debug(e); - return; + JobExecutionException ex = new JobExecutionException(e); + ex.setRefireImmediately(false); + ex.setUnscheduleFiringTrigger(true); + throw ex; + } catch (PersistenceException e) { + JobExecutionException ex = new JobExecutionException(e); + ex.setRefireImmediately(false); + throw ex; } // FIXME hacky. This opens a remote connection on each call and updates our database with // the data we find there. We should have some remote connection callback (JMX listener, From 283c619e4b2073be765a783a3c458d2192c5416d Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Thu, 23 Apr 2026 15:38:15 -0400 Subject: [PATCH 19/24] reduce plugin ping period and max failures --- src/main/resources/application.properties | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index ca8170857..b226b0d6e 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -32,8 +32,8 @@ cryostat.discovery.containers.poll-period=10s cryostat.discovery.containers.request-timeout=2s cryostat.discovery.podman.enabled=false cryostat.discovery.docker.enabled=false -cryostat.discovery.plugins.ping-period=5m -cryostat.discovery.plugins.max-failures=3 +cryostat.discovery.plugins.ping-period=1m +cryostat.discovery.plugins.max-failures=2 cryostat.discovery.plugins.max-backoff-multiplier=8 cryostat.discovery.plugins.jwt.secret.algorithm=AES cryostat.discovery.plugins.jwt.secret.keysize=256 From 6c39e95cc800d02366604dab3f75bbfa0dc8c2f9 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 24 Apr 2026 15:44:15 -0400 Subject: [PATCH 20/24] remove expiresAt, lastUsedAt, expiration job --- .../java/io/cryostat/ConfigProperties.java | 1 - .../io/cryostat/credentials/Credential.java | 56 +--- .../credentials/CredentialExpirationJob.java | 50 --- .../cryostat/discovery/DiscoveryPlugin.java | 9 - .../events/NotificationDirtyCheckSupport.java | 178 ----------- .../java/io/cryostat/targets/AgentClient.java | 3 - src/main/resources/application.properties | 1 - .../db/migration/V4.2.0__cryostat.sql | 4 - .../CredentialExpirationCleanupTest.java | 106 ------- .../CredentialNotificationTest.java | 298 ------------------ .../cryostat/credentials/CredentialsTest.java | 68 ---- .../DiscoveryPluginGracePeriodTest.java | 65 ---- 12 files changed, 3 insertions(+), 836 deletions(-) delete mode 100644 src/main/java/io/cryostat/credentials/CredentialExpirationJob.java delete mode 100644 src/main/java/io/cryostat/events/NotificationDirtyCheckSupport.java delete mode 100644 src/test/java/io/cryostat/credentials/CredentialExpirationCleanupTest.java delete mode 100644 src/test/java/io/cryostat/credentials/CredentialNotificationTest.java diff --git a/src/main/java/io/cryostat/ConfigProperties.java b/src/main/java/io/cryostat/ConfigProperties.java index 23adfc957..8c3a59b47 100644 --- a/src/main/java/io/cryostat/ConfigProperties.java +++ b/src/main/java/io/cryostat/ConfigProperties.java @@ -108,7 +108,6 @@ public class ConfigProperties { public static final String URI_RANGE = "cryostat.target.uri-range"; public static final String AGENT_TLS_REQUIRED = "cryostat.agent.tls.required"; - public static final String CREDENTIAL_EXPIRATION = "cryostat.credentials.expiration"; public static final String DECLARATIVE_CONFIG_RESOLVE_SYMLINKS = "cryostat.declarative-configuration.symlinks.resolve"; diff --git a/src/main/java/io/cryostat/credentials/Credential.java b/src/main/java/io/cryostat/credentials/Credential.java index c380839bc..4231d9532 100644 --- a/src/main/java/io/cryostat/credentials/Credential.java +++ b/src/main/java/io/cryostat/credentials/Credential.java @@ -15,15 +15,8 @@ */ package io.cryostat.credentials; -import java.time.Duration; -import java.time.Instant; -import java.util.List; - -import io.cryostat.ConfigProperties; import io.cryostat.credentials.events.CredentialEvents; import io.cryostat.discovery.DiscoveryPlugin; -import io.cryostat.discovery.InstantConverter; -import io.cryostat.events.NotificationDirtyCheckSupport; import io.cryostat.expressions.MatchExpression; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -36,22 +29,16 @@ import jakarta.persistence.Cacheable; import jakarta.persistence.CascadeType; import jakarta.persistence.Column; -import jakarta.persistence.Convert; import jakarta.persistence.Entity; import jakarta.persistence.EntityListeners; import jakarta.persistence.FetchType; import jakarta.persistence.JoinColumn; -import jakarta.persistence.NamedQueries; -import jakarta.persistence.NamedQuery; import jakarta.persistence.OneToOne; import jakarta.persistence.PostPersist; import jakarta.persistence.PostRemove; import jakarta.persistence.PostUpdate; -import jakarta.persistence.PrePersist; -import jakarta.persistence.PreUpdate; import jakarta.validation.constraints.NotBlank; import jakarta.validation.constraints.NotNull; -import org.eclipse.microprofile.config.inject.ConfigProperty; import org.hibernate.annotations.Cache; import org.hibernate.annotations.CacheConcurrencyStrategy; import org.hibernate.annotations.ColumnTransformer; @@ -76,11 +63,6 @@ @Entity @EntityListeners(Credential.Listener.class) @Cacheable -@NamedQueries({ - @NamedQuery( - name = "Credential.findExpired", - query = "SELECT c FROM Credential c WHERE c.expiresAt IS NOT NULL AND c.expiresAt < ?1") -}) public class Credential extends PanacheEntity { public static final String CREDENTIALS_STORED = "CredentialsStored"; @@ -147,41 +129,11 @@ public class Credential extends PanacheEntity { @Nullable public DiscoveryPlugin discoveryPlugin; - @Column(nullable = true) - @Convert(converter = InstantConverter.class) - @JsonIgnore - public Instant expiresAt; - - @Column(nullable = true) - @Convert(converter = InstantConverter.class) - @JsonIgnore - public Instant lastUsedAt; - - public static List findExpired() { - return findExpired(Instant.now()); - } - - public static List findExpired(Instant cutoff) { - return Credential.find("#Credential.findExpired", cutoff).list(); - } - @ApplicationScoped static class Listener { - @ConfigProperty(name = ConfigProperties.CREDENTIAL_EXPIRATION) - Duration credentialExpiration; - @Inject Event credentialCreatedEvent; @Inject Event credentialUpdatedEvent; @Inject Event credentialDeletedEvent; - @Inject NotificationDirtyCheckSupport dirtyCheckSupport; - - @PrePersist - @PreUpdate - public void applyDiscoveryPluginExpiry(Credential credential) { - if (credential.discoveryPlugin != null && credential.expiresAt == null) { - credential.expiresAt = Instant.now().plus(credentialExpiration); - } - } @PostPersist public void postPersist(Credential credential) { @@ -192,11 +144,9 @@ public void postPersist(Credential credential) { @PostUpdate public void postUpdate(Credential credential) { - if (dirtyCheckSupport.hasRelevantDirtyProperties(credential)) { - CredentialEvents.CredentialSnapshot snapshot = createSnapshot(credential); - credentialUpdatedEvent.fire( - new CredentialEvents.CredentialUpdated(credential.id, snapshot)); - } + CredentialEvents.CredentialSnapshot snapshot = createSnapshot(credential); + credentialUpdatedEvent.fire( + new CredentialEvents.CredentialUpdated(credential.id, snapshot)); } @PostRemove diff --git a/src/main/java/io/cryostat/credentials/CredentialExpirationJob.java b/src/main/java/io/cryostat/credentials/CredentialExpirationJob.java deleted file mode 100644 index 4843d6a87..000000000 --- a/src/main/java/io/cryostat/credentials/CredentialExpirationJob.java +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Copyright The Cryostat Authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.cryostat.credentials; - -import java.util.List; - -import io.cryostat.ConfigProperties; -import io.cryostat.discovery.DiscoveryPlugin; - -import io.quarkus.scheduler.Scheduled; -import jakarta.enterprise.context.ApplicationScoped; -import jakarta.inject.Inject; -import jakarta.transaction.Transactional; -import org.eclipse.microprofile.config.inject.ConfigProperty; -import org.jboss.logging.Logger; - -@ApplicationScoped -public class CredentialExpirationJob { - - @Inject Logger logger; - - @ConfigProperty(name = ConfigProperties.CREDENTIAL_EXPIRATION) - String credentialExpiration; - - @Scheduled(every = "${" + ConfigProperties.CREDENTIAL_EXPIRATION + "}") - @Transactional - void cleanupExpiredCredentials() { - List expired = Credential.findExpired(); - for (Credential credential : expired) { - long pluginCount = DiscoveryPlugin.count("credential", credential); - if (pluginCount == 0) { - logger.infov("Deleting expired credential {0}", credential.id); - credential.delete(); - } - } - } -} diff --git a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java index 21d5f7eb0..bc2d67858 100644 --- a/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java +++ b/src/main/java/io/cryostat/discovery/DiscoveryPlugin.java @@ -17,13 +17,11 @@ import java.net.URI; import java.net.URISyntaxException; -import java.time.Duration; import java.time.Instant; import java.util.Optional; import java.util.UUID; import java.util.function.Supplier; -import io.cryostat.ConfigProperties; import io.cryostat.credentials.Credential; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -59,7 +57,6 @@ import jakarta.ws.rs.core.MultivaluedMap; import jakarta.ws.rs.core.UriBuilder; import org.apache.commons.lang3.StringUtils; -import org.eclipse.microprofile.config.inject.ConfigProperty; import org.hibernate.annotations.Cache; import org.hibernate.annotations.CacheConcurrencyStrategy; import org.hibernate.annotations.UuidGenerator; @@ -155,9 +152,6 @@ static class Listener { @Inject Logger logger; @Inject PluginCallbackFactory callbackFactory; - @ConfigProperty(name = ConfigProperties.CREDENTIAL_EXPIRATION) - Duration credentialExpiration; - @PrePersist @Transactional public void prePersist(DiscoveryPlugin plugin) { @@ -171,9 +165,6 @@ public void prePersist(DiscoveryPlugin plugin) { var credential = getCredential(plugin); plugin.credential = credential; credential.discoveryPlugin = plugin; - if (credential.expiresAt == null) { - credential.expiresAt = Instant.now().plus(credentialExpiration); - } plugin.callback = UriBuilder.fromUri(plugin.callback).userInfo(null).build(); } if (plugin.nextPingAt != null diff --git a/src/main/java/io/cryostat/events/NotificationDirtyCheckSupport.java b/src/main/java/io/cryostat/events/NotificationDirtyCheckSupport.java deleted file mode 100644 index d0f491267..000000000 --- a/src/main/java/io/cryostat/events/NotificationDirtyCheckSupport.java +++ /dev/null @@ -1,178 +0,0 @@ -/* - * Copyright The Cryostat Authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.cryostat.events; - -import java.lang.reflect.Field; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; - -import com.fasterxml.jackson.annotation.JsonIgnore; -import com.fasterxml.jackson.annotation.JsonProperty; -import jakarta.enterprise.context.ApplicationScoped; -import jakarta.inject.Inject; -import jakarta.persistence.EntityManager; -import org.hibernate.engine.spi.EntityEntry; -import org.hibernate.engine.spi.SessionImplementor; -import org.hibernate.persister.entity.EntityPersister; -import org.jboss.logging.Logger; - -/** - * Utility class for checking which entity properties have been modified (dirty) in the current - * Hibernate session. This is used to determine whether entity updates should trigger WebSocket - * notifications based on which specific fields were changed. - * - *

This class provides methods to: - * - *

    - *
  • Check if any notification-relevant properties are dirty using Jackson annotation reflection - *
  • Get all dirty property names for debugging purposes - *
- * - *

The dirty-checking mechanism uses Hibernate's internal APIs to access the entity's dirty state - * metadata from the persistence context. Fields are considered notification-relevant unless they - * are marked with {@code @JsonIgnore} or {@code @JsonProperty(access = - * JsonProperty.Access.WRITE_ONLY)}. This approach automatically adapts to field additions, - * removals, and renames without requiring manual updates to property name lists. - */ -@ApplicationScoped -public class NotificationDirtyCheckSupport { - - @Inject EntityManager entityManager; - @Inject Logger logger; - - /** - * Checks if any notification-relevant properties are dirty (modified) for the given entity. A - * property is considered notification-relevant unless it is marked with {@code @JsonIgnore} or - * {@code @JsonProperty(access = JsonProperty.Access.WRITE_ONLY)}. - * - * @param entity the entity to check for dirty properties - * @return true if any notification-relevant property is dirty, false otherwise or if dirty - * checking is unavailable - */ - public boolean hasRelevantDirtyProperties(Object entity) { - Set dirtyProperties = getDirtyPropertyNames(entity); - - if (dirtyProperties.isEmpty()) { - return false; - } - - Set ignoredProperties = getIgnoredPropertyNames(entity.getClass()); - for (String dirtyProperty : dirtyProperties) { - if (!ignoredProperties.contains(dirtyProperty)) { - logger.tracev( - "Entity {0} has notification-relevant dirty property: {1}", - entity.getClass().getSimpleName(), dirtyProperty); - return true; - } - } - - logger.tracev( - "Entity {0} has dirty properties {1} but none are notification-relevant (all" - + " ignored: {2})", - entity.getClass().getSimpleName(), dirtyProperties, ignoredProperties); - return false; - } - - /** - * Gets the set of property names that should be ignored for notification purposes. Properties - * are ignored if they have {@code @JsonIgnore} or {@code @JsonProperty(access = - * JsonProperty.Access.WRITE_ONLY)} annotations. - * - * @param entityClass the entity class to inspect - * @return set of property names to ignore for notifications - */ - private Set getIgnoredPropertyNames(Class entityClass) { - Set ignoredProperties = new HashSet<>(); - - // Inspect all declared fields in the entity class - for (Field field : entityClass.getDeclaredFields()) { - if (field.isAnnotationPresent(JsonIgnore.class)) { - ignoredProperties.add(field.getName()); - continue; - } - - JsonProperty jsonProperty = field.getAnnotation(JsonProperty.class); - if (jsonProperty != null && jsonProperty.access() == JsonProperty.Access.WRITE_ONLY) { - ignoredProperties.add(field.getName()); - } - } - - return ignoredProperties; - } - - /** - * Gets all dirty (modified) property names for the given entity. This is useful for debugging - * and logging purposes. - * - * @param entity the entity to check for dirty properties - * @return set of dirty property names, or empty set if entity is not managed or dirty checking - * is unavailable - */ - public Set getDirtyPropertyNames(Object entity) { - try { - SessionImplementor session = entityManager.unwrap(SessionImplementor.class); - - EntityEntry entityEntry = session.getPersistenceContext().getEntry(entity); - - if (entityEntry == null) { - logger.debugv( - "Entity {0} is not managed, cannot determine dirty properties", - entity.getClass().getSimpleName()); - return Collections.emptySet(); - } - - EntityPersister persister = entityEntry.getPersister(); - String[] propertyNames = persister.getPropertyNames(); - - // Get current and loaded state to determine which properties are dirty - Object[] currentState = persister.getValues(entity); - Object[] loadedState = entityEntry.getLoadedState(); - - if (loadedState == null) { - // No loaded state available (e.g., new entity) - return Collections.emptySet(); - } - - // Build set of dirty property names by comparing current and loaded state - Set dirtyProperties = new HashSet<>(); - for (int i = 0; - i < propertyNames.length && i < currentState.length && i < loadedState.length; - i++) { - Object currentValue = currentState[i]; - Object loadedValue = loadedState[i]; - - // Check if the property value has changed - boolean isDirty = - (currentValue == null && loadedValue != null) - || (currentValue != null && !currentValue.equals(loadedValue)); - - if (isDirty) { - dirtyProperties.add(propertyNames[i]); - } - } - - return dirtyProperties; - - } catch (Exception e) { - logger.warnv( - e, - "Failed to determine dirty properties for entity {0}", - entity.getClass().getSimpleName()); - return Collections.emptySet(); - } - } -} diff --git a/src/main/java/io/cryostat/targets/AgentClient.java b/src/main/java/io/cryostat/targets/AgentClient.java index 744a321b0..a6daaee61 100644 --- a/src/main/java/io/cryostat/targets/AgentClient.java +++ b/src/main/java/io/cryostat/targets/AgentClient.java @@ -21,7 +21,6 @@ import java.io.InputStream; import java.net.URI; import java.time.Duration; -import java.time.Instant; import java.util.Arrays; import java.util.Collection; import java.util.HashMap; @@ -646,8 +645,6 @@ public AgentClient create(Target target) { if (credential == null) { throw new ConnectionException(NULL_CREDENTIALS); } - credential.lastUsedAt = Instant.now(); - credential.persist(); return new UsernamePasswordCredentials( credential.username, credential.password); }); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index b226b0d6e..dc7dc5810 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -91,7 +91,6 @@ cryostat.http.proxy.path=/ cryostat.target.uri-range=PUBLIC cryostat.agent.tls.required=true -cryostat.credentials.expiration=1h # these rest-client urls should always be overridden quarkus.rest-client.reports.url=http://localhost/ diff --git a/src/main/resources/db/migration/V4.2.0__cryostat.sql b/src/main/resources/db/migration/V4.2.0__cryostat.sql index 5d7befa00..2d182380e 100644 --- a/src/main/resources/db/migration/V4.2.0__cryostat.sql +++ b/src/main/resources/db/migration/V4.2.0__cryostat.sql @@ -333,8 +333,6 @@ CREATE TABLE Credential_AUD ( matchExpression BIGINT, username BYTEA, password BYTEA, - expiresAt BIGINT, - lastUsedAt BIGINT, PRIMARY KEY (id, REV), FOREIGN KEY (REV) REFERENCES REVINFO (REV), FOREIGN KEY (REVEND) REFERENCES REVINFO (REV) @@ -651,8 +649,6 @@ ALTER TABLE DiscoveryPlugin ADD COLUMN lastSuccessfulPing BIGINT; ALTER TABLE DiscoveryPlugin ADD COLUMN lastFailedPing BIGINT; ALTER TABLE DiscoveryPlugin ADD COLUMN backoffMultiplier INTEGER NOT NULL DEFAULT 1; ALTER TABLE DiscoveryPlugin ADD COLUMN nextPingAt BIGINT; -ALTER TABLE Credential ADD COLUMN expiresAt BIGINT; -ALTER TABLE Credential ADD COLUMN lastUsedAt BIGINT; -- Add unique constraint on DiscoveryNode name for Realm nodes to enforce realm name uniqueness CREATE UNIQUE INDEX uk_discovery_node_realm_name ON DiscoveryNode (name) WHERE (nodeType = 'Realm'); diff --git a/src/test/java/io/cryostat/credentials/CredentialExpirationCleanupTest.java b/src/test/java/io/cryostat/credentials/CredentialExpirationCleanupTest.java deleted file mode 100644 index 729c3cafe..000000000 --- a/src/test/java/io/cryostat/credentials/CredentialExpirationCleanupTest.java +++ /dev/null @@ -1,106 +0,0 @@ -/* - * Copyright The Cryostat Authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.cryostat.credentials; - -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; - -import java.net.URI; -import java.time.Instant; - -import io.cryostat.AbstractTransactionalTestBase; -import io.cryostat.discovery.DiscoveryNode; -import io.cryostat.discovery.DiscoveryPlugin; -import io.cryostat.discovery.NodeType; -import io.cryostat.expressions.MatchExpression; - -import io.quarkus.narayana.jta.QuarkusTransaction; -import io.quarkus.test.junit.QuarkusTest; -import jakarta.inject.Inject; -import org.junit.jupiter.api.Test; - -@QuarkusTest -public class CredentialExpirationCleanupTest extends AbstractTransactionalTestBase { - - @Inject CredentialExpirationJob credentialExpirationJob; - - @Test - void deletesExpiredUnassociatedCredentials() { - long credentialId = - QuarkusTransaction.requiringNew() - .call( - () -> { - var expr = new MatchExpression("true"); - expr.persist(); - - var credential = new Credential(); - credential.matchExpression = expr; - credential.username = "user"; - credential.password = "pass"; - credential.expiresAt = Instant.now().minusSeconds(10); - credential.persist(); - - return credential.id; - }); - - credentialExpirationJob.cleanupExpiredCredentials(); - - var deleted = - QuarkusTransaction.requiringNew() - .call(() -> Credential.findById(credentialId)); - assertNull(deleted); - } - - @Test - void doesNotDeleteExpiredCredentialsStillAssociatedWithPlugin() { - long credentialId = - QuarkusTransaction.requiringNew() - .call( - () -> { - var expr = new MatchExpression("true"); - expr.persist(); - - var credential = new Credential(); - credential.matchExpression = expr; - credential.username = "user"; - credential.password = "pass"; - credential.expiresAt = Instant.now().minusSeconds(10); - credential.persist(); - - var realm = new DiscoveryNode(); - realm.name = "cleanup_test_realm"; - realm.nodeType = NodeType.BaseNodeType.REALM.getKind(); - realm.persist(); - - var plugin = new DiscoveryPlugin(); - plugin.realm = realm; - plugin.callback = - URI.create("http://localhost:8081/health/liveness"); - plugin.builtin = false; - plugin.credential = credential; - plugin.persist(); - - return credential.id; - }); - - credentialExpirationJob.cleanupExpiredCredentials(); - - var retained = - QuarkusTransaction.requiringNew() - .call(() -> Credential.findById(credentialId)); - assertNotNull(retained); - } -} diff --git a/src/test/java/io/cryostat/credentials/CredentialNotificationTest.java b/src/test/java/io/cryostat/credentials/CredentialNotificationTest.java deleted file mode 100644 index 0a343fe16..000000000 --- a/src/test/java/io/cryostat/credentials/CredentialNotificationTest.java +++ /dev/null @@ -1,298 +0,0 @@ -/* - * Copyright The Cryostat Authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.cryostat.credentials; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.notNullValue; -import static org.junit.jupiter.api.Assertions.assertThrows; - -import java.time.Duration; -import java.time.Instant; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; - -import io.cryostat.AbstractTransactionalTestBase; -import io.cryostat.credentials.events.CredentialEvents; -import io.cryostat.expressions.MatchExpression; - -import io.quarkus.narayana.jta.QuarkusTransaction; -import io.quarkus.test.junit.QuarkusTest; -import jakarta.enterprise.context.ApplicationScoped; -import jakarta.enterprise.event.Observes; -import jakarta.enterprise.event.TransactionPhase; -import jakarta.inject.Inject; -import jakarta.transaction.Transactional; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -/** - * Tests for Credential entity dirty-field filtering implementation. Verifies that only relevant - * field updates trigger notifications. - * - *

The Credential entity uses the NotificationDirtyCheckSupport to filter out updates to fields - * marked with @JsonIgnore or @JsonProperty(access = WRITE_ONLY). This prevents unnecessary - * notifications when only internal tracking fields like expiresAt or lastUsedAt are updated. - */ -@QuarkusTest -class CredentialNotificationTest extends AbstractTransactionalTestBase { - - @Inject CredentialTestService credentialService; - @Inject CredentialEventObserver observer; - - @BeforeEach - void resetObserver() { - observer.reset(); - } - - @Test - void testIgnoredFieldUpdateSuppressed_expiresAt() { - Long credentialId = credentialService.createCredential("target.alias == 'test'"); - - observer.reset(); - - credentialService.updateExpiresAt(credentialId, Instant.now().plusSeconds(3600)); - - assertThrows( - TimeoutException.class, - () -> observer.awaitUpdate(credentialId, Duration.ofSeconds(2)), - "Update notification should not be fired when only expiresAt is changed"); - } - - @Test - void testIgnoredFieldUpdateSuppressed_lastUsedAt() { - Long credentialId = credentialService.createCredential("target.alias == 'test'"); - - observer.reset(); - - credentialService.updateLastUsedAt(credentialId, Instant.now()); - - assertThrows( - TimeoutException.class, - () -> observer.awaitUpdate(credentialId, Duration.ofSeconds(2)), - "Update notification should not be fired when only lastUsedAt is changed"); - } - - @Test - void testMultipleIgnoredFieldUpdatesStillSuppressed() { - Long credentialId = credentialService.createCredential("target.alias == 'test'"); - - observer.reset(); - - credentialService.updateExpiresAtAndLastUsedAt( - credentialId, Instant.now().plusSeconds(3600), Instant.now()); - - assertThrows( - TimeoutException.class, - () -> observer.awaitUpdate(credentialId, Duration.ofSeconds(2)), - "Update notification should not be fired when only ignored fields are changed"); - } - - @Test - void testCreateNotificationUnchanged() throws Exception { - observer.reset(); - - Long credentialId = credentialService.createCredential("target.alias == 'create-test'"); - - CredentialEventObserver.EventCapture capture = - observer.awaitCreate(credentialId, Duration.ofSeconds(5)); - - assertThat(capture, notNullValue()); - assertThat(capture.credentialId(), equalTo(credentialId)); - assertThat(capture.eventType(), equalTo("CREATE")); - } - - @Test - void testDeleteNotificationUnchanged() throws Exception { - Long credentialId = credentialService.createCredential("target.alias == 'delete-test'"); - - observer.reset(); - - credentialService.deleteCredential(credentialId); - - CredentialEventObserver.EventCapture capture = - observer.awaitDelete(credentialId, Duration.ofSeconds(5)); - - assertThat(capture, notNullValue()); - assertThat(capture.credentialId(), equalTo(credentialId)); - assertThat(capture.eventType(), equalTo("DELETE")); - } - - @Test - void testUpdateNotificationNotFiredOnRollback() { - Long credentialId = credentialService.createCredential("target.alias == 'test'"); - - observer.reset(); - - assertThrows( - RuntimeException.class, - () -> - credentialService.updateExpiresAtAndRollback( - credentialId, Instant.now().plusSeconds(3600))); - - assertThrows( - TimeoutException.class, - () -> observer.awaitUpdate(credentialId, Duration.ofSeconds(2)), - "Update notification should not be fired when transaction is rolled back"); - - Instant expiresAt = - QuarkusTransaction.requiringNew() - .call( - () -> { - Credential credential = Credential.findById(credentialId); - return credential.expiresAt; - }); - assertThat("expiresAt should not have changed after rollback", expiresAt, equalTo(null)); - } - - @ApplicationScoped - static class CredentialTestService { - - @Transactional - public Long createCredential(String matchExpressionScript) { - MatchExpression matchExpression = new MatchExpression(matchExpressionScript); - matchExpression.persist(); - - Credential credential = new Credential(); - credential.matchExpression = matchExpression; - credential.username = "testuser"; - credential.password = "testpass"; - credential.persist(); - - return credential.id; - } - - @Transactional - public void updateExpiresAt(Long credentialId, Instant expiresAt) { - Credential credential = Credential.findById(credentialId); - if (credential == null) { - throw new IllegalArgumentException("Credential not found: " + credentialId); - } - - credential.expiresAt = expiresAt; - credential.persist(); - } - - @Transactional - public void updateLastUsedAt(Long credentialId, Instant lastUsedAt) { - Credential credential = Credential.findById(credentialId); - if (credential == null) { - throw new IllegalArgumentException("Credential not found: " + credentialId); - } - - credential.lastUsedAt = lastUsedAt; - credential.persist(); - } - - @Transactional - public void updateExpiresAtAndLastUsedAt( - Long credentialId, Instant expiresAt, Instant lastUsedAt) { - Credential credential = Credential.findById(credentialId); - if (credential == null) { - throw new IllegalArgumentException("Credential not found: " + credentialId); - } - - credential.expiresAt = expiresAt; - credential.lastUsedAt = lastUsedAt; - credential.persist(); - } - - @Transactional - public void deleteCredential(Long credentialId) { - Credential credential = Credential.findById(credentialId); - if (credential == null) { - throw new IllegalArgumentException("Credential not found: " + credentialId); - } - - credential.delete(); - } - - @Transactional - public void updateExpiresAtAndRollback(Long credentialId, Instant expiresAt) { - Credential credential = Credential.findById(credentialId); - if (credential == null) { - throw new IllegalArgumentException("Credential not found: " + credentialId); - } - - credential.expiresAt = expiresAt; - credential.persist(); - - throw new RuntimeException("force rollback"); - } - } - - @ApplicationScoped - static class CredentialEventObserver { - - private volatile CompletableFuture createFuture = new CompletableFuture<>(); - private volatile CompletableFuture updateFuture = new CompletableFuture<>(); - private volatile CompletableFuture deleteFuture = new CompletableFuture<>(); - - void onCredentialCreated( - @Observes(during = TransactionPhase.AFTER_SUCCESS) - CredentialEvents.CredentialCreated event) { - createFuture.complete(new EventCapture(event.getEntityId(), "CREATE")); - } - - void onCredentialUpdated( - @Observes(during = TransactionPhase.AFTER_SUCCESS) - CredentialEvents.CredentialUpdated event) { - updateFuture.complete(new EventCapture(event.getEntityId(), "UPDATE")); - } - - void onCredentialDeleted( - @Observes(during = TransactionPhase.AFTER_SUCCESS) - CredentialEvents.CredentialDeleted event) { - deleteFuture.complete(new EventCapture(event.getEntityId(), "DELETE")); - } - - void reset() { - createFuture = new CompletableFuture<>(); - updateFuture = new CompletableFuture<>(); - deleteFuture = new CompletableFuture<>(); - } - - EventCapture awaitCreate(Long expectedId, Duration timeout) throws Exception { - EventCapture capture = createFuture.get(timeout.toMillis(), TimeUnit.MILLISECONDS); - if (!expectedId.equals(capture.credentialId())) { - throw new TimeoutException( - "Observed unexpected credential ID " + capture.credentialId()); - } - return capture; - } - - EventCapture awaitUpdate(Long expectedId, Duration timeout) throws Exception { - EventCapture capture = updateFuture.get(timeout.toMillis(), TimeUnit.MILLISECONDS); - if (!expectedId.equals(capture.credentialId())) { - throw new TimeoutException( - "Observed unexpected credential ID " + capture.credentialId()); - } - return capture; - } - - EventCapture awaitDelete(Long expectedId, Duration timeout) throws Exception { - EventCapture capture = deleteFuture.get(timeout.toMillis(), TimeUnit.MILLISECONDS); - if (!expectedId.equals(capture.credentialId())) { - throw new TimeoutException( - "Observed unexpected credential ID " + capture.credentialId()); - } - return capture; - } - - record EventCapture(Long credentialId, String eventType) {} - } -} diff --git a/src/test/java/io/cryostat/credentials/CredentialsTest.java b/src/test/java/io/cryostat/credentials/CredentialsTest.java index 50e7a8ec7..c6b1028c3 100644 --- a/src/test/java/io/cryostat/credentials/CredentialsTest.java +++ b/src/test/java/io/cryostat/credentials/CredentialsTest.java @@ -16,19 +16,15 @@ package io.cryostat.credentials; import static io.restassured.RestAssured.given; -import static org.junit.jupiter.api.Assertions.assertNull; -import java.time.Instant; import java.util.List; import io.cryostat.AbstractTransactionalTestBase; -import io.quarkus.narayana.jta.QuarkusTransaction; import io.quarkus.test.common.http.TestHTTPEndpoint; import io.quarkus.test.junit.QuarkusTest; import io.restassured.http.ContentType; import org.hamcrest.Matchers; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @QuarkusTest @@ -162,70 +158,6 @@ public void testCredentialCheck() { Matchers.matchesRegex("[\\s]*\"NA\"[\\s]*")); } - @Test - void testCreatedCredentialsDoNotExpireByDefault() { - int id = createTestCredential(); - - var credential = - QuarkusTransaction.requiringNew() - .call(() -> Credential.findById((long) id)); - - assertNull(credential.expiresAt); - assertNull(credential.lastUsedAt); - } - - @Test - void testPluginCredentialGetsDefaultExpiryWhenAssociated() { - var credentialId = - given().log() - .all() - .contentType(ContentType.URLENC) - .formParam( - "matchExpression", - "target.connectUrl == 'http://localhost:8081/health/liveness'") - .formParam("username", "user") - .formParam("password", "pass") - .when() - .post() - .then() - .log() - .all() - .and() - .assertThat() - .statusCode(201) - .contentType(ContentType.JSON) - .extract() - .jsonPath() - .getLong("id"); - - var callback = - String.format( - "http://storedcredentials:%d@localhost:8081/health/liveness", credentialId); - - given().log() - .all() - .when() - .body(java.util.Map.of("realm", "expiry_test_realm", "callback", callback)) - .contentType(ContentType.JSON) - .post("http://localhost:8081/api/v4/discovery") - .then() - .log() - .all() - .and() - .assertThat() - .statusCode(200); - - var credential = - QuarkusTransaction.requiringNew() - .call(() -> Credential.findById(credentialId)); - - Assertions.assertNotNull(credential.expiresAt); - Assertions.assertTrue( - !credential.expiresAt.isBefore(Instant.now().plusSeconds(3500)) - && !credential.expiresAt.isAfter(Instant.now().plusSeconds(3700))); - assertNull(credential.lastUsedAt); - } - private int createTestCredential() { return given().log() .all() diff --git a/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java b/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java index 572e5a2ef..ecb95a302 100644 --- a/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java +++ b/src/test/java/io/cryostat/discovery/DiscoveryPluginGracePeriodTest.java @@ -26,7 +26,6 @@ import io.cryostat.AbstractTransactionalTestBase; import io.cryostat.ConfigProperties; -import io.cryostat.credentials.Credential; import io.cryostat.discovery.DiscoveryPlugin.PluginCallback; import io.cryostat.targets.AgentClient; @@ -697,68 +696,4 @@ public void testBackoffResetsOnSuccess() throws Exception { assertNull(updatedPlugin.nextPingAt, "Next ping time should be cleared"); assertNotNull(updatedPlugin.lastSuccessfulPing, "Last successful ping should be set"); } - - @Test - public void testAgentCredentialUseUpdatesLastUsedAt() { - var credentialId = - given().log() - .all() - .when() - .formParams( - Map.of( - "username", - "user", - "password", - "pass", - "matchExpression", - "target.connectUrl ==" - + " 'http://localhost:8081/health/liveness'")) - .contentType(ContentType.URLENC) - .post("/api/v4/credentials") - .then() - .log() - .all() - .and() - .assertThat() - .statusCode(201) - .contentType(ContentType.JSON) - .extract() - .jsonPath() - .getLong("id"); - - var callback = - String.format( - "http://storedcredentials:%d@localhost:8081/health/liveness", credentialId); - - given().log() - .all() - .when() - .body(Map.of("realm", "test_last_used_realm", "callback", callback)) - .contentType(ContentType.JSON) - .post("/api/v4/discovery") - .then() - .log() - .all() - .and() - .assertThat() - .statusCode(200); - - var beforeUse = - QuarkusTransaction.requiringNew() - .call(() -> Credential.findById(credentialId)); - assertNull(beforeUse.lastUsedAt); - - var target = new io.cryostat.targets.Target(); - target.connectUrl = URI.create("http://localhost:8081/health/liveness"); - - var client = agentClientFactory.create(target); - - assertDoesNotThrow(() -> client.ping().await().indefinitely()); - - var afterUse = - QuarkusTransaction.requiringNew() - .call(() -> Credential.findById(credentialId)); - assertNotNull(afterUse.lastUsedAt); - assertTrue(afterUse.lastUsedAt.isBefore(Instant.now().plusSeconds(10))); - } } From d3f0a2e29c2c27156e1b5f97c76ca72304633d1c Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 24 Apr 2026 15:46:33 -0400 Subject: [PATCH 21/24] fixup! reset submodule --- src/main/webui | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/webui b/src/main/webui index a2acc1df1..cffcbf6ae 160000 --- a/src/main/webui +++ b/src/main/webui @@ -1 +1 @@ -Subproject commit a2acc1df1ddf60ed6431b43fba4a6e6db441fdbc +Subproject commit cffcbf6ae80cbb82da711dbf21a97a0606721f3a From a1d9171e74d8b19d7ca73434f51bbb5bdc5b9f6a Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 24 Apr 2026 15:50:41 -0400 Subject: [PATCH 22/24] allow 3 ping failures --- src/main/resources/application.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index dc7dc5810..854e93bb4 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -33,7 +33,7 @@ cryostat.discovery.containers.request-timeout=2s cryostat.discovery.podman.enabled=false cryostat.discovery.docker.enabled=false cryostat.discovery.plugins.ping-period=1m -cryostat.discovery.plugins.max-failures=2 +cryostat.discovery.plugins.max-failures=3 cryostat.discovery.plugins.max-backoff-multiplier=8 cryostat.discovery.plugins.jwt.secret.algorithm=AES cryostat.discovery.plugins.jwt.secret.keysize=256 From 4c1f6618c262b57a161f5ef5c1fda5e0d7a64c3a Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 24 Apr 2026 16:08:20 -0400 Subject: [PATCH 23/24] reduce max backoff --- src/main/resources/application.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 854e93bb4..320a6c9cc 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -34,7 +34,7 @@ cryostat.discovery.podman.enabled=false cryostat.discovery.docker.enabled=false cryostat.discovery.plugins.ping-period=1m cryostat.discovery.plugins.max-failures=3 -cryostat.discovery.plugins.max-backoff-multiplier=8 +cryostat.discovery.plugins.max-backoff-multiplier=5 cryostat.discovery.plugins.jwt.secret.algorithm=AES cryostat.discovery.plugins.jwt.secret.keysize=256 cryostat.discovery.plugins.jwt.signature.algorithm=HS256 From 15dea58f3449e4b2f89d9167208ae36886c22adc Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 24 Apr 2026 16:08:32 -0400 Subject: [PATCH 24/24] additional debug logging --- .../java/io/cryostat/discovery/Discovery.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/main/java/io/cryostat/discovery/Discovery.java b/src/main/java/io/cryostat/discovery/Discovery.java index dff4ec813..adda8ca3f 100644 --- a/src/main/java/io/cryostat/discovery/Discovery.java +++ b/src/main/java/io/cryostat/discovery/Discovery.java @@ -952,6 +952,13 @@ public void execute(JobExecutionContext context) throws JobExecutionException { p.backoffMultiplier = 1; p.nextPingAt = null; p.persist(); + + logger.debugv( + "Plugin ping successful - lastSuccessfulPing: {0}," + + " consecutiveFailures reset to 0," + + " backoffMultiplier reset to 1: {1} @" + + " {2}", + p.lastSuccessfulPing, p.realm.name, p.callback); } catch (NoResultException e) { throw e; } catch (Exception e) { @@ -992,6 +999,19 @@ public void execute(JobExecutionContext context) throws JobExecutionException { p.nextPingAt = Instant.now().plus(backoffPeriod); p.persist(); + logger.debugv( + "Plugin ping failed - lastFailedPing: {0}," + + " consecutiveFailures: {1}/{2}," + + " backoffMultiplier: {3}, nextPingAt:" + + " {4}: {5} @ {6}", + p.lastFailedPing, + p.consecutiveFailures, + maxConsecutiveFailures, + p.backoffMultiplier, + p.nextPingAt, + p.realm.name, + p.callback); + if (p.consecutiveFailures >= maxConsecutiveFailures) { logger.warnv( "Pruning discovery plugin after {0}"