hsheinblatt commented on code in PR #1456:
URL: https://github.com/apache/knox/pull/1456#discussion_r4299600912
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerServiceTest.java:
##########
@@ -493,10 +497,274 @@ public void testConcurrentInitDoesNotReinitialize()
throws Exception {
sentinel, FieldUtils.readField(service, "database", true));
}
+ // ------------------------------------------------------------------
+ // Registry snapshot TTL (HA eventual consistency)
+ // ------------------------------------------------------------------
+ // Direct JDBC writes stand in for another Knox node writing to the shared
table. With a TTL of
+ // 10s the effective (jittered) TTL is in [9s, 10s] and the background
refresh is due after at
+ // most 5s, so +6s means "refresh due, not expired" and +11s means "expired".
+
+ private static final int TTL_SECS = 10;
+
+ @Test
+ public void testRemoteRegistrationVisibleAfterBackgroundRefresh() throws
Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(TTL_SECS, now);
+ insertRow("https://remote.example.com";, false, Instant.now());
+
+ assertFalse("A fresh snapshot is served without hitting the DB",
+ ttlService.isTrusted("https://remote.example.com";));
+
+ advance(now, 6);
+ ttlService.isTrusted("https://remote.example.com";); // triggers the
background refresh, served stale
Review Comment:
How do you ensure the background refresh completes before the next line?
Isn't there a race?
And shouldn't this also assertFalse on the result?
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerServiceTest.java:
##########
@@ -493,10 +497,274 @@ public void testConcurrentInitDoesNotReinitialize()
throws Exception {
sentinel, FieldUtils.readField(service, "database", true));
}
+ // ------------------------------------------------------------------
+ // Registry snapshot TTL (HA eventual consistency)
+ // ------------------------------------------------------------------
+ // Direct JDBC writes stand in for another Knox node writing to the shared
table. With a TTL of
+ // 10s the effective (jittered) TTL is in [9s, 10s] and the background
refresh is due after at
+ // most 5s, so +6s means "refresh due, not expired" and +11s means "expired".
+
+ private static final int TTL_SECS = 10;
+
+ @Test
+ public void testRemoteRegistrationVisibleAfterBackgroundRefresh() throws
Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(TTL_SECS, now);
+ insertRow("https://remote.example.com";, false, Instant.now());
+
+ assertFalse("A fresh snapshot is served without hitting the DB",
+ ttlService.isTrusted("https://remote.example.com";));
+
+ advance(now, 6);
+ ttlService.isTrusted("https://remote.example.com";); // triggers the
background refresh, served stale
+ assertTrue("Remote registration must be visible once the background
refresh completed",
+ ttlService.isTrusted("https://remote.example.com";));
+ }
+
+ @Test
+ public void testRemoteDeregistrationNeverServedAfterTtl() throws Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(TTL_SECS, now);
+ ttlService.register(issuer("https://issuer.example.com";, false));
+ deleteRow("https://issuer.example.com";);
+
+ // No read in between, so no background refresh ran: the expired snapshot
must not be served
+ advance(now, 11);
+ assertFalse("A revoked issuer must not be trusted once the TTL elapsed",
+ ttlService.isTrusted("https://issuer.example.com";));
+ }
+
+ @Test
+ public void testNonPositiveTtlFallsBackToDefault() throws Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(0, now);
+ insertRow("https://remote.example.com";, false, Instant.now());
+
+ // Default TTL is 60s: effective TTL in [54s, 60s], refresh due after at
least 27s
+ advance(now, 25);
+ ttlService.isTrusted("https://remote.example.com";);
+ assertFalse(ttlService.isTrusted("https://remote.example.com";));
+
+ advance(now, 36);
+ assertTrue(ttlService.isTrusted("https://remote.example.com";));
+ }
+
+ /**
+ * DB outage: the snapshot is served stale until the TTL, a failed refresh
neither extends the
+ * TTL nor is retried on every request, then the registry fails closed and
recovers once the DB
+ * is readable again.
+ */
+ @Test
+ public void testDatabaseOutageServesStaleThenFailsClosedThenRecovers()
throws Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(TTL_SECS, now);
+ ttlService.register(issuer("https://issuer.example.com";, false));
+ final TrustedOidcIssuerDatabase realDatabase =
+ (TrustedOidcIssuerDatabase) FieldUtils.readField(ttlService,
"database", true);
+
+ final TrustedOidcIssuerDatabase failingDb =
EasyMock.createMock(TrustedOidcIssuerDatabase.class);
+ EasyMock.expect(failingDb.selectAll()).andThrow(new SQLException("database
is down")).times(2);
+ EasyMock.replay(failingDb);
+ useDatabase(ttlService, failingDb);
+
+ advance(now, 6);
+ for (int i = 0; i < 5; i++) {
+ assertTrue("The cached snapshot must be served while it is younger than
the TTL",
+ ttlService.isTrusted("https://issuer.example.com";));
+ }
+
+ advance(now, 5); // 11s after the last successful load, although the
failed refresh ran at 6s
+ assertFalse("The registry must fail closed once the snapshot expired",
+ ttlService.isTrusted("https://issuer.example.com";));
+ assertTrue(ttlService.list().isEmpty());
+ assertTrue(registrySnapshotLoader(ttlService).isFailedClosed());
+ // One failed background refresh at 6s plus one failed load at 11s: no
per-request DB retries
+ EasyMock.verify(failingDb);
+
+ useDatabase(ttlService, realDatabase);
+ advance(now, 6);
Review Comment:
Why do you need to advance? The limit on a failed closed db is still ttl/2
to retry?
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerServiceTest.java:
##########
@@ -493,10 +497,274 @@ public void testConcurrentInitDoesNotReinitialize()
throws Exception {
sentinel, FieldUtils.readField(service, "database", true));
}
+ // ------------------------------------------------------------------
+ // Registry snapshot TTL (HA eventual consistency)
+ // ------------------------------------------------------------------
+ // Direct JDBC writes stand in for another Knox node writing to the shared
table. With a TTL of
+ // 10s the effective (jittered) TTL is in [9s, 10s] and the background
refresh is due after at
+ // most 5s, so +6s means "refresh due, not expired" and +11s means "expired".
+
+ private static final int TTL_SECS = 10;
+
+ @Test
+ public void testRemoteRegistrationVisibleAfterBackgroundRefresh() throws
Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(TTL_SECS, now);
+ insertRow("https://remote.example.com";, false, Instant.now());
+
+ assertFalse("A fresh snapshot is served without hitting the DB",
+ ttlService.isTrusted("https://remote.example.com";));
+
+ advance(now, 6);
+ ttlService.isTrusted("https://remote.example.com";); // triggers the
background refresh, served stale
+ assertTrue("Remote registration must be visible once the background
refresh completed",
+ ttlService.isTrusted("https://remote.example.com";));
+ }
+
+ @Test
+ public void testRemoteDeregistrationNeverServedAfterTtl() throws Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(TTL_SECS, now);
+ ttlService.register(issuer("https://issuer.example.com";, false));
+ deleteRow("https://issuer.example.com";);
+
+ // No read in between, so no background refresh ran: the expired snapshot
must not be served
+ advance(now, 11);
+ assertFalse("A revoked issuer must not be trusted once the TTL elapsed",
+ ttlService.isTrusted("https://issuer.example.com";));
+ }
+
+ @Test
+ public void testNonPositiveTtlFallsBackToDefault() throws Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(0, now);
+ insertRow("https://remote.example.com";, false, Instant.now());
+
+ // Default TTL is 60s: effective TTL in [54s, 60s], refresh due after at
least 27s
+ advance(now, 25);
+ ttlService.isTrusted("https://remote.example.com";);
+ assertFalse(ttlService.isTrusted("https://remote.example.com";));
+
+ advance(now, 36);
+ assertTrue(ttlService.isTrusted("https://remote.example.com";));
+ }
+
+ /**
+ * DB outage: the snapshot is served stale until the TTL, a failed refresh
neither extends the
+ * TTL nor is retried on every request, then the registry fails closed and
recovers once the DB
+ * is readable again.
+ */
+ @Test
+ public void testDatabaseOutageServesStaleThenFailsClosedThenRecovers()
throws Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(TTL_SECS, now);
+ ttlService.register(issuer("https://issuer.example.com";, false));
+ final TrustedOidcIssuerDatabase realDatabase =
+ (TrustedOidcIssuerDatabase) FieldUtils.readField(ttlService,
"database", true);
+
+ final TrustedOidcIssuerDatabase failingDb =
EasyMock.createMock(TrustedOidcIssuerDatabase.class);
+ EasyMock.expect(failingDb.selectAll()).andThrow(new SQLException("database
is down")).times(2);
+ EasyMock.replay(failingDb);
+ useDatabase(ttlService, failingDb);
+
+ advance(now, 6);
+ for (int i = 0; i < 5; i++) {
+ assertTrue("The cached snapshot must be served while it is younger than
the TTL",
+ ttlService.isTrusted("https://issuer.example.com";));
+ }
+
+ advance(now, 5); // 11s after the last successful load, although the
failed refresh ran at 6s
+ assertFalse("The registry must fail closed once the snapshot expired",
+ ttlService.isTrusted("https://issuer.example.com";));
+ assertTrue(ttlService.list().isEmpty());
+ assertTrue(registrySnapshotLoader(ttlService).isFailedClosed());
+ // One failed background refresh at 6s plus one failed load at 11s: no
per-request DB retries
+ EasyMock.verify(failingDb);
+
+ useDatabase(ttlService, realDatabase);
+ advance(now, 6);
+ ttlService.isTrusted("https://issuer.example.com";); // triggers the
background refresh
Review Comment:
This should still fail, right, so assert on return value.
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerServiceTest.java:
##########
@@ -493,10 +497,274 @@ public void testConcurrentInitDoesNotReinitialize()
throws Exception {
sentinel, FieldUtils.readField(service, "database", true));
}
+ // ------------------------------------------------------------------
+ // Registry snapshot TTL (HA eventual consistency)
+ // ------------------------------------------------------------------
+ // Direct JDBC writes stand in for another Knox node writing to the shared
table. With a TTL of
+ // 10s the effective (jittered) TTL is in [9s, 10s] and the background
refresh is due after at
+ // most 5s, so +6s means "refresh due, not expired" and +11s means "expired".
+
+ private static final int TTL_SECS = 10;
+
+ @Test
+ public void testRemoteRegistrationVisibleAfterBackgroundRefresh() throws
Exception {
+ final AtomicLong now = new AtomicLong();
+ final JdbcTrustedOidcIssuerService ttlService = ttlService(TTL_SECS, now);
+ insertRow("https://remote.example.com";, false, Instant.now());
+
+ assertFalse("A fresh snapshot is served without hitting the DB",
+ ttlService.isTrusted("https://remote.example.com";));
+
+ advance(now, 6);
Review Comment:
If there is no request within the right timeframe, then the refresh never
happens and the next request can fail for TTL. Is it not possible to just have
a background thread running so every TTL/2 requests it refreshes instead? Then
it doesn't have to be inline with logic.
I thought it was never synchronously fetched -- if it were then if there
were a few concurrent requests after the timeout then it might burst load.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
