You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/09/30 15:42:51 UTC

[GitHub] [kafka] rondagostino opened a new pull request #9356: KAFKA-10556: NPE if sasl.mechanism is unrecognized

rondagostino opened a new pull request #9356:
URL: https://github.com/apache/kafka/pull/9356


   *More detailed description of your change,
   if necessary. The PR title and PR message become
   the squashed commit message, so use a separate
   comment to ping reviewers.*
   
   *Summary of testing strategy (including rationale)
   for the feature or bug fix. Unit and/or integration
   tests are expected for any behaviour change and
   system tests should be considered for larger changes.*
   
   ### 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] rondagostino commented on a change in pull request #9356: KAFKA-10556: NPE if sasl.mechanism is unrecognized

Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #9356:
URL: https://github.com/apache/kafka/pull/9356#discussion_r497653222



##########
File path: clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientAuthenticator.java
##########
@@ -214,7 +214,11 @@ SaslClient createSaslClient() {
                 String[] mechs = {mechanism};
                 log.debug("Creating SaslClient: client={};service={};serviceHostname={};mechs={}",
                     clientPrincipalName, servicePrincipal, host, Arrays.toString(mechs));
-                return Sasl.createSaslClient(mechs, clientPrincipalName, servicePrincipal, host, configs, callbackHandler);
+                SaslClient retvalSaslClient = Sasl.createSaslClient(mechs, clientPrincipalName, servicePrincipal, host, configs, callbackHandler);

Review comment:
       @rajinisivaram good point.  It's now fixed.  All client tests pass locally.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] rajinisivaram commented on pull request #9356: KAFKA-10556: NPE if sasl.mechanism is unrecognized

Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #9356:
URL: https://github.com/apache/kafka/pull/9356#issuecomment-701973208


   Test failures look like flaky tests, there are not related to the PR. Merging to trunk.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] rajinisivaram commented on a change in pull request #9356: KAFKA-10556: NPE if sasl.mechanism is unrecognized

Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #9356:
URL: https://github.com/apache/kafka/pull/9356#discussion_r497644530



##########
File path: clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientAuthenticator.java
##########
@@ -214,7 +214,11 @@ SaslClient createSaslClient() {
                 String[] mechs = {mechanism};
                 log.debug("Creating SaslClient: client={};service={};serviceHostname={};mechs={}",
                     clientPrincipalName, servicePrincipal, host, Arrays.toString(mechs));
-                return Sasl.createSaslClient(mechs, clientPrincipalName, servicePrincipal, host, configs, callbackHandler);
+                SaslClient retvalSaslClient = Sasl.createSaslClient(mechs, clientPrincipalName, servicePrincipal, host, configs, callbackHandler);

Review comment:
       Should we fix server creation as well?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] rondagostino commented on pull request #9356: KAFKA-10556: NPE if sasl.mechanism is unrecognized

Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9356:
URL: https://github.com/apache/kafka/pull/9356#issuecomment-703308436


   Yeah, we no longer contact the server if the client doesn't recognize the SASL mechanism.  The PR at https://github.com/apache/kafka/pull/9372 fixes this test, which was using an unknown SASL mechanism (SCRAM-SHA-256) according to the JAAS config (PLAIN).  Now it will use PLAIN and bad credentials.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] chia7712 commented on pull request #9356: KAFKA-10556: NPE if sasl.mechanism is unrecognized

Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9356:
URL: https://github.com/apache/kafka/pull/9356#issuecomment-703297575


   @rondagostino It seems ```MetricsTest.testMetrics```  start to fail after this PR is merged. Could you take a look?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] rajinisivaram merged pull request #9356: KAFKA-10556: NPE if sasl.mechanism is unrecognized

Posted by GitBox <gi...@apache.org>.
rajinisivaram merged pull request #9356:
URL: https://github.com/apache/kafka/pull/9356


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org