smolnar82 opened a new pull request, #1456: URL: https://github.com/apache/knox/pull/1456
[KNOX-3515](https://issues.apache.org/jira/browse/KNOX-3515) - Add trusted oidc issuer cache TTL/eventual consistency for HA configurations ## What changes were proposed in this pull request? `JdbcTrustedOidcIssuerService` held the whole `TRUSTED_OIDC_ISSUERS` table in memory as a snapshot that never expired. Only a local register or deregister rebuilt it. In an HA deployment with several gateways sharing one database, a node never saw issuers registered or deregistered on another node until it restarted. A revoked issuer could therefore keep exchanging tokens on the other nodes indefinitely. This PR makes the registry snapshot eventually consistent within a configurable TTL. It also couples the OIDC discovery cache to the registry. **Registry snapshot (`gateway-server`)** - The `AtomicReference` snapshot is now a single-key Caffeine `LoadingCache<String, RegistrySnapshot>`. It still caches the whole table, so an unknown `iss` in an unauthenticated request can never trigger a DB query. - `refreshAfterWrite(TTL/2)`: once a snapshot is older than TTL/2, the next read starts a background reload and is served the current snapshot meanwhile, so request threads don't wait on the DB. - `expireAfter(RegistrySnapshotExpiry)`: hard expiry at TTL, anchored to the snapshot's load time. A failed background reload keeps the original load time, so it can't extend the bound. **No node serves a snapshot older than the TTL**, which bounds how long a revocation takes to apply. - **Jitter:** each node shortens its TTL by a random amount of up to 10%, chosen once at init, so nodes started together don't reload in lockstep. - Local writes (`register` / `deregister`) still rebuild and `put` the snapshot immediately, so a node always reads its own writes. - **Failure handling.** The loader never throws, because Caffeine reports loader exceptions via JUL, which Knox doesn't route into its logs. - **Background refresh fails:** the node keeps serving the cached snapshot and logs WARN with its age and the time left until it fails closed. - **Snapshot expired and DB unreachable: fail closed.** No issuer is trusted. This is logged once at ERROR per failure period, repeated failures at DEBUG, and recovery at INFO. - Init still fails if the DB is unreachable at startup. - **Discovery cache coupling.** Whenever the snapshot is rebuilt, the `OIDCDiscoveryHelper` entries of added, removed or re-registered issuers (changed `dynamicJwks` / `registeredAt`) are invalidated via `discoveryHelper.invalidate(issuerUrl)`. A cold or hard-expired load has no previous snapshot to diff against, so it calls the new `OIDCDiscoveryHelper.invalidateAll()`. - The new logic lives in package-private `RegistrySnapshot`, `RegistrySnapshotLoader` and `RegistrySnapshotExpiry`. `JdbcTrustedOidcIssuerService` just wires them together. **Configuration (`gateway-spi`)** - New property `gateway.trusted.oidc.issuer.registry.cache.ttl.secs`, default `60`. A value ≤ 0 falls back to the default. **Docs (`knox-site`)** - `configuration.md` documents the new property. - `operations.md` gets a new "Trusted OIDC issuer registry consistency" subsection. It covers the TTL, fail-closed behaviour and its logging, and the remaining per-node behaviours. It also fixes the service implementation property names (`gateway.service.trustedoidcissuer.impl`, `gateway.service.delegationpolicy.impl`, `gateway.service.knoxidffederatedidentity.impl`, `gateway.service.tokenstate.impl`). ### KnoxIDF in-memory caches and their HA impact I audited every in-memory cache on the Trusted OIDC Issuer and Delegation Policy paths, plus the KnoxIDF stores around them: | # | Cache | Where | Type / TTL | Problem in HA? | |---|---|---|---|---| | 1 | Issuer registry snapshot | `JdbcTrustedOidcIssuerService` | Before: `AtomicReference<Map>`, **no TTL**, rebuilt only by local register/deregister. **Now:** Caffeine `LoadingCache`, refresh at TTL/2, hard expiry at TTL, per-node jitter, fails closed | **Yes; this was the JIRA, and this PR fixes it.** A registration or revocation on node A used to reach node B only after a restart. Now it reaches B within `registry.cache.ttl.secs`. | | 2 | Discovery cache (issuer URL → JWKS URI) | `OIDCDiscoveryHelper` | Caffeine `expireAfterWrite(discovery.cache.ttl.secs)`, default 600s | **Partly.** It is now invalidated whenever the registry snapshot changes. Still per node: `refresh-jwks` (admin API) evicts only on the node that receives the call. Documented in `operations.md`. | | 3 | JWKS sources (JWKS URL → `JWKSource`) | `DefaultTokenAuthorityService.cachedJwkSources` | Plain `HashMap`, **no TTL, no size limit, not thread-safe** | **No staleness issue.** Nimbus refreshes the keys itself (cache TTL + refresh-ahead), and the `isDynamicJwks` check blocks deregistered issuers before this map is used. But concurrent `put`s on a `HashMap` are a bug that predates this work, and entries for removed issuers stay forever. Out of scope; follow-up JIRA. | | 4 | Delegation policies | `JdbcDelegationPolicyService` | **No cache**; every `evaluate()` reads the DB | No. Already consistent across nodes; the new HA E2E test pins this down. | | 5 | Federated identities | `JdbcFederatedIdentityService` | No cache | No | | 6 | Authorize metadata, nonce and OP-config stores | `KnoxIDFArtifactStore` subclasses | Caffeine `expireAfterWrite(2×ttl)` | They don't go stale; they're just local to each node. Sticky sessions are already documented for this in `operations.md`. Out of scope. | | 7 | `SignatureVerificationCache` | jwt provider | Caffeine, size limit only | No. The issuer trust check (`validateToken` → `resolveRegisteredIssuerJwks`) runs before this cache is consulted, so revocation still applies. | | 8 | K8s SA annotations | `K8sServiceAccountResolver` | Caffeine `expireAfterWrite`, configurable | No | | 9 | LDAP entry / group-parent maps | `LdapProxyBackend` | New `HashMap` per call | No | **Known issues out of scope** (they predate this work; separate JIRAs): - #3 above: `cachedJwkSources` thread-safety and unbounded growth. - `KnoxDatabase.createTableIfNotExists` checks whether a table exists and then creates it. Two nodes starting at the same time against an empty schema can both reach `CREATE TABLE`, and the loser fails gateway startup. The HA compose setup avoids this by starting `knox2` only after `knox` is healthy. - The `max.issuers` limit is checked against each node's own snapshot, so concurrent registrations on different nodes can exceed it slightly. Documented in `operations.md`. ## How was this patch tested? **Unit tests** - `JdbcTrustedOidcIssuerServiceTest` has 27 tests (7 new). The new ones use a fake Caffeine `Ticker` and a same-thread executor, so there are no sleeps. They cover: - a registration made by another node becoming visible after a background refresh - a deregistration made by another node never being served past the TTL - DB outage: the stale snapshot is served, then the registry fails closed, then recovers - the TTL ≤ 0 fallback to the default - discovery-cache invalidation only for changed issuers on refresh, `invalidateAll()` on a load after expiry, and invalidation for an issuer re-registered locally - `OIDCDiscoveryHelperTest` has 13 tests (1 new, for `invalidateAll()`). ``` mvn -pl gateway-server test -Dtest='JdbcTrustedOidcIssuerServiceTest,OIDCDiscoveryHelperTest' ``` **Static checks:** checkstyle, PMD and spotbugs are clean on `gateway-server`, `gateway-spi` and `gateway-spi-common`. **Two-node HA E2E**, run locally against a fresh `-Ppackage,release` build. All 3 tests passed, on two separate runs: ``` docker compose -f ./.github/workflows/compose/docker-compose.yml build knox-dev docker compose -f ./.github/workflows/compose/docker-compose.yml \ -f ./.github/workflows/compose/docker-compose.knoxidf-ha.yml \ up --exit-code-from tests tests ``` ## Integration Tests New opt-in suite `test_knoxidf_ha.py`, with compose override `docker-compose.knoxidf-ha.yml`. The setup: - `postgres` (`postgres:16-alpine`) - `knox` and `knox2`, both running TrustedOidcIssuer, DelegationPolicy, FederatedIdentity and TokenState on JDBC against that one database, with `registry.cache.ttl.secs=5` All writes go to node A and every check runs on node B: - `test_issuer_registered_on_one_node_is_trusted_on_the_other`: register the k3s issuer on A. B lists it, and exchanging a real k3s SA token on B succeeds. - `test_issuer_deregistered_on_one_node_is_rejected_on_the_other`: deregister on A. The same token exchange on B is rejected. This is the revocation guarantee. - `test_delegation_policy_changed_on_one_node_applies_on_the_other`: a policy created and then deleted on A is applied on B **immediately**. This pins down that `DelegationPolicyService` has no cache. The tests assert only an upper time limit (polling up to about 3×TTL), never "still stale right after the write", so they aren't timing-flaky. Other changes: - The base `docker-compose.yml` ignores `test_knoxidf_ha.py`. - In `tests.yml`, the HA steps (start, wait, run, dump `knox` / `knox2` / `postgres` logs) are gated on the new `test-ha` label, `test-results-ha.xml` is added to the uploaded results, and teardown now runs `down --volumes --remove-orphans`. - `tests/README.md` gets an "Opt-in Suites on Pull Requests" section. ### Opt-in test suites (PR labels) Some integration suites are expensive and are **not** run on every PR. Add the corresponding label to this PR to run them: | Label | Runs | |-------|------| | `test-federation` | KnoxIDF federation E2E (`test_knoxidf_federation.py`). Stands up a real Keycloak as an external OpenID Provider and drives the full broker flow. Adds a few minutes (image pull + realm import). | | `test-ha` | KnoxIDF HA E2E (`test_knoxidf_ha.py`). Stands up PostgreSQL and a second gateway (`knox2`) sharing one database, and checks that registry changes made on one node reach the other within the registry cache TTL. Adds about a minute (postgres image + a second Knox startup). | | `skip-tests` | Skips the entire Docker Compose test job. | This PR should carries the `test-ha` label! -- 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]
