yuqi1129 opened a new pull request, #12231:
URL: https://github.com/apache/gravitino/pull/12231
### What changes were proposed in this pull request?
Ensures a role can never be recorded as loaded unless the enforcer actually
holds its policies, and that nothing can erase those policies behind the
loader's back.
1. **Record a role as loaded only when its policies fully loaded.**
`loadPolicyByRoleEntity` is split into `resolveRolePolicies` (resolves every
securable object to a metadata id, may hit the DB) and `applyRolePolicies`
(writes rows into the enforcers, pure in-memory). The intermediate
`ResolvedRolePolicies` carries the objects that failed to resolve, and
`loadedRoles.put` now runs only when that list is empty. Incomplete loads are
retried instead of being pinned, throttled by a short backoff
(`partialRoleLoadBackoff`, 10s) so that a permanently unresolvable object — a
role still referencing a dropped table, say — cannot turn into a DB round-trip
per request. `handleRolePrivilegeChange` drops the backoff so an explicit
privilege change is retried immediately.
2. **Serialize role policy mutations with `rolePolicyLock`.** The lock
covers the clear → apply → record sequence and `clearRolePolicies` itself,
which the `JcasbinLoadedRolesCache` removal listener calls from arbitrary
threads. Metadata ids are resolved before the lock is taken, so no DB call ever
runs inside the critical section. A single reentrant lock is used rather than
per-role striping: a `loadedRoles` write performed under the lock can evict a
*different* role and re-enter `clearRolePolicies`, which striped locks would
expose to lock-order inversion.
The clear before a reload is now unconditional. It was previously skipped
when `loadedRoles` had no entry, but rows can exist without an entry after an
incomplete load, and re-adding over them would leave stale rows behind.
3. **`JcasbinLoadedRolesCache` switches to `expireAfterWrite`**, matching
every other authorization cache.
Denials also report the enforcer state behind them, escalating to `WARN`
when a role is recorded as loaded but carries no allow policy. The scan is
gated on `DEBUG` because denials are an ordinary outcome on the request path;
enable `DEBUG` for `JcasbinAuthorizer` on the node under investigation and the
diagnosis itself still arrives at `WARN`.
### Why are the changes needed?
A node could reach a state where `loadedRoles` claims a role is loaded while
the enforcer holds none of its policies. The version check consults
`loadedRoles` alone, so it never notices and keeps skipping the reload; every
request carrying that role is denied on that node while peers serve the same
user correctly. See the issue for the three defects that combine to create and
then pin that state.
Fix: #12230
### Does this PR introduce _any_ user-facing change?
No. No API change and no new configuration property.
### How was this patch tested?
`./gradlew :server-common:test -PskipITs` — 245 tests, 0 failures, including
the 52 existing `TestJcasbinAuthorizer` cases.
Four new tests, each verified to fail when the corresponding fix is reverted:
- `TestJcasbinAuthorizer#testPartialPolicyLoadIsNotRecordedAsLoaded` — a
role whose securable object does not resolve is not recorded as loaded, the
backoff suppresses the immediate retry, and the role loads and is recorded once
the object resolves again. Reverting fix 1 fails it on *"a role whose policies
could not be loaded must not be recorded as loaded"*.
- `TestJcasbinAuthorizer#testClearRolePoliciesSerializesWithRolePolicyLock`
— `clearRolePolicies` blocks while the mutation lock is held and proceeds once
it is released. Reverting fix 2 fails it on *"clearRolePolicies must not
proceed while the role policy lock is held"*.
-
`TestJcasbinLoadedRolesCache#testTtlIsWriteBasedSoReadsCannotKeepAnEntryAlive`
— reads more frequent than the TTL do not keep an entry alive, and expiry still
clears the role's policies. Reverting fix 3 fails it.
- `TestJcasbinLoadedRolesCache#testReplacingAnEntryDoesNotClearPolicies` —
guards the existing behaviour that a refresh must not be treated as a removal.
--
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]
