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
