Skip to content

Commit 26e36a2

Browse files
rafaelweingartnerrafaelweingartner
committed
[CLOUDSTACK-10344] bug when moving ACL rules (change order with drag and drop)
1 parent bbc9204 commit 26e36a2

8 files changed

Lines changed: 254 additions & 19 deletions

File tree

‎api/src/main/java/com/cloud/network/vpc/NetworkACLService.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd;
2222
import org.apache.cloudstack.api.command.user.network.ListNetworkACLListsCmd;
2323
import org.apache.cloudstack.api.command.user.network.ListNetworkACLsCmd;
24+
import org.apache.cloudstack.api.command.user.network.MoveNetworkAclItemCmd;
2425
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLItemCmd;
2526
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLListCmd;
2627

@@ -90,4 +91,5 @@ public interface NetworkACLService {
9091

9192
NetworkACL updateNetworkACL(UpdateNetworkACLListCmd updateNetworkACLListCmd);
9293

93-
}
94+
NetworkACLItem moveNetworkAclRuleToNewPosition(MoveNetworkAclItemCmd moveNetworkAclItemCmd);
95+
}

‎api/src/main/java/org/apache/cloudstack/api/ApiConstants.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,8 @@ public class ApiConstants {
152152
public static final String ICMP_TYPE = "icmptype";
153153
public static final String ID = "id";
154154
public static final String IDS = "ids";
155+
public static final String ID_PREVIOUS_ACL_RULE = "previousaclruleid";
156+
public static final String ID_NEXT_ACL_RULE = "nextaclruleid";
155157
public static final String INTERNAL_DNS1 = "internaldns1";
156158
public static final String INTERNAL_DNS2 = "internaldns2";
157159
public static final String INTERVAL_TYPE = "intervaltype";

‎api/src/main/java/org/apache/cloudstack/api/command/user/network/MoveNetworkAclItemCmd.java‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package org.apache.cloudstack.api.command.user.network;
18+
19+
import org.apache.cloudstack.api.APICommand;
20+
import org.apache.cloudstack.api.ApiConstants;
21+
import org.apache.cloudstack.api.BaseAsyncCustomIdCmd;
22+
import org.apache.cloudstack.api.Parameter;
23+
import org.apache.cloudstack.api.response.NetworkACLItemResponse;
24+
import org.apache.cloudstack.context.CallContext;
25+
import org.apache.log4j.Logger;
26+
27+
import com.cloud.event.EventTypes;
28+
import com.cloud.network.vpc.NetworkACLItem;
29+
import com.cloud.user.Account;
30+
31+
@APICommand(name = "moveNetworkAclItem", description = "Move an ACL rule to a position bettwen two other ACL rules of the same ACL network list", responseObject = NetworkACLItemResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
32+
public class MoveNetworkAclItemCmd extends BaseAsyncCustomIdCmd {
33+
34+
public static final Logger s_logger = Logger.getLogger(MoveNetworkAclItemCmd.class.getName());
35+
private static final String s_name = "moveNetworkAclItemResponse";
36+
37+
@Parameter(name = ApiConstants.ID, type = CommandType.STRING, required = true, description = "The ID of the network ACL rule that is being moved to a new position.")
38+
private String uuidRuleBeingMoved;
39+
40+
@Parameter(name = ApiConstants.ID_PREVIOUS_ACL_RULE, type = CommandType.STRING, description = "The ID of the first rule that is right before 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 first position of the network ACL list.")
41+
private String previousAclRuleUuid;
42+
43+
@Parameter(name = ApiConstants.ID_NEXT_ACL_RULE, 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.")
44+
private String nextAclRuleUuid;
45+
46+
47+
@Override
48+
public void execute() {
49+
CallContext.current().setEventDetails(getEventDescription());
50+
51+
NetworkACLItem aclItem = _networkACLService.moveNetworkAclRuleToNewPosition(this);
52+
53+
NetworkACLItemResponse aclResponse = _responseGenerator.createNetworkACLItemResponse(aclItem);
54+
setResponseObject(aclResponse);
55+
aclResponse.setResponseName(getCommandName());
56+
}
57+
58+
public String getUuidRuleBeingMoved() {
59+
return uuidRuleBeingMoved;
60+
}
61+
62+
public String getPreviousAclRuleUuid() {
63+
return previousAclRuleUuid;
64+
}
65+
66+
public String getNextAclRuleUuid() {
67+
return nextAclRuleUuid;
68+
}
69+
70+
@Override
71+
public void checkUuid() {
72+
if (this.getCustomId() != null) {
73+
_uuidMgr.checkUuid(this.getCustomId(), NetworkACLItem.class);
74+
}
75+
}
76+
77+
@Override
78+
public String getEventType() {
79+
return EventTypes.EVENT_NETWORK_ACL_ITEM_UPDATE;
80+
}
81+
82+
@Override
83+
public String getEventDescription() {
84+
return String.format("Placing network ACL item [%s] between [%s] and [%s].", uuidRuleBeingMoved, previousAclRuleUuid, nextAclRuleUuid);
85+
}
86+
87+
@Override
88+
public String getCommandName() {
89+
return s_name;
90+
}
91+
92+
@Override
93+
public long getEntityOwnerId() {
94+
Account caller = CallContext.current().getCallingAccount();
95+
return caller.getAccountId();
96+
}
97+
}

‎engine/schema/src/main/java/com/cloud/network/vpc/NetworkACLItemDao.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,6 @@ public interface NetworkACLItemDao extends GenericDao<NetworkACLItemVO, Long> {
3636
NetworkACLItemVO findByAclAndNumber(long aclId, int number);
3737

3838
void loadCidrs(NetworkACLItemVO item);
39+
40+
void updateNumberFieldNetworkItem(long networkItemId, int newNumberValue);
3941
}

‎engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
// under the License.
1717
package com.cloud.network.vpc.dao;
1818

19+
import java.sql.PreparedStatement;
20+
import java.sql.SQLException;
1921
import java.util.List;
2022

2123
import javax.inject.Inject;
@@ -35,6 +37,7 @@
3537
import com.cloud.utils.db.SearchCriteria;
3638
import com.cloud.utils.db.SearchCriteria.Op;
3739
import com.cloud.utils.db.TransactionLegacy;
40+
import com.cloud.utils.exception.CloudRuntimeException;
3841

3942
@Component
4043
@DB()
@@ -172,4 +175,18 @@ public void loadCidrs(NetworkACLItemVO item) {
172175
List<String> cidrs = _networkACLItemCidrsDao.getCidrs(item.getId());
173176
item.setSourceCidrList(cidrs);
174177
}
175-
}
178+
179+
private String sqlUpdateNumberFieldNetworkItem = "UPDATE network_acl_item SET number = ? where id =?";
180+
@Override
181+
public void updateNumberFieldNetworkItem(long networkItemId, int newNumberValue) {
182+
try (TransactionLegacy txn = TransactionLegacy.currentTxn();
183+
PreparedStatement pstmt = txn.prepareAutoCloseStatement(sqlUpdateNumberFieldNetworkItem)) {
184+
pstmt.setLong(1, newNumberValue);
185+
pstmt.setLong(2, networkItemId);
186+
pstmt.executeUpdate();
187+
txn.commit();
188+
} catch (SQLException e) {
189+
throw new CloudRuntimeException(e);
190+
}
191+
}
192+
}

‎server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
package com.cloud.network.vpc;
1818

1919
import java.util.ArrayList;
20+
import java.util.Collections;
21+
import java.util.Comparator;
2022
import java.util.List;
2123
import java.util.Map;
2224

@@ -27,6 +29,7 @@
2729
import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd;
2830
import org.apache.cloudstack.api.command.user.network.ListNetworkACLListsCmd;
2931
import org.apache.cloudstack.api.command.user.network.ListNetworkACLsCmd;
32+
import org.apache.cloudstack.api.command.user.network.MoveNetworkAclItemCmd;
3033
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLItemCmd;
3134
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLListCmd;
3235
import org.apache.cloudstack.context.CallContext;
@@ -936,4 +939,123 @@ public NetworkACL updateNetworkACL(UpdateNetworkACLListCmd updateNetworkACLListC
936939
return _networkACLDao.findById(id);
937940
}
938941

942+
@Override
943+
public NetworkACLItem moveNetworkAclRuleToNewPosition(MoveNetworkAclItemCmd moveNetworkAclItemCmd) {
944+
String uuidRuleBeingMoved = moveNetworkAclItemCmd.getUuidRuleBeingMoved();
945+
String nextAclRuleUuid = moveNetworkAclItemCmd.getNextAclRuleUuid();
946+
String previousAclRuleUuid = moveNetworkAclItemCmd.getPreviousAclRuleUuid();
947+
948+
if (StringUtils.isBlank(previousAclRuleUuid) && StringUtils.isBlank(nextAclRuleUuid)) {
949+
throw new InvalidParameterValueException("Both previous and next ACL rule IDs cannot be blank.");
950+
}
951+
952+
NetworkACLItemVO ruleBeingMoved = _networkACLItemDao.findByUuid(uuidRuleBeingMoved);
953+
if (ruleBeingMoved == null) {
954+
throw new InvalidParameterValueException(String.format("Could not find a rule with ID[%s]", uuidRuleBeingMoved));
955+
}
956+
NetworkACLItemVO previousRule = retrieveAndValidateAclRule(previousAclRuleUuid);
957+
NetworkACLItemVO nextRule = retrieveAndValidateAclRule(nextAclRuleUuid);
958+
959+
validateMoveAclRulesData(ruleBeingMoved, previousRule, nextRule);
960+
961+
List<NetworkACLItemVO> allAclRules = getAllAclRulesSortedByNumber(ruleBeingMoved.getAclId());
962+
if(previousRule == null) {
963+
return moveRuleToTheTop(ruleBeingMoved, allAclRules);
964+
}
965+
if (nextRule == null) {
966+
return moveRuleToTheBottom(ruleBeingMoved, allAclRules);
967+
}
968+
969+
return moveRuleBetweenAclRules(ruleBeingMoved, allAclRules, previousRule, nextRule);
970+
}
971+
972+
private List<NetworkACLItemVO> getAllAclRulesSortedByNumber(long aclId) {
973+
List<NetworkACLItemVO> allAclRules = _networkACLItemDao.listByACL(aclId);
974+
Collections.sort(allAclRules, new Comparator<NetworkACLItemVO>() {
975+
@Override
976+
public int compare(NetworkACLItemVO o1, NetworkACLItemVO o2) {
977+
return o1.number - o2.number;
978+
}
979+
});
980+
return allAclRules;
981+
}
982+
983+
private NetworkACLItem moveRuleBetweenAclRules(NetworkACLItemVO ruleBeingMoved, List<NetworkACLItemVO> allAclRules, NetworkACLItemVO previousRule, NetworkACLItemVO nextRule) {
984+
if (previousRule.getNumber() + 1 != nextRule.getNumber()) {
985+
int newNumberFieldValue = previousRule.getNumber() + 1;
986+
for(NetworkACLItemVO networkACLItemVO: allAclRules) {
987+
if(networkACLItemVO.getNumber() == newNumberFieldValue) {
988+
throw new InvalidParameterValueException("There are some inconsistencies with the data you sent. The new position calculated already has a ACL rule on it.");
989+
}
990+
}
991+
ruleBeingMoved.setNumber(newNumberFieldValue);
992+
_networkACLItemDao.updateNumberFieldNetworkItem(ruleBeingMoved.getId(), newNumberFieldValue);
993+
return _networkACLItemDao.findById(ruleBeingMoved.getId());
994+
}
995+
int positionToStartProcessing = 0;
996+
for (int i = 0; i < allAclRules.size(); i++) {
997+
if (allAclRules.get(i).getId() == previousRule.getId()) {
998+
positionToStartProcessing = i + 1;
999+
break;
1000+
}
1001+
}
1002+
return updateAclRuleToNewPositionAndExecuteShiftIfNecessary(ruleBeingMoved, previousRule.getNumber() + 1, allAclRules, positionToStartProcessing);
1003+
}
1004+
1005+
private NetworkACLItem moveRuleToTheBottom(NetworkACLItemVO ruleBeingMoved, List<NetworkACLItemVO> allAclRules) {
1006+
NetworkACLItemVO lastAclRule = allAclRules.get(allAclRules.size() - 1);
1007+
1008+
int newNumberFieldValue = lastAclRule.getNumber() + 1;
1009+
ruleBeingMoved.setNumber(newNumberFieldValue);
1010+
1011+
_networkACLItemDao.updateNumberFieldNetworkItem(ruleBeingMoved.getId(), newNumberFieldValue);
1012+
return _networkACLItemDao.findById(ruleBeingMoved.getId());
1013+
}
1014+
1015+
private NetworkACLItem moveRuleToTheTop(NetworkACLItemVO ruleBeingMoved, List<NetworkACLItemVO> allAclRules) {
1016+
return updateAclRuleToNewPositionAndExecuteShiftIfNecessary(ruleBeingMoved, 1, allAclRules, 0);
1017+
}
1018+
1019+
private NetworkACLItem updateAclRuleToNewPositionAndExecuteShiftIfNecessary(NetworkACLItemVO ruleBeingMoved, int newNumberFieldValue, List<NetworkACLItemVO> allAclRules, int indexToStartProcessing) {
1020+
ruleBeingMoved.setNumber(newNumberFieldValue);
1021+
for (int i = indexToStartProcessing; i < allAclRules.size(); i++) {
1022+
NetworkACLItemVO networkACLItemVO = allAclRules.get(i);
1023+
if (networkACLItemVO.getId() == ruleBeingMoved.getId()) {
1024+
continue;
1025+
}
1026+
if (newNumberFieldValue != networkACLItemVO.getNumber()) {
1027+
break;
1028+
}
1029+
int newNumberFieldValueNextAclRule = newNumberFieldValue + 1;
1030+
updateAclRuleToNewPositionAndExecuteShiftIfNecessary(networkACLItemVO, newNumberFieldValueNextAclRule, allAclRules, i);
1031+
}
1032+
_networkACLItemDao.updateNumberFieldNetworkItem(ruleBeingMoved.getId(), newNumberFieldValue);
1033+
return _networkACLItemDao.findById(ruleBeingMoved.getId());
1034+
}
1035+
1036+
private NetworkACLItemVO retrieveAndValidateAclRule(String aclRuleUuid) {
1037+
if (StringUtils.isBlank(aclRuleUuid)) {
1038+
return null;
1039+
}
1040+
NetworkACLItemVO aclRule = _networkACLItemDao.findByUuid(aclRuleUuid);
1041+
if (aclRule == null) {
1042+
throw new InvalidParameterValueException(String.format("Could not find rule with ID [%s]", aclRuleUuid));
1043+
}
1044+
return aclRule;
1045+
}
1046+
1047+
private void validateMoveAclRulesData(NetworkACLItemVO ruleBeingMoved, NetworkACLItemVO previousRule, NetworkACLItemVO nextRule) {
1048+
if (nextRule == null && previousRule == null) {
1049+
throw new InvalidParameterValueException("Both previous and next ACL rule IDs cannot be invalid.");
1050+
}
1051+
long aclId = ruleBeingMoved.getAclId();
1052+
1053+
if((nextRule != null && nextRule.getAclId() != aclId) || (previousRule != null && previousRule.getAclId() != aclId)) {
1054+
throw new InvalidParameterValueException("Cannot use ACL rules from differenting ACLs. Rule being moved.");
1055+
}
1056+
NetworkACLVO acl = _networkACLDao.findById(aclId);
1057+
Vpc vpc = _entityMgr.findById(Vpc.class, acl.getVpcId());
1058+
Account caller = CallContext.current().getCallingAccount();
1059+
_accountMgr.checkAccess(caller, null, true, vpc);
1060+
}
9391061
}

‎server/src/main/java/com/cloud/server/ManagementServerImpl.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,7 @@
382382
import org.apache.cloudstack.api.command.user.network.ListNetworkACLsCmd;
383383
import org.apache.cloudstack.api.command.user.network.ListNetworkOfferingsCmd;
384384
import org.apache.cloudstack.api.command.user.network.ListNetworksCmd;
385+
import org.apache.cloudstack.api.command.user.network.MoveNetworkAclItemCmd;
385386
import org.apache.cloudstack.api.command.user.network.ReplaceNetworkACLListCmd;
386387
import org.apache.cloudstack.api.command.user.network.RestartNetworkCmd;
387388
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLItemCmd;
@@ -2958,6 +2959,7 @@ public List<Class<?>> getCommands() {
29582959
cmdList.add(ListNetworkACLListsCmd.class);
29592960
cmdList.add(ReplaceNetworkACLListCmd.class);
29602961
cmdList.add(UpdateNetworkACLItemCmd.class);
2962+
cmdList.add(MoveNetworkAclItemCmd.class);
29612963
cmdList.add(CleanVMReservationsCmd.class);
29622964
cmdList.add(UpgradeRouterTemplateCmd.class);
29632965
cmdList.add(UploadSslCertCmd.class);

‎ui/scripts/vpc.js‎

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -535,31 +535,22 @@
535535
moveDrag: {
536536
action: function(args) {
537537
var rule = args.context.multiRule[0];
538-
var number = 0;
539-
var prevItem = args.prevItem ? args.prevItem.number : null;
540-
var nextItem = args.nextItem ? args.nextItem.number : null;
541-
542-
if (!nextItem) { // Last item
543-
number = prevItem + 100;
544-
} else {
545-
if (nextItem - prevItem <= 10) {
546-
number = nextItem - parseInt(((nextItem - prevItem) / 2));
547-
} else {
548-
number = nextItem > 1 ? nextItem - 10 : 1;
549-
}
550-
}
551-
538+
539+
var previousRuleId = args.prevItem ? args.prevItem.id : undefined;
540+
var nextRuleId = args.nextItem ? args.nextItem.id : undefined;
541+
552542
$.ajax({
553-
url: createURL('updateNetworkACLItem'),
543+
url: createURL('moveNetworkAclItem'),
554544
data: {
555545
id: rule.id,
556-
number: number
546+
previousaclruleid: previousRuleId,
547+
nextaclruleid: nextRuleId
557548
},
558549
success: function(json) {
559550
var pollTimer = setInterval(function() {
560551
pollAsyncJobResult({
561552
_custom: {
562-
jobId: json.createnetworkaclresponse.jobid
553+
jobId: json.moveNetworkAclItemResponse.jobid
563554
},
564555
complete: function() {
565556
clearInterval(pollTimer);

0 commit comments

Comments
 (0)