You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "TaiJuWu (via GitHub)" <gi...@apache.org> on 2025/01/08 05:43:51 UTC
[PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3):INIT_PRODUCER_ID, ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
TaiJuWu opened a new pull request, #18432:
URL: https://github.com/apache/kafka/pull/18432
*More detailed description of your change,
- Remove INIT_PRODUCER_ID since it not support by kafkaApi
- 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913119176
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
By the way, here is a related response https://github.com/apache/kafka/pull/18432#discussion_r1910905034 for https://github.com/apache/kafka/pull/18432#discussion_r1910638103 and I believe it is an accident.
The question should be we should be authorize for `IncrementalAlterConfig` instead of `AlterConfig`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3):INIT_PRODUCER_ID, ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1906520376
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -850,6 +720,7 @@ class KafkaApisTest extends Logging {
(authorizedResource, unauthorizedResource)
}
+ @Disabled
Review Comment:
I don't have easy way to fix so temporarily disable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913119176
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
By the way, here is a related response https://github.com/apache/kafka/pull/18432#discussion_r1910905034 for https://github.com/apache/kafka/pull/18432#discussion_r1910638103 and I believe it is an accident.
The question should be we should authorize for `IncrementalAlterConfig` instead of `AlterConfig`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910777056
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
From my understanding, the authorization only work for `IncrementalAlterConfig`.
`AlterConfig` is legacy api and we don't do any authorization here.
You can refer [here](https://github.com/apache/kafka/blob/616bc610d075b87a6ac273d052eae9ce23bf466c/core/src/main/scala/kafka/server/KafkaApis.scala#L2504-L2525)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913065969
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> we used to do authorisation in two places (once on the broker and once at the controller)
Sorry for unclarification, for `IncrementalAlterConfig` we authorize `BORKER_LOGGER` only [1] and it apply to local (it does not send to controller) , and we do other authorization on controller.
`BORKER_LOGGER` does not support for `AlterConfig` [2] so we don't do any authorization here.
[1] https://github.com/apache/kafka/blob/bbdf79e1b4ffd4960f42ec128edbced11349c538/core/src/main/scala/kafka/server/ConfigAdminManager.scala#L137-L155
[2] https://github.com/apache/kafka/blob/cbbeccad632d37c131bfa494fdb2eabc5df7610d/core/src/main/scala/kafka/server/KafkaApis.scala#L2385-L2386
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913065969
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> we used to do authorisation in two places (once on the broker and once at the controller)
Sorry for unclarification, for `IncrementalAlterConfig` we authorize `BORKER_LOGGER` only [1] and it apply to local (it does not send to controller) , and we do other authorization on controller but it does not support for ZK mode [2].
[1] https://github.com/apache/kafka/blob/bbdf79e1b4ffd4960f42ec128edbced11349c538/core/src/main/scala/kafka/server/ConfigAdminManager.scala#L137-L155
[2] https://github.com/apache/kafka/blob/cbbeccad632d37c131bfa494fdb2eabc5df7610d/core/src/main/scala/kafka/server/KafkaApis.scala#L2385-L2386
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913025607
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
Apologies if I am missing something obvious, but here is my reasoning. If we keep authorization in IncrementalAlterConfigs before forwarding to the KRaft controller, then we ought to keep authorization in the AlterConfigs as well, regardless of whether the API is deprecated or not. As far as I can see, `processLegacyAlterConfigsRequest` used to carry out the authorisation w.r.t AlterConfigs. Am I wrong in my reading of the code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913065969
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> we used to do authorisation in two places (once on the broker and once at the controller)
Sorry for unclarification, for `IncrementalAlterConfig` we authorize `BORKER_LOGGER` only [1] and it apply to local (it does not send to controller) , and we do other authorization on controller.
and `BORKER_LOGGER` does not support for `AlterConfig` .
[1] https://github.com/apache/kafka/blob/bbdf79e1b4ffd4960f42ec128edbced11349c538/core/src/main/scala/kafka/server/ConfigAdminManager.scala#L137-L155
[2] https://github.com/apache/kafka/blob/cbbeccad632d37c131bfa494fdb2eabc5df7610d/core/src/main/scala/kafka/server/KafkaApis.scala#L2385-L2386
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913065969
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> we used to do authorisation in two places (once on the broker and once at the controller)
Sorry for unclarification, for `IncrementalAlterConfig` we authorize `BORKER_LOGGER` only [1] and it apply to local (it does not send to controller) , and we do other authorization on controller.
But `BORKER_LOGGER ` does not support for ZK mode [2].
[1] https://github.com/apache/kafka/blob/bbdf79e1b4ffd4960f42ec128edbced11349c538/core/src/main/scala/kafka/server/ConfigAdminManager.scala#L137-L155
[2] https://github.com/apache/kafka/blob/cbbeccad632d37c131bfa494fdb2eabc5df7610d/core/src/main/scala/kafka/server/KafkaApis.scala#L2385-L2386
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910638103
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
You still need to do some level of authorization before forwarding, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1912278967
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
All comments are addressed, PTAL
`testIncrementalAlterConfigsWithAuthorizer` cover such case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910508736
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2525,42 +2522,10 @@ class KafkaApis(val requestChannel: RequestChannel,
new AlterConfigsRequest(remaining, request.header.apiVersion()),
response => sendResponse(response.map(_.data())))
} else {
- sendResponse(Some(processLegacyAlterConfigsRequest(request, remaining)))
+ throw KafkaApis.shouldAlwaysForward(request)
Review Comment:
`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:
```java
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910905034
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
@TaiJuWu l left my comment on the incorrect conversation. My point is that we need the test which check the permission of handling `IncrementalAlterConfig`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18432:
URL: https://github.com/apache/kafka/pull/18432#issuecomment-2592249364
the failed tests are in clients module, so I think they are unrelated to this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #18432:
URL: https://github.com/apache/kafka/pull/18432
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913134917
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> > 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 here but no matter what we always need to send to controller.
There is no optimism space.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3):INIT_PRODUCER_ID, ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1906514697
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -630,51 +514,31 @@ class KafkaApisTest extends Logging {
val configs = Map(authorizedResource -> new AlterConfigsRequest.Config(configEntries))
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)
+ val request = buildRequest(apiRequest)
- when(controller.isActive).thenReturn(false)
- when(clientRequestQuotaManager.maybeRecordAndGetThrottleTimeMs(any[RequestChannel.Request](),
- any[Long])).thenReturn(0)
- when(adminManager.alterConfigs(any(), ArgumentMatchers.eq(false)))
- .thenReturn(Map(authorizedResource -> ApiError.NONE))
- kafkaApis = createKafkaApis(authorizer = Some(authorizer))
+ metadataCache = MetadataCache.kRaftMetadataCache(brokerId, () => KRaftVersion.LATEST_PRODUCTION)
+ kafkaApis = createKafkaApis(raftSupport = true)
kafkaApis.handleAlterConfigsRequest(request)
- val response = verifyNoThrottling[AlterConfigsResponse](request)
- verifyAlterConfigResult(response, Map(subscriptionName -> Errors.NONE))
- verify(authorizer, times(1)).authorize(any(), any())
- verify(adminManager).alterConfigs(any(), anyBoolean())
+ testNewBodyForwardableApi(request)
}
@Test
def testIncrementalClientMetricAlterConfigs(): Unit = {
Review Comment:
In Kraft mode, this request is redirected to controller.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910626658
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2647,31 +2612,12 @@ class KafkaApis(val requestChannel: RequestChannel,
zkSupport.controller.listPartitionReassignments(partitionsOpt, sendResponseCallback)
}
- private def configsAuthorizationApiError(resource: ConfigResource): ApiError = {
- val error = resource.`type` match {
- case ConfigResource.Type.BROKER | ConfigResource.Type.BROKER_LOGGER => Errors.CLUSTER_AUTHORIZATION_FAILED
- case ConfigResource.Type.TOPIC => Errors.TOPIC_AUTHORIZATION_FAILED
- case ConfigResource.Type.GROUP => Errors.GROUP_AUTHORIZATION_FAILED
- case rt => throw new InvalidRequestException(s"Unexpected resource type $rt for resource ${resource.name}")
- }
- new ApiError(error, null)
- }
-
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))
Review Comment:
when we set broker config (only for LOG4J), we also need to do authorization locally so we need to keep this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913134917
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910917574
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -598,6 +531,21 @@ class KafkaApisTest extends Logging {
)
}
+ private def testNewBodyForwardableApi(request: RequestChannel.Request
+ ): Unit = {
+ val forwardCallback: ArgumentCaptor[Option[AbstractResponse] => Unit] = ArgumentCaptor.forClass(classOf[Option[AbstractResponse] => Unit])
+ val newBody: ArgumentCaptor[AbstractRequest] = ArgumentCaptor.forClass(classOf[AbstractRequest])
+
+ verify(forwardingManager).forwardRequest(
Review Comment:
all we want to verify is the method `forwardRequest` gets executed only once, right? the following assert is unnecessary as they never be null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913106963
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review 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 optimization.
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review 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 an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1915718054
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -466,51 +423,39 @@ class KafkaApisTest extends Logging {
val configs = Map(authorizedResource -> new AlterConfigsRequest.Config(configEntries))
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)
Review Comment:
Done. Thanks for review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3):INIT_PRODUCER_ID, ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1906509319
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -324,79 +324,6 @@ class KafkaApisTest extends Logging {
assertEquals(propValue, describeConfigsResponseData.value)
}
- @Test
- def testEnvelopeRequestHandlingAsController(): Unit = {
Review Comment:
This block remove Envelop PRC and related https://github.com/apache/kafka/pull/18422
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2845,49 +2739,15 @@ class KafkaApis(val requestChannel: RequestChannel,
}
}
- // Forwarding has not happened yet, so handle both ZK and KRaft cases here
if (remaining.resources().isEmpty) {
sendResponse(Some(new IncrementalAlterConfigsResponseData()))
- } else if ((!request.isForwarded) && metadataSupport.canForward() && isKRaftController) {
+ } else if ((!request.isForwarded) && metadataSupport.canForward()) {
metadataSupport.forwardingManager.get.forwardRequest(request,
new IncrementalAlterConfigsRequest(remaining, request.header.apiVersion()),
response => sendResponse(response.map(_.data())))
} else {
- sendResponse(Some(processIncrementalAlterConfigsRequest(request, remaining)))
- }
- }
-
- def processIncrementalAlterConfigsRequest(
- originalRequest: RequestChannel.Request,
- data: IncrementalAlterConfigsRequestData
- ): IncrementalAlterConfigsResponseData = {
- val zkSupport = metadataSupport.requireZkOrThrow(KafkaApis.shouldAlwaysForward(originalRequest))
Review Comment:
ZK related.
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2686,42 +2631,10 @@ class KafkaApis(val requestChannel: RequestChannel,
new AlterConfigsRequest(remaining, request.header.apiVersion()),
response => sendResponse(response.map(_.data())))
} else {
- sendResponse(Some(processLegacyAlterConfigsRequest(request, remaining)))
+ throw KafkaApis.shouldAlwaysForward(request)
}
}
- def processLegacyAlterConfigsRequest(
- originalRequest: RequestChannel.Request,
- data: AlterConfigsRequestData
- ): AlterConfigsResponseData = {
- val zkSupport = metadataSupport.requireZkOrThrow(KafkaApis.shouldAlwaysForward(originalRequest))
Review Comment:
ZK related.
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -630,51 +514,31 @@ class KafkaApisTest extends Logging {
val configs = Map(authorizedResource -> new AlterConfigsRequest.Config(configEntries))
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)
+ val request = buildRequest(apiRequest)
- when(controller.isActive).thenReturn(false)
- when(clientRequestQuotaManager.maybeRecordAndGetThrottleTimeMs(any[RequestChannel.Request](),
- any[Long])).thenReturn(0)
- when(adminManager.alterConfigs(any(), ArgumentMatchers.eq(false)))
- .thenReturn(Map(authorizedResource -> ApiError.NONE))
- kafkaApis = createKafkaApis(authorizer = Some(authorizer))
+ metadataCache = MetadataCache.kRaftMetadataCache(brokerId, () => KRaftVersion.LATEST_PRODUCTION)
+ kafkaApis = createKafkaApis(raftSupport = true)
kafkaApis.handleAlterConfigsRequest(request)
- val response = verifyNoThrottling[AlterConfigsResponse](request)
- verifyAlterConfigResult(response, Map(subscriptionName -> Errors.NONE))
- verify(authorizer, times(1)).authorize(any(), any())
- verify(adminManager).alterConfigs(any(), anyBoolean())
+ testNewBodyForwardableApi(request)
}
@Test
def testIncrementalClientMetricAlterConfigs(): Unit = {
Review Comment:
ditto
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -630,51 +514,31 @@ class KafkaApisTest extends Logging {
val configs = Map(authorizedResource -> new AlterConfigsRequest.Config(configEntries))
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)
+ val request = buildRequest(apiRequest)
- when(controller.isActive).thenReturn(false)
- when(clientRequestQuotaManager.maybeRecordAndGetThrottleTimeMs(any[RequestChannel.Request](),
- any[Long])).thenReturn(0)
- when(adminManager.alterConfigs(any(), ArgumentMatchers.eq(false)))
- .thenReturn(Map(authorizedResource -> ApiError.NONE))
- kafkaApis = createKafkaApis(authorizer = Some(authorizer))
+ metadataCache = MetadataCache.kRaftMetadataCache(brokerId, () => KRaftVersion.LATEST_PRODUCTION)
+ kafkaApis = createKafkaApis(raftSupport = true)
kafkaApis.handleAlterConfigsRequest(request)
- val response = verifyNoThrottling[AlterConfigsResponse](request)
- verifyAlterConfigResult(response, Map(subscriptionName -> Errors.NONE))
- verify(authorizer, times(1)).authorize(any(), any())
- verify(adminManager).alterConfigs(any(), anyBoolean())
+ testNewBodyForwardableApi(request)
Review Comment:
In Kraft mode, this request is redirected to controller.
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
ZK only test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1907659450
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -830,78 +705,6 @@ class KafkaApisTest extends Logging {
.thenReturn(Seq(result).asJava)
}
- private def verifyAlterConfigResult(response: AlterConfigsResponse,
- expectedResults: Map[String, Errors]): Unit = {
- val responseMap = response.data.responses().asScala.map { resourceResponse =>
- resourceResponse.resourceName -> Errors.forCode(resourceResponse.errorCode)
- }.toMap
-
- assertEquals(expectedResults, responseMap)
- }
-
- private def createConfigsWithAuthorization(authorizer: Authorizer,
- authorizedTopic: String,
- unauthorizedTopic: String): (ConfigResource, ConfigResource) = {
- val authorizedResource = new ConfigResource(ConfigResource.Type.TOPIC, authorizedTopic)
-
- val unauthorizedResource = new ConfigResource(ConfigResource.Type.TOPIC, unauthorizedTopic)
-
- createTopicAuthorization(authorizer, AclOperation.ALTER_CONFIGS, authorizedTopic, unauthorizedTopic)
- (authorizedResource, unauthorizedResource)
- }
-
- @Test
- def testIncrementalAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
This test only verify log4j config so we can delete it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910638103
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
You still need to authorize before forwarding, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910623609
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
yes, we don't don any authorization on Kraft mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913088814
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
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 https://github.com/apache/kafka/pull/18432#discussion_r1910638103
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913106963
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1912278967
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
All comments are addressed, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913670616
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -466,51 +423,39 @@ class KafkaApisTest extends Logging {
val configs = Map(authorizedResource -> new AlterConfigsRequest.Config(configEntries))
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)
Review Comment:
in this `testAlterConfigsClientMetrics`, we don't need the `authorizer`. Hence, could you please remove the unused variable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910661898
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2647,31 +2612,12 @@ class KafkaApis(val requestChannel: RequestChannel,
zkSupport.controller.listPartitionReassignments(partitionsOpt, sendResponseCallback)
}
- private def configsAuthorizationApiError(resource: ConfigResource): ApiError = {
- val error = resource.`type` match {
- case ConfigResource.Type.BROKER | ConfigResource.Type.BROKER_LOGGER => Errors.CLUSTER_AUTHORIZATION_FAILED
- case ConfigResource.Type.TOPIC => Errors.TOPIC_AUTHORIZATION_FAILED
- case ConfigResource.Type.GROUP => Errors.GROUP_AUTHORIZATION_FAILED
- case rt => throw new InvalidRequestException(s"Unexpected resource type $rt for resource ${resource.name}")
- }
- new ApiError(error, null)
- }
-
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))
Review Comment:
Got it, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913029564
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
Or to phrase it differently, I am unclear as to why we used to do authorisation in two places (once on the broker and once at the controller) and now we are doing only one authorisation (only on the controller). We used to do double authorisation even in Zookeeper-world, no? In other words, where we are forwarding the request to is (somewhat) orthogonal to how many times we authorise
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910594225
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2647,31 +2612,12 @@ class KafkaApis(val requestChannel: RequestChannel,
zkSupport.controller.listPartitionReassignments(partitionsOpt, sendResponseCallback)
}
- private def configsAuthorizationApiError(resource: ConfigResource): ApiError = {
- val error = resource.`type` match {
- case ConfigResource.Type.BROKER | ConfigResource.Type.BROKER_LOGGER => Errors.CLUSTER_AUTHORIZATION_FAILED
- case ConfigResource.Type.TOPIC => Errors.TOPIC_AUTHORIZATION_FAILED
- case ConfigResource.Type.GROUP => Errors.GROUP_AUTHORIZATION_FAILED
- case rt => throw new InvalidRequestException(s"Unexpected resource type $rt for resource ${resource.name}")
- }
- new ApiError(error, null)
- }
-
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))
Review Comment:
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?
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913119176
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
By the way, here is a related response https://github.com/apache/kafka/pull/18432#discussion_r1910905034 for https://github.com/apache/kafka/pull/18432#discussion_r1910638103 and I believe it is a accident.
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
By the way, here is a related response https://github.com/apache/kafka/pull/18432#discussion_r1910905034 for https://github.com/apache/kafka/pull/18432#discussion_r1910638103 and I believe it is an accident.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913065969
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> we used to do authorisation in two places (once on the broker and once at the controller)
Sorry for unclarification, for `IncrementalAlterConfig` we authorize `BORKER_LOGGER` only [1], which apply to locally and it does not send to controller, other requests we do other authorization on controller.
`BORKER_LOGGER` does not support for `AlterConfig` [2] so we don't do any authorization here.
[1] https://github.com/apache/kafka/blob/bbdf79e1b4ffd4960f42ec128edbced11349c538/core/src/main/scala/kafka/server/ConfigAdminManager.scala#L137-L155
[2] https://github.com/apache/kafka/blob/cbbeccad632d37c131bfa494fdb2eabc5df7610d/core/src/main/scala/kafka/server/KafkaApis.scala#L2385-L2386
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910478175
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2525,42 +2522,10 @@ class KafkaApis(val requestChannel: RequestChannel,
new AlterConfigsRequest(remaining, request.header.apiVersion()),
response => sendResponse(response.map(_.data())))
} else {
- sendResponse(Some(processLegacyAlterConfigsRequest(request, remaining)))
+ throw KafkaApis.shouldAlwaysForward(request)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910626658
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2647,31 +2612,12 @@ class KafkaApis(val requestChannel: RequestChannel,
zkSupport.controller.listPartitionReassignments(partitionsOpt, sendResponseCallback)
}
- private def configsAuthorizationApiError(resource: ConfigResource): ApiError = {
- val error = resource.`type` match {
- case ConfigResource.Type.BROKER | ConfigResource.Type.BROKER_LOGGER => Errors.CLUSTER_AUTHORIZATION_FAILED
- case ConfigResource.Type.TOPIC => Errors.TOPIC_AUTHORIZATION_FAILED
- case ConfigResource.Type.GROUP => Errors.GROUP_AUTHORIZATION_FAILED
- case rt => throw new InvalidRequestException(s"Unexpected resource type $rt for resource ${resource.name}")
- }
- new ApiError(error, null)
- }
-
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))
Review Comment:
when we set broker config (only for LOG4J), we also need to do authorization so we need to keep this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1907659450
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -830,78 +705,6 @@ class KafkaApisTest extends Logging {
.thenReturn(Seq(result).asJava)
}
- private def verifyAlterConfigResult(response: AlterConfigsResponse,
- expectedResults: Map[String, Errors]): Unit = {
- val responseMap = response.data.responses().asScala.map { resourceResponse =>
- resourceResponse.resourceName -> Errors.forCode(resourceResponse.errorCode)
- }.toMap
-
- assertEquals(expectedResults, responseMap)
- }
-
- private def createConfigsWithAuthorization(authorizer: Authorizer,
- authorizedTopic: String,
- unauthorizedTopic: String): (ConfigResource, ConfigResource) = {
- val authorizedResource = new ConfigResource(ConfigResource.Type.TOPIC, authorizedTopic)
-
- val unauthorizedResource = new ConfigResource(ConfigResource.Type.TOPIC, unauthorizedTopic)
-
- createTopicAuthorization(authorizer, AclOperation.ALTER_CONFIGS, authorizedTopic, unauthorizedTopic)
- (authorizedResource, unauthorizedResource)
- }
-
- @Test
- def testIncrementalAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
This test only work for zk so we can delete it.
All requests are forwarded to controller directly except log4j setting but log4j request doesn’t redirect to controller.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910700757
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2684,49 +2628,15 @@ class KafkaApis(val requestChannel: RequestChannel,
}
}
- // Forwarding has not happened yet, so handle both ZK and KRaft cases here
if (remaining.resources().isEmpty) {
sendResponse(Some(new IncrementalAlterConfigsResponseData()))
- } else if ((!request.isForwarded) && metadataSupport.canForward() && isKRaftController) {
+ } else if ((!request.isForwarded) && metadataSupport.canForward()) {
Review Comment:
Please remove unreachable path too
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910777056
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
From my understanding, the authorization only work for `IncrementalAlterConfig`.
`AlterConfig` is legacy api so we don't do any authorization here.
You can refer [here](https://github.com/apache/kafka/blob/616bc610d075b87a6ac273d052eae9ce23bf466c/core/src/main/scala/kafka/server/KafkaApis.scala#L2504-L2525)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910797513
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -2684,49 +2628,15 @@ class KafkaApis(val requestChannel: RequestChannel,
}
}
- // Forwarding has not happened yet, so handle both ZK and KRaft cases here
if (remaining.resources().isEmpty) {
sendResponse(Some(new IncrementalAlterConfigsResponseData()))
- } else if ((!request.isForwarded) && metadataSupport.canForward() && isKRaftController) {
+ } else if ((!request.isForwarded) && metadataSupport.canForward()) {
Review Comment:
Thanks! This is my mistake did not remove this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1910777056
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
From my understanding, the authorization only work for `IncrementalAlterConfig` and `AlterConfig` is legacy api so we don't do any authorization here.
You can refer [here](https://github.com/apache/kafka/blob/616bc610d075b87a6ac273d052eae9ce23bf466c/core/src/main/scala/kafka/server/KafkaApis.scala#L2504-L2525)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (part 3): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1907659450
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -830,78 +705,6 @@ class KafkaApisTest extends Logging {
.thenReturn(Seq(result).asJava)
}
- private def verifyAlterConfigResult(response: AlterConfigsResponse,
- expectedResults: Map[String, Errors]): Unit = {
- val responseMap = response.data.responses().asScala.map { resourceResponse =>
- resourceResponse.resourceName -> Errors.forCode(resourceResponse.errorCode)
- }.toMap
-
- assertEquals(expectedResults, responseMap)
- }
-
- private def createConfigsWithAuthorization(authorizer: Authorizer,
- authorizedTopic: String,
- unauthorizedTopic: String): (ConfigResource, ConfigResource) = {
- val authorizedResource = new ConfigResource(ConfigResource.Type.TOPIC, authorizedTopic)
-
- val unauthorizedResource = new ConfigResource(ConfigResource.Type.TOPIC, unauthorizedTopic)
-
- createTopicAuthorization(authorizer, AclOperation.ALTER_CONFIGS, authorizedTopic, unauthorizedTopic)
- (authorizedResource, unauthorizedResource)
- }
-
- @Test
- def testIncrementalAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
This test only work for zk so we can delete it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913065969
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> we used to do authorisation in two places (once on the broker and once at the controller)
Sorry for unclarification, for `IncrementalAlterConfig` we authorize `BORKER_LOGGER` only [1] and it apply to local (it does not send to controller) , and we do other authorization on controller.
By the way, `BORKER_LOGGER ` does not support for ZK mode [2].
[1] https://github.com/apache/kafka/blob/bbdf79e1b4ffd4960f42ec128edbced11349c538/core/src/main/scala/kafka/server/ConfigAdminManager.scala#L137-L155
[2] https://github.com/apache/kafka/blob/cbbeccad632d37c131bfa494fdb2eabc5df7610d/core/src/main/scala/kafka/server/KafkaApis.scala#L2385-L2386
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913123323
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review Comment:
> 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18399 Remove ZooKeeper from KafkaApis (3/N): ALTER_CONFIG, INCREMENETAL_ALTER_CONFIG [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #18432:
URL: https://github.com/apache/kafka/pull/18432#discussion_r1913106963
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -485,44 +412,6 @@ class KafkaApisTest extends Logging {
}
}
- @Test
- def testAlterConfigsWithAuthorizer(): Unit = {
Review 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 https://github.com/apache/kafka/pull/18432#discussion_r1913134917
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org