Hi Willy,
Thanks for clarifying the connection modification situation, I think I now
understood the problem and the current state.
> No it's not too late. We're just trying to avoid last-minute breaking
> changes, such as the ones we usually tend to do late and to regret, either
> because they don't integrate well and make all of us spend our time trying to
> fix corner cases instead of cleaning up the rest, or because they break stuff
> late and complicate testing. Such a change is tiny and harmless, I'd almost
> accept it the week before the release (but please don't do that).
Okay, great. I am really trying to get it done today, but I have a couple
questions:
> BTW, please check if this works in default-server directives.
struct srv_pp_tlv_list {
struct list list;
struct list fmt;
unsigned char type;
};
To allow for use with the default server, I adjusted srv_settings_cpy
accordingly such that the server TLV entries (see above) are deep copied.
It works, but there is an edge case with the struct logformat_node that is
contained in such a TLV struct. default-server directives
Its member expr has the type of a void pointer, therefore I cannot directly
copy it.
Alternatively, if I would reuse the memory by simply copying the pointer, a
double free will likely happen in srv_drop (I always free the TLV list in it,
alongside the logformat node list).
Besides, the servers created from the default-backend should operate
independently, so this is not an option, I guess.
> You may want to have a look at how the "sni" keyword which also supports an
> expression is handled, as I don't recall the exact details.
> Maybe in your case we don't need the expression but the log-format instead
> and it's not an issue, I just don't remember the details without having a
> deeper look to be honest.
Maybe let's just use a sample expression as its usage is more straight forward,
basically similar to the sni option? Or do you have any other ideas on how to
tackle the issue I described above?
Disabling the default-server support could be another solution. I am very
interested in your opinion on this.
Best,
Alexander
-Original Message-
From: Willy Tarreau
Sent: Monday, October 23, 2023 2:33 PM
To: Stephan, Alexander
Cc: haproxy@formilux.org
Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated
proxy-v2-options
Hi Alexander,
On Mon, Oct 23, 2023 at 12:07:39PM +, Stephan, Alexander wrote:
> We can ignore the last two commits for now (LOW: connection: Add TLV
> update function and MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2
> TLVs).
> Based on the first two commits, I created a diff that would implement
> something like you requested (new parser and no use of remote).
> It should be fully working, so you could even try it out locally if you want.
> If you think it looks promising, I will adjust the tests and the docs,
> and update the series.
OK.
> Now to address your feedback in more detail:
>
> > I'm really having an issue with using "the remote frontend connection"
> > here. We've done this mistake in the past with the transparent mode that
> > connects to the original destination address, that resulted in "set-dst"
> > having to be implemented, then being broken by multiplexed streams (e.g.
> > h2), then having to be internally emulated at various layers (connection,
> > session, stream, transaction etc). Same for src. The solution is not
> > durable at all, and as you noticed, you already had to implement the
> > ability to modify these fields before passing them, hence reintroducing
> > that class of trouble on a new area.
>
> I choose to use the remote connection since it is already done that
> way for other TLV types like SRV_PP_V2_AUTHORITY
> (https://github.com/haproxy/haproxy/commit/8a4ffa0aabf8509098f95ac78fa48b18f415c42c).
> It simply forwards what is found the remote connection, if there is
> something set. Similarly, PP2_TYPE_NETNS and SRV_PP_V2_SSL depend on remote.
> First, I did not like the method with an extra fetch since it creates
> potential overhead. However, I am likely a bit excessive here. I am
> not opposed to not using remote at all. I simply was not aware of the
> intricacies.
Oh rest assured I'm not criticizing your choice, it's common to start from what
exists, I was merely explaining that what exists is the result of poor choices
and that we'd rather not spread them further ;-)
> > Also what will be left in the logs after you modify the contents ? etc.
> You mean that the log does not match the content of the actual TLV?
I mean once modified via a rule, it's difficult to know if the log contains a
modified field or an original one.
> It could happen, I suppose. But why doesn't this problem with
> multiplexed connections apply to all the TCP actions? As far as I know
> they all alter fields of the connection directly. Doesn't really
> matter though, I don't plan to use it anymore.
We do
