On Wed, Apr 10, 2024 at 03:28:06PM +0200, Christopher Faulet wrote:
> Hi,
>
> Thanks. I have few comments.
>
> First, your commit message must follow rules of CONTRIBUTING file. The
> commit subject must mention a level (here MINOR) and a scope (here
> connection). Then a commit message must be provided with details on the
> patch. You should describe what you want to achieve with this feature, how
> and when it should be used...
>
> Then, the documentation must be included in the same patch. This eases the
> maintenance.
>
> The 'dstip' parameter must also be added in the 'source' server keyword. And
> it must be documented. In the 'source' backend keyword, info about 'client'
> and 'clientip' parameters are provided. The same must be done for 'dstip'.
>
> In cfg_parse_listen(), 'dst' is used instead of 'dstip' in the error message
> and the wrong flag is set ( CO_SRC_TPROXY_CIP instead of .
> CO_SRC_TPROXY_DIP).
>
> Finally, I didn't checked deeply, but CO_SRC_TPROXY_CIP is used in
> proto_tcp.c and proto_quic.c files. I guess something must also be added
> here.
>
> I have also a question. For completeness, could the 'dst' parameter be useful
> ?
Actually I would *really* prefer that we support an expression here
rather than adding yet another exception to the parsing.
We already took great care to support "hdr_ip(name)" that matches the
expression syntax so that we could later use any other expression. With
an expression we could support maps and plenty of other facilities. Our
expressions already have an address output type so we could simply say
that we support client/clientip or an expression of type address. I
can easily imagine how some users would for example hope to map sni to
source IP etc. Given that we already support expressions for "sni", I
don't think that it's very difficult to do the same for "usesrc".
In this case the correct destination parameter for the case above would
be "dst" or even "fc_dst" so that it doesn't get broken by "set-dst".
Willy
