[GitHub] [zookeeper] ztzg commented on a change in pull request #1498: ZOOKEEPER-2649 - Add more session and authentication information to S…

[GitBox <gi...@apache.org>]

ztzg commented on a change in pull request #1498:
URL: https://github.com/apache/zookeeper/pull/1498#discussion_r504694182



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -1670,7 +1673,8 @@ private void processSasl(ByteBuffer incomingBuffer, ServerCnxn cnxn, RequestHead
                 responseToken = saslServer.evaluateResponse(clientToken);
                 if (saslServer.isComplete()) {
                     String authorizationID = saslServer.getAuthorizationID();
-                    LOG.info("adding SASL authorization for authorizationID: {}", authorizationID);
+                    LOG.info("Session 0x{}: adding SASL authorization for authorizationID: {}",

Review comment:
       +1.
   
   I'd like to have this in the tree, and was about to submit a patch with something like this.
   
   (Arguably, it'd be sufficient to augment logging here and not on the other path of `ZooKeeperServer`, as the "pluggable" authentication providers can and should do their own, "domain-specific," logging.)
   
   @Ghatage: are you planning to give this another go?
   

##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -1599,8 +1599,11 @@ public void processPacket(ServerCnxn cnxn, ByteBuffer incomingBuffer) throws IOE
                 }
             }
             if (authReturn == KeeperException.Code.OK) {
+                ZooKeeperSaslServer saslServer = cnxn.zooKeeperSaslServer;
                 LOG.debug("Authentication succeeded for scheme: {}", scheme);
-                LOG.info("auth success {}", cnxn.getRemoteSocketAddress());
+                LOG.info("Session 0x{}: auth success for authorizationID={}/{}",
+                        Long.toHexString(cnxn.getSessionId()), saslServer.getAuthorizationID(),
+                        cnxn.getRemoteSocketAddress());

Review comment:
       This is the "traditional" authentication path, used by providers others than SASL.  Unless I am missing something, `saslServer` is likely to be NULL here.
   
   (I suppose this is the reason for your "Needs more work" comment.)




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