> On Sept. 19, 2022, 1:24 a.m., Abhay Kulkarni wrote:
> > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
> > Line 107 (original), 102 (patched)
> > <https://reviews.apache.org/r/74112/diff/1/?file=2269952#file2269952line108>
> >
> > 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.
>
> Ramachandran Krishnan wrote:
> Thanks for reviewing it .I did not encounter the issue where
> deletedPolicies is more than one .It is very much possible that record will
> be more than one when compressDelta does not work correctly.In this case
> whatever the code is present is make sense .Let me close this review .I will
> do some testing .If i encounter any issue , i will create the patch for that
> issue
>
> Ramachandran Krishnan wrote:
> 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;
> }
>
> The below switch case statement is not needed
>
> 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;
> }
>
>
> As well as we no need to print the deletedPolicies as part of the log
> .Because we will upload the entire deletedPolicies into ELK stack when we
> index the log files
Or we need to the log under debug like
if (LOG.isDebugEnabled()) {
LOG.debug("Unexpected: found no policy
or multiple policies for CHANGE_TYPE_POLICY_UPDATE: " +
Arrays.toString(deletedPolicies.toArray()));
}
- Ramachandran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74112/#review224677
-----------------------------------------------------------
On Sept. 20, 2022, 4:11 a.m., Ramachandran Krishnan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74112/
> -----------------------------------------------------------
>
> (Updated Sept. 20, 2022, 4:11 a.m.)
>
>
> Review request for ranger and Abhay Kulkarni.
>
>
> 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
>
>
