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

Add ACL functions to admin client #697

Merged
merged 24 commits into from Oct 2, 2020

Conversation

shubhanilBag
Copy link
Contributor

Resolves issue: #465

Signed-off-by: Shubhanil Bag <shubhanil.bag@gmail.com>
Signed-off-by: Shubhanil Bag <shubhanil.bag@gmail.com>
Signed-off-by: Shubhanil Bag <shubhanil.bag@gmail.com>
Signed-off-by: Shubhanil Bag <shubhanil.bag@gmail.com>
Signed-off-by: Shubhanil Bag <shubhanil.bag@gmail.com>
Signed-off-by: Shubhanil Bag <shubhanil.bag@gmail.com>
Signed-off-by: Shubhanil Bag <shubhanil.bag@gmail.com>
Signed-off-by: Shubhanil Bag <shubhanil.bag@gmail.com>
Copy link
Collaborator

@Nevon Nevon left a comment

Choose a reason for hiding this comment

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

This is a really solid contribution! Thank you so much. I left some comments on things we should address, but in large this is really good.

@@ -0,0 +1,228 @@
const createAdmin = require('../index')
// const { KafkaJSProtocolError } = require('../../errors')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete these unused imports.

src/admin/__tests__/createAcls.spec.js Outdated Show resolved Hide resolved
saslBrokers,
} = require('testHelpers')

const RESOURCE_TYPES = require('../../protocol/resourceTypes')
Copy link
Collaborator

Choose a reason for hiding this comment

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

See #649 (comment) for some issues regarding RESOURCE_TYPES.

Basically, we have been misusing ResourceTypes for configs, when they are supposed to be used in the way that you are here, for ACLs. So before we start having more people use ResourceTypes, I would like to figure out a good way to split the two so that we have CONFIG_RESOURCE_TYPES and ACL_RESOURCE_TYPES.

The old RESOURCE_TYPES still needs to be exported for backwards compatibility until we decide to release a major version, but it would be good if we can prepare by having most people already now import either CONFIG_RESOURCE_TYPES or ACL_RESOURCE_TYPES, so that the break affects fewer people.

So what I would suggest is that we within this PR introduce ACL_RESOURCE_TYPES, which is just the old RESOURCE_TYPES re-exported under a different name, and use that in all examples and tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, we should probably prefix all the ACL-related protocol types with ACL_ just to avoid this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nevon SGTM, let's do that, also I would suggest to keep types in a separate folder like: src/protocol/types/ACL/Resource.js or src/protocol/types/ACLResources.js

src/admin/__tests__/createAcls.spec.js Outdated Show resolved Hide resolved
src/admin/index.js Outdated Show resolved Hide resolved
src/admin/index.js Outdated Show resolved Hide resolved
src/admin/index.js Outdated Show resolved Hide resolved
src/broker/index.js Show resolved Hide resolved
src/protocol/requests/deleteAcls/v0/response.js Outdated Show resolved Hide resolved
if (filterResponsesWithError.length > 0) {
throw createErrorFromCode(filterResponsesWithError[0].errorCode)
}
for (let i = 0; i < data.filterResponses.length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We try to avoid numeric for loops unless necessary. A simple for (let filterResponse of data.filterResponses) would be preferred.

@shubhanilBag
Copy link
Contributor Author

@Nevon Thanks for the thorough review! I opened the draft PR to check if the basic ACL test is working in the CI. I am still working to on it to make it ready for review, that's includes code cleanup, separating the test case into separate files, more proper validation checks in the admin client and others including your suggestions 😄

@Nevon Nevon marked this pull request as ready for review May 18, 2020 10:58
@EricMCornelius
Copy link

Anything I can do to assist in getting this feature moving along?

@tulios
Copy link
Owner

tulios commented Sep 29, 2020

Hey @EricMCornelius, I will take over this PR soon and make sure it gets merged. We had other priorities but now it's the time to get this one in.

@tulios
Copy link
Owner

tulios commented Sep 29, 2020

Step 1 to get this to master (#898)

@tulios tulios self-requested a review September 30, 2020 20:31
@tulios
Copy link
Owner

tulios commented Sep 30, 2020

@Nevon take a final look, it's all good for me now.

@tulios
Copy link
Owner

tulios commented Oct 1, 2020

I forgot about the Typescript types, that's still missing. I can add it later.

*/

module.exports = {
decode,
Copy link
Collaborator

@Nevon Nevon Oct 1, 2020

Choose a reason for hiding this comment

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

v1 of all the *Acls APIs return the responses before throttling, meaning that we should support client side throttling. That's already been implemented on the network layer, so all you have to do on the protocol level is to copy the throttleTime property to a clientSideThrottleTime property on the decoded object and the rest is handled automatically (see

const decode = async rawData => {
const decoded = await decodeV1(rawData)
return {
...decoded,
clientSideThrottleTime: decoded.throttleTime,
}
}
for example)

Copy link
Owner

@tulios tulios left a comment

Choose a reason for hiding this comment

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

The PR is also missing protocol and broker tests, I will add those before I merge.

@tulios tulios merged commit e72263f into tulios:master Oct 2, 2020
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.

None yet

4 participants