cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CLOUDSTACK-10332) Users are not able to change/edit the protocol of an ACL rule
Date Thu, 29 Mar 2018 08:07:00 GMT

    [ https://issues.apache.org/jira/browse/CLOUDSTACK-10332?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16418576#comment-16418576
] 

ASF GitHub Bot commented on CLOUDSTACK-10332:
---------------------------------------------

DaanHoogland closed pull request #2496: [CLOUDSTACK-10332] Users are not able to change/edit
the protocol of an ACL rule 
URL: https://github.com/apache/cloudstack/pull/2496
 
 
   

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 ac9406fee99..7825bb4fc93 100644
--- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
+++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
@@ -309,7 +309,7 @@
     public static final String USERNAME = "username";
     public static final String USER_SECURITY_GROUP_LIST = "usersecuritygrouplist";
     public static final String USE_VIRTUAL_NETWORK = "usevirtualnetwork";
-    public static final String Update_IN_SEQUENCE ="updateinsequence";
+    public static final String Update_IN_SEQUENCE = "updateinsequence";
     public static final String VALUE = "value";
     public static final String VIRTUAL_MACHINE_ID = "virtualmachineid";
     public static final String VIRTUAL_MACHINE_IDS = "virtualmachineids";
@@ -453,6 +453,7 @@
     public static final String NSP_ID = "nspid";
     public static final String ACL_TYPE = "acltype";
     public static final String ACL_REASON = "reason";
+    public static final String ACL_RULE_PARTIAL_UPGRADE = "partialupgrade";
     public static final String SUBDOMAIN_ACCESS = "subdomainaccess";
     public static final String LOAD_BALANCER_DEVICE_ID = "lbdeviceid";
     public static final String LOAD_BALANCER_DEVICE_NAME = "lbdevicename";
@@ -707,14 +708,12 @@
             + " and just a plain ascii/utf8 representation of a hexadecimal string. If it
is required to\n"
             + " use another algorithm the hexadecimal string is to be prefixed with a string
of the form,\n"
             + " \"{<algorithm>}\", not including the double quotes. In this <algorithm>
is the exact string\n"
-            + " representing the java supported algorithm, i.e. MD5 or SHA-256. Note that
java does not\n"
-            + " contain an algorithm called SHA256 or one called sha-256, only SHA-256.";
+            + " representing the java supported algorithm, i.e. MD5 or SHA-256. Note that
java does not\n" + " contain an algorithm called SHA256 or one called sha-256, only SHA-256.";
 
     public static final String HAS_ANNOTATION = "hasannotation";
     public static final String LAST_ANNOTATED = "lastannotated";
     public static final String LDAP_DOMAIN = "ldapdomain";
 
-
     public enum HostDetails {
         all, capacity, events, stats, min;
     }
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java
b/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java
index 1530239fd12..1215f5710a0 100644
--- a/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java
+++ b/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java
@@ -79,6 +79,9 @@
     @Parameter(name = ApiConstants.ACL_REASON, type = CommandType.STRING, description = "A
description indicating why the ACL rule is required.")
     private String reason;
 
+    @Parameter(name = ApiConstants.ACL_RULE_PARTIAL_UPGRADE, type = CommandType.BOOLEAN,
required = false, description = "Indicates if the ACL rule is to be updated partially (merging
the parameters sent with current configuration) or completely (disconsidering all of the current
configurations). The default value is 'true'.")
+    private boolean partialUpgrade = true;
+
     // ///////////////////////////////////////////////////
     // ///////////////// Accessors ///////////////////////
     // ///////////////////////////////////////////////////
@@ -188,4 +191,7 @@ public void checkUuid() {
         }
     }
 
+    public boolean isPartialUpgrade() {
+        return partialUpgrade;
+    }
 }
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 bfa842807f9..5369ffa4f8a 100644
--- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
+++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
@@ -829,6 +829,8 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork
      *  We transfer the update information form {@link UpdateNetworkACLItemCmd} to the {@link
NetworkACLItemVO} POJO passed as parameter.
      *  There is one validation performed here, which is regarding the number of the ACL.
We will check if there is already an ACL rule with that number, and if this is the case an
{@link InvalidParameterValueException} is thrown.
      *  All of the parameters in {@link UpdateNetworkACLItemCmd} that are not null will be
set to their corresponding fields in {@link NetworkACLItemVO}.
+     *  If the parameter {@link UpdateNetworkACLItemCmd#isPartialUpgrade()} returns false,
we will use null parameters, which will allow us to completely update the ACL rule.
+     *  However, the number and custom Uuid will never be set to null. Therefore, if it is
not a partial upgrade, these values will remain the same.
      *
      *  We use {@link #validateAndCreateNetworkAclRuleAction(String)} when converting an
action as {@link String} to its Enum corresponding value.
      */
@@ -841,38 +843,39 @@ protected void transferDataToNetworkAclRulePojo(UpdateNetworkACLItemCmd
updateNe
             }
             networkACLItemVo.setNumber(number);
         }
+        boolean isPartialUpgrade = updateNetworkACLItemCmd.isPartialUpgrade();
 
         Integer sourcePortStart = updateNetworkACLItemCmd.getSourcePortStart();
-        if (sourcePortStart != null) {
+        if (!isPartialUpgrade || sourcePortStart != null) {
             networkACLItemVo.setSourcePortStart(sourcePortStart);
         }
         Integer sourcePortEnd = updateNetworkACLItemCmd.getSourcePortEnd();
-        if (sourcePortEnd != null) {
+        if (!isPartialUpgrade || sourcePortEnd != null) {
             networkACLItemVo.setSourcePortEnd(sourcePortEnd);
         }
         List<String> sourceCidrList = updateNetworkACLItemCmd.getSourceCidrList();
-        if (CollectionUtils.isNotEmpty(sourceCidrList)) {
+        if (!isPartialUpgrade || CollectionUtils.isNotEmpty(sourceCidrList)) {
             networkACLItemVo.setSourceCidrList(sourceCidrList);
         }
         String protocol = updateNetworkACLItemCmd.getProtocol();
-        if (StringUtils.isNotBlank(protocol)) {
+        if (!isPartialUpgrade || StringUtils.isNotBlank(protocol)) {
             networkACLItemVo.setProtocol(protocol);
         }
         Integer icmpCode = updateNetworkACLItemCmd.getIcmpCode();
-        if (icmpCode != null) {
+        if (!isPartialUpgrade || icmpCode != null) {
             networkACLItemVo.setIcmpCode(icmpCode);
         }
         Integer icmpType = updateNetworkACLItemCmd.getIcmpType();
-        if (icmpType != null) {
+        if (!isPartialUpgrade || icmpType != null) {
             networkACLItemVo.setIcmpType(icmpType);
         }
         String action = updateNetworkACLItemCmd.getAction();
-        if (StringUtils.isNotBlank(action)) {
+        if (!isPartialUpgrade || StringUtils.isNotBlank(action)) {
             Action aclRuleAction = validateAndCreateNetworkAclRuleAction(action);
             networkACLItemVo.setAction(aclRuleAction);
         }
         TrafficType trafficType = updateNetworkACLItemCmd.getTrafficType();
-        if (trafficType != null) {
+        if (!isPartialUpgrade || trafficType != null) {
             networkACLItemVo.setTrafficType(trafficType);
         }
         String customId = updateNetworkACLItemCmd.getCustomId();
@@ -880,11 +883,11 @@ protected void transferDataToNetworkAclRulePojo(UpdateNetworkACLItemCmd
updateNe
             networkACLItemVo.setUuid(customId);
         }
         boolean display = updateNetworkACLItemCmd.isDisplay();
-        if (display != networkACLItemVo.isDisplay()) {
+        if (!isPartialUpgrade || display != networkACLItemVo.isDisplay()) {
             networkACLItemVo.setDisplay(display);
         }
         String reason = updateNetworkACLItemCmd.getReason();
-        if (StringUtils.isNotBlank(reason)) {
+        if (!isPartialUpgrade || StringUtils.isNotBlank(reason)) {
             networkACLItemVo.setReason(reason);
         }
     }
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 ee7a474bd19..ed8602b181c 100644
--- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java
+++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java
@@ -700,7 +700,8 @@ public void transferDataToNetworkAclRulePojoTestNumberOfAcltoBeUpdatedAlreadyInU
     }
 
     @Test
-    public void transferDataToNetworkAclRulePojoTestAllValuesNull() {
+    public void transferDataToNetworkAclRulePojoTestPartialUpgradeAllValuesNull() {
+        Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(true);
         Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null);
         Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(null);
         Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
@@ -733,6 +734,42 @@ public void transferDataToNetworkAclRulePojoTestAllValuesNull() {
         Mockito.verify(networkAclServiceImpl, Mockito.times(0)).validateAndCreateNetworkAclRuleAction(Mockito.anyString());
     }
 
+    @Test
+    public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeAllValuesNull() {
+        Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false);
+
+        Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getAction()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getCustomId()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getReason()).thenReturn(null);
+
+        Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(false);
+        Mockito.when(networkAclItemVoMock.isDisplay()).thenReturn(false);
+
+        networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock,
networkAclItemVoMock, networkAclMock);
+
+        Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setNumber(Mockito.anyInt());
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortStart(Mockito.anyInt());
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortEnd(Mockito.anyInt());
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourceCidrList(Mockito.anyListOf(String.class));
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setProtocol(Mockito.anyString());
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpCode(Mockito.anyInt());
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpType(Mockito.anyInt());
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setAction(Mockito.any(Action.class));
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setTrafficType(Mockito.any(TrafficType.class));
+        Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setUuid(Mockito.anyString());
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setReason(Mockito.anyString());
+        Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setDisplay(Mockito.anyBoolean());
+        Mockito.verify(networkAclServiceImpl, Mockito.times(1)).validateAndCreateNetworkAclRuleAction(Mockito.anyString());
+    }
+
     @Test
     public void transferDataToNetworkAclRulePojoTestAllValuesWithUpdateData() {
         Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(1);
diff --git a/ui/scripts/vpc.js b/ui/scripts/vpc.js
index dcf8e38e09b..7eb589eacc1 100644
--- a/ui/scripts/vpc.js
+++ b/ui/scripts/vpc.js
@@ -671,7 +671,7 @@
 
                         delete args.data.protocolnumber;
                     }
-
+                    data.partialupgrade = false;
                     $.ajax({
                         url: createURL('updateNetworkACLItem'),
                         data: data,


 

----------------------------------------------------------------
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:
users@infra.apache.org


> Users are not able to change/edit the protocol of an ACL rule 
> --------------------------------------------------------------
>
>                 Key: CLOUDSTACK-10332
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-10332
>             Project: CloudStack
>          Issue Type: New Feature
>      Security Level: Public(Anyone can view this level - this is the default.) 
>            Reporter: Rafael Weingärtner
>            Assignee: Rafael Weingärtner
>            Priority: Major
>             Fix For: 4.12
>
>
> Users should be able to edit an ACL rule completely. Therefore, they must be able to
change the protocol type and others configs of an ACL rules.
> Right now users are not able to execute the following. 
> * Create an ACL for ICMP
> * Click on edit and change the protocol to TCP
> * An error will happen when saving the rule.
> Users should be able to execute the protocol changes without problem.
> In addition, it is not just the protocol that users are not able to change. For instance,
after defining ports, or reason/description for the rule, users are not able to set those
values back to null. The same happens for ICMP code and type.
> We will introduce a new parameter called "partialUpdate", which will have its default
value as true to maintain backward compatibility. When this parameter is set to false, we
will consider only the parameters sent, and not the parameters we already have in the database
to change and validate the ACL rule data. This allows us to update parameters already set
back to null, and to completely change an ACL rule.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message