bug: the script field in Route is limited to map, instead of string

[imjoey]

Bug report

Describe the bug

In APISIX, we can see the script field in Route is of type string.
https://github.com/apache/apisix/blob/3e19b06293463f52c8d26f0958542fc475ff0fcd/apisix/schema_def.lua#L456

While as creating a Route in manager-api, we forcibly cast the script to map[string]interface{}.

apisix-dashboard/api/internal/handler/route/route.go

Line 332 in 0f322e1

input.Script, err = generateLuaCode(input.Script.(map[string]interface{}))

So if sending a request to manager-api as below,

curl http://127.0.0.1:8000/apisix/admin/routes -X POST -H "Authorization: auth-header" -d '{"uri":"/hello","script":"local _M = {} \n function _M.access(api_ctx) \n ngx.log(ngx.INFO,\"hit access phase\") \n end \nreturn _M","upstream":{"type":"roundrobin","nodes":{"127.0.0.1:1980":1}}}'

manager-api will always complain with the following error:

err； interface conversion: interface {} is string, not map[string]interface {}
[Recovery] 2021/01/12 - 14:23:53 panic recovered:

interface conversion: interface {} is string, not map[string]interface {}
/usr/local/opt/go/libexec/src/runtime/iface.go:261 (0x100cd6e)
	panicdottypeE: panic(&TypeAssertionError{iface, have, want, ""})
/Users/imjoey/Work/apache-apisix/apisix-dashboard/api/internal/handler/route/route.go:332 (0x193d9fe)

As a contrast, the similar request can be successfully processed by APISIX Admin API.

Is such situation mentioned above as expected? Much appreciated.

Expected behavior

manager-api could also accept the script field of string type.

System information

• OS: macOS
• Version: latest in master branch

Additional

This issue is also related to #1085, put here for tracking.

[starsz]

Good capture. I think we need to fix it.

By the way, I think we need middleware to recover the panic. Let me create another issue.
Already support.

[imjoey]

@juzhiyuan @starsz

I've one more question, when creating a Route, manager-api will store the script field into etcd with prefix /apisix/script, treating Script as top level resource type. While, AFAIK, not the same as APISIX, since it only regards Script as an inner property of Route, only stored with in Route object. This definitely introduces inconsistency between manager-api and Admin API. Is this also as expected, or would be a feature in future? Thanks.

[juzhiyuan]

cc @nic-chen

[nic-chen]

oh, please have a look at test cases for plugin orchestration:
https://github.com/apache/apisix-dashboard/blob/master/api/test/e2e/route_with_plugin_orchestration_test.go

and you will know why.

[imjoey]

oh, please have a look at test cases for plugin orchestration:
https://github.com/apache/apisix-dashboard/blob/master/api/test/e2e/route_with_plugin_orchestration_test.go

and you will know why.

@nic-chen

Thanks for pointing this out. I guess you mean the Script of map type is only for plugin orchestration in FE, just as the comment -- The 'script' fields below are used by dashboard for plugin orchestration in https://github.com/apache/apisix/blob/3e19b06293463f52c8d26f0958542fc475ff0fcd/apisix/schema_def.lua#L455 ? Is that correct?

If yes, may I ask that how we could add a script like "local _M = {} \n function _M.access(api_ctx) \n ngx.log(ngx.INFO,\"hit access phase\") \n end \nreturn _M" in Route via manager-api, to implement the serverless services?

Please let me know if I missed anything important. 😄 Much appreciated.

[nic-chen]

oh, please have a look at test cases for plugin orchestration:
https://github.com/apache/apisix-dashboard/blob/master/api/test/e2e/route_with_plugin_orchestration_test.go
and you will know why.

@nic-chen

Thanks for pointing this out. I guess you mean the Script of map type is only for plugin orchestration in FE, just as the comment -- The 'script' fields below are used by dashboard for plugin orchestration in https://github.com/apache/apisix/blob/3e19b06293463f52c8d26f0958542fc475ff0fcd/apisix/schema_def.lua#L455 ? Is that correct?

If yes, may I ask that how we could add a script like "local _M = {} \n function _M.access(api_ctx) \n ngx.log(ngx.INFO,\"hit access phase\") \n end \nreturn _M" in Route via manager-api, to implement the serverless services?

Please let me know if I missed anything important. 😄 Much appreciated.

Yes. The threshold for manually writing Lua code to implement plugin orchestration is too high. We prefer that users use the FE to drag for plugin orchestration, not write Lua code.

[imjoey]

oh, please have a look at test cases for plugin orchestration:
https://github.com/apache/apisix-dashboard/blob/master/api/test/e2e/route_with_plugin_orchestration_test.go
and you will know why.

@nic-chen
Thanks for pointing this out. I guess you mean the Script of map type is only for plugin orchestration in FE, just as the comment -- The 'script' fields below are used by dashboard for plugin orchestration in https://github.com/apache/apisix/blob/3e19b06293463f52c8d26f0958542fc475ff0fcd/apisix/schema_def.lua#L455 ? Is that correct?
If yes, may I ask that how we could add a script like "local _M = {} \n function _M.access(api_ctx) \n ngx.log(ngx.INFO,\"hit access phase\") \n end \nreturn _M" in Route via manager-api, to implement the serverless services?
Please let me know if I missed anything important. 😄 Much appreciated.

Yes. The threshold for manually writing Lua code to implement plugin orchestration is too high. We prefer that users use the FE to drag for plugin orchestration, not write Lua code.

@nic-chen got it. Much appreciated for the explanation. I will propose a PR that also supports string type script alongside the existing map type, in order to eliminate the errors. Pls assign this to me, if possible. Thanks.

[nic-chen]

oh, please have a look at test cases for plugin orchestration:
https://github.com/apache/apisix-dashboard/blob/master/api/test/e2e/route_with_plugin_orchestration_test.go
and you will know why.

@nic-chen
Thanks for pointing this out. I guess you mean the Script of map type is only for plugin orchestration in FE, just as the comment -- The 'script' fields below are used by dashboard for plugin orchestration in https://github.com/apache/apisix/blob/3e19b06293463f52c8d26f0958542fc475ff0fcd/apisix/schema_def.lua#L455 ? Is that correct?
If yes, may I ask that how we could add a script like "local _M = {} \n function _M.access(api_ctx) \n ngx.log(ngx.INFO,\"hit access phase\") \n end \nreturn _M" in Route via manager-api, to implement the serverless services?
Please let me know if I missed anything important. 😄 Much appreciated.

Yes. The threshold for manually writing Lua code to implement plugin orchestration is too high. We prefer that users use the FE to drag for plugin orchestration, not write Lua code.

@nic-chen got it. Much appreciated for the explanation. I will propose a PR that also supports string type script alongside the existing map type, in order to eliminate the errors. Pls assign this to me, if possible. Thanks.

Thanks.