KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG by TaiJuWu · Pull Request #18432 · apache/kafka

TaiJuWu · 2025-01-08T05:43:48Z

*More detailed description of your change,

Remove ALTER_CONFIG zk part
Remove INCREMENETAL_ALTER_CONFIG zk part

Committer Checklist (excluded from commit message)

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

TaiJuWu · 2025-01-08T05:49:40Z

  }

-  @Test
-  def testAlterConfigsWithAuthorizer(): Unit = {


ZK only test.

For my understanding, this is a ZK-only test because authorisation should no longer happen on a broker because the requests are forwarded to the KRaft controller where authorisation ought to happen?

yes, we don't don any authorization on Kraft mode.

You still need to do some level of authorization before forwarding, right?

You still need to do some level of authorization before forwarding

You are absolutely correct. The log4j operation is not forwarded to the controller because it is used to configure "this" specific broker. Therefore, authorization for this operation must be performed within this broker.

Please ensure that the related unit tests are maintained.

Thanks for the quick response! I understand where you are coming from with respect to the particular operation we are authorizing, what I do not understand is why are we removing the double authorisation (once on the broker and once on the controller)? I also believe this is what Ismael meant as well here #18432 (comment)

Oh, I got your point but this should be another issue and we can make it as follow ups.
We should not block it here since this ticket is for 4.0 and we are closer deadline and it more like an optimization.

This is a mistake see #18432 (comment)

By the way, here is a related response #18432 (comment) for #18432 (comment) and I believe it is an accident.

The question should be we should authorize for IncrementalAlterConfig instead of AlterConfig

but this should be another issue and we can make it as follow ups.

Pardon me, what is "another issue"? There is no extra authorisation for AlterConfigsRequest as it does not support to change logger. It seems to me the follow-up for this unit test is to add checks for configManager.preprocess

but this should be another issue and we can make it as follow ups.

Pardon me, what is "another issue"? There is no extra authorisation for AlterConfigsRequest as it does not support to change logger. It seems to me the follow-up for this unit test is to add checks for configManager.preprocess

Oh, sorry. I think this again.
All alterConfig (no matter incremental or not ) should forward to controller directly so there is no follow up.
Previous image (from mine), we can do some authorize locally but no matter what we always need to send to controller.
There is no optimism space.

ijuma · 2025-01-10T14:48:00Z

        response => sendResponse(response.map(_.data())))
    } else {
-      sendResponse(Some(processLegacyAlterConfigsRequest(request, remaining)))
+      throw KafkaApis.shouldAlwaysForward(request)


How could this happen in kraft mode? Do we need to change some code to make it impossible or is it already impossible? I'd rather do this in a generic way versus having each handler having to handle it.

metadataSupport.canForward() is always true in kraft and request does not have envelope in kraft broker. Hence, it would be better to simplify the code by removing the unnecessary path. for example:

if (remaining.resources().isEmpty) { sendResponse(Some(new AlterConfigsResponseData())) } else { metadataSupport.forwardingManager.get.forwardRequest(request, new AlterConfigsRequest(remaining, request.header.apiVersion()), response => sendResponse(response.map(_.data()))) }

BTW, I file https://issues.apache.org/jira/browse/KAFKA-18472 to cleanup metadataSupport

clolov

Did a first pass and left a couple of small questions for my understanding

clolov · 2025-01-10T16:05:11Z

  }

-  @Test
-  def testAlterConfigsWithAuthorizer(): Unit = {


For my understanding, this is a ZK-only test because authorisation should no longer happen on a broker because the requests are forwarded to the KRaft controller where authorisation ought to happen?

clolov · 2025-01-10T16:09:39Z

  def handleIncrementalAlterConfigsRequest(request: RequestChannel.Request): Unit = {
    val original = request.body[IncrementalAlterConfigsRequest]
    val preprocessingResponses = configManager.preprocess(original.data(),
      (rType, rName) => authHelper.authorize(request.context, ALTER_CONFIGS, rType, rName))


For my understanding (and here I am working under the assumption that these requests ought to be authorised on the KRaft controller now), should this be removed as well?

when we set broker config (only for LOG4J), we also need to do authorization locally so we need to keep this one.

Got it, thank you!

chia7712

@TaiJuWu thanks for this patch

chia7712 · 2025-01-10T17:49:42Z

  }

-  @Test
-  def testAlterConfigsWithAuthorizer(): Unit = {


You still need to do some level of authorization before forwarding

You are absolutely correct. The log4j operation is not forwarded to the controller because it is used to configure "this" specific broker. Therefore, authorization for this operation must be performed within this broker.

Please ensure that the related unit tests are maintained.

chia7712 · 2025-01-10T18:40:45Z

+    val forwardCallback: ArgumentCaptor[Option[AbstractResponse] => Unit] = ArgumentCaptor.forClass(classOf[Option[AbstractResponse] => Unit])
+    val newBody: ArgumentCaptor[AbstractRequest] = ArgumentCaptor.forClass(classOf[AbstractRequest])
+
+    verify(forwardingManager).forwardRequest(


all we want to verify is the method forwardRequest gets executed only once, right? the following assert is unnecessary as they never be null.

chia7712

@TaiJuWu thanks for this patch. one small comment is left.

chia7712 · 2025-01-13T18:56:14Z

    val requestHeader = new RequestHeader(ApiKeys.ALTER_CONFIGS, ApiKeys.ALTER_CONFIGS.latestVersion, clientId, 0)
-    val request = buildRequest(
-      new AlterConfigsRequest.Builder(configs.asJava, false).build(requestHeader.apiVersion))
+    val apiRequest = new AlterConfigsRequest.Builder(configs.asJava, false).build(requestHeader.apiVersion)


in this testAlterConfigsClientMetrics, we don't need the authorizer. Hence, could you please remove the unused variable?

Done. Thanks for review!

chia7712 · 2025-01-15T10:25:10Z

the failed tests are in clients module, so I think they are unrelated to this PR

…INCREMENETAL_ALTER_CONFIG (#18432) Reviewers: Christo Lolov <lolovc@amazon.com>, Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

…INCREMENETAL_ALTER_CONFIG (apache#18432) Reviewers: Christo Lolov <lolovc@amazon.com>, Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

github-actions Bot added triage PRs from the community core Kafka Broker labels Jan 8, 2025

TaiJuWu commented Jan 8, 2025

View reviewed changes

Comment thread core/src/test/scala/unit/kafka/server/KafkaApisTest.scala Outdated

TaiJuWu marked this pull request as ready for review January 8, 2025 05:58

chia7712 added the ci-approved label Jan 8, 2025

TaiJuWu marked this pull request as draft January 8, 2025 12:25

TaiJuWu changed the title ~~KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3):INIT_PRODUCER_ID, ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG~~ KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG Jan 8, 2025

remove alterConfig and IncrementalAlterConfig

7643d68

TaiJuWu force-pushed the rm_zk-2 branch from d1b23e7 to 7643d68 Compare January 8, 2025 12:45

remove incrementalAlterConfigWithAuthorizer

14ade5c

TaiJuWu marked this pull request as ready for review January 8, 2025 18:32

TaiJuWu commented Jan 8, 2025

View reviewed changes

Comment thread core/src/test/scala/unit/kafka/server/KafkaApisTest.scala

github-actions Bot removed the triage PRs from the community label Jan 9, 2025

TaiJuWu changed the title ~~KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG~~ KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG Jan 9, 2025

Merge branch 'trunk' into rm_zk-2

7ac5d9a

ijuma reviewed Jan 10, 2025

View reviewed changes

clolov reviewed Jan 10, 2025

View reviewed changes

address comments

616bc61

chia7712 reviewed Jan 10, 2025

View reviewed changes

address comments

da9e667

TaiJuWu force-pushed the rm_zk-2 branch from ec7ff25 to da9e667 Compare January 10, 2025 18:09

chia7712 reviewed Jan 10, 2025

View reviewed changes

TaiJuWu added 7 commits January 11, 2025 18:07

authorize

02805f3

fix verify

1519379

forward + authorization

bbb41aa

fix zk issue

7e8213b

Merge branch 'trunk' into rm_zk-2

556fce5

Merge branch 'trunk' into rm_zk-2

0740134

Merge branch 'trunk' into rm_zk-2

219347c

chia7712 reviewed Jan 13, 2025

View reviewed changes

Merge branch 'trunk' into rm_zk-2

f17aba8

TaiJuWu mentioned this pull request Jan 15, 2025

KAFKA-18399 Remove ZooKeeper from KafkaApis (12/N): clean up ZKMetadataCache, KafkaController and raftSupport #18542

Merged

3 tasks

chia7712 approved these changes Jan 15, 2025

View reviewed changes

chia7712 merged commit 3fcaec7 into apache:trunk Jan 15, 2025

TaiJuWu deleted the rm_zk-2 branch January 15, 2025 11:40

Conversation

TaiJuWu commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

Uh oh!

Uh oh!

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!

ijuma Jan 10, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

TaiJuWu Jan 13, 2025 • 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!

TaiJuWu Jan 13, 2025 • 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!

TaiJuWu Jan 13, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clolov 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!

TaiJuWu Jan 10, 2025 • 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!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 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!

chia7712 commented Jan 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

TaiJuWu commented Jan 8, 2025 •

edited

Loading

ijuma Jan 10, 2025 •

edited

Loading

TaiJuWu Jan 13, 2025 •

edited

Loading

TaiJuWu Jan 13, 2025 •

edited

Loading

TaiJuWu Jan 13, 2025 •

edited

Loading

TaiJuWu Jan 10, 2025 •

edited

Loading