Hi Baptiste,
On Wed, Jan 23, 2019 at 02:00:58PM +0100, Baptiste wrote:
> Hi Willy,
>
> Please find attached to this email a set of 4 patches which add a new HTTP
> action that can use a dns resolver section to perform a DNS resolution
> based on the output of a fetch.
> The use case is split DNS situations or with highly dynamic environment
> where servers behind HAProxy are just ephemeral services.
Ah thanks for having rebased them.
I have some comments below, some purely cosmetic, some less :
> diff --git a/include/types/stream.h b/include/types/stream.h
> index 5e854c5..02eacd9 100644
> --- a/include/types/stream.h
> +++ b/include/types/stream.h
> @@ -119,6 +119,7 @@ struct strm_logs {
> };
>
> struct stream {
> + enum obj_type obj_type; /* object type == OBJ_TYPE_STREAM */
Here this drills a 7-bytes hole between obj_type and flags. It would be
better to move this field elsewhere in the struct where there's a hole
already.
> int flags; /* some flags describing the stream */
> unsigned int uniq_id; /* unique ID used for the traces */
> enum obj_type *target; /* target to use for this stream */
> --
> From 077ea8af588e0f0ac2ac4070d514e27c6dac57c9 Mon Sep 17 00:00:00 2001
> From: Baptiste Assmann <bed...@gmail.com>
> Date: Mon, 21 Jan 2019 08:34:50 +0100
> Subject: [PATCH 4/4] MINOR: action: new 'http-request do-resolve' action
>
> The 'do-resolve' action is an http-request action which allows to run
> DNS resolution at run time in HAProxy.
> The name to be resolved can be picked up in the request sent by the
> client and the result of the resolution is stored in a variable.
> The time the resolution is being performed, the request is on pause.
> If the resolution can't provide a suitable result, then the variable
> will be empty. It's up to the admin to take decisions based on this
> statement (return 503 to prevent loops).
>
> Read carefully the documentation concerning this feature, to ensure your
> setup is secure and safe to be used in production.
> ---
> doc/configuration.txt | 54 +++++++++-
> include/proto/action.h | 3 +
> include/proto/dns.h | 2 +
> include/types/action.h | 8 ++
> include/types/stream.h | 10 ++
> src/action.c | 34 +++++++
> src/cfgparse.c | 18 ++++
> src/dns.c | 266
> +++++++++++++++++++++++++++++++++++++++++++++++++
> src/proto_http.c | 9 +-
> src/stream.c | 11 ++
> 10 files changed, 407 insertions(+), 8 deletions(-)
>
> diff --git a/doc/configuration.txt b/doc/configuration.txt
> index 2a7efe9..0155274 100644
> --- a/doc/configuration.txt
> +++ b/doc/configuration.txt
> @@ -4064,7 +4064,6 @@ http-check send-state
>
> See also : "option httpchk", "http-check disable-on-404"
>
> -
> http-request <action> [options...] [ { if | unless } <condition> ]
> Access control for Layer 7 requests
>
> @@ -4219,6 +4218,59 @@ http-request deny [deny_status <status>] [ { if |
> unless } <condition> ]
> those that can be overridden by the "errorfile" directive.
> No further "http-request" rules are evaluated.
>
> +http-request do-resolve(<var>,<resolvers>,[ipv4,ipv6]) <expr> :
> + This action performs a DNS resolution of the output of <expr> and stores
> + the result in the variable <var>. It uses the DNS resolvers section
> + pointed by <resolvers>.
> + It is possible to choose a resolution preference using the optional
> + arguments 'ipv4' or 'ipv6'.
> + When performing the DNS resolution, the client side connection is on
> + pause waiting till the end of the resolution.
> + If an IP address can be found, it is stored into <var>. If any kind of
> + error occurs, then <var> is not set.
Just to be sure, it is not set or not modified ? I guess the latter, which
is fine.
> diff --git a/include/types/stream.h b/include/types/stream.h
> index 02eacd9..26a5e4a 100644
> --- a/include/types/stream.h
> +++ b/include/types/stream.h
> @@ -179,6 +179,16 @@ struct stream {
> struct list *current_rule_list; /* this is used to store the
> current executed rule list. */
> void *current_rule; /* this is used to store the
> current rule to be resumed. */
> struct hlua *hlua; /* lua runtime context */
> +
> + /* Context */
> + union {
> + struct {
> + struct dns_requester *dns_requester;
> + char *hostname_dn;
> + int hostname_dn_len;
> + struct act_rule *parent;
> + } dns;
> + } ctx;
History has told us that every single time we created a union with a single
field inside hoping to reuse it later, we never reused it. Thus better
directly put the structure and call it "dns_ctx". It will also be clearer
because "ctx" or "context" are unclear here given that a stream *is* a
context already, so you have a generic context in a context. Also, given
that you have a 4-bytes hole after hostname_dn_len, maybe it could make
sense to place your obj_type there since it's only used by the dns. Well,
on the other hand it having objt_stream() look up the obj_type in the
struct dns could look strange as well. At least please mention after the
len that there's a 4-byte hole there.
> +int act_resolution_cb(struct dns_requester *requester, struct dns_nameserver
> *nameserver)
> +{
> + struct stream *stream = NULL;
you don't need to preset it to NULL since its first use is an assignment.
> + if (requester->resolution == NULL)
> + return 0;
> +
> + stream = objt_stream(requester->owner);
> + if (stream == NULL)
> + return 0;
> +
> + task_wakeup(stream->task, TASK_WOKEN_MSG);
> +
> + return 0;
> +}
> +
> +int act_resolution_error_cb(struct dns_requester *requester, int error_code)
> +{
> + struct stream *stream = NULL;
same here.
> + if (requester->resolution == NULL)
> + return 0;
> +
> + stream = objt_stream(requester->owner);
> + if (stream == NULL)
> + return 0;
> +
> + task_wakeup(stream->task, TASK_WOKEN_MSG);
> +
> + return 0;
> +}
> +
> diff --git a/src/cfgparse.c b/src/cfgparse.c
> index a12c4ee..8f9200f 100644
> --- a/src/cfgparse.c
> +++ b/src/cfgparse.c
> @@ -2729,6 +2729,24 @@ int check_config_validity()
> free(err);
> cfgerr++;
> }
> + /* link do-resolve HTTP action to its resolvers section
> */
> + if (arule->action == ACT_HTTP_DO_RESOLVE) {
> + struct dns_resolvers *resolvers = NULL;
> +
> + if (arule->arg.dns.resolvers_id == NULL) {
> + ha_alert("Proxy '%s': %s.\n",
> curproxy->id, "do-resolve action without resolvers");
> + cfgerr++;
> + err_code |= ERR_FATAL;
> + }
> + else if ((resolvers =
> find_resolvers_by_id(arule->arg.dns.resolvers_id)) == NULL) {
> + ha_alert("Proxy '%s': %s.\n",
> curproxy->id, "resolvers of do-resolve action is unknown");
> + cfgerr++;
> + err_code |= ERR_FATAL;
> + }
> +
> + arule->arg.dns.resolvers = resolvers;
> + }
> +
This check is supposed to be performed in arule->check_ptr(), so you need
to create a function which does these checks, and it will be called
automatically around line 2720. Please greop for "check_ptr", you'll see
a few examples like check_trk_action() or check_http_req_capture().
> +/*
> + * Execute the "do-resolution" action. May be called from {tcp,http}request.
> + */
> +enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy
> *px,
> + struct session *sess, struct
> stream *s, int flags)
> +{
(...)
> + /* need to configure and start a new DNS resolution */
> + if ((cli_conn = objt_conn(sess->origin)) && conn_ctrl_ready(cli_conn)) {
Please avoid allocations in "if" statements like this. You don't save
anything and generally complicate future debugging. E.g. if for a test
you need to comment out all this block, you suddenly create a new problem
without noticing. Just assign cli_conn before the condition then test it.
> + struct sample *smp;
> +
> + conn_get_from_addr(cli_conn);
> +
> + smp = sample_fetch_as_type(px, sess, s,
> SMP_OPT_DIR_REQ|SMP_OPT_FINAL, rule->arg.dns.expr, SMP_T_STR);
> + if (smp) {
> + char *fqdn;
> +
> + fqdn = smp->data.u.str.area;
> + if (action_prepare_for_resolution(s, fqdn) == -1) {
> + ha_alert("Can't create DNS resolution for
> server 'http request action'\n");
Please don't send runtime alerts. We've tried hard to clean them up so
that they remain only during startup.
> + return ACT_RET_ERR;
> + }
> +
> + s->ctx.dns.parent = rule;
> + dns_link_resolution(s, OBJ_TYPE_STREAM, 0);
> + dns_trigger_resolution(s->ctx.dns.dns_requester);
> + }
> + }
> + return ACT_RET_YIELD;
> +}
Unless I'm mistaken, if smp is NULL (no sample) you still return YIELD so
the rule will wait forever. I suspect that instead you need to do this :
smp = sample_fetch_as_type(...);
if (!smp)
return ACT_RET_CONT;
fqdn = ...
(...)
dns_trigger_resolution();
return ACT_RET_YIELD;
> +/* parse "do-resolve" action
> + * This action takes the following arguments:
> + * do-resolve(<varName>,<resolversSectionName>,<resolvePrefer>) <expr>
> + *
> + * - <varName> is the variable name where the result of the DNS resolution
> will be stored
> + * (mandatory)
> + * - <resolversSectionName> is the name of the resolvers section to use to
> perform the resolution
> + * (mandatory)
> + * - <resolvePrefer> can be either 'ipv4' or 'ipv6' and is the IP family
> we would like to resolve first
> + * (optional), defaults to ipv6
> + * - <expr> is an HAProxy expression used to fetch the name to be resolved
> + */
> +enum act_parse_ret dns_parse_do_resolve(const char **args, int *orig_arg,
> struct proxy *px, struct act_rule *rule, char **err)
> +{
> + int cur_arg;
> + struct sample_expr *expr;
> + unsigned int where;
> + const char *beg, *end;
> +
> + /* orig_arg points to the first argument, but we need to analyse the
> command itself first */
> + cur_arg = *orig_arg - 1;
> +
> + /* locate varName, which is mandatory */
> + beg = strchr(args[cur_arg], '(');
> + if (beg == NULL)
> + goto do_resolve_parse_error;
> + beg = beg + 1; /* beg should points to the first character after
> opening parenthesis '(' */
> + end = strchr(beg, ',');
> + if (end == NULL)
> + goto do_resolve_parse_error;
> + rule->arg.dns.varname = my_strndup(beg, end - beg);
You also need to check rule->arg.dns.varname here.
> + /* locate resolversSectionName, which is mandatory.
> + * Since next parameters are optional, the delimiter may be comma ','
> + * or closing parenthesis ')'
> + */
> + beg = end + 1;
> + if (beg == '\0')
This one is wrong, it will never match. You're testing if beg is NULL, which
will never be since end is valid and will not be equal to -1. I suspect that
what you wanted instead was if (!*beg) to check that you're not at the end
of the string, but then look :
> + goto do_resolve_parse_error;
> + end = strchr(beg, ',');
> + if (end == NULL)
> + end = strchr(beg, ')');
> + if (end == NULL)
> + goto do_resolve_parse_error;
This case is already handled here. So I think you can simply remove the
"if (beg == 0)" test.
> + rule->arg.dns.resolvers_id = my_strndup(beg, end - beg);
> +
> +
> + if (!rule->arg.dns.varname || !rule->arg.dns.resolvers_id)
> + goto do_resolve_parse_error;
Ah the test for varname is here, better move it where it's assigned,
it's easier to follow and maintain the code.
> + /* Default priority is ipv6 */
> + rule->arg.dns.dns_opts.family_prio = AF_INET6;
> +
> + /* optional arguments accepted for now:
> + * ipv4 or ipv6
> + */
> + while (*end != ')') {
> + beg = end + 1;
> + if (beg == '\0')
> + goto do_resolve_parse_error;
Same issue here.
> + end = strchr(beg, ',');
> + if (end == NULL)
> + end = strchr(beg, ')');
> + if (end == NULL)
> + goto do_resolve_parse_error;
> +
> + if (strncmp(beg, "ipv4", end - beg) == 0) {
> + rule->arg.dns.dns_opts.family_prio = AF_INET;
> + }
> + else if (strncmp(beg, "ipv6", end - beg) == 0) {
> + rule->arg.dns.dns_opts.family_prio = AF_INET6;
> + }
> + else {
> + goto do_resolve_parse_error;
> + }
> + }
> +
> + cur_arg = cur_arg + 1;
> +
> + expr = sample_parse_expr((char **)args, &cur_arg, px->conf.args.file,
> px->conf.args.line, err, &px->conf.args);
> + if (!expr)
> + goto do_resolve_parse_error;
> +
> +
> + where = 0;
> + if (px->cap & PR_CAP_FE)
> + where |= SMP_VAL_FE_HRQ_HDR;
> + if (px->cap & PR_CAP_BE)
> + where |= SMP_VAL_BE_HRQ_HDR;
> +
> + if (!(expr->fetch->val & where)) {
> + memprintf(err,
> + "fetch method '%s' extracts information from '%s',
> none of which is available here",
> + args[cur_arg-1], sample_src_names(expr->fetch->use));
> + free(expr);
> + return ACT_RET_PRS_ERR;
> + }
> + rule->arg.dns.expr = expr;
> + rule->action = ACT_HTTP_DO_RESOLVE;
> + rule->action_ptr = dns_action_do_resolve;
> + *orig_arg = cur_arg;
> +
> + return ACT_RET_PRS_OK;
> +
> + do_resolve_parse_error:
> + memprintf(err, "Can't parse '%s'. Expects
> 'do-resolve(<varname>,<resolvers>[,<options>]) <expr>'. Available options are
> 'ipv4' and 'ipv6'",
> + args[cur_arg]);
> + return ACT_RET_PRS_ERR;
Please note that ideally you should unroll everything you have allocated
here, either using unconditional free() on various stuff or by using
multiple labels. It's not critical but since we're trying to make the
code more dynamic, every single memory leak on error paths like this is
an obstacle so better do it while it's still fresh in your head.
> diff --git a/src/proto_http.c b/src/proto_http.c
> index 245fa98..e72a714 100644
> --- a/src/proto_http.c
> +++ b/src/proto_http.c
> @@ -54,6 +54,7 @@
> #include <proto/checks.h>
> #include <proto/cli.h>
> #include <proto/compression.h>
> +#include <proto/dns.h>
> #include <proto/stats.h>
> #include <proto/fd.h>
> #include <proto/filters.h>
> @@ -1766,6 +1767,7 @@ resume_execution:
> goto end;
> }
> break;
> + case ACT_HTTP_DO_RESOLVE:
> case ACT_CUSTOM:
> if ((s->req.flags & CF_READ_ERROR) ||
> ((s->req.flags & (CF_SHUTR|CF_READ_NULL)) &&
Suddenly that makes me wonder : why is it needed to have a dedicated
action since it uses the generic infrastructure with ACT_CUSTOM ?
> @@ -7369,13 +7371,6 @@ int http_replace_req_line(int action, const char
> *replace, int len,
> cur_ptr = ci_head(&s->req);
> cur_end = cur_ptr + txn->req.sl.rq.m_l;
>
> - /* adjust req line offsets and lengths */
> - delta = len - offset - (cur_end - cur_ptr);
> - txn->req.sl.rq.m_l += delta;
> - txn->req.sl.rq.u += delta;
> - txn->req.sl.rq.v += delta;
> - break;
> -
For me this is purely an accidental bug consisting in breaking some
http-request rules.
> diff --git a/src/stream.c b/src/stream.c
> index 2918589..b83ddef 100644
> --- a/src/stream.c
> +++ b/src/stream.c
> @@ -39,6 +39,7 @@
> #include <proto/checks.h>
> #include <proto/cli.h>
> #include <proto/connection.h>
> +#include <proto/dns.h>
> #include <proto/stats.h>
> #include <proto/fd.h>
> #include <proto/filters.h>
> @@ -85,6 +86,7 @@ int stream_create_from_cs(struct conn_stream *cs)
> if (strm == NULL)
> return -1;
>
> + strm->obj_type = OBJ_TYPE_STREAM;
Please move this one to stream_new() instead otherwise it will not always
be initialized.
> From 8d0634bfd1dafb185d3704710845d809b89ec382 Mon Sep 17 00:00:00 2001
> From: Baptiste Assmann <bed...@gmail.com>
> Date: Mon, 21 Jan 2019 08:18:09 +0100
> Subject: [PATCH 1/4] MINOR: dns: dns_requester structures are now in a memory
> pool
>
> dns_requester structure can be allocated at run time when servers get
> associated to DNS resolution (this happens when SRV records are used in
> conjunction with service discovery).
> Well, this memory allocation is safer if managed in an HAProxy pool,
> furthermore with upcoming HTTP action which can perform DNS resolution
> at runtime.
>
> This patch moves the memory management of the dns_requester structure
> into its own pool.
> ---
> include/types/dns.h | 2 ++
> src/dns.c | 5 +++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/types/dns.h b/include/types/dns.h
> index 3d022d4..81cd6d2 100644
> --- a/include/types/dns.h
> +++ b/include/types/dns.h
> @@ -34,6 +34,8 @@
> #include <types/server.h>
> #include <types/task.h>
>
> +extern struct pool_head *dns_requester_pool;
> +
> /*DNS maximum values */
> /*
> * Maximum issued from RFC:
> diff --git a/src/dns.c b/src/dns.c
> index 1d91e43..f39f3ff 100644
> --- a/src/dns.c
> +++ b/src/dns.c
> @@ -51,6 +51,7 @@ static THREAD_LOCAL int64_t dns_query_id_seed = 0; /*
> random seed */
>
> DECLARE_STATIC_POOL(dns_answer_item_pool, "dns_answer_item", sizeof(struct
> dns_answer_item));
> DECLARE_STATIC_POOL(dns_resolution_pool, "dns_resolution", sizeof(struct
> dns_resolution));
> +DECLARE_POOL(dns_requester_pool, "dns_requester", sizeof(struct
> dns_requester));
>
> static unsigned int resolution_uuid = 1;
>
> @@ -1384,7 +1385,7 @@ int dns_link_resolution(void *requester, int
> requester_type, int requester_locke
> if (!requester_locked)
> HA_SPIN_LOCK(SERVER_LOCK, &srv->lock);
> if (srv->dns_requester == NULL) {
> - if ((req = calloc(1, sizeof(*req))) == NULL) {
> + if ((req = pool_alloc(dns_requester_pool)) == NULL) {
> if (!requester_locked)
> HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
> goto err;
> @@ -1399,7 +1400,7 @@ int dns_link_resolution(void *requester, int
> requester_type, int requester_locke
> }
> else if (srvrq) {
> if (srvrq->dns_requester == NULL) {
> - if ((req = calloc(1, sizeof(*req))) == NULL)
> + if ((req = pool_alloc(dns_requester_pool)) == NULL)
> goto err;
> req->owner = &srvrq->obj_type;
> srvrq->dns_requester = req;
> --
> 2.7.4
There is a bug in this patch, it replaces calloc() with pool_alloc()
but it doesn't replace the calls to free(), so you're having two
different allocators for the same data structures once this patch is
applied. Please check if you didn't accidently slip the free() calls
into one of the other patches (I'm not very sure).
Thanks!
Willy
