Re: patch: nested acl evaluation

From: Jeffrey 'jf' Lim <jfs.world#gmail.com>
Date: Fri, 3 Apr 2009 13:37:53 +0800


On Fri, Apr 3, 2009 at 1:00 PM, Willy Tarreau <w#1wt.eu> wrote:
> On Fri, Apr 03, 2009 at 08:55:11AM +0800, Jeffrey 'jf' Lim wrote:
> (...)
>>
>> Having said that, there are blocks already in the standard config
>> (like "backend", with multiple "server" lines) - but the difference is
>> they don't need an ending keyword, so ok I see the problem here is
>> with the mandatory end (guess that's what you mean by "section", vs
>> "block"!)
>
> yes. It don't like it when we need to explicitly terminate a block (or
> section if you want). This always leads to trouble in the long term,
> because of missing or inverted termination statements. So the language
> must be unambiguous enough not to require this. BTW, that's what you
> have in cisco-like configs.
>

:) now that you mention it, yeah...

>> > With all that in mind, I'm wondering if what you want is not just sort
>> > of a massive remapping feature. I see that you have arbitrarily factored
>> > on the Host part and you are selecting the backend based on the path
>> > part. Is this always what you do ? Are there any other cases ?
>>
>> for us atm, no. (Although i do appreciate the flexibility, of course,
>> to factor on anything else ['for_acl' will do this right now]. Key
>> thing for me is, I want to reduce acl processing when we've got big
>> sticking common acl among all the statements [you've seen my
>> examples])
>
> OK. Just so that I get an idea, how many use_backend rules (and how many
> backends) do you have in a large config ? I'm asking because I want to be
> able to support ACL files and rules files, and the way to implement them
> should mostly be driven by large config users.
>

we're on the way to reaching 100 use_backend rules even as we speak... :) (and because these rules are largely of the pattern i've mentioned.. this motivated this patch here)

>
>> > If you think you still need ACLs but
>> > just for use_backend rules, maybe we should just use a slightly
>> > different keyword : simply not repeat "use_backend" and use "select"
>> > instead, which does not appear in the normal config section :
>> >
>> >   use_backend bk1 if acl1
>> >   use_backend_block if Host1
>> >        select bk1 if path1 or path2
>> >        select bk2 if path3
>> >        select bk3 if path4 src1
>> >   ...
>> >   use_backend bkN if aclN
>> >
>> > That one would present the advantage of being more intuitive and
>> > would integrate better with other rules. Also, it would make it
>> > more intuitive how to write such blocks for other rule sets, and
>> > is very close to what you've already done. And that does not
>> > require any end tag since the keyword used in the block ("select"
>> > above) is not present in the containing block.
>> >
>>
>> right! I would be fine with this. Perhaps we could use the keyword
>> "use" instead of "select"? so
>>
>> use_backend_block if host_www
>>     use bk1 if path 1 or path 2
>>     use bk2 if path3
>>     use bk3 if path4 src1
>> ...
>
> I have no problem with "use" if we only support "use_backend", but
> I was thinking about extending it to support other keywords, and
> thought it would be a bit awkward with keywords such as "redirect"
> or "block". Maybe I'm wrong and "use" fits all usages ?
>

'use' is kind of generic, i suppose. I have to admit, though - that was proposed with my usage situation and tons of 'use_backend' swimming around in my head! :P But I get your point - you want to expand this to other keywords.

(oh, and I don't like call/with either).

>> I can trivially modify my patch to be able to do this.
>
> The problem is not to modify a patch but more to do it the right
> way. Right now, from what I've seen, your patch cuts the use_backend
> processing in two halves, while I don't think we should implement it
> that way.
>

Yeah, I had to cut it in two halves cos of the "overloading" of the 'use_backend' keyword... Of course if we use a separate keyword, then nothing like that happens :P I cut acl processing in half too - but of course, i guess that's only a given since we want to be able to reduce processing anyway.

>> > Maybe with a little bit more thinking we could come up with something
>> > more generic like this :
>> >
>> >   ...
>> >   call use_backend if acl1
>> >      with bk1 if path1 or path2
>> >      with bk2 if ...
>> >   ...
>> >
>> >   call redirect if acl1
>> >      with prefix http://here.local if path2
>> >      with location / if path3
>> >   ...
>> >
>> > See ? That would basically add an iterator around any type of ACL
>> > rule, providing us with the ability to only specify the verb on
>> > the first line, and all the args in the list, and this would make
>> > a lot of sense. I don't like the "call" nor "with" keywords, it's
>> > just an illustration.
>> >
>>
>> ok. Adding this would be interesting, although wow, lots of code... :P
>
> Not necessarily. Right now, your code wraps around use_backend only.
> Instead, we could extend the rules themselves to be two-dimensional.
> Instead of evaluating them by scanning all of the terms after the if,
> we could run through the list first.
>
> The current evaluation looks like this :
>
>    use_backend B if X or Y or Z
>
>  => foreach acl in X Y Z; do
>       if (acl) use_backend B
>
> While we would have something more like this :
>
>    use_backend_block if X or Y
>       use B1 if X1 or Y1 or Z1
>       use B2 if X2 or Y2 or Z2
>
>  => foreach acl in X Y; do
>       if (acl)
>         foreach rule in B1 B2; do
>           foreach acl in X1 Y1 Z1; do
>             if (acl) use_backend $B
>

hm... I dunno. I think it's more of like this? (this is great for dealing with 'and' conditions as well, btw)

if X or Y; do // or could be 'X and Y'! 'unless X', ... whatever   if X1 or Y1 or Z1
    use_backend B1
  else if X2 or Y2 or Z2
    use_backend B2

> As you can see, this still covers the needs for the simple use_backend
> statement, which can be seen as :
>
>    use_backend_block if X or Y or Z
>      use B
>
> So by transforming the rules evaluation method, it would be easier
> to support the same mechanism for many other things. For instance,
> I want to be able to make the "req*" rules conditional, so that
> we can decide to replace or add a header only if a host name matches.
> It would be nice to be able to do that :
>
>   req_add_block if ! hdr_farm_present
>      use X-farm:\ farm1  if host1 or host3 or host5
>      use X-farm:\ farm2  if host2 or host4 or host6
>
> As always, that's just an example. But if we can find how to declare
> such blocks so that we still use the same keyword, it would require
> less code and less thinking for the guy writing the config. We could
> very well use "rules", "list", "enum" or anything like this, followed
> by the standard rule keyword ("use_backend", "redirect", "reqadd",...)
> then the first-level condition, and enumerate the list of rules for
> the second level.
>

right.

> In fact, the code is not that much important. Design is more important,
> because this is what provides us with something evolutive which does
> not conflict with other evolutions. If you look at what we have now,
> the things which were implemented because they were easy are the ones
> annoying us because we still need to support them eventhough they're
> not necessarily well thought (eg: the "block" rules).
>
> So let's think a little bit more about this interesting idea to ensure
> we do it right, even if this requires more code than your current patch.
>

ok got it. And thanks for the comments and thought over this.

-jf Received on 2009/04/03 07:37

This archive was generated by hypermail 2.2.0 : 2009/04/03 07:45 CEST