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: add token support for consul discovery (#9532) #10278

Merged

Conversation

sevensolutions
Copy link
Contributor

@sevensolutions sevensolutions commented Sep 28, 2023

Description

This PR adds support for ACL tokens when using consul or consul_kv service discovery as described in #9532.

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)

@monkeyDluffy6017
Copy link
Contributor

monkeyDluffy6017 commented Oct 2, 2023

Good job! I will check it later

Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek left a comment

Choose a reason for hiding this comment

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

please support consul_kv as well.

@monkeyDluffy6017
Copy link
Contributor

Good job! please resolve the review comments, thanks!

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Oct 7, 2023
@sevensolutions
Copy link
Contributor Author

please support consul_kv as well.

@shreemaan-abhishek I'am working on that.

Can anyone please tell me how to write an assert for accepting one of two error codes?
I tried:

--- error_code eval
qr/200|403/

but this doesn't seem to work. It doesn't match but according to Regex101 it should.
The error is:

got: '403'
expected: '(?^:200|403)'

The reason is, that bootstrapping the ACL twice is not allowed which returns 403 which is totally fine.

@shreemaan-abhishek
Copy link
Contributor

Could you please share the complete test case?

The reason is, that bootstrapping the ACL twice is not allowed which returns 403 which is totally fine.

Do the test case results alternate between 200 and 403? If it returns 403 every time you can just check for the 403 error code 🤔

@sevensolutions
Copy link
Contributor Author

This is the complete test:

=== TEST 13: bootstrap acl
--- config
location /v1/acl {
    proxy_pass http://127.0.0.1:8502;
}
--- request eval
"PUT /v1/acl/bootstrap\n" . "{\"BootstrapSecret\": \"2b778dd9-f5f1-6f29-b4b4-9a5fa948757a\"}"
--- error_code eval
qr/200|403/

Yes the result alternates. It returns 200 the first time but every re-run will return 403 because the ACL system can only be bootstrapped once. And because testing the ACL bootstrap procedure of consul is not the case here, I would say both retrurn codes are fine.

@shreemaan-abhishek
Copy link
Contributor

Is it okay to not consider the re-run case and expect error_code: 200? Because in the CI if during a re-run all systems start from scratch.

If you are testing manually in your machine consider restarting the docker container or something.

I hope I didn't get you wrong 😂

@sevensolutions
Copy link
Contributor Author

sevensolutions commented Oct 7, 2023

Is it okay to not consider the re-run case and expect error_code: 200? Because in the CI if during a re-run all systems start from scratch.

If you are testing manually in your machine consider restarting the docker container or something.

I hope I didn't get you wrong 😂

The thing is that i'am sharing the consul docker instance between the test files consul.t and consul_kv.t.
My idea was to add this test to both files because it's needed for the following tests to be executed.
Or is there a guaranteed order in which the test files are beeing executed?

EDIT: Got it working. There is a special error_code_like option.
--- error_code_like: ^(?:200|403)$

@monkeyDluffy6017 monkeyDluffy6017 added approved and removed wait for update wait for the author's response in this issue/PR labels Oct 8, 2023
@shreemaan-abhishek
Copy link
Contributor

please fix the failing tests

@monkeyDluffy6017 monkeyDluffy6017 added wait for update wait for the author's response in this issue/PR and removed approved user responded labels Oct 9, 2023
@sevensolutions
Copy link
Contributor Author

@monkeyDluffy6017 can you please re-run the failed chaos-test workflow? It couldnt install helm for some reason.

@monkeyDluffy6017 monkeyDluffy6017 merged commit 7390d77 into apache:master Oct 9, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user responded wait for update wait for the author's response in this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants