Hi Robin,
On Thu, Dec 15, 2016 at 10:49:21PM -0800, Robin H. Johnson wrote:
> Add a 'stats disable' option that can be used to explicitly disable the
> stats, without issuing the warning message as seen on TCP proxies.
>
> If any stats options are present in a default block, there is presently
> no way to explicitly disable them for a single proxy, other than
> defining a new default block with all of the options repeated EXCEPT the
> stats options.
>
> This normally generates a warning:
> [WARNING] ... 'stats' statement ignored for proxy 'my-proxy' as it
> requires HTTP mode.
I think we could improve this situation by checking if the stats statement
was inherited from the defaults section and automatically disable it
instead of emitting a warning.
> After the warning is issued, the stats for that proxy are disabled
> anyway.
>
> The new 'stats disable' option just disables the stats without
> generating the warning message; it uses the exact same means to disable
> the stats as used by the warning path.
I think it can be useful to disable stats on certain proxies working in
HTTP mode in fact. The way stats work is a mess, it's enough to declare
any stats directive to automatically enable them. If you want to put the
stats auth or uri in the defaults section, you may still want to disable
that for certain proxies and "stats disable" can be useful for this.
I'm just slightly concerned about a risk of memory leak if people alternate
"stats enable" and "stats disable" and while we don't have a generic
uri_auth_free() function we should probably at least add a FIXME comment
in the code regarding this. Or we can implement the free function this
way, I think it will work :
void uri_auth_free(struct uri_auth *ua)
{
free(ua->uri_prefix);
free(ua->auth_realm);
free(ua->node);
free(ua->desc);
userlist_free(ua->userlist);
free_http_req_rules(&ua->http_req_rules);
free(ua);
}
Then in your patch you may try to do this :
+ } else if (!strcmp(args[1], "disable")) {
+ if (curproxy == &defproxy || curproxy->uri_auth !=
defproxy.uri_auth)
+ uri_auth_free(curproxy->uri_auth);
+ curproxy->uri_auth = NULL;
Regarding the current warning, I'd want to automatically remove it but I'm
seeing we still have the issue that in check_config_validity() we've lost
the information regarding the fact that we inherited this from a defaults
section (and I think some checks are wrong there by the way), and we still
don't have some post-section checks.
I'll look into adding certain such checks and possibly backport them to
improve the situation.
> This patch should be back-ported to 1.7.
I'm fine with backporting your fix, but please check if the changes above
work fine for you so that we at least make it more reliable. If that doesn't
work, just add a fixme comment instead of the call to uri_auth_free() so
that we can re-work on it later.
Thanks,
Willy
