KAFKA-6250: Use existing Kafka Connect internal topics without requiring ACL by gavrie · Pull Request #4247 · apache/kafka

gavrie · 2017-11-22T07:03:05Z

When using Kafka Connect with a cluster that doesn't allow the user to
create topics (due to ACL configuration), Connect fails when trying to
create its internal topics, even if these topics already exist. This is
incorrect behavior according to the documentation, which mentions that
R/W access should be enough.

This happens specifically when using Aiven Kafka, which does not permit
creation of topics via the Kafka Admin Client API.

The patch ignores the returned error, similar to the behavior for older
brokers that don't support the API.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

rhauch

LGTM. Thanks, @gavrie!

@hachikuji, would you mind taking a look at this PR? It'd be great to backport this to 1.0.x and 0.11.x branches as well.

gavrie · 2018-01-05T07:19:22Z

@rhauch: Thank you. It will be great to be able to run an unpatched version.

hachikuji

Thanks for the patch. Left a couple comments. Can we emphasize in the title of this patch that the internal topics we are referring to are Kafka Connect internal topics?

Also, how much trouble would it be to write a test case to exercise this path? Without test cases, we risk regressing this behavior in the future.

hachikuji · 2018-01-05T23:30:16Z

Since we're in here anyway, can we fix the if in the line above to use cause instead of e.getCause()?

hachikuji · 2018-01-05T23:30:42Z

It seems like it would be useful to know in the log message below which case has actually happened. Perhaps we should use a separate if branch with a separate log message for this case?

Very well. I wanted to keep the change minimal, but the suggestion makes sense.

gavrie · 2018-01-09T07:06:15Z

@hachikuji I implemented the changes you suggested, and added a unit test that covers the case.

gavrie · 2018-01-09T13:52:36Z

Is there some problem with Jenkins? I can't get it to build my version, neither on trunk nor on 1.0.

hachikuji

Thanks for the updates and the clean test case! I added two more very small comments.

hachikuji · 2018-01-09T22:25:24Z

nit: looks like we're missing a space after the comma. It was a problem in the original as well.

hachikuji · 2018-01-09T22:27:55Z

Sorry to ask since it is a little orthogonal, but I feel this message is a little vague. Could we just say that the broker does not support the CreateTopics API?

hachikuji · 2018-01-09T23:22:16Z

@gavrie You probably need to rebase to get the jenkins build working.

…ing ACL When using Kafka Connect with a cluster that doesn't allow the user to create topics (due to ACL configuration), Connect fails when trying to create its internal topics even if these topics already exist. This is incorrect behavior according to the documentation, which mentions that R/W access should be enough. This happens specifically when using Aiven Kafka, which does not permit creation of topics via the Kafka Admin Client API. The patch ignores the returned error, similar to the behavior for older brokers that don't support the API.

gavrie · 2018-01-10T16:03:35Z

@hachikuji I think we’re done here, right?

hachikuji

LGTM. Thanks for the patch!

hachikuji · 2018-01-11T23:57:16Z

@gavrie By the way, I added you as a contributor, so you can assign yourself to other jiras in the future.

simplesteph · 2018-01-12T00:19:00Z

+                            topicNameList, bootstrapServers);
+                    return Collections.emptySet();
+                }
+                if (cause instanceof ClusterAuthorizationException) {


sorry, a bit late to the party, but if Kafka Connect can't create the topic but can still read / write to it, it should also have the describe rights. In which case, we can check if the topic exist using a describe?

I feel that right now this might be introducing a bug. Say the Kafka Connect isn't authorized to create a topic and the topic doesn't exist, then it will still go on with the execution

That's a fair point. I didn't think about it, but in fact if the user doesn't have permission to create the topic through the CreateTopic API, then they won't be able to use auto-creation either. I think it would be clearer to check topic existence prior to attempting creation. Then if topic creation fails due to a permission error, perhaps we should make the error fatal.

@rhauch what do you think?

rhauch · 2018-01-12T18:47:19Z

I don't like using describe before create since it's not atomic, and because create-topic will let us know if it already exists. I also don't think that we've introduced a bug, though I can see how we could improve the message and, as @hachikuji mentioned, fail if the topic does not exist and infer that autocreate would fail.

I guess it depends on what the error message actually is in the scenario when the user doesn't have create privilege but can read & write to it, but writing to the topic fails because the topic can't be auto-created. If it's just "unable to create topic", then will we be able to improve on that?

hachikuji · 2018-01-12T22:11:49Z

I'm not sure I understand the point about create being atomic. Can you elaborate? Note that using the admin client to describe the topic will not cause auto-creation to be triggered (the Metadata request has a flag which disables auto-creation).

rhauch · 2018-01-12T23:02:27Z

@hachikuji I was trying to make this point:

If the code first does a describe to see if the topic exists, and discovers it does not exist, then it still has to issue a create-topic request to create the topic. That's 2 requests, and it's possible a different Connect worker jumps in and creates the topic between the two requests.

OTOH, the current code simply issues a single create-topic request that will atomically do one of the following:

create the topic if it is not there; or
return that the topic already existed; or
fail if the topic does not exist but the user doesn't have privilege to create the topic.

So using a discover prior to the create-topic doesn't help and provides no additional clarity. Doing a discover after a failed create-topic might help us understand the particular case so that we can produce a better error message, but we should know what the current error message is before we know whether it will indeed be "better".

@simplesteph said:

I feel that right now this might be introducing a bug. Say the Kafka Connect isn't authorized to create a topic and the topic doesn't exist, then it will still go on with the execution

Yes, while this portion of the code will continue, the producers will definitely fail as soon as they attempt to write to a non-existant topic that won't be auto-created. IOW, it will still fail, just in a different point.

rhauch approved these changes Jan 4, 2018

View reviewed changes

hachikuji reviewed Jan 5, 2018

View reviewed changes

gavrie changed the title ~~KAFKA-6250: Use existing internal topics without requiring ACL~~ KAFKA-6250: Use existing Kafka Connect internal topics without requiring ACL Jan 9, 2018

gavrie force-pushed the trunk branch 2 times, most recently from 36fe8dc to d9aa9a3 Compare January 9, 2018 07:05

gavrie force-pushed the trunk branch from d9aa9a3 to 27b4f55 Compare January 9, 2018 10:56

gavrie changed the base branch from trunk to 1.0 January 9, 2018 13:22

gavrie force-pushed the trunk branch from 27b4f55 to 9673c91 Compare January 9, 2018 13:24

hachikuji reviewed Jan 9, 2018

View reviewed changes

gavrie force-pushed the trunk branch from 9673c91 to 940012d Compare January 10, 2018 12:27

gavrie changed the base branch from 1.0 to trunk January 10, 2018 12:27

hachikuji approved these changes Jan 11, 2018

View reviewed changes

hachikuji merged commit 936e81a into apache:trunk Jan 11, 2018

simplesteph reviewed Jan 12, 2018

View reviewed changes

arabelle mentioned this pull request Nov 15, 2018

KAFKA-7633: Allow Kafka Connect to access internal topics without cluster ACLs #5918

Merged

3 tasks

kkonstantine added the connect label Oct 16, 2020

Conversation

gavrie commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

gavrie commented Jan 5, 2018

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavrie commented Jan 9, 2018

Uh oh!

gavrie commented Jan 9, 2018

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Jan 9, 2018

Uh oh!

gavrie commented Jan 10, 2018

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhauch commented Jan 12, 2018

Uh oh!

hachikuji commented Jan 12, 2018

Uh oh!

rhauch commented Jan 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gavrie commented Nov 22, 2017 •

edited

Loading