RANGER-5678: Concurrent policy-engine rebuild on shared serviceDef ca…

[Sanket-Shelar]

…uses CME/NPE for delegated-admin User

What changes were proposed in this pull request?

Fixes concurrent policy-engine rebuilds mutating the shared cached RangerServiceDef during ServiceDefUtil.normalize(), which caused CME/NPE and getPolicyAdmin failures.

Changes: Added ServiceDefUtil.copyServiceDef() (JSON deep copy) and updated RangerPolicyRepository to normalize a private copy of serviceDef instead of the cache object. Added a unit test to ensure the source is not mutated.

How was this patch tested?

Local build passed, Unit tests added and tested.

[vyommani]

Thanks for digging into this, Sanket - the synchronized(policies) change does stop the crash for same-service concurrent rebuilds, confirmed that with a test. But I don't think this is the right fix, for a few reasons, and I'd like to hold off merging until we talk it through.

It locks far more than it needs to. The actual race is a handful of List mutations inside ServiceDefUtil.normalize() - a few clear()/addAll() and removeAll() calls. This patch instead synchronizes the entire RangerPolicyAdminImpl construction: building every policy evaluator, every context enricher, every resource trie for the service. Under the concurrent delegated-admin load that triggers this bug, that means policy engine builds for the same service go from running in parallel to queuing up one at a time behind this lock. That's a real latency cost under exactly the load pattern we're trying to fix, not a hypothetical one.

It doesn't cover the actual shared object. policies is a per-service ServicePolicies instance, but what's actually being mutated unsafely is the RangerServiceDef nested inside it - specifically the tag-service one, which isn't service-specific. It's the same object referenced from every component service's ServicePolicies. Two different services rebuilding concurrently synchronize on two different policies objects, so this lock provides no protection between them at all, even though they can both be calling normalize() on the identical shared tag serviceDef at the same time.

I didn't want to just argue this, so I wrote three concurrency tests against addPolicyAdmin() directly (happy to share them): same ServicePolicies instance across threads, a fresh instance per call wrapping the same RangerServiceDef, and two different services sharing one tag serviceDef. With this patch applied and confirmed compiled in, the first two pass - so the lock does work for the case it's scoped to. The third one fails, reproducing the exact CME/NPE from the original report, just across two services instead of one. That's not a hypothetical gap, it's reproducible.

Proposed fix instead: give RangerServiceDef (and its superclass RangerBaseModelObject) proper copy constructors - deep-copying the nested RangerDataMaskDef/RangerRowFilterDef and their access-type/resource lists, not just the top-level lists - and have ServiceDefUtil.normalize() copy the serviceDef before mutating it, instead of mutating the shared cached instance in place. Once normalize() operates on a private copy, there's no shared mutable state left for any lock to have to get right, so no lock is needed at all. I ran all three tests against this version and all three pass, including the cross-service case.

This also fixes the bug at its actual source rather than at one caller of it - any other code path that calls normalize() on a shared serviceDef gets the same protection for free, which this patch doesn't give us.

I am attaching my patch which resolve this CME without any lock. It has some memory cost but i believe we have to live with that.

change.patch