Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(plugins) Syncing plugin version to the Kong version #8772

Merged
merged 4 commits into from
Jun 2, 2022

Conversation

tyler-ball
Copy link
Contributor

Summary

With almost all Kong plugins bundled inside Kong itself we want to
remove customer confusion by syncing all plugin versions to the Kong
version. This allows users to:

  1. Refer to the Kong version when reporting plugin issues, instead of
    having to look up the plugin version.
  2. Update our plugin documentation to be Kong version centric instead
    of plugin version centric starting in Kong 3.0
  3. Developers no longer need to remember to update the plugin version
    when making bundled plugin changes.
  4. Simplify changelog management by only having 1 changelog for Kong

I suggest looking at and reviewing each commit individually as quite a few files were touched. I also deleted old unecessary files (rockspec, changelog, license).

Full changelog

  • Sync every plugin version to the Kong version

Issue reference

FT-2775

Copy link
Contributor

@flrgh flrgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful!

Just to double check: are there any unreleased plugin-specific changelog entries (i.e. post 2.8) that need to get copy-pasted into the top-level changelog so they aren't lost?

@@ -40,7 +41,7 @@ local ACLHandler = {}


ACLHandler.PRIORITY = 950
ACLHandler.VERSION = "3.0.1"
ACLHandler.VERSION = kong_meta._VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to discuss: currently, this will break in kong-ee because _VERSION has the following format x.y.z-enterprise-edition. see: https://github.com/Kong/kong-ee/blob/master/kong/meta.lua#L26

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to update this to kong_meta.version and introduce the version key, matching what is in https://github.com/Kong/kong-ee/pull/3191/files#diff-f2919e234cadaf71fe36827f6b1e90f6fd8babd16cb39f6a71fee5fccc85e3acR29

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@tyler-ball
Copy link
Contributor Author

tyler-ball commented May 11, 2022

NTS: check the / endpoint and see if it includes plugin versions

@tyler-ball
Copy link
Contributor Author

We do not return plugin version info on the / endpoint, just loaded_plugins list and available_on_server plugins

@Tieske
Copy link
Member

Tieske commented May 13, 2022

this looks like an extremely dangerous thing to do. The same plugin will report a different version on the next version of Kong. I'm not the expert on this but I think it will break all compatibilty checks between CP and DP when upgrading.

The version belongs to the plugin (it's plugin meta-data) and hence should be set there, never depend dynamically on the Kong version.

I think the better approach would be to update the plugin versions, hardcoded, not dynamic!, to the version of Kong in which they were shipped. Albeit that might defeat the purpose of this PR to reduce customer confusion.

my 2cts.

Ping @mikefero @hbagdi @RobSerafini

@Tieske Tieske added the version-compatibility Incompatibility with older data plane versions label May 13, 2022
@mikefero
Copy link
Contributor

this looks like an extremely dangerous thing to do. The same plugin will report a different version on the next version of Kong. I'm not the expert on this but I think it will break all compatibility checks between CP and DP when upgrading.

Version compatibility between the CP and DP currently use the Kong Gateway version and disregard plugin version; we went that route because we knew what features were bundled in each version of the gateway which simplified the logic quite a bit. With that stated we really should clean up the compatibility layer @tyler-ball to remove unnecessary checks that this PR creates in that layer (see check_configuration_version as one example).

I think the better approach would be to update the plugin versions, hardcoded, not dynamic!, to the version of Kong in which they were shipped. Albeit that might defeat the purpose of this PR to reduce customer confusion.

Are you suggesting doing this at release time? That is doable as well, but I think the end result would be the same. From the version compatibility standpoint this shouldn't cause any issues there, but does need to be thoroughly tested before merging this in.

@Tieske
Copy link
Member

Tieske commented May 13, 2022

Are you suggesting doing this at release time?

I'd suggest that at release time we update all UPDATED plugins to match the new Kong version, so anything that didn't change, doesn't get updated. Especially the dynamic link to the Kong version makes me very uncomfortable, because why wouldn't we simply remove the plugin version field from the code in that case?

Version compatibility between the CP and DP currently use the Kong Gateway version and disregard plugin version

That is ignoring the custom plugins? We have folks running dozens of custom plugins on a single dataplane...

@mikefero
Copy link
Contributor

because why wouldn't we simply remove the plugin version field from the code in that case?

I agree with you there, doesn't make much sense for sure. It does make sense to keep this in there for now and maybe deprecate in some future major; WDYT?

That is ignoring the custom plugins? We have folks running dozens of custom plugins on a single dataplane...

There isn't any version compatibility layer for custom plugins though; unless I am mistaken. I remember our conversation around moving all the VC logic into the plugins or out of the control_plane layer (which would be ideal), but we could still build that functionality into the gateway itself so custom plugin developers could add their own features for VC,

@tyler-ball
Copy link
Contributor Author

tyler-ball commented May 16, 2022

because why wouldn't we simply remove the plugin version field from the code in that case?

I'm also in favor of this. I had the same conclusion and investigated doing it at one point. I don't remember exactly why I didn't go that route - I don't remember what reads the plugin version. I think I also wanted to introduce less risk. But I am in favor of removing the version entirely in the future

@Tieske
Copy link
Member

Tieske commented May 17, 2022

Discussed this with @RobSerafini today. And by now I'm convinced we cannot remove the plugin versions. There will always be custom plugins and we need the version checks there anyway. Whether we like it or not we'll have to deal with compatibility logic.

There isn't any version compatibility layer for custom plugins though; unless I am mistaken. I remember our conversation around moving all the VC logic into the plugins or out of the control_plane layer (which would be ideal)

That would be the preferred way, since only the plugin itself has the contextual information to know how it should convert its config into an older-version-of-self config. But in that case; the plugin needs to carry a version, simply to know what the target version of the plugin is it needs to convert to.

We can take a shortcut to this by syncing all Kong plugin to the Kong core version. But in the end that's not enough because users will be running custom plugins. So imho we better have a single approach in this.

slightly related; #8810

Brain farts;

  • the VERSION constant in plugins should move into schema.lua I think, so Koko can also access it.
  • if the version is in the schema, the schema can also be the place where we add the version-conversion logic (since that is "simple Lua", whereas the handler.lua will probably pull in everything-but-the-kitchen-sink, which won't work in Koko)

@fffonion
Copy link
Contributor

I love the tags cloud on this PR 😄 .

For attendees of this PR, do we have agreed on the path for this PR? Do we want to put this in 3.0?

@tyler-ball tyler-ball added this to the 3.0 milestone May 25, 2022
@aboudreault
Copy link
Contributor

@fffonion yes, we would like this PR in 3.0.

@hbagdi Can you comment about the following points, raised by @Tieske :

  1. the VERSION constant in plugins should move into schema.lua I think, so Koko can also access it.
  2. if the version is in the schema, the schema can also be the place where we add the version-conversion logic (since that is "simple Lua", whereas the handler.lua will probably pull in everything-but-the-kitchen-sink, which won't work in Koko)

@aboudreault
Copy link
Contributor

As discussed offline, moving the VERSION to another file wont help.

tyler-ball and others added 4 commits June 2, 2022 08:45
With almost all Kong plugins bundled inside Kong itself we want to
remove customer confusion by syncing all plugin versions to the Kong
version. This allows users to:

1) Refer to the Kong version when reporting plugin issues, instead of
   having to look up the plugin version.
2) Update our plugin documentation to be Kong version centric instead
   of plugin version centric starting in Kong 3.0
3) Developers no longer need to remember to update the plugin version
   when making bundled plugin changes.
4) Simplify changelog management by only having 1 changelog for Kong

Signed-off-by: Tyler Ball <[email protected]>
Plugins that have been migrated to this repo had existing rockspec files
that are no longer used or necessary. These files are included into the
kong package by the top level rockspec file.

Signed-off-by: Tyler Ball <[email protected]>
Now that these plugins are no longer external, they are governed by the
top level LICENSE file.

Signed-off-by: Tyler Ball <[email protected]>
@aboudreault aboudreault merged commit c54a2e9 into master Jun 2, 2022
@aboudreault aboudreault deleted the feat/plugin_versions branch June 2, 2022 12:47
aboudreault added a commit that referenced this pull request Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment