Add CreateAcls Admin API support #839
Conversation
| ResourceTypeTopic ResourceType = 2 | ||
| ResourceTypeGroup ResourceType = 3 | ||
| // See https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/config/ConfigResource.java#L36 | ||
| ResourceTypeBroker ResourceType = 4 |
There was a problem hiding this comment.
IMO this should have been under a ConfigResourceType type but instead decided to keep it for backward compatibility.
zirkome
force-pushed
the
add-create-acls
branch
from
8380c8a
to
61d06a7
Compare
|
@rhansen2 Hi I see that you were assigned to the PR. Is there anything I can do to help move this forward? Thank you! |
|
Hi @zirkome, Currently this PR is not passing the CI steps due to failing tests. I think the code looks sane so once the tests are fixed-up I think we're good to move forward. Thanks! |
|
@rhansen2 The tests are not passing because it requires CI changes (i.e. enabling ACL authorizer) which I've made in the PR. But for obvious security reason it doesn't run the tests on this very PR's CI config. Should I make another PR with the CI changes or this is something you have to take care of? |
.circleci/config.yml
Outdated
| @@ -120,6 +120,8 @@ jobs: | |||
| working_directory: *working_directory | |||
| environment: | |||
| KAFKA_VERSION: "2.0.1" | |||
| KAFKA_AUTHORIZER_CLASS_NAME: 'kafka.security.auth.SimpleAclAuthorizer' | |||
There was a problem hiding this comment.
Should these configs be set in the kafka container?
There was a problem hiding this comment.
I see what you mean but all environment are alias from the same yaml anchor and can't be overridden AFAIK. Also depending on the Kafka version KAFKA_AUTHORIZER_CLASS_NAME needs to be different so this also can't be added to that one global environment in kafka-011.
Please advise.
There was a problem hiding this comment.
The Override Values section of this article https://support.atlassian.com/bitbucket-cloud/docs/yaml-anchors/ shows how to extend anchored values. You should be able to to use that syntax to set the authorizer specifically for each version without needed to redefine the entire environment. Lemmie know if that helps thanks!
There was a problem hiding this comment.
@rhansen2 Fixed it using that syntax, I have no way to test if it's correct though.
There was a problem hiding this comment.
You can test by running the circleci jobs locally or by examining the output of the jobs in circleci (I believe you should be able to view the runs for your PR from the details links). It looks like the class name is incorrect for the AclAuthorizer and should be kafka.security.authorizer.AclAuthorizer
There was a problem hiding this comment.
@rhansen2 Indeed good catch on the class name! Will update and see why things fails.
There was a problem hiding this comment.
@rhansen2 Fixed the ACL test as well as it was blocking the other tests. All green now!
zirkome
force-pushed
the
add-create-acls
branch
5 times, most recently
from
b53f5a0
to
dbe1cc4
Compare
This adds a CreateACLs method to the kafka Client to supports the CreateAcls Admin API. Signed-off-by: Guillaume Fillon <guillaume.fillon@auth0.com>
zirkome
force-pushed
the
add-create-acls
branch
from
dbe1cc4
to
27028ec
Compare
|
@rhansen2 Thanks for the help and merging the PR. Would you know when will this PR be released? Does kafka-go have a fixed release cadence? |
|
We don't really have a fixed release cadence. I've cut https://github.com/segmentio/kafka-go/releases/tag/v0.4.29 which contains these changes. |
This adds the
CreateACLsmethod to theClientstruct to supports theCreateAclsAdmin API.See #729
See https://kafka.apache.org/protocol.html#The_Messages_CreateAcls
Signed-off-by: Guillaume Fillon guillaume.fillon@auth0.com