Apply firewall resources alphabetically

[mcanevet]

This will apply firewall resources by alphabetical order.
This greatly simplify Module usage IMHO.

Just:

include ::firewall

resources { "firewall":
  purge => true
}

firewall { '000 accept all icmp':
  proto   => 'icmp',
  action  => 'accept',
}

firewall { '001 accept all to lo interface':
  proto   => 'all',
  iniface => 'lo',
  action  => 'accept',
}

firewall { '002 accept related established rules':
  proto   => 'all',
  ctstate => ['RELATED', 'ESTABLISHED'],
  action  => 'accept',
}

firewall { '999 drop all':
  proto   => 'all',
  action  => 'drop',
}

No more

Firewall {
  before  => Class['my_fw::post'],
  require => Class['my_fw::pre'],
}
class { ['my_fw::pre', 'my_fw::post']: }

[mcanevet]

@hunner @apenney my goal is to add next a rules parameter to init and a create_resources('firewall', $rules) so that we can use databinding to store firewall rules in hiera like this:

---
firewall::rules:
  000 accept all icmp:
    proto: icmp
    action: accept
  001 accept all to lo interface:
    proto: all
    iniface: lo
    action: accept
  002 accept related established rules:
    proto: all
    ctstate: ['RELATED', 'ESTABLISHED']
    action: accept
  999 drop all:
    proto: all
    action: drop

[mcanevet]

hmmm travis fails with ruby1.8.7. I just tested with master and it also fails so it does not come from my patch.

[mcanevet]

This PR fixes unit tests : #341

[apenney]

I merged in #341, can you rebase this one against master?

[mcanevet]

@apenney done

[mcanevet]

@apenney now travis is happy !

[csschwe]

+1 works great.

[apenney]

@hunner raised the issue that this causes all further rules to be skipped if a single rule fails to apply for any reason, which is not the current behavior. Do we think that's a significant problem?

[hunner]

It's cool to have an easier way to order rules that are applied first vs. later, but do we want to enforce that rules ordered first in the chain should almost always be applied to the machine first? It's going to overload the semantics of the title leading-numbers, and isn't the normal Puppet Way of ordering resource application.

OTOH, I don't want convention to stand in the way of ease-of-use when it wouldn't hurt to add it.

[csschwe]

On the plus side you usually want firewall rules applied in a specific order since the first rule that is matched wins and this give an easy way to accomplish that (lots of fun with Cisco ACLs in the past). This also should mean you want all rules to apply top down and probably have a further down rules skipped if a rule before it fails. We don't really want a deny all rule to be processed if our allow port 22 rule failed.

You could look at it like the File auto dependency chain between /etc and /etc/hosts, in this case the 000 and 001 are being used to make this auto dependency.

Personally I like the idea of knowing how what order all firewall rules will be applied in without having to create a manual requires for each one. But I can also understand it isn't really the normal way, but it does seem to make sense for firewall rules since order matters much more than in a normal puppet run.

[hunner]

Okay. Sounds good.

This should probably also come with some updates in the README, as it has a pretty large section dedicated to talking about pre/post classes to avoid locking yourself out of a box.

[mcanevet]

@hunner README updated.

[phemmer]

Would it not make more sense to do this similar to how the concat module works? Each concat_fragment has an order parameter, and that parameter defines the order in which the fragment gets included.

Then you don't have to rely upon alphabetically sorting the title, which is problematic in itself. What happens when you have 010, 20, and 030. It'll get applied as 010, 030, 20 because you forgot a zero on the 20. This has bit me in the ass numerous times.

Having an order parameter would allow this field to be explicitly converted into an integer and would solve this issue. It also keeps us from defining yet another way of ordering resources.

[mcanevet]

@phemmer hmmm, if you have 010, 20 and 030 will it be ordered 010, 20 and 030 in iptables's filter table or 010, 030 and 20 ?

If it's ordered 010, 030 and 20, then IMHO it makes sense that resources are applied in the same order without the need of an additional order parameter.

If it's ordered 010, 20 and 030, then you're probably right and my patch may confuse some people.

I'll try to look at the code to see what order is actually applied.

[mcanevet]

@phemmer I just tried and it looks like rules are also ordered alphabetically, 030 comes before 20. So I think my patch is relevant and there is no need to add an additional order parameter.
@hunner @apenney is there something preventing this to be merged ?

[phemmer]

@mckern that was my point. It is unintuitive for 030 to come before 20, and introduces yet another ordering convention.

[mcanevet]

@phemmer maybe, but my patch is consistent with the ordering convention already applied. So I think your point shouldn't block this PR to be merged.
BTW, to follow your more intuitive ordering convention, I'd prefer extract the order from the begining of the title and order towards it than adding a new order parameter.

[hunner]

The concat sorts the same way (alphabetically instead of numerically) and I've always hated that. It's also backwards incompatible to change the ordering to numeric.

I'd rather not make the same mistake here. Since we don't yet make guarantees about ordering, it will not be backwards incompatible to merge this. But as soon as we do, it will be backwards incompatible to change the way that ordering works.

If possible I'd rather get it right on the first try, now that firewall is 1.x

[mcanevet]

@hunner changing the ordering to numeric is not tricky at all:

autorequire(:firewall) do
  catalog.resources.select { |r| (r.class.to_s == "Puppet::Type::Firewall") and (r.name.split(' ')[0].to_i < name.split(' ')[0].to_i) }.sort.last
end

But this will mismatch the actual logic applied in iptables.
Can't we still merge my patch as it clearly simplify the module implementation and does not break bakwards compatibility, then change the code once the ordering is changed in the provider ?

[hunner]

Sigh, good point.