**Background:**

Kong ensures the backward compatibility through the logic of `remove_fields` that CP with higher verion can removes fields which are not supported by DPs in lower version before sending the configuration to DPs. In detail, CP checks the version reporting by DP. If the version from DP is less than the version defined in `remove_fields`, the fields will be removed. So the logic is just a simple comparison. **Issue:** This logic works well for most of the cases. However, after serval backports to various version series, the comparsion should become more complex as there would be several incontinous ranges where the fields should be removed due to multiple backports. **Solution:** In this PR, a lower bound is added to the comparison when necessary rather than only a upper bound previously. The new logic is that the comparison should punch through the range of the minor version for the earliest version since the field is supported, otherwise, the comparsion should be limited with the range of minor version. **Example** Let say a new field is initially introduced since 3.7.7. At this monent, an entry defined in `removed_fields.lua` should be: ``` -- Any dataplane older than 3.7.7 [3007007000] = { some_plugin = { "new_field", }, }, ``` And then it is backported into 3.5.5, 3.6.6. After that, the ranges where it's going to be removed or not turns into the followings: - (-infinite, 3.5.5 ), remove - [ 3.5.5, 3.6.0 ), don't remove - [ 3.6.0, 3.6.6 ), remove - [ 3.6.6, 3.7.0 ), don't remove - [ 3.7.0, 3.7.7 ), remove - [ 3.7.7, +infinite), don't remove At this monent, in terms of the new logic, the `removed_fields.lua` should be: ``` -- Any dataplane older than 3.5.5 [3005005000] = { some_plugin = { "new_field", }, }, -- Any dataplane older than 3.6.6 [3006006000] = { some_plugin = { "new_field", }, }, -- Any dataplane older than 3.7.7 [3007007000] = { some_plugin = { "new_field", }, }, ``` The comparsion should be `dp_version < 3005005000` for the first entry as it is the earliest version it is supported and it should punch through the minor version. The other two comparsions should be: `dp_version < 3006006000 and dp_version >= 3006000000` and `dp_version < 3007007000 and dp_version >= 3007000000` respectively as it is not the earliest version it is supported and the comparison should be limited within the minor version. With the help of this change, the `removed_fields.lua` can be consistent across version series, and we can easily append a new entry correspondingly when there is a backport.
Kong · Mar 25, 2024 · 7fccf6e · 7fccf6e
1 parent 58fc97d
commit 7fccf6e
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 77 deletions.
diff --git a/kong/clustering/compat/init.lua b/kong/clustering/compat/init.lua
@@ -9,6 +9,7 @@ local ipairs = ipairs
 local table_insert = table.insert
 local table_sort = table.sort
 local gsub = string.gsub
+local floor = math.floor
 local split = utils.split
 local deflate_gzip = require("kong.tools.gzip").deflate_gzip
 local cjson_encode = cjson.encode
@@ -24,7 +25,8 @@ local extract_major_minor = version.extract_major_minor
 
 local _log_prefix = "[clustering] "
 
-local REMOVED_FIELDS = require("kong.clustering.compat.removed_fields")
+local REMOVED_FIELDS_ITERATOR = require("kong.clustering.compat.removed_fields")
+local FIELDS = nil
 local COMPATIBILITY_CHECKERS = require("kong.clustering.compat.checkers")
 local CLUSTERING_SYNC_STATUS = constants.CLUSTERING_SYNC_STATUS
 local KONG_VERSION = meta.version
@@ -315,25 +317,38 @@ do
 
   function get_removed_fields(dp_version)
     local plugin_fields = cache[dp_version]
-    if plugin_fields ~= nil then
+    if plugin_fields then
       return plugin_fields or nil
     end
 
+    local seen = {}
+    local inserted = false
+
     -- Merge dataplane unknown fields; if needed based on DP version
-    for ver, plugins in pairs(REMOVED_FIELDS) do
-      if dp_version < ver then
-        for plugin, items in pairs(plugins) do
-          plugin_fields = plugin_fields or {}
-          plugin_fields[plugin] = plugin_fields[plugin] or {}
+    for ver, plugins in REMOVED_FIELDS_ITERATOR(FIELDS) do
+      for plugin, items in pairs(plugins) do
+        for _, name in ipairs(items) do
+          local key = plugin .. ":" .. name
+          if not seen[key] then
+            seen[key] = true
+            -- punchthrough minor version if the plugin is not already in the list
+            if dp_version < ver then
+              plugin_fields = plugin_fields or {}
+              plugin_fields[plugin] = plugin_fields[plugin] or {}
+              table_insert(plugin_fields[plugin], name)
+              inserted = true
+            end
 
-          for _, name in ipairs(items) do
+          elseif dp_version < ver and dp_version >= floor(ver/10000)*10000 then
+            -- compare within the minor version if the plugin is already in the list
             table_insert(plugin_fields[plugin], name)
+            inserted = true
           end
         end
       end
     end
 
-    if plugin_fields then
+    if inserted then
       -- sort for consistency
       for _, list in pairs(plugin_fields) do
         table_sort(list)
@@ -350,10 +365,8 @@ do
   -- expose for unit tests
   _M._get_removed_fields = get_removed_fields
   _M._set_removed_fields = function(fields)
-    local saved = REMOVED_FIELDS
-    REMOVED_FIELDS = fields
+    FIELDS = fields
     cache = {}
-    return saved
   end
 end
 

diff --git a/kong/clustering/compat/removed_fields.lua b/kong/clustering/compat/removed_fields.lua
@@ -1,59 +1,52 @@
-return {
-  -- Any dataplane older than 3.1.0
-  [3001000000] = {
-    -- OSS
-    acme = {
-      "enable_ipv4_common_name",
-      "storage_config.redis.ssl",
-      "storage_config.redis.ssl_verify",
-      "storage_config.redis.ssl_server_name",
-    },
-    rate_limiting = {
-      "error_code",
-      "error_message",
-    },
-    response_ratelimiting = {
-      "redis_ssl",
-      "redis_ssl_verify",
-      "redis_server_name",
+-- An iterator that returns key and value pairs of removed fields from the removed_fields_mapping table.
+-- The key and the corresponding value pairs returned by the iterator should increase in version number.
+-- The value is a table of fields to be removed in that version.
+local function iterator(t)
+  local keys = {}
+  for k in pairs(t) do
+    keys[#keys+1] = k
+  end
+  table.sort(keys)
+  local len = #keys
+  local i = 0
+  return function()
+    if i > len then
+      i = 0
+    end
+    i = i + 1
+    return keys[i], t[keys[i]]
+  end
+end
+
+local removed_fields_mapping = {
+  -- Any dataplane older than 3.6.0
+  [3006000000] = {
+    opentelemetry = {
+      "sampling_rate",
     },
-    datadog = {
-      "retry_count",
-      "queue_size",
-      "flush_timeout",
+  },
+
+  -- Any dataplane older than 3.5.0
+  [3005000000] = {
+    acme = {
+      "storage_config.redis.scan_count",
     },
-    statsd = {
-      "retry_count",
-      "queue_size",
-      "flush_timeout",
+    cors = {
+      "private_network",
     },
     session = {
-      "cookie_persistent",
-    },
-    zipkin = {
-      "http_response_header_for_traceid",
+      "read_body_for_logout",
     },
   },
-  -- Any dataplane older than 3.2.0
-  [3002000000] = {
-    statsd = {
-      "tag_style",
+
+  -- Any dataplane older than 3.4.0
+  [3004000000] = {
+    rate_limiting = {
+      "sync_rate",
     },
-    session = {
-      "audience",
-      "absolute_timeout",
-      "remember_cookie_name",
-      "remember_rolling_timeout",
-      "remember_absolute_timeout",
+    proxy_cache = {
       "response_headers",
-      "request_headers",
-    },
-    aws_lambda = {
-      "aws_imds_protocol_version",
     },
-    zipkin = {
-      "phase_duration_flavor",
-    }
   },
 
   -- Any dataplane older than 3.3.0
@@ -87,33 +80,65 @@ return {
     },
   },
 
-  -- Any dataplane older than 3.4.0
-  [3004000000] = {
-    rate_limiting = {
-      "sync_rate",
+  -- Any dataplane older than 3.2.0
+  [3002000000] = {
+    statsd = {
+      "tag_style",
     },
-    proxy_cache = {
+    session = {
+      "audience",
+      "absolute_timeout",
+      "remember_cookie_name",
+      "remember_rolling_timeout",
+      "remember_absolute_timeout",
       "response_headers",
+      "request_headers",
+    },
+    aws_lambda = {
+      "aws_imds_protocol_version",
     },
+    zipkin = {
+      "phase_duration_flavor",
+    }
   },
 
-  -- Any dataplane older than 3.5.0
-  [3005000000] = {
+  -- Any dataplane older than 3.1.0
+  [3001000000] = {
+    -- OSS
     acme = {
-      "storage_config.redis.scan_count",
+      "enable_ipv4_common_name",
+      "storage_config.redis.ssl",
+      "storage_config.redis.ssl_verify",
+      "storage_config.redis.ssl_server_name",
     },
-    cors = {
-      "private_network",
+    rate_limiting = {
+      "error_code",
+      "error_message",
+    },
+    response_ratelimiting = {
+      "redis_ssl",
+      "redis_ssl_verify",
+      "redis_server_name",
+    },
+    datadog = {
+      "retry_count",
+      "queue_size",
+      "flush_timeout",
+    },
+    statsd = {
+      "retry_count",
+      "queue_size",
+      "flush_timeout",
     },
     session = {
-      "read_body_for_logout",
+      "cookie_persistent",
     },
-  },
-
-  -- Any dataplane older than 3.6.0
-  [3006000000] = {
-    opentelemetry = {
-      "sampling_rate",
+    zipkin = {
+      "http_response_header_for_traceid",
     },
   },
 }
+
+return function(fields)
+  return iterator(fields or removed_fields_mapping)
+end
diff --git a/spec/01-unit/19-hybrid/03-compat_spec.lua b/spec/01-unit/19-hybrid/03-compat_spec.lua
@@ -6,7 +6,7 @@ local cjson_decode = require("cjson.safe").decode
 local ssl_fixtures = require ("spec.fixtures.ssl")
 
 local function reset_fields()
-  compat._set_removed_fields(require("kong.clustering.compat.removed_fields"))
+  compat._set_removed_fields()
 end
 
 describe("kong.clustering.compat", function()