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. 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 atomic can make lots of changes in one go.
> 
> How about we refactor this whole thing in Cactus?
> 
>> 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 nova_forward.
> 
> I'm currently doing the latter (adding a nova-local chain at the
> beginning of FORWARD that I use).
> 
> -- 
> Soren Hansen <email address hidden>
> Systems Architect, The Rackspace Cloud
> Ubuntu Developer
> 
> https://code.launchpad.net/~soren/nova/iptables-security-groups/+merge/43767
> You are reviewing the proposed merge of lp:~soren/nova/iptables-security-groups into lp:nova.