RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
Hi Willy,
Thanks again for the merge, very glad that it made its way into 2.9.
> I don't see how this is possible:
>
>list_for_each_entry(srv_tlv, >pp_tlvs, list) {
>if (srv_tlv == NULL)
> break;
> For srv_tlv to be NULL, it would mean that you visited (NULL)->list at some
> point, and apparently pp_tlvs is always manipulated with
> LIST_APPEND() only, so I don't see how you could end up with something like
> last->next = (NULL)->list. Are you sure it was not a side effect of a
> debugging session with some temporary code in it ?
> I'd be interested in knowing if you can reproduce it so that we can find the
> root cause (and hopefully address it).
I finally got back to this again. You should be able to reproduce it quite
easily. After removing the 'break' above, you can run the regression tests.
Then, tests which (presumably) use the server settings copy function fail or
timeout.
Probably an even better method is to use the following configuration
'haproxy.cfg':
global
defaults
log global
timeout connect 500ms
timeout client 5000ms
timeout server 5ms
backend dummy
mode tcp
default-server send-proxy-v2 set-proxy-v2-tlv-fmt(0xE1) %[fc_pp_tlv(0xE1)]
server dummy_server 127.0.0.1:2319
frontend dummy
log stdout local0
mode tcp
bind *:2320 accept-proxy
default_backend dummy
Run it with:
gdb --args ./haproxy '-f' './haproxy.cfg'
It should crash immediately with the following seg fault:
Thread 1 "haproxy" received signal SIGSEGV, Segmentation fault.
srv_settings_cpy (srv=srv@entry=0xbed7e0, src=src@entry=0xbebce0,
srv_tmpl=srv_tmpl@entry=0) at src/server.c:2526
2526new_srv_tlv->fmt_string = strdup(srv_tlv->fmt_string);
Overall, I am not too confident with the order of invocations in the server.
Maybe we have some initializations mixed up and unintuitive some dependencies.
> Another point, in make_proxy_line_v2() I'm seeing that you've placed
> everything under "if (strm)". I understand why, it's because you didn't want
> to call build_logline() without a stream. That made > me think "hmmm we
> already have the ability to do that", and I found it, you can use
> sess_build_logline() instead, that takes the session separately, and supports
> an optional stream. That would be useful for health checks for example, since
> they don't have streams. But there's a catch: in function
> make_proxy_line_v2() we don't have the session at the moment, and
> build_logline() extracts it from the stream. Normally during a session
> establishment, the session is present in connection->owner, thus
> remote->owner in make_proxy_line_v2(). I suggest that you give this a
> try to confirm that it works for you, this way we'd be sure that health
> checks can also send the ppv2 fields. But that's not critical given that
> there's no bad behavior right now, it's just that fields will be ignored, so
> this can be done in a subsequent patch.
I looked into it again. I did not see an obvious way to retrieve the current or
pass the session as parameter.
I mean, the easy option, as you said, would be to use remote->owner, which
would require replacing if (stream) with if (remote) to prevent the NULL
dereference.
But we still want to send out TLVs when there is no remote. So maybe I could
change it such that it uses remote->owner when available and otherwise, falls
back to using the session from the stream?
Maybe something along the lines of
struct session *sess = (remote != NULL) ? remote->owner : strm->sess;
replace->data = sess_build_logline(sess, strm, replace->area,
replace->size, _tlv->fmt);
?
It seems to achieve the behavior you described regarding the health checks,
right? If that's alright, I will send the corresponding patch.
Best,
Alexander
-Original Message-
From: Willy Tarreau
Sent: Saturday, November 4, 2023 5:02 AM
To: Stephan, Alexander
Cc: haproxy@formilux.org
Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated
proxy-v2-options
Alexander,
I now merged your patch with the SMP_VAL_ change, after verifying that the
reg-test is still OK. Thus 2.9-dev9 will contain it. Thanks!
Willy
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
Hi Willy,
Thanks for the review. No problem for calling me Stephan, I am totally used to
that, my teachers did that for years.
> At first glance the patches look nice.
Great!
> Yeah I agree, that part is not pretty. And as often in such code, not
> handling errors makes them even more complicated to unroll as you experienced
> in the list loop you added, which could otherwise have been addressed using
> just a goto and a few free().
> Do not hesitate to propose extra patches to improve this, contributions that
> make the code more reliable and maintainable are totally welcome!
Sure, if I have some extra time on my hands. Actually, a colleague of mine is
preparing some patches like this. Since there are many small changes (e.g.,
adding a free), should can we batch them or are individual commits required?
> Are you sure it was not a side effect of a debugging session with some
> temporary code in it ?
Pretty sure, yes. I'll will get back to this at the beginning of next week, if
it's okay. I compiled with a debug flag, but I wouldn't usually expect this to
be an issue.
> So if that's OK for you I can change it now before merging.
Ah, I had used a SMP_VAL_* before, but I was not 100% about the meaning. Then I
fell back to the proxy. Feel free to change it!
> Yes very likely. Originally the code didn't check for allocation errors
> during parsing because it was the boot phase, and we used to consider that a
> memory allocation error would not always be reportable anyway, and given that
> it was at boot time, the result was going to be the same.
> But much later the code began to be a bit more dynamic, and such practices
> are no longer acceptable in parts that can be called at run time (e.g.
> during an "add server" on the CLI). It's particularly difficult to address
> some of these historic remains but from time to time we manage to pass an
> extra pointer to some functions to write an error, and make a function return
> a set of flags among ERR_{WARN,FATAL,ALERT,ABORT}. But it takes a lot of time
> > for little to no perceived value for the vast majority of users :-(
Interesting how that came to be, no accusations. I see that this is not a high
priority. Not sure if I have the time to provide a fix here but I'll keep it
mind.
> Another point, in make_proxy_line_v2() I'm seeing that you've placed
> everything under "if (strm)". I understand why, it's because you didn't want
> to call build_logline() without a stream. That made me think "hmmm we already
> have the ability to do that", and I found it, you can use
> sess_build_logline() instead, that takes the session separately, and supports
> an optional stream. That would be useful for health checks for example, since
> they don't have streams. But there's a catch: in function
> make_proxy_line_v2() we don't have the session at the moment, and
> build_logline() extracts it from the stream. Normally during a session
> establishment, the session is present in connection->owner, thus
> remote->owner in make_proxy_line_v2(). I suggest that you give this a
> try to confirm that it works for you, this way we'd be sure that health
> checks can also send the ppv2 fields. But that's not critical given that
> there's no bad behavior right now, it's just that fields will be ignored, so
> this can be done in a subsequent patch.
Again, I don't mind if you make the SMP_VAL_*. I tried the
sess_build_logline() and it seems to work perfectly. The tests also still pass,
so looks good. I would like to provide a second patch for this next week, if
this okay.
Best,
Alexander
-Original Message-
From: Willy Tarreau
Sent: Friday, November 3, 2023 8:11 PM
To: Stephan, Alexander
Cc: haproxy@formilux.org
Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated
proxy-v2-options
Hi Alexander,
(and BTW sorry for having called you Stephan twice in the last thread, each
time I have to make a mental effort due to how your first and last names are
presented in your e-mail address).
On Sat, Oct 28, 2023 at 07:32:20PM +, Stephan, Alexander wrote:
> I've just finished the implementation based on the tip with the
> deferred log format parsing so the default-server should be also fully
> supported now. ?
At first glance the patches look nice.
> I guess using the finalize method of the parser is the canonic implementation
> of this.
>
> I have a few remarks about the current state of the code:
> - srv_settings_cpy in server.c has no form of error handling
> available. This is potentially causes trouble due to lack of error handling:
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Fhaproxy%2Fhaproxy%2Fblob%2F47ed1181f29a56ccb04e5b80ef4f941446
> 6ada7a%2Fsrc%2Fserver.c%23L2
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
Hi Willy, Sorry, my email client probably did something weird... I attached them now, should hopefully prevent any reformatting. Best, Alexander -Original Message- From: Willy Tarreau Sent: Friday, November 3, 2023 5:52 PM To: Stephan, Alexander Cc: haproxy@formilux.org Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options On Fri, Nov 03, 2023 at 05:14:33PM +0100, Willy Tarreau wrote: > Hi Stephan, > > On Fri, Nov 03, 2023 at 01:54:26PM +, Stephan, Alexander wrote: > > Hi Willy, > > > > Did you receive the other two mails with the updated patches? I > > couldn't find it the reply to first page in the archive although I > > CCed the list. That's why I wanted to double-check, not to run in the > > previous situation. > > Yes, sorry, it's just that I have been busy on other stuff and didn't > have the time to have a look yet, but I'm seeing them still marked > unread here. I'm doing my best to review and merge them ASAP. Stephan, I just noticed that your patches were mangled with tabs turned to spaces. Please just resend them attached (the two in the same message if you want). Thanks, Willy 0001-MINOR-server-Add-parser-support-for-set-proxy-v2-tlv.patch Description: 0001-MINOR-server-Add-parser-support-for-set-proxy-v2-tlv.patch 0002-MINOR-connection-Send-out-generic-user-defined-serve.patch Description: 0002-MINOR-connection-Send-out-generic-user-defined-serve.patch
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
Hi Willy,
Did you receive the other two mails with the updated patches? I couldn't find
it the reply to first page in the archive although I CCed the list. That's why
I wanted to double-check, not to run in the previous situation.
Best,
Alexander
-Original Message-
From: Willy Tarreau
Sent: Friday, October 27, 2023 4:22 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 Fri, Oct 27, 2023 at 02:12:10PM +, Stephan, Alexander wrote:
> > 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.
Definitely. From what I remember about what we do for other such expressions
that are inherited from defaults, we use a trick: we only save the expression
as a string during parsing, that's the same that is kept on the default server.
Thus the new server just does an strdup() of that log-format expression that's
just a string. And only later we resolve it. That's not the best example but
the first I just found, please have a look at uniqueid_format_string. It's
handled exactly like this and resolved in check_config_validity().
> > 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?
Parsing the log-format string late definitely is the best option, and not too
cumbersome. You can even still report the line number etc from the "server"
struct to indicate in the parsing error if any, since everything is on the same
line, so there's really no problem with parsing late.
> Disabling the default-server support could be another solution. I am
> very interested in your opinion on this.
I'd really avoid disabling default-server as much as possible, or it will start
to be quite difficult to configure given that other related options are
accepted there. I tend to consider that the effort needed by users to work
around our limited designs often outweighs the efforts we should have involved
to make it smoother for them, so until we're certain it's not possible it's
worth trying ;-)
Thanks!
Willy
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
stom_tlv_a)]
+
+# Check that TLVs without an value are sent out.
+http-request set-var(txn.custom_tlv_b) fc_pp_tlv(0xE2)
+http-after-response set-header proxy_custom_tlv_b
%[var(txn.custom_tlv_b)]
+
+# Note that we do not check for an invalid TLV ID as that would result
in an
+# parser error anway.
+
+http-request return status 200
+} -start
+
+
+client c1 -connect ${h1_feS_sock} {
+txreq -url "/"
+rxresp
+expect resp.http.proxy_custom_tlv_a == "foo"
+expect resp.http.proxy_custom_tlv_b == ""
+} -run
+
+# Ensure that we achieve the same via a default-server.
+haproxy h2 -conf {
+defaults
+mode http
+log global
+
+timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+timeout client "${HAPROXY_TEST_TIMEOUT-5s}"
+timeout server "${HAPROXY_TEST_TIMEOUT-5s}"
+
+listen sender
+bind "fd@${feS}"
+default-server send-proxy-v2 set-proxy-v2-tlv-fmt(0xE1) %[str("bar")]
+server example ${h1_feR_addr}:${h1_feR_port}
+
+listen receiver
+bind "fd@${feR}" accept-proxy
+
+http-request set-var(txn.custom_tlv_a) fc_pp_tlv(0xE1)
+http-after-response set-header proxy_custom_tlv_a
%[var(txn.custom_tlv_a)]
+
+http-request return status 200
+} -start
+
+
+client c2 -connect ${h2_feS_sock} {
+txreq -url "/"
+rxresp
+expect resp.http.proxy_custom_tlv_a == "bar"
+} -run
diff --git a/src/connection.c b/src/connection.c
index 59893fbb5..3a13c2542 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -1927,10 +1927,11 @@ static int make_proxy_line_v2(char *buf, int buf_len,
struct server *srv, struct
int ret = 0;
struct proxy_hdr_v2 *hdr = (struct proxy_hdr_v2 *)buf;
struct sockaddr_storage null_addr = { .ss_family = 0 };
+ struct srv_pp_tlv_list *srv_tlv = NULL;
const struct sockaddr_storage *src = _addr;
const struct sockaddr_storage *dst = _addr;
- const char *value;
- int value_len;
+ const char *value = "";
+ int value_len = 0;
if (buf_len < PP2_HEADER_LEN)
return 0;
@@ -2000,6 +2001,37 @@ static int make_proxy_line_v2(char *buf, int buf_len,
struct server *srv, struct
}
}
+ if (strm) {
+ struct buffer *replace = NULL;
+
+ list_for_each_entry(srv_tlv, >pp_tlvs, list) {
+ replace = NULL;
+
+ /* Users will always need to provide a value, in case
of forwarding, they should use fc_pp_tlv.
+* for generic types. Otherwise, we will send an empty
TLV.
+*/
+ if (!LIST_ISEMPTY(_tlv->fmt)) {
+ replace = alloc_trash_chunk();
+ if (unlikely(!replace))
+ return 0;
+
+ replace->data = build_logline(strm,
replace->area, replace->size, _tlv->fmt);
+
+ if (unlikely((buf_len - ret) < sizeof(struct
tlv))) {
+ free_trash_chunk(replace);
+ return 0;
+ }
+ ret += make_tlv([ret], (buf_len - ret),
srv_tlv->type, replace->data, replace->area);
+ free_trash_chunk(replace);
+ }
+ else {
+ /* Create empty TLV as no value was specified */
+ ret += make_tlv([ret], (buf_len - ret),
srv_tlv->type, 0, NULL);
+ }
+ }
+ }
+
+ /* Handle predefined TLVs as usual */
if (srv->pp_opts & SRV_PP_V2_CRC32C) {
uint32_t zero_crc32c = 0;
--
2.35.3
-Original Message-
From: Willy Tarreau
Sent: Friday, October 27, 2023 4:22 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 Fri, Oct 27, 2023 at 02:12:10PM +, Stephan, Alexander wrote:
> > 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 r
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
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
>
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
*/
+ { "resolve-net", srv_parse_resolve_net, 1, 1, 0 },
/* Set the preferred network range for name resolution */
+ { "resolve-opts", srv_parse_resolve_opts, 1, 1, 0 },
/* Set options for name resolution */
+ { "resolve-prefer", srv_parse_resolve_prefer, 1, 1, 0 },
/* Set the preferred family for name resolution */
+ { "resolvers",srv_parse_resolvers,1, 1, 0 },
/* Configure the resolver to use for name resolution */
+ { "send-proxy", srv_parse_send_proxy, 0, 1, 1 },
/* Enforce use of PROXY V1 protocol */
+ { "send-proxy-v2",srv_parse_send_proxy_v2,0, 1, 1 },
/* Enforce use of PROXY V2 protocol */
+ { "set-proxy-v2-tlv-fmt", srv_parse_set_proxy_v2_tlv_fmt, 0, 1, 1 },
/* Set TLV of PROXY V2 protocol */
+ { "shard",srv_parse_shard,1, 1, 1 },
/* Server shard (only in peers protocol context) */
+ { "slowstart",srv_parse_slowstart,1, 1, 1 },
/* Set the warm-up timer for a previously failed server */
+ { "source", srv_parse_source, -1, 1, 1 },
/* Set the source address to be used to connect to the server */
+ { "stick",srv_parse_stick,0, 1, 0 },
/* Enable stick-table persistence */
+ { "tfo", srv_parse_tfo, 0, 1, 1 },
/* enable TCP Fast Open of server */
+ { "track",srv_parse_track,1, 1, 1 },
/* Set the current state of the server, tracking another one */
+ { "socks4", srv_parse_socks4, 1, 1, 0 },
/* Set the socks4 proxy of the server*/
+ { "usesrc", srv_parse_usesrc, 0, 1, 1 },
/* safe-guard against usesrc without preceding keyword */
+ { "weight", srv_parse_weight, 1, 1, 1 },
/* Set the load-balancing weight */
{ NULL, NULL, 0 },
}};
-Original Message-
From: Willy Tarreau
Sent: Wednesday, October 18, 2023 9:33 AM
To: Stephan, Alexander
Cc: haproxy@formilux.org
Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated
proxy-v2-options
Hi Alexander,
I'm starting from the doc as it eases the discussion.
On Thu, Oct 05, 2023 at 11:05:50AM +, Stephan, Alexander wrote:
> --- a/doc/configuration.txt
> +++ b/doc/configuration.txt
> @@ -16671,6 +16671,26 @@ proxy-v2-options [,]*
> generated unique ID is also used for the first HTTP request
> within a Keep-Alive connection.
>
> + Besides, you can specify the type of TLV numerically instead.
> +
> + Example 1:
> + server example 127.0.0.1:2319 send-proxy-v2 proxy-v2-options
> + crc32c,0xEE
> +
> + The following logic applies: By default, the remote frontend
> + connection is searched for the value 0xEE. If found, it is simply
> + forwarded. Otherwise, a TLV with the ID "0xEE" and empty payload is
> + sent out. In addition, crc32c is also set here.
> +
> + You can also specify a key-value pair = syntax.
> + represents the
> + 8 bit TLV type field in the range of 0 to 255. It can be expressed
> + in decimal or hexadecimal format (prefixed by "0x").
> +
> + Example 2:
> + server example_server 127.0.0.1:2319 send-proxy-v2 proxy-v2-options
> + 0xEE=%[str("foo")]
> +
> + This will always send out the value "foo". Another common use case
> + would be to reference a variable.
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. Also what will be left in the logs after you modify the contents ?
etc.
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 woul
RE: [PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs
Hi Willy, Do you know whether this can/will make it to the next release? It would be crucial for us to know. Best, Alexander -Original Message- From: Willy Tarreau Sent: Thursday, October 5, 2023 2:42 PM To: Stephan, Alexander Cc: haproxy@formilux.org Subject: Re: [PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs Hi Alexander, On Thu, Oct 05, 2023 at 11:13:16AM +, Stephan, Alexander wrote: > Hi Willy, > > Ah, what a pity. Anyway, I sent them again with you in CC. Does it look > alright now? Yep, received both ways this time, thank you! Willy
RE: [PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs
Hi Willy, Ah, what a pity. Anyway, I sent them again with you in CC. Does it look alright now? Best, Alexander -Original Message- From: Willy Tarreau Sent: Wednesday, October 4, 2023 3:21 PM To: Stephan, Alexander Cc: haproxy@formilux.org Subject: Re: [PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs Hi Alexander, On Wed, Oct 04, 2023 at 12:56:07PM +, Stephan, Alexander wrote: > Can you find them if you search for the text that is shown in the archive? Not at all, not even in the spam logs :-( Please note that we've had a short outage from an haproxy core filling the whole FS ~10 days ago, I don't know if that could have matched, but in any case at least you should have received a delivery error. > If not, I can of course send them again. Yes please do and CC me so that I know when they're sent and can compare if I continue not to see them on the ML. Thank you! Willy
RE: [PATCH 4/4] MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs
From da4dc50153fe6cc7e562b63439dd8be4846e0dcf Mon Sep 17 00:00:00 2001
From: Alexander Stephan
mailto:alexander.step...@sap.com>>
Date: Fri, 15 Sep 2023 12:25:03 +0200
Subject: [PATCH 4/4] MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs
This commit adds an action called set-tlv() that allows to directly
update the TLV data structure within a connection for all type of connection
events. It can be used to modify TLVs before they are forwarded (if specified
in proxy-v2-options) while keeping the previously allocated memory, if the new
and the old value map to the same pool. This function can also be used to
enhance readability if setting many TLVs at once, as an alternative to
specifying
type and value directly in the server.
---
doc/configuration.txt | 25 +++-
.../proxy_protocol_send_generic.vtc | 31 +
src/tcp_act.c | 120 --
3 files changed, 161 insertions(+), 15 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index aeff9e4db..a0317f005 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -7011,6 +7011,7 @@ http-request [options...] [ { if | unless }
]
- set-src
- set-src-port
- set-timeout { server | tunnel } { | }
+- set-tlv()
- set-tos
- set-uri
- set-var([,...])
@@ -7943,6 +7944,22 @@ http-request set-timeout { server | tunnel } {
| }
http-request set-timeout tunnel 5s
http-request set-timeout server req.hdr(host),map_int(host.lst)
+http-request set-tlv() [ { if | unless } ]
+
+ This is used to alter a PROXY protocol v2 TLV that has been sent by the
client.
+ It can be used to efficiently alter already allocated TLVs in-place. If no
TLV with
+ the specified TLV ID has been received yet, a new TLV with and the
current
+ value of is added.
+
+ The parameter represents the 8 bit TLV type field in the range 0 to 255.
+ It can be expressed in decimal, hexadecimal format (prefixed by "0x") or
octal
+ (prefixed by "0").
+
+ Typically, it is used together with generic proxy-v2-options.
+
+ Example:
+http-request set-tlv(0xE1) str("foo")
+
http-request set-tos [ { if | unless } ]
This is used to set the TOS or DSCP field value of packets sent to the client
@@ -13502,6 +13519,7 @@ tcp-request content [{if | unless} ]
- set-priority-offset
- set-src
- set-src-port
+- set-tlv()
- set-tos
- set-var([,...])
- set-var-fmt([,...])
@@ -13741,6 +13759,11 @@ tcp-request content set-src-port [ { if |
unless } ]
specified expression. Please refer to "http-request set-src" and
"http-request set-src-port" for a complete description.
+tcp-request content set-tlv() [ { if | unless } ]
+
+ This is used to alter a PROXY protocol v2 TLV that has been sent by the
client.
+ Please refer to "http-request set-tlv" for a complete description.
+
tcp-request content set-tos [ { if | unless } ]
This is used to set the TOS or DSCP field value of packets sent to the client
@@ -16686,7 +16709,7 @@ proxy-v2-options [,]*
or hexadecimal format (prefixed by "0x").
Example 2:
- server example_server 127.0.0.1:2319 send-proxy-v2 proxy-v2-options
0xEE=%[str("foo")]
+ server example 127.0.0.1:2319 send-proxy-v2 proxy-v2-options
0xEE=%[str("foo")]
This will always send out the value "foo". Another common use case would be
to
reference a variable.
diff --git a/reg-tests/connection/proxy_protocol_send_generic.vtc
b/reg-tests/connection/proxy_protocol_send_generic.vtc
index e0bd15a1b..1c48964be 100644
--- a/reg-tests/connection/proxy_protocol_send_generic.vtc
+++ b/reg-tests/connection/proxy_protocol_send_generic.vtc
@@ -24,6 +24,33 @@ haproxy h1 -conf {
http-request set-var(txn.custom_tlv_b) fc_pp_tlv(0xE2)
http-after-response set-header proxy_custom_tlv_b
%[var(txn.custom_tlv_b)]
+http-request set-tlv(0xE3) str("bar")
+http-request set-var(txn.custom_tlv_c) fc_pp_tlv(0xE3)
+http-after-response set-header proxy_custom_tlv_c
%[var(txn.custom_tlv_c)]
+
+# Check that we can alter the TLV in the connection on http-request
level.
+http-request set-tlv(0xE3) str("bar")
+http-request set-var(txn.custom_tlv_c) fc_pp_tlv(0xE3)
+http-after-response set-header proxy_custom_tlv_c
%[var(txn.custom_tlv_c)]
+
+# Check that we can alter the TLV in the connection on tcp-content
level.
+tcp-request content set-tlv(0xE4) str("bar")
+http-request set-var(txn.custom_tlv_d) fc_pp_tlv(0xE4)
+http-after-response set-header proxy_custom_tlv_d
%[var(txn.custom_tlv_d)]
+
+# Check that we can overwrite an existing TLV.
+tcp-request content set-tlv(0xE5) str("bar")
+http-request set-var(txn.custom_tlv_e) fc_pp_tlv(0xE5)
+http-after-response set-header proxy_custom_tlv_e
%[var(txn.custom_tlv_e)]
+
+# Check that we can
RE: [PATCH 3/4] LOW: connection: Add TLV update function
From cc8fe58a8d2f8d47b03d03fd1048fe1b9babca70 Mon Sep 17 00:00:00 2001
From: Alexander Stephan
mailto:alexander.step...@sap.com>>
Date: Fri, 15 Sep 2023 12:18:10 +0200
Subject: [PATCH 3/4] LOW: connection: Add TLV update function
Until now, it was not possible to deliberatily change received TLVs
that are stored within a connection. An HAProxy backend server reads
TLVs out the (remote) connection. This function allows us to update
TLVs in-place before they are forwarded, given that the new and the
old value map to the same pool. Besides, if a TLV does not already
exist, it is created and added to the list. As TLV values often have
similiar length this is more efficient than allocating a new TLV for
each value.
---
include/haproxy/connection.h | 73
1 file changed, 73 insertions(+)
diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index fb14eed12..b775996ba 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -451,6 +451,79 @@ static inline void conn_set_tos(const struct connection
*conn, int tos)
#endif
}
+/* Adds or updates a TLV item on an existing connection.
+ * The connection is tested and if it is null, nothing is done.
+ */
+static inline int conn_set_tlv(struct connection *conn, int type, void *value,
int len)
+{
+ struct conn_tlv_list *tlv = NULL, *tlv_back = NULL;
+ int reuse = 0, found = 0;
+
+ if (!conn || !conn_ctrl_ready(conn) || len > HA_PP2_MAX_ALLOC)
+ return -1;
+ /* search whether we already have a TLV of the requested type */
+ list_for_each_entry(tlv, >tlv_list, list) {
+ if (tlv->type != type)
+ continue;
+ /* Set reuse flag according to whether both, new and old TLV,
+* are in the same interval.
+*/
+ if (len >= 0) {
+ if (tlv->len <= HA_PP2_TLV_VALUE_128 && len <= HA_PP2_TLV_VALUE_128)
+ reuse = 1;
+ else if (tlv->len > HA_PP2_TLV_VALUE_128 && len > HA_PP2_TLV_VALUE_128) {
+ if ((tlv->len <= HA_PP2_TLV_VALUE_256 && len <= HA_PP2_TLV_VALUE_256) ||
+ (tlv->len > HA_PP2_TLV_VALUE_256 && len > HA_PP2_TLV_VALUE_256))
+ reuse = 1;
+ }
+ }
+ found = 1;
+ break;
+ }
+ if (found && reuse) {
+ /* We have found the TLV and it fits in the already allocated
+* memory, in-place editing is enough. freeing logic stays
+* identical since the length still maps to the old pool.
+*/
+ memcpy(tlv->value, value, len);
+ tlv->len = len;
+ return 0;
+ }
+
+ if (found) {
+ /* first, we need to free the previous TLV item */
+ list_for_each_entry_safe(tlv, tlv_back, >tlv_list, list) {
+ if (tlv->type != type)
+ continue;
+
+ LIST_DELETE(>list);
+ if (tlv->len > HA_PP2_TLV_VALUE_256)
+ free(tlv);
+ else if (tlv->len < HA_PP2_TLV_VALUE_128)
+ pool_free(pool_head_pp_tlv_128, tlv);
+ else
+ pool_free(pool_head_pp_tlv_256, tlv);
+ }
+ }
+
+ if (len > HA_PP2_TLV_VALUE_256)
+ tlv = malloc(len + sizeof(struct conn_tlv_list));
+ else if (len <= HA_PP2_TLV_VALUE_128)
+ tlv = pool_alloc(pool_head_pp_tlv_128);
+ else
+ tlv = pool_alloc(pool_head_pp_tlv_256);
+
+ if (unlikely(!tlv))
+ return -1;
+
+ tlv->len = len;
+ tlv->type = type;
+ memcpy(tlv->value, value, len);
+
+ LIST_APPEND(>tlv_list, >list);
+ return 0;
+}
+
/* Sets the netfilter mark on the connection's socket. The connection is tested
* and if it is null, nothing is done.
*/
--
2.35.3
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
From 84608ed754c1a92e85e03036e8b0cd0949721ffb Mon Sep 17 00:00:00 2001
From: Alexander Stephan
mailto:alexander.step...@sap.com>>
Date: Fri, 15 Sep 2023 12:42:36 +0200
Subject: [PATCH 2/4] MEDIUM: connection: Send out generically allocated
proxy-v2-options
This commit removes the previous limitations on the existing, fixed PPv2 TLV
types
that can be sent out. This is done by leveraging the previously implemented
parsing. We iterate over the server TLV list and either forward or send a new
TLV, depending on whether a TLV exists in the remote connection.
---
doc/configuration.txt | 20
.../proxy_protocol_send_generic.vtc | 35 +
src/connection.c | 51 +--
3 files changed, 103 insertions(+), 3 deletions(-)
create mode 100644 reg-tests/connection/proxy_protocol_send_generic.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 14f6b906d..aeff9e4db 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -16671,6 +16671,26 @@ proxy-v2-options [,]*
generated unique ID is also used for the first HTTP request
within a Keep-Alive connection.
+ Besides, you can specify the type of TLV numerically instead.
+
+ Example 1:
+ server example 127.0.0.1:2319 send-proxy-v2 proxy-v2-options crc32c,0xEE
+
+ The following logic applies: By default, the remote frontend connection is
+ searched for the value 0xEE. If found, it is simply forwarded. Otherwise,
+ a TLV with the ID "0xEE" and empty payload is sent out. In addition, crc32c
+ is also set here.
+
+ You can also specify a key-value pair = syntax. represents the
+ 8 bit TLV type field in the range of 0 to 255. It can be expressed in decimal
+ or hexadecimal format (prefixed by "0x").
+
+ Example 2:
+ server example_server 127.0.0.1:2319 send-proxy-v2 proxy-v2-options
0xEE=%[str("foo")]
+
+ This will always send out the value "foo". Another common use case would be
to
+ reference a variable.
+
send-proxy-v2-ssl
The "send-proxy-v2-ssl" parameter enforces use of the PROXY protocol version
2 over any connection established to this server. The PROXY protocol informs
diff --git a/reg-tests/connection/proxy_protocol_send_generic.vtc
b/reg-tests/connection/proxy_protocol_send_generic.vtc
new file mode 100644
index 0..e0bd15a1b
--- /dev/null
+++ b/reg-tests/connection/proxy_protocol_send_generic.vtc
@@ -0,0 +1,35 @@
+varnishtest "Check that generic TLV IDs are sent properly"
+
+#REQUIRE_VERSION=2.2
+
+feature ignore_unknown_macro
+
+haproxy h1 -conf {
+defaults
+mode http
+log global
+
+listen sender
+bind "fd@${feS}"
+server example ${h1_feR_addr}:${h1_feR_port} send-proxy-v2
proxy-v2-options 0xE1=%[str("foo")],0xE2
+
+listen receiver
+bind "fd@${feR}" accept-proxy
+
+# Check that the TLV value is set in the backend.
+http-request set-var(txn.custom_tlv_a) fc_pp_tlv(0xE1)
+http-after-response set-header proxy_custom_tlv_a
%[var(txn.custom_tlv_a)]
+
+# Check that TLVs without an value are sent out.
+http-request set-var(txn.custom_tlv_b) fc_pp_tlv(0xE2)
+http-after-response set-header proxy_custom_tlv_b
%[var(txn.custom_tlv_b)]
+
+http-request return status 200
+} -start
+
+client c1 -connect ${h1_feS_sock} {
+txreq -url "/"
+rxresp
+expect resp.http.proxy_custom_tlv_a == "foo"
+expect resp.http.proxy_custom_tlv_b == ""
+} -run
diff --git a/src/connection.c b/src/connection.c
index 5f7226aae..51584b1ed 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -548,7 +549,7 @@ struct connection *conn_new(void *target)
/* Releases a connection previously allocated by conn_new() */
void conn_free(struct connection *conn)
{
- struct conn_tlv_list *tlv, *tlv_back = NULL;
+ struct conn_tlv_list *tlv = NULL, *tlv_back = NULL;
if (conn_is_back(conn))
conn_backend_deinit(conn);
@@ -1920,10 +1921,12 @@ static int make_proxy_line_v2(char *buf, int buf_len,
struct server *srv, struct
int ret = 0;
struct proxy_hdr_v2 *hdr = (struct proxy_hdr_v2 *)buf;
struct sockaddr_storage null_addr = { .ss_family = 0 };
+ struct srv_pp_tlv_list *srv_tlv = NULL;
const struct sockaddr_storage *src = _addr;
const struct sockaddr_storage *dst = _addr;
- const char *value;
- int value_len;
+ const char *value = "";
+ int value_len = 0;
+ int found = 0;
if (buf_len < PP2_HEADER_LEN)
return 0;
@@ -1993,6 +1996,48 @@ static int make_proxy_line_v2(char *buf, int buf_len,
struct server *srv, struct
}
}
+ if (strm) {
+ struct buffer *replace = NULL;
+
+ list_for_each_entry(srv_tlv, >pp_tlvs, list) {
+ replace = NULL;
+
+ if (!LIST_ISEMPTY(_tlv->fmt)) {
+ replace =
RE: [PATCH 1/4] MEDIUM: server: Parse generic type-value pairs as proxy-v2-options
From fb8714c5aebd7fe957264d0f2234182f55f952fe Mon Sep 17 00:00:00 2001
From: Alexander Stephan
Date: Fri, 15 Sep 2023 12:38:46 +0200
Subject: [PATCH 1/4] MEDIUM: server: Parse generic type-value pairs as
proxy-v2-options
This commit introduces a generic server-side parsing of type-value pair
arguments and allocation of a TLV list. This allows to 1) forward any TLV
type, 2) generally send out any TLV type, and 3) allows to specify concrete
values via an '=' after the type and a log fmt expression. If there is no
TLV found in the remote connection, an empty TLV is sent out.
---
include/haproxy/server-t.h | 10 +
src/server.c | 76 +++---
2 files changed, 81 insertions(+), 5 deletions(-)
diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 1175a470e..7b93bf48f 100644
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -256,6 +256,15 @@ enum __attribute__((__packed__)) srv_ws_mode {
SRV_WS_H2,
};
+/* Server-side TLV list, contains the types of the TLVs that should be sent
out.
+ * Additionally, it can contain data, if specified in the config.
+ */
+struct srv_pp_tlv_list {
+ struct list list;
+ struct list fmt;
+ unsigned char type;
+};
+
struct proxy;
struct server {
/* mostly config or admin stuff, doesn't change often */
@@ -421,6 +430,7 @@ struct server {
struct list srv_rec_item; /* to attach server to a srv record item */
struct list ip_rec_item;/* to attach server to a A or record
item */
struct ebpt_node host_dn; /* hostdn store for srvrq and state file
matching*/
+ struct list pp_tlvs;/* to send out PROXY protocol v2 TLVs */
struct task *srvrq_check; /* Task testing SRV record
expiration date for this server */
struct {
const char *file; /* file where the section appears */
diff --git a/src/server.c b/src/server.c
index 3673340d1..08f86d030 100644
--- a/src/server.c
+++ b/src/server.c
@@ -1036,7 +1036,13 @@ static int srv_parse_proto(char **args, int *cur_arg,
static int srv_parse_proxy_v2_options(char **args, int *cur_arg,
struct proxy *px, struct server *newsrv, char **err)
{
- char *p, *n;
+ char *p = NULL, *n = NULL, *m = NULL;
+ char *key = NULL, *val = NULL;
+ char *endp = NULL;
+ unsigned char idx = 0;
+ size_t key_length = 0, val_length = 0;
+ struct srv_pp_tlv_list *srv_tlv = NULL;
+
for (p = args[*cur_arg+1]; p; p = n) {
n = strchr(p, ',');
if (n)
@@ -1061,13 +1067,61 @@ static int srv_parse_proxy_v2_options(char **args, int
*cur_arg,
newsrv->pp_opts |= SRV_PP_V2_CRC32C;
} else if (strcmp(p, "unique-id") == 0) {
newsrv->pp_opts |= SRV_PP_V2_UNIQUE_ID;
- } else
- goto fail;
+ } else {
+ /* reset val in case it was set before */
+ val = NULL;
+
+ /* try to split by '=' into generic pair key value pair
(=) */
+ m = strchr(p, '=');
+ if (m) {
+ key_length = m - p;
+ key = (char *) malloc(key_length + 1);
+ if (unlikely(!key))
+ goto fail;
+ snprintf(key, key_length + 1, "%s", p);
+
+ val_length = strlen(p) - key_length;
+ val = (char *) malloc(val_length + 1);
+ if (unlikely(!val))
+ goto fail;
+ snprintf(val, val_length + 1, "%s", m + 1);
+ }
+ else {
+ key = (char *) malloc(strlen(p) + 1);
+ if (unlikely(!key))
+ goto fail;
+ snprintf(key, strlen(p) + 1, "%s", p);
+ }
+
+ errno = 0;
+ idx = strtoul(key, , 0);
+ if (unlikely((endp && *endp) != '\0' || (key == endp) || (errno !=
0)))
+ goto fail;
+
+ /* processing is done in connection.c */
+ srv_tlv = malloc(sizeof(struct srv_pp_tlv_list));
+ if (unlikely(!srv_tlv))
+ goto fail;
+
+ srv_tlv->type = idx;
+ LIST_INIT(_tlv->fmt);
+ if (val && unlikely(!parse_logformat_string(val, px, _tlv->fmt,
LOG_OPT_HTTP, PR_CAP_LISTEN, err)))
+ goto fail;
+
+ LIST_APPEND(>pp_tlvs, _tlv->list);
+ free(key);
+ free(val);
+ }
}
return 0;
- fail:
+fail:
+ free(key);
+ free(val);
+ free(srv_tlv);
+ errno = 0;
+
if (err)
- memprintf(err, "'%s' : proxy v2 option not implemented", p);
+ memprintf(err, "'%s' : failed to set proxy v2 option", p);
return ERR_ALERT | ERR_FATAL;
}
@@ -2428,6 +2482,7 @@ struct server *new_server(struct proxy *proxy)
LIST_APPEND(_list, >global_list);
LIST_INIT(>srv_rec_item);
LIST_INIT(>ip_rec_item);
+ LIST_INIT(>pp_tlvs);
MT_LIST_INIT(>prev_deleted);
event_hdl_sub_list_init(>e_subs);
srv->rid = 0; /* rid defaults to 0 */
@@
RE: [PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs
Hi Willy, Maybe it is because I replied to cover letter and not to the individual patches? This time, I sent them as separate mails, with no attachment. The patches should be responses to the cover letter I replied to in the previous mail. Here are the links to the patches again: [PATCH 1/4] MEDIUM: server: Parse generic type-value pairs as proxy-v2-options: https://www.mail-archive.com/haproxy@formilux.org/msg44023.html [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options: https://www.mail-archive.com/haproxy@formilux.org/msg44024.html [PATCH 3/4] LOW: connection: Add TLV update function: https://www.mail-archive.com/haproxy@formilux.org/msg44025.html [PATCH 4/4] MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs: https://www.mail-archive.com/haproxy@formilux.org/msg44026.html Can you find them if you search for the text that is shown in the archive? If not, I can of course send them again. Best, Alexander -Original Message- From: Willy Tarreau Sent: Tuesday, October 3, 2023 11:27 AM To: Stephan, Alexander Cc: haproxy@formilux.org Subject: Re: [PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs Hi Alexander, On Mon, Oct 02, 2023 at 10:22:17AM +, Stephan, Alexander wrote: > Hi, > > I am back from my vacation and I wanted to ask whether somebody has had the > chance to look at this. > A short ACK that the changes will be reviewed would be much appreciated. I'm sorry but I can't find any trace of the original. Could you please resend it ? thank you! Willy
RE: [PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs
Hi, I am back from my vacation and I wanted to ask whether somebody has had the chance to look at this. A short ACK that the changes will be reviewed would be much appreciated. Best, Alexander SAP SE Germany From: Stephan, Alexander Sent: Friday, September 15, 2023 6:39 PM To: haproxy@formilux.org Cc: Menges, Christian Norbert Subject: [PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs Hi all, As a follow-up to my last patches that allow to fetch any TLV in the frontend, I have also implemented similar functionality in the backend that allows to send out TLVs of any type. This is an addition to the existing proxy-protocol-v2-option constants like crc32c. Unlike the pre-defined TLVs, generic TLVs, indicated by their type number, have a common behavior. I think the test I've added as well as the documentation should explain best how it works in more detail. To update TLVs in-place in a connection, before forwarding, I have also added a corresponding TCP action. PPv2 is not quite TCP level, but it seems like the best fit. I am very interested in what you think about the changes. Essentially, these are two commits split into four, for easier review (split by component). I will be on vacation the next two weeks, but my colleague Christian Menges will jump in during that time and address your feedback. Thanks and Best, Alexander SAP SE Germany --- Alexander Stephan (4): MEDIUM: server: Parse generic type-value pairs as proxy-v2-options MEDIUM: connection: Send out generically allocated proxy-v2-options LOW: connection: Add TLV update function MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs doc/configuration.txt | 43 +++ include/haproxy/connection.h | 73 +++ include/haproxy/server-t.h| 10 ++ .../proxy_protocol_send_generic.vtc | 66 ++ src/connection.c | 51 +++- src/server.c | 76 ++- src/tcp_act.c | 120 -- 7 files changed, 417 insertions(+), 22 deletions(-) create mode 100644 reg-tests/connection/proxy_protocol_send_generic.vtc -- 2.35.3
RE: [PATCH 4/4] MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs
From da4dc50153fe6cc7e562b63439dd8be4846e0dcf Mon Sep 17 00:00:00 2001
From: Alexander Stephan
Date: Fri, 15 Sep 2023 12:25:03 +0200
Subject: [PATCH 4/4] MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs
This commit adds an action called set-tlv() that allows to directly
update the TLV data structure within a connection for all type of connection
events. It can be used to modify TLVs before they are forwarded (if specified
in proxy-v2-options) while keeping the previously allocated memory, if the new
and the old value map to the same pool. This function can also be used to
enhance readability if setting many TLVs at once, as an alternative to
specifying
type and value directly in the server.
---
doc/configuration.txt | 25 +++-
.../proxy_protocol_send_generic.vtc | 31 +
src/tcp_act.c | 120 --
3 files changed, 161 insertions(+), 15 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index aeff9e4db..a0317f005 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -7011,6 +7011,7 @@ http-request [options...] [ { if | unless }
]
- set-src
- set-src-port
- set-timeout { server | tunnel } { | }
+- set-tlv()
- set-tos
- set-uri
- set-var([,...])
@@ -7943,6 +7944,22 @@ http-request set-timeout { server | tunnel } {
| }
http-request set-timeout tunnel 5s
http-request set-timeout server req.hdr(host),map_int(host.lst)
+http-request set-tlv() [ { if | unless } ]
+
+ This is used to alter a PROXY protocol v2 TLV that has been sent by the
client.
+ It can be used to efficiently alter already allocated TLVs in-place. If no
TLV with
+ the specified TLV ID has been received yet, a new TLV with and the
current
+ value of is added.
+
+ The parameter represents the 8 bit TLV type field in the range 0 to 255.
+ It can be expressed in decimal, hexadecimal format (prefixed by "0x") or
octal
+ (prefixed by "0").
+
+ Typically, it is used together with generic proxy-v2-options.
+
+ Example:
+http-request set-tlv(0xE1) str("foo")
+
http-request set-tos [ { if | unless } ]
This is used to set the TOS or DSCP field value of packets sent to the client
@@ -13502,6 +13519,7 @@ tcp-request content [{if | unless} ]
- set-priority-offset
- set-src
- set-src-port
+- set-tlv()
- set-tos
- set-var([,...])
- set-var-fmt([,...])
@@ -13741,6 +13759,11 @@ tcp-request content set-src-port [ { if |
unless } ]
specified expression. Please refer to "http-request set-src" and
"http-request set-src-port" for a complete description.
+tcp-request content set-tlv() [ { if | unless } ]
+
+ This is used to alter a PROXY protocol v2 TLV that has been sent by the
client.
+ Please refer to "http-request set-tlv" for a complete description.
+
tcp-request content set-tos [ { if | unless } ]
This is used to set the TOS or DSCP field value of packets sent to the client
@@ -16686,7 +16709,7 @@ proxy-v2-options [,]*
or hexadecimal format (prefixed by "0x").
Example 2:
- server example_server 127.0.0.1:2319 send-proxy-v2 proxy-v2-options
0xEE=%[str("foo")]
+ server example 127.0.0.1:2319 send-proxy-v2 proxy-v2-options
0xEE=%[str("foo")]
This will always send out the value "foo". Another common use case would be
to
reference a variable.
diff --git a/reg-tests/connection/proxy_protocol_send_generic.vtc
b/reg-tests/connection/proxy_protocol_send_generic.vtc
index e0bd15a1b..1c48964be 100644
--- a/reg-tests/connection/proxy_protocol_send_generic.vtc
+++ b/reg-tests/connection/proxy_protocol_send_generic.vtc
@@ -24,6 +24,33 @@ haproxy h1 -conf {
http-request set-var(txn.custom_tlv_b) fc_pp_tlv(0xE2)
http-after-response set-header proxy_custom_tlv_b
%[var(txn.custom_tlv_b)]
+http-request set-tlv(0xE3) str("bar")
+http-request set-var(txn.custom_tlv_c) fc_pp_tlv(0xE3)
+http-after-response set-header proxy_custom_tlv_c
%[var(txn.custom_tlv_c)]
+
+# Check that we can alter the TLV in the connection on http-request
level.
+http-request set-tlv(0xE3) str("bar")
+http-request set-var(txn.custom_tlv_c) fc_pp_tlv(0xE3)
+http-after-response set-header proxy_custom_tlv_c
%[var(txn.custom_tlv_c)]
+
+# Check that we can alter the TLV in the connection on tcp-content
level.
+tcp-request content set-tlv(0xE4) str("bar")
+http-request set-var(txn.custom_tlv_d) fc_pp_tlv(0xE4)
+http-after-response set-header proxy_custom_tlv_d
%[var(txn.custom_tlv_d)]
+
+# Check that we can overwrite an existing TLV.
+tcp-request content set-tlv(0xE5) str("bar")
+http-request set-var(txn.custom_tlv_e) fc_pp_tlv(0xE5)
+http-after-response set-header proxy_custom_tlv_e
%[var(txn.custom_tlv_e)]
+
+# Check that we can move from a small to a medium pool
RE: [PATCH 3/4] LOW: connection: Add TLV update function
From cc8fe58a8d2f8d47b03d03fd1048fe1b9babca70 Mon Sep 17 00:00:00 2001
From: Alexander Stephan
Date: Fri, 15 Sep 2023 12:18:10 +0200
Subject: [PATCH 3/4] LOW: connection: Add TLV update function
Until now, it was not possible to deliberatily change received TLVs
that are stored within a connection. An HAProxy backend server reads
TLVs out the (remote) connection. This function allows us to update
TLVs in-place before they are forwarded, given that the new and the
old value map to the same pool. Besides, if a TLV does not already
exist, it is created and added to the list. As TLV values often have
similiar length this is more efficient than allocating a new TLV for
each value.
---
include/haproxy/connection.h | 73
1 file changed, 73 insertions(+)
diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index fb14eed12..b775996ba 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -451,6 +451,79 @@ static inline void conn_set_tos(const struct connection
*conn, int tos)
#endif
}
+/* Adds or updates a TLV item on an existing connection.
+ * The connection is tested and if it is null, nothing is done.
+ */
+static inline int conn_set_tlv(struct connection *conn, int type, void *value,
int len)
+{
+ struct conn_tlv_list *tlv = NULL, *tlv_back = NULL;
+ int reuse = 0, found = 0;
+
+ if (!conn || !conn_ctrl_ready(conn) || len > HA_PP2_MAX_ALLOC)
+ return -1;
+ /* search whether we already have a TLV of the requested type */
+ list_for_each_entry(tlv, >tlv_list, list) {
+ if (tlv->type != type)
+ continue;
+ /* Set reuse flag according to whether both, new and old TLV,
+* are in the same interval.
+*/
+ if (len >= 0) {
+ if (tlv->len <= HA_PP2_TLV_VALUE_128 && len <= HA_PP2_TLV_VALUE_128)
+ reuse = 1;
+ else if (tlv->len > HA_PP2_TLV_VALUE_128 && len > HA_PP2_TLV_VALUE_128) {
+ if ((tlv->len <= HA_PP2_TLV_VALUE_256 && len <= HA_PP2_TLV_VALUE_256) ||
+ (tlv->len > HA_PP2_TLV_VALUE_256 && len > HA_PP2_TLV_VALUE_256))
+ reuse = 1;
+ }
+ }
+ found = 1;
+ break;
+ }
+ if (found && reuse) {
+ /* We have found the TLV and it fits in the already allocated
+* memory, in-place editing is enough. freeing logic stays
+* identical since the length still maps to the old pool.
+*/
+ memcpy(tlv->value, value, len);
+ tlv->len = len;
+ return 0;
+ }
+
+ if (found) {
+ /* first, we need to free the previous TLV item */
+ list_for_each_entry_safe(tlv, tlv_back, >tlv_list, list) {
+ if (tlv->type != type)
+ continue;
+
+ LIST_DELETE(>list);
+ if (tlv->len > HA_PP2_TLV_VALUE_256)
+ free(tlv);
+ else if (tlv->len < HA_PP2_TLV_VALUE_128)
+ pool_free(pool_head_pp_tlv_128, tlv);
+ else
+ pool_free(pool_head_pp_tlv_256, tlv);
+ }
+ }
+
+ if (len > HA_PP2_TLV_VALUE_256)
+ tlv = malloc(len + sizeof(struct conn_tlv_list));
+ else if (len <= HA_PP2_TLV_VALUE_128)
+ tlv = pool_alloc(pool_head_pp_tlv_128);
+ else
+ tlv = pool_alloc(pool_head_pp_tlv_256);
+
+ if (unlikely(!tlv))
+ return -1;
+
+ tlv->len = len;
+ tlv->type = type;
+ memcpy(tlv->value, value, len);
+
+ LIST_APPEND(>tlv_list, >list);
+ return 0;
+}
+
/* Sets the netfilter mark on the connection's socket. The connection is tested
* and if it is null, nothing is done.
*/
--
2.35.3
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
From 84608ed754c1a92e85e03036e8b0cd0949721ffb Mon Sep 17 00:00:00 2001
From: Alexander Stephan
Date: Fri, 15 Sep 2023 12:42:36 +0200
Subject: [PATCH 2/4] MEDIUM: connection: Send out generically allocated
proxy-v2-options
This commit removes the previous limitations on the existing, fixed PPv2 TLV
types
that can be sent out. This is done by leveraging the previously implemented
parsing. We iterate over the server TLV list and either forward or send a new
TLV, depending on whether a TLV exists in the remote connection.
---
doc/configuration.txt | 20
.../proxy_protocol_send_generic.vtc | 35 +
src/connection.c | 51 +--
3 files changed, 103 insertions(+), 3 deletions(-)
create mode 100644 reg-tests/connection/proxy_protocol_send_generic.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 14f6b906d..aeff9e4db 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -16671,6 +16671,26 @@ proxy-v2-options [,]*
generated unique ID is also used for the first HTTP request
within a Keep-Alive connection.
+ Besides, you can specify the type of TLV numerically instead.
+
+ Example 1:
+ server example 127.0.0.1:2319 send-proxy-v2 proxy-v2-options crc32c,0xEE
+
+ The following logic applies: By default, the remote frontend connection is
+ searched for the value 0xEE. If found, it is simply forwarded. Otherwise,
+ a TLV with the ID "0xEE" and empty payload is sent out. In addition, crc32c
+ is also set here.
+
+ You can also specify a key-value pair = syntax. represents the
+ 8 bit TLV type field in the range of 0 to 255. It can be expressed in decimal
+ or hexadecimal format (prefixed by "0x").
+
+ Example 2:
+ server example_server 127.0.0.1:2319 send-proxy-v2 proxy-v2-options
0xEE=%[str("foo")]
+
+ This will always send out the value "foo". Another common use case would be
to
+ reference a variable.
+
send-proxy-v2-ssl
The "send-proxy-v2-ssl" parameter enforces use of the PROXY protocol version
2 over any connection established to this server. The PROXY protocol informs
diff --git a/reg-tests/connection/proxy_protocol_send_generic.vtc
b/reg-tests/connection/proxy_protocol_send_generic.vtc
new file mode 100644
index 0..e0bd15a1b
--- /dev/null
+++ b/reg-tests/connection/proxy_protocol_send_generic.vtc
@@ -0,0 +1,35 @@
+varnishtest "Check that generic TLV IDs are sent properly"
+
+#REQUIRE_VERSION=2.2
+
+feature ignore_unknown_macro
+
+haproxy h1 -conf {
+defaults
+mode http
+log global
+
+listen sender
+bind "fd@${feS}"
+server example ${h1_feR_addr}:${h1_feR_port} send-proxy-v2
proxy-v2-options 0xE1=%[str("foo")],0xE2
+
+listen receiver
+bind "fd@${feR}" accept-proxy
+
+# Check that the TLV value is set in the backend.
+http-request set-var(txn.custom_tlv_a) fc_pp_tlv(0xE1)
+http-after-response set-header proxy_custom_tlv_a
%[var(txn.custom_tlv_a)]
+
+# Check that TLVs without an value are sent out.
+http-request set-var(txn.custom_tlv_b) fc_pp_tlv(0xE2)
+http-after-response set-header proxy_custom_tlv_b
%[var(txn.custom_tlv_b)]
+
+http-request return status 200
+} -start
+
+client c1 -connect ${h1_feS_sock} {
+txreq -url "/"
+rxresp
+expect resp.http.proxy_custom_tlv_a == "foo"
+expect resp.http.proxy_custom_tlv_b == ""
+} -run
diff --git a/src/connection.c b/src/connection.c
index 5f7226aae..51584b1ed 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -548,7 +549,7 @@ struct connection *conn_new(void *target)
/* Releases a connection previously allocated by conn_new() */
void conn_free(struct connection *conn)
{
- struct conn_tlv_list *tlv, *tlv_back = NULL;
+ struct conn_tlv_list *tlv = NULL, *tlv_back = NULL;
if (conn_is_back(conn))
conn_backend_deinit(conn);
@@ -1920,10 +1921,12 @@ static int make_proxy_line_v2(char *buf, int buf_len,
struct server *srv, struct
int ret = 0;
struct proxy_hdr_v2 *hdr = (struct proxy_hdr_v2 *)buf;
struct sockaddr_storage null_addr = { .ss_family = 0 };
+ struct srv_pp_tlv_list *srv_tlv = NULL;
const struct sockaddr_storage *src = _addr;
const struct sockaddr_storage *dst = _addr;
- const char *value;
- int value_len;
+ const char *value = "";
+ int value_len = 0;
+ int found = 0;
if (buf_len < PP2_HEADER_LEN)
return 0;
@@ -1993,6 +1996,48 @@ static int make_proxy_line_v2(char *buf, int buf_len,
struct server *srv, struct
}
}
+ if (strm) {
+ struct buffer *replace = NULL;
+
+ list_for_each_entry(srv_tlv, >pp_tlvs, list) {
+ replace = NULL;
+
+ if (!LIST_ISEMPTY(_tlv->fmt)) {
+ replace = alloc_trash_chunk();
+ if
RE: [PATCH 1/4] MEDIUM: server: Parse generic type-value pairs as proxy-v2-options
From fb8714c5aebd7fe957264d0f2234182f55f952fe Mon Sep 17 00:00:00 2001
From: Alexander Stephan
mailto:alexander.step...@sap.com>>
Date: Fri, 15 Sep 2023 12:38:46 +0200
Subject: [PATCH 1/4] MEDIUM: server: Parse generic type-value pairs as
proxy-v2-options
This commit introduces a generic server-side parsing of type-value pair
arguments and allocation of a TLV list. This allows to 1) forward any TLV
type, 2) generally send out any TLV type, and 3) allows to specify concrete
values via an '=' after the type and a log fmt expression. If there is no
TLV found in the remote connection, an empty TLV is sent out.
---
include/haproxy/server-t.h | 10 +
src/server.c | 76 +++---
2 files changed, 81 insertions(+), 5 deletions(-)
diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 1175a470e..7b93bf48f 100644
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -256,6 +256,15 @@ enum __attribute__((__packed__)) srv_ws_mode {
SRV_WS_H2,
};
+/* Server-side TLV list, contains the types of the TLVs that should be sent
out.
+ * Additionally, it can contain data, if specified in the config.
+ */
+struct srv_pp_tlv_list {
+ struct list list;
+ struct list fmt;
+ unsigned char type;
+};
+
struct proxy;
struct server {
/* mostly config or admin stuff, doesn't change often */
@@ -421,6 +430,7 @@ struct server {
struct list srv_rec_item; /* to attach server to a srv record item */
struct list ip_rec_item;/* to attach server to a A or record
item */
struct ebpt_node host_dn; /* hostdn store for srvrq and state file
matching*/
+ struct list pp_tlvs;/* to send out PROXY protocol v2 TLVs */
struct task *srvrq_check; /* Task testing SRV record
expiration date for this server */
struct {
const char *file; /* file where the section appears */
diff --git a/src/server.c b/src/server.c
index 3673340d1..08f86d030 100644
--- a/src/server.c
+++ b/src/server.c
@@ -1036,7 +1036,13 @@ static int srv_parse_proto(char **args, int *cur_arg,
static int srv_parse_proxy_v2_options(char **args, int *cur_arg,
struct proxy *px, struct server *newsrv, char **err)
{
- char *p, *n;
+ char *p = NULL, *n = NULL, *m = NULL;
+ char *key = NULL, *val = NULL;
+ char *endp = NULL;
+ unsigned char idx = 0;
+ size_t key_length = 0, val_length = 0;
+ struct srv_pp_tlv_list *srv_tlv = NULL;
+
for (p = args[*cur_arg+1]; p; p = n) {
n = strchr(p, ',');
if (n)
@@ -1061,13 +1067,61 @@ static int srv_parse_proxy_v2_options(char **args, int
*cur_arg,
newsrv->pp_opts |= SRV_PP_V2_CRC32C;
} else if (strcmp(p, "unique-id") == 0) {
newsrv->pp_opts |= SRV_PP_V2_UNIQUE_ID;
- } else
- goto fail;
+ } else {
+ /* reset val in case it was set before */
+ val = NULL;
+
+ /* try to split by '=' into generic pair key value pair
(=) */
+ m = strchr(p, '=');
+ if (m) {
+ key_length = m - p;
+ key = (char *) malloc(key_length + 1);
+ if (unlikely(!key))
+ goto fail;
+ snprintf(key, key_length + 1, "%s", p);
+
+ val_length = strlen(p) - key_length;
+ val = (char *) malloc(val_length + 1);
+ if (unlikely(!val))
+ goto fail;
+ snprintf(val, val_length + 1, "%s", m + 1);
+ }
+ else {
+ key = (char *) malloc(strlen(p) + 1);
+ if (unlikely(!key))
+ goto fail;
+ snprintf(key, strlen(p) + 1, "%s", p);
+ }
+
+ errno = 0;
+ idx = strtoul(key, , 0);
+ if (unlikely((endp && *endp) != '\0' || (key == endp) || (errno !=
0)))
+ goto fail;
+
+ /* processing is done in connection.c */
+ srv_tlv = malloc(sizeof(struct srv_pp_tlv_list));
+ if (unlikely(!srv_tlv))
+ goto fail;
+
+ srv_tlv->type = idx;
+ LIST_INIT(_tlv->fmt);
+ if (val && unlikely(!parse_logformat_string(val, px, _tlv->fmt,
LOG_OPT_HTTP, PR_CAP_LISTEN, err)))
+ goto fail;
+
+ LIST_APPEND(>pp_tlvs, _tlv->list);
+ free(key);
+ free(val);
+ }
}
return 0;
- fail:
+fail:
+ free(key);
+ free(val);
+ free(srv_tlv);
+ errno = 0;
+
if (err)
- memprintf(err, "'%s' : proxy v2 option not implemented", p);
+ memprintf(err, "'%s' : failed to set proxy v2 option", p);
return ERR_ALERT | ERR_FATAL;
}
@@ -2428,6 +2482,7 @@ struct server *new_server(struct proxy *proxy)
LIST_APPEND(_list, >global_list);
LIST_INIT(>srv_rec_item);
LIST_INIT(>ip_rec_item);
+ LIST_INIT(>pp_tlvs);
MT_LIST_INIT(>prev_deleted);
event_hdl_sub_list_init(>e_subs);
[PATCH 0/4] Support server-side sending and forwarding of arbitrary PPv2 TLVs
Hi all, As a follow-up to my last patches that allow to fetch any TLV in the frontend, I have also implemented similar functionality in the backend that allows to send out TLVs of any type. This is an addition to the existing proxy-protocol-v2-option constants like crc32c. Unlike the pre-defined TLVs, generic TLVs, indicated by their type number, have a common behavior. I think the test I've added as well as the documentation should explain best how it works in more detail. To update TLVs in-place in a connection, before forwarding, I have also added a corresponding TCP action. PPv2 is not quite TCP level, but it seems like the best fit. I am very interested in what you think about the changes. Essentially, these are two commits split into four, for easier review (split by component). I will be on vacation the next two weeks, but my colleague Christian Menges will jump in during that time and address your feedback. Thanks and Best, Alexander SAP SE Germany --- Alexander Stephan (4): MEDIUM: server: Parse generic type-value pairs as proxy-v2-options MEDIUM: connection: Send out generically allocated proxy-v2-options LOW: connection: Add TLV update function MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs doc/configuration.txt | 43 +++ include/haproxy/connection.h | 73 +++ include/haproxy/server-t.h| 10 ++ .../proxy_protocol_send_generic.vtc | 66 ++ src/connection.c | 51 +++- src/server.c | 76 ++- src/tcp_act.c | 120 -- 7 files changed, 417 insertions(+), 22 deletions(-) create mode 100644 reg-tests/connection/proxy_protocol_send_generic.vtc -- 2.35.3
RE: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values
Hi Willy and Ilya, Sorry for the absence, I was mostly out-of-office the last week. I am really sorry for causing this bug. Thanks so much Willy for fixing it as well as for the pool inspection feature I saw today, that looks really handy. I actually debugged the code for quite some time as well, I also tried analyzers but sadly was not able to find it. While looking through the CI history, I found https://github.com/haproxy/haproxy/runs/16184951880, also related to TLVs and only failing on BSD. I guess, this is actual flakiness then? I didn’t find any related bug fix yet. Best, Alexander From: Илья Шипицин Sent: Thursday, August 31, 2023 8:56 PM To: Willy Tarreau Cc: Stephan, Alexander ; haproxy@formilux.org Subject: Re: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values You don't often get email from chipits...@gmail.com<mailto:chipits...@gmail.com>. Learn why this is important<https://aka.ms/LearnAboutSenderIdentification> cirrus-ci backtrace freebsd (cirrus-ci) crash · Issue #2275 · haproxy/haproxy (github.com)<https://github.com/haproxy/haproxy/issues/2275> as usual, I'll send CI improvements once polished чт, 31 авг. 2023 г. в 18:22, Илья Шипицин mailto:chipits...@gmail.com>>: while trying to enable "gdb bt" on cirrus-ci, I noticed that we have similar crashes on musl (where gdb implemented already) https://github.com/haproxy/haproxy/issues/2274 ср, 30 авг. 2023 г. в 05:29, Willy Tarreau mailto:w...@1wt.eu>>: On Tue, Aug 29, 2023 at 11:16:32PM +0200, ??? wrote: > ??, 29 ???. 2023 ?. ? 16:45, Willy Tarreau mailto:w...@1wt.eu>>: > > > On Tue, Aug 29, 2023 at 04:31:31PM +0200, Willy Tarreau wrote: > > > On Tue, Aug 29, 2023 at 02:16:55PM +, Stephan, Alexander wrote: > > > > However, I noticed there is a problem now with the FreeBSD test. Have > > you > > > > already looked into it? > > > > > > Ah no, I had not noticed. I first pushed into a temporary branch and > > > everything was OK so I pushed into master again without checking since > > > it was the exact same commit. > > > > > > > I was not able to reproduce it for now. Looks like the test passes but > > it > > > > crashes afterwards? Maybe some SEGFAULT. Not sure... the CI on your > > branch > > > > looked good iirc. > > > > > > Hmmm no, signal 4 is SIGILL, we're using it for our asserts (BUG_ON()). > > > I never know how to retrieve the stderr log from these tests, there are > > > too many layers for me, I've been explained several times but cannot > > > memorize it. > > > > > > I'm rebuilding here on a freebsd machine, hoping to see it happen again. > > > > > > Note that we cannot rule out a transient issue such as a temporary memory > > > shortage or too long a CPU pause as such VMs are usually overloaded. > > > Maybe it has nothing to do with your new test but just randomly triggered > > > there, or maybe it put the light on a corner case. At least it's not a > > > segfault or such a crash that would indicate a pointer misuse. > > > > I'm getting totally random results from vtest on FreeBSD, I don't know > > why. I even tried to limit to one parallel process and am still getting > > on average 2 errors per run, most always on SSL but not only. Some of > > them include connection failures (sC) with 503 being returned and showing > > "Timeout during SSL handshake". I'm not getting the SIGILL though. Thus > > I don't know what to think about it, especially since it previously worked. > > We'll see if it happens again on next push. > > > > Willy > > > > we can disable cirrus-ci, not sure what is the purpose of randomly failing > CI :) I think it's still used for arm builds or something like this. Regardless, given that freebsd randomly fails on my local platform,there might be something else. I remember that cirrus used to fail some time ago, I don't know what its status is right now. Willy
RE: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values
Hi Willy, > And I was wrong, they were indeed for the first one. However I had to also > remove the NOT_LAST from the intermediate patches using the list_for_each(). > I put quotes around the symbolic names in the doc to make it clearer which > one was to be used and which one it corresponds to, as I had a moment of > hesitation when reading it the first time, so I profitted from this first > encounter to try to further limit doubts. Thanks for the adjustments and clarifications. > That's now merged, thanks again! Very happy that is merged now. However, I noticed there is a problem now with the FreeBSD test. Have you already looked into it? I was not able to reproduce it for now. Looks like the test passes but it crashes afterwards? Maybe some SEGFAULT. Not sure... the CI on your branch looked good iirc. Best, Alexander
RE: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values
Hi Willy, > finally back to this! Overall it's a great and very clean series, I really > want to thank you for this high quality work! Thanks for the compliment, really glad to hear! :) > Yeah it initially gave me a bit of head scratching when reading this part but > I understood what you did and find that it pretty much makes sense after all. > It's not necessarily a hack given that what you've done consists in passing a > pre-parsed argument internally. It's not much different from the few cases > where we emulate older functions or actions using new mechanisms, building > some config elements on the fly just to parse them. Okay, great if there is already similar code. Hack was a bit of an overstatement by me, at least if works a bit different than usually. > I'm fine with this, however I find that the doc is not very clear about what > is permitted I agree that doc needs some more details. I added the note about the iterations and described all the symbolic constants, see at the end of this mail. > Unless you're having any objection, I'm going to flip type and len below: Sorry, that was an oversight on my end. Sure, go ahead. I actually thought I would have arranged it that way, but it somehow slipped my view. > I tend to *prefer* to have them separately for long series that take a lot of > time to review and cannot be done at once. But this series could be reviewed > at once, most patches remain small enough so that's no big deal. OK, great. But I try to keep it in mind for future work anyway. > In this case it could be clarified in the doc that the sample fetch functions > for authority/uniqueid only return the first of each, and that > fc_pp_tlv() iterates over all occurrences of the requested ID. This would > then place a clear separation between trying to extract THE authority, or > checking ALL TLVs of type AUTHORITY. > What do you think ? Good idea, agreed. That also aligns with my goals to keep the existing behavior as much as possible. > If you're OK with this, I'd appreciate it if you could send a few informal > incremental patches that I'd squash into yours. I'm OK with this. :-) The first patch (0001) should be squashed into the commit that introduces the fetch fc_pp_tlv. I guess it fits there because it actually retains and documents prior behavior. The behavior regarding duplicates is also already present, therefore I already added corresponding docs there. The second one (0002) could then be applied to the last patch with the introduction of the constants. Would that work for you? Best, Alexander 0002-Extend-docs-of-fc_pp_tlv-with-constants.patch Description: 0002-Extend-docs-of-fc_pp_tlv-with-constants.patch 0001-Restrict-fc_pp_authority-and-fc_pp_unique_id-to-firs.patch Description: 0001-Restrict-fc_pp_authority-and-fc_pp_unique_id-to-firs.patch
RE: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values
Hi Willy, Did you already have the chance to take a look at the updated patches? No hurry though, I just wanted to make sure that the message didn't get lost. As mentioned, I am aware that sending individual patches is better in the common case. If that is a problem here, please just let me know. Best, Alexander
RE: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values
Hi Willy,
Thanks for the clarifications. I've implemented your requested changes and
split my changes in 6 consecutive patches.
I was not able to use a check function for authority and unique_id without
modifying sample.c or allowing an argument.
As far as I can tell, the check function is only executed for sample fetches
that have not specified 0 for the arg function.
I think it's okay this way since, semantically, those function do not handle
arguments, so doing the argument setup internally makes sense IMO.
If there is way to adjust this without any hacks, I will of course apply it.
For now, I have appended them to this message to not cause to much spam on the
list while also keeping the previous, related discussion.
If I should send them individually, please tell me and I will do so.
Restructuring, especially in regard to pooling lead to quite a bit of changes,
but the overall logic still applies and should hopefully be relatively straight
forward.
Actually, the code even got simpler in many places! :)
BTW, I also added some TLV type constants in the 6th patch, feel free to merge
it or ignore it.
I think it helps with readability and is not really risky.
If there should be a nit that you quickly want to change, feel free to. I am
not upset about it at all.
Best,
Alexander
-Original Message-
From: Willy Tarreau
Sent: Sunday, August 13, 2023 10:01 AM
To: Stephan, Alexander
Cc: haproxy@formilux.org
Subject: Re: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY
protocol v2 TLV values
[You don't often get email from w...@1wt.eu. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]
Hi Alexander,
On Fri, Aug 11, 2023 at 02:08:37PM +, Stephan, Alexander wrote:
> Hi Willy,
>
> Thanks for the nice, detailed feedback.
> Overall, I agree with all of your listed points, so no need for further
> discussions. ?
> I will hopefully send the separated patches at the beginning of next week.
OK! No rush anyway, such changes having a low impact can be merged late in the
cycle if needed, so take your time.
> Just a comment and two small questions to make sure I got things correct:
>
> > As such I'd like them to be renamed to remove this confusion.
> > Maybe just call them HA_PP2_* ?
>
> Yes, such a prefix will clean it up quite nicely IMO. Will add that to
> the first patch of the series.
Thanks.
> > [...]
> > It may even encourage us to create
> > smaller pools if ever deemed really useful (e.g. a 4- or 8- byte
> > load balancing hash key for end-to-end consistency would only take a
> > total of 32 or 40, malloc included).
>
> Just to make sure: Right now, we don't want to optimize further for
> small TLVs other than removing the second pool for the values and
> using the new struct with the VLA to reduce the overhead.
> For normal use, a pool with 160 B could be used now to accommodate
> for the new conn_tlv_list struct and 128 B TLVs (e.g., UNIQUE_ID)?
> For the authority type, it would then be a 255 + 32 B sized pool?
> Maybe that could be used for the value range 128 <= x <= 255, and
> then, for > 255, malloc?
Yes I think that will do the job (more like 128 < x < 256 BTW). I think you'd
rather just focus on the type size (like you did) and not on the type itself,
so that pools will automatically fit (i.e. have
128+sizeof(conn_tlv_list) and 256+sizeof() and the rest is for malloc().
> Other, smaller pools are future work?
Yes, as I'm not sure they're really that much used, and it will be easy to
create smaller pools if we figure they're needed.
> >struct conn_tlv_list {
> > struct list list;
> >unsigned short len; // 65535 should be more than enough!
> >unsigned char type;
> >char contents[0];
> > };
>
> I am also a bit confused about the second struct variant of this. IMO
> this is the optimal struct layout in regards to size, that I would like to
> use.
> What is the other struct for, where `len` and `type` are switched?
Ah, at first I didn't know what you were talking about, I remember having added
the type while writing the message, it's just that I poorly pasted it the
second time :-) It's better to keep it as above, where types are better
aligned and leave no hole. Hmmm BTW the struct will be padded, so you should
use offsetof(conn_tlv_list, contents) rather than
sizeof(conn_tlv_list) for the size calculations. Or you can mark the struct
with __attribute__((packed)), it will do the job as well. It's up to you.
> Thanks again for your efforts, it's really interesting for me to work
> on HAProxy.
You're welcome, do not hesitate to send any question you may have.
Cheers,
Willy
0001-CLEANUP-MINOR-connection-Improve-consistency-of-PPv2.patch
Description: 0001-CLEANUP-MINOR-connection-Improve
RE: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values
Hi Willy,
Thanks for the nice, detailed feedback.
Overall, I agree with all of your listed points, so no need for further
discussions.
I will hopefully send the separated patches at the beginning of next week.
Just a comment and two small questions to make sure I got things correct:
> As such I'd like them to be renamed to remove this confusion.
> Maybe just call them HA_PP2_* ?
Yes, such a prefix will clean it up quite nicely IMO. Will add that to the
first patch of the series.
> [...]
> It may even encourage us to create
> smaller pools if ever deemed really useful (e.g. a 4- or 8- byte
> load balancing hash key for end-to-end consistency would only
> take a total of 32 or 40, malloc included).
Just to make sure: Right now, we don't want to optimize further for small TLVs
other than removing the second pool for the values and using the new struct
with the VLA to reduce the overhead.
For normal use, a pool with 160 B could be used now to accommodate for the new
conn_tlv_list struct and 128 B TLVs (e.g., UNIQUE_ID)?
For the authority type, it would then be a 255 + 32 B sized pool? Maybe that
could be used for the value range 128 <= x <= 255, and then, for > 255, malloc?
Other, smaller pools are future work?
>struct conn_tlv_list {
> struct list list;
>unsigned short len; // 65535 should be more than enough!
>unsigned char type;
>char contents[0];
> };
I am also a bit confused about the second struct variant of this. IMO this is
the optimal struct layout in regards to size, that I would like to use.
What is the other struct for, where `len` and `type` are switched?
Thanks again for your efforts, it's really interesting for me to work on
HAProxy.
Best,
Alexander
-Original Message-
From: Willy Tarreau
Sent: Thursday, August 10, 2023 9:18 AM
To: Stephan, Alexander
Cc: haproxy@formilux.org
Subject: Re: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY
protocol v2 TLV values
[You don't often get email from w...@1wt.eu. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]
Hi Alexander,
On Mon, Jul 31, 2023 at 01:11:35PM +, Stephan, Alexander wrote:
> Dear HAProxy-maintainers,
>
> As proposed by my colleague Christian Menges in [1], I've implemented
> support for fetching arbitrary TLV values for PROXY protocol V2 via a sample
> fetch.
I'm afraid I don't remember seeing this message, and it was left without
response, so it fell through the cracks, that's not cool for your colleague.
> It can be used by calling 'fc_pp_tlv' with the numerical value of the
> desired TLV type. This also fixes issue [2].
Indeed.
> Some design considerations for my approach:
> o *Do not break the existing API*: Keep methods for authority and
> unique ID, but refactor them internally with the updated, generified logic.
That's indeed preferable :-)
> o *Keep existing behavior and performance*: Pools for authority and
> unique ID are still used. Already implemented parsing logic is still
> applied. All information about a TLV payload that is already should be
> used for validation.
OK.
> o *Small memory footprint*: As there might be up to 50k connections,
> an array or hash map is too memory intensive. Therefore, a simple list
> is used. It is the best choice here in my opinion, as there are
> typically only a handful of TLVs.
I agree. At first I thought "Hmmm lists?" but we're speaking about very few
items here, less than a handfull in general, so yes, I agree that the list
might probably be the cheapest option.
> Additionally, memory pooling is used where possible. When TLV values
> become too large there is a fallback to 'malloc'.
At first glance I like this approach. We can see in field how it deals with the
traffic, but it should be OK. However I'm noticing that you put a limit to 127,
which is a bit unfortunate, because we switched the sockaddr_storage to a pool
a few years ago, and they're of size 128, so for one extra byte you could pick
items from that shared pool.
> Note that I choose to not overwrite existing TLVs in the list. This
> way, we return always the first found but would allow duplicate
> entries. This would be relevant when there are duplicate TLVs.
Good point, I hadn't thought about this. And this is desirable because our ACLs
can iterate over multiple instances of a sample. It seems that the current
function smp_fetch_fc_pp_tlv() doesn't support it by the way, and will only
return the first one. Please have a look at
smp_fetch_ssl_hello_alpn() or more generally some HTTP sample fetches that set
SMP_F_NOT_LAST. This tells the ACL engine that if the last returned entry does
not match, it can come back with the same context to retrieve the next one, and
so on. Once you reach the end, you just drop that flag and the
[PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values
Dear HAProxy-maintainers, As proposed by my colleague Christian Menges in [1], I’ve implemented support for fetching arbitrary TLV values for PROXY protocol V2 via a sample fetch. It can be used by calling ‘fc_pp_tlv’ with the numerical value of the desired TLV type. This also fixes issue [2]. Some design considerations for my approach: • *Do not break the existing API*: Keep methods for authority and unique ID, but refactor them internally with the updated, generified logic. • *Keep existing behavior and performance*: Pools for authority and unique ID are still used. Already implemented parsing logic is still applied. All information about a TLV payload that is already should be used for validation. • *Small memory footprint*: As there might be up to 50k connections, an array or hash map is too memory intensive. Therefore, a simple list is used. It is the best choice here in my opinion, as there are typically only a handful of TLVs. Additionally, memory pooling is used where possible. When TLV values become too large there is a fallback to ‘malloc’. Note that I choose to not overwrite existing TLVs in the list. This way, we return always the first found but would allow duplicate entries. This would be relevant when there are duplicate TLVs. Besides, I used ‘strtoul’ for the argument conversion to be consistent with other fetches that already use ‘strtoul’. If ‘strtoul’ is too slow for this use case, I am not opposed to reverting it to the more efficient HAProxy helper. *Important*: I will add the documentation after the code review, when the design is finalized. This email address is not subscribed to the list, please CC it when replying. Anyway, I would love to hear your feedback on this! Best Regards, Alexander Stephan SAP SE Germany PS: This is only the front-end part of my implementation, I will follow up with a backend implementation that allows to send user defined TLVs. I already implemented it, since SAP needs it any case. However, we are also very interested in contributing it. [1]: https://www.mail-archive.com/haproxy@formilux.org/msg43381.html [2]: https://github.com/haproxy/haproxy/issues/1947 0001-MEDIUM-sample-Implement-sample-fetch-for-arbitrary-P.patch Description: 0001-MEDIUM-sample-Implement-sample-fetch-for-arbitrary-P.patch
RE: [PR] Implement fetch for arbitrary TLV payloads
Dear list, This PR request was not meant to be sent to the upstream repository. Furthermore, this is not ready to receive any maintainer feedback yet. I accidentally selected the wrong base repository when I was about to create a draft PR to show my code to some colleagues. Unfortunately, I did not spot my mistake before the bot became active. Of course, I am aware that this does not meet the requirements for a contribution and it also was not meant to be. I am really sorry for creating spam here. I will send a new, fully compliant patch in the next weeks. I would highly appreciate, if you then could take a look at my changes. Best Regards, Alexander Stephan -Original Message- From: PR Bot Sent: Friday, June 30, 2023 1:23 PM To: haproxy@formilux.org Cc: Stephan, Alexander Subject: [PR] Implement fetch for arbitrary TLV payloads [You don't often get email from haproxy-pr-bot-no-re...@ltri.eu. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] Dear list! Author: Alexander Stephan Number of patches: 1 This is an automated relay of the Github pull request: Implement fetch for arbitrary TLV payloads Patch title(s): Fully working version with debug statements and missing test Link: https://github.com/haproxy/haproxy/pull/2199 Edit locally: wget https://github.com/haproxy/haproxy/pull/2199.patch && vi 2199.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/2199.patch | git am - Description: TODO: - [ ] Add / adjust test - [ ] Remove debug logs - [ ] Always append to the list and return the first hit or traverse the list and overwrite. This is relevant for duplicate TLV which are technically possible, although unusual. - [ ] Make CRC32 and NETNS fetchable for consistency - [ ] Formatting to meet HAProxy contribution guidelines Instructions: This github pull request will be closed automatically; patch should be reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is invited to comment, even the patch's author. Please keep the author and list CCed in replies. Please note that in absence of any response this pull request will be lost.
