From: Willy Tarreau Date: Mon, 18 Nov 2024 18:47:59 +0000 (+0100) Subject: MEDIUM: config: warn on unitless timeouts < 100 ms X-Git-Tag: v3.1-dev14~115 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9c6ccb8dbb242aec2844d9eff2d5e73c96756e73;p=thirdparty%2Fhaproxy.git MEDIUM: config: warn on unitless timeouts < 100 ms From time to time we face a configuration with very small timeouts which look accidental because there could be expectations that they're expressed in seconds and not milliseconds. This commit adds a check for non-nul unitless values smaller than 100 and emits a warning suggesting to append an explicit unit if that was the intent. Only the common timeouts, the server check intervals and the resolvers hold and timeout values were covered for now. All the code needs to be manually reviewed to verify if it supports emitting warnings. This may break some configs using "zero-warning", but greps in existing configs indicate that these are extremely rare and solely intentionally done during tests. At least even if a user leaves that after a test, it will be more obvious when reading 10ms that something's probably not correct. --- diff --git a/include/haproxy/tools.h b/include/haproxy/tools.h index 8e29d38cc2..aa9fd777c4 100644 --- a/include/haproxy/tools.h +++ b/include/haproxy/tools.h @@ -1256,6 +1256,23 @@ static inline void update_char_fingerprint(uint8_t *fp, char prev, char curr) fp[32 * from + to]++; } +/* checks that the numerical argument, if passed without units and is non-zero, + * is at least as large as value . It returns 1 if the value is too small, + * otherwise zero. This is used to warn about the use of small values without + * units. + */ +static inline int warn_if_lower(const char *text, long min) +{ + int digits; + long value; + + digits = strspn(text, "0123456789"); + if (digits < strlen(text)) + return 0; // there are non-digits here. + + value = atol(text); + return value && value < min; +} /* compare the current OpenSSL version to a string */ int openssl_compare_current_version(const char *version); diff --git a/src/check.c b/src/check.c index 85691de87e..29a86b7dd3 100644 --- a/src/check.c +++ b/src/check.c @@ -2190,6 +2190,12 @@ static int srv_parse_agent_inter(char **args, int *cur_arg, struct proxy *curpx, } srv->agent.inter = delay; + if (warn_if_lower(args[*cur_arg+1], 100)) { + memprintf(errmsg, "'%s %u' in server '%s' is suspiciously small for a value in milliseconds. Please use an explicit unit ('%ums') if that was the intent", + args[*cur_arg], delay, srv->id, delay); + err_code |= ERR_WARN; + } + out: return err_code; @@ -2459,6 +2465,12 @@ static int srv_parse_check_inter(char **args, int *cur_arg, struct proxy *curpx, } srv->check.inter = delay; + if (warn_if_lower(args[*cur_arg+1], 100)) { + memprintf(errmsg, "'%s %u' in server '%s' is suspiciously small for a value in milliseconds. Please use an explicit unit ('%ums') if that was the intent", + args[*cur_arg], delay, srv->id, delay); + err_code |= ERR_WARN; + } + out: return err_code; @@ -2504,6 +2516,12 @@ static int srv_parse_check_fastinter(char **args, int *cur_arg, struct proxy *cu } srv->check.fastinter = delay; + if (warn_if_lower(args[*cur_arg+1], 100)) { + memprintf(errmsg, "'%s %u' in server '%s' is suspiciously small for a value in milliseconds. Please use an explicit unit ('%ums') if that was the intent", + args[*cur_arg], delay, srv->id, delay); + err_code |= ERR_WARN; + } + out: return err_code; @@ -2549,6 +2567,12 @@ static int srv_parse_check_downinter(char **args, int *cur_arg, struct proxy *cu } srv->check.downinter = delay; + if (warn_if_lower(args[*cur_arg+1], 100)) { + memprintf(errmsg, "'%s %u' in server '%s' is suspiciously small for a value in milliseconds. Please use an explicit unit ('%ums') if that was the intent", + args[*cur_arg], delay, srv->id, delay); + err_code |= ERR_WARN; + } + out: return err_code; diff --git a/src/proxy.c b/src/proxy.c index 6f951cd506..357704209f 100644 --- a/src/proxy.c +++ b/src/proxy.c @@ -620,6 +620,12 @@ static int proxy_parse_timeout(char **args, int section, struct proxy *proxy, return -1; } + if (warn_if_lower(args[1], 100)) { + memprintf(err, "'timeout %s %u' in %s '%s' is suspiciously small for a value in milliseconds. Please use an explicit unit ('%ums') if that was the intent.", + name, timeout, proxy_type_str(proxy), proxy->id, timeout); + retval = 1; + } + if (!(proxy->cap & cap)) { memprintf(err, "'timeout %s' will be ignored because %s '%s' has no %s capability", name, proxy_type_str(proxy), proxy->id, diff --git a/src/resolvers.c b/src/resolvers.c index 8874689809..f8f0c8edfc 100644 --- a/src/resolvers.c +++ b/src/resolvers.c @@ -3733,6 +3733,12 @@ int cfg_parse_resolvers(const char *file, int linenum, char **args, int kwm) goto out; } + if (warn_if_lower(args[2], 100)) { + ha_alert("parsing [%s:%d] : '%s %s %u' looks suspiciously small for a value in milliseconds." + " Please use an explicit unit ('%ums') if that was the intent.\n", + file, linenum, args[0], args[1], time, time); + err_code |= ERR_WARN; + } } else if (strcmp(args[0], "accepted_payload_size") == 0) { int i = 0; @@ -3813,6 +3819,12 @@ int cfg_parse_resolvers(const char *file, int linenum, char **args, int kwm) curr_resolvers->px->timeout.connect = tout; } + if (warn_if_lower(args[2], 100)) { + ha_alert("parsing [%s:%d] : '%s %s %u' looks suspiciously small for a value in milliseconds." + " Please use an explicit unit ('%ums') if that was the intent.\n", + file, linenum, args[0], args[1], tout, tout); + err_code |= ERR_WARN; + } } else { ha_alert("parsing [%s:%d] : '%s' expects 'retry' or 'resolve' and