You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "pprovenzano (via GitHub)" <gi...@apache.org> on 2023/04/06 13:07:17 UTC
[GitHub] [kafka] pprovenzano opened a new pull request, #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
pprovenzano opened a new pull request, #13513:
URL: https://github.com/apache/kafka/pull/13513
Replace saltedPassword with serverKey and StoredKey in UserScramCredentialRecord.
Bump IBP record version for SCRAM support to IBP_3_5_IV2.
Update latest IBP record version to IBP_3_5_IV2
### 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
[GitHub] [kafka] cmccabe commented on pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #13513:
URL: https://github.com/apache/kafka/pull/13513#issuecomment-1499594184
Thanks for this, @pprovenzano . Seems like a relatively straightforward change. However, you need to give a reason why we should do this. I think the reason here is for compatibility with SCRAM credentials migrated from ZK, right?
The comments in `UserScramCredentialRecord.json` for `StoredKey` and `ServerKey` are very unhelpful. They just repeat the name fo the field. If I understand correctly the meanings should be something like: `StoredKey: used by the server to verify the client's identity` and `ServerKey: used by the server to prove its identity to the client` ?
It seems that `ScramControlManager.toRecord` is no longer used; can we remove it?
Can we have a `ScramCredentialDataTest` that tests `ScramCredentialData.toRecord` / `ScramCredentialData.fromRecord` ?
--
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
[GitHub] [kafka] cmccabe merged pull request #13513: KAFKA-14881: Rework UserScramCredentialRecord
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe merged PR #13513:
URL: https://github.com/apache/kafka/pull/13513
--
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
[GitHub] [kafka] pprovenzano commented on a diff in pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "pprovenzano (via GitHub)" <gi...@apache.org>.
pprovenzano commented on code in PR #13513:
URL: https://github.com/apache/kafka/pull/13513#discussion_r1162780060
##########
metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java:
##########
@@ -458,8 +458,9 @@ public void run() throws Exception {
zkMigrationClient.readAllMetadata(batch -> {
try {
if (log.isTraceEnabled()) {
- log.trace("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
+ log.info("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
} else {
+ log.info("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
Review Comment:
This file shouldn't have been part of this commit at all. Thanks for catching this.
##########
metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java:
##########
@@ -458,8 +458,9 @@ public void run() throws Exception {
zkMigrationClient.readAllMetadata(batch -> {
try {
if (log.isTraceEnabled()) {
- log.trace("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
+ log.info("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
Review Comment:
This file shouldn't have been part of this commit at all. Thanks for catching 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
[GitHub] [kafka] cmccabe commented on a diff in pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13513:
URL: https://github.com/apache/kafka/pull/13513#discussion_r1169018558
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -167,11 +167,14 @@ public enum MetadataVersion {
// and updates to a handful of RPCs.
IBP_3_4_IV0(8, "3.4", "IV0", true),
- // Support for tiered storage (KIP-405) and SCRAM
+ // Support for tiered storage (KIP-405)
IBP_3_5_IV0(9, "3.5", "IV0", false),
// Adds replica epoch to Fetch request (KIP-903).
- IBP_3_5_IV1(10, "3.5", "IV1", false);
+ IBP_3_5_IV1(10, "3.5", "IV1", false),
+
+ // Support for SCRAM
+ IBP_3_5_IV2(11, "3.5", "IV2", false);
Review Comment:
didMetadataChange needs to be `true` here
--
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
[GitHub] [kafka] cmccabe commented on pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #13513:
URL: https://github.com/apache/kafka/pull/13513#issuecomment-1511734452
Here's a Jenkins failure I have not seen before.
```
671 [2023-04-11T13:07:59.254Z] FAILURE: Build failed with an exception.
672 [2023-04-11T13:07:59.254Z]
673 [2023-04-11T13:07:59.254Z] * What went wrong:
674 [2023-04-11T13:07:59.254Z] A problem was found with the configuration of task ':rat' (type 'RatTask').
675 [2023-04-11T13:07:59.254Z] - Gradle detected a problem with the following location: '/home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-13513'.
676 [2023-04-11T13:07:59.254Z]
677 [2023-04-11T13:07:59.254Z] Reason: Task ':rat' uses this output of task ':clients:processTestMessages' without declaring an explicit or implicit dependency. This c
678 [2023-04-11T13:07:59.254Z]
679 [2023-04-11T13:07:59.254Z] Possible solutions:
680 [2023-04-11T13:07:59.254Z] 1. Declare task ':clients:processTestMessages' as an input of ':rat'.
681 [2023-04-11T13:07:59.254Z] 2. Declare an explicit dependency on ':clients:processTestMessages' from ':rat' using Task#dependsOn.
682 [2023-04-11T13:07:59.254Z] 3. Declare an explicit dependency on ':clients:processTestMessages' from ':rat' using Task#mustRunAfter.
683 [2023-04-11T13:07:59.254Z]
684 [2023-04-11T13:07:59.254Z] Please refer to https://docs.gradle.org/8.0.2/userguide/validation_problems.html#implicit_dependency for more details about this problem
685 [2023-04-11T13:07:59.254Z]
686 [2023-04-11T13:07:59.254Z] * Try:
687 [2023-04-11T13:07:59.254Z] > Run with --stacktrace option to get the stack trace.
688 [2023-04-11T13:07:59.254Z] > Run with --info or --debug option to get more log output.
689 [2023-04-11T13:07:59.254Z] > Run with --scan to get full insights.
690 [2023-04-11T13:07:59.254Z]
691 [2023-04-11T13:07:59.254Z] * Get more help at https://help.gradle.org
692 [2023-04-11T13:07:59.254Z]
693 [2023-04-11T13:07:59.254Z] Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
694 [2023-04-11T13:07:59.254Z]
695 [2023-04-11T13:07:59.254Z] You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
696 [2023-04-11T13:07:59.254Z]
697 [2023-04-11T13:07:59.254Z] See https://docs.gradle.org/8.0.2/userguide/command_line_interface.html#sec:command_line_warnings```
Perhaps someone broke the build again?
--
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
[GitHub] [kafka] cmccabe commented on a diff in pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13513:
URL: https://github.com/apache/kafka/pull/13513#discussion_r1162102172
##########
metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java:
##########
@@ -458,8 +458,9 @@ public void run() throws Exception {
zkMigrationClient.readAllMetadata(batch -> {
try {
if (log.isTraceEnabled()) {
- log.trace("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
+ log.info("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
} else {
+ log.info("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
Review Comment:
This looks like a typo. Why is this the same as in the trace-enabled 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
[GitHub] [kafka] cmccabe commented on pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #13513:
URL: https://github.com/apache/kafka/pull/13513#issuecomment-1502329503
> Should I move ScramCredentialData.java from metadata/src/main/java/org/apache/kafka/image to metadata/src/main/java/org/apache/kafka/metadata ?
yes, please.
--
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
[GitHub] [kafka] pprovenzano commented on a diff in pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "pprovenzano (via GitHub)" <gi...@apache.org>.
pprovenzano commented on code in PR #13513:
URL: https://github.com/apache/kafka/pull/13513#discussion_r1169182411
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -167,11 +167,14 @@ public enum MetadataVersion {
// and updates to a handful of RPCs.
IBP_3_4_IV0(8, "3.4", "IV0", true),
- // Support for tiered storage (KIP-405) and SCRAM
+ // Support for tiered storage (KIP-405)
IBP_3_5_IV0(9, "3.5", "IV0", false),
// Adds replica epoch to Fetch request (KIP-903).
- IBP_3_5_IV1(10, "3.5", "IV1", false);
+ IBP_3_5_IV1(10, "3.5", "IV1", false),
+
+ // Support for SCRAM
+ IBP_3_5_IV2(11, "3.5", "IV2", false);
Review Comment:
Fixed! Hopefully it will build and auto merge.
--
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
[GitHub] [kafka] pprovenzano commented on pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "pprovenzano (via GitHub)" <gi...@apache.org>.
pprovenzano commented on PR #13513:
URL: https://github.com/apache/kafka/pull/13513#issuecomment-1500649945
Should I move `ScramCredentialData.java` from `metadata/src/main/java/org/apache/kafka/image` to `metadata/src/main/java/org/apache/kafka/metadata `?
--
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
[GitHub] [kafka] pprovenzano commented on pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "pprovenzano (via GitHub)" <gi...@apache.org>.
pprovenzano commented on PR #13513:
URL: https://github.com/apache/kafka/pull/13513#issuecomment-1500649607
> Thanks for this, @pprovenzano . Seems like a relatively straightforward change. However, you need to give a reason why we should do this. I think the reason here is for compatibility with SCRAM credentials migrated from ZK, right? (This should be in the change description)
>
> The current comments in `UserScramCredentialRecord.json` for `StoredKey` and `ServerKey` are not helpful. They just repeat the name of the field. If I understand correctly the meanings should be something like: `StoredKey: used by the server to verify the client's identity` and `ServerKey: used by the server to prove its identity to the client` ?
>
> It seems that `ScramControlManager.toRecord` is no longer used; can we remove it?
>
> Can we have a `ScramCredentialDataTest` that tests `ScramCredentialData.toRecord` / `ScramCredentialData.fromRecord` ?
Thank you for the review. I've updated the PR with all of your suggestions.
--
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
[GitHub] [kafka] cmccabe commented on a diff in pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13513:
URL: https://github.com/apache/kafka/pull/13513#discussion_r1162102531
##########
metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java:
##########
@@ -458,8 +458,9 @@ public void run() throws Exception {
zkMigrationClient.readAllMetadata(batch -> {
try {
if (log.isTraceEnabled()) {
- log.trace("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
+ log.info("Migrating {} records from ZK: {}", batch.size(), recordBatchToString(batch));
Review Comment:
It looks like you changed this for the purpose of debugging and forgot to change it back. BTW you can just change the test log4j.properties file to avoid 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
[GitHub] [kafka] mimaison commented on pull request #13513: KAFKA-14881: Update UserScramCredentialRecord for SCRAM ZK to KRaft migration
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #13513:
URL: https://github.com/apache/kafka/pull/13513#issuecomment-1511108866
@cmccabe It looks like this would be nice to backport to 3.5. If I understand correctly it would allow users enabling SCRAM in 3.5 to update to 3.6 without issues.
--
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