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

[Soren Hansen]

Add a new firewall backend for libvirt, based on iptables.

[Soren Hansen (soren)]

Oh, I should mention that granting access to another security group (i.e. saying that instances that are a member of security group A are allowed to access ports Y-Z on security group W) doesn't work right now. It's not super hard, but I need to work a bit on the kernel support for it.

[Jay Pipes (jaypipes)]

I really, really like this work. Needs to be merged with trunk and ported for eventlet. Other than that, I think it's great stuff.

review: Needs Fixing

[Jay Pipes (jaypipes)]

Coolness :) Great work, Soren.

review: Approve

[Vish Ishaya (vishvananda)]

Minor issue:

171 options(joinedload('security_groups')).\
172 + options(joinedload_all('security_groups.rules')).\

joinedload_all will load all of the objects in the chain so lines 171 and 179 are redundant and can be removed.

Also, have you tested with --use_nova_chains as well?

Finally, do you think that the we be iptables security groups is strictly superior to nwfilter security groups? Should we continue to support the other driver or deprecate it?

review: Needs Fixing

[Eric Day (eday)]

48: line is not needed
69, 91: should use self.db.<...>, not just db.<...> for consistency

review: Needs Fixing

[Soren Hansen (soren)]

> Minor issue:
>
> 171 options(joinedload('security_groups')).\
> 172 +
> options(joinedload_all('security_groups.rules')).\
>
> joinedload_all will load all of the objects in the chain so lines 171 and 179
> are redundant and can be removed.

Good call. Fixed.

> Also, have you tested with --use_nova_chains as well?

Uh.. No. Not at all. I've completely missed that. :( I'll do so and set this mp to WiP.

> Finally, do you think that the we be iptables security groups is strictly
> superior to nwfilter security groups?

Right now, yes. No doubt. Bug 659135 basically makes nwfilter non functional. NWFilter has other problems as well (its attempts to avoid being racy are somewhat misguided, for instance).

> Should we continue to support the other driver or deprecate it?

Temporary deprecation might make sense, yeah. I know I won't be using it and I wouldn't recommend it to anyone.

[Soren Hansen (soren)]

> 48: line is not needed

Fixed.

> 69, 91: should use self.db.<...>, not just db.<...> for consistency

Fixed, although I have to admit that it's a bit of a mystery to me what purpose the db instance attribute serves.

[Soren Hansen (soren)]

> Also, have you tested with --use_nova_chains as well?

I'm having trouble understanding that code, to be honest.

If use_nova_chains is enabled (I don't understand why it's configurable?), we do this:
    At the end of FLAGS.input_chain (which I can't see getting DEFINE'd anywhere?) we add a nova_input chain.
    At the end of FORWARD, we add a nova_forward chain.
    At the end of OUTPUT, we add a nova_output chain.
    At the end of nat:PREROUTING, we add nova_prerouting.
    At the end of nat:POSTROUTING, we add nova_postrouting and nova_snatting (in that order).
    At the end of nat:OUTPUT, we add nova_output.

If use_nova_chains isn't enabled, we add a new chain, called SNATTING, to the end of nat:POSTROUTING.

Regardless of the use_nova_chains setting, we now add a rule to the SNATTING chain, even though AFAICT it only exists if use_nova_chains is False?

In nova.network.linux_net.ensure_bridge:

If use_nova_chains is enabled, we create nova_forward (again?) and add it to the end of FORWARD.

We then prepend the ACCEPT rules for the bridge to the FORWARD chain (rather than the nova_forward chain?).

I'm at a bit of a loss as to where to integrate my stuff. What I need is a place to put some forwarding rules that get applied before the ACCEPT rules for the bridge. I could simply just leave it where it is in the current patch (which is at the very top of the FORWARD chain), and that would totally work with use_nova_chains as far as I can see, but I'm not sure if that's what is indended in the use_nova_chains paradigm?

[Vish Ishaya (vishvananda)]

Some discussion inline.

On Jan 7, 2011, at 4:31 AM, Soren Hansen wrote:

>> Also, have you tested with --use_nova_chains as well?
>
> I'm having trouble understanding that code, to be honest.
>
> If use_nova_chains is enabled (I don't understand why it's configurable?), we do this:
> At the end of FLAGS.input_chain (which I can't see getting DEFINE'd anywhere?) we add a nova_input chain.
> At the end of FORWARD, we add a nova_forward chain.
> At the end of OUTPUT, we add a nova_output chain.
> At the end of nat:PREROUTING, we add nova_prerouting.
> At the end of nat:POSTROUTING, we add nova_postrouting and nova_snatting (in that order).
> At the end of nat:OUTPUT, we add nova_output.
>
> If use_nova_chains isn't enabled, we add a new chain, called SNATTING, to the end of nat:POSTROUTING.

splitting postrouting into two chains allows us to ensure the right ordering for the postrouting rules.

>
> Regardless of the use_nova_chains setting, we now add a rule to the SNATTING chain, even though AFAICT it only exists if use_nova_chains is False?

if nova chains is true, the rule will be rewritten in confirm_rule and it will be added to nova_snatting instead

>
> In nova.network.linux_net.ensure_bridge:
>
> If use_nova_chains is enabled, we create nova_forward (again?) and add it to the end of FORWARD.

It creates nova_forward if it hasn't been created yet

>
> We then prepend the ACCEPT rules for the bridge to the FORWARD chain (rather than the nova_forward chain?).

once again the rule will go into the nova_forward chain

>
> I'm at a bit of a loss as to where to integrate my stuff. What I need is a place to put some forwarding rules that get applied before the ACCEPT rules for the bridge. I could simply just leave it where it is in the current patch (which is at the very top of the FORWARD chain), and that would totally work with use_nova_chains as far as I can see, but I'm not sure if that's what is indended in the use_nova_chains paradigm?

The idea here is that all of the rules go into nova specific chains instead of the default chains so that sysadmins can add and remove rules by dealing with the shorter default chains. It is mostly for so that the chains remain relatively clean. and that all of the rules added by nova are in one place so that they are easy to flush/remove if necessary. For example, we tend to have a pretty locked down set of rules, so we have custom exceptions for a bunch of non-nova services. There have been many cases where there are extraneous due to a misconfigured flag or some such. Once the flag is changed, the iptables -D doesn't delete the old rule. With use nova_chains you can just flush the nova_xxxx chain and restart the network worker instead of manually removing bad rules from the input chain.

I don't know that your code needs to follow the same pattern, although it could. I just wanted to make sure it wouldn't break with use_nova_chains, since we use it. If all of your rules are in forward, you could have a conditional based on --use_nova_chains that ensures the chain exists and then adds them to nova-forward instead. Or you could have a specific new nova_xxxxx chain before n...

[Vish Ishaya (vishvananda)]

On Jan 7, 2011, at 4:31 AM, Soren Hansen wrote:

>> Also, have you tested with --use_nova_chains as well?
>
> I'm having trouble understanding that code, to be honest.
>
> If use_nova_chains is enabled (I don't understand why it's configurable?), we do this:

Configurable flag is a legacy holdover. It was initially there because the chains were being created by a separate script in tools, so I wanted it to work in dev mode without running the script. I think it is strictly cleaner with use_nova_chains set to true, so perhaps I should submit a patch that removes the flag. The rewriting of confirm_rule will look a bit strange without the flag though :).

Vish

[Soren Hansen (soren)]

2011/1/7 Vish Ishaya <email address hidden>:
>>> Also, have you tested with --use_nova_chains as well?
>> I'm having trouble understanding that code, to be honest.
>>
>> If use_nova_chains is enabled (I don't understand why it's
>> configurable?), we do this:
>>    At the end of FLAGS.input_chain (which I can't see getting DEFINE'd anywhere?) we add a nova_input chain.
>>    At the end of FORWARD, we add a nova_forward chain.
>>    At the end of OUTPUT, we add a nova_output chain.
>>    At the end of nat:PREROUTING, we add nova_prerouting.
>>    At the end of nat:POSTROUTING, we add nova_postrouting and nova_snatting (in that order).
>>    At the end of nat:OUTPUT, we add nova_output.
>>
>> If use_nova_chains isn't enabled, we add a new chain, called SNATTING, to the end of nat:POSTROUTING.
> splitting postrouting into two chains allows us to ensure the right ordering for the postrouting rules.

Ok. I can relate to that. I just don't understand why that split is
conditional then? That feeds back into my "why is this configurable?"
question, I guess:)

>> Regardless of the use_nova_chains setting, we now add a rule to the SNATTING chain, even though AFAICT it only exists if use_nova_chains is False?
> if nova chains is true, the rule will be rewritten in confirm_rule and it will be added to nova_snatting instead

Oh, ok! I completely missed the change to confirm_rule.

>> In nova.network.linux_net.ensure_bridge:
>> If use_nova_chains is enabled, we create nova_forward (again?) and add it to the end of FORWARD.
> It creates nova_forward if it hasn't been created yet

I just didn't see when that would be the case, but ok :)

>> We then prepend the ACCEPT rules for the bridge to the FORWARD chain (rather than the nova_forward chain?).
> once again the rule will go into the nova_forward chain

Right, right, because of _confirm_rule. I completely missed that.

>> I'm at a bit of a loss as to where to integrate my stuff. What I need
>> is a place to put some forwarding rules that get applied before the
>> ACCEPT rules for the bridge. I could simply just leave it where it is
>> in the current patch (which is at the very top of the FORWARD chain),
>> and that would totally work with use_nova_chains as far as I can see,
>> but I'm not sure if that's what is indended in the use_nova_chains
>> paradigm?
> The idea here is that all of the rules go into nova specific chains
> instead of the default chains so that sysadmins can add and remove
> rules by dealing with the shorter default chains. It is mostly for so
> that the chains remain relatively clean.

Right, I understand the motivation behind it. I'm following sort of the
same pattern in my stuff where everything is packaged away in separate
chains, all named "nova-<something>".

I think consolidating our iptables stuff is a good idea. However, IMHO
my approach to iptables handling is better :) iptables (the command line
tool, not subsystem) is racy and unnecessarily costly. Each call to
iptables makes it load the entire table into memory, make the change,
and put it back. In the mean time, it could have been altered by
another invocation of iptables. iptables-save and iptables-restore are
perfectly...

[Vish Ishaya (vishvananda)]

Agreed on the cactus refactor. Looks like it is working for now. so we should merge this.

Vish

On Jan 7, 2011, at 1:46 PM, Soren Hansen wrote:

> 2011/1/7 Vish Ishaya <email address hidden>:
>>>> Also, have you tested with --use_nova_chains as well?
>>> I'm having trouble understanding that code, to be honest.
>>>
>>> If use_nova_chains is enabled (I don't understand why it's
>>> configurable?), we do this:
>>> At the end of FLAGS.input_chain (which I can't see getting DEFINE'd anywhere?) we add a nova_input chain.
>>> At the end of FORWARD, we add a nova_forward chain.
>>> At the end of OUTPUT, we add a nova_output chain.
>>> At the end of nat:PREROUTING, we add nova_prerouting.
>>> At the end of nat:POSTROUTING, we add nova_postrouting and nova_snatting (in that order).
>>> At the end of nat:OUTPUT, we add nova_output.
>>>
>>> If use_nova_chains isn't enabled, we add a new chain, called SNATTING, to the end of nat:POSTROUTING.
>> splitting postrouting into two chains allows us to ensure the right ordering for the postrouting rules.
>
> Ok. I can relate to that. I just don't understand why that split is
> conditional then? That feeds back into my "why is this configurable?"
> question, I guess:)
>
>>> Regardless of the use_nova_chains setting, we now add a rule to the SNATTING chain, even though AFAICT it only exists if use_nova_chains is False?
>> if nova chains is true, the rule will be rewritten in confirm_rule and it will be added to nova_snatting instead
>
> Oh, ok! I completely missed the change to confirm_rule.
>
>>> In nova.network.linux_net.ensure_bridge:
>>> If use_nova_chains is enabled, we create nova_forward (again?) and add it to the end of FORWARD.
>> It creates nova_forward if it hasn't been created yet
>
> I just didn't see when that would be the case, but ok :)
>
>>> We then prepend the ACCEPT rules for the bridge to the FORWARD chain (rather than the nova_forward chain?).
>> once again the rule will go into the nova_forward chain
>
> Right, right, because of _confirm_rule. I completely missed that.
>
>>> I'm at a bit of a loss as to where to integrate my stuff. What I need
>>> is a place to put some forwarding rules that get applied before the
>>> ACCEPT rules for the bridge. I could simply just leave it where it is
>>> in the current patch (which is at the very top of the FORWARD chain),
>>> and that would totally work with use_nova_chains as far as I can see,
>>> but I'm not sure if that's what is indended in the use_nova_chains
>>> paradigm?
>> The idea here is that all of the rules go into nova specific chains
>> instead of the default chains so that sysadmins can add and remove
>> rules by dealing with the shorter default chains. It is mostly for so
>> that the chains remain relatively clean.
>
> Right, I understand the motivation behind it. I'm following sort of the
> same pattern in my stuff where everything is packaged away in separate
> chains, all named "nova-<something>".
>
> I think consolidating our iptables stuff is a good idea. However, IMHO
> my approach to iptables handling is better :) iptables (the command line
> tool, not subsystem) is racy and unnecessarily costly. Eac...

[Vish Ishaya (vishvananda)]

review: Approve

[Eric Day (eday)]

lgtm

review: Approve

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in nova/virt/libvirt_conn.py

[Vish Ishaya (vishvananda)]

Looks like you need another merge

[Soren Hansen (soren)]

Done.

[OpenStack Infra (hudson-openstack)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Jay Pipes (jaypipes)]

let's try again :)

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~soren/nova/iptables-security-groups into lp:nova failed. Below is the output from the failed tests.

TrialTestCase
    runTest ok
AdminAPITest
    test_admin_disabled ok
    test_admin_enabled ok
APITest
    test_exceptions_are_converted_to_faults ok
Test
    test_authorize_token ok
    test_authorize_user ok
    test_bad_token ok
    test_bad_user ok
    test_no_user ok
    test_token_expiry ok
TestLimiter
    test_authorize_token ok
TestFaults
    test_fault_parts ok
    test_raise ok
    test_retry_header ok
FlavorsTest
    test_get_flavor_by_id ok
    test_get_flavor_list ok
GlanceImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
ImageControllerWithGlanceServiceTest
    test_get_image_details ok
    test_get_image_index ok
LocalImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
LimiterTest
    test_minute ok
    test_one_per_period ok
    test_second ok
    test_users_get_separate_buckets ok
    test_we_can_go_indefinitely_if_we_spread_out_requests ok
WSGIAppProxyTest
    test_200 ok
    test_403 ok
    test_failure ok
WSGIAppTest
    test_escaping ok
    test_good_urls ok
    test_invalid_methods ok
    test_invalid_urls ok
    test_response_to_delays ok
ServersTest
    test_create_backup_schedules ok
    test_create_instance ok
    test_delete_backup_schedules ok
    test_delete_server_instance ...

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~soren/nova/iptables-security-groups into lp:nova failed. Below is the output from the failed tests.

TrialTestCase
    runTest ok
AdminAPITest
    test_admin_disabled ok
    test_admin_enabled ok
APITest
    test_exceptions_are_converted_to_faults ok
Test
    test_authorize_token ok
    test_authorize_user ok
    test_bad_token ok
    test_bad_user ok
    test_no_user ok
    test_token_expiry ok
TestLimiter
    test_authorize_token ok
TestFaults
    test_fault_parts ok
    test_raise ok
    test_retry_header ok
FlavorsTest
    test_get_flavor_by_id ok
    test_get_flavor_list ok
GlanceImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
ImageControllerWithGlanceServiceTest
    test_get_image_details ok
    test_get_image_index ok
LocalImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
LimiterTest
    test_minute ok
    test_one_per_period ok
    test_second ok
    test_users_get_separate_buckets ok
    test_we_can_go_indefinitely_if_we_spread_out_requests ok
WSGIAppProxyTest
    test_200 ok
    test_403 ok
    test_failure ok
WSGIAppTest
    test_escaping ok
    test_good_urls ok
    test_invalid_methods ok
    test_invalid_urls ok
    test_response_to_delays ok
ServersTest
    test_create_backup_schedules ok
    test_create_instance ok
    test_delete_backup_schedules ok
    test_delete_server_instance ...

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

452. By Soren Hansen on 2011-01-10

Create LibvirtConnection directly, rather than going through libvirt_conn.get_connection. This should remove the dependency on libvirt for tests.

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-01-07 23:22:52 +0000
+++ nova/api/ec2/cloud.py	2011-01-10 10:38:10 +0000
@@ -133,15 +133,6 @@
                     result[key] = [line]
         return result
 
-    def _trigger_refresh_security_group(self, context, security_group):
-        nodes = set([instance['host'] for instance in security_group.instances
-                       if instance['host'] is not None])
-        for node in nodes:
-            rpc.cast(context,
-                     '%s.%s' % (FLAGS.compute_topic, node),
-                     {"method": "refresh_security_group",
-                      "args": {"security_group_id": security_group.id}})
-
     def get_metadata(self, address):
         ctxt = context.get_admin_context()
         instance_ref = self.compute_api.get_all(ctxt, fixed_ip=address)
@@ -419,7 +410,8 @@
                     match = False
             if match:
                 db.security_group_rule_destroy(context, rule['id'])
-                self._trigger_refresh_security_group(context, security_group)
+                self.compute_api.trigger_security_group_rules_refresh(context,
+                                                          security_group['id'])
                 return True
         raise exception.ApiError(_("No rule for the specified parameters."))
 
@@ -444,7 +436,8 @@
 
         security_group_rule = db.security_group_rule_create(context, values)
 
-        self._trigger_refresh_security_group(context, security_group)
+        self.compute_api.trigger_security_group_rules_refresh(context,
+                                                          security_group['id'])
 
         return True
 

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-01-07 14:46:17 +0000
+++ nova/compute/api.py	2011-01-10 10:38:10 +0000
@@ -185,6 +185,9 @@
                       "args": {"topic": FLAGS.compute_topic,
                                "instance_id": instance_id}})
 
+        for group_id in security_groups:
+            self.trigger_security_group_members_refresh(elevated, group_id)
+
         return instances
 
     def ensure_default_security_group(self, context):
@@ -204,6 +207,60 @@
                       'project_id': context.project_id}
             db.security_group_create(context, values)
 
+    def trigger_security_group_rules_refresh(self, context, security_group_id):
+        """Called when a rule is added to or removed from a security_group"""
+
+        security_group = self.db.security_group_get(context, security_group_id)
+
+        hosts = set()
+        for instance in security_group['instances']:
+            if instance['host'] is not None:
+                hosts.add(instance['host'])
+
+        for host in hosts:
+            rpc.cast(context,
+                     self.db.queue_get_for(context, FLAGS.compute_topic, host),
+                     {"method": "refresh_security_group_rules",
+                      "args": {"security_group_id": security_group.id}})
+
+    def trigger_security_group_members_refresh(self, context, group_id):
+        """Called when a security group gains a new or loses a member
+
+        Sends an update request to each compute node for whom this is
+        relevant."""
+
+        # First, we get the security group rules that reference this group as
+        # the grantee..
+        security_group_rules = \
+                self.db.security_group_rule_get_by_security_group_grantee(
+                                                                     context,
+                                                                     group_id)
+
+        # ..then we distill the security groups to which they belong..
+        security_groups = set()
+        for rule in security_group_rules:
+            security_groups.add(rule['parent_group_id'])
+
+        # ..then we find the instances that are members of these groups..
+        instances = set()
+        for security_group in security_groups:
+            for instance in security_group['instances']:
+                instances.add(instance['id'])
+
+        # ...then we find the hosts where they live...
+        hosts = set()
+        for instance in instances:
+            if instance['host']:
+                hosts.add(instance['host'])
+
+        # ...and finally we tell these nodes to refresh their view of this
+        # particular security group.
+        for host in hosts:
+            rpc.cast(context,
+                     self.db.queue_get_for(context, FLAGS.compute_topic, host),
+                     {"method": "refresh_security_group_members",
+                      "args": {"security_group_id": group_id}})
+
     def update(self, context, instance_id, **kwargs):
         """Updates the instance in the datastore.
 

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-01-07 14:46:17 +0000
+++ nova/compute/manager.py	2011-01-10 10:38:10 +0000
@@ -137,9 +137,16 @@
                                      host)
 
     @exception.wrap_exception
-    def refresh_security_group(self, context, security_group_id, **_kwargs):
-        """This call passes stright through to the virtualization driver."""
-        self.driver.refresh_security_group(security_group_id)
+    def refresh_security_group_rules(self, context,
+                                     security_group_id, **_kwargs):
+        """This call passes straight through to the virtualization driver."""
+        return self.driver.refresh_security_group_rules(security_group_id)
+
+    @exception.wrap_exception
+    def refresh_security_group_members(self, context,
+                                       security_group_id, **_kwargs):
+        """This call passes straight through to the virtualization driver."""
+        return self.driver.refresh_security_group_members(security_group_id)
 
     @exception.wrap_exception
     def run_instance(self, context, instance_id, **_kwargs):

=== modified file 'nova/db/api.py'
--- nova/db/api.py	2011-01-04 17:07:09 +0000
+++ nova/db/api.py	2011-01-10 10:38:10 +0000
@@ -772,6 +772,13 @@
                                                           security_group_id)
 
 
+def security_group_rule_get_by_security_group_grantee(context,
+                                                      security_group_id):
+    """Get all rules that grant access to the given security group."""
+    return IMPL.security_group_rule_get_by_security_group_grantee(context,
+                                                             security_group_id)
+
+
 def security_group_rule_destroy(context, security_group_rule_id):
     """Deletes a security group rule."""
     return IMPL.security_group_rule_destroy(context, security_group_rule_id)

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-01-07 01:19:22 +0000
+++ nova/db/sqlalchemy/api.py	2011-01-10 10:38:10 +0000
@@ -650,7 +650,7 @@
     if is_admin_context(context):
         result = session.query(models.Instance).\
                          options(joinedload_all('fixed_ip.floating_ips')).\
-                         options(joinedload('security_groups')).\
+                         options(joinedload_all('security_groups.rules')).\
                          options(joinedload('volumes')).\
                          filter_by(id=instance_id).\
                          filter_by(deleted=can_read_deleted(context)).\
@@ -658,7 +658,7 @@
     elif is_user_context(context):
         result = session.query(models.Instance).\
                          options(joinedload_all('fixed_ip.floating_ips')).\
-                         options(joinedload('security_groups')).\
+                         options(joinedload_all('security_groups.rules')).\
                          options(joinedload('volumes')).\
                          filter_by(project_id=context.project_id).\
                          filter_by(id=instance_id).\
@@ -1579,6 +1579,44 @@
 
 
 @require_context
+def security_group_rule_get_by_security_group(context, security_group_id,
+                                              session=None):
+    if not session:
+        session = get_session()
+    if is_admin_context(context):
+        result = session.query(models.SecurityGroupIngressRule).\
+                         filter_by(deleted=can_read_deleted(context)).\
+                         filter_by(parent_group_id=security_group_id).\
+                         all()
+    else:
+        # TODO(vish): Join to group and check for project_id
+        result = session.query(models.SecurityGroupIngressRule).\
+                         filter_by(deleted=False).\
+                         filter_by(parent_group_id=security_group_id).\
+                         all()
+    return result
+
+
+@require_context
+def security_group_rule_get_by_security_group_grantee(context,
+                                                      security_group_id,
+                                                      session=None):
+    if not session:
+        session = get_session()
+    if is_admin_context(context):
+        result = session.query(models.SecurityGroupIngressRule).\
+                         filter_by(deleted=can_read_deleted(context)).\
+                         filter_by(group_id=security_group_id).\
+                         all()
+    else:
+        result = session.query(models.SecurityGroupIngressRule).\
+                         filter_by(deleted=False).\
+                         filter_by(group_id=security_group_id).\
+                         all()
+    return result
+
+
+@require_context
 def security_group_rule_create(context, values):
     security_group_rule_ref = models.SecurityGroupIngressRule()
     security_group_rule_ref.update(values)

=== modified file 'nova/network/linux_net.py'
--- nova/network/linux_net.py	2011-01-04 05:23:35 +0000
+++ nova/network/linux_net.py	2011-01-10 10:38:10 +0000
@@ -209,6 +209,8 @@
 
     _confirm_rule("FORWARD", "--in-interface %s -j ACCEPT" % bridge)
     _confirm_rule("FORWARD", "--out-interface %s -j ACCEPT" % bridge)
+    _execute("sudo iptables -N nova-local", check_exit_code=False)
+    _confirm_rule("FORWARD", "-j nova-local")
 
 
 def get_dhcp_hosts(context, network_id):

=== modified file 'nova/tests/test_virt.py'
--- nova/tests/test_virt.py	2010-12-28 01:37:04 +0000
+++ nova/tests/test_virt.py	2011-01-10 10:38:10 +0000
@@ -208,8 +208,99 @@
         self.manager.delete_user(self.user)
 
 
+class IptablesFirewallTestCase(test.TestCase):
+    def setUp(self):
+        super(IptablesFirewallTestCase, self).setUp()
+
+        self.manager = manager.AuthManager()
+        self.user = self.manager.create_user('fake', 'fake', 'fake',
+                                             admin=True)
+        self.project = self.manager.create_project('fake', 'fake', 'fake')
+        self.context = context.RequestContext('fake', 'fake')
+        self.network = utils.import_object(FLAGS.network_manager)
+        self.fw = libvirt_conn.IptablesFirewallDriver()
+
+    def tearDown(self):
+        self.manager.delete_project(self.project)
+        self.manager.delete_user(self.user)
+        super(IptablesFirewallTestCase, self).tearDown()
+
+    def _p(self, *args, **kwargs):
+        if 'iptables-restore' in args:
+            print ' '.join(args), kwargs['stdin']
+        if 'iptables-save' in args:
+            return
+
+    in_rules = [
+      '# Generated by iptables-save v1.4.4 on Mon Dec  6 11:54:13 2010',
+      '*filter',
+      ':INPUT ACCEPT [969615:281627771]',
+      ':FORWARD ACCEPT [0:0]',
+      ':OUTPUT ACCEPT [915599:63811649]',
+      ':nova-block-ipv4 - [0:0]',
+      '-A INPUT -i virbr0 -p udp -m udp --dport 53 -j ACCEPT ',
+      '-A INPUT -i virbr0 -p tcp -m tcp --dport 53 -j ACCEPT ',
+      '-A INPUT -i virbr0 -p udp -m udp --dport 67 -j ACCEPT ',
+      '-A INPUT -i virbr0 -p tcp -m tcp --dport 67 -j ACCEPT ',
+      '-A FORWARD -d 192.168.122.0/24 -o virbr0 -m state --state RELATED'
+      ',ESTABLISHED -j ACCEPT ',
+      '-A FORWARD -s 192.168.122.0/24 -i virbr0 -j ACCEPT ',
+      '-A FORWARD -i virbr0 -o virbr0 -j ACCEPT ',
+      '-A FORWARD -o virbr0 -j REJECT --reject-with icmp-port-unreachable ',
+      '-A FORWARD -i virbr0 -j REJECT --reject-with icmp-port-unreachable ',
+      'COMMIT',
+      '# Completed on Mon Dec  6 11:54:13 2010'
+    ]
+
+    def test_static_filters(self):
+        self.fw.execute = self._p
+        instance_ref = db.instance_create(self.context,
+                                          {'user_id': 'fake',
+                                          'project_id': 'fake'})
+        ip = '10.11.12.13'
+
+        network_ref = db.project_get_network(self.context,
+                                             'fake')
+
+        fixed_ip = {'address': ip,
+                    'network_id': network_ref['id']}
+
+        admin_ctxt = context.get_admin_context()
+        db.fixed_ip_create(admin_ctxt, fixed_ip)
+        db.fixed_ip_update(admin_ctxt, ip, {'allocated': True,
+                                            'instance_id': instance_ref['id']})
+
+        secgroup = db.security_group_create(admin_ctxt,
+                                            {'user_id': 'fake',
+                                             'project_id': 'fake',
+                                             'name': 'testgroup',
+                                             'description': 'test group'})
+
+        db.security_group_rule_create(admin_ctxt,
+                                      {'parent_group_id': secgroup['id'],
+                                       'protocol': 'tcp',
+                                       'from_port': 80,
+                                       'to_port': 81,
+                                       'cidr': '192.168.10.0/24'})
+
+        db.instance_add_security_group(admin_ctxt, instance_ref['id'],
+                                       secgroup['id'])
+        instance_ref = db.instance_get(admin_ctxt, instance_ref['id'])
+
+        self.fw.add_instance(instance_ref)
+
+        out_rules = self.fw.modify_rules(self.in_rules)
+
+        in_rules = filter(lambda l: not l.startswith('#'), self.in_rules)
+        for rule in in_rules:
+            if not 'nova' in rule:
+                self.assertTrue(rule in out_rules,
+                                'Rule went missing: %s' % rule)
+
+        print '\n'.join(out_rules)
+
+
 class NWFilterTestCase(test.TestCase):
-
     def setUp(self):
         super(NWFilterTestCase, self).setUp()
 
@@ -224,7 +315,8 @@
 
         self.fake_libvirt_connection = Mock()
 
-        self.fw = libvirt_conn.NWFilterFirewall(self.fake_libvirt_connection)
+        self.fw = libvirt_conn.NWFilterFirewall(
+                                         lambda: self.fake_libvirt_connection)
 
     def tearDown(self):
         self.manager.delete_project(self.project)
@@ -337,7 +429,7 @@
                                        self.security_group.id)
         instance = db.instance_get(self.context, inst_id)
 
-        self.fw.setup_base_nwfilters()
-        self.fw.setup_nwfilters_for_instance(instance)
+        self.fw.setup_basic_filtering(instance)
+        self.fw.prepare_instance_filter(instance)
         _ensure_all_called()
         self.teardown_security_group()

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-01-04 05:26:41 +0000
+++ nova/virt/libvirt_conn.py	2011-01-10 10:38:10 +0000
@@ -86,6 +86,9 @@
 flags.DEFINE_bool('allow_project_net_traffic',
                   True,
                   'Whether to allow in project network traffic')
+flags.DEFINE_string('firewall_driver',
+                    'nova.virt.libvirt_conn.IptablesFirewallDriver',
+                    'Firewall driver (defaults to iptables)')
 
 
 def get_connection(read_only):
@@ -125,16 +128,24 @@
         self._wrapped_conn = None
         self.read_only = read_only
 
+        self.nwfilter = NWFilterFirewall(self._get_connection)
+
+        if not FLAGS.firewall_driver:
+            self.firewall_driver = self.nwfilter
+            self.nwfilter.handle_security_groups = True
+        else:
+            self.firewall_driver = utils.import_object(FLAGS.firewall_driver)
+
     def init_host(self):
-        NWFilterFirewall(self._conn).setup_base_nwfilters()
+        pass
 
-    @property
-    def _conn(self):
+    def _get_connection(self):
         if not self._wrapped_conn or not self._test_connection():
             LOG.debug(_('Connecting to libvirt: %s'), self.libvirt_uri)
             self._wrapped_conn = self._connect(self.libvirt_uri,
                                                self.read_only)
         return self._wrapped_conn
+    _conn = property(_get_connection)
 
     def _test_connection(self):
         try:
@@ -351,10 +362,13 @@
                               instance['id'],
                               power_state.NOSTATE,
                               'launching')
-        NWFilterFirewall(self._conn).setup_nwfilters_for_instance(instance)
+
+        self.nwfilter.setup_basic_filtering(instance)
+        self.firewall_driver.prepare_instance_filter(instance)
         self._create_image(instance, xml)
         self._conn.createXML(xml, 0)
         LOG.debug(_("instance %s: is running"), instance['name'])
+        self.firewall_driver.apply_instance_filter(instance)
 
         timer = utils.LoopingCall(f=None)
 
@@ -693,18 +707,55 @@
         domain = self._conn.lookupByName(instance_name)
         return domain.interfaceStats(interface)
 
-    def refresh_security_group(self, security_group_id):
-        fw = NWFilterFirewall(self._conn)
-        fw.ensure_security_group_filter(security_group_id)
-
-
-class NWFilterFirewall(object):
+    def refresh_security_group_rules(self, security_group_id):
+        self.firewall_driver.refresh_security_group_rules(security_group_id)
+
+    def refresh_security_group_members(self, security_group_id):
+        self.firewall_driver.refresh_security_group_members(security_group_id)
+
+
+class FirewallDriver(object):
+    def prepare_instance_filter(self, instance):
+        """Prepare filters for the instance.
+
+        At this point, the instance isn't running yet."""
+        raise NotImplementedError()
+
+    def apply_instance_filter(self, instance):
+        """Apply instance filter.
+
+        Once this method returns, the instance should be firewalled
+        appropriately. This method should as far as possible be a
+        no-op. It's vastly preferred to get everything set up in
+        prepare_instance_filter.
+        """
+        raise NotImplementedError()
+
+    def refresh_security_group_rules(self, security_group_id):
+        """Refresh security group rules from data store
+
+        Gets called when a rule has been added to or removed from
+        the security group."""
+        raise NotImplementedError()
+
+    def refresh_security_group_members(self, security_group_id):
+        """Refresh security group members from data store
+
+        Gets called when an instance gets added to or removed from
+        the security group."""
+        raise NotImplementedError()
+
+
+class NWFilterFirewall(FirewallDriver):
     """
     This class implements a network filtering mechanism versatile
     enough for EC2 style Security Group filtering by leveraging
     libvirt's nwfilter.
 
     First, all instances get a filter ("nova-base-filter") applied.
+    This filter provides some basic security such as protection against
+    MAC spoofing, IP spoofing, and ARP spoofing.
+
     This filter drops all incoming ipv4 and ipv6 connections.
     Outgoing connections are never blocked.
 
@@ -738,38 +789,79 @@
 
     (*) This sentence brought to you by the redundancy department of
         redundancy.
+
     """
 
     def __init__(self, get_connection):
-        self._conn = get_connection
-
-    nova_base_filter = '''<filter name='nova-base' chain='root'>
-                            <uuid>26717364-50cf-42d1-8185-29bf893ab110</uuid>
-                            <filterref filter='no-mac-spoofing'/>
-                            <filterref filter='no-ip-spoofing'/>
-                            <filterref filter='no-arp-spoofing'/>
-                            <filterref filter='allow-dhcp-server'/>
-                            <filterref filter='nova-allow-dhcp-server'/>
-                            <filterref filter='nova-base-ipv4'/>
-                            <filterref filter='nova-base-ipv6'/>
-                          </filter>'''
-
-    nova_dhcp_filter = '''<filter name='nova-allow-dhcp-server' chain='ipv4'>
-                            <uuid>891e4787-e5c0-d59b-cbd6-41bc3c6b36fc</uuid>
-                              <rule action='accept' direction='out'
-                                    priority='100'>
-                                <udp srcipaddr='0.0.0.0'
-                                     dstipaddr='255.255.255.255'
-                                     srcportstart='68'
-                                     dstportstart='67'/>
-                              </rule>
-                              <rule action='accept' direction='in'
-                                    priority='100'>
-                                <udp srcipaddr='$DHCPSERVER'
-                                     srcportstart='67'
-                                     dstportstart='68'/>
-                              </rule>
-                            </filter>'''
+        self._libvirt_get_connection = get_connection
+        self.static_filters_configured = False
+        self.handle_security_groups = False
+
+    def _get_connection(self):
+        return self._libvirt_get_connection()
+    _conn = property(_get_connection)
+
+    def nova_dhcp_filter(self):
+        """The standard allow-dhcp-server filter is an <ip> one, so it uses
+           ebtables to allow traffic through. Without a corresponding rule in
+           iptables, it'll get blocked anyway."""
+
+        return '''<filter name='nova-allow-dhcp-server' chain='ipv4'>
+                    <uuid>891e4787-e5c0-d59b-cbd6-41bc3c6b36fc</uuid>
+                    <rule action='accept' direction='out'
+                          priority='100'>
+                      <udp srcipaddr='0.0.0.0'
+                           dstipaddr='255.255.255.255'
+                           srcportstart='68'
+                           dstportstart='67'/>
+                    </rule>
+                    <rule action='accept' direction='in'
+                          priority='100'>
+                      <udp srcipaddr='$DHCPSERVER'
+                           srcportstart='67'
+                           dstportstart='68'/>
+                    </rule>
+                  </filter>'''
+
+    def setup_basic_filtering(self, instance):
+        """Set up basic filtering (MAC, IP, and ARP spoofing protection)"""
+        logging.info('called setup_basic_filtering in nwfilter')
+
+        if self.handle_security_groups:
+            # No point in setting up a filter set that we'll be overriding
+            # anyway.
+            return
+
+        logging.info('ensuring static filters')
+        self._ensure_static_filters()
+
+        instance_filter_name = self._instance_filter_name(instance)
+        self._define_filter(self._filter_container(instance_filter_name,
+                                                   ['nova-base']))
+
+    def _ensure_static_filters(self):
+        if self.static_filters_configured:
+            return
+
+        self._define_filter(self._filter_container('nova-base',
+                                                   ['no-mac-spoofing',
+                                                    'no-ip-spoofing',
+                                                    'no-arp-spoofing',
+                                                    'allow-dhcp-server']))
+        self._define_filter(self.nova_base_ipv4_filter)
+        self._define_filter(self.nova_base_ipv6_filter)
+        self._define_filter(self.nova_dhcp_filter)
+        self._define_filter(self.nova_vpn_filter)
+        if FLAGS.allow_project_net_traffic:
+            self._define_filter(self.nova_project_filter)
+
+        self.static_filters_configured = True
+
+    def _filter_container(self, name, filters):
+        xml = '''<filter name='%s' chain='root'>%s</filter>''' % (
+                 name,
+                 ''.join(["<filterref filter='%s'/>" % (f,) for f in filters]))
+        return xml
 
     nova_vpn_filter = '''<filter name='nova-vpn' chain='root'>
                            <uuid>2086015e-cf03-11df-8c5d-080027c27973</uuid>
@@ -783,7 +875,7 @@
         retval = "<filter name='nova-base-ipv4' chain='ipv4'>"
         for protocol in ['tcp', 'udp', 'icmp']:
             for direction, action, priority in [('out', 'accept', 399),
-                                                ('inout', 'drop', 400)]:
+                                                ('in', 'drop', 400)]:
                 retval += """<rule action='%s' direction='%s' priority='%d'>
                                <%s />
                              </rule>""" % (action, direction,
@@ -795,7 +887,7 @@
         retval = "<filter name='nova-base-ipv6' chain='ipv6'>"
         for protocol in ['tcp', 'udp', 'icmp']:
             for direction, action, priority in [('out', 'accept', 399),
-                                                ('inout', 'drop', 400)]:
+                                                ('in', 'drop', 400)]:
                 retval += """<rule action='%s' direction='%s' priority='%d'>
                                <%s-ipv6 />
                              </rule>""" % (action, direction,
@@ -819,43 +911,49 @@
         # execute in a native thread and block current greenthread until done
         tpool.execute(self._conn.nwfilterDefineXML, xml)
 
-    def setup_base_nwfilters(self):
-        self._define_filter(self.nova_base_ipv4_filter)
-        self._define_filter(self.nova_base_ipv6_filter)
-        self._define_filter(self.nova_dhcp_filter)
-        self._define_filter(self.nova_base_filter)
-        self._define_filter(self.nova_vpn_filter)
-        if FLAGS.allow_project_net_traffic:
-            self._define_filter(self.nova_project_filter)
-
-    def setup_nwfilters_for_instance(self, instance):
+    def prepare_instance_filter(self, instance):
         """
         Creates an NWFilter for the given instance. In the process,
         it makes sure the filters for the security groups as well as
         the base filter are all in place.
         """
 
-        nwfilter_xml = ("<filter name='nova-instance-%s' "
-                        "chain='root'>\n") % instance['name']
-
         if instance['image_id'] == FLAGS.vpn_image_id:
-            nwfilter_xml += "  <filterref filter='nova-vpn' />\n"
+            base_filter = 'nova-vpn'
         else:
-            nwfilter_xml += "  <filterref filter='nova-base' />\n"
+            base_filter = 'nova-base'
+
+        instance_filter_name = self._instance_filter_name(instance)
+        instance_secgroup_filter_name = '%s-secgroup' % (instance_filter_name,)
+        instance_filter_children = [base_filter, instance_secgroup_filter_name]
+        instance_secgroup_filter_children = ['nova-base-ipv4',
+                                             'nova-base-ipv6',
+                                             'nova-allow-dhcp-server']
+
+        ctxt = context.get_admin_context()
 
         if FLAGS.allow_project_net_traffic:
-            nwfilter_xml += "  <filterref filter='nova-project' />\n"
-
-        for security_group in instance.security_groups:
-            self.ensure_security_group_filter(security_group['id'])
-
-            nwfilter_xml += ("  <filterref filter='nova-secgroup-%d' "
-                             "/>\n") % security_group['id']
-        nwfilter_xml += "</filter>"
-
-        self._define_filter(nwfilter_xml)
-
-    def ensure_security_group_filter(self, security_group_id):
+            instance_filter_children += ['nova-project']
+
+        for security_group in db.security_group_get_by_instance(ctxt,
+                                                               instance['id']):
+
+            self.refresh_security_group_rules(security_group['id'])
+
+            instance_secgroup_filter_children += [('nova-secgroup-%s' %
+                                                         security_group['id'])]
+
+        self._define_filter(
+                    self._filter_container(instance_secgroup_filter_name,
+                                           instance_secgroup_filter_children))
+
+        self._define_filter(
+                    self._filter_container(instance_filter_name,
+                                           instance_filter_children))
+
+        return
+
+    def refresh_security_group_rules(self, security_group_id):
         return self._define_filter(
                    self.security_group_to_nwfilter_xml(security_group_id))
 
@@ -886,3 +984,162 @@
         xml = "<filter name='nova-secgroup-%s' chain='ipv4'>%s</filter>" % \
               (security_group_id, rule_xml,)
         return xml
+
+    def _instance_filter_name(self, instance):
+        return 'nova-instance-%s' % instance['name']
+
+
+class IptablesFirewallDriver(FirewallDriver):
+    def __init__(self, execute=None):
+        self.execute = execute or utils.execute
+        self.instances = set()
+
+    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)
+
+    def add_instance(self, instance):
+        self.instances.add(instance)
+
+    def prepare_instance_filter(self, instance):
+        self.add_instance(instance)
+        self.apply_ruleset()
+
+    def apply_ruleset(self):
+        current_filter, _ = self.execute('sudo iptables-save -t filter')
+        current_lines = current_filter.split('\n')
+        new_filter = self.modify_rules(current_lines)
+        self.execute('sudo iptables-restore',
+                     process_input='\n'.join(new_filter))
+
+    def modify_rules(self, current_lines):
+        ctxt = context.get_admin_context()
+        # Remove any trace of nova rules.
+        new_filter = filter(lambda l: 'nova-' not in l, current_lines)
+
+        seen_chains = False
+        for rules_index in range(len(new_filter)):
+            if not seen_chains:
+                if new_filter[rules_index].startswith(':'):
+                    seen_chains = True
+            elif seen_chains == 1:
+                if not new_filter[rules_index].startswith(':'):
+                    break
+
+        our_chains = [':nova-ipv4-fallback - [0:0]']
+        our_rules = ['-A nova-ipv4-fallback -j DROP']
+
+        our_chains += [':nova-local - [0:0]']
+        our_rules += ['-A FORWARD -j nova-local']
+
+        security_groups = set()
+        # Add our chains
+        # First, we add instance chains and rules
+        for instance in self.instances:
+            chain_name = self._instance_chain_name(instance)
+            ip_address = self._ip_for_instance(instance)
+
+            our_chains += [':%s - [0:0]' % chain_name]
+
+            # Jump to the per-instance chain
+            our_rules += ['-A nova-local -d %s -j %s' % (ip_address,
+                                                         chain_name)]
+
+            # Always drop invalid packets
+            our_rules += ['-A %s -m state --state '
+                          'INVALID -j DROP' % (chain_name,)]
+
+            # Allow established connections
+            our_rules += ['-A %s -m state --state '
+                          'ESTABLISHED,RELATED -j ACCEPT' % (chain_name,)]
+
+            # Jump to each security group chain in turn
+            for security_group in \
+                            db.security_group_get_by_instance(ctxt,
+                                                              instance['id']):
+                security_groups.add(security_group)
+
+                sg_chain_name = self._security_group_chain_name(security_group)
+
+                our_rules += ['-A %s -j %s' % (chain_name, sg_chain_name)]
+
+            # Allow DHCP responses
+            dhcp_server = self._dhcp_server_for_instance(instance)
+            our_rules += ['-A %s -s %s -p udp --sport 67 --dport 68' %
+                                                     (chain_name, dhcp_server)]
+
+            # If nothing matches, jump to the fallback chain
+            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)
+            our_chains += [':%s - [0:0]' % chain_name]
+
+            rules = \
+              db.security_group_rule_get_by_security_group(ctxt,
+                                                          security_group['id'])
+
+            for rule in rules:
+                logging.info('%r', rule)
+                args = ['-A', chain_name, '-p', rule.protocol]
+
+                if rule.cidr:
+                    args += ['-s', rule.cidr]
+                else:
+                    # Eventually, a mechanism to grant access for security
+                    # groups will turn up here. It'll use ipsets.
+                    continue
+
+                if rule.protocol in ['udp', 'tcp']:
+                    if rule.from_port == rule.to_port:
+                        args += ['--dport', '%s' % (rule.from_port,)]
+                    else:
+                        args += ['-m', 'multiport',
+                                 '--dports', '%s:%s' % (rule.from_port,
+                                                        rule.to_port)]
+                elif rule.protocol == 'icmp':
+                    icmp_type = rule.from_port
+                    icmp_code = rule.to_port
+
+                    if icmp_type == '-1':
+                        icmp_type_arg = None
+                    else:
+                        icmp_type_arg = '%s' % icmp_type
+                        if not icmp_code == '-1':
+                            icmp_type_arg += '/%s' % icmp_code
+
+                    if icmp_type_arg:
+                        args += ['-m', 'icmp', '--icmp_type', icmp_type_arg]
+
+                args += ['-j ACCEPT']
+                our_rules += [' '.join(args)]
+
+        new_filter[rules_index:rules_index] = our_rules
+        new_filter[rules_index:rules_index] = our_chains
+        logging.info('new_filter: %s', '\n'.join(new_filter))
+        return new_filter
+
+    def refresh_security_group_members(self, security_group):
+        pass
+
+    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 _instance_chain_name(self, instance):
+        return 'nova-inst-%s' % (instance['id'],)
+
+    def _ip_for_instance(self, instance):
+        return db.instance_get_fixed_address(context.get_admin_context(),
+                                             instance['id'])
+
+    def _dhcp_server_for_instance(self, instance):
+        network = db.project_get_network(context.get_admin_context(),
+                                         instance['project_id'])
+        return network['gateway']