RANGER-5230: Synchronous policy cache download via plugin service (repo) configs

[fateh288]

What changes were proposed in this pull request?

Proposes new config set via service plugin configs (repo configs) ranger.plugin.<service_name>.policy.refresh.synchronous=true that can be used to synchronously download policies during policy evaluation to reduce time waiting for plugin tests to download latest policies

How was this patch tested?

Manually tested and mvn clean install

1. Start docker containers.
2. Make policy changes for hbase service
3. Tested with hbase plugin for synchronous policy updates — policy changes reflect immediately without waiting for 30 seconds
4. Some sample hbase commands for reference:

   create 't1', 'cf2'

put 't1', 'r2', 'cf2:c2', 10

I have also tested that config can be set from ranger UI / service configs.

All docker containers come up successfully with proposed changes, however synchronous policy download I have tested for hbase and hdfs plugin only

[fateh288]

@dhavalshah9131
I think you implemented a way to add service configs for all services using a single config at ranger admin, can you share details on that (is there a specific prefix required for that ?) -- can that be used here ?

[mneethiraj]

• to be consistent with rest of policy related plugin configurations, I suggest using properyy prefix ranger.plugin.<serviceType>.policy.
• I suggest renaming cache.refresh.mode to refresh.synchronous, with default value as faise
ranger.plugin.hive.policy.refresh.synchronous
• isSynchronousPolicyCacheUpdateEnabled() => isSynchronousPolicyRefresh()

[fateh288]

@mneethiraj

1. The convention that "ranger admin configs" starting with ranger.plugins.conf will sync as service configs to all the plugins has been introduced in RANGER-5215:
   https://github.com/apache/ranger/blob/master/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java#L233
   The property prefix you are mentioning will be used when the property needs to be set at "per service level". Anything starting with ranger.plugins.conf will be synced to all services -- thus I have used it here

2. Addressed

3. Addressed

[mneethiraj]

This configuration is used in the plugin side (and not in Ranger admin server). Hence, I suggest using existing convention in naming configurations - "ranger.plugin." + serviceType + ".policy.refresh.synchronous".

[fateh288]

But we want to set it at ranger admin level via ranger-admin-site.xml and not via service configs from Ranger UI / repo settings.
If we want to set this config on a per service basis from Ranger UI, then we can do what you are recommending --- my initial implementation / commit in this PR had this implementation but I changed it after discussion with @spolavarapu1
We can go either ways depending on our need - whether we want to set it on per service level from ranger UI service configs or at once via ranger admin configs

[fateh288]

This configuration is used in the plugin side (and not in Ranger admin server) ==>. this is not true --- ranger admin configs starting with "ranger.plugins.conf " are now synced to all the plugins too

[mneethiraj]

This configuration is used in the plugin side (and not in Ranger admin server). Hence, I suggest using existing convention in naming configurations - "ranger.plugin." + serviceType + ".policy.refresh.synchronous".

[vyommani]

Thank you for sharing the proposed change to reduce test wait times for policy/tag synchronization in RangerBasePlugin. I understand the goal is to minimize delays by introducing a configuration to control policy refreshes. However, I have concerns about the approach of calling refreshPoliciesAndTags() within isAccessAllowed() when the synchronousPolicyRefresh flag is enabled.


isAccessAllowed() is a public API designed solely to evaluate access permissions. Embedding a call to refreshPoliciesAndTags(), even if guarded by a flag, introduces a secondary responsibility of policy/tag synchronisation. This violates SRP and makes the method’s behaviour less predictable, as callers may not expect a refresh to occur.


Both isAccessAllowed() and refreshPoliciesAndTags() are public methods with distinct purposes. Mixing their responsibilities, even under a configuration flag, could lead to unintended side effects. Since isAccessAllowed() is a performance-critical API, enabling synchronous refreshes could significantly slow down production systems if the flag is misconfigured by a client. This risk is particularly concerning for a public API where users expect consistent and predictable behaviour.


I’m curious did you explore calling refreshPoliciesAndTags() explicitly before invoking isAccessAllowed(). While modifying every test to include an explicit
refresh may not be ideal, other options could be considered, such as, Adjust the default sync interval (30s to 200ms or 1 second) to minimize wait times without
embedding refresh logic in isAccessAllowed().


Have you conducted performance tests to evaluate the impact of this change? Even if the additional if condition for the flag adds minimal overhead (a few
nanoseconds ), understanding the performance implications is critical, particularly for a public API used in production environments.

I recommend keeping isAccessAllowed() focused on its core responsibility and exploring alternative ways to achieve the goal of reducing test wait times. Let’s maintain the separation of concerns between these public APIs to ensure clarity, predictability, and performance.

Please let me know your thoughts or if there are additional considerations (e.g., specific constraints that led to this design). I’d be happy to discuss further or assist in exploring other solutions.

[fateh288]

@vyommani
Thanks a lot for your comments.
We cannot call refreshPoliciesAndTags from each test as the synchronous policy download config has been developed for ranger integration tests and not unit tests wherein we have control of when to call this method. The components still will call call only isAccessAllowed and not any other methods.
As you mentioned, the performance impact will be minimal since its just a boolean check.
The config is not exposed via UI for any component. The config can only be enabled by ranger admins so while there is a possibility of misconfiguration, I consider it as a low risk as misconfiguration of any property in general can lead to performance degradation.
I think that configuration management by admins or any privileged users is in general a safe idea as one signs up for any side effects when any config changes are made.
@mneethiraj can add any additional comments on what is his opinion.

[vyommani]

@fateh288

Have you considered adding a new method as shown below?

public RangerAccessResult isAccessAllowedSync(RangerAccessRequest request) {
refreshPoliciesAndTags();
return isAccessAllowed(request);
}
This approach keeps the existing isAccessAllowed method unchanged and allows you to use the new method to reduce the execution time of your integration tests. If you decide against adding a new method, please provide a report detailing any performance impact of your code changes. I strongly recommend adding the new method rather than modifying the existing one.

[mneethiraj]

@vyommani - with this approach (of having isAccessAllowedSync() methods), every plugin must be updated to call *Sync() method version instead of existing calls toisAccessAllowed() methods when a flag is set. This will be a significant task, involving updates to large number of modules (20+ plugins?). The performance impact of additional if on a boolean member in each authz call should be negligible. I suggest avoiding updates to large number of modules, and go with the approach in this PR.