-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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(acls) add admin endpoints to interact with acls #3039
Conversation
Two features that are missing: Retrieve consumers associated with a groupAn endpoint such as Retrieve ACL group namesI feel having an endpoint The problem I'm facing, it seems there is no straightforward way of doing: select distinct group from acls; Is there away around of doing this, or we will need to update the core DB logic? I could take a stab at it if there is another use case for such a query. IMHO, this would be pretty useful for admin purposes. Another issue is we don't have An inefficient implementation could be a brute-force approach, which traverses through an entire paginated set of ACLs and figure out the unique groups. @thibaultcha Thoughts? |
@hbagdi Very nice! This is super exciting. The issue indeed is that there is no way to distinctively select groups. The DAO doesn't support this feature, and cannot, as the limitation comes from Cassandra itself (and the data model chosen for this entity).
If carefully written this could be fine but it isn't ideal that's for sure. I'd rather not risk it, iterating over entire tables/column families (in Cassandra) is very much a discouraged practice. We have users and customers using those features more or less extensively, so this could cause serious performance issues (potentially slowing down an entire cluster) for those heavy users.
If we write a migration to create an index on the Ideally, we come up with a better ACL solution once our new data model is completed (we are working on it). |
@thibaultcha I've been thinking a little bit about grouping or tagging of APIs/Consumers. It's also possible to start an experimenting with such a plugin and then move it into the core if there's traction. Although, supporting it from the core would give the ability to maintain/manage a group of entities with ease. |
@thibaultcha Since, this PR is related with other *-auth changes in admin API, can we will include this in the upcoming release? |
Two new endpoints have been added to the ACL plugin: * `/acls/` to paginate through all acls for all consumers * `/acls/:acl_id/consumer` to retrieve the Consumer associated with an acl See Kong/kong#3039
I just had a look and it seems like the However, I'm not sure how well this will play with Lapis and the other endpoint we register: |
|
||
assert.not_same(json_1.data, json_2.data) | ||
assert.is_nil(json_2.offset) -- last page | ||
end) |
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.
I had a doubt and tried to run this test with Cassandra, and encountered a few errors that made it fail. Here is the patch I had to add to make this test succeed with Cassandra:
diff --git a/spec/03-plugins/19-acl/01-api_spec.lua b/spec/03-plugins/19-acl/01-api_spec.lua
index 99ad25f9..31435e59 100644
--- a/spec/03-plugins/19-acl/01-api_spec.lua
+++ b/spec/03-plugins/19-acl/01-api_spec.lua
@@ -331,7 +331,11 @@ describe("Plugin: acl (API)", function()
res = assert(admin_client:send {
method = "GET",
- path = "/acls?size=3&offset=" .. json_1.offset,
+ path = "/acls",
+ query = {
+ size = 3,
+ offset = json_1.offset,
+ }
})
body = assert.res_status(200, res)
local json_2 = cjson.decode(body)
@@ -340,7 +344,10 @@ describe("Plugin: acl (API)", function()
assert.equal(6, json_2.total)
assert.not_same(json_1.data, json_2.data)
- assert.is_nil(json_2.offset) -- last page
+ -- Disabled: on Cassandra, the last page still returns a
+ -- next_page token, and thus, an offset proprty in the
+ -- response of the Admin API.
+ --assert.is_nil(json_2.offset) -- last page
end)
it("retrieves acls for a consumer_id", function()
local res = assert(admin_client:send {
It is likely the case for other contributions we've recently commited that embed the same test.
I just gave it a try, and, lucky for us, it seems like Lapis is perfectly able to make the distinction between the two endpoints! ["/acls/:acl_group/consumers"] = {
GET = function(self, dao_factory, helpers)
return helpers.responses.send_HTTP_OK("/acls/:acl_group/consumers")
end
}
|
I have just opened #3051 to address the failures when using Cassandra for your previous contributions. Mind giving it a look? Also, FYI, our merging window will be closed this Monday, before the release on Wednesday. |
* `/acls/` to paginate through all acls for all consumers * `/acls/:acl_id/consumer` to retrieve the Consumer associated with an acl
Regarding:
I had tried the same thing and in fact the endpoint was not an issue but processing the request with pagination support. Since, we can find Acls belonging to a single In such a case, if a group has a few thousand Consumers then we need pagination support for the endpoint. A hack comes to mind, where we could internally paginate based on I have bandwidth to include further changes today. |
@thibaultcha Good catch with the pagination support. I've included your patch in the updated commit on this PR. |
Yes, the pagination in such a case might be tricky and might require custom usage of the DAO (the "CRUD helpers" are, after all, just helpers for the most common tasks). Maybe we should leave this out of 0.11.2 and I am thinking of simply taking this PR as is for now. |
Yes, let's take this PR as for 0.11.2.
Let me know if we need to update anything.
…On Nov 26, 2017 1:11 PM, "Thibault Charbonnier" ***@***.***> wrote:
Yes, the pagination in such a case might be tricky and might require
custom usage of the DAO (the "CRUD helpers" are, after all, just helpers
for the most common tasks.
Maybe we should leave this out of 0.11.2 and I am thinking of simply
taking this PR as is for now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3039 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEeiOfWExGljB5rVkFGMW77gx0iKUT6Xks5s6dPqgaJpZM4Qi3Zw>
.
|
return helpers.responses.send_HTTP_NOT_FOUND() | ||
end | ||
|
||
self.params.username_or_id = acls[1].consumer_id |
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.
I think that we are missing a self.params.acl_id = nil
instruction here. Not an issue with find_consumer_by_username_or_id
, but it could become one further down the road in one of the HTTP verb handlers. Better safe than sorry!
I will take care of this while merging, no worries.
Manually merged with some minor modifications - thank you very much for your work! |
Two new endpoints have been added to the ACL plugin: * `/acls/` to paginate through all ACLs * `/acls/:acl_id/consumer` to retrieve the Consumer associated with an ACL See Kong/kong#3039 From #557 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
Two new endpoints have been added to the ACL plugin: * `/acls/` to paginate through all ACLs * `/acls/:acl_id/consumer` to retrieve the Consumer associated with an ACL See Kong/kong#3039 From #557 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
Hi everyone, |
/acls/
to paginate through all acls for all consumers/acls/:acl_id/consumer
to retrieve the Consumer associated with an aclIssues resolved
Fix #2188
Close #2371