server: Add new command to update security group name (#3739)

By default, once we create a security group we cant change its name.
In this feature, we introduce a new API command "updateSecurityGroup"
which allows us to rename the security group name. Although we can't
change the name of the "default" security group.

.travis.yml

@@ -103,6 +103,7 @@ env:
              smoke/test_ssvm
              smoke/test_staticroles
              smoke/test_templates
+             smoke/test_update_security_group
              smoke/test_usage
              smoke/test_usage_events"
 

api/src/main/java/com/cloud/event/EventTypes.java

@@ -327,6 +327,7 @@ public class EventTypes {
     public static final String EVENT_SECURITY_GROUP_DELETE = "SG.DELETE";
     public static final String EVENT_SECURITY_GROUP_ASSIGN = "SG.ASSIGN";
     public static final String EVENT_SECURITY_GROUP_REMOVE = "SG.REMOVE";
+    public static final String EVENT_SECURITY_GROUP_UPDATE = "SG.UPDATE";
 
     // Host
     public static final String EVENT_HOST_RECONNECT = "HOST.RECONNECT";

api/src/main/java/com/cloud/network/security/SecurityGroupService.java

@@ -18,16 +18,17 @@ package com.cloud.network.security;
 
 import java.util.List;
 
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.exception.ResourceInUseException;
+
 import org.apache.cloudstack.api.command.user.securitygroup.AuthorizeSecurityGroupEgressCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.AuthorizeSecurityGroupIngressCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.CreateSecurityGroupCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.DeleteSecurityGroupCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.RevokeSecurityGroupEgressCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.RevokeSecurityGroupIngressCmd;
-
+import org.apache.cloudstack.api.command.user.securitygroup.UpdateSecurityGroupCmd;
-import com.cloud.exception.InvalidParameterValueException;
-import com.cloud.exception.PermissionDeniedException;
-import com.cloud.exception.ResourceInUseException;
 
 public interface SecurityGroupService {
     /**
@@ -43,6 +44,8 @@ public interface SecurityGroupService {
 
     boolean deleteSecurityGroup(DeleteSecurityGroupCmd cmd) throws ResourceInUseException;
 
+    SecurityGroup updateSecurityGroup(UpdateSecurityGroupCmd cmd);
+
     public List<? extends SecurityRule> authorizeSecurityGroupIngress(AuthorizeSecurityGroupIngressCmd cmd);
 
     public List<? extends SecurityRule> authorizeSecurityGroupEgress(AuthorizeSecurityGroupEgressCmd cmd);

api/src/main/java/org/apache/cloudstack/api/command/user/securitygroup/UpdateSecurityGroupCmd.java

@@ -0,0 +1,105 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.user.securitygroup;
+
+import org.apache.log4j.Logger;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.acl.SecurityChecker.AccessType;
+import org.apache.cloudstack.api.ACL;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCustomIdCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.SecurityGroupResponse;
+
+import com.cloud.network.security.SecurityGroup;
+import com.cloud.user.Account;
+
+@APICommand(name = UpdateSecurityGroupCmd.APINAME, description = "Updates a security group", responseObject = SecurityGroupResponse.class, entityType = {SecurityGroup.class},
+        requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
+        since = "4.14.0.0",
+        authorized = {RoleType.Admin})
+public class UpdateSecurityGroupCmd extends BaseCustomIdCmd {
+    public static final String APINAME = "updateSecurityGroup";
+    public static final Logger s_logger = Logger.getLogger(UpdateSecurityGroupCmd.class.getName());
+    private static final String s_name = "updatesecuritygroupresponse";
+
+    /////////////////////////////////////////////////////
+    //////////////// API parameters /////////////////////
+    /////////////////////////////////////////////////////
+
+    @ACL(accessType = AccessType.OperateEntry)
+    @Parameter(name = ApiConstants.ID, type = CommandType.UUID, required=true, description="The ID of the security group.", entityType=SecurityGroupResponse.class)
+    private Long id;
+
+    @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "The new name of the security group.")
+    private String name;
+
+    /////////////////////////////////////////////////////
+    /////////////////// Accessors ///////////////////////
+    /////////////////////////////////////////////////////
+
+    public Long getId() {
+        return id;
+    }
+
+    public String getName() {
+        return name;
+    }
+
+    /////////////////////////////////////////////////////
+    /////////////// API Implementation///////////////////
+    /////////////////////////////////////////////////////
+
+    @Override
+    public String getCommandName() {
+        return s_name;
+    }
+
+    @Override
+    public long getEntityOwnerId() {
+        SecurityGroup securityGroup = _entityMgr.findById(SecurityGroup.class, getId());
+        if (securityGroup != null) {
+            return securityGroup.getAccountId();
+        }
+
+        return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked
+    }
+
+    @Override
+    public void execute() {
+        SecurityGroup result = _securityGroupService.updateSecurityGroup(this);
+        if (result != null) {
+            SecurityGroupResponse response = _responseGenerator.createSecurityGroupResponse(result);
+            response.setResponseName(getCommandName());
+            setResponseObject(response);
+        } else {
+            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update security group");
+        }
+    }
+
+    @Override
+    public void checkUuid() {
+        if (getCustomId() != null) {
+            _uuidMgr.checkUuid(getCustomId(), SecurityGroup.class);
+        }
+    }
+
+}

engine/schema/src/main/java/com/cloud/network/security/SecurityGroupVO.java

@@ -70,6 +70,10 @@ public class SecurityGroupVO implements SecurityGroup {
         return name;
     }
 
+    public void setName(String name) {
+        this.name = name;
+    }
+
     @Override
     public String getDescription() {
         return description;

server/src/main/java/com/cloud/network/security/SecurityGroupManagerImpl.java

@@ -44,6 +44,7 @@ import org.apache.cloudstack.api.command.user.securitygroup.CreateSecurityGroupC
 import org.apache.cloudstack.api.command.user.securitygroup.DeleteSecurityGroupCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.RevokeSecurityGroupEgressCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.RevokeSecurityGroupIngressCmd;
+import org.apache.cloudstack.api.command.user.securitygroup.UpdateSecurityGroupCmd;
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
@@ -879,6 +880,10 @@ public class SecurityGroupManagerImpl extends ManagerBase implements SecurityGro
         Account caller = CallContext.current().getCallingAccount();
         Account owner = _accountMgr.finalizeOwner(caller, cmd.getAccountName(), cmd.getDomainId(), cmd.getProjectId());
 
+        if (StringUtils.isBlank(name)) {
+            throw new InvalidParameterValueException("Security group name cannot be empty");
+        }
+
         if (_securityGroupDao.isNameInUse(owner.getId(), owner.getDomainId(), cmd.getSecurityGroupName())) {
             throw new InvalidParameterValueException("Unable to create security group, a group with name " + name + " already exists.");
         }
@@ -1117,6 +1122,60 @@ public class SecurityGroupManagerImpl extends ManagerBase implements SecurityGro
         s_logger.debug("Security group mappings are removed successfully for vm id=" + userVmId);
     }
 
+    @DB
+    @Override
+    @ActionEvent(eventType = EventTypes.EVENT_SECURITY_GROUP_UPDATE, eventDescription = "updating security group")
+    public SecurityGroup updateSecurityGroup(UpdateSecurityGroupCmd cmd) {
+        final Long groupId = cmd.getId();
+        final String newName = cmd.getName();
+        Account caller = CallContext.current().getCallingAccount();
+
+        SecurityGroupVO group = _securityGroupDao.findById(groupId);
+        if (group == null) {
+            throw new InvalidParameterValueException("Unable to find security group: " + groupId + "; failed to update security group.");
+        }
+
+        if (newName == null) {
+            s_logger.debug("security group name is not changed. id=" + groupId);
+            return group;
+        }
+
+        if (StringUtils.isBlank(newName)) {
+            throw new InvalidParameterValueException("Security group name cannot be empty");
+        }
+
+        // check permissions
+        _accountMgr.checkAccess(caller, null, true, group);
+
+        return Transaction.execute(new TransactionCallback<SecurityGroupVO>() {
+            @Override
+            public SecurityGroupVO doInTransaction(TransactionStatus status) {
+                SecurityGroupVO group = _securityGroupDao.lockRow(groupId, true);
+                if (group == null) {
+                    throw new InvalidParameterValueException("Unable to find security group by id " + groupId);
+                }
+
+                if (newName.equals(group.getName())) {
+                    s_logger.debug("security group name is not changed. id=" + groupId);
+                    return group;
+                } else if (newName.equalsIgnoreCase(SecurityGroupManager.DEFAULT_GROUP_NAME)) {
+                    throw new InvalidParameterValueException("The security group name " + SecurityGroupManager.DEFAULT_GROUP_NAME + " is reserved");
+                }
+
+                if (group.getName().equalsIgnoreCase(SecurityGroupManager.DEFAULT_GROUP_NAME)) {
+                    throw new InvalidParameterValueException("The default security group cannot be renamed");
+                }
+
+                group.setName(newName);
+                _securityGroupDao.update(groupId, group);
+
+                s_logger.debug("Updated security group id=" + groupId);
+
+                return group;
+            }
+        });
+    }
+
     @DB
     @Override
     @ActionEvent(eventType = EventTypes.EVENT_SECURITY_GROUP_DELETE, eventDescription = "deleting security group")

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

@@ -425,6 +425,7 @@ import org.apache.cloudstack.api.command.user.securitygroup.DeleteSecurityGroupC
 import org.apache.cloudstack.api.command.user.securitygroup.ListSecurityGroupsCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.RevokeSecurityGroupEgressCmd;
 import org.apache.cloudstack.api.command.user.securitygroup.RevokeSecurityGroupIngressCmd;
+import org.apache.cloudstack.api.command.user.securitygroup.UpdateSecurityGroupCmd;
 import org.apache.cloudstack.api.command.user.snapshot.ArchiveSnapshotCmd;
 import org.apache.cloudstack.api.command.user.snapshot.CreateSnapshotCmd;
 import org.apache.cloudstack.api.command.user.snapshot.CreateSnapshotFromVMSnapshotCmd;
@@ -2895,6 +2896,7 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe
         cmdList.add(ListSecurityGroupsCmd.class);
         cmdList.add(RevokeSecurityGroupEgressCmd.class);
         cmdList.add(RevokeSecurityGroupIngressCmd.class);
+        cmdList.add(UpdateSecurityGroupCmd.class);
         cmdList.add(CreateSnapshotCmd.class);
         cmdList.add(CreateSnapshotFromVMSnapshotCmd.class);
         cmdList.add(DeleteSnapshotCmd.class);

test/integration/smoke/test_update_security_group.py

@@ -0,0 +1,312 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Tests for updating security group name
+"""
+
+# Import Local Modules
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.cloudstackAPI import updateSecurityGroup, createSecurityGroup
+from marvin.sshClient import SshClient
+from marvin.lib.utils import (validateList,
+                              cleanup_resources,
+                              random_gen)
+from marvin.lib.base import (PhysicalNetwork,
+                             Account,
+                             Host,
+                             TrafficType,
+                             Domain,
+                             Network,
+                             NetworkOffering,
+                             VirtualMachine,
+                             ServiceOffering,
+                             Zone,
+                             SecurityGroup)
+from marvin.lib.common import (get_domain,
+                               get_zone,
+                               get_template,
+                               list_virtual_machines,
+                               list_routers,
+                               list_hosts,
+                               get_free_vlan)
+from marvin.codes import (PASS, FAIL)
+import logging
+import random
+import time
+
+class TestUpdateSecurityGroup(cloudstackTestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.testClient = super(
+            TestUpdateSecurityGroup,
+            cls).getClsTestClient()
+        cls.apiclient = cls.testClient.getApiClient()
+        cls.testdata = cls.testClient.getParsedTestDataConfig()
+        cls.services = cls.testClient.getParsedTestDataConfig()
+
+        zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.zone = Zone(zone.__dict__)
+        cls.template = get_template(cls.apiclient, cls.zone.id)
+        cls._cleanup = []
+
+        if str(cls.zone.securitygroupsenabled) != "True":
+            sys.exit(1)
+
+        cls.logger = logging.getLogger("TestUpdateSecurityGroup")
+        cls.stream_handler = logging.StreamHandler()
+        cls.logger.setLevel(logging.DEBUG)
+        cls.logger.addHandler(cls.stream_handler)
+
+        # Get Zone, Domain and templates
+        cls.domain = get_domain(cls.apiclient)
+        testClient = super(TestUpdateSecurityGroup, cls).getClsTestClient()
+        cls.zone = get_zone(cls.apiclient, testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # Create new domain, account, network and VM
+        cls.user_domain = Domain.create(
+            cls.apiclient,
+            services=cls.testdata["acl"]["domain2"],
+            parentdomainid=cls.domain.id)
+
+        # Create account
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.testdata["acl"]["accountD2"],
+            admin=True,
+            domainid=cls.user_domain.id
+        )
+
+        cls._cleanup.append(cls.account)
+        cls._cleanup.append(cls.user_domain)
+
+    @classmethod
+    def tearDownClass(self):
+        try:
+            cleanup_resources(self.apiclient, self._cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.cleanup = []
+        return
+
+    def tearDown(self):
+        try:
+            cleanup_resources(self.apiclient, self.cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    @attr(tags=["advancedsg"], required_hardware="false")
+    def test_01_create_security_group(self):
+        # Validate the following:
+        #
+        # 1. Create a new security group
+        # 2. Update the security group with new name
+        # 3. List the security group with new name as the keyword
+        # 4. Make sure that the response is not empty
+
+        security_group = SecurityGroup.create(
+            self.apiclient,
+            self.testdata["security_group"],
+            account=self.account.name,
+            domainid=self.account.domainid
+        )
+        self.debug("Created security group with ID: %s" % security_group.id)
+
+        initial_secgroup_name = security_group.name
+        new_secgroup_name = "testing-update-security-group"
+
+        cmd = updateSecurityGroup.updateSecurityGroupCmd()
+        cmd.id = security_group.id
+        cmd.name = new_secgroup_name
+        self.apiclient.updateSecurityGroup(cmd)
+
+        new_security_group = SecurityGroup.list(
+            self.apiclient,
+            account=self.account.name,
+            domainid=self.account.domainid,
+            keyword=new_secgroup_name
+        )
+        self.assertNotEqual(
+            len(new_security_group),
+            0,
+            "Update security group failed"
+        )
+
+    @attr(tags=["advancedsg"], required_hardware="false")
+    def test_02_duplicate_security_group_name(self):
+        # Validate the following
+        #
+        # 1. Create a security groups with name "test"
+        # 2. Try to create another security group with name "test"
+        # 3. Creation of second security group should fail
+
+        security_group_name = "test"
+        security_group = SecurityGroup.create(
+            self.apiclient,
+            {"name": security_group_name},
+            account=self.account.name,
+            domainid=self.account.domainid
+        )
+        self.debug("Created security group with name: %s" % security_group.name)
+
+        security_group_list = SecurityGroup.list(
+            self.apiclient,
+            account=self.account.name,
+            domainid=self.account.domainid,
+            keyword=security_group.name
+        )
+        self.assertNotEqual(
+            len(security_group_list),
+            0,
+            "Creating security group failed"
+        )
+
+        # Need to use createSecurituGroupCmd since SecurityGroup.create
+        # adds random string to security group name
+        with self.assertRaises(Exception):
+            cmd = createSecurityGroup.createSecurityGroupCmd()
+            cmd.name = security_group.name
+            cmd.account = self.account.name
+            cmd.domainid = self.account.domainid
+            self.apiclient.createSecurityGroup(cmd)
+
+    @attr(tags=["advancedsg"], required_hardware="false")
+    def test_03_update_security_group_with_existing_name(self):
+        # Validate the following
+        #
+        # 1. Create a security groups with name "test"
+        # 2. Create another security group
+        # 3. Try to update the second security group to update its name to "test"
+        # 4. Update security group should fail
+
+        # Create security group
+        security_group = SecurityGroup.create(
+            self.apiclient,
+            self.testdata["security_group"],
+            account=self.account.name,
+            domainid=self.account.domainid
+        )
+        self.debug("Created security group with ID: %s" % security_group.id)
+        security_group_name = security_group.name
+
+        # Make sure its created
+        security_group_list = SecurityGroup.list(
+            self.apiclient,
+            account=self.account.name,
+            domainid=self.account.domainid,
+            keyword=security_group_name
+        )
+        self.assertNotEqual(
+            len(security_group_list),
+            0,
+            "Creating security group failed"
+        )
+
+        # Create another security group
+        second_security_group = SecurityGroup.create(
+            self.apiclient,
+            self.testdata["security_group"],
+            account=self.account.name,
+            domainid=self.account.domainid
+        )
+        self.debug("Created security group with ID: %s" % second_security_group.id)
+
+        # Make sure its created
+        security_group_list = SecurityGroup.list(
+            self.apiclient,
+            account=self.account.name,
+            domainid=self.account.domainid,
+            keyword=second_security_group.name
+        )
+        self.assertNotEqual(
+            len(security_group_list),
+            0,
+            "Creating security group failed"
+        )
+
+        # Update the security group
+        with self.assertRaises(Exception):
+            cmd = updateSecurityGroup.updateSecurityGroupCmd()
+            cmd.id = second_security_group.id
+            cmd.name = security_group_name
+            self.apiclient.updateSecurityGroup(cmd)
+
+    @attr(tags=["advancedsg"], required_hardware="false")
+    def test_04_update_security_group_with_empty_name(self):
+        # Validate the following
+        #
+        # 1. Create a security group
+        # 2. Update the security group to an empty name
+        # 3. Update security group should fail
+
+        # Create security group
+        security_group = SecurityGroup.create(
+            self.apiclient,
+            self.testdata["security_group"],
+            account=self.account.name,
+            domainid=self.account.domainid
+        )
+        self.debug("Created security group with ID: %s" % security_group.id)
+
+        # Make sure its created
+        security_group_list = SecurityGroup.list(
+            self.apiclient,
+            account=self.account.name,
+            domainid=self.account.domainid,
+            keyword=security_group.name
+        )
+        self.assertNotEqual(
+            len(security_group_list),
+            0,
+            "Creating security group failed"
+        )
+
+        # Update the security group
+        with self.assertRaises(Exception):
+            cmd = updateSecurityGroup.updateSecurityGroupCmd()
+            cmd.id = security_group.id
+            cmd.name = ""
+            self.apiclient.updateSecurityGroup(cmd)
+
+    @attr(tags=["advancedsg"], required_hardware="false")
+    def test_05_rename_security_group(self):
+        # Validate the following
+        #
+        # 1. Create a security group
+        # 2. Update the security group and change its name to "default"
+        # 3. Exception should be thrown as "default" name cant be used
+
+        security_group = SecurityGroup.create(
+            self.apiclient,
+            self.testdata["security_group"],
+            account=self.account.name,
+            domainid=self.account.domainid
+        )
+        self.debug("Created security group with ID: %s" % security_group.id)
+
+        with self.assertRaises(Exception):
+            cmd = updateSecurityGroup.updateSecurityGroupCmd()
+            cmd.id = security_group.id
+            cmd.name = "default"
+            self.apiclient.updateSecurityGroup(cmd)

ui/scripts/network.js

@@ -361,6 +361,7 @@
                 args.context.item.state != 'Destroyed' &&
                 args.context.item.name != 'default') {
                 allowedActions.push('remove');
+                allowedActions.push('edit');
             }
 
             return allowedActions;
@@ -4523,7 +4524,11 @@
                                 title: 'label.details',
                                 fields: [{
                                     name: {
-                                        label: 'label.name'
+                                        label: 'label.name',
+                                        isEditable: true,
+                                        validation: {
+                                            required: true
+                                        }
                                     }
                                 }, {
                                     id: {
@@ -5075,6 +5080,30 @@
                         },
 
                         actions: {
+                            edit: {
+                                label: 'label.edit',
+                                action: function(args) {
+                                    var data = {
+                                        id: args.context.securityGroups[0].id
+                                    };
+                                    if (args.data.name != args.context.securityGroups[0].name) {
+                                        $.extend(data, {
+                                            name: args.data.name
+                                        });
+                                    };
+                                    $.ajax({
+                                        url: createURL('updateSecurityGroup'),
+                                        data: data,
+                                        success: function(json) {
+                                            var item = json.updatesecuritygroupresponse.securitygroup;
+                                            args.response.success({
+                                                data: item
+                                            });
+                                        }
+                                    });
+                                }
+                            },
+
                             remove: {
                                 label: 'label.action.delete.security.group',
                                 messages: {