This is an automated email from the ASF dual-hosted git repository.
rafael pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/master by this push:
new 70bd177 Fix concurrency problem when moving ACL rules with drag&drop
(#2703)
70bd177 is described below
commit 70bd177064edda2d89e696e953cfe2392333d869
Author: Rafael Weingärtner <[email protected]>
AuthorDate: Thu Jul 19 12:47:54 2018 -0300
Fix concurrency problem when moving ACL rules with drag&drop (#2703)
There was a concurrency problem with the “moveNetworkAclItem” API method.
If two users were changing the ACL rules order at the same time, this could
lead to inconsistent actions.
To solve the problem we added a “consistency check ” parameter, which is
used to hold the consistency hash. This hash is created using an MD5 hash
function on a String that is created with all ACL rules UUIDs concatenated in
their order, which is defined via the ‘number’ field.
We also lock the editing of the ACL while executing the upgrade. This
allows us to handle race conditions nicely, and present a good feedback for the
user.
---
.../org/apache/cloudstack/api/ApiConstants.java | 1 +
.../user/network/MoveNetworkAclItemCmd.java | 9 ++-
.../cloud/network/vpc/NetworkACLServiceImpl.java | 55 +++++++++++--
.../network/vpc/NetworkACLServiceImplTest.java | 89 +++++++++++++++++++++-
ui/scripts/vpc.js | 12 ++-
5 files changed, 156 insertions(+), 10 deletions(-)
diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
index 1a57313..275a3cc 100644
--- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
+++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
@@ -155,6 +155,7 @@ public class ApiConstants {
public static final String IDS = "ids";
public static final String PREVIOUS_ACL_RULE_ID = "previousaclruleid";
public static final String NEXT_ACL_RULE_ID = "nextaclruleid";
+ public static final String MOVE_ACL_CONSISTENCY_HASH =
"aclconsistencyhash";
public static final String INTERNAL_DNS1 = "internaldns1";
public static final String INTERNAL_DNS2 = "internaldns2";
public static final String INTERVAL_TYPE = "intervaltype";
diff --git
a/api/src/main/java/org/apache/cloudstack/api/command/user/network/MoveNetworkAclItemCmd.java
b/api/src/main/java/org/apache/cloudstack/api/command/user/network/MoveNetworkAclItemCmd.java
index aaa9c18..0343e50 100644
---
a/api/src/main/java/org/apache/cloudstack/api/command/user/network/MoveNetworkAclItemCmd.java
+++
b/api/src/main/java/org/apache/cloudstack/api/command/user/network/MoveNetworkAclItemCmd.java
@@ -43,6 +43,9 @@ public class MoveNetworkAclItemCmd extends
BaseAsyncCustomIdCmd {
@Parameter(name = ApiConstants.NEXT_ACL_RULE_ID, type =
CommandType.STRING, description = "The ID of the rule that is right after the
new position where the rule being moved is going to be placed. This value can
be 'NULL' if the rule is being moved to the last position of the network ACL
list.")
private String nextAclRuleUuid;
+ @Parameter(name = ApiConstants.MOVE_ACL_CONSISTENCY_HASH, type =
CommandType.STRING, description = "Md5 hash used to check the consistency of
the ACL rule list before applying the ACL rule move. This check is useful to
manage concurrency problems that may happen when multiple users are editing the
same ACL rule listing. The parameter is not required. Therefore, if the user
does not send it, he/she is assuming the risk of moving ACL rules without
checking the consistency of the access [...]
+ private String aclConsistencyHash;
+
@Override
public void execute() {
CallContext.current().setEventDetails(getEventDescription());
@@ -93,4 +96,8 @@ public class MoveNetworkAclItemCmd extends
BaseAsyncCustomIdCmd {
Account caller = CallContext.current().getCallingAccount();
return caller.getAccountId();
}
-}
+
+ public String getAclConsistencyHash() {
+ return aclConsistencyHash;
+ }
+}
\ No newline at end of file
diff --git
a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
index 674910d..8734ec6 100644
--- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
+++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
@@ -33,6 +33,7 @@ import
org.apache.cloudstack.api.command.user.network.MoveNetworkAclItemCmd;
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLItemCmd;
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLListCmd;
import org.apache.cloudstack.context.CallContext;
+import org.apache.commons.codec.digest.DigestUtils;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.log4j.Logger;
@@ -58,6 +59,7 @@ import com.cloud.tags.ResourceTagVO;
import com.cloud.tags.dao.ResourceTagDao;
import com.cloud.user.Account;
import com.cloud.user.AccountManager;
+import com.cloud.user.User;
import com.cloud.utils.Pair;
import com.cloud.utils.Ternary;
import com.cloud.utils.component.ManagerBase;
@@ -958,15 +960,56 @@ public class NetworkACLServiceImpl extends ManagerBase
implements NetworkACLServ
validateMoveAclRulesData(ruleBeingMoved, previousRule, nextRule);
- List<NetworkACLItemVO> allAclRules =
getAllAclRulesSortedByNumber(ruleBeingMoved.getAclId());
- if (previousRule == null) {
- return moveRuleToTheTop(ruleBeingMoved, allAclRules);
+ try {
+ NetworkACLVO lockedAcl =
_networkACLDao.acquireInLockTable(ruleBeingMoved.getAclId());
+ List<NetworkACLItemVO> allAclRules =
getAllAclRulesSortedByNumber(lockedAcl.getId());
+ validateAclConsistency(moveNetworkAclItemCmd, lockedAcl,
allAclRules);
+
+ if (previousRule == null) {
+ return moveRuleToTheTop(ruleBeingMoved, allAclRules);
+ }
+ if (nextRule == null) {
+ return moveRuleToTheBottom(ruleBeingMoved, allAclRules);
+ }
+ return moveRuleBetweenAclRules(ruleBeingMoved, allAclRules,
previousRule, nextRule);
+ } finally {
+ _networkACLDao.releaseFromLockTable(ruleBeingMoved.getAclId());
}
- if (nextRule == null) {
- return moveRuleToTheBottom(ruleBeingMoved, allAclRules);
+ }
+
+ /**
+ * Validates the consistency of the ACL; the validation process is the
following.
+ * <ul>
+ * <li> If the ACL does not have rules yet, we do not have any validation
to perform;
+ * <li> we will check first if the user provided a consistency hash; if
not, we will log a warning message informing administrators that the user is
performing the call is assuming the risks of applying ACL replacement without a
consistency check;
+ * <li> if the ACL consistency hash is entered by the user, we check if
it is the same as we currently have in the database. If it is different we
throw an exception.
+ * </ul>
+ *
+ * If the consistency hash sent by the user is the same as the one we get
with the database data we should be safe to proceed.
+ */
+ protected void validateAclConsistency(MoveNetworkAclItemCmd
moveNetworkAclItemCmd, NetworkACLVO lockedAcl, List<NetworkACLItemVO>
allAclRules) {
+ if (CollectionUtils.isEmpty(allAclRules)) {
+ s_logger.debug(String.format("No ACL rules for [id=%s, name=%s].
Therefore, there is no need for consistency validation.", lockedAcl.getUuid(),
lockedAcl.getName()));
+ return;
}
+ String aclConsistencyHash =
moveNetworkAclItemCmd.getAclConsistencyHash();
+ if (StringUtils.isBlank(aclConsistencyHash)) {
+ User callingUser = CallContext.current().getCallingUser();
+ Account callingAccount = CallContext.current().getCallingAccount();
- return moveRuleBetweenAclRules(ruleBeingMoved, allAclRules,
previousRule, nextRule);
+ s_logger.warn(String.format(
+ "User [id=%s, name=%s] from Account [id=%s, name=%s] has
not entered an ACL consistency hash to execute the replacement of an ACL rule.
Therefore, she/he is assuming all of the risks of procedding without this
validation.",
+ callingUser.getUuid(), callingUser.getUsername(),
callingAccount.getUuid(), callingAccount.getAccountName()));
+ return;
+ }
+ String aclRulesUuids = StringUtils.EMPTY;
+ for (NetworkACLItemVO rule : allAclRules) {
+ aclRulesUuids += rule.getUuid();
+ }
+ String md5UuidsSortedByNumber = DigestUtils.md5Hex(aclRulesUuids);
+ if (!md5UuidsSortedByNumber.equals(aclConsistencyHash)) {
+ throw new InvalidParameterValueException("It seems that the access
control list in the database is not in the state that you used to apply the
changed. Could you try it again?");
+ }
}
/**
diff --git
a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java
b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java
index 5980574..9815526 100644
--- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java
+++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java
@@ -56,6 +56,7 @@ import com.cloud.network.vpc.NetworkACLItem.TrafficType;
import com.cloud.network.vpc.dao.NetworkACLDao;
import com.cloud.user.Account;
import com.cloud.user.AccountManager;
+import com.cloud.user.User;
import com.cloud.utils.db.EntityManager;
import com.cloud.utils.exception.CloudRuntimeException;
@@ -113,10 +114,15 @@ public class NetworkACLServiceImplTest {
private NetworkACLItemVO nextAclRuleMock;
private String nextAclRuleUuid = "uuidNextAclRule";
+ @Mock
+ private CallContext callContextMock;
+
@Before
public void befoteTest() {
PowerMockito.mockStatic(CallContext.class);
-
PowerMockito.when(CallContext.current()).thenReturn(Mockito.mock(CallContext.class));
+ PowerMockito.when(CallContext.current()).thenReturn(callContextMock);
+
Mockito.doReturn(Mockito.mock(User.class)).when(callContextMock).getCallingUser();
+
Mockito.doReturn(Mockito.mock(Account.class)).when(callContextMock).getCallingAccount();
Mockito.when(networkAclDaoMock.findById(networkAclListId)).thenReturn(networkACLVOMock);
Mockito.when(createNetworkAclCmdMock.getNetworkId()).thenReturn(1L);
@@ -936,6 +942,8 @@ public class NetworkACLServiceImplTest {
Mockito.verify(networkAclServiceImpl,
Mockito.times(0)).moveRuleToTheBottom(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class));
Mockito.verify(networkAclServiceImpl,
Mockito.times(0)).moveRuleBetweenAclRules(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class), Mockito.eq(previousAclRuleMock),
Mockito.eq(nextAclRuleMock));
+ Mockito.verify(networkAclServiceImpl,
Mockito.times(1)).validateAclConsistency(Mockito.any(MoveNetworkAclItemCmd.class),
Mockito.any(NetworkACLVO.class),
+ Mockito.anyListOf(NetworkACLItemVO.class));
}
@Test
@@ -957,6 +965,8 @@ public class NetworkACLServiceImplTest {
Mockito.verify(networkAclServiceImpl,
Mockito.times(1)).moveRuleToTheBottom(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class));
Mockito.verify(networkAclServiceImpl,
Mockito.times(0)).moveRuleBetweenAclRules(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class), Mockito.eq(previousAclRuleMock),
Mockito.eq(nextAclRuleMock));
+ Mockito.verify(networkAclServiceImpl,
Mockito.times(1)).validateAclConsistency(Mockito.any(MoveNetworkAclItemCmd.class),
Mockito.any(NetworkACLVO.class),
+ Mockito.anyListOf(NetworkACLItemVO.class));
}
@Test
@@ -978,11 +988,17 @@ public class NetworkACLServiceImplTest {
Mockito.verify(networkAclServiceImpl,
Mockito.times(0)).moveRuleToTheBottom(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class));
Mockito.verify(networkAclServiceImpl,
Mockito.times(1)).moveRuleBetweenAclRules(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class), Mockito.eq(previousAclRuleMock),
Mockito.eq(nextAclRuleMock));
+ Mockito.verify(networkAclServiceImpl,
Mockito.times(1)).validateAclConsistency(Mockito.any(MoveNetworkAclItemCmd.class),
Mockito.any(NetworkACLVO.class),
+ Mockito.anyListOf(NetworkACLItemVO.class));
}
private void configureMoveMethodsToDoNothing() {
- Mockito.doReturn(new
ArrayList<>()).when(networkAclServiceImpl).getAllAclRulesSortedByNumber(networkAclMockId);
+
Mockito.doReturn(networkACLVOMock).when(networkAclDaoMock).acquireInLockTable(Mockito.anyLong());
+
Mockito.doReturn(true).when(networkAclDaoMock).releaseFromLockTable(Mockito.anyLong());
+
+
Mockito.doNothing().when(networkAclServiceImpl).validateAclConsistency(Mockito.any(MoveNetworkAclItemCmd.class),
Mockito.any(NetworkACLVO.class), Mockito.anyListOf(NetworkACLItemVO.class));
+ Mockito.doReturn(new
ArrayList<>()).when(networkAclServiceImpl).getAllAclRulesSortedByNumber(networkAclMockId);
Mockito.doReturn(aclRuleBeingMovedMock).when(networkAclServiceImpl).moveRuleToTheTop(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class));
Mockito.doReturn(aclRuleBeingMovedMock).when(networkAclServiceImpl).moveRuleToTheBottom(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class));
Mockito.doReturn(aclRuleBeingMovedMock).when(networkAclServiceImpl).moveRuleBetweenAclRules(Mockito.eq(aclRuleBeingMovedMock),
Mockito.anyListOf(NetworkACLItemVO.class),
@@ -1281,4 +1297,73 @@ public class NetworkACLServiceImplTest {
Assert.assertEquals(14, networkACLItemVO13.getNumber());
Assert.assertEquals(15, networkACLItemVO14.getNumber());
}
+
+ @Test
+ public void validateAclConsistencyTestRuleListEmpty() {
+
networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock,
networkACLVOMock, new ArrayList<>());
+
+ Mockito.verify(moveNetworkAclItemCmdMock,
Mockito.times(0)).getAclConsistencyHash();
+ }
+
+ @Test
+ public void validateAclConsistencyTestRuleListNull() {
+
networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock,
networkACLVOMock, null);
+
+ Mockito.verify(moveNetworkAclItemCmdMock,
Mockito.times(0)).getAclConsistencyHash();
+ }
+
+ @Test
+ public void validateAclConsistencyTestAclConsistencyHashIsNull() {
+
Mockito.doReturn(null).when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+ validateAclConsistencyTestAclConsistencyHashBlank();
+ }
+
+ @Test
+ public void validateAclConsistencyTestAclConsistencyHashIsEmpty() {
+
Mockito.doReturn("").when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+ validateAclConsistencyTestAclConsistencyHashBlank();
+ }
+
+ @Test
+ public void validateAclConsistencyTestAclConsistencyHashIsBlank() {
+ Mockito.doReturn("
").when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+ validateAclConsistencyTestAclConsistencyHashBlank();
+ }
+
+ private void validateAclConsistencyTestAclConsistencyHashBlank() {
+ ArrayList<NetworkACLItemVO> allAclRules = new ArrayList<>();
+ allAclRules.add(networkAclItemVoMock);
+
+
networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock,
networkACLVOMock, allAclRules);
+
+ Mockito.verify(moveNetworkAclItemCmdMock,
Mockito.times(1)).getAclConsistencyHash();
+ Mockito.verify(callContextMock, Mockito.times(1)).getCallingAccount();
+ Mockito.verify(callContextMock, Mockito.times(1)).getCallingUser();
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void
validateAclConsistencyTestAclConsistencyHashIsNotEqualsToDatabaseHash() {
+
Mockito.doReturn("differentHash").when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+ ArrayList<NetworkACLItemVO> allAclRules = new ArrayList<>();
+ allAclRules.add(networkAclItemVoMock);
+
+
networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock,
networkACLVOMock, allAclRules);
+ }
+
+ @Test
+ public void validateAclConsistencyTest() {
+
Mockito.doReturn("eac527fe45c77232ef06d9c7eb8abd94").when(moveNetworkAclItemCmdMock).getAclConsistencyHash();
+
+ ArrayList<NetworkACLItemVO> allAclRules = new ArrayList<>();
+ allAclRules.add(networkAclItemVoMock);
+
+ Mockito.doReturn("someUuid").when(networkAclItemVoMock).getUuid();
+
networkAclServiceImpl.validateAclConsistency(moveNetworkAclItemCmdMock,
networkACLVOMock, allAclRules);
+
+ Mockito.verify(moveNetworkAclItemCmdMock,
Mockito.times(1)).getAclConsistencyHash();
+ }
}
\ No newline at end of file
diff --git a/ui/scripts/vpc.js b/ui/scripts/vpc.js
index 8a0bb96..04125ff 100644
--- a/ui/scripts/vpc.js
+++ b/ui/scripts/vpc.js
@@ -15,6 +15,10 @@
// specific language governing permissions and limitations
// under the License.
(function($, cloudStack) {
+ //The drag and drop function to order ACL rules does not have access to
the whole ACL.
+ //Therefore, we store the "state-hash" of the list being displayed for use
in the drag and drop function.
+ var accessControlListConsistentyHashForDragAndDropFunction = "";
+
var isNumeric = function (n) {
return !isNaN(parseFloat(n));
};
@@ -544,7 +548,8 @@
data: {
id: rule.id,
previousaclruleid: previousRuleId,
- nextaclruleid: nextRuleId
+ nextaclruleid: nextRuleId,
+ aclconsistencyhash:
accessControlListConsistentyHashForDragAndDropFunction
},
success: function(json) {
var pollTimer = setInterval(function() {
@@ -1415,6 +1420,11 @@
return acl;
});
+ var allUuids = '';
+
items.forEach(function(aclRule){
+
allUuids += aclRule.id;
+ });
+
accessControlListConsistentyHashForDragAndDropFunction = $.md5(allUuids);
}
args.response.success({
