refactor(clustering/compat): using array to store compatible checker functions

[chronolaw]

Summary

Move compatible config-checking logic into a hash table, making code more readable.

Need special cherry-pick action to EE after #10400 and #10501 be cherry-picked.

Checklist

• The Pull Request has tests
• There's an entry in the CHANGELOG
• There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

[flrgh]

I'm not sure that pairs() is sufficient for this. There is an expectation that the compatibility change functions are executed in descending order (3.2.0.0, then 3.1.0.0, then 3.0.0.0, etc).

Curious, have you taken a look at the compat code in koko? It's not without its quirks, but I think this would be a good pattern for us to follow--pretty big task to implement though.

[chronolaw]

Thanks for your opinion. I have changed the hash table to an array.

Do you think it is OK?

[chronolaw]

Need update to #10192

[oowl]

For the compact problem, I have another piece of advice. we have all of the kong schemas, and we know all of the kong version schemas, Could this compatibility logic be solved in a similar way to how we use code generation in bindgen? we use more reliable code(iterating all kong schema) to generate the Version compatibility code.

[chronolaw]

Need update to #10400

[vm-001]

The instance_name part looks OK to me. And also has a corresponding test to cover that.

[chronolaw]

For the compact problem, I have another piece of advice. we have all of the kong schemas, and we know all of the kong version schemas, Could this compatibility logic be solved in a similar way to how we use code generation in bindgen? we use more reliable code(iterating all kong schema) to generate the Version compatibility code.

It is a good idea, perhaps we can do it in another PR in the future.

[chronolaw]

Echoing request-review request from Harry.

1/ From the maintainability perspective

1. I don't see any sort function being called before iterating the COMPATIBILITY_CHECKERS, which means the program correctness is relying on the order of definition. Could we add some comments above the compatible_checkers var to describe that? Or could we introduce a sort mechanism?
2. We may add more and more properties to each checker. I feel using the array index checker[n] is not friendly. What do you think of using map table to replace array table for each checker?

local compatible_checkers = {
  { 
    -- desc = "resolve plugin.instance_name",
    -- ... any other properties
    version = x, 
    fn = function() end,
   }
}

2/ From the style perspective

1. checker is not an accuracy variable name here as it not only checks something but also rewrites something inside. resolver, or handler (or something else) may be a good candidate.

Overall, this is just my option and won't block the merge progress.

It is a very valuable suggestion, thank you @vm-001.

But Applying these changes may need more time and discussion, I think we could improve it in another PR later.

[liverpool8056]

Agree with @attenuation . And I think the current logic for compatibility is so complex, and we have to write the spec very carefully for ensuring the compatibility won't be broken when schemas are changed. It should have been a chance to simplify it, cause I think it's the DP's responsibility to ensure it can work even if it receives a unexpected config, most of the time it just receives some unknown fields or so. DP should have been capable to protect itself, at the same time, from the perspective of the users. DP should have been capable to ensure there is no unexpected behavior for the users as well.

[chronolaw]

Updated to #10501.

[bungle]

I think it's the DP's responsibility to ensure it can work even if it receives a unexpected config, most of the time it just receives some unknown fields or so. DP should have been capable to protect itself, at the same time, from the perspective of the users. DP should have been capable to ensure there is no unexpected behavior for the users as well.

@liverpool8056,

I think it is CPs responsibility to send right config in a first place. The DPs should just put everything CP sends in LMDB. The DPs also already have ability to remove unknown fields on read. It would also be great if CP streams config in such a way that it could be easily be imported in LMDB, and not through all the unnecessary parsing, flattening, validation - so what you describe is what DPs already do, just that we need to remove the validation/flattening part when receiving config so that it does not error on it, of course this needs then to be backported to all supported lower versions. The current way we do this is backwards. We try to remove unknown fields on CP side while we still allow configuring them. You cannot ever solve this issue with it:

CP 1.0:
Imaginary plugin ACL-NEW has config.deny field.

DP 1.0:
Gets config from CP 1.0 and knows what to do with config.deny field.

CP upgraded to 1.1:
The ACL-NEW plugin gets new config.allow field.
Admin configures a new ACL-NEW plugin and sets config.allow

DP 1.0:
Gets config from CP 1.1, but CP removes the config.allow field from newly added ACL-NEW plugin, the ACL-NEW plugin does nothing on DP 1.0 and it is a serious security issue. (this cannot ever be solved with our current way of doing it). The DP cannot ever ensure it can work the right way in this situation. There is zero that DP can do, other than ignore or error. Our current strategy is to remove field on CP, which is about the same as DP ignoring errors. But a bit more visible to us, and more involving, and more error prone. Downgrading "never" works. Upgrading might, but we don't support newer DPs than CP, so you cannot upgrade DP beyond CP version.

DP 1.1:
Gets config from CP 1.1 and ACL-NEW plugin works.

On DBless it is worth to error on invalid config. just not on hybrid as there we should not have invalid configs in a first place, which means CP should use lowest (supported) common denominator when selecting schema version (which is set by the oldest supported connected DP, and when CP starts to support newer schema it should forbid older DPs connecting).

[hbagdi]

@chronolaw Please open the corresponding EE cherry pick PR.

[chronolaw]

@chronolaw Please open the corresponding EE cherry pick PR.

Sure, but this PR need #10400 and #10501 to be cherry-picked first.

[hbagdi]

@chronolaw Please help out reviewing the cherrypicks of those PRs and then work on cherry-picking this one.

[outsinre]

@chronolaw @hbagdi , as not yet manually cherry-picked. This will be cherry-picked by CE2EE merge PR: https://github.com/Kong/kong-ee/pull/4985.