From commits-return-4558-archive-asf-public=cust-asf.ponee.io@ranger.apache.org Fri Jul 27 11:04:46 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id CDC4B180657 for ; Fri, 27 Jul 2018 11:04:45 +0200 (CEST) Received: (qmail 90667 invoked by uid 500); 27 Jul 2018 09:04:44 -0000 Mailing-List: contact commits-help@ranger.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@ranger.apache.org Delivered-To: mailing list commits@ranger.apache.org Received: (qmail 90658 invoked by uid 99); 27 Jul 2018 09:04:44 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 27 Jul 2018 09:04:44 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 70325DFBC7; Fri, 27 Jul 2018 09:04:44 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: mehul@apache.org To: commits@ranger.apache.org Message-Id: X-Mailer: ASF-Git Admin Mailer Subject: ranger git commit: RANGER-2158: Performance improvement to REST API call to update policy Date: Fri, 27 Jul 2018 09:04:44 +0000 (UTC) Repository: ranger Updated Branches: refs/heads/ranger-1 77be4a1ab -> b9db17321 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/b9db1732 Tree: http://git-wip-us.apache.org/repos/asf/ranger/tree/b9db1732 Diff: http://git-wip-us.apache.org/repos/asf/ranger/diff/b9db1732 Branch: refs/heads/ranger-1 Commit: b9db173212c6642ab8812fd2ab02ef2765e27102 Parents: 77be4a1 Author: Abhay Kulkarni Authored: Sun Jul 22 18:52:04 2018 -0700 Committer: Mehul Parikh Committed: Fri Jul 27 14:33:58 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/b9db1732/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 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/b9db1732/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 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 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 getPoliciesForResourceSignature(String serviceName, String policySignature) { http://git-wip-us.apache.org/repos/asf/ranger/blob/b9db1732/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 getPolicies(SearchFilter filter) throws Exception; + Long getPolicyId(final Long serviceId, final String policyName); + PList getPaginatedPolicies(SearchFilter filter) throws Exception; List getPoliciesByResourceSignature(String serviceName, String policySignature, Boolean isPolicyEnabled) throws Exception; http://git-wip-us.apache.org/repos/asf/ranger/blob/b9db1732/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()); // 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 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 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 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 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/b9db1732/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 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 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/b9db1732/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 trxLogList = policyService.getTransactionLog(policy, xxExisting, RangerPolicyService.OPERATION_UPDATE_CONTEXT); + List 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 policies, HttpServletResponse response) throws Exception { if (LOG.isDebugEnabled()) { LOG.debug("==> ServiceDBStore.getPoliciesInExcel()"); http://git-wip-us.apache.org/repos/asf/ranger/blob/b9db1732/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 getTransactionLog(RangerPolicy vPolicy, int action) { - return getTransactionLog(vPolicy, null, action); + return getTransactionLog(vPolicy, null, null, action); } - public List getTransactionLog(RangerPolicy vObj, XXPolicy mObj, int action) { + public List 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