[
https://issues.apache.org/jira/browse/KNOX-3515?focusedWorklogId=1047414&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1047414
]
ASF GitHub Bot logged work on KNOX-3515:
----------------------------------------
Author: ASF GitHub Bot
Created on: 14/Oct/26 11:56
Start Date: 14/Oct/26 11:56
Worklog Time Spent: 10m
Work Description: 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!
Issue Time Tracking
-------------------
Worklog Id: (was: 1047414)
Remaining Estimate: 0h
Time Spent: 10m
> 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: 10m
> 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
(v8.20.10#820010)
