rafaelweingartner closed pull request #2703: Fix concurrency problem when
moving ACL rules with drag&drop
URL: https://github.com/apache/cloudstack/pull/2703
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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 504b2149837..5b778d077b6 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 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 aaa9c185526..0343e5012e4 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 @@
@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 control list before executing the move.
We use MD5 hash function on a String that is composed of all UUIDs of the ACL
rules in concatenated in their respective order (order defined via 'number'
field).")
+ private String aclConsistencyHash;
+
@Override
public void execute() {
CallContext.current().setEventDetails(getEventDescription());
@@ -93,4 +96,8 @@ public long getEntityOwnerId() {
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 674910d8d1e..8734ec61086 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.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.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 NetworkACLItem
moveNetworkAclRuleToNewPosition(MoveNetworkAclItemCmd move
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 598057402f2..9815526b6aa 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.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 @@
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 void
moveNetworkAclRuleToNewPositionTestMoveRuleToTop() {
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 void
moveNetworkAclRuleToNewPositionTestMoveRuleToBottom() {
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 void
moveNetworkAclRuleToNewPositionTestMoveBetweenAclRules() {
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 void
updateAclRuleToNewPositionAndExecuteShiftIfNecessaryTest() {
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 8a0bb968a59..04125ff6eda 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({
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
