[
https://issues.apache.org/jira/browse/KNOX-3515?focusedWorklogId=1049262&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1049262
]
ASF GitHub Bot logged work on KNOX-3515:
----------------------------------------
Author: ASF GitHub Bot
Created on: 22/Oct/26 10:30
Start Date: 22/Oct/26 10:30
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #1456:
URL: https://github.com/apache/knox/pull/1456#discussion_r4347246022
##########
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:
Yes. The fail-closed snapshot is a normal cache entry stamped at the failure
time, so it follows the same schedule: retried in the background after TTL/2,
reloaded synchronously after TTL.
Without the advance it's fresh and is served without hitting the DB. That's
deliberate: one DB attempt per TTL/2 during an outage, not one per request.
I'll clarify in `operations.md` that recovery can take up to about TTL/2.
Issue Time Tracking
-------------------
Worklog Id: (was: 1049262)
Time Spent: 2h (was: 1h 50m)
> Add trusted oidc issuer cache TTL/eventual consistency for HA configurations
> ----------------------------------------------------------------------------
>
> Key: KNOX-3515
> URL: https://issues.apache.org/jira/browse/KNOX-3515
> Project: Apache Knox
> Issue Type: Sub-task
> Components: JWT
> Reporter: Harrison Sheinblatt
> Assignee: Sandor Molnar
> Priority: Major
> Time Spent: 2h
> Remaining Estimate: 0h
>
> Knox nodes in an HA configuration will not all see changes made on other
> nodes within a known time limit, and some will never be updated.
> Currently there are a few cache layers used with Trusted OIDC Issuers:
> # Registry snapshot in JdbcTrustedOidcIssuerService
> # Discovery document cache in OIDCDiscoveryHelper
> # JWKS source cache in DefaultTokenAuthorityService (this is preexisting)
> The registry snapshot never expires and is only mutated by the local admin
> API calls. See reloadRegistrySnapshot. The discovery document cache has a
> TTL, but won't be invalidated when the table data is changed.
> To enable HA configurations to work properly, the registry snapshot needs to
> be invalidated within a configurable time limit, ideally by a
> background/scheduled thread with jitter. This will be the same as the
> revocation timeout and the time before a newly registered issuer can be
> safely used for token exchange.
> To make the full cache stack work, when the registry snapshot cache is
> invalidated, the discovery cache must also be invalidated:
> discoveryHelper.invalidate(issuerUrl).
> There are other options to allow HA configurations to work, but they have
> drawbacks, including but not limited to:
> # Removing the registry cache exposes a DOS surface for invalid issuers
> ## Could use a negative cache with rate limiting to mitigate the DOS threat
> # One could optimize for reducing add issuer staleness: on issuer not found,
> have a faster interval to read through to the database. Likely would still
> need rate limiting.
> # Operator could call each node in the HA set. This is prone to error and
> may not be possible if the operator is not colocated in the cluster.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
