-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: Redesign the architecture of the plugin system #6580
Comments
yes! Some time I want use a plugin to config diffent properties. |
@nfrankel Could you give a configuration snippet about how to configuring plugins when using the new arch? Also, we need to evaluate the cost for the re-design, such as how to refactor plugins so that plugin codes are reentrant. |
@tokers Would something like that be possible? curl -i https://my.host/apisix/admin/plugin_configs/1 -H 'X-API-KEY: abc' -X PUT -d '{
"plugins": [
"plugin": {
"id": "ext-plugin-req",
"priority": 500,
"conf": [{
"name": "ext-plugin-A",
"value": "{\"enable\":\"feature\"}"}
]},
},
"plugin": {
"id": "ext-plugin-req",
"priority": -500,
"conf": [{
"name": "ext-plugin-B",
"value": "{\"disable\":\"feature\"}"}
]},
}
]
}' |
Got it. But since we use the array to orchestrate plugins, I think we can remove the priority field. |
I don't think so. AFAIK, plugins can be set on different levels ( |
IMHO that's confusing, the intuitive order is Route > Service, if we let users decide the order, it's error-prone. |
Imagine the user has defined a Service with plugin A and priority 5. Now, they define a route with plugin B and C. How would you compute the order based on the array? |
That's tricky. We may think about another way to allow a plugin can be run multiple times but still use the object to store their config. I'm wondering if there are some circumstances that people want to run the same plugin in a single route but not continuously (e.g. run plugin A, plugin B, plugin A). |
If not, what about the following way: {
"uri": "/api/*",
"host": "foo.com",
"plugins": {
"limit-count": [
{
"time_window": 60,
"count": 10,
"key": "remote_addr"
},
{
"time_window": 1,
"count": 5,
"key": "route_id"
}
]
}
}
|
Is it possible to support more flexible configuration methods? For example, hamc-auth supports multiple configurations and uses the |
I think it's not that hmac-auth itself supports |
yes. but may be not the same plugin. perhaps the same type. |
hi @nfrankel , I create a new example follow your way. I think it looks good now. $ curl -i https://my.host/apisix/admin/plugin_configs/1 -H 'X-API-KEY: abc' -X PUT -d '{
"desc": "blah",
"plugins": [
{
"id": "limit-count",
"priority": 500,
"conf": [
{
"time_window": 60,
"count": 10,
"key": "remote_addr"
},
{
"time_window": 1,
"count": 5,
"key": "route_id"
}
]
},
{
"id": "ip-restriction",
"priority": 200,
"conf": [
{
"whitelist": [
"127.0.0.1",
"113.74.26.106/24"
]
}
]
}
]
}' |
@membphis LGTM 🙂 |
I still think it'll confuse users, an array has an order attribute, but indeed we don't run the plugin as per their order in the array. |
@tokers That's a legitimate concern. What about: $ curl -i https://my.host/apisix/admin/plugin_configs/1 -H 'X-API-KEY: abc' -X PUT -d '{
"desc": "blah",
"plugins": {
"limit-count": {
"priority": 500,
"conf": [
{
"time_window": 60,
"count": 10,
"key": "remote_addr"
},
{
"time_window": 1,
"count": 5,
"key": "route_id"
}
]
},
"ip-restriction": {
"priority": 200,
"conf": [
{
"whitelist": [
"127.0.0.1",
"113.74.26.106/24"
]
}
]
}
}
}' |
Looks better than the original one, but still has the |
It is too similar to the old plugins configuration structure. How to be compatible with the old version? {
"desc": "blah",
"plugins": {
"limit-count": {
...
},
"ip-restriction": {
...
}
}
} |
From a developer's aspect, it is hard to distinguish this from the current arch, as "priority" / "conf" can be a valid field. What about using a new field instead of "plugins" for the route conf? |
Do you have any suggestion? |
Let's take a step back. At the moment, we are not discussing what is the best developer experience but how not to make it too similar to the existing approach. That's another problem, and we can tackle it in another thread using API versioning, for which there are several solutions (e.g., path, header, query parameter). |
I still cannot capture the meaning for using a "priority" field. |
Currently, APISIX is route-based proxy (every configuration is bound to the route). This proposal will require changing APISIX to a plugin-based proxy (every configuration is organized as a plugin configuration), which needs to rewrite APISIX totally, so IMHO I can't accept such a proposal. What about a smaller change, like adding a priority field in the route's plugin? Like:
|
I think this is acceptable. |
Why the extra verbosity?
|
@nfrankel |
Issue description
Current situation
At the moment, you add a plugin in the request flow by referencing with its name and setting the configuration. The plugin is effectively a singleton.
This approach has some downsides:
Proposal
In Object-Oriented speak, make
Plugin
a class and introducePluginInstance
"objects" (or something similarly named):Plugin
defines the codePluginInstance
references thePlugin
and associates it with a priority and a configurationDecoupling the code from the configuration/priority would bring much more freedom for users.
The text was updated successfully, but these errors were encountered: