Re: [PATCH]: Check for duplicated conflicting proxies

On Sat, 20 Oct 2007, Willy Tarreau wrote:

> Hi again Krzysztof,

Hi, :)

> On Fri, Oct 19, 2007 at 04:50:26PM +0200, Krzysztof Oledzki wrote:
>> Hello,
>>
>> Currently haproxy accepts a config with duplicated proxies
>> (listen/fronted/backed/ruleset). This patch fix this, so the application
>> will complain when there is an error.
>>
>> With this modification it is still possible to use the same name for two
>> proxies (for example frontend&backend) as long there is no conflict:
>>
>> listen backend frontend ruleset
>> listen - - - OK
>> backend - - OK OK
>> frontend - OK - OK
>> ruleset OK OK OK -
>
> After some thinking, the listen should not share its name with anything,
> because it also provides the "ruleset" capability. BTW, while rulesets
> can be declared, they are currently not really used. The goal was to be
> able to share complex rulesets between various listeners instead of
> having to rewrite them each time.

Indeed, I was really surprised when I found this in the code as there is nothing about this in the documentation. Now I know why. ;)

> I would rather have something like this :
>
> listen backend frontend ruleset
> listen - - - -
> backend - - OK OK
> frontend - OK - OK
> ruleset - OK OK -
>
> This translates into an even simpler test, as instead of this :
>
>> + if (!strcmp(curproxy->id, args[1]) &&
>> + ((curproxy->cap==rc) || (curproxy->cap & rc & (PR_CAP_FE | PR_CAP_BE)))) {
>> + Alert("parsing %s: duplicated proxy %s with conflicting capabilities: %X/%X!\n",
>> + file, args[1], curproxy->cap, rc);
>> + return -1;
>> + }
>
> We can use this :
>
> + if (!strcmp(curproxy->id, args[1]) &&
> + (curproxy->cap & rc & (PR_CAP_LISTEN))) {
> + Alert("parsing %s: duplicated proxy %s with conflicting capabilities: %X/%X!\n",
> + file, args[1], curproxy->cap, rc);
> + return -1;
> + }

I'm not sure about this as all frontend, backend and listen get the PR_CAP_RS attribute:

         if (!strcmp(args[0], "listen"))
                 rc = PR_CAP_LISTEN;
         else if (!strcmp(args[0], "frontend"))
                 rc = PR_CAP_FE | PR_CAP_RS;
         else if (!strcmp(args[0], "backend"))
                 rc = PR_CAP_BE | PR_CAP_RS;
         else if (!strcmp(args[0], "ruleset"))
                 rc = PR_CAP_RS;
         else
                 rc = PR_CAP_NONE;

If I get the code rigth, PR_CAP_LISTEN simply means "match all":

#define PR_CAP_NONE    0x0000
#define PR_CAP_FE      0x0001
#define PR_CAP_BE      0x0002
#define PR_CAP_RS      0x0004
#define PR_CAP_LISTEN  (PR_CAP_FE|PR_CAP_BE|PR_CAP_RS)

Additionally IMHO, there is no gain using PR_CAP_LISTEN in the "if".

So, to do it what you described we would rather need something like this:

         if (x==y || ((x & y) & (PR_CAP_FE | PR_CAP_BE)) || x==PR_CAP_LISTEN || y==PR_CAP_LISTEN)

                listen backend frontend ruleset
listen             -      -       -        -
backend            -      -       OK       OK
frontend           -      OK      -        OK
ruleset            -      OK      OK       -

The question remains if we should allow to use ruleset also with backend and frontend. Reading your above explanation I think no, so as soon you confirm this, I will send a fixed patch with:

         if (((x | y) != PR_CAP_LISTEN) || x==PR_CAP_LISTEN || y==PR_CAP_LISTEN)

                listen backend frontend ruleset
listen             -      -       -        -
backend            -      -       OK       -
frontend           -      OK      -        -
ruleset            -      -       -        -

Best regards,

                         Krzysztof Olêdzki Received on 2007/10/20 14:22