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

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

@@ -1009,7 +1009,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1060,7 +1060,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1094,7 +1094,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1128,7 +1128,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1162,7 +1162,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1194,7 +1194,7 @@ passed
                         },
                         "key": "/apisix/routes/1"
                     },
-                    "action": "set"
+                    "action": "compareAndSwap"
                 }]]
             )
 
@@ -1246,7 +1246,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"
             }]]
             )