Introducing Myself + Potential Article Idea for Haproxy
Hi there, I wanted to get in touch about contributing an article to Haproxy. A few areas that I'd be qualified to pontificate on are current trends in infosec, payment systems and platforms, and IoT security. I also love anything related to eCommerce as I run a few Shopify sites on the side. I am a Certified Information Systems Security Professional (CISSP) specialized in Network and IoT Security and have spent most of my career in the APAC region, though I recently relocated from Shenzhen to San Francisco to be closer to family. My writing has been published at Cybersecurity.att.com, Staysafeonline.org and Eccouncil.org. Let me know if you'd like me to further flesh out a few ideas and I'll send a few your right way! Best, David
lua: Add missed lua 5.4 references
Add missed lua 5.4 references Missed in INSTALL and Makefile Lua 5.4 support has already been added (needs backport to 2.0) --- a/INSTALL +++ b/INSTALL @@ -295,9 +295,9 @@ Lua is an embedded programming language supported by HAProxy to provide more advanced scripting capabilities. Only versions 5.3 and above are supported. In order to enable Lua support, please specify "USE_LUA=1" on the command line. Some systems provide this library under various names to avoid conflicts with -previous versions. By default, HAProxy looks for "lua5.3", "lua53", "lua". If -your system uses a different naming, you may need to set the library name in -the "LUA_LIB_NAME" variable. +previous versions. By default, HAProxy looks for "lua5.4", "lua54","lua5.3", +"lua53","lua". If your system uses a different naming, you may need to set +the library name in the "LUA_LIB_NAME" variable. If Lua is not provided on your system, it can be very simply built locally. It can be downloaded from https://www.lua.org/, extracted and built, for example : --- a/Makefile +++ b/Makefile @@ -589,11 +589,11 @@ OPTIONS_CFLAGS += $(if $(LUA_INC),-I$(LUA_INC)) LUA_LD_FLAGS := -Wl,$(if $(EXPORT_SYMBOL),$(EXPORT_SYMBOL),--export-dynamic) $(if $(LUA_LIB),-L$(LUA_LIB)) ifeq ($(LUA_LIB_NAME),) # Try to automatically detect the Lua library -LUA_LIB_NAME := $(firstword $(foreach lib,lua5.3 lua53 lua,$(call check_lua_lib,$(lib),$(LUA_LD_FLAGS +LUA_LIB_NAME := $(firstword $(foreach lib,lua5.4 lua54 lua5.3 lua53 lua,$(call check_lua_lib,$(lib),$(LUA_LD_FLAGS ifeq ($(LUA_LIB_NAME),) -$(error unable to automatically detect the Lua library name, you can enforce its name with LUA_LIB_NAME= (where can be lua5.3, lua53, lua, ...)) +$(error unable to automatically detect the Lua library name, you can enforce its name with LUA_LIB_NAME= (where can be lua5.4, lua54, lua5.3, lua53, lua, ...)) endif -LUA_INC := $(firstword $(foreach lib,lua5.3 lua53 lua,$(call check_lua_inc,$(lib),"/usr/include/"))) +LUA_INC := $(firstword $(foreach lib,lua5.4 lua54 lua5.3 lua53 lua,$(call check_lua_inc,$(lib),"/usr/include/"))) ifneq ($(LUA_INC),) OPTIONS_CFLAGS += -I$(LUA_INC) endif
Re: [PATCH 1/9] MAJOR: contrib/prometheus-exporter: move health check status to labels
On Mon, Feb 1, 2021 at 12:05 PM Pierre Cheynier wrote:
> In addition to this update, I would add some recommendations about the user
> setup in the README ("how do I prevent my prometheus instance to explode
> when scrapping this endpoint?").
> For server-template users:
> -
> params:
> no-maint:
> - empty
>
> Generally speaking, to prevent all server metrics to be saved, except this
> one:
> -
>metric_relabel_configs:
>- source_labels: ['__name__']
> regex: 'haproxy_(process_|frontend_|backend_|server_check_status).*'
> action: keep
agreed taken into account in v2
> "Status of last health check, per state value" ?
updated in v2.
--
William
RE: [PATCH 1/9] MAJOR: contrib/prometheus-exporter: move health check status to labels
De : William Dauchy
Envoyé : samedi 30 janvier 2021 16:21
> this is a follow up of commit c6464591a365bfcf509b322bdaa4d608c9395d75
> ("MAJOR: contrib/prometheus-exporter: move ftd/bkd/srv states to
> labels"). The main goal being to be better aligned with prometheus use
> cases in terms of queries. More specifically to health checks, Pierre C.
> mentioned the possible quirks he had to put in place in order to make
> use of those metrics through prometheus:
>
> by(proxy, check_status) (count_values by(proxy,
> instance) ("check_status", haproxy_server_check_status))
Indeed, we wanted to aggregate at backend level to produce a view which
represent the "number of servers per health status". Still, this was not ideal
since I'm getting the status code and not the status name.
> I am perfectly aware this introduces a lot more metrics but I don't see
> how we can improve the usability without it. The main issue remains in
> the cardinality of the states which are > 20. Prometheus recommends to
> stay below a cardinality of 10 for a given metric but I consider our
> case very specific, because highly linked to the level of precision
> haproxy exposes.
>
> Even before this patch I saw several large production setup (a few
> hundreds of MB in output) which are making use of the scope parameter to
> simply ignore the server metrics, so that the scrapping can be faster,
> and memory consumed on client side not too high. So I believe we should
> eventually continue in that direction and offer more granularity of
> filtering of the output. That being said it is already possible to
> filter out the data on prometheus client side.
True, I think this is the right approach to represent such data in Prometheus.
It will create some challenges for people like us, who use server-templates
and create thousands of backends, but we'll have to deal with it.
In addition to this update, I would add some recommendations about the user
setup in the README ("how do I prevent my prometheus instance to explode
when scrapping this endpoint?").
For server-template users:
-
params:
no-maint:
- empty
Generally speaking, to prevent all server metrics to be saved, except this one:
-
metric_relabel_configs:
- source_labels: ['__name__']
regex: 'haproxy_(process_|frontend_|backend_|server_check_status).*'
action: keep
A long-term alternative (at least for my use-case) would be to provide this
data at
backend-level, as initially suggested here:
https://www.mail-archive.com/haproxy@formilux.org/msg35369.html
> Signed-off-by: William Dauchy
> ---
> @@ -319,7 +320,7 @@ const struct ist promex_st_metric_desc[ST_F_TOTAL_FIELDS]
> = {
> [ST_F_RATE] = IST("Current number of sessions per second
> over last elapsed second."),
> [ST_F_RATE_LIM] = IST("Configured limit on new sessions per
> second."),
> [ST_F_RATE_MAX] = IST("Maximum observed number of sessions per
> second."),
> - [ST_F_CHECK_STATUS] = IST("Status of last health check
> (HCHK_STATUS_* values)."),
> + [ST_F_CHECK_STATUS] = IST("Status of last health check (0/1
> depending on current `state` label value)."),
"Status of last health check, per state value" ?
--
Pierre
Re: [PATCH 0/9] prometheus: health check as labels + cleanup
On Mon, Feb 01, 2021 at 11:30:18AM +0100, Christopher Faulet wrote:
> Don't be sorry to send patches ! If you think the number of labels for the
> check_status metric is not a problem, even for huge configurations, I trust
> you.
> And I guess we can reduce this number to 16 status only, removing all status
> with an unknown check result (HCHK_STATUS_UNKNOWN, HCHK_STATUS_INI,
> HCHK_STATUS_START, HCHK_STATUS_L57DATA) and the status regarding the
> agent-check
> only (HCHK_STATUS_CHECKED).
>
> For info, HCHK_STATUS_UNKNOWN is only used for uninitialized health-check
> (during configuration parsing). HCHK_STATUS_INI is used for initialized
> health-check and before the first run. HCHK_STATUS_START and
> HCHK_STATUS_L57DATA
> are dummy status and never assigned to a health-check. And as said,
> HCHK_STATUS_CHECKED is only used for agent-check.
>
> It is a slight improvement, but still better than nothing. To do so, I propose
> you to add a function in check.c to get the corresponding check result of a
> status (the attached patch). This way, in your first patch, we can filter on
> check result. I may amend your first patch this way if you're ok:
definitely worth it indeed. I let you do the amend
> --- a/contrib/prometheus-exporter/service-prometheus.c
> +++ b/contrib/prometheus-exporter/service-prometheus.c
> @@ -880,6 +880,8 @@ static int promex_dump_srv_metrics(struct appctx *appctx,
> struct htx *htx)
> goto next_sv;
> for (i = 0; i <
> HCHK_STATUS_SIZE; i++) {
> + if
> (get_check_status_result(i) < CHK_RES_FAILED)
> + continue;
> val =
> mkf_u32(FO_STATUS, sv->check.status == i);
> check_state =
> get_check_status_info(i);
> labels[2].name =
> ist("state");
>
> No comments about the other patches. Except the README is now outdated and
> does not reflect recent changes. It could be good to keep it up-to-date as far
> as possible.
ah true, I will send an update about it soon.
--
William
[PATCH 1/2] BUG/MINOR: cli: fix set server addr/port coherency with health checks
while reading `update_server_addr_port` I found out some things which
can be seen as incoherency. I hope I did not overlooked anything:
- one comment is stating check's address should be updated if it uses
the server one; however the condition checks if `SRV_F_CHECKADDR` is
set; this flag is set when a check address is set; result is that we
override the check address where I was not expecting it. In fact we
don't need to update anything here as server addr is used when check
addr is not set.
- same goes for check agent addr
- for port, it is a bit different, we update the check port if it is
unset. This is harmless because we also use server port if check port
is unset. However it creates some incoherency before/after using this
command, as check port should stay unset througout the life of the
process unless it is is set by `set server check-port` command.
quite hard to locate the origin of this this issue but the function was
introduced in commit d458adcc52b74608e2fe6a2a95f09ce5e94932b7 ("MINOR:
new update_server_addr_port() function to change both server's ADDR and
service PORT"). I was however not able to determine whether this is due
to a change of behavior along the years. So this patch can potentially
be backported up to v1.8.
Signed-off-by: William Dauchy
---
src/server.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/src/server.c b/src/server.c
index 10f528640..99b7e9181 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3625,16 +3625,6 @@ const char *update_server_addr_port(struct server *s,
const char *addr, const ch
ipcpy(&sa, &s->addr);
changed = 1;
- /* we also need to update check's ADDR only if it uses the
server's one */
- if ((s->check.state & CHK_ST_CONFIGURED) && (s->flags &
SRV_F_CHECKADDR)) {
- ipcpy(&sa, &s->check.addr);
- }
-
- /* we also need to update agent ADDR only if it use the
server's one */
- if ((s->agent.state & CHK_ST_CONFIGURED) && (s->flags &
SRV_F_AGENTADDR)) {
- ipcpy(&sa, &s->agent.addr);
- }
-
/* update report for caller */
chunk_printf(msg, "IP changed from '%s' to '%s'", current_addr,
addr);
}
@@ -3714,11 +3704,6 @@ const char *update_server_addr_port(struct server *s,
const char *addr, const ch
}
chunk_appendf(msg, "%d'", new_port);
-
- /* we also need to update health checks port only if it
uses server's realport */
- if ((s->check.state & CHK_ST_CONFIGURED) && !(s->flags
& SRV_F_CHECKPORT)) {
- s->check.port = new_port;
- }
}
else {
chunk_appendf(msg, "no need to change the port");
--
2.29.2
Re: [PATCH 0/9] prometheus: health check as labels + cleanup
Le 30/01/2021 à 16:21, William Dauchy a écrit :
Hi Christopher,
Sorry for this long series but I believe this one is fairly easy to
review:
- the major change is for health checks on prometheus side, there are
more details in the commit message; this is something I wanted early
so people can start testing it, especially on large setup. I however
think we will need to improve the `scope` filtering. The good point is
that after this patch, this metric will be way easier to use; the bad
point is that it is a breaking change between v2.3 and v2.4. But I
believe this is acceptable following the previous state changes
already merged.
- next patch is the continuation of the cleaning work:
* try to improve descriptions so we can use them in both case
* merge them when possible, override otherwise
- I finished with a bit of minor cleaning which pointed me a few missing
fields. I know we probably can improve that to avoid forgetting adding
those fields in the future, but I assume it is ok for now to simply
come back with a sane result. The postive point is I did not had to
add description and implementation, as we make use of stats.c
Hi Willian,
Don't be sorry to send patches ! If you think the number of labels for the
check_status metric is not a problem, even for huge configurations, I trust you.
And I guess we can reduce this number to 16 status only, removing all status
with an unknown check result (HCHK_STATUS_UNKNOWN, HCHK_STATUS_INI,
HCHK_STATUS_START, HCHK_STATUS_L57DATA) and the status regarding the agent-check
only (HCHK_STATUS_CHECKED).
For info, HCHK_STATUS_UNKNOWN is only used for uninitialized health-check
(during configuration parsing). HCHK_STATUS_INI is used for initialized
health-check and before the first run. HCHK_STATUS_START and HCHK_STATUS_L57DATA
are dummy status and never assigned to a health-check. And as said,
HCHK_STATUS_CHECKED is only used for agent-check.
It is a slight improvement, but still better than nothing. To do so, I propose
you to add a function in check.c to get the corresponding check result of a
status (the attached patch). This way, in your first patch, we can filter on
check result. I may amend your first patch this way if you're ok:
--- a/contrib/prometheus-exporter/service-prometheus.c
+++ b/contrib/prometheus-exporter/service-prometheus.c
@@ -880,6 +880,8 @@ static int promex_dump_srv_metrics(struct appctx *appctx,
struct htx *htx)
goto next_sv;
for (i = 0; i < HCHK_STATUS_SIZE; i++) {
+ if
(get_check_status_result(i) < CHK_RES_FAILED)
+ continue;
val = mkf_u32(FO_STATUS,
sv->check.status == i);
check_state =
get_check_status_info(i);
labels[2].name =
ist("state");
No comments about the other patches. Except the README is now outdated and
does not reflect recent changes. It could be good to keep it up-to-date as far
as possible.
--
Christopher Faulet
>From 9eb33b4ed7bd2017422dff1c4076c105a23a1e2d Mon Sep 17 00:00:00 2001
From: Christopher Faulet
Date: Mon, 1 Feb 2021 11:00:49 +0100
Subject: [PATCH] MINOR: checks: Add function to get the result code
corresponding to a status
The function get_check_status_result() can now be used to get the result
code (CHK_RES_*) corresponding to a check status (HCHK_STATUS_*). It will be
used by the Prometheus exporter when reporting the check status of a server.
---
include/haproxy/check.h | 1 +
src/check.c | 9 +
2 files changed, 10 insertions(+)
diff --git a/include/haproxy/check.h b/include/haproxy/check.h
index 2e697ee78..8d70040a3 100644
--- a/include/haproxy/check.h
+++ b/include/haproxy/check.h
@@ -29,6 +29,7 @@
extern struct data_cb check_conn_cb;
extern struct proxy checks_fe;
+short get_check_status_result(short check_status);
const char *get_check_status_description(short check_status);
const char *get_check_status_info(short check_status);
int httpchk_build_status_header(struct server *s, struct buffer *buf);
diff --git a/src/check.c b/src/check.c
index 0528372d2..879fe84ce 100644
--- a/src/check.c
+++ b/src/check.c
@@ -152,6 +152,15 @@ static inline int unclean_errno(int err)
return err;
}
+/* Converts check_status code to result code */
+short get_check_status_result(short check_status)
+{
+ if (check_status < HCHK_STATUS_SIZE)
+ return check_statuses[check_status].result;
+ else
+ return check_statuses[HCHK_STATUS_UNKNOWN].result;
+}
+
/* Converts check_status code to description */
const char *get_check_status_description(short check_status) {
--
2.29.2
[PATCH 2/2] BUG/MINOR: cli: set server check-port missing flag
when we set check port flag, we need to set a flag on server.
this is missing since the origin of the feature: commit
5094656a67fa1b56f30cd2316adb675ca9d2a79a ("MINOR: cli: change a server
health check port through the stats socket"). So it can potentially be
backport up to v1.8.
Signed-off-by: William Dauchy
---
src/server.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/server.c b/src/server.c
index 99b7e9181..ded3d47ff 100644
--- a/src/server.c
+++ b/src/server.c
@@ -4405,6 +4405,7 @@ static int cli_parse_set_server(char **args, char
*payload, struct appctx *appct
goto out_unlock;
}
sv->check.port = i;
+ sv->flags |= SRV_F_CHECKPORT;
cli_msg(appctx, LOG_NOTICE, "health check port updated.\n");
}
else if (strcmp(args[3], "addr") == 0) {
--
2.29.2
