MEDIUM: cfgparse: acknowledge that proxy ID auto numbering starts at 2

Each frontend/backend/listen proxies is assigned an unique ID. It can
either be set explicitely via 'id' keyword, or automatically assigned on
post parsing depending on the available values.

It was expected that the first automatically assigned value would start
at '1'. However, due to a legacy bug this is not the case as this value
is always skipped. Thus, automatically assigned proxies always start at
'2' or more.

To avoid breaking the current existing state, this situation is now
acknowledged with the current patch. The code is rewritten with an
explicit warning to ensure that this won't be fixed without knowing the
current status. A new regtest also ensures this.

diff --git a/doc/configuration.txt b/doc/configuration.txt
index ef79c9be70fc3d739afdb7cba2ad69aa6a06d751..0d147633319d7a5be5d1b72414c07e528735b8ec 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -647,8 +647,8 @@ which must be placed before other sections, but it may be repeated if needed.
 In addition, some automatic identifiers may automatically be assigned to some
 of the created objects (e.g. proxies), and by reordering sections, their
 identifiers will change. These ones appear in the statistics for example. As
-such, the configuration below will assign "foo" ID number 1 and "bar" ID number
-2, which will be swapped if the two sections are reversed:
+such, the configuration below will assign "foo" an ID number smaller than its
+"bar" counterpart. This will be swapped if the two sections are reversed:
 
      listen foo
          bind :80
@@ -8692,9 +8692,11 @@ id <value>
 
   Arguments : none
 
-  Set a persistent ID for the proxy. This ID must be unique and positive.
-  An unused ID will automatically be assigned if unset. The first assigned
-  value will be 1. This ID is currently only returned in statistics.
+  Set a persistent ID for the proxy. This ID must be unique and positive. An
+  unused ID will automatically be assigned if unset. Due to an historical
+  behavior, value 1 is not used unless explicitely set. Thus, the lowest value
+  automatically assigned will be 2. This ID is currently only returned in
+  statistics.
 
 
 ignore-persist { if | unless } <condition>

diff --git a/reg-tests/proxy/proxy_id.vtc b/reg-tests/proxy/proxy_id.vtc
new file mode 100644 (file)
index 0000000..4a2b851
--- /dev/null
+++ b/reg-tests/proxy/proxy_id.vtc
@@ -0,0 +1,58 @@
+varnishtest "Ensure that proxies automatic numbering remains consistent across versions"
+
+feature ignore_unknown_macro
+
+# No ID explicitely set. First automatically assigned value must be set to '2'.
+# Value '1' is skipped due to an historical bug.
+haproxy h1 -conf {
+       defaults
+               timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+               timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+               timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+       listen fe1
+               bind "fd@${fe1}"
+
+       listen fe2
+               bind "fd@${fe2}"
+} -start
+
+haproxy h1 -cli {
+       send "show stat 1 -1 -1"
+       expect !~ "fe[12],"
+
+       send "show stat 2 -1 -1"
+       expect ~ "fe1,"
+
+       send "show stat 3 -1 -1"
+       expect ~ "fe2,"
+}
+
+# Explicitely uses ID 1 and 2. First automatically assigned value must be
+# set to '3'.
+haproxy h2 -conf {
+       defaults
+               timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+               timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+               timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+       listen fe1
+               bind "fd@${fe1}"
+       listen fe2
+               id 1  # 1 set as automatic value
+               bind "fd@${fe1}"
+       listen fe3
+               id 2
+               bind "fd@${fe3}"
+} -start
+
+haproxy h2 -cli {
+       send "show stat 1 -1 -1"
+       expect ~ "fe2,"
+
+       send "show stat 2 -1 -1"
+       expect ~ "fe3,"
+
+       send "show stat 3 -1 -1"
+       expect ~ "fe1,"
+}

diff --git a/src/cfgparse.c b/src/cfgparse.c
index 3065319ad1706838f28868441f89d5ceff577bbf..bcb53d00b44fd3913204e65b083baa389f167407 100644 (file)
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -2786,7 +2786,8 @@ int check_config_validity()
        struct server *newsrv = NULL;
        struct mt_list back;
        int err_code = 0;
-       unsigned int next_pxid = 1;
+       /* Value forced to skip '1' due to an historical bug, see below for more details. */
+       unsigned int next_pxid = 2;
        struct bind_conf *bind_conf;
        char *err;
        struct cfg_postparser *postparser;
@@ -2874,16 +2875,19 @@ init_proxies_list_stage1:
                unsigned int next_id;
 
                proxy_init_per_thr(curproxy);
+
+               /* Assign automatic UUID if unset except for internal proxies.
+                *
+                * WARNING proxy UUID initialization is buggy as value '1' is
+                * skipped if not explicitely used. This is an historical bug
+                * and should not be corrected to prevent breakage on future
+                * versions.
+                */
                if (!(curproxy->cap & PR_CAP_INT) && curproxy->uuid < 0) {
-                       /* proxy ID not set, use automatic numbering with first
-                        * spare entry starting with next_pxid. We don't assign
-                        * numbers for internal proxies as they may depend on
-                        * build or config options and we don't want them to
-                        * possibly reuse existing IDs.
-                        */
                        next_pxid = proxy_get_next_id(next_pxid);
                        curproxy->uuid = next_pxid;
                        proxy_index_id(curproxy);
+                       next_pxid++;
                }
 
                if (curproxy->mode == PR_MODE_HTTP && global.tune.bufsize >= (256 << 20) && ONLY_ONCE()) {
@@ -2892,17 +2896,6 @@ init_proxies_list_stage1:
                        cfgerr++;
                }
 
-               /* next IDs are shifted even if the proxy is disabled, this
-                * guarantees that a proxy that is temporarily disabled in the
-                * configuration doesn't cause a renumbering. Internal proxies
-                * that are not assigned a static ID must never shift the IDs
-                * either since they may appear in any order (Lua, logs, etc).
-                * The GLOBAL proxy that carries the stats socket has its ID
-                * forced to zero.
-                */
-               if (curproxy->uuid >= 0)
-                       next_pxid++;
-
                if (curproxy->flags & PR_FL_DISABLED) {
                        /* ensure we don't keep listeners uselessly bound. We
                         * can't disable their listeners yet (fdtab not