Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for inverting filter match conditions #98

Closed
wants to merge 1 commit into from

Conversation

DanielG
Copy link
Contributor

@DanielG DanielG commented Jan 26, 2023

No description provided.

@jech
Copy link
Owner

jech commented Mar 17, 2023

Could you please explain why this is useful?

@DanielG
Copy link
Contributor Author

DanielG commented Jul 22, 2023

I believe my motivation for this was to slightly alleviate the problem of babeld's router filter composability. Like I mentioned in #74 (comment) I have multiple config files, one common one deployed across all routers and one local to each router. The local one is included via -c first and then the common one.

Problem is right now babeld's filter rules are "last match wins" which makes it extremely hard to compose things this way since any policy decision made in the local config file might be (usually accidentally) overridden by common.conf.

With support for negation it would at least be possible to make exceptions to overlapping rules, but I'm starting to think this really is not enough.

Perhaps a config option to change filter semantics to "first match wins" would be more universally useful.

@jech
Copy link
Owner

jech commented Jul 23, 2023

Problem is right now babeld's filter rules are "last match wins"

Typo here? It's the first match that wins, unless something broke at some point.

I believe my motivation for this was to slightly alleviate the problem of babeld's router filter composability.

I'm sorry if I'm being dense, but could you please provide a concrete example of something that would be useful in production? I'd really like to avoid adding even more complexity unless we have a compelling example of why a given feature is useful.

@DanielG
Copy link
Contributor Author

DanielG commented Jul 23, 2023

Indeed a typo I meant last match and that's the problem.

It's not you, it's me :) I write these reports while knee deep in my configs so I have to be a bit terse. I'm always happy to elaborate, so here's an example of what I mean:

I run babeld something like this babeld -c common.conf -c local.conf ...

I have this in common.conf

redistribute ip ::/0 ge 8 allow # any ipv6 prefixes
redistribute ip 0.0.0.0/0 local deny # no ipv4 addrs

Then in local.conf I had:

redistribute metric 2000
redistribute local metric 2000

in order to demote this router quite hard as I was still working on setting it up.

Now what I found (and surprised me) is that all the IPv4 addresses are getting redistributed too since the earlier policy of denying ipv4 was being overridden by the >0 metric and last-match-wins semantics.

This is by no means the only time this happened though, it's just the one that got me to report it ;) I think this is counterintuitive behavior in general. Every networking config I know uses first-match-wins plus usually some way to abort evaluating any more rules cf firewall rules. Routing tables being the exception of course, but even there policy-routing/ip-rule has the behaviour I want ;)

Now it could be argued that perhaps I should just reorder my config files like this: babeld -c local.conf -c common.conf to deal with this, but I'm not sure how global/interface options would behave then? I would assume (but haven't checked) those have last-occurrence-wins behavior too which would then give me local being overridden by common, which isn't what I want there either.

I think we have a couple of options to make babeld better here, I can think of:

  • Add an "final" actions to make a stop filter evaluation early.
  • Add a match on the current (internal) metric. With this I could say redistribute get-metric!=infty metric 2000 to only change the metric of non denied routes.
  • Add a global option to reverse the filter rule processing order. I'm not sure this is powerful enough, might just be putting lipstick on a pig :)
  • Add per match clause negation. With this I can always exclude routes that are handled by common policy in local policy but it'd be pretty brittle and not DRY admittedly.
  • Add "functions" packaging up multiple rules, plus per-clause negation. Then I can just call into common policy and have filters not overlap in a more maintainable way.

The original idea of this patch (negation) was, to stay with the redistribute example, to be able to say in local.conf (parens for clarity:

redistribute not (ip 0.0.0.0/0 local)  metric 2000

But that is going to not be powerful enough as soon as there's multiple filters in common.conf themselves using negation. Then we'd need per match clause negation. Anyway I digress :)

In summary I'm not really sure what's best here. If it were up to me I'd probably add "final" actions and the metric match but maybe even this "hidden" state is too much complexity hrm.

--Daniel

@jech
Copy link
Owner

jech commented Aug 6, 2023

Daniel, I'm confused: I've just tested, and it's the first match that triggers, which is the expected outcome.

@DanielG
Copy link
Contributor Author

DanielG commented Oct 18, 2023

Well colour me confused as well.

I'm back in the bowls of my babled config and here's what I'm seeing now. I'm trying to exclude a sub-prefix of my router's /56 from getting announce so I'm trying to deny that prefix in the redistribute filter. Curiously both of the orderings below give the same behavour: I see routes covered by :2ff::/64 in xroute and consequently being announced to other routers.

redistribute ip 2001:678:4d8:2ff::/64 deny
redistribute ip 2001:678:4d8:200::/56 le 56 allow

and

redistribute ip 2001:678:4d8:200::/56 le 56 allow
redistribute ip 2001:678:4d8:2ff::/64 deny

If babeld was indeed implementing first-match-wins I would expect the former ordering to filter the :2ff::/64 prefix, but not the latter ordering.

So in a way we seem to be both wrong as this looks like no-match-wins :) Last-match-wins would explain this as the overlapping redistribute ip ::/0 ge 8 allow in babeld.conf overrides the deny in local.conf. See below.

I have these cmdline args -c /etc/babeld.local.conf -c /etc/babeld.conf -c /etc/babeld.acl.conf and configs look like this (in cmdline order):

$ grep ^redistribute /etc/babeld.local.conf
redistribute ip 2001:678:4d8:200::/56 le 56 allow
redistribute ip 2001:678:4d8:2ff::/64 deny
# [non-overlapping filters omitted]

$ grep ^redistribute /etc/babeld.conf
redistribute ip ::/0 ge 8 allow # any ipv6 prefixes
redistribute ip ::/0 ge 8 proto 3 deny # Note [boot routes suck usually]
redistribute ip 0.0.0.0/0 local deny # no ipv4 addrs
redistribute proto 12 deny #12=bird

$ grep ^redistribute /etc/babeld.acl.conf

Thoroughly confused,
--Daniel

@DanielG
Copy link
Contributor Author

DanielG commented Oct 18, 2023

Ok, the weirdness continues. I realised I might be missing local since it's /128s that I'm trying to filter. Look like that is one problem, but then I found this:

redistribute ip 2001:678:4d8:2ff::/64 local metric 421
redistribute ip 2001:678:4d8:2ff::/64 metric 420
redistribute ip ::/0 ge 8 allow

Will cause all /128s xroutes to be metric 421 so we can establish that the first rule matches them in my environment. However `s/metric 421/deny/:

redistribute ip 2001:678:4d8:2ff::/64 local deny
redistribute ip 2001:678:4d8:2ff::/64 metric 420
redistribute ip ::/0 ge 8 allow

causes those same /128s to be metric 420. My understanding of local is that when it's not used the local address /128s will not match the filter. If I'm right the following should give metric 0 (and indeed it does):

redistribute ip 2001:678:4d8:2ff::/64 metric 420
redistribute ip ::/0 ge 8 allow

WTF? It's as if the localnes of the routes is being changed by denying them, but also this shows (again) that the metric 420 rule above will match even after the local deny rule has already matched.

--Daniel

EDIT: add redistribute ip ::/0 ge 8 allow implied by above report for clarity.

@DanielG
Copy link
Contributor Author

DanielG commented Oct 18, 2023

Ok I found one problem in my report above: I had le 56 which doesn't overlap with the /64 like I thought. Ignore that bit, let's focus on the new weirdness.

@DanielG
Copy link
Contributor Author

DanielG commented Oct 18, 2023

Additional datapoints. This gives me metric 421 /128s xroutes. This shows (again) local behaves how I outlined above:

redistribute ip 2001:678:4d8:2ff::/64 metric 420
redistribute ip 2001:678:4d8:2ff::/64 local metric 421
redistribute ip ::/0 ge 8 allow

Further, this gives me the behaviour I'm looking for, filter away the /128s:

redistribute ip 2001:678:4d8:2ff::/64 local deny
redistribute ip 2001:678:4d8:2ff::/64 deny
redistribute ip ::/0 ge 8 allow

@jech
Copy link
Owner

jech commented Oct 20, 2023

Please show your kernel's addresses and routing table (ip -6 addr show, ip -6 route show).

@DanielG
Copy link
Contributor Author

DanielG commented Oct 29, 2023

Hi Juliusz,

Please show your kernel's addresses and routing table (ip -6 addr show, ip -6 route show).

See direct mail "Re: babeld#98, filter evaluation debugging" the dumps were very large :)


Here's another more digestible datapoint from a different routing policy change I just implemented. xroute dump with just my default babel.conf (redistribute ip ::/0 ge 8 allow etc. see above):

add xroute 2001:678:4d8:40::/64-::/0 prefix 2001:678:4d8:40::/64 from ::/0 metric 0
add xroute 2001:678:4d8:acdd::/64-::/0 prefix 2001:678:4d8:acdd::/64 from ::/0 metric 0
add xroute 2001:678:4d8:40:250::/80-::/0 prefix 2001:678:4d8:40:250::/80 from ::/0 metric 0
add xroute 2001:678:4d8::250/128-::/0 prefix 2001:678:4d8::250/128 from ::/0 metric 0
add xroute 2001:678:4d8:40::250/128-::/0 prefix 2001:678:4d8:40::250/128 from ::/0 metric 0
add xroute 2001:678:4d8:40:250::1/128-::/0 prefix 2001:678:4d8:40:250::1/128 from ::/0 metric 0
add xroute 2001:678:4d8:40:250::2049/128-::/0 prefix 2001:678:4d8:40:250::2049/128 from ::/0 metric 0
add xroute 2001:678:4d8:acdd::1/128-::/0 prefix 2001:678:4d8:acdd::1/128 from ::/0 metric 0

Now I add the following in babeld.local.conf (before the ip ::/0 ge 8 allow rule) to stop announcing 2001:678:4d8:40::/64 but still allow 2001:678:4d8:40:250::/80:

redistribute ip 2001:678:4d8:40::/64 local deny
redistribute ip 2001:678:4d8:40::/64 eq 64 deny
redistribute ip 2001:678:4d8:40:250::/80 le 80 allow

Resulting xroute dump:

add xroute 2001:678:4d8:acdd::/64-::/0 prefix 2001:678:4d8:acdd::/64 from ::/0 metric 0
add xroute 2001:678:4d8:40:250::/80-::/0 prefix 2001:678:4d8:40:250::/80 from ::/0 metric 0
add xroute 2001:678:4d8::250/128-::/0 prefix 2001:678:4d8::250/128 from ::/0 metric 0
add xroute 2001:678:4d8:40:250::2049/128-::/0 prefix 2001:678:4d8:40:250::2049/128 from ::/0 metric 0
add xroute 2001:678:4d8:acdd::1/128-::/0 prefix 2001:678:4d8:acdd::1/128 from ::/0 metric 0

Ignoring the need to do the local deny trick to get the second rule to match, I have to put the /80 rule last to get it to have the desired effect. This is further proof that there is no first-match-wins behaviour. If there were I would have to put this rule first to avoid the /64 match taking precedence for the /80 route and /128 below it.

--Daniel

@snh
Copy link

snh commented Nov 8, 2023

If there were I would have to put this rule first to avoid the /64 match taking precedence for the /80 route and /128 below it.

The rule you have to deny the /64 won't apply to an /80 or /128 though regardless, as it is eq 64 in your example , not le 64.

In your example the le 80 allow rule is indeed the first match, so behaving as expected.

@DanielG
Copy link
Contributor Author

DanielG commented Dec 6, 2023

If there were I would have to put this rule first to avoid the /64 match taking precedence for the /80 route and /128 below it.

The rule you have to deny the /64 won't apply to an /80 or /128 though regardless, as it is eq 64 in your example , not le 64.

Doh, typo. Now I wish we had >=/<= instead of the letter forms :)

Ok, so changing eq 64 -> ge 64 (Note not le 64 as you suggest) I do see the first-match-wins behaviour. Hmm. Maybe it was always just the strange local behaviour that threw me off when testing.

@jech
Copy link
Owner

jech commented Dec 7, 2023

Thanks to both of you for investigating. I'm closing this, please reopen if you still think there's a bug.

Now I wish we had >=/<= instead of the letter forms :)

I'm rather fond of the current syntax, but I'm not opposed to tweaking it if there are strong arguments. Feel free to open a discussion on the mailing list.

@jech jech closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants