You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/04/26 15:25:19 UTC
[GitHub] [accumulo] jmark99 opened a new pull request, #2651: Update ChangeSecret to work as documentation states
jmark99 opened a new pull request, #2651:
URL: https://github.com/apache/accumulo/pull/2651
There are inconsistencies in how ChangeSecret should be run, as well as
exceptions thrown when attempting to execute ChangeSecret.
In two locations, instructions for executing ChangeSecret indicate
./bin/accumulo org.apache.accumulo.server.util.ChangeSecret
But within accumulo-site.xml, instructions indicate
./bin/accumulo org.apache.accumulo.server.util.ChangeSecret [oldpasswd] [newpasswd]
Neither one of the two methods work properly.
Running with no parameters throws a NullPointerException. Running with
the indicated parameters presents a usage message with an error
message provided. Additionally, the usage message makes no mention of
the old and new password parameters.
This PR updates site.xml to match the instructions within the rest of
the code, i.e., execute the command with no parameters.
It also updates the parseArgs method within ChangeSecret to parse
the argsList variable as it did previously, thereby prompting the user for
the old and new password as appears to be the original intention.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
jmark99 commented on PR #2651:
URL: https://github.com/apache/accumulo/pull/2651#issuecomment-1110093861
@ctubbsii I updated with your suggestion and made a few additional changes to get it working. If you are okay with these changes I will commit and then you can made any additional changes you would like.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 merged pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
jmark99 merged PR #2651:
URL: https://github.com/apache/accumulo/pull/2651
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2651:
URL: https://github.com/apache/accumulo/pull/2651#discussion_r858882326
##########
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java:
##########
@@ -79,7 +79,7 @@ public static void main(String[] args) throws Exception {
argsList.add("--new");
argsList.addAll(Arrays.asList(args));
- opts.parseArgs(ChangeSecret.class.getName(), args);
+ opts.parseArgs(ChangeSecret.class.getName(), argsList.toArray(new String[0]));
Review Comment:
All of this seems quite dubious to me. This is a very convoluted way to get JCommander to do the prompting for us. Instead of doing that, we can just prompt directly for the passwords:
```suggestion
opts.parseArgs(ChangeSecret.class.getName(), args);
String oldPass = String.valueOf(System.console().readPassword("Old password: "));
String newPass = String.valueOf(System.console().readPassword("New password: "));
```
Can then just completely remove the embedded `Opts` class, and use `ServerUtilOpts` directly. This could use some tweaks, though... like using `!StringUtils.isBlank(oldPass)` instead of checking if `opts.oldPass != null` below.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a diff in pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
jmark99 commented on code in PR #2651:
URL: https://github.com/apache/accumulo/pull/2651#discussion_r858967891
##########
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java:
##########
@@ -79,7 +79,7 @@ public static void main(String[] args) throws Exception {
argsList.add("--new");
argsList.addAll(Arrays.asList(args));
- opts.parseArgs(ChangeSecret.class.getName(), args);
+ opts.parseArgs(ChangeSecret.class.getName(), argsList.toArray(new String[0]));
Review Comment:
Committed the suggestion before seeing your last comment. Let me get it in a working state and then I can commit and you can add additional modifications.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2651:
URL: https://github.com/apache/accumulo/pull/2651#discussion_r859019910
##########
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java:
##########
@@ -45,51 +45,37 @@
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.common.StringUtils;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Stat;
-import com.beust.jcommander.Parameter;
-
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Scope;
public class ChangeSecret {
- static class Opts extends ServerUtilOpts {
- @Parameter(names = "--old", description = "old zookeeper password", password = true,
- hidden = true)
- String oldPass;
- @Parameter(names = "--new", description = "new zookeeper password", password = true,
- hidden = true)
- String newPass;
- }
-
public static void main(String[] args) throws Exception {
var siteConfig = SiteConfiguration.auto();
var hadoopConf = new Configuration();
- Opts opts = new Opts();
+ ServerUtilOpts opts = new ServerUtilOpts();
ServerContext context = opts.getServerContext();
try (var fs = context.getVolumeManager()) {
ServerDirs serverDirs = new ServerDirs(siteConfig, hadoopConf);
verifyHdfsWritePermission(serverDirs, fs);
- List<String> argsList = new ArrayList<>(args.length + 2);
- argsList.add("--old");
- argsList.add("--new");
- argsList.addAll(Arrays.asList(args));
-
- opts.parseArgs(ChangeSecret.class.getName(), args);
+ String oldPass = String.valueOf(System.console().readPassword("Old password: "));
+ String newPass = String.valueOf(System.console().readPassword("New password: "));
Review Comment:
I just realized it might make sense if we make it prompts say "secret" instead of "password".
```suggestion
String oldPass = String.valueOf(System.console().readPassword("Old secret: "));
String newPass = String.valueOf(System.console().readPassword("New secret: "));
```
##########
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java:
##########
@@ -45,51 +45,37 @@
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.common.StringUtils;
Review Comment:
This should use the Apache Commons StringUtils, not the one from ZooKeeper.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2651:
URL: https://github.com/apache/accumulo/pull/2651#discussion_r858962023
##########
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java:
##########
@@ -79,7 +79,7 @@ public static void main(String[] args) throws Exception {
argsList.add("--new");
argsList.addAll(Arrays.asList(args));
- opts.parseArgs(ChangeSecret.class.getName(), args);
+ opts.parseArgs(ChangeSecret.class.getName(), argsList.toArray(new String[0]));
Review Comment:
If you want to merge your fixes as-is, I can do a follow-up to implement my suggestion.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2651:
URL: https://github.com/apache/accumulo/pull/2651#discussion_r858961554
##########
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java:
##########
@@ -79,7 +79,7 @@ public static void main(String[] args) throws Exception {
argsList.add("--new");
argsList.addAll(Arrays.asList(args));
- opts.parseArgs(ChangeSecret.class.getName(), args);
+ opts.parseArgs(ChangeSecret.class.getName(), argsList.toArray(new String[0]));
Review Comment:
Yeah, your code would have fixed the bug. But, I think it's probably better to fix the problem that caused the bug: the unnecessary complexity by trying to use JCommander to do the prompting.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a diff in pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
jmark99 commented on code in PR #2651:
URL: https://github.com/apache/accumulo/pull/2651#discussion_r858959318
##########
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java:
##########
@@ -79,7 +79,7 @@ public static void main(String[] args) throws Exception {
argsList.add("--new");
argsList.addAll(Arrays.asList(args));
- opts.parseArgs(ChangeSecret.class.getName(), args);
+ opts.parseArgs(ChangeSecret.class.getName(), argsList.toArray(new String[0]));
Review Comment:
That is fine. I was just attempting to keep it in the form that it was originally written in case there was a particular reason for that format.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a diff in pull request #2651: Update ChangeSecret to work as documentation states
Posted by GitBox <gi...@apache.org>.
jmark99 commented on code in PR #2651:
URL: https://github.com/apache/accumulo/pull/2651#discussion_r859028483
##########
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java:
##########
@@ -45,51 +45,37 @@
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.common.StringUtils;
Review Comment:
Yep! Intellij added the zk one and I missed it.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org