Repository: ranger Updated Branches: refs/heads/ranger-1.1 66d571ee3 -> a0738db90
RANGER-2158: Performance improvement to REST API call to update policy Project: http://git-wip-us.apache.org/repos/asf/ranger/repo Commit: http://git-wip-us.apache.org/repos/asf/ranger/commit/a0738db9 Tree: http://git-wip-us.apache.org/repos/asf/ranger/tree/a0738db9 Diff: http://git-wip-us.apache.org/repos/asf/ranger/diff/a0738db9 Branch: refs/heads/ranger-1.1 Commit: a0738db908b7f203c222d51d99365e5d94348410 Parents: 66d571e Author: Abhay Kulkarni <[email protected]> Authored: Sun Jul 22 18:52:04 2018 -0700 Committer: Mehul Parikh <[email protected]> Committed: Fri Jul 27 14:33:01 2018 +0530 ---------------------------------------------------------------------- .../model/validation/RangerPolicyValidator.java | 95 ++++++++++---------- .../model/validation/RangerValidator.java | 22 ++--- .../ranger/plugin/store/ServiceStore.java | 2 + .../validation/TestRangerPolicyValidator.java | 49 ++++------ .../model/validation/TestRangerValidator.java | 36 -------- .../org/apache/ranger/biz/ServiceDBStore.java | 19 +++- .../ranger/service/RangerPolicyService.java | 11 ++- 7 files changed, 94 insertions(+), 140 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/ranger/blob/a0738db9/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java ---------------------------------------------------------------------- diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java index 4d2fa23..9de860d 100644 --- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java +++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java @@ -167,72 +167,67 @@ public class RangerPolicyValidator extends RangerValidator { } String policyName = policy.getName(); String serviceName = policy.getService(); - if (StringUtils.isBlank(policyName)) { + + RangerService service = null; + boolean serviceNameValid = false; + if (StringUtils.isBlank(serviceName)) { ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_MISSING_FIELD; failures.add(new ValidationFailureDetailsBuilder() - .field("name") - .isMissing() - .becauseOf(error.getMessage("name")) - .errorCode(error.getErrorCode()) - .build()); + .field("service name") + .isMissing() + .becauseOf(error.getMessage("service name")) + .errorCode(error.getErrorCode()) + .build()); valid = false; } else { - List<RangerPolicy> policies = getPolicies(serviceName, policyName); - if (CollectionUtils.isNotEmpty(policies)) { - if (policies.size() > 1) { - ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_POLICY_NAME_MULTIPLE_POLICIES_WITH_SAME_NAME; - failures.add(new ValidationFailureDetailsBuilder() - .field("name") - .isAnInternalError() - .becauseOf(error.getMessage(policyName)) - .errorCode(error.getErrorCode()) - .build()); - valid = false; - } else if (action == Action.CREATE) { // size == 1 - ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_POLICY_NAME_CONFLICT; - failures.add(new ValidationFailureDetailsBuilder() - .field("policy name") - .isSemanticallyIncorrect() - .becauseOf(error.getMessage(policies.iterator().next().getId(), serviceName)) - .errorCode(error.getErrorCode()) - .build()); - valid = false; - } else if (!policies.iterator().next().getId().equals(id)) { // size == 1 && action == UPDATE - ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_POLICY_NAME_CONFLICT; - failures.add(new ValidationFailureDetailsBuilder() - .field("id/name") + service = getService(serviceName); + if (service == null) { + ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_INVALID_SERVICE_NAME; + failures.add(new ValidationFailureDetailsBuilder() + .field("service name") .isSemanticallyIncorrect() - .becauseOf(error.getMessage(policies.iterator().next().getId(), serviceName)) + .becauseOf(error.getMessage(serviceName)) .errorCode(error.getErrorCode()) .build()); - valid = false; - } + valid = false; + } else { + serviceNameValid = true; } } - RangerService service = null; - boolean serviceNameValid = false; - if (StringUtils.isBlank(serviceName)) { + + if (StringUtils.isBlank(policyName)) { ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_MISSING_FIELD; failures.add(new ValidationFailureDetailsBuilder() - .field("service name") + .field("name") .isMissing() - .becauseOf(error.getMessage("service name")) + .becauseOf(error.getMessage("name")) .errorCode(error.getErrorCode()) .build()); valid = false; } else { - service = getService(serviceName); - if (service == null) { - ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_INVALID_SERVICE_NAME; - failures.add(new ValidationFailureDetailsBuilder() - .field("service name") - .isSemanticallyIncorrect() - .becauseOf(error.getMessage(serviceName)) - .errorCode(error.getErrorCode()) - .build()); - valid = false; - } else { - serviceNameValid = true; + if (service != null) { + Long policyId = getPolicyId(service.getId(), policyName); + if (policyId != null) { + if (action == Action.CREATE) { + ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_POLICY_NAME_CONFLICT; + failures.add(new ValidationFailureDetailsBuilder() + .field("policy name") + .isSemanticallyIncorrect() + .becauseOf(error.getMessage(policyId, serviceName)) + .errorCode(error.getErrorCode()) + .build()); + valid = false; + } else if (!policyId.equals(id)) { // action == UPDATE + ValidationErrorCode error = ValidationErrorCode.POLICY_VALIDATION_ERR_POLICY_NAME_CONFLICT; + failures.add(new ValidationFailureDetailsBuilder() + .field("id/name") + .isSemanticallyIncorrect() + .becauseOf(error.getMessage(policyId, serviceName)) + .errorCode(error.getErrorCode()) + .build()); + valid = false; + } + } } } http://git-wip-us.apache.org/repos/asf/ranger/blob/a0738db9/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java ---------------------------------------------------------------------- diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java index f2f5977..ed5aa8d 100644 --- a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java +++ b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java @@ -247,29 +247,23 @@ public abstract class RangerValidator { return result; } - List<RangerPolicy> getPolicies(final String serviceName, final String policyName) { + Long getPolicyId(final Long serviceId, final String policyName) { if(LOG.isDebugEnabled()) { - LOG.debug("==> RangerValidator.getPolicies(" + serviceName + ", " + policyName + ")"); + LOG.debug("==> RangerValidator.getPolicyId(" + serviceId + ", " + policyName + ")"); } - List<RangerPolicy> policies = null; + Long policyId = null; try { - SearchFilter filter = new SearchFilter(); - if (StringUtils.isNotBlank(policyName)) { - filter.setParam(SearchFilter.POLICY_NAME, policyName); - } - filter.setParam(SearchFilter.SERVICE_NAME, serviceName); - - policies = _store.getPolicies(filter); + policyId = _store.getPolicyId(serviceId, policyName); + } catch (Exception e) { LOG.debug("Encountred exception while retrieving service from service store!", e); } - + if(LOG.isDebugEnabled()) { - int count = policies == null ? 0 : policies.size(); - LOG.debug("<== RangerValidator.getPolicies(" + serviceName + ", " + policyName + "): count[" + count + "], " + policies); + LOG.debug("<== RangerValidator.getPolicyId(" + serviceId + ", " + policyName + "): policy-id[" + policyId + "]"); } - return policies; + return policyId; } List<RangerPolicy> getPoliciesForResourceSignature(String serviceName, String policySignature) { http://git-wip-us.apache.org/repos/asf/ranger/blob/a0738db9/agents-common/src/main/java/org/apache/ranger/plugin/store/ServiceStore.java ---------------------------------------------------------------------- diff --git a/agents-common/src/main/java/org/apache/ranger/plugin/store/ServiceStore.java b/agents-common/src/main/java/org/apache/ranger/plugin/store/ServiceStore.java index 2c57a6f..9924cb4 100644 --- a/agents-common/src/main/java/org/apache/ranger/plugin/store/ServiceStore.java +++ b/agents-common/src/main/java/org/apache/ranger/plugin/store/ServiceStore.java @@ -74,6 +74,8 @@ public interface ServiceStore { List<RangerPolicy> getPolicies(SearchFilter filter) throws Exception; + Long getPolicyId(final Long serviceId, final String policyName); + PList<RangerPolicy> getPaginatedPolicies(SearchFilter filter) throws Exception; List<RangerPolicy> getPoliciesByResourceSignature(String serviceName, String policySignature, Boolean isPolicyEnabled) throws Exception; http://git-wip-us.apache.org/repos/asf/ranger/blob/a0738db9/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java ---------------------------------------------------------------------- diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java index d8a1354..140a9ed 100644 --- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java +++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java @@ -42,7 +42,6 @@ import org.apache.ranger.plugin.model.RangerServiceDef.RangerResourceDef; import org.apache.ranger.plugin.model.validation.RangerValidator.Action; import org.apache.ranger.plugin.store.ServiceStore; import org.apache.ranger.plugin.util.RangerObjectFactory; -import org.apache.ranger.plugin.util.SearchFilter; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -229,6 +228,7 @@ public class TestRangerPolicyValidator { // service name exists RangerService service = mock(RangerService.class); when(service.getType()).thenReturn("service-type"); + when(service.getId()).thenReturn(2L); when(_store.getServiceByName("service-name")).thenReturn(service); // service points to a valid service-def _serviceDef = _utils.createServiceDefWithAccessTypes(accessTypes); @@ -240,17 +240,7 @@ public class TestRangerPolicyValidator { when(existingPolicy.getId()).thenReturn(8L); when(existingPolicy.getService()).thenReturn("service-name"); when(_store.getPolicy(8L)).thenReturn(existingPolicy); - SearchFilter createFilter = new SearchFilter(); - createFilter.setParam(SearchFilter.SERVICE_TYPE, "service-type"); - createFilter.setParam(SearchFilter.POLICY_NAME, "policy-name-1"); // this name would be used for create - when(_store.getPolicies(createFilter)).thenReturn(new ArrayList<RangerPolicy>()); // a matching policy should not exist for update. - SearchFilter updateFilter = new SearchFilter(); - updateFilter.setParam(SearchFilter.SERVICE_TYPE, "service-type"); - updateFilter.setParam(SearchFilter.POLICY_NAME, "policy-name-2"); // this name would be used for update - List<RangerPolicy> existingPolicies = new ArrayList<>(); - existingPolicies.add(existingPolicy); - when(_store.getPolicies(updateFilter)).thenReturn(existingPolicies); // valid policy can have empty set of policy items if audit is turned on // null value for audit is treated as audit on. // for now we want to turn any resource related checking off @@ -262,6 +252,7 @@ public class TestRangerPolicyValidator { if (action == Action.CREATE) { when(_policy.getId()).thenReturn(7L); when(_policy.getName()).thenReturn("policy-name-1"); + when(_store.getPolicyId(service.getId(), _policy.getName())).thenReturn(null); Assert.assertTrue("" + action + ", " + auditEnabled, _validator.isValid(_policy, action, isAdmin, _failures)); Assert.assertTrue(_failures.isEmpty()); } else { @@ -272,6 +263,7 @@ public class TestRangerPolicyValidator { Assert.assertTrue(_failures.isEmpty()); when(_policy.getName()).thenReturn("policy-name-2"); + when(_store.getPolicyId(service.getId(), _policy.getName())).thenReturn(null); Assert.assertTrue("" + action + ", " + auditEnabled, _validator.isValid(_policy, action, isAdmin, _failures)); Assert.assertTrue(_failures.isEmpty()); } @@ -370,20 +362,22 @@ public class TestRangerPolicyValidator { checkFailure_isValid(action, "missing", "id"); } } + RangerService service = mock(RangerService.class); /* * Id is ignored for Create but name should not belong to an existing policy. For update, policy should exist for its id and should match its name. */ when(_policy.getName()).thenReturn("policy-name"); when(_policy.getService()).thenReturn("service-name"); + when(_store.getServiceByName("service-name")).thenReturn(service); + when(service.getId()).thenReturn(2L); + RangerPolicy existingPolicy = mock(RangerPolicy.class); when(existingPolicy.getId()).thenReturn(7L); + when(existingPolicy.getService()).thenReturn("service-name"); List<RangerPolicy> existingPolicies = new ArrayList<>(); - existingPolicies.add(existingPolicy); - SearchFilter filter = new SearchFilter(); - filter.setParam(SearchFilter.SERVICE_NAME, "service-name"); - filter.setParam(SearchFilter.POLICY_NAME, "policy-name"); - when(_store.getPolicies(filter)).thenReturn(existingPolicies); + + when(_store.getPolicyId(service.getId(), "policy-name")).thenReturn(7L); checkFailure_isValid(Action.CREATE, "semantic", "policy name"); // update : does not exist for id @@ -395,21 +389,11 @@ public class TestRangerPolicyValidator { when(_store.getPolicy(7L)).thenReturn(existingPolicy); RangerPolicy anotherExistingPolicy = mock(RangerPolicy.class); when(anotherExistingPolicy.getId()).thenReturn(8L); - existingPolicies.clear(); + when(anotherExistingPolicy.getService()).thenReturn("service-name"); + existingPolicies.add(anotherExistingPolicy); - when(_store.getPolicies(filter)).thenReturn(existingPolicies); + when(_store.getPolicyId(service.getId(), "policy-name")).thenReturn(8L); checkFailure_isValid(Action.UPDATE, "semantic", "id/name"); - - // more than one policies with same name is also an internal error - when(_policy.getName()).thenReturn("policy-name"); - when(_store.getPolicies(filter)).thenReturn(existingPolicies); - existingPolicies.add(existingPolicy); - existingPolicy = mock(RangerPolicy.class); - existingPolicies.add(existingPolicy); - for (boolean isAdmin : new boolean[] { true, false }) { - _failures.clear(); Assert.assertFalse(_validator.isValid(_policy, Action.UPDATE, isAdmin, _failures)); - _utils.checkFailureForInternalError(_failures); - } // policy must have service name on it and it should be valid when(_policy.getName()).thenReturn("policy-name"); @@ -450,9 +434,6 @@ public class TestRangerPolicyValidator { // policy must contain at least one policy item List<RangerPolicyItem> policyItems = new ArrayList<>(); - when(_policy.getService()).thenReturn("service-name"); - RangerService service = mock(RangerService.class); - when(_store.getServiceByName("service-name")).thenReturn(service); for (Action action : cu) { for (boolean isAdmin : new boolean[] { true, false }) { // when it is null @@ -474,6 +455,8 @@ public class TestRangerPolicyValidator { when(_store.getServiceDefByName("service-type")).thenReturn(null); for (Action action : cu) { for (boolean isAdmin : new boolean[] { true, false }) { + when(_policy.getService()).thenReturn("service-name"); + when(_store.getServiceByName("service-name")).thenReturn(service); _failures.clear(); Assert.assertFalse(_validator.isValid(_policy, action, isAdmin, _failures)); _utils.checkFailureForInternalError(_failures, "policy service def"); } @@ -491,7 +474,7 @@ public class TestRangerPolicyValidator { // create the right service def with right resource defs - this is the same as in the happypath test above. _serviceDef = _utils.createServiceDefWithAccessTypes(accessTypes, "service-type"); - when(_store.getPolicies(filter)).thenReturn(null); + when(_store.getPolicyId(service.getId(), "policy-name")).thenReturn(null); List<RangerResourceDef> resourceDefs = _utils.createResourceDefs(resourceDefData); when(_serviceDef.getResources()).thenReturn(resourceDefs); when(_store.getServiceDefByName("service-type")).thenReturn(_serviceDef); http://git-wip-us.apache.org/repos/asf/ranger/blob/a0738db9/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java ---------------------------------------------------------------------- diff --git a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java index a749b27..f9b3428 100644 --- a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java +++ b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java @@ -405,42 +405,6 @@ public class TestRangerValidator { result = _validator.getIsAuditEnabled(policy); Assert.assertTrue(result); } - - @Test - public void test_getPolicies() throws Exception { - - // returns null when store returns null - String policyName = "aPolicy"; - String serviceName = "aService"; - SearchFilter filter = new SearchFilter(); - filter.setParam(SearchFilter.POLICY_NAME, policyName); - filter.setParam(SearchFilter.SERVICE_NAME, serviceName); - - when(_store.getPolicies(filter)).thenReturn(null); - List<RangerPolicy> result = _validator.getPolicies(serviceName, policyName); - // validate store is queried with both parameters - verify(_store).getPolicies(filter); - Assert.assertNull(result); - - // returns null if store throws an exception - when(_store.getPolicies(filter)).thenThrow(new Exception()); - result = _validator.getPolicies(serviceName, policyName); - Assert.assertNull(result); - - // does not shove policy into search filter if policy name passed in is "blank" - filter = new SearchFilter(); - filter.setParam(SearchFilter.SERVICE_NAME, serviceName); - - List<RangerPolicy> policies = new ArrayList<>(); - RangerPolicy policy = mock(RangerPolicy.class); - policies.add(policy); - - when(_store.getPolicies(filter)).thenReturn(policies); - for (String aName : new String[]{ null, "", " "}) { - result = _validator.getPolicies(serviceName, aName); - Assert.assertTrue(result.iterator().next() == policy); - } - } @Test public void test_getServiceDef_byId() throws Exception { http://git-wip-us.apache.org/repos/asf/ranger/blob/a0738db9/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java ---------------------------------------------------------------------- diff --git a/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java b/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java index f00d311..8efc950 100644 --- a/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java +++ b/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java @@ -2001,7 +2001,7 @@ public class ServiceDBStore extends AbstractServiceStore { policy.setGuid(xxExisting.getGuid()); policy.setVersion(xxExisting.getVersion()); - List<XXTrxLog> trxLogList = policyService.getTransactionLog(policy, xxExisting, RangerPolicyService.OPERATION_UPDATE_CONTEXT); + List<XXTrxLog> trxLogList = policyService.getTransactionLog(policy, xxExisting, existing, RangerPolicyService.OPERATION_UPDATE_CONTEXT); updatePolicySignature(policy); @@ -2127,6 +2127,23 @@ public class ServiceDBStore extends AbstractServiceStore { return ret; } + @Override + public Long getPolicyId(final Long serviceId, final String policyName) { + if(LOG.isDebugEnabled()) { + LOG.debug("==> ServiceDBStore.getPolicyId()"); + } + Long ret = null; + XXPolicy xxPolicy = daoMgr.getXXPolicy().findByNameAndServiceId(policyName, serviceId); + if (xxPolicy != null) { + ret = xxPolicy.getId(); + } + if(LOG.isDebugEnabled()) { + LOG.debug("<== ServiceDBStore.getPolicyId()"); + } + return ret; + } + + public void getPoliciesInExcel(List<RangerPolicy> policies, HttpServletResponse response) throws Exception { if (LOG.isDebugEnabled()) { LOG.debug("==> ServiceDBStore.getPoliciesInExcel()"); http://git-wip-us.apache.org/repos/asf/ranger/blob/a0738db9/security-admin/src/main/java/org/apache/ranger/service/RangerPolicyService.java ---------------------------------------------------------------------- diff --git a/security-admin/src/main/java/org/apache/ranger/service/RangerPolicyService.java b/security-admin/src/main/java/org/apache/ranger/service/RangerPolicyService.java index 519d8e9..a3ff825 100644 --- a/security-admin/src/main/java/org/apache/ranger/service/RangerPolicyService.java +++ b/security-admin/src/main/java/org/apache/ranger/service/RangerPolicyService.java @@ -138,10 +138,10 @@ public class RangerPolicyService extends RangerPolicyServiceBase<XXPolicy, Range } public List<XXTrxLog> getTransactionLog(RangerPolicy vPolicy, int action) { - return getTransactionLog(vPolicy, null, action); + return getTransactionLog(vPolicy, null, null, action); } - public List<XXTrxLog> getTransactionLog(RangerPolicy vObj, XXPolicy mObj, int action) { + public List<XXTrxLog> getTransactionLog(RangerPolicy vObj, XXPolicy mObj, RangerPolicy oldPolicy, int action) { if (vObj == null || action == 0 || (action == OPERATION_UPDATE_CONTEXT && mObj == null)) { return null; } @@ -157,7 +157,7 @@ public class RangerPolicyService extends RangerPolicyServiceBase<XXPolicy, Range if (!trxLogAttrs.containsKey(field.getName())) { continue; } - XXTrxLog xTrxLog = processFieldToCreateTrxLog(field, objectName, nameField, vObj, mObj, action); + XXTrxLog xTrxLog = processFieldToCreateTrxLog(field, objectName, nameField, vObj, mObj, oldPolicy, action); if (xTrxLog != null) { trxLogList.add(xTrxLog); } @@ -167,7 +167,7 @@ public class RangerPolicyService extends RangerPolicyServiceBase<XXPolicy, Range .getDeclaredFields(); for (Field field : superClassFields) { if ("isEnabled".equalsIgnoreCase(field.getName())) { - XXTrxLog xTrx = processFieldToCreateTrxLog(field, objectName, nameField, vObj, mObj, action); + XXTrxLog xTrx = processFieldToCreateTrxLog(field, objectName, nameField, vObj, mObj, oldPolicy, action); if (xTrx != null) { trxLogList.add(xTrx); } @@ -184,7 +184,7 @@ public class RangerPolicyService extends RangerPolicyServiceBase<XXPolicy, Range } private XXTrxLog processFieldToCreateTrxLog(Field field, String objectName, - Field nameField, RangerPolicy vObj, XXPolicy mObj, int action) { + Field nameField, RangerPolicy vObj, XXPolicy mObj, RangerPolicy oldPolicy, int action) { String actionString = ""; @@ -274,7 +274,6 @@ public class RangerPolicyService extends RangerPolicyServiceBase<XXPolicy, Range break; } } - RangerPolicy oldPolicy = populateViewBean(mObj); if (POLICY_RESOURCE_CLASS_FIELD_NAME.equalsIgnoreCase(fieldName)) { if (oldPolicy != null) { oldValue = processPolicyResourcesForTrxLog(oldPolicy.getResources());
