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(api) expose arbitrary plugin data from a schema-meta.lua #5171

Closed
wants to merge 4 commits into from

Conversation

javierguerragiraldez
Copy link
Contributor

Summary

Adds a /plugins/meta/:name endpoint that returns a JSON representation of the value returned by a (optional) schema-meta.lua module in the plugin directory.

Any case of wrong plugin name, missing schema-meta or bad Lua syntax results in a 404 response. If no name is given, returns a 400 response

Issues resolved

PoC for Trello card: https://trello.com/c/ut8gMx93

@@ -186,6 +186,24 @@ return {
end
},

["/plugins/meta/:name"] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

/plugins/schema/:name has been deprecated and moved over to /schemas/plugins/:name (/plugins/X/* is generally used by plugin X so adding entries there is prone to naming conflicts).

In the case of /plugins/schema/:name, it has been deprecated and moved over to /schemas/plugins/:name.

Perhaps we could do the same and do /schemas/plugins/:name/metadata? The /schemas/* entries are currently defined in kong/api/routes/kong.lua.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about /plugins/:name/metadata ? it's really not about schema at all, after all

Copy link
Member

Choose a reason for hiding this comment

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

I think we should move away from /plugins endpoint.

Do we plan on including meta for custom DAOs as @bungle is asking. And is it possible that we have these meta for core entities too some day?
I think these extensions could be useful in future to further add in more meta for each entity.
I think we should this under and /meta collection of endpoints as /meta/plugins/:name, which keeps the door open for core entities in future, such as /meta/service.

end

-- TODO: either unload module or use loadfile() instead
local ok, meta = utils.load_module_if_exists("kong.plugins." .. name .. ".schema-meta")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a matter of taste, but I would just go with .metadata instead (schema-meta sounds too much like metaschema, which is unrelated, and even more so to the also-confusing schema_meta table from the database migrations which is even more unrelated).

Copy link
Member

Choose a reason for hiding this comment

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

Do we plan to include metadata of DAOs too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand the requirement, it's about user-visible info to describe the plugin itself, description, type, icon. I don't think DAOs are visible enough to need such annotations.

(Of course, once it's in place, somebody will find new uses for it)

Copy link
Member

@kikito kikito left a comment

Choose a reason for hiding this comment

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

In order to be completed, this PR needs at least one integration test which exercises the new endpoint.

Edit: I missed that this was for a PoC. I'm fine if there are no tests for this at this point then.

description = [[ Developer-supplied data. ]],
["/metadata/plugins/:name"] = {
GET = {
title = [[ A plugin's metadata. ]],
Copy link
Contributor

@hishamhm hishamhm Nov 6, 2019

Choose a reason for hiding this comment

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

the title field usually starts with an action verb, with no period:

Suggested change
title = [[ A plugin's metadata. ]],
title = [[Retrieve the metadata for a plugin]],

@@ -390,6 +391,33 @@ return {
},
},
},
metadata = {
title = [[ Metadata ]],
description = [[ Developer-supplied data. ]],
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs at least a paragraph-long, explaining that this is arbitrary free-form data, etc.

Copy link
Member

Choose a reason for hiding this comment

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

There is an implicit assumption that almost everything on the admin api is part of the compatibility promise.

If this is going to be free form data, it would help us in future if we clearly document the compatibility promise here for Open source.

Copy link

Choose a reason for hiding this comment

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

I know I asked for this feature, but I get cold feet when we save user-generated, user-visible, free-form data, especially without a plan for localization. Plugin category (Authentication, Logging, etc) and possibly icon URL seem less problematic, but this begs the question whether all of this should go in an external registry as has been suggested elsewhere.

return kong.response.exit(400, { message = "Bad query" })
end

-- TODO: either unload module or use loadfile() instead
Copy link
Contributor

Choose a reason for hiding this comment

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

load_module_if_exists wraps require and is the way used for loading plugin modules elsewhere, so it's probably best to keep using it and unload the module from package.loaded.

@javierguerragiraldez
Copy link
Contributor Author

@Kong/team-interfaces is this what you need?

@kinman
Copy link

kinman commented Nov 13, 2019

@javierguerragiraldez sorry, this fell off our radar a bit. Will get back to you by end of this week.

metadata = {
title = [[ Metadata ]],
description = [[ Developer-supplied data. ]],
["/metadata/plugins/:name"] = {
Copy link

Choose a reason for hiding this comment

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

I was discussing this with @darrenjennings , who makes the good point that while this is RESTful and consistent with the rest of the Admin API, it also requires the frontend to make upwards of 40 API calls just to render a list of plugins like https://manager-steady.kong-cloud.com/default/plugins/select/. That's not really practical, but the alternatives don't fit well into the Admin API as it exists today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a few alternatives off the top of my head:

  • gather the metadata of all installed plugins on a single call (just /metadata/plugins/, without a name?)
  • let the client ask for specific metadata fields (/metadata/plugins/<name>/?f[]=icon&f[]=author&f[]=description&f[]=licence. optional plugin name, as on previous point). after all, only known fields can be rendered on the UI, right?
  • use a registry. not only can be managed, but could be GraphQL if you wish!

personally, I still think a registry is the best solution, but i'm aware that the decision hinges mostly on what's the desired process for external plugin writers.

@guanlan guanlan closed this Nov 19, 2019
@javierguerragiraldez javierguerragiraldez deleted the feat/meta-info branch March 31, 2020 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants