[PATCH] DOC: configuration: clarify http-request wait-for-body
Make it more explicit what happens in the various scenarios that cause HAProxy to stop waiting when "http-request wait-for-body" is used. Also fix a couple of grammatical errors. Fixes: #2410 Signed-Off-By: Thayne McCombs --- doc/configuration.txt | 24 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index f9bb147832..5581c7d94f 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -15229,13 +15229,21 @@ wait-for-body time [ at-least ] Usable in: TCP RqCon| RqSes| RqCnt| RsCnt|HTTP Req| Res| Aft - | - | - | - | X | X | - - This will delay the processing of the request or response waiting for the - payload for at most milliseconds. if "at-least" argument is specified, - HAProxy stops waiting for the payload when the first bytes are - received. 0 means no limit, it is the default value. Regardless the - "at-least" argument value, HAProxy stops to wait if the whole payload is - received or if the request buffer is full. This action may be used as a - replacement to "option http-buffer-request". + This will delay the processing of the request or response until one of the following + conditions occurs: + - The full request body is received, in which case processing proceeds +normally. + - bytes have been received, when the "at-least" argument is given and + is non-zero, in which case processing proceeds normally. + - The request buffer is full, in which case processing proceeds normally. The +size of this buffer is determined by the "tune.bufsize" option. + - The request has been waiting for more than milliseconds. In this +case HAProxy will respond with a 408 "Request Timeout" error to the client +and stop processing the request. Note that if any of the other conditions +happens first, this timeout will not occur even if the full body has +not yet been recieved. + + This action may be used as a replacement for "option http-buffer-request". Arguments : @@ -15244,7 +15252,7 @@ wait-for-body time [ at-least ] is optional. It is the minimum payload size to receive to stop to wait. It follows the HAProxy size format and is expressed in - bytes. + bytes. A value of 0 (the default) means no limit. Example: http-request wait-for-body time 1s at-least 1k if METH_POST -- 2.43.0
[PATCH] MINOR : converter: add param converter
Add a converter that extracts a parameter from string of delimited
key/value pairs.
Fixes: #1697
---
doc/configuration.txt | 26
reg-tests/converter/param.vtc | 80 +++
src/sample.c | 64
3 files changed, 170 insertions(+)
create mode 100644 reg-tests/converter/param.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index c45f0b4b68..0cc2bdee3b 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -17702,6 +17702,32 @@ or()
This prefix is followed by a name. The separator is a '.'. The name may only
contain characters 'a-z', 'A-Z', '0-9', '.' and '_'.
+param(,[])
+ This extracts the first occurrence of the parameter in the input
string
+ where parameters are delimited by , which defaults to "&", and the
name
+ and value of the parameter are separated by a "=". If there is no "=" and
+ value before the end of the parameter segment, it is treated as equivalent to
+ a value of an empty string.
+
+ This can be useful for extracting parameters from a query string, or possibly
+ a x-www-form-urlencoded body. In particular, `query,param()` can be
used
+ as an alternative to `urlp()` which only uses "&" as a delimiter,
+ whereas "urlp" also uses "?" and ";".
+
+ Note that this converter doesn't do anything special with url encoded
+ characters. If you want to decode the value, you can use the url_dec
converter
+ on the output. If the name of the parameter in the input might contain
encoded
+ characters, you'll probably want do normalize the input before calling
+ "param". This can be done using "http-request normalize-uri", in particular
+ the percent-decode-unreserved and percent-to-uppercase options.
+
+ Example :
+ str(a=b=d=r),param(a) # b
+ str(a=c),param(a) # ""
+ str(a==a),param(b) # ""
+ str(a=1;b=2;c=4),param(b,;) # 2
+ query,param(redirect_uri),urldec()
+
port_only
Converts a string which contains a Host header value into an integer by
returning its port.
diff --git a/reg-tests/converter/param.vtc b/reg-tests/converter/param.vtc
new file mode 100644
index 00..1633603823
--- /dev/null
+++ b/reg-tests/converter/param.vtc
@@ -0,0 +1,80 @@
+varnishtest "param converter Test"
+
+feature ignore_unknown_macro
+
+server s1 {
+ rxreq
+ txresp -hdr "Connection: close"
+} -repeat 10 -start
+
+haproxy h1 -conf {
+ defaults
+ mode http
+ timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout client "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout server "${HAPROXY_TEST_TIMEOUT-5s}"
+
+ frontend fe
+ bind "fd@${fe}"
+
+ ### requests
+ http-request set-var(txn.query) query
+ http-response set-header Found %[var(txn.query),param(test)] if {
var(txn.query),param(test) -m found }
+
+ default_backend be
+
+ backend be
+ server s1 ${s1_addr}:${s1_port}
+} -start
+
+client c1 -connect ${h1_fe_sock} {
+ txreq -url "/foo/?test=1=4"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "1"
+
+ txreq -url "/?a=1=4=34"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "34"
+
+ txreq -url "/?test=bar"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "bar"
+
+ txreq -url "/?a=b=d"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+
+ txreq -url "/?a=b=t=d"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "t"
+
+ txreq -url "/?a=b=d"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+
+ txreq -url "/?test="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+
+txreq -url "/?a=b"
+rxresp
+expect resp.status == 200
+expect resp.http.found == ""
+
+txreq -url "/?testing=123"
+rxresp
+expect resp.status == 200
+expect resp.http.found == ""
+
+txreq -url "/?testing=123=4"
+rxresp
+expect resp.status == 200
+expect resp.http.found == "4"
+} -run
diff --git a/src/sample.c b/src/sample.c
index 62a372b81c..7a612fc033 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -2607,6 +2607,69 @@ static int sample_conv_word(const struct arg *arg_p,
struct sample *smp, void *p
return 1;
}
+static int sample_conv_param_check(struct arg *arg, struct sample_conv *conv,
+ const char *file, int line, char **err)
+{
+ if (arg[1].type == ARGT_STR && arg[1].data.str.data != 1) {
+ memprintf(err, "Delimiter must be exactly 1 character.");
+ return 0;
+ }
+
+ return 1;
+}
+
+static int sample_conv_param(const struct arg *arg_p, struct sample *smp, void
*private)
+{
+ char *pos, *end, *pend, *equal;
+ char delim = '&';
+ const char *name =
[PATCH] MINOR : converter: add param converter
Ok. I think this patch addresses all of your feedback. Thanks for
looking at it.
-- >8 --
Add a converter that extracts a parameter from string of delimited
key/value pairs.
Fixes: #1697
---
doc/configuration.txt | 26
reg-tests/converter/param.vtc | 80 +++
src/sample.c | 64
3 files changed, 170 insertions(+)
create mode 100644 reg-tests/converter/param.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index c45f0b4b68..0cc2bdee3b 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -17702,6 +17702,32 @@ or()
This prefix is followed by a name. The separator is a '.'. The name
may only
contain characters 'a-z', 'A-Z', '0-9', '.' and '_'.
+param(,[])
+ This extracts the first occurrence of the parameter in the
input string
+ where parameters are delimited by , which defaults to "&", and
the name
+ and value of the parameter are separated by a "=". If there is no "=" and
+ value before the end of the parameter segment, it is treated as
equivalent to
+ a value of an empty string.
+
+ This can be useful for extracting parameters from a query string, or
possibly
+ a x-www-form-urlencoded body. In particular, `query,param()`
can be used
+ as an alternative to `urlp()` which only uses "&" as a delimiter,
+ whereas "urlp" also uses "?" and ";".
+
+ Note that this converter doesn't do anything special with url encoded
+ characters. If you want to decode the value, you can use the url_dec
converter
+ on the output. If the name of the parameter in the input might
contain encoded
+ characters, you'll probably want do normalize the input before calling
+ "param". This can be done using "http-request normalize-uri", in
particular
+ the percent-decode-unreserved and percent-to-uppercase options.
+
+ Example :
+ str(a=b=d=r),param(a) # b
+ str(a=c),param(a) # ""
+ str(a==a),param(b) # ""
+ str(a=1;b=2;c=4),param(b,;) # 2
+ query,param(redirect_uri),urldec()
+
port_only
Converts a string which contains a Host header value into an integer by
returning its port.
diff --git a/reg-tests/converter/param.vtc b/reg-tests/converter/param.vtc
new file mode 100644
index 00..1633603823
--- /dev/null
+++ b/reg-tests/converter/param.vtc
@@ -0,0 +1,80 @@
+varnishtest "param converter Test"
+
+feature ignore_unknown_macro
+
+server s1 {
+ rxreq
+ txresp -hdr "Connection: close"
+} -repeat 10 -start
+
+haproxy h1 -conf {
+ defaults
+ mode http
+ timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout client "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout server "${HAPROXY_TEST_TIMEOUT-5s}"
+
+ frontend fe
+ bind "fd@${fe}"
+
+ ### requests
+ http-request set-var(txn.query) query
+ http-response set-header Found %[var(txn.query),param(test)] if {
var(txn.query),param(test) -m found }
+
+ default_backend be
+
+ backend be
+ server s1 ${s1_addr}:${s1_port}
+} -start
+
+client c1 -connect ${h1_fe_sock} {
+ txreq -url "/foo/?test=1=4"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "1"
+
+ txreq -url "/?a=1=4=34"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "34"
+
+ txreq -url "/?test=bar"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "bar"
+
+ txreq -url "/?a=b=d"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+
+ txreq -url "/?a=b=t=d"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "t"
+
+ txreq -url "/?a=b=d"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+
+ txreq -url "/?test="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+
+ txreq -url "/?a=b"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+
+ txreq -url "/?testing=123"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+
+ txreq -url "/?testing=123=4"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "4"
+} -run
diff --git a/src/sample.c b/src/sample.c
index 62a372b81c..7a612fc033 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -2607,6 +2607,69 @@ static int sample_conv_word(const struct arg
*arg_p, struct sample *smp, void *p
return 1;
}
+static int sample_conv_param_check(struct arg *arg, struct sample_conv
*conv,
+ const char *file, int line, char **err)
+{
+ if (arg[1].type == ARGT_STR && arg[1].data.str.data != 1) {
+ memprintf(err, "Delimiter must be exactly 1 character.");
+ return 0;
+ }
+
+ return 1;
+}
+
+static int sample_conv_param(const struct arg *arg_p, struct sample
*smp, void *private)
+{
+ char *pos, *end, *pend, *equal;
+ char delim = '&';
+ const char *name = arg_p[0].data.str.area;
+ size_t name_l =
Re: [PATCH 2/2] MINOR : converter: add param converter
Just wondering (maybe something to add to the doc or test): Should this handle URL encoded parameter names or parameter values? It probably should not, because that's makes the converter less general. But it would certainly be useful to explain how to properly retrieve those values. Simply url-decoding the full query string before doesn't do the job, because then the additional delimiters might be introduced. This likely needs to be combined with the URI normalization feature, as the encoding of a parameter name is not a 1:1 relationship. Hmm, Initially I was thinking that it would be sufficient to use `urldec` on the result to handle url encoding, but I didn't think about the name itself being encoded. I'll add something to the docs recommending using uri-normalization for now. Although, I suppose one downside to that is it doesn't help if the input doesn't come from the uri (for example if it is in the body or a header).
Re: [PATCH 2/2] MINOR : converter: add param converter
There were a couple of things I wasn't entirely sure about: 1. Should this allow specifying the separator between key and value, rather than always using "="? 2. How should it handle the case where there isn't a value given, the current implementation treats "a" as equivalent to "a="
Re: [PATCH 1/2] BUG/MEDIUM: tools: Fix `inet_ntop` usage in sa2str
Thanks for catching that.
Re: [PATCH] MINOR: sample: Add a regmatch converter
On 4/13/21 3:44 AM, Willy Tarreau wrote:
I'm failing to see how it differs from regsub() which already does the
same with the reference (you'd write \1 instead of 1) and also allows to
compose something out of multiple matches. Am I missing something, or a
case where regsub() is really not convenient compared to this one ?
Thanks,
Willy
Mostly just convenience. It is possible to accomplish the same thing
using something like
`'regsub(".*(mypattern).*","\1")'`, but you need to remember to include
the ".*" on both sides.
I also suspect (although I haven't benchmarked it) that regmatch would
perform better than the
equivalent regsub.
That said, I do recognize that this doesn't add any completely new
functionality, and I will not
be offended at all if you don't think it is worth merging.
[PATCH] MINOR: sample: Add a regmatch converter
Add a new sample converter that finds the first regex match and returns
the substring for that match, or a capture group, if an index is
provided.
---
doc/configuration.txt| 22 +++
reg-tests/converter/regmatch.vtc | 39 +++
src/sample.c | 66
3 files changed, 127 insertions(+)
create mode 100644 reg-tests/converter/regmatch.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index f21a29a68..e84395d23 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -16238,6 +16238,28 @@ protobuf(,[])
More information may be found here about the protocol buffers message field
types:
https://developers.google.com/protocol-buffers/docs/encoding
+regmatch([,[,]])
+ This extracts a substring that matches the regex pattern. It will return the
first
+ match in the input string. By default it returns the entire match, but if
+ is supplied, then the capture group for that number will be returned
instead. A
+ value of 0 returns the entire match. The regex can be made case insensitive
by
+ adding the flag "i" in .
+
+ It is highly recommended to enclose the regex part using protected quotes to
+ improve clarity and never have a closing parenthesis from the regex mixed up
with
+ the parenthesis from the function. Just like in Bourne shell, the first
level of
+ quotes is processed when delimiting word groups on the line, a second level
is
+ usable for argument. It is recommended to use single quotes outside since
these
+ ones do not try to resolve backslashes nor dollar signs.
+
+ Examples:
+
+ # extract part of content-type
+ http-request set-var(txn.imtype)
'hdr(content-type),regmatch("image/(.*)",1)'
+
+ # extract cookie with certain pattern
+ http-request set-header x-test-cookie
%[hdr(cookie),'regmatch(test-\w+=\d+)']
+
regsub(,[,])
Applies a regex-based substitution to the input string. It does the same
operation as the well-known "sed" utility with "s///". By
diff --git a/reg-tests/converter/regmatch.vtc b/reg-tests/converter/regmatch.vtc
new file mode 100644
index 0..46df78ee0
--- /dev/null
+++ b/reg-tests/converter/regmatch.vtc
@@ -0,0 +1,39 @@
+varnishtest "regmatch converter Test"
+
+feature ignore_unknown_macro
+
+server s1 {
+ rxreq
+ txresp
+} -repeat 3 -start
+
+haproxy h1 -conf {
+ defaults
+ mode http
+ timeout connect 1s
+ timeout client 1s
+ timeout server 1s
+
+ frontend fe
+ bind "fd@${fe}"
+
+ requests
+ http-request set-var(txn.match) "path,regmatch('test/(\d+)/',1)"
+ http-response set-header Found %[var(txn.match)] if { var(txn.match) -m
found }
+
+ default_backend be
+
+ backend be
+ server s1 ${s1_addr}:${s1_port}
+} -start
+
+client c1 -connect ${h1_fe_sock} {
+ txreq -url "/test/123/456"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "123"
+ txreq -url "/foo"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+} -run
diff --git a/src/sample.c b/src/sample.c
index 835a18115..66d674e3f 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -2671,6 +2671,71 @@ static int sample_conv_word(const struct arg *arg_p,
struct sample *smp, void *p
return 1;
}
+static int sample_conv_regmatch_check(struct arg *args, struct sample_conv
*conv,
+ const char *file, int line, char **err)
+{
+ struct arg *arg = args;
+ char *p;
+ int len;
+
+ if (arg[1].type == ARGT_SINT && (arg[1].data.sint < 0 ||
arg[1].data.sint > MAX_MATCH)) {
+ memprintf(err, "invalid capture group number %lld. must be
between 0 and %d", arg[1].data.sint, MAX_MATCH);
+ return 0;
+ }
+
+ /* arg0 is a regex, it uses type_flag for ICASE */
+ arg[0].type_flags = 0;
+
+ if (arg[2].type != ARGT_STR)
+ return 1;
+
+ p = arg[2].data.str.area;
+ len = arg[2].data.str.data;
+ while (len) {
+ if (*p == 'i') {
+ arg[0].type_flags |= ARGF_REG_ICASE;
+ }
+ else {
+ memprintf(err, "invalid regex flag '%c', only 'i' is
supported", *p);
+ return 0;
+ }
+ p++;
+ len--;
+ }
+ return 1;
+}
+
+/* This sample function is designed to find the first match of a regex in the
input string.
+ * If arg1 is supplied, that is used as the capture group to return (or the
whole match if 0).
+ */
+static int sample_conv_regmatch(const struct arg *arg_p, struct sample *smp,
void *private)
+{
+ struct my_regex *reg = arg_p[0].data.reg;
+ regmatch_t pmatch[MAX_MATCH];
+ regmatch_t capture;
+ int nmatch = (arg_p[1].type == ARGT_SINT) ? arg_p[1].data.sint : 0;
+ int found;
+
+ found =
[PATCH] BUG/MINOR: sample: Fix adjusting size in field converter
Adjust the size of the sample buffer before we change the "area" pointer. The change in size is calculated as the difference between the original pointer and the new start pointer. But since the `smp->data.u.str.area` assignment results in `smp->data.u.str.area` and `start` being the same pointer, we always ended up substracting zero. This changes it to change the size by the actual amount it changed. I'm not entirely sure what the impact of this is, but the previous code seemed wrong. --- src/sample.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sample.c b/src/sample.c index eac489538..05d78bcb9 100644 --- a/src/sample.c +++ b/src/sample.c @@ -2561,13 +2561,13 @@ static int sample_conv_field(const struct arg *arg_p, struct sample *smp, void * if (!smp->data.u.str.data) return 1; - smp->data.u.str.area = start; - /* Compute remaining size if needed Note: smp->data.u.str.size cannot be set to 0 */ if (smp->data.u.str.size) smp->data.u.str.size -= start - smp->data.u.str.area; + smp->data.u.str.area = start; + return 1; } -- 2.31.1
Re: [PATCH] BUG/MINOR: tools: fix parsing "us" unit for timers
Oh, and this should be backported to all supported versions.
[PATCH] BUG/MINOR: tools: fix parsing "us" unit for timers
Commit c20ad0d8dbd1bb5707bbfe23632415c3062e046c (BUG/MINOR: tools: make
parse_time_err() more strict on the timer validity) broke parsing the "us"
unit in timers. It caused `parse_time_err()` to return the string "s",
which indicates an error.
Now if the "u" is followed by an "s" we properly continue processing the
time instead of immediately failing.
This fixes https://github.com/haproxy/haproxy/issues/1209
---
src/tools.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/tools.c b/src/tools.c
index 546ad4416..4924ad1a0 100644
--- a/src/tools.c
+++ b/src/tools.c
@@ -2326,6 +2326,7 @@ const char *parse_time_err(const char *text, unsigned
*ret, unsigned unit_flags)
if (text[1] == 's') {
idiv = 100;
text++;
+ break;
}
return text;
case 'm': /* millisecond : "ms" or minute: "m" */
--
2.31.1
Re: [PATCH] Fix memory leak from used_server_addr during deinit
> I'm just wondering if it technically would be necessary to remove the server from the tree or not? I don't think so. But I'm not sure. On Sat, Jan 9, 2021, 06:34 Tim Düsterhus wrote: > Thayne, > > Am 09.01.21 um 08:45 schrieb Thayne McCombs: > > > > After actually reading the full CONTRIBUTING file, I now include my > patch with a better commit message. Sorry about that. > > > > Thanks, the commit message looks good to me and I can confirm your patch > fixes the issue I was seeing. > > I'm just wondering if it technically would be necessary to remove the > server from the tree or not? > > Best regards > Tim Düsterhus >
Re: [PATCH] Fix memory leak from used_server_addr during deinit
After actually reading the full CONTRIBUTING file, I now include my patch with
a better commit message. Sorry about that.
>From b449379a7b36c213408b1ae28924475c413bd6ba Mon Sep 17 00:00:00 2001
From: Thayne McCombs
Date: Thu, 7 Jan 2021 22:11:05 -0700
Subject: [PATCH] BUG/MINOR: server: Memory leak of proxy.used_server_addr
during deinit
GitHub Issue #1037 Reported a memory leak in deinit() caused by an
allocation made in sa2str() that was stored in srv_set_addr_desc().
When destroying each server for a proxy in deinit, include freeing the
memory in the key of server->addr_node.
The leak was introduced in commit 92149f9a8 ("MEDIUM: stick-tables: Add
srvkey option to stick-table") which is not in any released version so
no backport is needed.
Cc: Tim Duesterhus
---
src/haproxy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/haproxy.c b/src/haproxy.c
index a28b45fb9..1d16b507d 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2649,6 +2649,7 @@ void deinit(void)
free(s->available_conns);
free(s->curr_idle_thr);
free(s->resolvers_id);
+ free(s->addr_node.key);
if (s->use_ssl == 1 || s->check.use_ssl == 1 || (s->proxy->options & PR_O_TCPCHK_SSL)) {
if (xprt_get(XPRT_SSL) && xprt_get(XPRT_SSL)->destroy_srv)
--
2.30.0
[PATCH] Fix memory leak from used_server_addr during deinit
Fixes #1037
---
src/haproxy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/haproxy.c b/src/haproxy.c
index a28b45fb9..1d16b507d 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2649,6 +2649,7 @@ void deinit(void)
free(s->available_conns);
free(s->curr_idle_thr);
free(s->resolvers_id);
+ free(s->addr_node.key);
if (s->use_ssl == 1 || s->check.use_ssl == 1 ||
(s->proxy->options & PR_O_TCPCHK_SSL)) {
if (xprt_get(XPRT_SSL) &&
xprt_get(XPRT_SSL)->destroy_srv)
--
2.30.0
Re: [PATCH] A bunch of spelling fixes.
Here it is split up into multiple patches.
I also included a patch that adds a few words to the
ignore-words-list in the codespell workflow.
-- >8 --
>From 65d9fd1583841a92afdc276c8fca72ed7f49345b Mon Sep 17 00:00:00 2001
From: Thayne McCombs
Date: Thu, 7 Jan 2021 21:24:41 -0700
Subject: [PATCH 1/4] Spelling fixes in documentation
---
BRANCHES | 2 +-
CONTRIBUTING | 2 +-
INSTALL | 2 +-
doc/configuration.txt| 10 +-
doc/internals/acl.txt| 6 +++---
doc/internals/buffer-api.txt | 2 +-
doc/internals/htx-api.txt| 4 ++--
doc/intro.txt| 2 +-
doc/management.txt | 2 +-
9 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/BRANCHES b/BRANCHES
index 6cb275c94..53b2ee996 100644
--- a/BRANCHES
+++ b/BRANCHES
@@ -134,7 +134,7 @@ to make a safe guess about what to pick.
Branches up to 1.8 are all designated as "long-term supported" ("LTS" for
short), which means that they are maintained for several years after the
release. These branches were emitted at a pace of one per year since 1.5 in
-2014. As of 2019, 1.5 is still supported and widely used, eventhough it very
+2014. As of 2019, 1.5 is still supported and widely used, even though it very
rarely receives updates. After a few years these LTS branches enter a
"critical fixes only" status, which means that they will rarely receive a fix
but if that a critital issue affects them, a release will be made, with or
diff --git a/CONTRIBUTING b/CONTRIBUTING
index 88733e19e..73a27c7db 100644
--- a/CONTRIBUTING
+++ b/CONTRIBUTING
@@ -154,7 +154,7 @@ features are disabled. Similarly, when modifying the SSL
stack, please always
ensure that supported OpenSSL versions continue to build and to work,
especially
if you modify support for alternate libraries. Clean support for the legacy
OpenSSL libraries is mandatory, support for its derivatives is a bonus and may
-occasionally break eventhough a great care is taken. In other words, if you
+occasionally break even though a great care is taken. In other words, if you
provide a patch for OpenSSL you don't need to test its derivatives, but if you
provide a patch for a derivative you also need to test with OpenSSL.
diff --git a/INSTALL b/INSTALL
index 32cf5d91e..32c0dd338 100644
--- a/INSTALL
+++ b/INSTALL
@@ -528,7 +528,7 @@ because of strange symbols or section mismatches, simply
remove -g from
DEBUG_CFLAGS.
Building on AIX 7.2 works fine using the "aix72-gcc" TARGET. It adds two
-special CFLAGS to prevent the loading of AIXs xmem.h and var.h. This is done
+special CFLAGS to prevent the loading of AIX's xmem.h and var.h. This is done
by defining the corresponding include-guards _H_XMEM and _H_VAR. Without
excluding those header-files the build fails because of redefinition errors.
Furthermore, the atomic library is added to the LDFLAGS to allow for
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 31ab5906b..d357b89c2 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -406,7 +406,7 @@ HAProxy's configuration process involves 3 major sources of
parameters :
- the arguments from the command-line, which always take precedence
- the configuration file(s), whose format is described here
- - the running process' environment, in case some environment variables are
+ - the running process's environment, in case some environment variables are
explicitly referenced
The configuration file follows a fairly simple hierarchical format which obey
@@ -1083,7 +1083,7 @@ external-check
"insecure-setuid-wanted".
gid
- Changes the process' group ID to . It is recommended that the group
+ Changes the process's group ID to . It is recommended that the group
ID is dedicated to HAProxy or to a small set of similar daemons. HAProxy must
be started with a user belonging to this group, or with superuser privileges.
Note that if haproxy is started from a user having supplementary groups, it
@@ -1756,7 +1756,7 @@ stats maxconn
possible to change this value with "stats maxconn".
uid
- Changes the process' user ID to . It is recommended that the user ID
+ Changes the process's user ID to . It is recommended that the user ID
is dedicated to HAProxy or to a small set of similar daemons. HAProxy must
be started with superuser privileges in order to be able to switch to another
one. See also "gid" and "user".
@@ -14410,7 +14410,7 @@ weight
HAProxy allows using a host name on the server line to retrieve its IP address
using name servers. By default, HAProxy resolves the name when parsing the
-configuration file, at startup and cache the result for the process' life.
+configuration file, at startup and cache the result for the process's life.
This is not sufficient in some cases, such as in Amazon where a server's IP
can change
Re: [PATCH] A bunch of spelling fixes.
okay, sounds good. I'll send up separated patches later today. > `iff` means `if and only if`. I was aware of this abbreviation, but wasn't sure if that was the intended meaning or if it was a typo. Do you think it would be better to spell it out as "if and only if", or to add iff to the list of ignored words? Thayne McCombs Senior Software Engineer tha...@lucidchart.com <https://www.golucid.co/> Lucid.co <http://lucid.co> On Thu, Jan 7, 2021 at 8:36 AM Tim Düsterhus wrote: > Thayne, > > Am 07.01.21 um 10:09 schrieb Thayne McCombs: > > diff --git a/doc/internals/htx-api.txt b/doc/internals/htx-api.txt > > index e783e0ebf..07224821c 100644 > > --- a/doc/internals/htx-api.txt > > +++ b/doc/internals/htx-api.txt > > @@ -113,7 +113,7 @@ payload. > > > > Because the payloads part may wrap, there are 2 usable free spaces: > > > > -- The free space in front of the blocks part. This one is used iff > > the other > > +- The free space in front of the blocks part. This one is used if > > the other > >one was not used yet. > > > > - The free space at the beginning of the message. Once this one is > > used, the > > Didn't check all in detail, but that one is most likely not a typo. > `iff` means `if and only if`. > > Best regards > Tim Düsterhus >
[PATCH] A bunch of spelling fixes.
From the output of codespell.
---
BRANCHES | 2 +-
CONTRIBUTING | 2 +-
INSTALL| 2 +-
Makefile | 4 ++--
doc/configuration.txt | 10 +-
doc/internals/acl.txt | 6 +++---
doc/internals/buffer-api.txt | 2 +-
doc/internals/htx-api.txt | 2 +-
doc/intro.txt | 2 +-
doc/management.txt | 2 +-
include/haproxy/action-t.h | 2 +-
include/haproxy/connection-t.h | 4 ++--
include/haproxy/listener.h | 2 +-
include/haproxy/proxy-t.h | 2 +-
include/haproxy/server-t.h | 2 +-
include/import/ebmbtree.h | 2 +-
scripts/announce-release | 2 +-
src/backend.c | 2 +-
src/fd.c | 2 +-
src/haproxy.c | 2 +-
src/hlua.c | 16
src/http.c | 2 +-
src/http_act.c | 2 +-
src/htx.c | 4 ++--
src/pattern.c | 4 ++--
src/proxy.c| 2 +-
src/stream_interface.c | 6 +++---
27 files changed, 46 insertions(+), 46 deletions(-)
diff --git a/BRANCHES b/BRANCHES
index 6cb275c94..53b2ee996 100644
--- a/BRANCHES
+++ b/BRANCHES
@@ -134,7 +134,7 @@ to make a safe guess about what to pick.
Branches up to 1.8 are all designated as "long-term supported" ("LTS" for
short), which means that they are maintained for several years after the
release. These branches were emitted at a pace of one per year since
1.5 in
-2014. As of 2019, 1.5 is still supported and widely used, eventhough it
very
+2014. As of 2019, 1.5 is still supported and widely used, even though
it very
rarely receives updates. After a few years these LTS branches enter a
"critical fixes only" status, which means that they will rarely
receive a fix
but if that a critital issue affects them, a release will be made, with or
diff --git a/CONTRIBUTING b/CONTRIBUTING
index 88733e19e..73a27c7db 100644
--- a/CONTRIBUTING
+++ b/CONTRIBUTING
@@ -154,7 +154,7 @@ features are disabled. Similarly, when modifying the
SSL stack, please always
ensure that supported OpenSSL versions continue to build and to work,
especially
if you modify support for alternate libraries. Clean support for the
legacy
OpenSSL libraries is mandatory, support for its derivatives is a bonus
and may
-occasionally break eventhough a great care is taken. In other words, if you
+occasionally break even though a great care is taken. In other words,
if you
provide a patch for OpenSSL you don't need to test its derivatives,
but if you
provide a patch for a derivative you also need to test with OpenSSL.
diff --git a/INSTALL b/INSTALL
index 32cf5d91e..32c0dd338 100644
--- a/INSTALL
+++ b/INSTALL
@@ -528,7 +528,7 @@ because of strange symbols or section mismatches,
simply remove -g from
DEBUG_CFLAGS.
Building on AIX 7.2 works fine using the "aix72-gcc" TARGET. It adds two
-special CFLAGS to prevent the loading of AIXs xmem.h and var.h. This is
done
+special CFLAGS to prevent the loading of AIX's xmem.h and var.h. This
is done
by defining the corresponding include-guards _H_XMEM and _H_VAR. Without
excluding those header-files the build fails because of redefinition
errors.
Furthermore, the atomic library is added to the LDFLAGS to allow for
diff --git a/Makefile b/Makefile
index 4e6c834ed..fba5d4267 100644
--- a/Makefile
+++ b/Makefile
@@ -30,7 +30,7 @@
# USE_TPROXY : enable transparent proxy. Automatic.
# USE_LINUX_TPROXY : enable full transparent proxy. Automatic.
# USE_LINUX_SPLICE : enable kernel 2.6 splicing. Automatic.
-# USE_LIBCRYPT : enable crypted passwords using -lcrypt
+# USE_LIBCRYPT : enable encrypted passwords using -lcrypt
# USE_CRYPT_H : set it if your system requires including
crypt.h
# USE_GETADDRINFO : use getaddrinfo() to resolve IPv6 host names.
# USE_OPENSSL : enable use of OpenSSL. Recommended, but see
below.
@@ -435,7 +435,7 @@ endif
ifeq ($(TARGET),cygwin)
set_target_defaults = $(call default_opts, \
USE_POLL USE_TPROXY USE_OBSOLETE_LINKER)
- # Cygwin adds IPv6 support only in version 1.7 (in beta right now).
+ # Cygwin adds IPv6 support only in version 1.7 (in beta right now).
TARGET_CFLAGS = $(if $(filter 1.5.%, $(shell uname -r)), -DUSE_IPV6
-DAF_INET6=23 -DINET6_ADDRSTRLEN=46, )
endif
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 31ab5906b..d357b89c2 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -406,7 +406,7 @@ HAProxy's configuration process involves 3 major
sources of parameters :
- the arguments from the command-line, which always take precedence
- the configuration file(s), whose format is described here
- - the running process' environment, in case some environment
variables are
+ - the running process's environment,
Re: [PATCH] BUG/MINOR: Error out when a `server` has an `AF_UNSPEC` address
On Mon, Jan 04, 2021 at 12:58:25AM +0100, Tim Duesterhus wrote:
> I am adding Thayne as CC as it was your commit that uncovered the issue and
> the
> crash happened in a function you wrote. Maybe you might want to add some
> additional checks somewhere?
Oops, my bad. I actually meant to put a NULL check in there, but must have
forgotten.
On 1/5/21 3:37 AM, Willy Tarreau wrote:
> Unfortunately we cannot do that or it destroys usage of the DNS or
> even any runtime address assignment over the CLI. For example if you
> write:
>
> server foo foo init-addr none
>
> I guess it won't work anymore since that's placed just after the address
> parsing. This means however that there probably is something more
> problematic in the referencing of address-less servers. Either servers
> are added/updated each time their address changes (to follow DNS) and
> then we can simply skip their registration when they're address-less.
> Or the servers are only registered at boot time, and the synchronization
> will fail after any address update. What do you think, Thayne ?
I'm partial to skipping registration when they are address-less.
Here's a patch that I think should fix this:
-- >8 --
Subject: [PATCH] Fix a segfault in srv_set_addr_desc
Check to make sure the address key is non-null before using it for
comparison or inserting it into the tree.
---
src/server.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/server.c b/src/server.c
index 50c6da131..d1aa51dc8 100644
--- a/src/server.c
+++ b/src/server.c
@@ -204,7 +204,7 @@ static void srv_set_addr_desc(struct server *s)
key = sa2str(>addr, s->svc_port, s->flags & SRV_F_MAPPORTS);
if (s->addr_node.key) {
- if (strcmp(key, s->addr_node.key) == 0) {
+ if (key && strcmp(key, s->addr_node.key) == 0) {
free(key);
return;
}
@@ -218,9 +218,11 @@ static void srv_set_addr_desc(struct server *s)
s->addr_node.key = key;
- HA_RWLOCK_WRLOCK(PROXY_LOCK, >lock);
- ebis_insert(>used_server_addr, >addr_node);
- HA_RWLOCK_WRUNLOCK(PROXY_LOCK, >lock);
+ if (s->addr_node.key) {
+ HA_RWLOCK_WRLOCK(PROXY_LOCK, >lock);
+ ebis_insert(>used_server_addr, >addr_node);
+ HA_RWLOCK_WRUNLOCK(PROXY_LOCK, >lock);
+ }
}
/*
--
2.30.0
Re: [PR] Add srvkey option to stick-table
Awesome! Thanks everyone. Thayne McCombs Senior Software Engineer tha...@lucidchart.com <https://www.golucid.co/> Lucid.co <http://lucid.co> On Thu, Dec 31, 2020 at 2:10 AM Willy Tarreau wrote: > On Tue, Dec 29, 2020 at 10:52:56AM +0100, Frederic Lecaille wrote: > > > +#REQUIRE_VERSION=2.0 > > > > Ok for this test but I think we should use 2.4 which is the current > > developemnt version. > > OK I applied this change. Tested here and it works. > > > Willy, I think we can import this patch after having merged the previous > one > > [1/2] patch about this feature implementation. > > Now done. > > Thanks! > Willy >
Re: [PR] Add srvkey option to stick-table
> I am not sure we will import the second patch for the reg test: it makes usage of curl. We try to not use external programs for reg tests except if there is no choice. Fine by me. I actually meant to say that my second attempt at writing a reg attempt should replace that patch. Apparently I forgot. Thayne McCombs Senior Software Engineer tha...@lucidchart.com <https://www.golucid.co/> Lucid.co <http://lucid.co> On Tue, Dec 29, 2020 at 2:52 AM Frederic Lecaille wrote: > On 12/19/20 9:07 AM, Thayne McCombs wrote: > > From 731d6a00f3cf0c70d7a8a916e1984c225f3a9dd6 Mon Sep 17 00:00:00 2001 > > From: Thayne McCombs > > Date: Sat, 19 Dec 2020 00:59:35 -0700 > > Subject: [PATCH] Add test for stickiness using the server address for the > > server key > > > > --- > > reg-tests/stickiness/srvkey-addr.vtc | 259 +++ > > 1 file changed, 259 insertions(+) > > create mode 100644 reg-tests/stickiness/srvkey-addr.vtc > > > > diff --git a/reg-tests/stickiness/srvkey-addr.vtc > > b/reg-tests/stickiness/srvkey-addr.vtc > > new file mode 100644 > > index 0..b5675089f > > --- /dev/null > > +++ b/reg-tests/stickiness/srvkey-addr.vtc > > @@ -0,0 +1,259 @@ > > +vtest "A reg test for stickiness with srvkey addr" > > +feature ignore_unknown_macro > > + > > + > > +# The aim of this test is to check that "stick on" rules > > +# do the job they are supposed to do. > > +# If we remove one of the "stick on" rule, this script fails. > > + > > +#REQUIRE_VERSION=2.0 > > Ok for this test but I think we should use 2.4 which is the current > developemnt version. > > Willy, I think we can import this patch after having merged the previous > one [1/2] patch about this feature implementation. >
Re: [PR] Add srvkey option to stick-table
On 12/10/20 1:36 AM, Frederic Lecaille wrote:
About a possible reg test, I remember there exists already one to test
the stickiness. Have a look to reg-tests/stickiness/lb-services.vtc.
I think it should be preferable to make a new reg test which would be a
copy of this latter but with the "server key" feature you have implemented.
Here it is:
From 731d6a00f3cf0c70d7a8a916e1984c225f3a9dd6 Mon Sep 17 00:00:00 2001
From: Thayne McCombs
Date: Sat, 19 Dec 2020 00:59:35 -0700
Subject: [PATCH] Add test for stickiness using the server address for the
server key
---
reg-tests/stickiness/srvkey-addr.vtc | 259 +++
1 file changed, 259 insertions(+)
create mode 100644 reg-tests/stickiness/srvkey-addr.vtc
diff --git a/reg-tests/stickiness/srvkey-addr.vtc
b/reg-tests/stickiness/srvkey-addr.vtc
new file mode 100644
index 0..b5675089f
--- /dev/null
+++ b/reg-tests/stickiness/srvkey-addr.vtc
@@ -0,0 +1,259 @@
+vtest "A reg test for stickiness with srvkey addr"
+feature ignore_unknown_macro
+
+
+# The aim of this test is to check that "stick on" rules
+# do the job they are supposed to do.
+# If we remove one of the "stick on" rule, this script fails.
+
+#REQUIRE_VERSION=2.0
+
+server s_not_used_1 {}
+server s_not_used_2 {}
+server s_not_used_3 {}
+server s_not_used_4 {}
+server s_not_used_5 {}
+server s_not_used_6 {}
+server s_not_used_7 {}
+server s_not_used_8 {}
+server s_not_used_9 {}
+server s_not_used_10 {}
+server s_not_used_11 {}
+server s_not_used_12 {}
+
+# h1/be1 servers
+server s1 {
+rxreq
+txresp -hdr "Server: s1"
+} -repeat 8 -start
+
+server s2 {
+rxreq
+txresp -hdr "Server: s2"
+} -repeat 8 -start
+
+haproxy h1 -arg "-L A" -conf {
+defaults
+mode http
+${no-htx} option http-use-htx
+timeout server 1s
+timeout connect 1s
+timeout client 1s
+log stdout format raw local0 debug
+
+peers mypeers
+bind "fd@${A}"
+server A
+server B ${h2_B_addr}:${h2_B_port}
+table mytable type string size 10m srvkey addr
+
+backend be1
+balance roundrobin
+stick on urlp(client) table mypeers/mytable
+server srv1 ${s1_addr}:${s1_port}
+server srv2 ${s2_addr}:${s2_port}
+
+backend be2
+balance roundrobin
+stick on urlp(client) table mypeers/mytable
+server s_not_used_1 ${s_not_used_1_addr}:${s_not_used_1_port}
+server s_not_used_2 ${s_not_used_2_addr}:${s_not_used_2_port}
+server s_not_used_3 ${s_not_used_3_addr}:${s_not_used_3_port}
+server srv2_2 ${s2_addr}:${s2_port}
+server s_not_used_4 ${s_not_used_4_addr}:${s_not_used_4_port}
+server s_not_used_5 ${s_not_used_5_addr}:${s_not_used_5_port}
+server s_not_used_6 ${s_not_used_6_addr}:${s_not_used_6_port}
+server srv1_2 ${s1_addr}:${s1_port}
+
+frontend fe
+acl acl_be1 path_beg /be1
+acl acl_be2 path_beg /be2
+use_backend be1 if acl_be1
+use_backend be2 if acl_be2
+bind "fd@${fe}"
+} -start
+
+haproxy h2 -arg "-L B" -conf {
+defaults
+mode http
+${no-htx} option http-use-htx
+timeout server 1s
+timeout connect 1s
+timeout client 1s
+
+peers mypeers
+bind "fd@${B}"
+server A ${h1_A_addr}:${h1_A_port}
+server B
+table mytable type string size 10m srvkey addr
+
+backend be1
+balance roundrobin
+stick on urlp(client) table mypeers/mytable
+server s_not_used_7 ${s_not_used_7_addr}:${s_not_used_7_port}
+server s_not_used_8 ${s_not_used_8_addr}:${s_not_used_8_port}
+server s_not_used_9 ${s_not_used_9_addr}:${s_not_used_9_port}
+server srv1_h2_1 ${s1_addr}:${s1_port}
+server s_not_used_10 ${s_not_used_10_addr}:${s_not_used_10_port}
+server s_not_used_11 ${s_not_used_11_addr}:${s_not_used_11_port}
+server s_not_used_12 ${s_not_used_12_addr}:${s_not_used_12_port}
+server srv2_h2_1 ${s2_addr}:${s2_port}
+
+backend be2
+balance roundrobin
+stick on urlp(client) table mypeers/mytable
+server s_not_used_1 ${s_not_used_1_addr}:${s_not_used_1_port}
+server s_not_used_2 ${s_not_used_2_addr}:${s_not_used_2_port}
+server s_not_used_3 ${s_not_used_3_addr}:${s_not_used_3_port}
+server s_not_used_4 ${s_not_used_4_addr}:${s_not_used_4_port}
+server s_not_used_5 ${s_not_used_5_addr}:${s_not_used_5_port}
+server s_not_used_6 ${s_not_used_6_addr}:${s_not_used_6_port}
+server srv1_h2_2 ${s1_addr}:${s1_port}
+server srv2_h2_2 ${s2_addr}:${s2_port}
+
+frontend fe
+acl acl_be1 path_beg /be1
+acl acl_be2 path_beg /be2
+use_backend be1 if acl_be1
+use_backend be2 if acl_be2
+bind "fd@${fe}&quo
Re: [PR] Add srvkey option to stick-table
On 12/10/20 1:31 AM, Frederic Lecaille wrote:
>
> It would be preferable to send all your patches, so that others than me
> may review your work (no diff between different versions of patches) and
> continue to split your work in several patches.
>
Ok, here is what I have so far as two patches (I combined feedback into the
original commit):
>From cf965f47e04776ca20d2ee6ed22028741493824c Mon Sep 17 00:00:00 2001
From: Thayne McCombs
Date: Fri, 20 Nov 2020 01:28:26 -0700
Subject: [PATCH 1/2] Add srvkey option to stick-table
This allows using the address of the server rather than the name of the
server for keeping track of servers in a backend for stickiness.
Fixes #814
---
doc/configuration.txt | 12 -
include/haproxy/dict.h | 1 +
include/haproxy/proxy-t.h | 1 +
include/haproxy/server-t.h | 1 +
include/haproxy/server.h| 2 +-
include/haproxy/stick_table-t.h | 11 ++--
include/haproxy/tools.h | 13 +
src/cfgparse-listen.c | 1 +
src/cfgparse.c | 4 +--
src/dict.c | 24 -
src/peers.c | 9 +--
src/server.c| 40 ++--
src/stick_table.c | 31 +-
src/stream.c| 47 +++--
src/tools.c | 45 +++
15 files changed, 216 insertions(+), 26 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index e60e3428d..e17061518 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -10649,7 +10649,7 @@ stick store-request [table ] [{if |
unless} ]
stick-table type {ip | integer | string [len ] | binary [len ]}
-size [expire ] [nopurge] [peers ]
+size [expire ] [nopurge] [peers ] [srvkey
]
[store ]*
Configure the stickiness table for the current section
May be used in sections : defaults | frontend | listen | backend
@@ -10726,6 +10726,16 @@ stick-table type {ip | integer | string [len ]
| binary [len ]}
be removed once full. Be sure not to use the "nopurge" parameter
if not expiration delay is specified.
+ specifies how each server is identified for the purposes of the
+ stick table. The valid values are "name" and "addr". If "name"
is
+ given, then argument for the server (may be generated by
+ a template). If "addr" is given, then the server is identified
+ by its current network address, including the port. "addr" is
+ especially useful if you are using service discovery to generate
+ the addresses for servers with peered stick-tables and want
+ to consistently use the same host across peers for a stickiness
+ token.
+
is used to store additional information in the stick-table. This
may be used by ACLs in order to control various criteria related
to the activity of the client matching the stick-table. For each
diff --git a/include/haproxy/dict.h b/include/haproxy/dict.h
index 59e81352c..c55834ca5 100644
--- a/include/haproxy/dict.h
+++ b/include/haproxy/dict.h
@@ -31,5 +31,6 @@
struct dict *new_dict(const char *name);
struct dict_entry *dict_insert(struct dict *d, char *str);
+void dict_entry_unref(struct dict *d, struct dict_entry *de);
#endif /* _HAPROXY_DICT_H */
diff --git a/include/haproxy/proxy-t.h b/include/haproxy/proxy-t.h
index 998e210f6..e62b79765 100644
--- a/include/haproxy/proxy-t.h
+++ b/include/haproxy/proxy-t.h
@@ -424,6 +424,7 @@ struct proxy {
char *lfsd_file;/* file name where the
structured-data logformat string for RFC5424 appears (strdup) */
int lfsd_line; /* file name where the
structured-data logformat string for RFC5424 appears */
} conf; /* config information */
+ struct eb_root used_server_addr;/* list of server addresses in
use */
void *parent; /* parent of the proxy when
applicable */
struct comp *comp; /* http compression */
diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 0e66be693..13f5a5dab 100644
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -337,6 +337,7 @@ struct server {
struct ebpt_node name; /* place in the tree of used
names */
int line; /* line where the section
appears */
} conf; /* config information */
+ struct ebpt_node addr_node; /* Node for string
representation of address for the server (including port number) */
Re: [PR] Add srvkey option to stick-table
Here are my updates from the feedback. It took me quite a while to figure out
how to send this diff properly formatted with gmail.
If you would like a single patch with all my changes, I can send that as well.
>From 9c71b643e993dcafca36feae71cdfacc24ffaaa2 Mon Sep 17 00:00:00 2001
From: Thayne McCombs
Date: Sat, 5 Dec 2020 23:09:24 -0700
Subject: [PATCH 1/2] Feedback from initial review
---
doc/configuration.txt | 4 ++--
include/haproxy/dict.h | 1 +
include/haproxy/server-t.h | 1 -
src/cfgparse-listen.c | 1 +
src/dict.c | 24 +-
src/peers.c| 7 ++-
src/server.c | 41 --
src/stick_table.c | 8
src/stream.c | 2 +-
src/tools.c| 3 ++-
10 files changed, 66 insertions(+), 26 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 95c9abd8c..d1f7374ea 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -10711,8 +10711,8 @@ stick-table type {ip | integer | string [len ]
| binary [len ]}
specifies how each server is identified for the purposes of the
stick table. The valid values are "name" and "addr". If "name"
is
given, then argument for the server (may be generated by
- a template). If "addr" is given, then the server is idntified
- by it's current network address, including the port. "addr" is
+ a template). If "addr" is given, then the server is identified
+ by its current network address, including the port. "addr" is
especially useful if you are using service discovery to generate
the addresses for servers with peered stick-tables and want
to consistently use the same host across peers for a stickiness
diff --git a/include/haproxy/dict.h b/include/haproxy/dict.h
index 59e81352c..c55834ca5 100644
--- a/include/haproxy/dict.h
+++ b/include/haproxy/dict.h
@@ -31,5 +31,6 @@
struct dict *new_dict(const char *name);
struct dict_entry *dict_insert(struct dict *d, char *str);
+void dict_entry_unref(struct dict *d, struct dict_entry *de);
#endif /* _HAPROXY_DICT_H */
diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 5a4dadfbd..13f5a5dab 100644
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -274,7 +274,6 @@ struct server {
const struct netns_entry *netns;/* contains network namespace
name or NULL. Network namespace comes from configuration */
/* warning, these structs are huge, keep them at the bottom */
struct sockaddr_storage addr; /* the address to connect to,
doesn't include the port */
- char *addr_desc;/* string description of the
address and port for the server */
struct xprt_ops *xprt; /* transport-layer operations */
unsigned int svc_port; /* the port to connect to (for
relevant families) */
unsigned down_time; /* total time the server was
down */
diff --git a/src/cfgparse-listen.c b/src/cfgparse-listen.c
index 97a97e746..a493e741c 100644
--- a/src/cfgparse-listen.c
+++ b/src/cfgparse-listen.c
@@ -457,6 +457,7 @@ int cfg_parse_listen(const char *file, int linenum, char
**args, int kwm)
curproxy->grace = defproxy.grace;
curproxy->conf.used_listener_id = EB_ROOT;
curproxy->conf.used_server_id = EB_ROOT;
+ curproxy->used_server_addr = EB_ROOT_UNIQUE;
if (defproxy.check_path)
curproxy->check_path = strdup(defproxy.check_path);
diff --git a/src/dict.c b/src/dict.c
index 903f07323..9b3536d96 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -87,8 +87,10 @@ struct dict_entry *dict_insert(struct dict *d, char *s)
HA_RWLOCK_RDLOCK(DICT_LOCK, >rwlock);
de = __dict_lookup(d, s);
HA_RWLOCK_RDUNLOCK(DICT_LOCK, >rwlock);
- if (de)
+ if (de) {
+ HA_ATOMIC_ADD(>refcount, 1);
return de;
+ }
de = new_dict_entry(s);
if (!de)
@@ -105,3 +107,23 @@ struct dict_entry *dict_insert(struct dict *d, char *s)
return de;
}
+
+/*
+ * Unreference a dict entry previously acquired with .
+ * If this is the last live reference to the entry, it is
+ * removed from the dictionary.
+ */
+void dict_entry_unref(struct dict *d, struct dict_entry *de)
+{
+ if (!de)
+ return;
+
+ if (HA_ATOMIC_SUB(>refcount, 1) != 0)
+ return;
+
+ HA_RWLOCK_WRLOCK(DICT_LOCK, >rwlock);
+ ebpt_delete(>value);
+ HA_RWLOCK_WRUNLOCK(DICT_LOCK, >rwlock);
+
+ free_dict_entry(de);
+}
diff --git a/src/peers.c b/src/pee
Re: [PR] Add srvkey option to stick-table
Thanks for your feedback. I have some followup questions.
> I think you should initialized this ebtree with EB_ROOT_UNIQUE as
value.
I'm not sure I understand what the distinction between EB_ROOT and
EB_ROOT_UNIQUE is. I'm happy to add this, but I'd like to understand
why it should be EB_ROOT_UNIQUE here (since it looks like it is
initialized as EB_ROOT for used_server_id and used_listener_id, and I
can't find where it is initialized for used_server_name, which I
believe would be equivalent since EB_ROOT is basically just zero
initialized).
> I think this is dangerous to initialize two objects with the same value
as a string. I would strdup() to initialize s->addr_node.key.
ok. I was following the pattern used with conf.name in struct server.
Would it be better to use strdup, or to not have a separate field for
addr_desc, and just use addr_node.key (which is a void*)? If the
latter maybe rename addr_node as addr_desc?
Additionally:
In srv_set_addr_desc, would it be better to hold the lock over the
entire operation, or just while we are actually manipulating the tree?
Meaning should I release the lock between deleting the old node and
inserting the new node? And should this use the existing proxy lock,
or should there be a separate dedicated lock for used_server_addr?
I'm also a little bit concerned about leaking memory in the
server_key_dict. I couldn't find anything that cleans up these keys,
although there is a refcount on them. In an environment that uses
service discovery and the servers change frequently, this could lead
to a memory leak as this dict only grows in size.
Thank you,
Thayne
Thayne McCombs
Senior Software Engineer
tha...@lucidchart.com
Lucid.co
On Tue, Dec 1, 2020 at 10:24 AM Frederic Lecaille wrote:
>
> On 12/1/20 11:50 AM, Emeric Brun wrote:
> > Hi,
>
> Hi Thayne,
>
> See my answers below.
>
> > On 11/30/20 10:23 AM, PR Bot wrote:
> >> Dear list!
> >>
> >> Author: Thayne McCombs
> >> Number of patches: 2
> >>
> >> This is an automated relay of the Github pull request:
> >> Add srvkey option to stick-table
> >>
> >> Patch title(s):
> >> Add srvkey option to stick-table
> >> Harden sa2str agains 107-byte-long abstract unix domain path
> >>
> >> Link:
> >> https://github.com/haproxy/haproxy/pull/979
> >>
> >> Edit locally:
> >> wget https://github.com/haproxy/haproxy/pull/979.patch && vi 979.patch
> >>
> >> Apply locally:
> >> curl https://github.com/haproxy/haproxy/pull/979.patch | git am -
> >>
> >> Description:
> >> This allows using the address of the server rather than the name of
> >> the
> >> server for keeping track of servers in a backend for
> >> stickiness.
> >>
> >> Fixes #814
> >>
> >> I haven't tested this at all
> >> yet, and it still needs some polish, but here is a draft of how to fix
> >> #814.
> >>
> >> This is my first significant contribution to haproxy,
> >> so I would not be surprised if I'm doing something terribly wrong, and
> >> I'm sure there are at least some small mistakes in it. Initial
> >> feedback would be very welcome.
>
> Thank you for this first contribution which looks almost good!!! We all
> do wrong things ;) Even if it not easy to comment your code because it
> is not included in this mail, let's giving a try.
>
>
> I have noticed two places where initializations are missing:
>
>
> diff --git a/include/haproxy/proxy-t.h b/include/haproxy/proxy-t.h
> index 998e210f6..e62b79765 100644
> --- a/include/haproxy/proxy-t.h
> +++ b/include/haproxy/proxy-t.h
> @@ -424,6 +424,7 @@ struct proxy {
> char *lfsd_file;/* file name where the
> structured-data logformat
> string for RFC5424 appears (strdup) */
> int lfsd_line; /* file name where the
> structured-data logformat
> string for RFC5424 appears */
> } conf; /* config information */
> + struct eb_root used_server_addr;/* list of server addresses in
> use */
>
>
> < used_server_addr> ebtree is not initialized. This would make haproxy
> crash. I think you should initialized this ebtree with EB_ROOT_UNIQUE as
> value.
>
>
> You should comment all the new functions (see srv_set_addr_desc() and
> perhaps others).
>
> The following function should be commente as this done for
> srv_set_dyncookie() espcially for the locks we need even if this
> function is only used at parsing time, as far as I see. What if we wan
Ambiguity in documentation for `http-request set-header`
The documentation for `http-request set-header` is a little ambiguous about whether it removes all occurrences of the header if it previously existed or just the first one. From experimentation it appears it is all occurrences (which I think is preferable). May I suggest rewording "except that the header name is first removed if it existed" to "except that all occurrences of the header name are first removed if any existed"?
Re: log-format in defaults section in 1.7
Thank you! That's good to know. On Thu, Nov 2, 2017 at 5:36 PM Cyril Bonté <cyril.bo...@free.fr> wrote: > Hi Thayne, > > Le 02/11/2017 à 23:08, Thayne McCombs a écrit : > > So, I looked into using `no log` in non http frontends. But that isn't > > sufficient. > > > > For example, if I have: > > > > global > >log-tag "test" > >log localhost:514 len 65535 local2 info info > > > > defaults > >mode http > >timeout connect 100 > >timeout server 3 > >timeout client 3 > >log-format "%Tq" > > > > listen mine > >log global > >bind :80 > >server localhost localhost:8080 > > > > listen health_url > >bind :27000 > >mode health > >option httpchk > >no log > > > > > > I still get [ALERT] 305/160229 (21975) : Parsing [test.cfg:10]: failed > > to parse log-format : format variable 'Tq' is reserved for HTTP mode. > > You can specify several "defaults" sections in your configuration : one > for http, and one for tcp frontends. > > global >log-tag "test" >log localhost:514 len 65535 local2 info info > > defaults >mode http >timeout connect 100 >timeout server 3 >timeout client 3 >log-format "%Tq" > > listen mine >log global >bind :8080 >server localhost localhost:80 > > # ... > # Other HTTP frontends > # ... > > defaults >mode tcp >timeout connect 100 >timeout server 3 >timeout client 3 > > listen health_url >bind :27000 >mode health >option httpchk > > # ... > # Other TCP frontends > # ... > > > > However, if I add `log-format "GARBAGE"` to the health_url listener, > > then the error goes away. > > Or you can specify "option tcplog" in your "health_url" section (or any > other tcp sections). > > > -- > Cyril Bonté > -- *Thayne McCombs* *Senior Software Engineer* Lucid Software, Inc.
Re: log-format in defaults section in 1.7
So, I looked into using `no log` in non http frontends. But that isn't sufficient. For example, if I have: global log-tag "test" log localhost:514 len 65535 local2 info info defaults mode http timeout connect 100 timeout server 3 timeout client 3 log-format "%Tq" listen mine log global bind :80 server localhost localhost:8080 listen health_url bind :27000 mode health option httpchk no log I still get [ALERT] 305/160229 (21975) : Parsing [test.cfg:10]: failed to parse log-format : format variable 'Tq' is reserved for HTTP mode. However, if I add `log-format "GARBAGE"` to the health_url listener, then the error goes away. It seems like if I specify `no log` then the log-format should be ignored, even if it comes from the default. On Mon, Oct 9, 2017 at 10:35 AM Thayne McCombs <tha...@lucidpress.com> wrote: > Actually, I just remembered that we do have a few tcp mode frontends. > Maybe that is the reason for the error? Still, is there a way to use a > default log-format for the http frontends? I'm going to try turning logs > off for tcp mode frontends and see if that fixes the error. > > On Mon, Oct 9, 2017 at 10:22 AM Thayne McCombs <tha...@lucidpress.com> > wrote: > >> I am working on upgrading haproxy from 1.6 to 1.7 on our load balancers. >> >> However, on 1.7 with our current config I get the following error: >> >> [ALERT] 278/170234 (8363) : Parsing [/etc/haproxy/haproxy-staged.cfg:31]: >> failed to parse log-format : format variable 'Tq' is reserved for HTTP mode. >> >> The log-format directive is in the *defaults* section, which also has a *mode >> http* directive. Was there a change in 1.7 that made the use of Tq (and >> other http specific variables) illegal in the log-format of a defaults >> section? >> >> All of my frontends are http frontends. Is there any way I can use a >> common default log-format for all of them that uses http variables (for >> example, something like an http-log-format directive) in 1.7? Or do I have >> to duplicate the log-format for all of my frontends? >> >> Thanks, >> >> Thayne McCombs >> Lucid Software, Inc. >> -- >> *Thayne McCombs* >> *Senior Software Engineer* >> Lucid Software, Inc. >> >> -- > *Thayne McCombs* > *Senior Software Engineer* > Lucid Software, Inc. > > -- *Thayne McCombs* *Senior Software Engineer* Lucid Software, Inc.
Re: log-format in defaults section in 1.7
Actually, I just remembered that we do have a few tcp mode frontends. Maybe that is the reason for the error? Still, is there a way to use a default log-format for the http frontends? I'm going to try turning logs off for tcp mode frontends and see if that fixes the error. On Mon, Oct 9, 2017 at 10:22 AM Thayne McCombs <tha...@lucidpress.com> wrote: > I am working on upgrading haproxy from 1.6 to 1.7 on our load balancers. > > However, on 1.7 with our current config I get the following error: > > [ALERT] 278/170234 (8363) : Parsing [/etc/haproxy/haproxy-staged.cfg:31]: > failed to parse log-format : format variable 'Tq' is reserved for HTTP mode. > > The log-format directive is in the *defaults* section, which also has a *mode > http* directive. Was there a change in 1.7 that made the use of Tq (and > other http specific variables) illegal in the log-format of a defaults > section? > > All of my frontends are http frontends. Is there any way I can use a > common default log-format for all of them that uses http variables (for > example, something like an http-log-format directive) in 1.7? Or do I have > to duplicate the log-format for all of my frontends? > > Thanks, > > Thayne McCombs > Lucid Software, Inc. > -- > *Thayne McCombs* > *Senior Software Engineer* > Lucid Software, Inc. > > -- *Thayne McCombs* *Senior Software Engineer* Lucid Software, Inc.
log-format in defaults section in 1.7
I am working on upgrading haproxy from 1.6 to 1.7 on our load balancers. However, on 1.7 with our current config I get the following error: [ALERT] 278/170234 (8363) : Parsing [/etc/haproxy/haproxy-staged.cfg:31]: failed to parse log-format : format variable 'Tq' is reserved for HTTP mode. The log-format directive is in the *defaults* section, which also has a *mode http* directive. Was there a change in 1.7 that made the use of Tq (and other http specific variables) illegal in the log-format of a defaults section? All of my frontends are http frontends. Is there any way I can use a common default log-format for all of them that uses http variables (for example, something like an http-log-format directive) in 1.7? Or do I have to duplicate the log-format for all of my frontends? Thanks, Thayne McCombs Lucid Software, Inc. -- *Thayne McCombs* *Senior Software Engineer* Lucid Software, Inc.
