-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: support aliyun mse nacos ram #4702
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
Conversation
|
@zll2 Hi, please have a check at the CI https://github.com/apache/apisix/pull/4702/checks?check_run_id=3190661841 |
docs/en/latest/discovery/nacos.md
Outdated
| - "http://${username}:${password}@${host1}:${port1}" | ||
| authorization: | ||
| type: "basic_auth" # default basic_auth | ||
| access_key: "" # default empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can omit the key for this auth type.
BTW, basic_auth makes me think about this: https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication. Is there a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basic_auth 是原先 nacos.lua 里的分支判断,enreka 是使用了该判断,我看了下实际是不需要,就给去掉了;为了区分和 阿里云 MSE Nacos 的认证鉴权类型区分,然后就拿 basic_auth 作为默认的 type 了;开源版的 Nacos 自建的话,可以使用 用户名 及 密码进行加强安全验证,也可以不使用,但 阿里云 MSE Nacos 的商业版本在安全认证的方面仅支持 AccessKey & Access Secret 进行 OpenAPI 交互;
我看 APISIX 插件中的也是叫 basic—auth,你有更好的建议吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The official docs don't give a name to this authorization: https://nacos.io/zh-cn/docs/auth.html
What about just call it "default"? The default authorization of Nacos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好的,我重新提交一个 PR,这个 PR 我先关闭了。
apisix/discovery/nacos.lua
Outdated
| local group_name = param_values['group_name'] | ||
| local time_ngx = ngx.utctime() | ||
| local time_change = string.gsub(time_ngx, " ", "T") | ||
| local time_utc = table.concat({time_change, "Z"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use time_change .. "Z" directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
哈哈哈,第一次写 Lua,有些语法和函数上就不是特别了解
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我改改掉
apisix/discovery/nacos.lua
Outdated
| end | ||
|
|
||
| return url, username, password | ||
| return url, username, password, access_key, secret_key, authorization_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about returning:
url, authorization_type, username, password, access_key, secret_key
or
url, authorization_type, access_key, secret_key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里的建议是 authorization_type 返回顺序往前提?
1、username, password
2、access_key, secret_key
1 和 2 是两两组合使用,用于支持 2 种不同的认证方式
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo in my suggestion...
It should be
url, authorization_type, username, password
or
url, authorization_type, access_key, secret_key
It seems that each authorization_type only uses two of them, so we can return only four values instead of six.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好的,我改下
support aliyun mse nacos ram authentication
What this PR does / why we need it:
Pre-submission checklist: