You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by omkreddy <gi...@git.apache.org> on 2018/01/19 16:28:35 UTC
[GitHub] storm pull request #2519: MINOR: Fix possible NullPointerException in Abstra...
GitHub user omkreddy opened a pull request:
https://github.com/apache/storm/pull/2519
MINOR: Fix possible NullPointerException in AbstractAutoCreds and doc cleanups
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/omkreddy/storm AUTO-CREDS-CLEANUP
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2519.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2519
----
commit 469759e48dfc2bc9d3ac6ee6bf26fcca6707b175
Author: Manikumar Reddy <ma...@...>
Date: 2018-01-19T15:51:49Z
MINOR: Fix possible NullPointerException in AbstractAutoCreds logs and doc cleanups
----
---
[GitHub] storm pull request #2519: STORM-2903: Fix possible NullPointerException in A...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2519#discussion_r162705370
--- Diff: external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java ---
@@ -215,9 +215,17 @@ private void addTokensToUGI(Subject subject) {
if (allTokens != null) {
for (Token<? extends TokenIdentifier> token : allTokens) {
try {
+
+ if (token == null) {
+ LOG.debug("Ignoring null token");
+ continue;
+ }
+
LOG.debug("Current user: {}", UserGroupInformation.getCurrentUser());
- LOG.debug("Token from credential: {} / {}", token.toString(),
- token.decodeIdentifier().getUser());
+ LOG.debug("Token from Credentials : {}", token);
+
+ if (token.decodeIdentifier() != null)
--- End diff --
@omkreddy , toString already handles it and just printing the token would take care of decoding and printing the user information.
https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/Token.java#L429
---
[GitHub] storm pull request #2519: MINOR: Fix possible NullPointerException in Abstra...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/2519#discussion_r162677211
--- Diff: external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java ---
@@ -216,8 +216,7 @@ private void addTokensToUGI(Subject subject) {
for (Token<? extends TokenIdentifier> token : allTokens) {
try {
LOG.debug("Current user: {}", UserGroupInformation.getCurrentUser());
- LOG.debug("Token from credential: {} / {}", token.toString(),
- token.decodeIdentifier().getUser());
+ LOG.debug("Extracted Token from Credentials : {}", token);
--- End diff --
token.toString() will not print token.decodeIdentifier().getUser()) which may be useful in debugging. `UserGroupInformation.getCurrentUser().addToken(token)` ignores null tokens.
Better to log and skip when `token` is null like below instead of changing the current log statements.
```
for (Token<? extends TokenIdentifier> token : allTokens) {
try {
if(token == null) {
LOG.debug("Ignoring null token");
continue;
}
LOG.debug("Current user: {}", UserGroupInformation.getCurrentUser());
LOG.debug("Token from credential: {} / {}", token.toString(),
token.decodeIdentifier().getUser());
UserGroupInformation.getCurrentUser().addToken(token);
LOG.info("Added delegation tokens to UGI.");
} catch (IOException e) {
LOG.error("Exception while trying to add tokens to ugi", e);
}
}
```
---
[GitHub] storm issue #2519: STORM-2903: Fix possible NullPointerException in Abstract...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/2519
@arunmahadevan @omkreddy I have looked into that code before putting my initial comment. Not all `TokenIdentifier` implementations have implemented `toString()` with user information. `org.apache.hadoop.hbase.security.token.AuthenticationTokenIdentifier` does not implement toString() and the logging statement was added while fixing STORM-2555 with the change [here](https://github.com/apache/storm/commit/2544b6d99f163cd2552485a629651f8e97fff8ee#diff-a81e93604bd2f50eae45a7a3bee59d88) which is related to HBase delegation tokens.
---
[GitHub] storm issue #2519: STORM-2903: Fix possible NullPointerException in Abstract...
Posted by omkreddy <gi...@git.apache.org>.
Github user omkreddy commented on the issue:
https://github.com/apache/storm/pull/2519
@satishd @HeartSaVioR Unfortunately token.decodeIdentified() return null value in Hive related Tokens. So if we want this log, we need to add null check before printing token.decodeIdentified()..getUser()
---
[GitHub] storm pull request #2519: MINOR: Fix possible NullPointerException in Abstra...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2519#discussion_r162679780
--- Diff: external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java ---
@@ -216,8 +216,7 @@ private void addTokensToUGI(Subject subject) {
for (Token<? extends TokenIdentifier> token : allTokens) {
try {
LOG.debug("Current user: {}", UserGroupInformation.getCurrentUser());
- LOG.debug("Token from credential: {} / {}", token.toString(),
- token.decodeIdentifier().getUser());
+ LOG.debug("Extracted Token from Credentials : {}", token);
--- End diff --
It appears that the token.toString() does some decoding so might print the user info.
---
[GitHub] storm pull request #2519: STORM-2903: Fix possible NullPointerException in A...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2519
---
[GitHub] storm issue #2519: STORM-2903: Fix possible NullPointerException in Abstract...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2519
@omkreddy @arunmahadevan @satishd
My bad I forgot what I was doing. Yes that was why I got user information from `token.decodeIdentified().getUser()`.
Do we want to revert the commit, or craft a follow-up patch?
---
[GitHub] storm issue #2519: MINOR: Fix possible NullPointerException in AbstractAutoC...
Posted by omkreddy <gi...@git.apache.org>.
Github user omkreddy commented on the issue:
https://github.com/apache/storm/pull/2519
PR for master: https://github.com/apache/storm/pull/2520
---
[GitHub] storm issue #2519: MINOR: Fix possible NullPointerException in AbstractAutoC...
Posted by omkreddy <gi...@git.apache.org>.
Github user omkreddy commented on the issue:
https://github.com/apache/storm/pull/2519
@arunmahadevan @HeartSaVioR
Observed below exception while testing Hive token mechanism.
```
Caused by: java.lang.NullPointerException
at org.apache.storm.common.AbstractAutoCreds.addTokensToUGI(AbstractAutoCreds.java:219) ~[storm-autocreds-1.2.0.3.1.0.0-526.jar:1.2.0.3.1.0.0-526]
at org.apache.storm.common.AbstractAutoCreds.populateSubject(AbstractAutoCreds.java:118) ~[storm-autocreds-1.2.0.3.1.0.0-526.jar:1.2.0.3.1.0.0-526]
at org.apache.storm.security.auth.AuthUtils.populateSubject(AuthUtils.java:228) ~[storm-core-1.2.0.3.1.0.0-526.jar:1.2.0.3.1.0.0-526]
... 10 more
2018-01-19 16:23:26.157 o.a.s.util main [ERROR] Halting process: ("Error on initialization")
```
---
[GitHub] storm issue #2519: STORM-2903: Fix possible NullPointerException in Abstract...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/2519
>@satishd @HeartSaVioR Unfortunately token.decodeIdentified() returns null value in Hive related Tokens. So previous code is broken for Hive. If we want this log, we need to add null check before printing token.decodeIdentified()..getUser()
@omkreddy No harm in having null check and have good debug information in the logs.
>Do we want to revert the commit, or craft a follow-up patch?
@HeartSaVioR Better to have a followup patch.
---
[GitHub] storm issue #2519: STORM-2903: Fix possible NullPointerException in Abstract...
Posted by omkreddy <gi...@git.apache.org>.
Github user omkreddy commented on the issue:
https://github.com/apache/storm/pull/2519
@arunmahadevan thanks for the review. yes, printing token should be sufficient. reverting PR to first version
---
[GitHub] storm issue #2519: MINOR: Fix possible NullPointerException in AbstractAutoC...
Posted by omkreddy <gi...@git.apache.org>.
Github user omkreddy commented on the issue:
https://github.com/apache/storm/pull/2519
@satishd @arunmahadevan Actually exception is from token.decodeIdentifier(). decodeIdentifier can return null. Updated the PR with review comments.
---
[GitHub] storm issue #2519: MINOR: Fix possible NullPointerException in AbstractAutoC...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the issue:
https://github.com/apache/storm/pull/2519
+1, Thanks for the patch. Can you also associate it with a JIRA and raise a patch for master branch as well?
---
[GitHub] storm issue #2519: STORM-2903: Fix possible NullPointerException in Abstract...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the issue:
https://github.com/apache/storm/pull/2519
+1
---