Skip to content

ACCUMULO-4461: modified ClientOpts to prompt for missing password#154

Closed
milleruntime wants to merge 3 commits into
apache:1.7from
milleruntime:ACCUMULO-4461
Closed

ACCUMULO-4461: modified ClientOpts to prompt for missing password#154
milleruntime wants to merge 3 commits into
apache:1.7from
milleruntime:ACCUMULO-4461

Conversation

@milleruntime

@milleruntime milleruntime commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

Created promptUser() to prompt the user on the console for a password if one is missing. If System.console is null, it will return null causing getToken() to return null (like it does currently).

joshelser
joshelser requested changes Sep 19, 2016
View reviewed changes

@joshelser joshelser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit-picky things about throwing exceptions instead of returning null in expected situations.

Comment thread core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java Outdated
*/
public static Password promptUser() throws IOException {
if (System.console() == null) {
return null;

@joshelser joshelser Sep 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that throwing an exception here would be more clear. This method cannot properly function as the caller expected, so they should get a clear exception in this case (not just a null return value).

@milleruntime milleruntime Sep 20, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 895591c

Comment thread core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java Outdated
throw new RuntimeException(e);
}
}
if (pass != null) {

@joshelser joshelser Sep 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This becomes simpler too. Either we throw an RTE from the attempt to get a password, or we create a PasswordToken with the results. This method always gets to return a non-null value (woo!).

@milleruntime milleruntime Sep 20, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had something like that before running tests. TestClientOpts.test expects for that method and ones that call it to return null.

@joshelser joshelser Sep 20, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a weird test. I feel like the fail() is the expected execution path (e.g. it's asserting that getPrincipal() throw an exception) and the assertNull() is an afterthought? I wouldn't be too worried about making sure the tests pass exactly as-is. This isn't public API, so the semantics are likely not well-defined.

@milleruntime milleruntime Sep 20, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I like removing the null return. If I get rid of the try/catch, I'd have to make getToken() throw IOException which will touch a bunch of files.

@milleruntime milleruntime Sep 20, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep the try catch but remove the check for null

@joshelser

joshelser commented Sep 20, 2016

Copy link
Copy Markdown
Member

This looks good to me. I will try to find some time to test and merge. Thanks Mike!

@joshelser

joshelser commented Sep 21, 2016

Copy link
Copy Markdown
Member

I will try to find some time to test and merge

Started running a verify -Psunny. Will get this in today.

@joshelser

joshelser commented Sep 21, 2016

Copy link
Copy Markdown
Member

Uh oh. Looks like we have a failure in ReadWriteIT#sunnyDay()

Tests run: 8, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 160.031 sec <<< FAILURE! - in org.apache.accumulo.test.functional.ReadWriteIT
sunnyDay(org.apache.accumulo.test.functional.ReadWriteIT)  Time elapsed: 14.882 sec  <<< ERROR!
java.io.IOException: Failed to run `accumulo admin stopAll`
    at org.apache.accumulo.test.functional.ReadWriteIT.sunnyDay(ReadWriteIT.java:169)

2016-09-21 12:18:57,587 [util.Admin] ERROR: java.io.IOException: Attempted to prompt user on the console when System.console = null
java.lang.RuntimeException: java.io.IOException: Attempted to prompt user on the console when System.console = null
        at org.apache.accumulo.core.cli.ClientOpts.getToken(ClientOpts.java:171)
        at org.apache.accumulo.server.util.Admin.execute(Admin.java:213)
        at org.apache.accumulo.server.util.Admin.main(Admin.java:154)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at org.apache.accumulo.start.Main$2.run(Main.java:157)
        at java.lang.Thread.run(Thread.java:745)
Caused by: java.io.IOException: Attempted to prompt user on the console when System.console = null
        at org.apache.accumulo.core.cli.ClientOpts$Password.promptUser(ClientOpts.java:105)
        at org.apache.accumulo.core.cli.ClientOpts.getToken(ClientOpts.java:169)
        ... 8 more

Kind of seems like we don't have a good understanding of commands/tools which need a username + credentials (password) and which don't. Maybe that's how we need to address the problem?

@milleruntime

milleruntime commented Sep 21, 2016

Copy link
Copy Markdown
Contributor Author

Similar to the test error, this change also forces the user to enter password even when running stop-all.sh. Which is not necessary and cumbersome.

@milleruntime

milleruntime commented Sep 22, 2016

Copy link
Copy Markdown
Contributor Author

@joshelser check out 52e5444 I tested the Admin commands and I couldn't find any that required password or auth token. This looks to be the only call to getToken that checks for null.

@joshelser

joshelser commented Sep 22, 2016

Copy link
Copy Markdown
Member

Looks reasonable. Let's see what https://builds.apache.org/job/Accumulo-Pull-Requests/449/console does. It looks like it's doing the right thing. I tweaked this earlier this week to do a normal mvn verify -Psunny. Hopefully, I don't have to re-validate things by hand :)

@milleruntime

milleruntime commented Sep 22, 2016

Copy link
Copy Markdown
Contributor Author

Ok cool. I ran mvn verify -Psunny locally but coudln't figure out why tests were failing on ShellServerIT trace https://github.com/apache/accumulo/blob/1.7/test/src/test/java/org/apache/accumulo/test/ShellServerIT.java#L1444

@joshelser

joshelser commented Sep 22, 2016

Copy link
Copy Markdown
Member

coudln't figure out why tests were failing on ShellServerIT trace

Yeah, that one is flaky. Don't worry about it :)

@asfgit asfgit closed this in d23676d Sep 23, 2016
asfgit pushed a commit that referenced this pull request Sep 26, 2016
@milleruntime @joshelser
ACCUMULO-4461: modified commands to not prompt for a password
064ae0a 
Closes #154

Signed-off-by: Josh Elser <elserj@apache.org>
@milleruntime milleruntime deleted the ACCUMULO-4461 branch November 18, 2016 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshelser joshelser joshelser requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@milleruntime @joshelser