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(plugin): add an optional instance_name field for plugin entity #10077

Merged
merged 6 commits into from
Jan 20, 2023

Conversation

vm-001
Copy link
Contributor

@vm-001 vm-001 commented Jan 6, 2023

Summary

Currently, a Plugin instance must be identified by id, and users are unable to give a custom name for a Plugin instance. This is very user-unfriendly, and causes users confusion on Kong Manager.

Therefore, adding an optional instance_name field for Plugin entity could address this kind of situation.

Not only the instance_name can be used in the request body for CRUD of /plugins/{id}, but also

  • /plugins/{instance_name}
  • /services/{service}/plugins/{instance_name}
  • /routes/{route}/plugins/{instance_name}

Checklist

Full changelog

  • Add an optional instance_name field for Plugin entity

Issue reference

KAG-134

@pull-request-size pull-request-size bot added size/L and removed size/XS labels Jan 6, 2023
@vm-001 vm-001 force-pushed the feat/plugin-custom-name branch from 3ae0ff3 to ebe8176 Compare January 6, 2023 07:02
@vm-001 vm-001 changed the title feat(test): test ci feat(plugin): add plugin custom name Jan 6, 2023
@vm-001 vm-001 changed the title feat(plugin): add plugin custom name feat(plugin): add custom_name field for plugin entity Jan 6, 2023
@vm-001 vm-001 changed the title feat(plugin): add custom_name field for plugin entity feat(plugin): add an optional custom_name field for plugin entity Jan 6, 2023
@vm-001 vm-001 force-pushed the feat/plugin-custom-name branch 2 times, most recently from b78a761 to 05f1294 Compare January 6, 2023 07:52
@vm-001 vm-001 marked this pull request as ready for review January 6, 2023 07:52
@vm-001 vm-001 force-pushed the feat/plugin-custom-name branch from 05f1294 to 104a95b Compare January 6, 2023 08:17
@vm-001 vm-001 force-pushed the feat/plugin-custom-name branch from 104a95b to 6c112c1 Compare January 9, 2023 05:00
@vm-001 vm-001 requested a review from hishamhm January 9, 2023 05:09
@vm-001 vm-001 force-pushed the feat/plugin-custom-name branch from 6c112c1 to 8be8225 Compare January 9, 2023 07:03
@vm-001 vm-001 requested a review from bungle January 9, 2023 07:55
@chronolaw
Copy link
Contributor

chronolaw commented Jan 9, 2023

It looks good, did you consider the clustering compatibility?

Refer to #9972 .

@vm-001 vm-001 requested a review from flrgh January 9, 2023 08:13
@vm-001 vm-001 added this to the 3.2 milestone Jan 9, 2023
@GGabriele
Copy link
Contributor

@vm-001 I think Kong should validate that custom_name is not set to any "reserved" plugin name (basically, custom_name not in plugins.available_on_server).

WDYT?

@vm-001
Copy link
Contributor Author

vm-001 commented Jan 10, 2023

@vm-001 I think Kong should validate that custom_name is not set to any "reserved" plugin name (basically, custom_name not in plugins.available_on_server).

WDYT?

Kong will has more and more official plugins during its iteration. I'm concerned if I do so, then a valid custom_name will become invalid after upgrading Kong, because it conflicts with the new plugin. Therefore, I suggest not adding this kind of validation.

@@ -1386,7 +1386,7 @@ function _M.new(connector, schema, errors)
})

local conflict_key = unique_escaped
if has_composite_cache_key then
if has_composite_cache_key and not unique_field.is_endpoint_key then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When doing the operation upsert by field, if that field is used as endpoint_key, then it is most likely unique(based Kong RESTful style), and should be used as SQL conflict condition, instead of using the composite cache_key.

@vm-001
Copy link
Contributor Author

vm-001 commented Jan 11, 2023

It looks good, did you consider the clustering compatibility?

Refer to #9972 .

Done. d80e10b

@bungle
Copy link
Member

bungle commented Jan 11, 2023

Yes, it is unfortunate that plugin.name is reserved and hard to break (as name we use in general with other entities to have a freeform name for them). Aka have plugin.handler=request-transformer, plugin.name=my-transformer. But I am pretty sure we cannot do that (especially in minor release).

Alternatives to custom_name:

  • title
  • alias
  • nick or nickname

Do we need to think about name more broadly if we in a future use this to make it possible to run same plugin multiple times?

@vm-001
Copy link
Contributor Author

vm-001 commented Jan 12, 2023

Alternatives to custom_name: title, alias, nick or nickname.

Let's do a very simple poll in the community. Adding emoji on this comment to vote.

🚀 to vote custom_name
👍 to vote title
😄 to vote alias
🎉 to vote nickname

@hanshuebner
Copy link
Contributor

Sorry to come up with another suggestion. What we're naming is the plugin instance, so why not be explicit and use instance_name? alias implies that there is a 1:1 correspondence with another name, but as we are using name to name the plugin type, it would be misleading. Likewise for nickname and custom_name. title sounds too descriptive.

@bungle
Copy link
Member

bungle commented Jan 12, 2023

Sorry to come up with another suggestion. What we're naming is the plugin instance, so why not be explicit and use instance_name? alias implies that there is a 1:1 correspondence with another name, but as we are using name to name the plugin type, it would be misleading. Likewise for nickname and custom_name. title sounds too descriptive.

I was thinking about that too. But my personal self hates the long names with underscores and it means a lot more to type with everyone (users/customers included). Internally we talk about instances, but I am not sure if that is right term either. We don't create instances of plugins, we just call the same static "class" like plugin with different configuration. Of course here by instance we mean applying logic (aka run on this route/service/consumer or global or combination) and the configuration which to call the plugin. All that said, I am fine with instance_name, it just wouldn't be my pick, ;-).

Edit: on the otherhand if we allow to run same plugin multiple times, the instance_name is quite good.

@bungle
Copy link
Member

bungle commented Jan 12, 2023

but as we are using name to name the plugin type, it would be misleading

Yes that is unfortunate. If I were to choose without thinking about legacy and tooling we would have type/handler field that replaces current name field, and then name field for free form name of the "instance".

@jschmid1
Copy link
Contributor

What we're naming is the plugin instance, so why not be explicit and use instance_name? alias implies that there is a 1:1 correspondence with another name, but as we are using name to name the plugin type, it would be misleading.

Imo, this is the right hint. It is really just a way to customize the identifier to a plugin instance. Also bearing in mind that we may eventually have support for multiple plugin instances, this is the right path 👍🏻

@hanshuebner hanshuebner force-pushed the feat/plugin-custom-name branch from 2ba1d1f to 5fac9f3 Compare January 19, 2023 05:51
@hanshuebner hanshuebner requested a review from bungle January 19, 2023 05:56
@hanshuebner hanshuebner changed the title feat(plugin): add an optional custom_name field for plugin entity feat(plugin): add an optional instance_name field for plugin entity Jan 19, 2023
@hanshuebner hanshuebner force-pushed the feat/plugin-custom-name branch from 6ef363c to c35a10d Compare January 19, 2023 13:56
@hanshuebner hanshuebner merged commit f7f5367 into master Jan 20, 2023
@hanshuebner hanshuebner deleted the feat/plugin-custom-name branch January 20, 2023 09:29
@chronolaw
Copy link
Contributor

Should we add a field in kong/include/kong/model/plugin.proto?

@StarlightIbuki
Copy link
Contributor

Should we add a field in kong/include/kong/model/plugin.proto?

Seems this file is not needed anymore. Will confirm tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants