You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "hanicz (via GitHub)" <gi...@apache.org> on 2024/10/18 10:54:54 UTC
[PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
hanicz opened a new pull request, #940:
URL: https://github.com/apache/knox/pull/940
…ew create-list-alias command to create multiple aliases for multiple clusters and also list them.
## What changes were proposed in this pull request?
This PR proposes a few changes to the KnoxCLI list-alias command and a completely new create-list-aliases command. usage was missing for BatchAliasCreateCommand in printKnoxShellUsage so I added that as well.
With this change the list-alias command will be able to list for multiple clusters. The cluster names have to be concatenated by ':'. It is backward compatible with the current usage so it is able to list for a single cluster as well.
Usage: list-alias --cluster1:cluster2:clusterN
The new create-list-alias command can create multiple aliases for multiple clusters with a simple call and list them when they are created. An alias will be created for a cluster if it is listed before the cluster.
Usage: create-list-aliases --alias alias1 --value value1 --alias alias2 --value value2 --cluster cluster1 --alias alias3 --value value3 --cluster cluster2 --alias aliasN --value valueN --cluster clusterN
In the above example alias1 and alias 2 will be created for cluster1, alias3 for cluster2 and aliasN for clusterN. The --value arg is not required as it could be generated or the user will be prompt to provide it.
## How was this patch tested?
I wrote new unit tests to cover the new functionalities and tested manually on my local computer.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
Posted by "hanicz (via GitHub)" <gi...@apache.org>.
hanicz commented on code in PR #940:
URL: https://github.com/apache/knox/pull/940#discussion_r1840059865
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -275,6 +277,12 @@ private int init(String[] args) throws IOException {
printKnoxShellUsage();
return -1;
}
+ } else if (args[i].equals("create-list-aliases")) {
Review Comment:
It could be the same command. My thinking was that we are introducing two new functionalities which in my opinion could warrant a new command.
1. With this change we are able to add multiple aliases to multiple topologies.
2. When the alias creation is done for a specific topology it will list all of the aliases for 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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
Posted by "hanicz (via GitHub)" <gi...@apache.org>.
hanicz commented on code in PR #940:
URL: https://github.com/apache/knox/pull/940#discussion_r1808330808
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -701,31 +717,34 @@ protected RemoteConfigurationRegistryClientService getRemoteConfigRegistryClient
private class AliasListCommand extends Command {
- public static final String USAGE = "list-alias [--cluster clustername]";
+ public static final String USAGE = "list-alias [--cluster cluster1:clusterN]";
public static final String DESC = "The list-alias command lists all of the aliases\n" +
- "for the given hadoop --cluster. The default\n" +
+ "for the given hadoop --cluster(s). The default\n" +
"--cluster being the gateway itself.";
@Override
public void execute() throws Exception {
AliasService as = getAliasService();
- KeystoreService keystoreService = getKeystoreService();
+ KeystoreService keystoreService = getKeystoreService();
if (cluster == null) {
cluster = "__gateway";
}
- boolean credentialStoreForClusterAvailable =
- keystoreService.isCredentialStoreForClusterAvailable(cluster);
- if (credentialStoreForClusterAvailable) {
- out.println("Listing aliases for: " + cluster);
- List<String> aliases = as.getAliasesForCluster(cluster);
- for (String alias : aliases) {
- out.println(alias);
- }
- out.println("\n" + aliases.size() + " items.");
- } else {
- out.println("Invalid cluster name provided: " + cluster);
- }
+ String[] clusters = cluster.split(":");
Review Comment:
Yes, it makes more sense to use `,`. I also added it as a constant.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
Posted by "pzampino (via GitHub)" <gi...@apache.org>.
pzampino commented on code in PR #940:
URL: https://github.com/apache/knox/pull/940#discussion_r1871878393
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -275,6 +277,12 @@ private int init(String[] args) throws IOException {
printKnoxShellUsage();
return -1;
}
+ } else if (args[i].equals("create-list-aliases")) {
Review Comment:
If the new functionality can be added as an option(s) to the existing command without changing the previous behavior of the existing command (without those options), then I think I would prefer this over an entirely separate command. Ultimately, we're just creating aliases.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
Posted by "moresandeep (via GitHub)" <gi...@apache.org>.
moresandeep commented on code in PR #940:
URL: https://github.com/apache/knox/pull/940#discussion_r1806663703
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -136,7 +136,8 @@ public class KnoxCLI extends Configured implements Tool {
" [" + TopologyConverter.USAGE + "]\n" +
" [" + JWKGenerator.USAGE + "]\n" +
" [" + GenerateDescriptorCommand.USAGE + "]\n" +
- " [" + TokenMigration.USAGE + "]\n";
+ " [" + TokenMigration.USAGE + "]\n" +
+ " [" + CreateListAliasesCreateCommand.USAGE + "]\n";
Review Comment:
Great catch!
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -701,31 +717,34 @@ protected RemoteConfigurationRegistryClientService getRemoteConfigRegistryClient
private class AliasListCommand extends Command {
- public static final String USAGE = "list-alias [--cluster clustername]";
+ public static final String USAGE = "list-alias [--cluster cluster1:clusterN]";
public static final String DESC = "The list-alias command lists all of the aliases\n" +
- "for the given hadoop --cluster. The default\n" +
+ "for the given hadoop --cluster(s). The default\n" +
"--cluster being the gateway itself.";
@Override
public void execute() throws Exception {
AliasService as = getAliasService();
- KeystoreService keystoreService = getKeystoreService();
+ KeystoreService keystoreService = getKeystoreService();
if (cluster == null) {
cluster = "__gateway";
}
- boolean credentialStoreForClusterAvailable =
- keystoreService.isCredentialStoreForClusterAvailable(cluster);
- if (credentialStoreForClusterAvailable) {
- out.println("Listing aliases for: " + cluster);
- List<String> aliases = as.getAliasesForCluster(cluster);
- for (String alias : aliases) {
- out.println(alias);
- }
- out.println("\n" + aliases.size() + " items.");
- } else {
- out.println("Invalid cluster name provided: " + cluster);
- }
+ String[] clusters = cluster.split(":");
Review Comment:
1. `:` should be a constant e.g. CLUSTER_STRING_SEPARATOR
2. I think `,` would be more obvious, no?
3. What if the cluster had `:` it should not but we had a case where alias name had special characters.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
Posted by "hanicz (via GitHub)" <gi...@apache.org>.
hanicz commented on code in PR #940:
URL: https://github.com/apache/knox/pull/940#discussion_r1873057667
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -275,6 +277,12 @@ private int init(String[] args) throws IOException {
printKnoxShellUsage();
return -1;
}
+ } else if (args[i].equals("create-list-aliases")) {
Review Comment:
I moved the new functionalities into the existing command. This modification is not backward compatible because previously the --cluster parameter's position didn't matter, however from now on an alias will be created for a cluster if it is listed before the cluster.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
Posted by "hanicz (via GitHub)" <gi...@apache.org>.
hanicz commented on PR #940:
URL: https://github.com/apache/knox/pull/940#issuecomment-2820718088
I reverted the latest commit and rebased the branch. I agree with the backward compatibility concerns I believe it is better to have a completely new command that can create multiple aliases for multiple clusters.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
Posted by "pzampino (via GitHub)" <gi...@apache.org>.
pzampino merged PR #940:
URL: https://github.com/apache/knox/pull/940
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3071: New ability in list-alias to list for multiple clusters. N… [knox]
Posted by "pzampino (via GitHub)" <gi...@apache.org>.
pzampino commented on code in PR #940:
URL: https://github.com/apache/knox/pull/940#discussion_r1835098900
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -275,6 +277,12 @@ private int init(String[] args) throws IOException {
printKnoxShellUsage();
return -1;
}
+ } else if (args[i].equals("create-list-aliases")) {
Review Comment:
Does this need to be a separate command, or could it be another option to the existing create-alias(es) commands?
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org