Re: Review Request 74281: RANGER-4026: Allow sync source updates for existing users synced via different sync sources

2023-01-26 Thread Sailaja Polavarapu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74281/#review225133
---


Ship it!




Ship It!

- Sailaja Polavarapu


On Jan. 15, 2023, 11:13 p.m., Abhishek  Kumar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74281/
> ---
> 
> (Updated Jan. 15, 2023, 11:13 p.m.)
> 
> 
> Review request for ranger, Madhan Neethiraj and Sailaja Polavarapu.
> 
> 
> Bugs: RANGER-4026
> https://issues.apache.org/jira/browse/RANGER-4026
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> The change allows existing external users and groups in ranger to sync from a 
> different sync source.
> 
> 
> Diffs
> -
> 
>   
> ugsync/src/main/java/org/apache/ranger/unixusersync/config/UserGroupSyncConfig.java
>  531e35a95 
>   
> ugsync/src/main/java/org/apache/ranger/unixusersync/process/PolicyMgrUserGroupBuilder.java
>  0aa419f3f 
> 
> 
> Diff: https://reviews.apache.org/r/74281/diff/1/
> 
> 
> Testing
> ---
> 
> mvn clean package runs fine.
> 
> 
> Thanks,
> 
> Abhishek  Kumar
> 
>

Re: Review Request 74112: RANGER-3903:Improvement in RangerPolicyDeltaUtil--> applyDeltas method

2023-01-26 Thread Abhishek Kumar 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74112/#review225134
---




agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Line 62 (original), 64 (patched)


Although the comment explains how it affects the change, I believe it won't 
serve any purpose once the code gets merged. It would be good to have this 
comment as part of this review instead.



agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Lines 70 (patched)


There are no duplicates in the input param List policies so 
the Map> should be Map instead.



agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Lines 72 (patched)


policy.getId() is called 3 times here, assigning it to a variable is 
suggested here.



agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Line 75 (original), 80 (patched)


A better name here would be hasAtLeastOneExpectedServiceType



agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Line 87 (original), 90 (patched)


List deletedPolicies = new ArrayList<>(); 

should be 

RangerPolicy deletedPolicy;

instead.



agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Line 101 (original), 98 (patched)


Null check on the deleted policy should suffice and CollectionUtils may be 
avoided.



agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Line 107 (original), 104 (patched)


size check can be avoided.



agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
Line 113 (original), 110 (patched)


size check can be avoided.


- Abhishek  Kumar


On Jan. 25, 2023, 4:50 a.m., Ramachandran Krishnan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74112/
> ---
> 
> (Updated Jan. 25, 2023, 4:50 a.m.)
> 
> 
> Review request for ranger, Don Bosco Durai, Kirby Zhou, Abhay Kulkarni, 
> Madhan Neethiraj, Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, 
> Selvamohan Neethiraj, Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan 
> Periasamy.
> 
> 
> Bugs: RANGER-3903
> https://issues.apache.org/jira/browse/RANGER-3903
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> After going through the below code snippets in the master branch 
> 
> while (iter.hasNext()) {
> RangerPolicy policy = iter.next();
> if (policyId.equals(policy.getId()) && (changeType == 
> RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == 
> RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) {
> deletedPolicies.add(policy);
> iter.remove();
> }
> }
> switch (changeType) {
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE:
> {
> if (CollectionUtils.isNotEmpty(deletedPolicies)) {
> LOG.warn("Unexpected: found existing policy for 
> CHANGE_TYPE_POLICY_CREATE: " + Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || 
> deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies 
> for CHANGE_TYPE_POLICY_UPDATE: " + 
> Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> case RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE:
> {
> if (CollectionUtils.isEmpty(deletedPolicies) || 
> deletedPolicies.size() > 1) {
> LOG.warn("Unexpected: found no policy or multiple policies 
> for CHANGE_TYPE_POLICY_DELETE: " + 
> Arrays.toString(deletedPolicies.toArray()));
> }
> break;
> }
> default:
> break;
> }
> 
> 1st#improvement:
> 
> From the above code, we iterate delta policies and check if this policy 
> exists in the existing policy, we add that to deletePolicies list.
> 
> The delta change type condition for created/updated/deleted is added on top 
> of the if the condition so adding the condition again is not necessary 
> 
> 2nd#improvement:
> From the above code, we see for each element in the deltas,w

Re: Review Request 74251: RANGER-4012:getPolicyByName searches policy by serviceName, policyName simply by traverse all policies in RangerServicePoliciesCache instead of DB

2023-01-26 Thread Ramachandran Krishnan 


> On Jan. 24, 2023, 9:06 p.m., Abhishek  Kumar wrote:
> > security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java
> > Line 464 (original), 464 (patched)
> > 
> >
> > Client side code needs update:
> > 
> > https://github.com/apache/ranger/blob/master/intg/src/main/java/org/apache/ranger/RangerClient.java#L283
> > 
> > Changes may be required here as well:
> > 
> > ranger-examples/sample-client/src/main/java/org/apache/ranger/examples/sampleclient/SampleClient.java

Abhishek Kumar
Added the code changes in SampleClient for the new API


- Ramachandran


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74251/#review225128
---


On Jan. 25, 2023, 2:49 p.m., Ramachandran Krishnan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74251/
> ---
> 
> (Updated Jan. 25, 2023, 2:49 p.m.)
> 
> 
> Review request for ranger, Don Bosco Durai, Kirby Zhou, Abhay Kulkarni, 
> Madhan Neethiraj, Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, 
> Selvamohan Neethiraj, Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan 
> Periasamy.
> 
> 
> Bugs: RANGER-4012
> https://issues.apache.org/jira/browse/RANGER-4012
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> getPolicyByName searches policy by serviceName, policyName simply by traverse 
> all policies in RangerServicePoliciesCache. 
> 
> However, It takes more time to search for policies from the cache when there 
> are millions of policies
> 
> As well as The above REST API sometimes gives stable data due to the deleted 
> element is present in the Cache 
> 
> We need to call the DB to fetch policy instead of calling 
> RangerServicePoliciesCache
> 
> In PublicAPIsv2 we add the API's which are available in ServiceREST as an API 
> and the getPolicyByName is not available as an API in ServiceREST.
> 
> getPolicyByName ---> (/api/service/{servicename}/policy/{policyname}) in 
> PublicAPIsv2
> 
> I guess we should add the below  API in ServiceREST also for the same.
> 
> getPolicyByName ---> (/policies/service/{serviceName}/policy/{policyName}) in 
> ServiceREST
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/ranger/RangerClient.java e4e3a57ad 
>   
> ranger-examples/sample-client/src/main/java/org/apache/ranger/examples/sampleclient/SampleClient.java
>  d0202e47e 
>   security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java 
> d98910bee 
>   security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 
> ec02f47f7 
>   security-admin/src/test/java/org/apache/ranger/rest/TestPublicAPIsv2.java 
> 7409883ab 
>   security-admin/src/test/java/org/apache/ranger/rest/TestServiceREST.java 
> 8fdcc43c8 
> 
> 
> Diff: https://reviews.apache.org/r/74251/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ramachandran Krishnan
> 
>

Re: Review Request 74112: RANGER-3903:Improvement in RangerPolicyDeltaUtil--> applyDeltas method

2023-01-26 Thread Ramachandran Krishnan 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74112/
---

(Updated Jan. 27, 2023, 3:51 a.m.)


Review request for ranger, Don Bosco Durai, Kirby Zhou, Abhay Kulkarni, Madhan 
Neethiraj, Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan 
Neethiraj, Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan Periasamy.


Bugs: RANGER-3903
https://issues.apache.org/jira/browse/RANGER-3903


Repository: ranger


Description (updated)
---

After going through the below code snippets in the master branch 

while (iter.hasNext()) {
RangerPolicy policy = iter.next();
if (policyId.equals(policy.getId()) && (changeType == 
RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == 
RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) {
deletedPolicies.add(policy);
iter.remove();
}
}
switch (changeType) {
case RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE:
{
if (CollectionUtils.isNotEmpty(deletedPolicies)) {
LOG.warn("Unexpected: found existing policy for 
CHANGE_TYPE_POLICY_CREATE: " + Arrays.toString(deletedPolicies.toArray()));
}
break;
}
case RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE:
{
if (CollectionUtils.isEmpty(deletedPolicies) || 
deletedPolicies.size() > 1) {
LOG.warn("Unexpected: found no policy or multiple policies for 
CHANGE_TYPE_POLICY_UPDATE: " + Arrays.toString(deletedPolicies.toArray()));
}
break;
}
case RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE:
{
if (CollectionUtils.isEmpty(deletedPolicies) || 
deletedPolicies.size() > 1) {
LOG.warn("Unexpected: found no policy or multiple policies for 
CHANGE_TYPE_POLICY_DELETE: " + Arrays.toString(deletedPolicies.toArray()));
}
break;
}
default:
break;
}

1st#improvement:

>From the above code, we iterate delta policies and check if this policy exists 
>in the existing policy, we add that to deletePolicies list.

The delta change type condition for created/updated/deleted is added on top of 
the if the condition so adding the condition again is not necessary 

2nd#improvement:
>From the above code, we see for each element in the deltas,we iterate policies 
>and check if this delta policy exists in the existing policy, we add that to 
>deletePolicies list.

Solution:
We need to use Map instead of iterating policies for every element of deltas 
--> Map policiesIdMap
Building index map key will be policyId and value will be policy associated 
with the same policyId
For  each policy in the deltas ,we check on the policiesIdMap whether the same 
policyId is present or not ?.
if yes, we will add all the associated policy  into deletedPolicy variable and 
remove the policyId from policiesIdMap
After an end of the iteration,we will iterate the policiesIdMap and get all the 
policy associated with policyId and add into result set

This will give better performance when the policies list is huge.


Diffs (updated)
-

  
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
 e9223fe69 


Diff: https://reviews.apache.org/r/74112/diff/8/

Changes: https://reviews.apache.org/r/74112/diff/7-8/


Testing
---

Tested the below Rest API's to make sure everything works fine

1.  ServiceREST Rest API :GET /plugins/policies/download/{serviceName}

2.  ServiceREST Rest API :GET /plugins/secure/policies/download/{serviceName}


Thanks,

Ramachandran Krishnan