Re: patch: nested acl evaluation

2009-04-04 Thread Willy Tarreau
On Sat, Apr 04, 2009 at 10:20:23AM +0800, Jeffrey 'jf' Lim wrote:
  OK maybe use is OK in fact, considering the alternatives.
 
 
 :) some proposals for the keywords:
 
 for/use
 condition/use
 cond/use
 
 (cond/use seems the best compromise - short, but understandable enough)

what would you think about do/use ?

  If we extend the system, we'll have to associate parameters
  with a condition.
 
  But since the entry point is the switching rule here, maybe we'll end up
  with something very close to what you have implemented in your patch, in
  the end, and it's just that we want to make it more generic to use the
  two conditions layers in any ruleset.
 
 
 I would guess so! Even the redirect rules and 'block' rules look
 pretty similar... :)

yes, and maybe this is what we should try to improve : converge towards
a generic condition matching/execution system, to which we pass the
action and the args. That way, we just have to run it over redirect_rules
or switching_rules always using the same code.

Willy




Re: patch: nested acl evaluation

2009-04-02 Thread Willy Tarreau
Hi Jeffrey,

On Thu, Apr 02, 2009 at 02:23:44PM +0800, Jeffrey 'jf' Lim wrote:
(...)
 Ok perhaps combinatorial was not the word that i should have used,
 but... I hope you can see the point/s with the explanation that i
 gave. The head acl only gets checked once - thereafter which it goes
 into the body (you could treat it like the standard if statement in
 any programming language) to do the evaluation.
 
 Without this, the head acl has to be evaluated every time for every
 combination of head acl + sub acl for you can go into each
 'use_backend'. So eg:
 
 use_backend b1 if host_www.domain.com path1
 use_backend b2 if host_www.domain.com path2
 use_backend b3 if host_www.domain.com path3
 use_backend b4 if host_www.domain path4 or host_www.domain path5
 ...
 
 use_backend b10 if host_www.d2.com path1
 use_backend b11 if host_www.d2.com path2
 ...
 
 
 So does it make sense to cut out all of this? Of course it does.
 Faster acl processing (you dont repeat having to process
 'host_www.domain.com' every time!), much neater (and maintainable)
 config file.
 
 ==
 
 The following is a refined patch (did i mention i was serious about
 this? just to allay the April Fool's thing)
 
 One caveat to take note: at this point in time I'm not going to cater
 for nested 'for_acl's (I think if u really need this, you probably
 have bigger problems). One level of 'for_acl' should be all you
 need... (why is this deja vu) (Willy, I'll work out the documentation
 later once/if u give the go ahead for this, thanks)

I understand the usefulness of your proposal, but I really dislike
the implementation for at least two reasons :
  - right now the config works on a line basis. There are sections, but
no block. This is the first statement which introduces the notion
of a block, with a beginning and a mandatory end. I don't like that
for the same reason I don't like config languages with braces. It's
harder to debug and maintain.

  - only the use_backend rules are covered. So the for_acl keyword
is not suited since in fact you're just processing a use_backend
list. Also, that leaves me with two new questions : what could be
done for other rules ? Do we need to duplicate all the code, or can
we factor the same block out and use it everywhere ? Second
question : maybe you need this for use_backend only, which marks it
as a special case which might be handled even more easily ?

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 ? You still
have to declare ACLs for each path. Maybe it would be better to simply
support something like this :

use_backend back1 if acl1
map_path_to_backend if Host1
use BK1 on ^/img
use BK2 on ^/js
use BK3 on ^/static/.*\.{jpg|gif|png}
...
use_backend backN if aclN


See ? No more ACLs on the path and direct mapping between path and
backends for a given host. 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.

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.

I'd like to get opinions for other massive ACL users, because any
minor change might have significant impact to many of us, and we
must keep in mind that what we develop has to be maintained over
the years, sometimes conflicting with further features.

Best regards,
Willy