improve: use optimistic locking to avoid concurrency problem in admin…

… PATCH APIs.

There is a potential concurrency problem in all admin PATCH APIs when
two patch requests come in simultaneously, in such case, the patched
result of the first applied request will be overridden, also the
probability is tidy, but from the perspective of software's robust,
that's not what we wanna to see.

In this commit, we use the optimistic locking to avoid this problem, for
the example aforementioned, the second PATCH request will failure, and
it's up to the user to retry this PATCH request again.

The optimistic locking mechanism in ETCD v2 APIs is showed by arg
`prevIndex`.

Signed-off-by: tokers <zchao1995@gmail.com>

apisix/admin/global_rules.lua

@@ -135,6 +135,7 @@ function _M.patch(id, conf, sub_path)
                   core.json.delay_encode(res_old, true))
 
     local node_value = res_old.body.node.value
+    local modified_index = res_old.body.node.modifiedIndex
 
     if sub_path and sub_path ~= "" then
         local code, err, node_val = core.table.patch(node_value, sub_path, conf)
@@ -153,8 +154,7 @@ function _M.patch(id, conf, sub_path)
         return 400, err
     end
 
-    -- TODO: this is not safe, we need to use compare-set
-    local res, err = core.etcd.set(key, node_value)
+    local res, err = core.etcd.atomic_set(key, node_value, nil, modified_index)
     if not res then
         core.log.error("failed to set new global rule[", key, "]: ", err)
         return 500, {error_msg = err}

apisix/admin/routes.lua

@@ -241,6 +241,7 @@ function _M.patch(id, conf, sub_path, args)
                   core.json.delay_encode(res_old, true))
 
     local node_value = res_old.body.node.value
+    local modified_index = res_old.body.node.modifiedIndex
 
     if sub_path and sub_path ~= "" then
         local code, err, node_val = core.table.patch(node_value, sub_path, conf)
@@ -259,8 +260,7 @@ function _M.patch(id, conf, sub_path, args)
         return 400, err
     end
 
-    -- TODO: this is not safe, we need to use compare-set
-    local res, err = core.etcd.set(key, node_value, args.ttl)
+    local res, err = core.etcd.atomic_set(key, node_value, args.ttl, modified_index)
     if not res then
         core.log.error("failed to set new route[", key, "] to etcd: ", err)
         return 500, {error_msg = err}

apisix/admin/services.lua

@@ -219,6 +219,7 @@ function _M.patch(id, conf, sub_path)
                   core.json.delay_encode(res_old, true))
 
     local node_value = res_old.body.node.value
+    local modified_index = res_old.body.node.modifiedIndex
 
     if sub_path and sub_path ~= "" then
         local code, err, node_val = core.table.patch(node_value, sub_path, conf)
@@ -237,8 +238,7 @@ function _M.patch(id, conf, sub_path)
         return 400, err
     end
 
-    -- TODO: this is not safe, we need to use compare-set
-    local res, err = core.etcd.set(key, node_value)
+    local res, err = core.etcd.atomic_set(key, node_value, nil, modified_index)
     if not res then
         core.log.error("failed to set new service[", key, "]: ", err)
         return 500, {error_msg = err}

apisix/admin/ssl.lua

@@ -198,6 +198,7 @@ function _M.patch(id, conf)
 
 
     local node_value = res_old.body.node.value
+    local modified_index = res_old.body.node.modifiedIndex
 
     node_value = core.table.merge(node_value, conf);
 
@@ -208,8 +209,7 @@ function _M.patch(id, conf)
         return 400, err
     end
 
-    -- TODO: this is not safe, we need to use compare-set
-    local res, err = core.etcd.set(key, node_value)
+    local res, err = core.etcd.atomic_set(key, node_value, nil, modified_index)
     if not res then
         core.log.error("failed to set new ssl[", key, "] to etcd: ", err)
         return 500, {error_msg = err}

apisix/admin/upstreams.lua

@@ -253,6 +253,7 @@ function _M.patch(id, conf, sub_path)
                   core.json.delay_encode(res_old, true))
 
     local new_value = res_old.body.node.value
+    local modified_index = res_old.body.node.modifiedIndex
 
     if sub_path and sub_path ~= "" then
         local code, err, node_val = core.table.patch(new_value, sub_path, conf)
@@ -271,8 +272,7 @@ function _M.patch(id, conf, sub_path)
         return 400, err
     end
 
-    -- TODO: this is not safe, we need to use compare-set
-    local res, err = core.etcd.set(key, new_value)
+    local res, err = core.etcd.atomic_set(key, new_value, nil, modified_index)
     if not res then
         core.log.error("failed to set new upstream[", key, "]: ", err)
         return 500, {error_msg = err}

apisix/core/etcd.lua

@@ -178,6 +178,65 @@ end
 _M.set = set
 
 
+function _M.atomic_set(key, value, ttl, mod_revision)
+    local etcd_cli, prefix, err = new()
+    if not etcd_cli then
+        return nil, err
+    end
+
+    local lease_id
+    if ttl then
+        local data, grant_err = etcd_cli:grant(tonumber(ttl))
+        if not data then
+            return nil, grant_err
+        end
+
+        lease_id = data.body.ID
+    end
+
+    key = prefix .. key
+
+    local compare = {
+        {
+            key = key,
+            target = "MOD",
+            result = "EQUAL",
+            mod_revision = mod_revision,
+        }
+    }
+
+    local success = {
+        {
+            requestPut = {
+                key = key,
+                value = value,
+                lease = lease_id,
+            }
+        }
+    }
+
+    local res, err = etcd_cli:txn(compare, success)
+    if not res then
+        return nil, err
+    end
+
+    if not res.body.succeeded then
+        return nil, "value changed before overwritten"
+    end
+
+    res.headers["X-Etcd-Index"] = res.body.header.revision
+    -- etcd v3 set would not return kv info
+    res.body.action = "compareAndSwap"
+    res.body.node = {
+        key = key,
+        value = value,
+    }
+    res.status = 201
+
+    return res, nil
+end
+
+
 function _M.push(key, value, ttl)
     local etcd_cli, prefix, err = new()
     if not etcd_cli then

t/admin/global-rules.t

@@ -189,7 +189,7 @@ passed
                         },
                         "key": "/apisix/global_rules/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
                 )
 
@@ -235,7 +235,7 @@ passed
                         },
                         "key": "/apisix/global_rules/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
                 )
 

t/admin/routes.t

@@ -1005,7 +1005,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1056,7 +1056,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1090,7 +1090,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1124,7 +1124,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1158,7 +1158,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1190,7 +1190,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1242,7 +1242,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 

t/admin/services-string-id.t

@@ -657,7 +657,7 @@ GET /t
                         },
                         "key": "/apisix/services/5eeb3dc90f747328b2930b0b"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -697,7 +697,7 @@ passed
                         },
                         "key": "/apisix/services/5eeb3dc90f747328b2930b0b"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 

t/admin/services.t

@@ -657,7 +657,7 @@ GET /t
                         },
                         "key": "/apisix/services/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -697,7 +697,7 @@ passed
                         },
                         "key": "/apisix/services/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -788,7 +788,7 @@ passed
                         },
                         "key": "/apisix/services/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -826,7 +826,7 @@ passed
                         },
                         "key": "/apisix/services/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 

t/admin/upstream.t

@@ -672,7 +672,7 @@ GET /t
                         },
                         "key": "/apisix/upstreams/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -710,7 +710,7 @@ passed
                         },
                         "key": "/apisix/upstreams/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -833,7 +833,7 @@ passed
                         },
                         "key": "/apisix/upstreams/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -869,7 +869,7 @@ passed
                         },
                         "key": "/apisix/upstreams/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 

t/router/radixtree-sni.t

@@ -589,7 +589,7 @@ location /t {
                     },
                     "key": "/apisix/ssl/1"
                 },
-                "action": "set"
+                "action": "compareAndSwap"
             }]]
             )
 
@@ -670,7 +670,7 @@ location /t {
                     },
                     "key": "/apisix/ssl/1"
                 },
-                "action": "set"
+                "action": "compareAndSwap"
             }]]
             )