----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74112/#review224677 -----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 107 (original), 102 (patched) <https://reviews.apache.org/r/74112/#comment313474> Current code here (and in line 100) is to ensure that the call to ServiceDBStore.compressDeltas() - which builds the list of deltas that is input to this function - is returning correct set of policy-deltas. For example, if compressDelta does not work correctly, then there may be more than one record for a given policy with change-type of either UPDATE or DELETE ( or there may a delta for DELETE as well as CREATE of the same policy). These checks ensure that the value of the argument "deltas" is internally consistent. agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 113 (original), 108 (patched) <https://reviews.apache.org/r/74112/#comment313473> For a deleted policy, there will always be exactly one element in deletedPolicies array. Therefore, the error condition is if that is not true. Please review this change, and provide a test case where the existing condition is shown to be incorrect. - Abhay Kulkarni On Sept. 13, 2022, 8:56 a.m., Ramachandran Krishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74112/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2022, 8:56 a.m.) > > > Review request for ranger, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, > Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja > Polavarapu, and Velmurugan Periasamy. > > > Bugs: RANGER-3903 > https://issues.apache.org/jira/browse/RANGER-3903 > > > Repository: ranger > > > Description > ------- > > 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; > } > > > > we iterate delta policies and check if this policy exists in the existing > policy, we add that to deletePolicies list. > > If a delta change type is created, we expect that it should not be in the > existing old policy which should not have happened. So the below block code > is not needed > > > If policy Id is uniqiue and found a match, we will add the break instead of > iterating the entire list > > > > 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(); > } > } > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java > e9223fe69 > > > Diff: https://reviews.apache.org/r/74112/diff/1/ > > > 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 > >
