Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
Hi Alexander,
On Fri, Nov 10, 2023 at 08:44:31PM +, Stephan, Alexander wrote:
> > I don't see how this is possible:
> >
> >list_for_each_entry(srv_tlv, &src->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.
(...)
> 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.
Thanks! I could indeed reproduce it and the cause is indeed a missing
initialization, because the very first element is NULL ;-) I found it,
there's still an ugly manual initialization of the default server made
in proxy_preset_defaults(). One day we'll have a function to reinitialize
a server... This patch fixes it, I'm going to merge it and get rid of
the test in the loop:
diff --git a/src/proxy.c b/src/proxy.c
index 7ff087190..544c22f82 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -1469,6 +1469,7 @@ void proxy_preset_defaults(struct proxy *defproxy)
defproxy->defsrv.onerror = DEF_HANA_ONERR;
defproxy->defsrv.consecutive_errors_limit = DEF_HANA_ERRLIMIT;
defproxy->defsrv.uweight = defproxy->defsrv.iweight = 1;
+ LIST_INIT(&defproxy->defsrv.pp_tlvs);
defproxy->email_alert.level = LOG_ALERT;
defproxy->load_server_state_from_file = PR_SRV_STATE_FILE_UNSPEC;
> > 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.
Hmm you're right, I misanalyzed the situation there. Actually the
conn_send_proxy() function does have the current connection we're
working on, but the functions it calls (make_proxy_line()) doesn't
have it. However we pass either the remote connection when there is
a stream, or the local connection when there is no stream, hence
remote->owner should always be valid, and apparently it's done
precisely for checks:
/* The target server expects a PROXY line to be sent first.
* If the send_proxy_ofs is negative, it corresponds to the
* offset to start sending from then end of the proxy string
* (which is recomputed every time since it's constant). If
* it is positive, it means we have to send from the start.
* We can only send a "normal" PROXY line when the connection
* is attached to a stream connector. Otherwise we can only
* send a LOCAL line (eg: for use with health checks).
*/
if (sc && sc_strm(sc)) {
ret = make_proxy_line(trash.area, trash.size,
objt_server(conn->target),
sc_conn(sc_opposite(sc)),
__sc_strm(sc));
}
else {
/* The target s
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, &src->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, &srv_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
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
On Fri, Nov 03, 2023 at 08:35:08PM +, Stephan, Alexander wrote:
> Hi Willy,
>
> Thanks for the review. No problem for calling me Stephan, I am totally used
> to that, my teachers did that for years.
Oh I was sure you were used to but anyway I don't like calling people
incorrectly.
> > 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?
It depends. The rule of thumb for this is to think about the risk of breakage
and the ability to bisect (and possibly revert) later. But it's clear that
some API changes need to be performed atomically and can sometimes be a bit
larger than initially expected. Among the things we practice if some large
area changes are needed, is to try to make a first set of preparation patches
that apply most of the visual changes without changing semantics (e.g. move
some code into a separate function, or reindent 500 lines at once in an
"if (1)" block etc). And only then the patches that perform the real changes
are applied. This has the benefit that the first patch promises not to
change the logic so that it cannot break on bisect and doesn't need to be
analyzed later, and the patches that change the logic are much smaller.
> > 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.
OK let's try to figure that later. I'll merge your code in its current
form for now.
> > 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!
;-) Will do.
> > 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.
OK. No rush but similarly, patches welcome of course.
> > 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_*.
OK will do it.
> 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.
Sure, that would be great!
thanks and have a nice week-end!
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%2F
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://github.com/haproxy/haproxy/blob/47ed1181f29a56ccb04e5b80ef4f9414466ada7a/src/server.c#L2370
> For now, I did not want to change the signature, but I think, error handling
> would be beneficial here.
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!
> - In the new list_for_each in srv_settings_cpy I have to check for srv_tlv ==
> NULL as for some reason, the list contains NULL entries. I don't see any
> mistakes with the list handling. Maybe it is just due to the structure of the
> server logic.
I don't see how this is possible:
list_for_each_entry(srv_tlv, &src->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).
> - Please double check that my arguments for the parse_logformat_string
> function are correct. I omit log options for now and use the capabilities of
> the proxy. Seems like the best fit, but I could be wrong.
Ah good point. The argument you're missing and for which use used
px->cap is one of SMP_VAL_*. It indicates at which point(s) the
log-format may be called so that the parser can emit suitable
warnings if some sample fetch functions are not available. Here it
will be used during the server connect() phase, so there is already
one perfect for this, which is SMP_VAL_BE_SRV_CON. It's already used
by the source and sni arguments. For the options I think it's OK that
it stays zero, that's how it's used everywhere else :-)
I could verify that your reg test still works with this change:
--- a/src/server.c
+++ b/src/server.c
@@ -3252,7 +3252,7 @@ static int _srv_parse_finalize(char **args, int cur_arg,
list_for_each_entry(srv_tlv, &srv->pp_tlvs, list) {
LIST_INIT(&srv_tlv->fmt);
if (srv_tlv->fmt_string &&
unlikely(!parse_logformat_string(srv_tlv->fmt_string,
- srv->proxy, &srv_tlv->fmt, 0, px->cap, &errmsg))) {
+ srv->proxy, &srv_tlv->fmt, 0, SMP_VAL_BE_SRV_CON,
&errmsg))) {
if (errmsg) {
ha_alert("%s\n", errmsg);
free(errmsg);
So if that's OK for you I can change it now before merging.
> - I noticed that there are also no checks for strdup in server.c, that might
> need to be addressed in the future. For the srv_settings_cpy I cannot give an
> error, but only try to avoid the memory leak.
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 :-(
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 abil
Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
On Fri, Nov 03, 2023 at 05:15:03PM +, Stephan, Alexander wrote: > Hi Willy, > > Sorry, my email client probably did something weird... > I attached them now, should hopefully prevent any reformatting. Thanks for the fast response. I'll check them keeping in mind your last comments in your previous e-mail for patch2. Hopefully this evening before issuing 2.9-dev9 with them. Willy
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
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
Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
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. Thanks, Willy
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
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)]
+
+# 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 = &null_addr;
const struct sockaddr_storage *dst = &null_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, &srv->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(&srv_tlv->fmt)) {
+ replace = alloc_trash_chunk();
+ if (unlikely(!replace))
+ return 0;
+
+ replace->data = build_logline(strm,
replace->area, replace->size, &srv_tlv->fmt);
+
+ if (unlikely((buf_len - ret) < sizeof(struct
tlv))) {
+ free_trash_chunk(replace);
+ return 0;
+ }
+ ret += make_tlv(&buf[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(&buf[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,
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
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 origi
Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
Hi Alexander, On Mon, Oct 23, 2023 at 12:07:39PM +, Stephan, Alexander wrote: > We can ignore the last two commits for now (LOW: connection: Add TLV update > function and MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs). > Based on the first two commits, I created a diff that would implement > something like you requested (new parser and no use of remote). > It should be fully working, so you could even try it out locally if you want. > If you think it looks promising, I will adjust the tests and the docs, and > update the series. OK. > Now to address your feedback in more detail: > > > I'm really having an issue with using "the remote frontend connection" > > here. We've done this mistake in the past with the transparent mode that > > connects to the original destination address, that resulted in "set-dst" > > having to be implemented, then being broken by multiplexed streams (e.g. > > h2), then having to be internally emulated at various layers (connection, > > session, stream, transaction etc). Same for src. The solution is not > > durable at all, and as you noticed, you already had to implement the > > ability to modify these fields before passing them, hence reintroducing > > that class of trouble on a new area. > > I choose to use the remote connection since it is already done that way for > other TLV types like SRV_PP_V2_AUTHORITY > (https://github.com/haproxy/haproxy/commit/8a4ffa0aabf8509098f95ac78fa48b18f415c42c). > It simply forwards what is found the remote connection, if there is something > set. Similarly, PP2_TYPE_NETNS and SRV_PP_V2_SSL depend on remote. > First, I did not like the method with an extra fetch since it creates > potential overhead. However, I am likely a bit excessive here. I am not > opposed to not using remote at all. I simply was not aware of the > intricacies. Oh rest assured I'm not criticizing your choice, it's common to start from what exists, I was merely explaining that what exists is the result of poor choices and that we'd rather not spread them further ;-) > > Also what will be left in the logs after you modify the contents ? etc. > You mean that the log does not match the content of the actual TLV? I mean once modified via a rule, it's difficult to know if the log contains a modified field or an original one. > It could happen, I suppose. But why doesn't this problem with multiplexed > connections apply to all the TCP actions? As far as I know they all alter > fields of the connection directly. Doesn't really matter though, I don't plan > to use it anymore. We do 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
RE: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options
y orphan idle connections, defaults to 1s */
+ { "proto",srv_parse_proto,1, 1, 1 },
/* Set the proto to use for all outgoing connections */
+ { "proxy-v2-options", srv_parse_proxy_v2_options, 1, 1, 1 },
/* options for send-proxy-v2 */
+ { "redir",srv_parse_redir,1, 1, 0 },
/* Enable redirection mode */
+ { "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 c
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 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.
I'm seeing another issue with passing type=fmt like this. The option is
currently defined as cutting around commas, and that's what it does. This
means that the format will not be parsable (no sample fetch functions with
more than one argument, no converters). Example:
proxy-v2-options 0xEE=%[var(txn.clientname,_unknown_)]
^ ^^^
First option Second option
Also the syntax in the format something=value is not that great for config
parsers and generators. However it looks like our server keywords support
parenthesis, so I think that instead you could do the following:
proxy-v2-options(0xEE) %[var(txn.clientname,_unknown_)]
In this case the proxy-v2-options keyword would only add one option and
take a log-format, and you'd add as many such keywords for each option.
Note that from a configuration perspective this is cleaner as it's easier
to maintain value pairs like this for config management. But I don't think
the option name is that great especially to send a single one. So maybe
this one instead should be "proxy-v2-option(type) " though it may
maintain some confusion (option vs options). The other solution might be
to have a shorter one, which is also nicer when using many of these, e.g.
"proxy-v2-opt()".
Such an approach would basically split the proxy-v2 usage in two groups:
- options that are either there or not there (proxy-v2-options)
- options that need to take a value (proxy-v2-opt()).
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. For set-var() we're using a raw
typed expression, but then we created set-var-fmt() to have one that takes
a log format. Just thinking loud, but then what about "proxy-v2-optfmt()"
or even "proxy-v2-tlvfmt()" or "pp-tlv-fmt()" (after all when speaking
about tlv we imply v2, right?). That would allow us later to implement
"pp-tlv()" taking an expression if really needed, e.g. to pass a DER
certificate, a SHA256, a sub-tlv or any such a thing.
Generally speaking I'm fine with the approach of making the output PP TLV
configurable, I think it does bring some value and flexibility in general.
That's also why I'm extremely careful about the config shortcomings that
can come with it, because I suspect it
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 = &null_addr;
const struct sockaddr_storage *dst = &null_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, &srv->pp_tlvs, list) {
+ replace = NULL;
+
+ if (!LIST_ISEMPTY(&srv_tlv->fmt)) {
+ replace
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 = &null_addr;
const struct sockaddr_storage *dst = &null_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, &srv->pp_tlvs, list) {
+ replace = NULL;
+
+ if (!LIST_ISEMPTY(&srv_tlv->fmt)) {
+ replace = alloc_trash_chunk();
+
