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 <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.
> >
>
> 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, &http_req_dns_actions);
And you're done, more or less a few includes of course :-)
Cheers,
Willy
