Re: [PATCH] runtime do-resolve http action
On Fri, Jan 25, 2019 at 3:28 PM Willy Tarreau wrote:
> On Fri, Jan 25, 2019 at 03:09:52PM +0100, Baptiste wrote:
> > Hi Willy,
> >
> > Thanks for the review!!!
> > I fixed most of the problems, but I have a 3 points I'd like to discuss:
> >
> > > + If an IP address can be found, it is stored into . If any kind
> of
> > > > + error occurs, then is not set.
> > >
> > > Just to be sure, it is not set or not modified ? I guess the latter,
> which
> > > is fine.
> > >
> >
> > Yes, not set. So '-m found' can be used.
>
> So you actually *remove* the variable if you don't get a response,
> that's it ? I would have possibly found it more convenient to just
> stay on the not modified approach so that you could possibly chain
> multiple do-resolve actions and hope that at least one of them could
> pick the response. Think about environments where you have multiple
> sets of resolvers (internal, admin, internet) and for unkonwn names
> you don't know which onee to ask so you ask all of them with 3
> different rules.
>
The code let the variable untouched. I just call vars_set_by_name() if an
IP is returned.
http-request do-resolve(txn.myip,internal_dns,ipv4) hdr(Host),lower
http-request do-resolve(txn.myip,external_dns,ipv4) hdr(Host),lower
unless { var(txn.myip) -m found }
should work.
> > > + 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.
> > >
> >
> > In this function, I have a proxy structure. Should I use send_log() on it
> > to report the error?
>
> You could but then it'd be better to perform some form of rate-limiting.
> It is possible that the same reason causes the function to fail in loops
> for all requests and it's not very cool to spam logs with info that are
> already present in the request's failure anyway. In general an alert log
> is made so that someone can do something about it. What could be done
> however is to emit this error once if it's a matter of config, and to
> increment a counter reported in "show info". We already do this at some
> places, I just don't remember which ones :-)
>
Ok, I set up a global counter to track those errors. I call my
field INF_DORESOLVE_ERRORS and the
global varialble is called dns_doresolve_errors.
A show info shows the following line:
DoResolveErrors: 0
Let me know if that is ok for you this way.
Also, I am planing to allow this action at the "tcp-request content" layer,
to be able to execute it using SNI information.
Baptiste
Re: [PATCH] runtime do-resolve http action
On Fri, Jan 25, 2019 at 03:09:52PM +0100, Baptiste wrote:
> Hi Willy,
>
> Thanks for the review!!!
> I fixed most of the problems, but I have a 3 points I'd like to discuss:
>
> > + If an IP address can be found, it is stored into . If any kind of
> > > + error occurs, then is not set.
> >
> > Just to be sure, it is not set or not modified ? I guess the latter, which
> > is fine.
> >
>
> Yes, not set. So '-m found' can be used.
So you actually *remove* the variable if you don't get a response,
that's it ? I would have possibly found it more convenient to just
stay on the not modified approach so that you could possibly chain
multiple do-resolve actions and hope that at least one of them could
pick the response. Think about environments where you have multiple
sets of resolvers (internal, admin, internet) and for unkonwn names
you don't know which onee to ask so you ask all of them with 3
different rules.
> > > + 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.
> >
>
> In this function, I have a proxy structure. Should I use send_log() on it
> to report the error?
You could but then it'd be better to perform some form of rate-limiting.
It is possible that the same reason causes the function to fail in loops
for all requests and it's not very cool to spam logs with info that are
already present in the request's failure anyway. In general an alert log
is made so that someone can do something about it. What could be done
however is to emit this error once if it's a matter of config, and to
increment a counter reported in "show info". We already do this at some
places, I just don't remember which ones :-)
> > > + 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 ?
> >
>
> I think this must have been one of the first thing I did during my
> development phase so I would be able to "isolate" my code when needed.
> Now you said it, and I step back a bit, I also consider there is no value
> in this action, appart being clear on the action name and gives us the
> ability to be very cautious if we update the behavior of ACT_CUSTOM in the
> future.
> I can remove ACT_HTTP_DO_RESOLVE and add a comment in ACT_CUSTOM saying
> that the do-resolve action relies on this code, just in case.
Normally the vast majority of actions are already in ACT_CUSTOM nowadays.
The other ones are just historical exceptions. Please have a look at
http_req_actions to see how to declare yours. In short you'll have to
add something like this to dns.c (please excuse the copy-paste which
will not work, but you'll get the idea) :
static struct action_kw_list http_req_dns_actions = {
.kw = {
{ "do-resolve", parse_http_req_do_resolve },
{ NULL, NULL }
}
};
INITCALL1(STG_REGISTER, http_req_keywords_register, _req_dns_actions);
And you're done, more or less a few includes of course :-)
Cheers,
Willy
Re: [PATCH] runtime do-resolve http action
Hi Willy,
Thanks for the review!!!
I fixed most of the problems, but I have a 3 points I'd like to discuss:
> + If an IP address can be found, it is stored into . If any kind of
> > + error occurs, then is not set.
>
> Just to be sure, it is not set or not modified ? I guess the latter, which
> is fine.
>
Yes, not set. So '-m found' can be used.
> > + 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.
>
In this function, I have a proxy structure. Should I use send_log() on it
to report the error?
> > + 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 ?
>
I think this must have been one of the first thing I did during my
development phase so I would be able to "isolate" my code when needed.
Now you said it, and I step back a bit, I also consider there is no value
in this action, appart being clear on the action name and gives us the
ability to be very cautious if we update the behavior of ACT_CUSTOM in the
future.
I can remove ACT_HTTP_DO_RESOLVE and add a comment in ACT_CUSTOM saying
that the do-resolve action relies on this code, just in case.
Baptiste
Re: [PATCH] runtime do-resolve http action
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
> 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 [options...] [ { if | unless } ]
>Access control for Layer 7 requests
>
> @@ -4219,6 +4218,59 @@ http-request deny [deny_status ] [ { if |
> unless } ]
>those that can be overridden by the "errorfile" directive.
>No further "http-request" rules are evaluated.
>
> +http-request do-resolve(,,[ipv4,ipv6]) :
> + This action performs a DNS resolution of the output of and stores
> + the result in the variable . It uses the DNS resolvers section
> + pointed by .
> + 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 . If any kind of
> + error occurs, then 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
[PATCH] runtime do-resolve http action
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.
Baptiste
From c3baea8c50a7dcbe4557c4a578fcbd252ffb7c56 Mon Sep 17 00:00:00 2001
From: Baptiste Assmann
Date: Tue, 30 Jan 2018 08:10:20 +0100
Subject: [PATCH 3/4] MINOR: obj_type: new object type for struct stream
This patch creates a new obj_type for the struct stream in HAProxy.
---
include/proto/obj_type.h | 13 +
include/types/obj_type.h | 1 +
include/types/stream.h | 1 +
3 files changed, 15 insertions(+)
diff --git a/include/proto/obj_type.h b/include/proto/obj_type.h
index 47273ca..19865bb 100644
--- a/include/proto/obj_type.h
+++ b/include/proto/obj_type.h
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include
static inline enum obj_type obj_type(enum obj_type *t)
@@ -158,6 +159,18 @@ static inline struct dns_srvrq *objt_dns_srvrq(enum obj_type *t)
return __objt_dns_srvrq(t);
}
+static inline struct stream *__objt_stream(enum obj_type *t)
+{
+ return container_of(t, struct stream, obj_type);
+}
+
+static inline struct stream *objt_stream(enum obj_type *t)
+{
+ if (!t || *t != OBJ_TYPE_STREAM)
+ return NULL;
+ return __objt_stream(t);
+}
+
static inline void *obj_base_ptr(enum obj_type *t)
{
switch (obj_type(t)) {
diff --git a/include/types/obj_type.h b/include/types/obj_type.h
index e141d69..9410718 100644
--- a/include/types/obj_type.h
+++ b/include/types/obj_type.h
@@ -41,6 +41,7 @@ enum obj_type {
OBJ_TYPE_CONN, /* object is a struct connection */
OBJ_TYPE_SRVRQ,/* object is a struct dns_srvrq */
OBJ_TYPE_CS, /* object is a struct conn_stream */
+ OBJ_TYPE_STREAM, /* object is a struct stream */
OBJ_TYPE_ENTRIES /* last one : number of entries */
} __attribute__((packed)) ;
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 */
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 */
--
2.7.4
From 7f4b2ae2e0a98efd2fa162e906c4bb641732ae98 Mon Sep 17 00:00:00 2001
From: Baptiste Assmann
Date: Tue, 30 Jan 2018 08:08:04 +0100
Subject: [PATCH 2/4] MINOR: dns: move callback affection in
dns_link_resolution()
In dns.c, dns_link_resolution(), each type of dns requester is managed
separately, that said, the callback function is affected globaly (and
points to server type callbacks only).
This design prevents the addition of new dns requester type and this
patch aims at fixing this limitation: now, the callback setting is done
directly into the portion of code dedicated to each requester type.
---
src/dns.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/dns.c b/src/dns.c
index f39f3ff..8ac5024 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -1397,6 +1397,9 @@ int dns_link_resolution(void *requester, int requester_type, int requester_locke
req = srv->dns_requester;
if (!requester_locked)
HA_SPIN_UNLOCK(SERVER_LOCK, >lock);
+
+ req->requester_cb = snr_resolution_cb;
+ req->requester_error_cb = snr_resolution_error_cb;
}
else if (srvrq) {
if (srvrq->dns_requester == NULL) {
@@ -1407,13 +1410,14 @@ int dns_link_resolution(void *requester, int requester_type, int requester_locke
}
else
req = srvrq->dns_requester;
+
+ req->requester_cb = snr_resolution_cb;
+ req->requester_error_cb = snr_resolution_error_cb;
}
else
goto err;
req->resolution = res;
- req->requester_cb = snr_resolution_cb;
- req->requester_error_cb = snr_resolution_error_cb;
LIST_ADDQ(>requesters, >list);
return 0;
--
2.7.4
From 077ea8af588e0f0ac2ac4070d514e27c6dac57c9 Mon Sep 17 00:00:00 2001
From: Baptiste Assmann
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.
---
