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: support Nacos ak/sk authentication #10445

Merged
merged 8 commits into from
Nov 13, 2023

Conversation

yuweizzz
Copy link
Contributor

@yuweizzz yuweizzz commented Nov 5, 2023

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)

@@ -308,6 +308,8 @@ nginx_config: # Config for render the template to generate n
# connect: 2000 # Default 2000ms
# send: 2000 # Default 2000ms
# read: 5000 # Default 5000ms
# access_key: ""
# secret_key: ""
Copy link
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?

@@ -39,6 +39,8 @@ local auth_path = 'auth/login'
local instance_list_path = 'ns/instance/list?healthyOnly=true&serviceName='
local default_namespace_id = "public"
local default_group_name = "DEFAULT_GROUP"
local access_key
local secret_key
Copy link
Contributor

@monkeyDluffy6017 monkeyDluffy6017 Nov 6, 2023

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
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
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)

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Nov 6, 2023


=== TEST 7: get APISIX-NACOS info from NACOS - configured in services
--- yaml_config eval: $::yaml_config
Copy link
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
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 on lines +555 to +556
access_key: "my_access_key"
secret_key: "my_secret_key"
Copy link
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
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?

Copy link
Contributor

@monkeyDluffy6017 monkeyDluffy6017 Nov 8, 2023

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

@monkeyDluffy6017 monkeyDluffy6017 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants