Repository navigation
Conversation
|
Cc. @hsheinblatt |
79c874c to
f2f9ed6
Compare
Test Results 5 files 5 suites 1m 47s ⏱️ Results for commit 6599410. ♻️ This comment has been updated with latest results. |
pzampino
left a comment
There was a problem hiding this comment.
LGTM, but someone else should also review.
…r HA configurations Co-Authored-By: Claude <noreply@anthropic.com>
hsheinblatt
left a comment
There was a problem hiding this comment.
Thanks Sandor. It looks good to me. In a normal production state, refreshing the cache after ttl/2 makes sense. I'm concerned if there are problems with the LB or in a test environment if one node might be idle for a while and then test startup might see failures for the first few queries until it starts to work. Possibly this is fine, just warm before test, and simpler than an active poll. I'm not sure how much harder an active poll would be.
| ttlService.isTrusted("https://remote.example.com")); | ||
|
|
||
| advance(now, 6); | ||
| ttlService.isTrusted("https://remote.example.com"); // triggers the background refresh, served stale |
There was a problem hiding this 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?
| assertFalse("A fresh snapshot is served without hitting the DB", | ||
| ttlService.isTrusted("https://remote.example.com")); | ||
|
|
||
| advance(now, 6); |
There was a problem hiding this 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.
| EasyMock.verify(failingDb); | ||
|
|
||
| useDatabase(ttlService, realDatabase); | ||
| advance(now, 6); |
There was a problem hiding this comment.
Why do you need to advance? The limit on a failed closed db is still ttl/2 to retry?
|
|
||
| useDatabase(ttlService, realDatabase); | ||
| advance(now, 6); | ||
| ttlService.isTrusted("https://issuer.example.com"); // triggers the background refresh |
There was a problem hiding this comment.
This should still fail, right, so assert on return value.
KNOX-3515 - Add trusted oidc issuer cache TTL/eventual consistency for HA configurations
What changes were proposed in this pull request?
JdbcTrustedOidcIssuerServiceheld the wholeTRUSTED_OIDC_ISSUERStable 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
AtomicReferencesnapshot is now a single-key CaffeineLoadingCache<String, RegistrySnapshot>. It still caches the whole table, so an unknownissin 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.register/deregister) still rebuild andputthe 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.
Discovery cache and CA trust coupling. Whenever the snapshot is rebuilt, entries are invalidated for every issuer that was added, removed or re-registered (changed
dynamicJwks,registeredAtorjwksCa). This covers two caches:OIDCDiscoveryHelpercache, viadiscoveryHelper.invalidate(issuerUrl)IssuerTrustProvidercache ofjwksCaTLS trust added by KNOX-3512, viatrustProvider.invalidate(issuerUrl)So a re-registration on another node forces a CA Secret re-read, just as a local one does. A cold or hard-expired load has no previous snapshot to diff against, so it calls the new
invalidateAll()on both.The new logic lives in package-private
RegistrySnapshot,RegistrySnapshotLoaderandRegistrySnapshotExpiry.JdbcTrustedOidcIssuerServicejust wires them together.Configuration (
gateway-spi)gateway.trusted.oidc.issuer.registry.cache.ttl.secs, default60. A value ≤ 0 falls back to the default.Docs (
knox-site)configuration.mddocuments the new property.operations.mdgets 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:
JdbcTrustedOidcIssuerServiceAtomicReference<Map>, no TTL, rebuilt only by local register/deregister. Now: CaffeineLoadingCache, refresh at TTL/2, hard expiry at TTL, per-node jitter, fails closedregistry.cache.ttl.secs.OIDCDiscoveryHelperexpireAfterWrite(discovery.cache.ttl.secs), default 600srefresh-jwks(admin API) evicts only on the node that receives the call. Documented inoperations.md.jwksCa(KNOX-3512)IssuerTrustProviderConcurrentHashMap; Secret-backed bundles re-read afterca.secret.max.age.secsor a TLS failurerefresh-jwks. It is now also cleared when the registry snapshot shows the issuer changed.refresh-jwksis still local only.JWKSource)DefaultTokenAuthorityService.cachedIssuerTrustJwkSourcesexpireAfterAccess; closed on evictionJWKSource)DefaultTokenAuthorityService.cachedJwkSourcesHashMapfilled withcomputeIfAbsent, no TTL, no size limit, not thread-safeisDynamicJwkscheck blocks deregistered issuers before this map is used. But concurrentputs on aHashMapare a bug that predates this work, and entries for removed issuers stay forever. Out of scope; follow-up JIRA.JdbcDelegationPolicyServiceevaluate()reads the DBJdbcFederatedIdentityServiceKnoxIDFArtifactStoresubclassesexpireAfterWrite(2×ttl)operations.md. Out of scope.SignatureVerificationCachevalidateToken→resolveRegisteredIssuerJwks) runs before this cache is consulted, so revocation still applies.K8sServiceAccountResolverexpireAfterWrite, configurableLdapProxyBackendHashMapper callKnown issues out of scope (they predate this work; separate JIRAs):
cachedJwkSourcesthread-safety and unbounded growth.KnoxDatabase.createTableIfNotExistschecks whether a table exists and then creates it. Two nodes starting at the same time against an empty schema can both reachCREATE TABLE, and the loser fails gateway startup. The HA compose setup avoids this by startingknox2only afterknoxis healthy.max.issuerslimit is checked against each node's own snapshot, so concurrent registrations on different nodes can exceed it slightly. Documented inoperations.md.How was this patch tested?
Unit tests
JdbcTrustedOidcIssuerServiceTesthas 34 tests (8 new). The new ones use a fake CaffeineTickerand a same-thread executor, so there are no sleeps. They cover:invalidateAll()on a load after expiry, and invalidation for an issuer re-registered locallyOIDCDiscoveryHelperTesthas 18 tests (1 new, forinvalidateAll()). The KNOX-3512 suites (IssuerTrustProviderTest,IssuerJwksCaAcceptanceTest,TrustedOidcIssuerTest,TrustedOidcIssuersSchemaTest) still pass after the rebase.Static checks: checkstyle, PMD and spotbugs are clean on
gateway-server,gateway-spiandgateway-spi-common.Two-node HA E2E, run locally against a fresh
-Ppackage,releasebuild, both before and after the rebase onto KNOX-3512. All 3 tests passed:Integration Tests
New opt-in suite
test_knoxidf_ha.py, with compose overridedocker-compose.knoxidf-ha.yml. The setup:postgres(postgres:16-alpine)knoxandknox2, both running TrustedOidcIssuer, DelegationPolicy, FederatedIdentity and TokenState on JDBC against that one database, withregistry.cache.ttl.secs=5All 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, with the k3s serving CA as itsjwksCa(KNOX-3512). B lists it, and exchanging a real k3s SA token on B succeeds. This means B built its per-issuer TLS trust from the shared row alone.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 thatDelegationPolicyServicehas 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:
docker-compose.ymlignorestest_knoxidf_ha.py.knox2uses the same/gateway.shcommand and mounts as the baseknoxservice. KNOX-3512 removed thegateway-k8s-delegation.shcacerts-import entrypoint.tests.ymlalso finds stopped containers, and fails fast with the exit code if a gateway exits during startup.tests.yml, the HA steps (start, wait, run, dumpknox/knox2/postgreslogs) are gated on the newtest-halabel,test-results-ha.xmlis added to the uploaded results, and teardown now runsdown --volumes --remove-orphans.tests/README.mdgets 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:
test-federationtest_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-hatest_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-testsThis PR should carry the
test-halabel!