Skip to content

feat: add token support for consul discovery (#9532)#10278

Merged
juststillthinking merged 4 commits into
apache:masterfrom
sevensolutions:feat/add-consul-token-support
Oct 9, 2023
Merged

feat: add token support for consul discovery (#9532)#10278
juststillthinking merged 4 commits into
apache:masterfrom
sevensolutions:feat/add-consul-token-support

Conversation

@sevensolutions

@sevensolutions sevensolutions commented Sep 28, 2023

Copy link
Copy Markdown
Contributor

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)

@juststillthinking

juststillthinking commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Good job! I will check it later

@shreemaan-abhishek shreemaan-abhishek left a comment

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.

please support consul_kv as well.

Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread ci/pod/docker-compose.first.yml
@juststillthinking

Copy link
Copy Markdown
Contributor

Good job! please resolve the review comments, thanks!

@juststillthinking juststillthinking added the wait for update wait for the author's response in this issue/PR label Oct 7, 2023
@sevensolutions

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

sevensolutions commented Oct 7, 2023

Copy link
Copy Markdown
Contributor Author

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

@juststillthinking juststillthinking added approved and removed wait for update wait for the author's response in this issue/PR labels Oct 8, 2023
@shreemaan-abhishek
shreemaan-abhishek self-requested a review October 8, 2023 14:22
@shreemaan-abhishek

Copy link
Copy Markdown
Contributor

please fix the failing tests

@juststillthinking juststillthinking 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
Copy Markdown
Contributor Author

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

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