Added sync and purge security group states

- Added list security groups CLI
- Removed unnecessary security group rule store

Change-Id: I62ac652e0af73c5f771f0caec87acd5dfe4abedd

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/api/OpenstackNetworkEvent.java

@@ -21,8 +21,6 @@ import org.openstack4j.model.network.Network;
 import org.openstack4j.model.network.Port;
 import org.openstack4j.model.network.Subnet;
 
-import java.util.Collection;
-
 import static com.google.common.base.MoreObjects.toStringHelper;
 
 /**
@@ -32,7 +30,7 @@ public class OpenstackNetworkEvent extends AbstractEvent<OpenstackNetworkEvent.T
 
     private final Port port;
     private final Subnet subnet;
-    private final Collection<String> sgRuleIds;
+    private final String securityGroupId;
 
     public enum Type {
         /**
@@ -83,12 +81,12 @@ public class OpenstackNetworkEvent extends AbstractEvent<OpenstackNetworkEvent.T
         /**
          * Signifies that the OpenStack security group rule is added to a specific port.
          */
-        OPENSTACK_SECURITY_GROUP_ADDED_TO_PORT,
+        OPENSTACK_PORT_SECURITY_GROUP_ADDED,
 
         /**
          * Signifies that the OpenStack security group rule is removed from a specific port.
          */
-        OPENSTACK_SECURITY_GROUP_REMOVED_FROM_PORT
+        OPENSTACK_PORT_SECURITY_GROUP_REMOVED
     }
 
     /**
@@ -100,7 +98,7 @@ public class OpenstackNetworkEvent extends AbstractEvent<OpenstackNetworkEvent.T
         super(type, network);
         this.port = null;
         this.subnet = null;
-        this.sgRuleIds = null;
+        this.securityGroupId = null;
     }
 
     /**
@@ -115,7 +113,7 @@ public class OpenstackNetworkEvent extends AbstractEvent<OpenstackNetworkEvent.T
         super(type, network);
         this.port = port;
         this.subnet = null;
-        this.sgRuleIds = null;
+        this.securityGroupId = null;
     }
 
     /**
@@ -130,21 +128,21 @@ public class OpenstackNetworkEvent extends AbstractEvent<OpenstackNetworkEvent.T
         super(type, network);
         this.port = null;
         this.subnet = subnet;
-        this.sgRuleIds = null;
+        this.securityGroupId = null;
     }
 
     /**
      * Creates an event of a given type for the specified port and security groups.
      *
      * @param type openstack network event type
-     * @param sgRuleIds openstack security group rules
      * @param port openstack port
+     * @param securityGroupId openstack security group
      */
-    public OpenstackNetworkEvent(Type type, Collection<String> sgRuleIds, Port port) {
+    public OpenstackNetworkEvent(Type type, Port port, String securityGroupId) {
         super(type, null);
         this.port = port;
-        this.sgRuleIds = sgRuleIds;
         this.subnet = null;
+        this.securityGroupId = securityGroupId;
     }
 
     /**
@@ -168,10 +166,10 @@ public class OpenstackNetworkEvent extends AbstractEvent<OpenstackNetworkEvent.T
     /**
      * Returns the security group rule IDs updated.
      *
-     * @return collection of security group rule ID
+     * @return openstack security group
      */
-    public Collection<String> securityGroupRuleIds() {
+    public String securityGroupId() {
-        return sgRuleIds;
+        return securityGroupId;
     }
 
     @Override
@@ -185,7 +183,7 @@ public class OpenstackNetworkEvent extends AbstractEvent<OpenstackNetworkEvent.T
                 .add("network", subject())
                 .add("port", port)
                 .add("subnet", subnet)
-                .add("security group rules", securityGroupRuleIds())
+                .add("security group", securityGroupId())
                 .toString();
     }
 }

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/api/OpenstackSecurityGroupAdminService.java

@@ -29,6 +29,13 @@ public interface OpenstackSecurityGroupAdminService {
      */
     void createSecurityGroup(SecurityGroup sg);
 
+    /**
+     * Updates the security group.
+     *
+     * @param sg security group
+     */
+    void updateSecurityGroup(SecurityGroup sg);
+
     /**
      * Removes the security group.
      *
@@ -49,4 +56,9 @@ public interface OpenstackSecurityGroupAdminService {
      * @param sgRuleId security group rule ID
      */
     void removeSecurityGroupRule(String sgRuleId);
+
+    /**
+     * Removes the existing security groups.
+     */
+    void clear();
 }

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/api/OpenstackSecurityGroupEvent.java

@@ -63,10 +63,12 @@ public class OpenstackSecurityGroupEvent
      * SecurityGroupEvent constructor.
      *
      * @param type SecurityGroupEvent type
+     * @param sg security group
      * @param sgRule SecurityGroup object
      */
-    public OpenstackSecurityGroupEvent(OpenstackSecurityGroupEvent.Type type, SecurityGroupRule sgRule) {
+    public OpenstackSecurityGroupEvent(OpenstackSecurityGroupEvent.Type type, SecurityGroup sg,
-        super(type, null);
+                                       SecurityGroupRule sgRule) {
+        super(type, sg);
         this.sgRule = sgRule;
     }
 

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/api/OpenstackSecurityGroupService.java

@@ -17,15 +17,22 @@ package org.onosproject.openstacknetworking.api;
 
 import org.onosproject.event.ListenerService;
 import org.openstack4j.model.network.SecurityGroup;
-import org.openstack4j.model.network.SecurityGroupRule;
+
+import java.util.Set;
 
 /**
  * Service for interfacing OpenStack SecurityGroup events and SecurityGroup store.
- *
  */
 public interface OpenstackSecurityGroupService
         extends ListenerService<OpenstackSecurityGroupEvent, OpenstackSecurityGroupListener> {
 
+    /**
+     * Returns all security groups.
+     *
+     * @return set of security group
+     */
+    Set<SecurityGroup> securityGroups();
+
     /**
      * Returns the security group for the sgId.
      *
@@ -33,13 +40,4 @@ public interface OpenstackSecurityGroupService
      * @return security group
      */
     SecurityGroup securityGroup(String sgId);
-
-    /**
-     * Returns the security group rule for the sgRuleId given.
-     *
-     * @param sgRuleId security group rule Id
-     * @return security group rule
-     */
-    SecurityGroupRule securityGroupRule(String sgRuleId);
-
 }

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/api/OpenstackSecurityGroupStore.java

@@ -18,7 +18,8 @@ package org.onosproject.openstacknetworking.api;
 
 import org.onosproject.store.Store;
 import org.openstack4j.model.network.SecurityGroup;
-import org.openstack4j.model.network.SecurityGroupRule;
+
+import java.util.Set;
 
 /**
  * Manages inventory of OpenStack security group states; not intended for direct use.
@@ -36,11 +37,9 @@ public interface OpenstackSecurityGroupStore
     /**
      * Updates the security group with the security group ID with the security group object.
      *
-     * @param sgId security group ID
      * @param sg new SecurityGroup object
-     * @return old SecurityGroup object
      */
-    SecurityGroup updateSecurityGroup(String sgId, SecurityGroup sg);
+    void updateSecurityGroup(SecurityGroup sg);
 
     /**
      * Removes the security group with the security group ID.
@@ -50,21 +49,6 @@ public interface OpenstackSecurityGroupStore
      */
     SecurityGroup removeSecurityGroup(String sgId);
 
-    /**
-     * Creates a security group rule.
-     *
-     * @param sgRule security group rule
-     */
-    void createSecurityGroupRule(SecurityGroupRule sgRule);
-
-    /**
-     * Removes the security group rule with the security group rule ID.
-     *
-     * @param sgRuleId security group rule ID to remove
-     * @return SecurityGroupRule object removed
-     */
-    SecurityGroupRule removeSecurityGroupRule(String sgRuleId);
-
     /**
      * Returns the security group with the security group ID.
      *
@@ -74,11 +58,14 @@ public interface OpenstackSecurityGroupStore
     SecurityGroup securityGroup(String sgId);
 
     /**
-     * Returns the security group rule with the security group ID.
+     * Returns all security groups.
      *
-     * @param sgRuleId security group rule ID
+     * @return set of security groups
-     * @return Security Group Rule
      */
-    SecurityGroupRule securityGroupRule(String sgRuleId);
+    Set<SecurityGroup> securityGroups();
 
+    /**
+     * Clears the security group store.
+     */
+    void clear();
 }

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/cli/OpenstackPurgeStateCommand.java

@@ -19,6 +19,7 @@ import org.apache.karaf.shell.commands.Command;
 import org.onosproject.cli.AbstractShellCommand;
 import org.onosproject.openstacknetworking.api.OpenstackNetworkAdminService;
 import org.onosproject.openstacknetworking.api.OpenstackRouterAdminService;
+import org.onosproject.openstacknetworking.api.OpenstackSecurityGroupAdminService;
 
 /**
  * Purges all existing network states.
@@ -29,7 +30,8 @@ public class OpenstackPurgeStateCommand extends AbstractShellCommand {
 
     @Override
     protected void execute() {
-        get(OpenstackNetworkAdminService.class).clear();
         get(OpenstackRouterAdminService.class).clear();
+        get(OpenstackNetworkAdminService.class).clear();
+        get(OpenstackSecurityGroupAdminService.class).clear();
     }
 }

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/cli/OpenstackSecurityGroupListCommand.java

@@ -0,0 +1,90 @@
+/*
+ * Copyright 2017-present Open Networking Laboratory
+ *
+ * Licensed 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.onosproject.openstacknetworking.cli;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.google.common.collect.Lists;
+import org.apache.karaf.shell.commands.Argument;
+import org.apache.karaf.shell.commands.Command;
+import org.onosproject.cli.AbstractShellCommand;
+import org.onosproject.openstacknetworking.api.OpenstackSecurityGroupService;
+import org.openstack4j.core.transport.ObjectMapperSingleton;
+import org.openstack4j.model.network.SecurityGroup;
+import org.openstack4j.openstack.networking.domain.NeutronSecurityGroup;
+
+import java.util.Comparator;
+import java.util.List;
+
+import static com.fasterxml.jackson.databind.SerializationFeature.INDENT_OUTPUT;
+
+/**
+ * Lists OpenStack security groups.
+ */
+@Command(scope = "onos", name = "openstack-security-groups",
+        description = "Lists all OpenStack security groups")
+public class OpenstackSecurityGroupListCommand extends AbstractShellCommand {
+
+    private static final String FORMAT = "%-40s%-20s";
+
+    @Argument(name = "networkId", description = "Network ID")
+    private String networkId = null;
+
+    @Override
+    protected void execute() {
+        OpenstackSecurityGroupService service =
+                AbstractShellCommand.get(OpenstackSecurityGroupService.class);
+
+        List<SecurityGroup> sgs = Lists.newArrayList(service.securityGroups());
+        sgs.sort(Comparator.comparing(SecurityGroup::getId));
+
+        if (outputJson()) {
+            try {
+                print("%s", mapper().writeValueAsString(json(sgs)));
+            } catch (JsonProcessingException e) {
+                error("Failed to list security groups in JSON format");
+            }
+            return;
+        }
+
+        print("Hint: use --json option to see security group rules as well\n");
+        print(FORMAT, "ID", "Name");
+        for (SecurityGroup sg: service.securityGroups()) {
+            print(FORMAT, sg.getId(), sg.getName());
+        }
+    }
+
+    private JsonNode json(List<SecurityGroup> securityGroups) {
+        ArrayNode result = mapper().enable(INDENT_OUTPUT).createArrayNode();
+        for (SecurityGroup sg: securityGroups) {
+            result.add(writeSecurityGroup(sg));
+        }
+        return result;
+    }
+
+    private ObjectNode writeSecurityGroup(SecurityGroup sg) {
+        try {
+            String strSg = ObjectMapperSingleton.getContext(NeutronSecurityGroup.class)
+                    .writerFor(NeutronSecurityGroup.class)
+                    .writeValueAsString(sg);
+            return (ObjectNode) mapper().readTree(strSg.getBytes());
+        } catch (Exception e) {
+            throw new IllegalArgumentException();
+        }
+    }
+}

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/cli/OpenstackSyncStateCommand.java

@@ -24,6 +24,8 @@ import org.onosproject.openstacknetworking.api.OpenstackNetworkAdminService;
 import org.onosproject.openstacknetworking.api.OpenstackNetworkService;
 import org.onosproject.openstacknetworking.api.OpenstackRouterAdminService;
 import org.onosproject.openstacknetworking.api.OpenstackRouterService;
+import org.onosproject.openstacknetworking.api.OpenstackSecurityGroupAdminService;
+import org.onosproject.openstacknetworking.api.OpenstackSecurityGroupService;
 import org.openstack4j.api.OSClient;
 import org.openstack4j.api.exceptions.AuthenticationException;
 import org.openstack4j.model.identity.Access;
@@ -67,6 +69,7 @@ public class OpenstackSyncStateCommand extends AbstractShellCommand {
             required = true, multiValued = false)
     private String password = null;
 
+    private static final String SECURITY_GROUP_FORMAT = "%-40s%-20s";
     private static final String NETWORK_FORMAT = "%-40s%-20s%-20s%-8s";
     private static final String SUBNET_FORMAT = "%-40s%-20s%-20s";
     private static final String PORT_FORMAT = "%-40s%-20s%-20s%-8s";
@@ -78,6 +81,8 @@ public class OpenstackSyncStateCommand extends AbstractShellCommand {
 
     @Override
     protected void execute() {
+        OpenstackSecurityGroupAdminService osSgAdminService = get(OpenstackSecurityGroupAdminService.class);
+        OpenstackSecurityGroupService osSgService = get(OpenstackSecurityGroupService.class);
         OpenstackNetworkAdminService osNetAdminService = get(OpenstackNetworkAdminService.class);
         OpenstackNetworkService osNetService = get(OpenstackNetworkService.class);
         OpenstackRouterAdminService osRouterAdminService = get(OpenstackRouterAdminService.class);
@@ -101,7 +106,18 @@ public class OpenstackSyncStateCommand extends AbstractShellCommand {
 
         OSClient osClient = OSFactory.clientFromAccess(osAccess);
 
-        print("Synchronizing OpenStack networks...");
+        print("Synchronizing OpenStack security groups");
+        print(SECURITY_GROUP_FORMAT, "ID", "Name");
+        osClient.networking().securitygroup().list().forEach(osSg -> {
+            if (osSgService.securityGroup(osSg.getId()) != null) {
+                osSgAdminService.updateSecurityGroup(osSg);
+            } else {
+                osSgAdminService.createSecurityGroup(osSg);
+            }
+            print(SECURITY_GROUP_FORMAT, osSg.getId(), osSg.getName());
+        });
+
+        print("\nSynchronizing OpenStack networks");
         print(NETWORK_FORMAT, "ID", "Name", "VNI", "Subnets");
         osClient.networking().network().list().forEach(osNet -> {
             if (osNetService.network(osNet.getId()) != null) {
@@ -112,7 +128,7 @@ public class OpenstackSyncStateCommand extends AbstractShellCommand {
             printNetwork(osNet);
         });
 
-        print("\nSynchronizing OpenStack subnets...");
+        print("\nSynchronizing OpenStack subnets");
         print(SUBNET_FORMAT, "ID", "Network", "CIDR");
         osClient.networking().subnet().list().forEach(osSubnet -> {
             if (osNetService.subnet(osSubnet.getId()) != null) {
@@ -123,7 +139,7 @@ public class OpenstackSyncStateCommand extends AbstractShellCommand {
             printSubnet(osSubnet, osNetService);
         });
 
-        print("\nSynchronizing OpenStack ports...");
+        print("\nSynchronizing OpenStack ports");
         print(PORT_FORMAT, "ID", "Network", "MAC", "Fixed IPs");
         osClient.networking().port().list().forEach(osPort -> {
             if (osNetService.port(osPort.getId()) != null) {
@@ -134,7 +150,7 @@ public class OpenstackSyncStateCommand extends AbstractShellCommand {
             printPort(osPort, osNetService);
         });
 
-        print("\nSynchronizing OpenStack routers...");
+        print("\nSynchronizing OpenStack routers");
         print(ROUTER_FORMAT, "ID", "Name", "External", "Internal");
         osClient.networking().router().list().forEach(osRouter -> {
             if (osRouterService.router(osRouter.getId()) != null) {
@@ -153,7 +169,7 @@ public class OpenstackSyncStateCommand extends AbstractShellCommand {
             printRouter(osRouter, osNetService);
         });
 
-        print("\nSynchronizing OpenStack floating IPs...");
+        print("\nSynchronizing OpenStack floating IPs");
         print(FLOATING_IP_FORMAT, "ID", "Floating IP", "Fixed IP");
         osClient.networking().floatingip().list().forEach(osFloating -> {
             if (osRouterService.floatingIp(osFloating.getId()) != null) {

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/impl/DistributedOpenstackNetworkStore.java

@@ -15,8 +15,8 @@
  */
 package org.onosproject.openstacknetworking.impl;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
-import org.apache.commons.collections.CollectionUtils;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
@@ -53,7 +53,7 @@ import org.openstack4j.openstack.networking.domain.NeutronPort;
 import org.openstack4j.openstack.networking.domain.NeutronSubnet;
 import org.slf4j.Logger;
 
-import java.util.Collection;
+import java.util.List;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.stream.Collectors;
@@ -358,27 +358,10 @@ public class DistributedOpenstackNetworkStore
                     eventExecutor.execute(() -> {
                         Port oldPort = event.oldValue().value();
                         Port newPort = event.newValue().value();
-
                         notifyDelegate(new OpenstackNetworkEvent(
                                 OPENSTACK_PORT_UPDATED,
                                 network(event.newValue().value().getNetworkId()), newPort));
-
+                        processSecurityGroupUpdate(oldPort, newPort);
-                        if (!newPort.getSecurityGroups().equals(oldPort.getSecurityGroups())) {
-                            Collection<String> sgToAdd = CollectionUtils.subtract(newPort.getSecurityGroups(),
-                                    oldPort.getSecurityGroups());
-                            if (!sgToAdd.isEmpty()) {
-                                notifyDelegate(new OpenstackNetworkEvent(
-                                        OpenstackNetworkEvent.Type.OPENSTACK_SECURITY_GROUP_ADDED_TO_PORT,
-                                        sgToAdd, newPort));
-                            }
-                            Collection<String> sgToRemove = CollectionUtils.subtract(oldPort.getSecurityGroups(),
-                                    newPort.getSecurityGroups());
-                            if (!sgToRemove.isEmpty()) {
-                                notifyDelegate(new OpenstackNetworkEvent(
-                                        OpenstackNetworkEvent.Type.OPENSTACK_SECURITY_GROUP_REMOVED_FROM_PORT,
-                                        sgToRemove, newPort));
-                            }
-                        }
                     });
                     break;
                 case INSERT:
@@ -404,5 +387,24 @@ public class DistributedOpenstackNetworkStore
                     break;
             }
         }
+
+        private void processSecurityGroupUpdate(Port oldPort, Port newPort) {
+            List<String> oldSecurityGroups = oldPort.getSecurityGroups() == null ?
+                    ImmutableList.of() : oldPort.getSecurityGroups();
+            List<String> newSecurityGroups = newPort.getSecurityGroups() == null ?
+                    ImmutableList.of() : newPort.getSecurityGroups();
+
+            oldSecurityGroups.stream()
+                    .filter(sgId -> !newPort.getSecurityGroups().contains(sgId))
+                    .forEach(sgId -> notifyDelegate(new OpenstackNetworkEvent(
+                            OPENSTACK_PORT_SECURITY_GROUP_REMOVED, newPort, sgId
+                    )));
+
+            newSecurityGroups.stream()
+                    .filter(sgId -> !oldPort.getSecurityGroups().contains(sgId))
+                    .forEach(sgId -> notifyDelegate(new OpenstackNetworkEvent(
+                            OPENSTACK_PORT_SECURITY_GROUP_ADDED, newPort, sgId
+                    )));
+        }
     }
 }

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/impl/DistributedSecurityGroupStore.java

@@ -15,6 +15,7 @@
  */
 package org.onosproject.openstacknetworking.impl;
 
+import com.google.common.collect.ImmutableSet;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
@@ -41,12 +42,15 @@ import org.openstack4j.openstack.networking.domain.NeutronSecurityGroup;
 import org.openstack4j.openstack.networking.domain.NeutronSecurityGroupRule;
 import org.slf4j.Logger;
 
+import java.util.Set;
 import java.util.concurrent.ExecutorService;
+import java.util.stream.Collectors;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static java.util.concurrent.Executors.newSingleThreadExecutor;
 import static org.onlab.util.Tools.groupedThreads;
 import static org.onosproject.openstacknetworking.api.Constants.OPENSTACK_NETWORKING_APP_ID;
+import static org.onosproject.openstacknetworking.api.OpenstackSecurityGroupEvent.Type.*;
 import static org.slf4j.LoggerFactory.getLogger;
 
 /**
@@ -83,11 +87,8 @@ public class DistributedSecurityGroupStore
 
     private final MapEventListener<String, SecurityGroup> securityGroupMapListener =
             new OpenstackSecurityGroupMapListener();
-    private final MapEventListener<String, SecurityGroupRule> securityGroupRuleMapListener =
-            new OpenstackSecurityGroupRuleMapListener();
 
     private ConsistentMap<String, SecurityGroup> osSecurityGroupStore;
-    private ConsistentMap<String, SecurityGroupRule> osSecurityGroupRuleStore;
 
     @Activate
     protected void activate() {
@@ -100,20 +101,12 @@ public class DistributedSecurityGroupStore
                 .build();
         osSecurityGroupStore.addListener(securityGroupMapListener);
 
-        osSecurityGroupRuleStore = storageService.<String, SecurityGroupRule>consistentMapBuilder()
-                .withSerializer(Serializer.using(SERIALIZER_SECURITY_GROUP))
-                .withName("openstack-securitygrouprulestore")
-                .withApplicationId(appId)
-                .build();
-        osSecurityGroupRuleStore.addListener(securityGroupRuleMapListener);
-
         log.info("Started");
     }
 
     @Deactivate
     protected void deactivate() {
         osSecurityGroupStore.removeListener(securityGroupMapListener);
-        osSecurityGroupRuleStore.removeListener(securityGroupRuleMapListener);
         eventExecutor.shutdown();
 
         log.info("Stopped");
@@ -129,9 +122,12 @@ public class DistributedSecurityGroupStore
     }
 
     @Override
-    public SecurityGroup updateSecurityGroup(String sgId, SecurityGroup newSg) {
+    public void updateSecurityGroup(SecurityGroup sg) {
-        Versioned<SecurityGroup> sg = osSecurityGroupStore.replace(sgId, newSg);
+        osSecurityGroupStore.compute(sg.getId(), (id, existing) -> {
-        return sg == null ? null : sg.value();
+            final String error = sg.getName() + ERR_NOT_FOUND;
+            checkArgument(existing != null, error);
+            return sg;
+        });
     }
 
     @Override
@@ -140,21 +136,6 @@ public class DistributedSecurityGroupStore
         return sg == null ? null : sg.value();
     }
 
-    @Override
-    public void createSecurityGroupRule(SecurityGroupRule sgRule) {
-        osSecurityGroupRuleStore.compute(sgRule.getId(), (id, existing) -> {
-            final String error = sgRule.getId() + ERR_DUPLICATE;
-            checkArgument(existing == null, error);
-            return sgRule;
-        });
-    }
-
-    @Override
-    public SecurityGroupRule removeSecurityGroupRule(String sgRuleId) {
-        Versioned<SecurityGroupRule> sgRule = osSecurityGroupRuleStore.remove(sgRuleId);
-        return sgRule == null ? null : sgRule.value();
-    }
-
     @Override
     public SecurityGroup securityGroup(String sgId) {
         Versioned<SecurityGroup> osSg = osSecurityGroupStore.get(sgId);
@@ -162,9 +143,16 @@ public class DistributedSecurityGroupStore
     }
 
     @Override
-    public SecurityGroupRule securityGroupRule(String sgRuleId) {
+    public Set<SecurityGroup> securityGroups() {
-        Versioned<SecurityGroupRule> osSgRule = osSecurityGroupRuleStore.get(sgRuleId);
+        Set<SecurityGroup> osSgs = osSecurityGroupStore.values().stream()
-        return osSgRule == null ? null : osSgRule.value();
+                .map(Versioned::value)
+                .collect(Collectors.toSet());
+        return ImmutableSet.copyOf(osSgs);
+    }
+
+    @Override
+    public void clear() {
+        osSecurityGroupStore.clear();
     }
 
     private class OpenstackSecurityGroupMapListener implements MapEventListener<String, SecurityGroup> {
@@ -173,47 +161,43 @@ public class DistributedSecurityGroupStore
         public void event(MapEvent<String, SecurityGroup> event) {
             switch (event.type()) {
                 case INSERT:
-                    log.debug("Openstack Security Group created {}", event.newValue());
+                    log.debug("OpenStack security group created {}", event.newValue());
                     eventExecutor.execute(() ->
                             notifyDelegate(new OpenstackSecurityGroupEvent(
-                                    OpenstackSecurityGroupEvent.Type.OPENSTACK_SECURITY_GROUP_CREATED,
+                                    OPENSTACK_SECURITY_GROUP_CREATED,
-                                    securityGroup(event.newValue().value().getId()))));
+                                    event.newValue().value())));
+                    break;
+                case UPDATE:
+                    log.debug("OpenStack security group updated {}", event.newValue());
+                    eventExecutor.execute(() -> processUpdate(
+                            event.oldValue().value(),
+                            event.newValue().value()));
                     break;
-
                 case REMOVE:
-                    log.debug("Openstack Security Group removed {}", event.newValue());
+                    log.debug("OpenStack security group removed {}", event.newValue());
                     eventExecutor.execute(() ->
                             notifyDelegate(new OpenstackSecurityGroupEvent(
-                                    OpenstackSecurityGroupEvent.Type.OPENSTACK_SECURITY_GROUP_REMOVED,
+                                    OPENSTACK_SECURITY_GROUP_REMOVED,
                                     event.oldValue().value())));
                     break;
                 default:
             }
         }
-    }
 
-    private class OpenstackSecurityGroupRuleMapListener implements MapEventListener<String, SecurityGroupRule> {
+        private void processUpdate(SecurityGroup oldSg, SecurityGroup newSg) {
+            Set<String> oldSgRuleIds = oldSg.getRules().stream()
+                    .map(SecurityGroupRule::getId).collect(Collectors.toSet());
+            Set<String> newSgRuleIds = newSg.getRules().stream()
+                    .map(SecurityGroupRule::getId).collect(Collectors.toSet());
 
-        @Override
+            oldSg.getRules().stream().filter(sgRule -> !newSgRuleIds.contains(sgRule.getId()))
-        public void event(MapEvent<String, SecurityGroupRule> event) {
+                    .forEach(sgRule -> notifyDelegate(new OpenstackSecurityGroupEvent(
-            switch (event.type()) {
+                            OPENSTACK_SECURITY_GROUP_RULE_REMOVED, newSg, sgRule)
-                case INSERT:
+                    ));
-                    log.debug("Openstack Security Group Rule created {}", event.newValue());
+            newSg.getRules().stream().filter(sgRule -> !oldSgRuleIds.contains(sgRule.getId()))
-                    eventExecutor.execute(() ->
+                    .forEach(sgRule -> notifyDelegate(new OpenstackSecurityGroupEvent(
-                            notifyDelegate(new OpenstackSecurityGroupEvent(
+                            OPENSTACK_SECURITY_GROUP_RULE_CREATED, newSg, sgRule)
-                                    OpenstackSecurityGroupEvent.Type.OPENSTACK_SECURITY_GROUP_RULE_CREATED,
+                    ));
-                                    securityGroupRule(event.newValue().value().getId()))));
-                    break;
-
-                case REMOVE:
-                    log.debug("Openstack Security Group Rule removed {}", event.oldValue());
-                    eventExecutor.execute(() ->
-                            notifyDelegate(new OpenstackSecurityGroupEvent(
-                                    OpenstackSecurityGroupEvent.Type.OPENSTACK_SECURITY_GROUP_RULE_REMOVED,
-                                    event.oldValue().value())));
-                    break;
-                default:
-            }
         }
     }
 }

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java

@@ -53,7 +53,6 @@ import org.openstack4j.model.network.SecurityGroupRule;
 import org.openstack4j.openstack.networking.domain.NeutronSecurityGroupRule;
 import org.slf4j.Logger;
 
-import java.util.Collection;
 import java.util.Collections;
 import java.util.Objects;
 import java.util.Set;
@@ -84,7 +83,7 @@ public class OpenstackSecurityGroupHandler {
     protected MastershipService mastershipService;
 
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
-    protected OpenstackNetworkService openstackService;
+    protected OpenstackNetworkService osNetService;
 
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected OpenstackSecurityGroupService securityGroupService;
@@ -113,7 +112,7 @@ public class OpenstackSecurityGroupHandler {
         appId = coreService.registerApplication(OPENSTACK_NETWORKING_APP_ID);
         instancePortService.addListener(instancePortListener);
         securityGroupService.addListener(securityGroupListener);
-        openstackService.addListener(portListener);
+        osNetService.addListener(portListener);
 
         log.info("Started");
     }
@@ -122,7 +121,7 @@ public class OpenstackSecurityGroupHandler {
     protected void deactivate() {
         instancePortService.removeListener(instancePortListener);
         securityGroupService.removeListener(securityGroupListener);
-        openstackService.removeListener(portListener);
+        osNetService.removeListener(portListener);
         eventExecutor.shutdown();
 
         log.info("Stopped");
@@ -130,13 +129,14 @@ public class OpenstackSecurityGroupHandler {
 
     private void setSecurityGroupRules(InstancePort instPort, Port port, boolean install) {
         port.getSecurityGroups().forEach(sgId -> {
-            log.debug("security group rule ID : " + sgId.toString());
             SecurityGroup sg = securityGroupService.securityGroup(sgId);
             if (sg == null) {
                 log.error("Security Group Not Found : {}", sgId);
                 return;
             }
             sg.getRules().forEach(sgRule -> updateSecurityGroupRule(instPort, port, sgRule, install));
+            final String action = install ? "Installed " : "Removed ";
+            log.debug(action + "security group rule ID : " + sgId);
         });
     }
 
@@ -185,7 +185,7 @@ public class OpenstackSecurityGroupHandler {
     private Set<InstancePort> getRemoteInstPorts(String tenantId, String sgId) {
         Set<InstancePort> remoteInstPorts;
 
-        remoteInstPorts = openstackService.ports().stream()
+        remoteInstPorts = osNetService.ports().stream()
                 .filter(port -> port.getTenantId().equals(tenantId))
                 .filter(port -> port.getSecurityGroups().contains(sgId))
                 .map(port -> instancePortService.instancePort(port.getId()))
@@ -307,81 +307,78 @@ public class OpenstackSecurityGroupHandler {
             switch (event.type()) {
                 case OPENSTACK_INSTANCE_PORT_UPDATED:
                 case OPENSTACK_INSTANCE_PORT_DETECTED:
+                    log.debug("Instance port detected MAC:{} IP:{}",
+                            instPort.macAddress(),
+                            instPort.ipAddress());
                     eventExecutor.execute(() -> {
-                        log.info("Instance port detected MAC:{} IP:{}",
+                        setSecurityGroupRules(instPort,
-                                instPort.macAddress(),
+                                osNetService.port(event.subject().portId()),
-                                instPort.ipAddress());
+                                true);
-                        instPortDetected(event.subject(), openstackService.port(event.subject().portId()));
                     });
                     break;
                 case OPENSTACK_INSTANCE_PORT_VANISHED:
+                    log.debug("Instance port vanished MAC:{} IP:{}",
+                            instPort.macAddress(),
+                            instPort.ipAddress());
                     eventExecutor.execute(() -> {
-                        log.info("Instance port vanished MAC:{} IP:{}",
+                        setSecurityGroupRules(instPort,
-                                instPort.macAddress(),
+                                osNetService.port(event.subject().portId()),
-                                instPort.ipAddress());
+                                false);
-                        instPortRemoved(event.subject(), openstackService.port(event.subject().portId()));
                     });
                     break;
                 default:
                     break;
             }
         }
-
-        private void instPortDetected(InstancePort instPort, Port port) {
-            setSecurityGroupRules(instPort, port, true);
-        }
-
-        private void instPortRemoved(InstancePort instPort, Port port) {
-            setSecurityGroupRules(instPort, port, false);
-        }
     }
 
     private class InternalOpenstackPortListener implements OpenstackNetworkListener {
 
         @Override
         public boolean isRelevant(OpenstackNetworkEvent event) {
-            Port osPort = event.port();
+            if (event.port() == null || !Strings.isNullOrEmpty(event.port().getId())) {
-            if (osPort == null) {
                 return false;
             }
-            return !Strings.isNullOrEmpty(osPort.getId());
+            if (event.securityGroupId() == null ||
+                    securityGroupService.securityGroup(event.securityGroupId()) == null) {
+                return false;
+            }
+            if (instancePortService.instancePort(event.port().getId()) == null) {
+                return false;
+            }
+            return true;
         }
 
         @Override
         public void event(OpenstackNetworkEvent event) {
+            Port osPort = event.port();
+            InstancePort instPort = instancePortService.instancePort(osPort.getId());
+            SecurityGroup osSg = securityGroupService.securityGroup(event.securityGroupId());
+
             switch (event.type()) {
-                case OPENSTACK_SECURITY_GROUP_ADDED_TO_PORT:
+                case OPENSTACK_PORT_SECURITY_GROUP_ADDED:
-                    securityGroupAddedToPort(event.securityGroupRuleIds(), event.port());
+                    eventExecutor.execute(() -> {
+                        osSg.getRules().forEach(sgRule -> {
+                            updateSecurityGroupRule(instPort, osPort, sgRule, true);
+                        });
+                        log.info("Added security group {} to port {}",
+                                event.securityGroupId(), event.port().getId());
+                    });
                     break;
-                case OPENSTACK_SECURITY_GROUP_REMOVED_FROM_PORT:
+                case OPENSTACK_PORT_SECURITY_GROUP_REMOVED:
-                    securityGroupRemovedFromPort(event.securityGroupRuleIds(), event.port());
+                    eventExecutor.execute(() -> {
+                        osSg.getRules().forEach(sgRule -> {
+                            updateSecurityGroupRule(instPort, osPort, sgRule, false);
+                        });
+                        log.info("Removed security group {} from port {}",
+                                event.securityGroupId(), event.port().getId());
+                    });
                     break;
                 default:
+                    // do nothing for the other events
                     break;
             }
         }
-
-        private void securityGroupAddedToPort(Collection<String> sgToAdd, Port osPort) {
-            sgToAdd.forEach(sg -> {
-                InstancePort instPort = instancePortService.instancePort(osPort.getId());
-                if (instPort != null) {
-                    securityGroupService.securityGroup(sg).getRules().stream()
-                            .forEach(sgRule -> updateSecurityGroupRule(instancePortService.instancePort(
-                                    osPort.getId()), osPort, sgRule, true));
-                }
-            });
-        }
-
-        private void securityGroupRemovedFromPort(Collection<String> sgToRemove, Port osPort) {
-            sgToRemove.forEach(sg -> {
-                InstancePort instPort = instancePortService.instancePort(osPort.getId());
-                if (instPort != null) {
-                    securityGroupService.securityGroup(sg).getRules().stream()
-                            .forEach(sgRule -> updateSecurityGroupRule(instancePortService.instancePort(
-                                    osPort.getId()), osPort, sgRule, false));
-                }
-            });
-        }
     }
 
     private class InternalSecurityGroupListener implements OpenstackSecurityGroupListener {
@@ -389,46 +386,53 @@ public class OpenstackSecurityGroupHandler {
         @Override
         public void event(OpenstackSecurityGroupEvent event) {
             switch (event.type()) {
-                case OPENSTACK_SECURITY_GROUP_CREATED:
-                case OPENSTACK_SECURITY_GROUP_REMOVED:
-                    break;
                 case OPENSTACK_SECURITY_GROUP_RULE_CREATED:
                     SecurityGroupRule securityGroupRuleToAdd = event.securityGroupRule();
                     eventExecutor.execute(() -> {
-                        log.info("Security group rule detected: ID {}",
-                                securityGroupRuleToAdd.getId());
                         securityGroupRuleAdded(securityGroupRuleToAdd);
+                        log.info("Applied new security group rule {} to ports",
+                                securityGroupRuleToAdd.getId());
                     });
                     break;
 
                 case OPENSTACK_SECURITY_GROUP_RULE_REMOVED:
                     SecurityGroupRule securityGroupRuleToRemove = event.securityGroupRule();
                     eventExecutor.execute(() -> {
-                        log.info("security gorup rule removed: ID {}",
-                                securityGroupRuleToRemove.getId());
                         securityGroupRuleRemoved(securityGroupRuleToRemove);
+                        log.info("Removed security group rule {} from ports",
+                                securityGroupRuleToRemove.getId());
                     });
                     break;
+                case OPENSTACK_SECURITY_GROUP_CREATED:
+                case OPENSTACK_SECURITY_GROUP_REMOVED:
                 default:
+                    // do nothing
+                    break;
             }
         }
 
         private void securityGroupRuleAdded(SecurityGroupRule sgRule) {
-            log.debug("securityGroupRuleAdded : {}" + sgRule);
+            osNetService.ports().stream()
-
-            openstackService.ports().stream()
                     .filter(port -> port.getSecurityGroups().contains(sgRule.getSecurityGroupId()))
-                    .forEach(port -> updateSecurityGroupRule(instancePortService.instancePort(port.getId()),
+                    .forEach(port -> {
-                            port, sgRule, true));
+                        updateSecurityGroupRule(
+                                instancePortService.instancePort(port.getId()),
+                                port, sgRule, true);
+                        log.debug("Applied security group rule {} to port {}",
+                                sgRule.getId(), port.getId());
+                    });
         }
 
         private void securityGroupRuleRemoved(SecurityGroupRule sgRule) {
-            log.debug("securityGroupRuleRemoved : {}" + sgRule);
+            osNetService.ports().stream()
-
-            openstackService.ports().stream()
                     .filter(port -> port.getSecurityGroups().contains(sgRule.getSecurityGroupId()))
-                    .forEach(port -> updateSecurityGroupRule(instancePortService.instancePort(port.getId()),
+                    .forEach(port -> {
-                            port, sgRule, false));
+                        updateSecurityGroupRule(
+                                instancePortService.instancePort(port.getId()),
+                                port, sgRule, false);
+                        log.debug("Removed security group rule {} from port {}",
+                                sgRule.getId(), port.getId());
+                    });
         }
     }
 }

apps/openstacknetworking/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupManager.java

@@ -38,15 +38,16 @@ import org.openstack4j.openstack.networking.domain.NeutronSecurityGroup;
 import org.slf4j.Logger;
 
 import java.util.List;
+import java.util.Objects;
+import java.util.Set;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static org.slf4j.LoggerFactory.getLogger;
 
 /**
- * Provides implementation of administering and interfaceing Openstack security
+ * Provides implementation of administering and interfacing OpenStack security
  * groups.
- *
  */
 @Service
 @Component(immediate = true)
@@ -57,8 +58,7 @@ public class OpenstackSecurityGroupManager
     protected final Logger log = getLogger(getClass());
 
     private static final String MSG_SG = "OpenStack security group %s %s";
-    private static final String MSG_SG_RULE = "OpenStack security group %s %s";
+    private static final String MSG_SG_RULE = "OpenStack security group rule %s %s";
-
 
     private static final String MSG_CREATED = "created";
     private static final String MSG_REMOVED = "removed";
@@ -67,6 +67,8 @@ public class OpenstackSecurityGroupManager
     private static final String ERR_NULL_SG_ID = "OpenStack security group ID cannot be null";
     private static final String ERR_NULL_SG_RULE = "OpenStack security group rule cannot be null";
     private static final String ERR_NULL_SG_RULE_ID = "OpenStack security group rule ID cannot be null";
+    private static final String ERR_NOT_FOUND = "not found";
+    private static final String ERR_DUPLICATE = "already exist";
 
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected CoreService coreService;
@@ -98,6 +100,14 @@ public class OpenstackSecurityGroupManager
         log.info(String.format(MSG_SG, sg.getId(), MSG_CREATED));
     }
 
+    @Override
+    public void updateSecurityGroup(SecurityGroup sg) {
+        checkNotNull(sg, ERR_NULL_SG);
+        checkArgument(!Strings.isNullOrEmpty(sg.getId()), ERR_NULL_SG_ID);
+
+        osSecurityGroupStore.updateSecurityGroup(sg);
+    }
+
     @Override
     public void removeSecurityGroup(String sgId) {
         checkNotNull(sgId, ERR_NULL_SG_ID);
@@ -110,30 +120,68 @@ public class OpenstackSecurityGroupManager
     public void createSecurityGroupRule(SecurityGroupRule sgRule) {
         checkNotNull(sgRule, ERR_NULL_SG_RULE);
         checkArgument(!Strings.isNullOrEmpty(sgRule.getId()), ERR_NULL_SG_RULE_ID);
+        checkArgument(!Strings.isNullOrEmpty(sgRule.getSecurityGroupId()), ERR_NULL_SG_ID);
 
-        synchronized (osSecurityGroupStore) {
+        synchronized (this) {
             SecurityGroup sg = securityGroup(sgRule.getSecurityGroupId());
-            List sgRules = sg.getRules();
+            if (sg == null) {
-            sgRules.add(sgRule);
+                final String error = String.format(MSG_SG, sgRule.getSecurityGroupId(), ERR_NOT_FOUND);
-            SecurityGroup newSg = new NeutronSecurityGroup.SecurityGroupConcreteBuilder().from(sg).build();
+                throw new IllegalStateException(error);
-            SecurityGroup oldSg = osSecurityGroupStore.updateSecurityGroup(sgRule.getSecurityGroupId(), newSg);
+            }
-            if (oldSg == null) {
+            if (sg.getRules().stream().anyMatch(rule -> Objects.equals(rule.getId(), sgRule.getId()))) {
-                log.warn("Failed to add the security group rule {} to security group", sgRule.getId());
+                final String error = String.format(MSG_SG_RULE,
+                        sgRule.getSecurityGroupId(), ERR_DUPLICATE);
+                throw new IllegalStateException(error);
             }
 
-            osSecurityGroupStore.createSecurityGroupRule(sgRule);
+            // FIXME we cannot add element to extend list
-            log.info(String.format(MSG_SG_RULE, sgRule.getId(), MSG_CREATED));
+            List updatedSgRules = sg.getRules();
+            updatedSgRules.add(sgRule);
+            SecurityGroup updatedSg = NeutronSecurityGroup.builder().from(sg).build();
+            osSecurityGroupStore.updateSecurityGroup(updatedSg);
         }
+
+        log.info(String.format(MSG_SG_RULE, sgRule.getId(), MSG_CREATED));
     }
 
     @Override
     public void removeSecurityGroupRule(String sgRuleId) {
-        checkNotNull(sgRuleId, ERR_NULL_SG_RULE_ID);
+        checkArgument(!Strings.isNullOrEmpty(sgRuleId), ERR_NULL_SG_RULE_ID);
+
+        synchronized (this) {
+            SecurityGroupRule sgRule = securityGroupRule(sgRuleId);
+            if (sgRule == null) {
+                final String error = String.format(MSG_SG_RULE, sgRuleId, ERR_NOT_FOUND);
+                throw new IllegalStateException(error);
+            }
+
+            SecurityGroup sg = securityGroup(sgRule.getSecurityGroupId());
+            if (sg == null) {
+                final String error = String.format(MSG_SG, sgRule.getSecurityGroupId(), ERR_NOT_FOUND);
+                throw new IllegalStateException(error);
+            }
+
+            if (sg.getRules().stream().noneMatch(rule -> Objects.equals(rule.getId(), sgRule.getId()))) {
+                final String error = String.format(MSG_SG_RULE,
+                        sgRule.getSecurityGroupId(), ERR_NOT_FOUND);
+                throw new IllegalStateException(error);
+            }
+
+            // FIXME we cannot handle the element of extend list as a specific class object
+            List updatedSgRules = sg.getRules();
+            updatedSgRules.removeIf(r -> ((SecurityGroupRule) r).getId().equals(sgRuleId));
+            SecurityGroup updatedSg = NeutronSecurityGroup.builder().from(sg).build();
+            osSecurityGroupStore.updateSecurityGroup(updatedSg);
+        }
 
-        osSecurityGroupStore.removeSecurityGroupRule(sgRuleId);
         log.info(String.format(MSG_SG_RULE, sgRuleId, MSG_REMOVED));
     }
 
+    @Override
+    public Set<SecurityGroup> securityGroups() {
+        return osSecurityGroupStore.securityGroups();
+    }
+
     @Override
     public SecurityGroup securityGroup(String sgId) {
         checkArgument(!Strings.isNullOrEmpty(sgId), ERR_NULL_SG_ID);
@@ -141,9 +189,15 @@ public class OpenstackSecurityGroupManager
     }
 
     @Override
-    public SecurityGroupRule securityGroupRule(String sgRuleId) {
+    public void clear() {
-        checkArgument(!Strings.isNullOrEmpty(sgRuleId), ERR_NULL_SG_RULE_ID);
+        osSecurityGroupStore.clear();
-        return osSecurityGroupStore.securityGroupRule(sgRuleId);
+    }
+
+    private SecurityGroupRule securityGroupRule(String sgRuleId) {
+        return osSecurityGroupStore.securityGroups().stream()
+                .flatMap(sg -> sg.getRules().stream())
+                .filter(sgRule -> Objects.equals(sgRule.getId(), sgRuleId))
+                .findFirst().orElse(null);
     }
 
     private class InternalSecurityGroupStoreDelegate implements OpenstackSecurityGroupStoreDelegate {

apps/openstacknetworking/src/main/resources/OSGI-INF/blueprint/shell-config.xml

@@ -33,5 +33,8 @@
         <command>
             <action class="org.onosproject.openstacknetworking.cli.OpenstackSyncStateCommand"/>
         </command>
+        <command>
+            <action class="org.onosproject.openstacknetworking.cli.OpenstackSecurityGroupListCommand"/>
+        </command>
     </command-bundle>
 </blueprint>

apps/openstacknode/src/main/java/org/onosproject/openstacknode/OpenstackNodeManager.java

@@ -438,10 +438,7 @@ public final class OpenstackNodeManager extends ListenerRegistry<OpenstackNodeEv
     }
 
     private void setNodeState(OpenstackNode node, NodeState newState) {
-        if (node.state() != newState) {
+        nodeStore.put(node.hostname(), OpenstackNode.getUpdatedNode(node, newState));
-            log.debug("Changed {} state: {}", node.hostname(), newState);
-            nodeStore.put(node.hostname(), OpenstackNode.getUpdatedNode(node, newState));
-        }
     }
 
     private NodeState nodeState(OpenstackNode node) {