Skip to content

feat: support Nacos ak/sk authentication#10445

Merged
moonming merged 8 commits into
apache:masterfrom
yuweizzz:feat/nacos-ak-sk
Nov 13, 2023
Merged

feat: support Nacos ak/sk authentication#10445
moonming merged 8 commits into
apache:masterfrom
yuweizzz:feat/nacos-ak-sk

Conversation

@yuweizzz

@yuweizzz yuweizzz commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

Description

feat: support Nacos ak/sk authentication

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Comment thread apisix/discovery/nacos/init.lua
Comment thread conf/config-default.yaml Outdated
# send: 2000 # Default 2000ms
# read: 5000 # Default 5000ms
# access_key: ""
# secret_key: ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add some comments for the two configurations?

Comment thread t/discovery/nacos3.t
local default_namespace_id = "public"
local default_group_name = "DEFAULT_GROUP"
local access_key
local secret_key

@juststillthinking juststillthinking Nov 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm not very familiar with nacos, how does these configurations work? Don't we need to configure the Nacos?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In nacos open source version, these configurations was no needed, but it can work in Alibaba Cloud MSE Nacos when MSE Nacos enabled authentication, and it also need to create a RAM user and grant permissions. More details in https://www.alibabacloud.com/help/en/mse/user-guide/access-authentication-by-the-nacos-client .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So it's for MSE only? Could you describe this in the comment? like

#    access_key: ""        # Nacos AccessKey ID in Alibaba Cloud, notice that it's for Nacos instances on Microservices Engine (MSE)
#    secret_key: ""         # Nacos AccessKey Secret in Alibaba Cloud, notice that it's for Nacos instances on Microservices Engine (MSE)

@juststillthinking juststillthinking added the wait for update wait for the author's response in this issue/PR label Nov 6, 2023
Comment thread t/discovery/nacos3.t


=== TEST 7: get APISIX-NACOS info from NACOS - configured in services
--- yaml_config eval: $::yaml_config

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How does this work if the opensource nacos doesn't support MSE but you add access_key and secret_key here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the part is a implement from nacos sdk, the sign functions will add three args in query request url, if you use opensource nacos, extras args will not take effect in query request, but it's hard for me to make a mse nacos here for test.

Comment thread t/discovery/nacos3.t
Comment on lines +555 to +556
access_key: "my_access_key"
secret_key: "my_secret_key"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If these configurations don't work, why do you add so many test cases?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because we can use mse nacos instead of it and run test in local environment easily, and also see does it take any effect in open sourece verison. what do you think of it?

@juststillthinking juststillthinking Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add a comment at the top of the test file, like: we can't use mse nacos to test, access_key and secret_key won't affect the open source nacos

@juststillthinking juststillthinking added approved and removed wait for update wait for the author's response in this issue/PR labels Nov 8, 2023
@moonming
moonming merged commit d8bd344 into apache:master Nov 13, 2023
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.

3 participants