Add ACL protocol and admin methods #1646

ulrikjohansson · 2018-11-17T11:39:01Z

Added protocols for ACLs
Added KafkaAdmin methods for manipulating ACLs
Added very basic test of the acl_resource module contents

I haven't been able to run the test suite locally for some reason, just small parts in isolation, like running the new test case with pytest /test/test_admin.py

I've tested this to some extent manually, both with a simple docker kafka setup (kafka v2.0), as well as with a more serious cluster secured with SSL/ACLs already (kafka v 1.1). Describing, Creating and Deleting ALCs seem to work with both v0 and v1 versions of the protocol.

Fixes #1638

This change is

jeffwidman · 2018-11-17T11:45:05Z

This looks great!

I'm in the middle of cleaning up some of the core sending methods (#1639, #1640, and #1641) and I'd like to land those first because it'll help keep things consistent...

I suspect it will make your life easier as well because of the clear pattern for error handling / raising.

Unfortunately, it will require a rebase, but hopefully we can get this in within the next few days...

jeffwidman · 2018-11-19T00:51:01Z

@ulrikjohansson I just finished merging all my changes, so can you rebase and then I'll review?

You'll probably want to look through these recent commits for examples of various ways to handle things by leveraging other pieces of the library... for example error handling, retries (not sure if you need those) properly sending to coordinator / controller / nodes, etc.

I know @dpkp would like to cut a new release early this coming week and it'd be nice to get these in as well since they add nice functionality... If you can rebase today, I'll try to review tonight or tomorrow morning.

ulrikjohansson · 2018-11-19T14:32:27Z

I'll have a look today, but can't promise I'll have time for more than the rebase. We'll start there and see what I can manage 😀 Btw, there were lots of failing stuff in the travis build, was that all stuff I introduced?

ulrikjohansson · 2018-11-19T18:08:14Z

I've done the refactoring I could clearly see needed fixed. If there are other things that need some attention before this can be merged, please point me in the right direction.

jeffwidman · 2018-11-19T19:11:30Z

Thanks, I'll review right now.

Btw, there were lots of failing stuff in the travis build, was that all stuff I introduced?

No, that was because pytest went to 4.0 and some warnings are now errors. Dana pinned it in eae0edd

Fixing it requires redoing the pytest fixtures that we use which would be much simpler after the fix for #1193 is merged, so probably will wait til then to clean that up.

jeffwidman

A few comments... the capitalization of the ACL classes + more extensive docstrings for the public methods are the two changes I'd like to see before merging.

jeffwidman · 2018-11-19T19:16:33Z

kafka/admin/acl_resource.py

+except ImportError:
+    # vendored backport module
+    from kafka.vendor.enum34 import IntEnum
+


style nit: pep8 suggests 2 blank lines between imports and code

kafka/admin/acl_resource.py

jeffwidman · 2018-11-19T19:24:06Z

kafka/admin/kafka.py

-    # describe_acls protocol not yet implemented
-    # Note: send the request to the least_loaded_node()
+    def describe_acls(self, acl_resource):
+        """Describe a set of ACLs


Can you expand this docstring to describe the input param?

It matters because the docstring is what shows up in https://kafka-python.readthedocs.io/en/master/apidoc/KafkaAdmin.html and is what many users will be referring to for how to use this method, for example what to pass in as an acl_resource...

So specify what the input params are, what return value is (which is currently just the response struct, so the user will need to do their own error-checking), and anything a user needs to be careful about or be aware of when using the method.

This is true of all the public methods, including the create/delete methods as well.

jeffwidman · 2018-11-19T19:26:22Z

kafka/admin/acl_resource.py

+            pattern_type=AclResourcePatternType.LITERAL
+    ):
+        if not isinstance(resource_type, AclResourceType):
+            resource_type = AclResourceType[str(resource_type).upper()]  # pylint: disable-msg=unsubscriptable-object


IIUC, you are accepting both strings and AclResourceTypes...

But I don't see much (any?) error checking, so could this result in a user thinking they were successful but they actually weren't?

IE, would it be better to just require folks to always pass in AclResourceType objects?

I don't know the answer, as I haven't ever worked with Kafka ACLs, so I don't know the complexity of the types... but wanted to better understand the tradeoffs here...

To be honest, I've pretty much just copied how it was done for the ConfigResource. That essentially goes for this whole PR. I tried to just keep to the same patterns that were already there.
I do not have extensive experience/knowledge of how ACLs are implemented in the Java client either, this PR is driven by a need for a more flexible and workable solution to administrate ACLs than the shell/java-combination that ships with kafka.

I think that the above is just a convenience thing what was built into the ConfigResource, and perhaps it's better to not do that to start with. It doesn't really require much more effort, since they are simple enums. I'll go with that for now.

Agree, for now let's go with the more strongly-typed approach, as its far easier to relax the constraints later rather than the other way around.

jeffwidman · 2018-11-19T19:30:53Z

kafka/admin/kafka.py

+        )
+
+    def create_acls(self, acl_resources):
+        """Create a set of ACLs"""


Needs more extensive documentation, see: #1646 (comment)

jeffwidman · 2018-11-19T19:31:01Z

kafka/admin/kafka.py

+        )
+
+    def delete_acls(self, acl_resources):
+        """Delete a set of ACLSs"""


Needs more extensive documentation, see: #1646 (comment)

ulrikjohansson · 2018-11-19T21:49:04Z

The more I look at the ACL methods in kafka/admin/kafka.py, the less convinced I am that this should get pushed through in a hurry.
I'm not an expert in the intricacies of how the Java client interacts with kafka, and what a good API would be in the KafkaAdmin class. There are lots of edge cases (if found some more already), where some combinations of ACLRequest and ACLResource config results in exceptions in the broker, and just drops the connection for example.

Perhaps it would be a better idea to split this PR up into 2 parts instead:
Part 1 has the protocol stuff, that can probably get pushed pretty much right away, it's just basically a copy of the protocol specification at https://kafka.apache.org/protocol
If people really want to start fiddling with these protocols they can, but they're on their own for now (people like me).
Part 2 would be the stuff on top using the protocol classes. This needs more thinking and polish, since the users of this interface are not people used to digging through the innards of the packages they use.

What are your thoughts on this?

(It's getting late over here. I'm calling it quits for today, but will check in again in the morning)

jeffwidman · 2018-11-20T01:03:42Z

I think that sounds wise.

Good suggestion to pull in the protocol stuff immediately, that will let you start forging ahead with playing with this... I double-checked the definitions, and it all looks correct to me. So I just cherry-picked your protocol commit onto master to make sure it lands in the 1.4.4 release.

But the rest of this I absolutely agree it makes sense to discuss/research w/o as much time pressure. If you can continue to lead the charge here, I would really appreciate it, as Kafka ACLs aren't something that I have any experience with yet... I expect we'll start implementing them at work in the next 6-18 months depending on various things, and then I'll know more. But it'd be nice to have this land sooner than that, plus you are already aware of edge cases, etc.

ulrikjohansson · 2018-11-20T08:31:23Z

I'll keep working on it =)

jeffwidman · 2019-05-29T09:10:47Z

Any update on this?

ulrikjohansson · 2019-05-29T13:46:21Z

Haven't worked much on ACL stuff at work lately, so it's kind of been put on the back burner.
I still have a branch with most of the stuff working, so I'll try to find some time this weekend to just go through it again.
Perhaps it might be easier to just close this PR and make a new one when I'm ready for another round, it can get a bit messy with rebases and stuff. What do you think?
I'll try to address all the feedback brought up in this PR either way.

ulrikjohansson · 2019-06-03T20:41:36Z

I opened a new PR, since the stuff is on a different local branch now, see #1833

ulrikjohansson force-pushed the acl_protocol branch from bedaef0 to 94fc746 Compare November 19, 2018 17:14

jeffwidman requested changes Nov 19, 2018

View reviewed changes

jeffwidman reviewed Nov 19, 2018

View reviewed changes

dpkp force-pushed the master branch from c09bb5c to 45196e3 Compare November 20, 2018 16:38

Ulrik Johansson added 8 commits November 24, 2018 12:31

Add KafkaAdmin ACL methods

5132fa8

Add basic test for AclResource

628cc48

Use new client errors

fc54f94

Bugfix, match against correct schema

7fa3c51

Send Acl requests to least loaded node

ae9ab37

Capitalize ACL types properly

0349bc9

Fix pep-8 blank line issue

7c7f5bf

Require resource type objects when creating an ACLResource

6f40051

ulrikjohansson force-pushed the acl_protocol branch from 7a8f837 to 6f40051 Compare November 24, 2018 11:54

ulrikjohansson mentioned this pull request Jun 3, 2019

Add ACL api to KafkaAdminClient #1833

Merged

ulrikjohansson closed this Aug 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ACL protocol and admin methods #1646

Add ACL protocol and admin methods #1646

ulrikjohansson commented Nov 17, 2018 •

edited by dpkp

Loading

jeffwidman commented Nov 17, 2018 •

edited

Loading

jeffwidman commented Nov 19, 2018 •

edited

Loading

ulrikjohansson commented Nov 19, 2018 via email •

edited

Loading

ulrikjohansson commented Nov 19, 2018

jeffwidman commented Nov 19, 2018 •

edited

Loading

jeffwidman left a comment

jeffwidman Nov 19, 2018

jeffwidman Nov 19, 2018 •

edited

Loading

jeffwidman Nov 19, 2018

ulrikjohansson Nov 19, 2018

jeffwidman Nov 20, 2018 •

edited

Loading

jeffwidman Nov 19, 2018 •

edited

Loading

jeffwidman Nov 19, 2018 •

edited

Loading

ulrikjohansson commented Nov 19, 2018 •

edited

Loading

jeffwidman commented Nov 20, 2018 •

edited

Loading

ulrikjohansson commented Nov 20, 2018

jeffwidman commented May 29, 2019

ulrikjohansson commented May 29, 2019

ulrikjohansson commented Jun 3, 2019

Add ACL protocol and admin methods #1646

Add ACL protocol and admin methods #1646

Conversation

ulrikjohansson commented Nov 17, 2018 • edited by dpkp Loading

jeffwidman commented Nov 17, 2018 • edited Loading

jeffwidman commented Nov 19, 2018 • edited Loading

ulrikjohansson commented Nov 19, 2018 via email • edited Loading

ulrikjohansson commented Nov 19, 2018

jeffwidman commented Nov 19, 2018 • edited Loading

jeffwidman left a comment

Choose a reason for hiding this comment

jeffwidman Nov 19, 2018

Choose a reason for hiding this comment

jeffwidman Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

jeffwidman Nov 19, 2018

Choose a reason for hiding this comment

ulrikjohansson Nov 19, 2018

Choose a reason for hiding this comment

jeffwidman Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

jeffwidman Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

jeffwidman Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

ulrikjohansson commented Nov 19, 2018 • edited Loading

jeffwidman commented Nov 20, 2018 • edited Loading

ulrikjohansson commented Nov 20, 2018

jeffwidman commented May 29, 2019

ulrikjohansson commented May 29, 2019

ulrikjohansson commented Jun 3, 2019

ulrikjohansson commented Nov 17, 2018 •

edited by dpkp

Loading

jeffwidman commented Nov 17, 2018 •

edited

Loading

jeffwidman commented Nov 19, 2018 •

edited

Loading

ulrikjohansson commented Nov 19, 2018 via email •

edited

Loading

jeffwidman commented Nov 19, 2018 •

edited

Loading

jeffwidman Nov 19, 2018 •

edited

Loading

jeffwidman Nov 20, 2018 •

edited

Loading

jeffwidman Nov 19, 2018 •

edited

Loading

jeffwidman Nov 19, 2018 •

edited

Loading

ulrikjohansson commented Nov 19, 2018 •

edited

Loading

jeffwidman commented Nov 20, 2018 •

edited

Loading