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 <w...@1wt.eu>
Sent: Monday, October 23, 2023 2:33 PM
To: Stephan, Alexander <alexander.step...@sap.com>
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 +0000, 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 not modifiy the connection anymore since 2.7 or so, the set-src and
set-dst caused us a lot of misery for the exact reason you mentioned. Now this
is transparently intercepted at various levels to act either on the stream,
session or connection depending where it's done, and potentially it will
extract the lower value, modify it and assign it to an upper layer.
Yeah, that's totally ugly, but required to preserve compatibility with what
we've done before. We'd definitely not want to have to do that for ppv2 and
completely free fields! As a general rule we now try hard *not* to modify
existing information and rather use expressions or variables wherever possible
because it allows anyone to adjust the contents as they see fit without having
to later add exceptions for certain corner cases.
> > Why not something like "set-proxy-v2-tlv"? Maybe we then could also leave
> > out the v2.
>
> I would propose to go with the set-proxy-v2-tlv-fmt as you suggested
> later. I would leave in the v2 as other server options that concern v2
> also leave it in. I don't mind being more verbose here. Realistically,
> there are only 2-3 of these set-proxy-v2-tlv-fmt anyway. Expression
> could make sense at some later point in time.
That's what I thought as well, not that many fields there so no big deal.
> > That's also why I'm extremely careful about the config shortcomings
> > that can come with it, because I suspect it could become popular and
> > we really want to avoid new fall traps.
>
> Sure, that is completely understandable.
>
> > I've been wondering if we want to use a format string or raw binary
> > data here, because some options are binary and nothing indicates
> > that this will not continue in the future. However since fc_pp_tlv()
> > returns a string, I guess most of them will continue this way.
>
> I also think type string is fine.
Great!
> > Also once this is merged, a new problem will emerge. There's still no
> > registry for the PPv2 TLV types.
>
> I guess this should be okay for most cases where it is just used internally.
> But yeah, most people will just come up with TLVs off the top their
> head. I will definitely add a note about the ranges.
Thanks ;-)
> > Given that you had previously contributed the ability to fetch a TLV
> > field from the front connection's proxy protocol using fc_pp_tlv(),
> > I'd rather make use of these ones explicitly. If you need to change
> > them, then you just assign them to a variable and edit that
> > variable, then send the variable. Or you can change them in-flight
> > using a converter. Of course it would make configs slightly more
> > verbose, but they would always do the right thing and we would not
> > have to continue to special-case them over time because they break
> > expectations like set-src/set-dst did.
>
> Generally, I wouldn't mind a more verbose config. As usually there
> shouldn't be too many TLV types that need to be considered. I have no
> problem leaving out the setter. It was just merely an (premature)
> optimization approach.
I totally understand, especially based on history.
> > I think that most of your patch will remain as-is, it's only the
> > parsing part which needs to change a little bit to take such points into
> > account.
>
> Ok, great!
>
> Here is how I adjusted the first and the second commit (besides Doc
> and Tests). It works quite nicely with the keyword.
>
> An example server config works like this now:
> server dummy_server 127.0.0.1:2319 send-proxy-v2
> set-proxy-v2-tlv-fmt(0x10) %[str("hi")] set-proxy-v2-tlv-fmt(0x11)
> %[fc_pp_tlv(0x11)]
>
> I could perfectly replicate the forwarding behavior with the
> fc_pp_tlv. I hope you are fine with the changes. If so, I would send the
> updated series.
> Then, we could hopefully get into the final, detailed part of review.
Looks good to me, thank you!
> BTW, I've read that there is a feature freeze for 2.9, so I guess this
> change comes too late now?
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).
BTW, please check if this works in default-server directives. 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.
Thanks!
Willy
