KAFKA-3396 : Unauthorized topics are returned to the user by edoardocomar · Pull Request #1908 · apache/kafka

edoardocomar · 2016-09-26T18:15:52Z

Reopening of #1428

ijuma · 2016-09-26T18:31:36Z

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala

This seems unintentional?

thanks for spotting that shameful oops, @ijuma . full test passes

edoardocomar · 2016-09-27T08:11:13Z

Hi @ijuma all tests seem to have passed (they do locally for me) but the Jenkins build failed

ijuma · 2016-09-27T10:48:35Z

Thanks @edoardocomar, will take another look soon. Can you please add a note to upgrade.html?

ijuma · 2016-09-27T10:55:31Z

core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala

There should be no space before the :. There are a few other cases like that.

ijuma · 2016-09-27T10:59:41Z

core/src/main/scala/kafka/server/KafkaApis.scala

I think the usage of INVALID_TOPIC_EXCEPTION is intentional here. One of the differences is that INVALID_TOPIC_EXCEPTION is not retriable. @granthenke, is that right?

I think changing to a UNKNOWN_TOPIC_OR_PARTITION exception might make sense in this case. Especially if that is the behavior we are using for the other requests where a topic is not authorized and we don't want to leak the information. Consistency is always easier to reason about.

The most important thing is that the exception doesn't give any indication of whether or not the topic exists. Its behavior should the same as if the topic isn't there at all. If this exception is changed, I think the InvalidTopicException in AdminUtils may need to be changed too:
https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/AdminUtils.scala#L328

edoardocomar · 2016-09-27T12:51:24Z

added a note to upgrade.html in the upgrade_1010_notable section

ijuma · 2016-09-27T13:03:26Z

docs/upgrade.html

Closing <\li> missing

hachikuji

Left a couple more comments. It's looking pretty good on the server side. However, I still do not see any checks on the client side in the offset fetch and offset commit response handlers for the UNKNOWN_TOPIC_OR_PARTITION error code. I feel it would also be helpful when we catch this error explicitly and retry internally to log a message which mentions that the user may not have Describe access. Is there any reason we shouldn't do this?

hachikuji · 2016-09-28T03:56:34Z

core/src/main/scala/kafka/server/KafkaApis.scala

Would it be easier to handle the logging for this case in the initial call to partition above? May as well save the redundant cache lookup and it seems possible that the cache may have changed in between the first check and this one. Another minor point: is it worthwhile checking if DEBUG is enabled prior to doing this loop?

done thanks

hachikuji · 2016-09-28T04:33:06Z

core/src/main/scala/kafka/server/KafkaApis.scala

For discussion: Since Write access subsumes Describe, I'm wondering if it would make sense to change the order of the checks to something like this:

Check for Write access on all topics.

For those topics without Write access:

If no Describe access or not exists -> UNKNOWN_TOPIC_OR_PARTITION

Otherwise -> TOPIC_AUTHORIZATION_FAILED

The intuition is that failing Write should be the exceptional case. The one annoying thing is that you also need to check existence for topics which are authorized for Write. The fetch request handler could be done similarly since Read access also subsumes Describe.

hachikuji · 2016-09-28T04:47:01Z

core/src/main/scala/kafka/server/KafkaApis.scala

@ijuma Is it correct that we return TOPIC_AUTHORIZATION_FAILED here? Whether or not it's ideal, the authorization check is on the Cluster resource currently.

That's a bit weird, isn't it? In a KIP discussion, we had said that we wanted to change it so that one would require a Create Topic instead of Create Cluster ACL for topic creation (would probably need both for compatibility), so it seems reasonable to keep throwing a TOPIC_AUTHORIZATION_FAILED here.

Ack. I agree that this is probably better long term, even if it's a little misleading at the moment.

edoardocomar · 2016-09-28T15:40:15Z

Handling UTOP in consumerCoordinator handleResponse for fetchOffset and commitOffset, with more descriptive msg

edoardocomar · 2016-09-29T13:00:48Z

rebased/merged and fixed spurious test failure

edoardocomar · 2016-09-29T17:36:10Z

@ijuma the Jenkins build failed to complete again. However all tests pass locally for me

hachikuji · 2016-09-30T18:11:02Z

I've been running system tests against this branch. There were a couple test failures, but I think they are known transient failures. I'm running again to be sure. In the meantime, I'll do another pass over the code and hopefully we can merge this today.

Modified KafkaApis to return Errors.UNKNOWN_TOPIC_OR_PARTITION if principal has no Describe access to topic Unit tests expanded Some paths cause the client to block due to bug https://issues.apache.org/jira/browse/KAFKA-3727?filter=-2 tests work around this by executing in separate thread

- Added tests from 44ad3ec - Small refactorings

Rebased after kip-79 changes. Fixing leak of topic for LIST_OFFSETS when unauthorized. Added tests.

cleanup after review

Cleanup addressing comments

Revised handling of FETCH PRODUCE and DELETE requests

oops had commented out part os a test by mistake

addressing comments: code formatting, not in upgrade.html

addressing comments: missing </li>

addressing comments: handling UTOP in ConsumerCoordinator offset fetch/commit improved logging efficiency on the server

test cleanup and fixing intermittent failure

throwing UTOP also form AdminUtils.delete

edoardocomar · 2016-09-30T21:27:16Z

@hachikuji I rebased and rerun all the tests locally on my mac and they pass.

hachikuji · 2016-10-01T06:00:59Z

LGTM. Merging to trunk and 0.10.1. Thanks @edoardocomar and @mimaison for the hard work (and patience) on this patch! I have some minor cleanups/improvements on the client and in testing, which I will submit in a follow-up PR.

Reopening of #1428 Author: Edoardo Comar <ecomar@uk.ibm.com> Author: Mickael Maison <mickael.maison@gmail.com> Reviewers: Grant Henke <granthenke@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io> Closes #1908 from edoardocomar/KAFKA-3396 (cherry picked from commit 8124f6e) Signed-off-by: Jason Gustafson <jason@confluent.io>

hachikuji · 2016-10-01T06:09:58Z

By the way, I changed the title. Hopefully it's reasonable.

…ame broker (apache#1908)

ijuma reviewed Sep 26, 2016

View reviewed changes

ijuma reviewed Sep 27, 2016

View reviewed changes

hachikuji reviewed Sep 28, 2016

View reviewed changes

edoardocomar changed the title ~~Kafka 3396~~ KAFKA-3396 : Unauthorized topics are returned to the user Sep 28, 2016

edoardocomar force-pushed the KAFKA-3396 branch from d18fbe1 to 5e92586 Compare September 29, 2016 12:59

edoardocomar and others added 14 commits September 30, 2016 22:03

KAFKA-3396: Updates and cleanups following the feedback

9ebc96b

KAFKA-3396: More updates + rebased off trunk

d47bac1

- Added tests from 44ad3ec - Small refactorings

KAFKA-3396 : Unauthorized topics are returned to the user

f69e0aa

Rebased after kip-79 changes. Fixing leak of topic for LIST_OFFSETS when unauthorized. Added tests.

KAFKA-3396 : Unauthorized topics are returned to the user

0ba2d79

cleanup after review

KAFKA-3396 : Unauthorized topics are returned to the user

6c7e51f

Cleanup addressing comments

KAFKA-3396 : Unauthorized topics are returned to the user

8d4f7f7

Cleanup addressing comments

KAFKA-3396 : Unauthorized topics are returned to the user

7a8393c

Revised handling of FETCH PRODUCE and DELETE requests

KAFKA-3396 : Unauthorized topics are returned to the user

94b5f5c

oops had commented out part os a test by mistake

KAFKA-3396 : Unauthorized topics are returned to the user

6e51c47

addressing comments: code formatting, not in upgrade.html

KAFKA-3396 : Unauthorized topics are returned to the user

4bd02a1

addressing comments: missing </li>

KAFKA-3396 : Unauthorized topics are returned to the user

5ee7bd1

addressing comments: handling UTOP in ConsumerCoordinator offset fetch/commit improved logging efficiency on the server

KAFKA-3396 : Unauthorized topics are returned to the user

376a5a7

test cleanup and fixing intermittent failure

KAFKA-3396 : Unauthorized topics are returned to the user

52232c9

throwing UTOP also form AdminUtils.delete

edoardocomar force-pushed the KAFKA-3396 branch from 15d1f23 to 52232c9 Compare September 30, 2016 21:27

asfgit closed this in 8124f6e Oct 1, 2016

edoardocomar deleted the KAFKA-3396 branch June 22, 2017 12:11

efeg pushed a commit to efeg/kafka that referenced this pull request May 29, 2024

Add endpoint to move all replicas from a disk to other disks of the s…

2af7b93

…ame broker (apache#1908)

Conversation

edoardocomar commented Sep 26, 2016

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!

edoardocomar commented Sep 27, 2016

Uh oh!

ijuma commented Sep 27, 2016

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!

edoardocomar commented Sep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoardocomar commented Sep 28, 2016

Uh oh!

edoardocomar commented Sep 29, 2016

Uh oh!

edoardocomar commented Sep 29, 2016

Uh oh!

hachikuji commented Sep 30, 2016

Uh oh!

edoardocomar commented Sep 30, 2016

Uh oh!

hachikuji commented Oct 1, 2016

Uh oh!

hachikuji commented Oct 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

hachikuji Sep 28, 2016 •

edited

Loading

hachikuji Sep 28, 2016 •

edited

Loading

hachikuji Sep 28, 2016 •

edited

Loading

ijuma Sep 28, 2016 •

edited

Loading