Merge lp:~soren/nova/iptables-security-groups into lp:~hudson-openstack/nova/trunk

[Soren Hansen]

Add a new method to firewall drivers to tell them to stop filtering a particular instance. Call it when an instance has been destroyed.

Use dict()s (keyed off id's) instead of set()s for keeping track of instances and security groups in the iptables firewall driver. __eq__ for objects from sqlalchemy fetched in different sessions doesn't work as expected, so I needed to explicitly filter on ID.

[Thierry Carrez (ttx)]

Fixes the bug alright.

Why the changes in version.py, though ?

L79: Attempted to untiler --> you mean unfilter ?

review: Needs Fixing

[Soren Hansen (soren)]

Gah, they werent' supposed to be included in the patch. I just did it locally because the million char wide log lines p'ed me off.

[lp:~soren/nova/iptables-security-groups]

459. By Soren Hansen on 2011-01-13

Revert changes to version.py

[Thierry Carrez (ttx)]

You missed this remark:
L79: Attempted to untiler --> you mean unfilter ?

[lp:~soren/nova/iptables-security-groups]

460. By Soren Hansen on 2011-01-13

Spelling is hard. Typing even moreso.

[Eric Day (eday)]

lgtm

review: Approve

[Vish Ishaya (vishvananda)]

lgtm

review: Approve

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-01-13 01:25:08 +0000
+++ nova/virt/libvirt_conn.py	2011-01-13 17:11:31 +0000
@@ -214,6 +214,8 @@
                                       power_state.SHUTDOWN)
                 break
 
+        self.firewall_driver.unfilter_instance(instance)
+
         if cleanup:
             self._cleanup(instance)
 
@@ -774,6 +776,10 @@
         At this point, the instance isn't running yet."""
         raise NotImplementedError()
 
+    def unfilter_instance(self, instance):
+        """Stop filtering instance"""
+        raise NotImplementedError()
+
     def apply_instance_filter(self, instance):
         """Apply instance filter.
 
@@ -964,6 +970,10 @@
         # execute in a native thread and block current greenthread until done
         tpool.execute(self._conn.nwfilterDefineXML, xml)
 
+    def unfilter_instance(self, instance):
+        # Nothing to do
+        pass
+
     def prepare_instance_filter(self, instance):
         """
         Creates an NWFilter for the given instance. In the process,
@@ -1045,17 +1055,25 @@
 class IptablesFirewallDriver(FirewallDriver):
     def __init__(self, execute=None):
         self.execute = execute or utils.execute
-        self.instances = set()
+        self.instances = {}
 
     def apply_instance_filter(self, instance):
         """No-op. Everything is done in prepare_instance_filter"""
         pass
 
     def remove_instance(self, instance):
-        self.instances.remove(instance)
+        if instance['id'] in self.instances:
+            del self.instances[instance['id']]
+        else:
+            LOG.info(_('Attempted to unfilter instance %s which is not '
+                       'filtered'), instance['id'])
 
     def add_instance(self, instance):
-        self.instances.add(instance)
+        self.instances[instance['id']] = instance
+
+    def unfilter_instance(self, instance):
+        self.remove_instance(instance)
+        self.apply_ruleset()
 
     def prepare_instance_filter(self, instance):
         self.add_instance(instance)
@@ -1088,10 +1106,11 @@
         our_chains += [':nova-local - [0:0]']
         our_rules += ['-A FORWARD -j nova-local']
 
-        security_groups = set()
+        security_groups = {}
         # Add our chains
         # First, we add instance chains and rules
-        for instance in self.instances:
+        for instance_id in self.instances:
+            instance = self.instances[instance_id]
             chain_name = self._instance_chain_name(instance)
             ip_address = self._ip_for_instance(instance)
 
@@ -1113,9 +1132,10 @@
             for security_group in \
                             db.security_group_get_by_instance(ctxt,
                                                               instance['id']):
-                security_groups.add(security_group)
+                security_groups[security_group['id']] = security_group
 
-                sg_chain_name = self._security_group_chain_name(security_group)
+                sg_chain_name = self._security_group_chain_name(
+                                                          security_group['id'])
 
                 our_rules += ['-A %s -j %s' % (chain_name, sg_chain_name)]
 
@@ -1128,13 +1148,13 @@
             our_rules += ['-A %s -j nova-ipv4-fallback' % (chain_name,)]
 
         # then, security group chains and rules
-        for security_group in security_groups:
-            chain_name = self._security_group_chain_name(security_group)
+        for security_group_id in security_groups:
+            chain_name = self._security_group_chain_name(security_group_id)
             our_chains += [':%s - [0:0]' % chain_name]
 
             rules = \
               db.security_group_rule_get_by_security_group(ctxt,
-                                                          security_group['id'])
+                                                          security_group_id)
 
             for rule in rules:
                 logging.info('%r', rule)
@@ -1182,8 +1202,8 @@
     def refresh_security_group_rules(self, security_group):
         self.apply_ruleset()
 
-    def _security_group_chain_name(self, security_group):
-        return 'nova-sg-%s' % (security_group['id'],)
+    def _security_group_chain_name(self, security_group_id):
+        return 'nova-sg-%s' % (security_group_id,)
 
     def _instance_chain_name(self, instance):
         return 'nova-inst-%s' % (instance['id'],)