You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/05/05 04:20:22 UTC
[GitHub] [ozone] smengcl opened a new pull request, #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
smengcl opened a new pull request, #3381:
URL: https://github.com/apache/ozone/pull/3381
## What changes were proposed in this pull request?
For all tenant commands, add CLI option (`--json`) to print results in JSON.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6340
## How was this patch tested?
- [ ] Update unit test
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877575369
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantAssignUserAccessIdHandler.java:
##########
@@ -54,56 +49,35 @@ public class TenantAssignUserAccessIdHandler extends TenantHandler {
+ "If unspecified, accessId would be in the form of "
+ "TenantName$Principal.",
hidden = true)
- // This option is intentionally hidden for now. Because accessId isn't
- // restricted in any way so far and this could cause some conflict with
- // `s3 getsecret` and leak the secret if an admin isn't careful.
+ // This option is intentionally hidden for now. Because if accessId isn't
+ // restricted in any way this might cause `ozone s3 getsecret` to
+ // unintentionally leak secret if an admin isn't careful.
private String accessId;
- // TODO: HDDS-6340. Add an option to print JSON result
-
- private String getDefaultAccessId(String userPrincipal) {
- return tenantId + TENANT_ID_USERNAME_DELIMITER + userPrincipal;
+ private String getDefaultAccessId(String userPrinc) {
+ return tenantId + TENANT_ID_USERNAME_DELIMITER + userPrinc;
}
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
if (StringUtils.isEmpty(accessId)) {
- accessId = getDefaultAccessId(userPrincipals.get(0));
- } else if (userPrincipals.size() > 1) {
- err().println("Manually specifying accessId is only supported when there "
- + "is one user principal in the command line. Reduce the number of "
- + "principal to one and try again.");
- return;
+ accessId = getDefaultAccessId(userPrincipal);
}
- for (int i = 0; i < userPrincipals.size(); i++) {
- final String principal = userPrincipals.get(i);
- try {
- if (i >= 1) {
- accessId = getDefaultAccessId(principal);
- }
- final S3SecretValue resp =
- objStore.tenantAssignUserAccessId(principal, tenantId, accessId);
- err().println("Assigned '" + principal + "' to '" + tenantId +
- "' with accessId '" + accessId + "'.");
- out().println("export AWS_ACCESS_KEY_ID='" +
- resp.getAwsAccessKey() + "'");
- out().println("export AWS_SECRET_ACCESS_KEY='" +
- resp.getAwsSecret() + "'");
- } catch (IOException e) {
- err().println("Failed to assign '" + principal + "' to '" +
- tenantId + "': " + e.getMessage());
- if (e instanceof OMException) {
- final OMException omException = (OMException) e;
- if (omException.getResult().equals(
- OMException.ResultCodes.TENANT_NOT_FOUND)) {
- // If tenant does not exist, don't bother continuing the loop
- break;
- }
- }
- }
+ final S3SecretValue resp = client.getObjectStore()
+ .tenantAssignUserAccessId(userPrincipal, tenantId, accessId);
+
+ out().println(
+ "export AWS_ACCESS_KEY_ID='" + resp.getAwsAccessKey() + "'");
+ out().println(
+ "export AWS_SECRET_ACCESS_KEY='" + resp.getAwsSecret() + "'");
+
+ if (isVerbose()) {
+ err().println("Assigned '" + userPrincipal + "' to '" + tenantId +
Review Comment:
Same as https://github.com/apache/ozone/pull/3381#discussion_r877107059
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r876441073
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantRevokeUserAccessIdHandler.java:
##########
@@ -33,26 +30,16 @@
description = "Revoke user accessId to tenant")
public class TenantRevokeUserAccessIdHandler extends TenantHandler {
- @CommandLine.Spec
- private CommandLine.Model.CommandSpec spec;
-
- @CommandLine.Parameters(description = "List of user accessIds", arity = "1..")
- private List<String> accessIds = new ArrayList<>();
-
- // TODO: HDDS-6340. Add an option to print JSON result
+ @CommandLine.Parameters(description = "Access ID", arity = "1..1")
+ private String accessId;
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
- accessIds.forEach(accessId -> {
- try {
- objStore.tenantRevokeUserAccessId(accessId);
- err().format("Revoked accessId '%s'.%n", accessId);
- } catch (IOException e) {
- err().format("Failed to revoke accessId '%s': %s%n",
- accessId, e.getMessage());
- }
- });
+ client.getObjectStore().tenantRevokeUserAccessId(accessId);
+ if (isVerbose()) {
+ err().format("Revoked accessId '%s'.%n", accessId);
Review Comment:
Should this be sent to `stderr()`? From my understanding, if the request is a success shouldn't we print it on stdout()?
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantDeleteHandler.java:
##########
@@ -35,30 +38,36 @@ public class TenantDeleteHandler extends TenantHandler {
@CommandLine.Parameters(description = "Tenant name", arity = "1..1")
private String tenantId;
- // TODO: HDDS-6340. Add an option to print JSON result
-
@Override
protected void execute(OzoneClient client, OzoneAddress address)
throws IOException {
- try {
- final DeleteTenantState resp =
- client.getObjectStore().deleteTenant(tenantId);
- out().println("Deleted tenant '" + tenantId + "'.");
- long volumeRefCount = resp.getVolRefCount();
- assert (volumeRefCount >= 0L);
- final String volumeName = resp.getVolumeName();
- final String extraPrompt =
- "But the associated volume '" + volumeName + "' is not removed. ";
- if (volumeRefCount == 0L) {
- out().println(extraPrompt + "To delete it, run"
- + "\n ozone sh volume delete " + volumeName + "\n");
- } else {
- out().println(extraPrompt + "And it is still referenced by some other "
- + "Ozone features (refCount is " + volumeRefCount + ").");
- }
- } catch (IOException e) {
- // Throw exception to make client exit code non-zero
- throw new IOException("Failed to delete tenant '" + tenantId + "'", e);
+
+ final DeleteTenantState resp =
+ client.getObjectStore().deleteTenant(tenantId);
+
+ err().println("Deleted tenant '" + tenantId + "'.");
Review Comment:
I think we remove this, We wouldn't want to put this in `stderr()` if the request is a success.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantGetSecretHandler.java:
##########
@@ -57,14 +53,11 @@ protected void execute(OzoneClient client, OzoneAddress address)
try {
final S3SecretValue accessIdSecretKeyPair =
objectStore.getS3Secret(accessId, false);
- if (export) {
- out().println("export AWS_ACCESS_KEY_ID='" +
- accessIdSecretKeyPair.getAwsAccessKey() + "'");
- out().println("export AWS_SECRET_ACCESS_KEY='" +
- accessIdSecretKeyPair.getAwsSecret() + "'");
- } else {
- out().println(accessIdSecretKeyPair);
- }
+ // Always print export format
+ out().println("export AWS_ACCESS_KEY_ID='" +
+ accessIdSecretKeyPair.getAwsAccessKey() + "'");
+ out().println("export AWS_SECRET_ACCESS_KEY='" +
+ accessIdSecretKeyPair.getAwsSecret() + "'");
} catch (OMException omEx) {
Review Comment:
Should we remove the try-catch block here similar to `TenantSetSecretHandler` to make it consistent with other CLI commands?
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantAssignUserAccessIdHandler.java:
##########
@@ -54,56 +49,35 @@ public class TenantAssignUserAccessIdHandler extends TenantHandler {
+ "If unspecified, accessId would be in the form of "
+ "TenantName$Principal.",
hidden = true)
- // This option is intentionally hidden for now. Because accessId isn't
- // restricted in any way so far and this could cause some conflict with
- // `s3 getsecret` and leak the secret if an admin isn't careful.
+ // This option is intentionally hidden for now. Because if accessId isn't
+ // restricted in any way this might cause `ozone s3 getsecret` to
+ // unintentionally leak secret if an admin isn't careful.
private String accessId;
- // TODO: HDDS-6340. Add an option to print JSON result
-
- private String getDefaultAccessId(String userPrincipal) {
- return tenantId + TENANT_ID_USERNAME_DELIMITER + userPrincipal;
+ private String getDefaultAccessId(String userPrinc) {
+ return tenantId + TENANT_ID_USERNAME_DELIMITER + userPrinc;
}
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
if (StringUtils.isEmpty(accessId)) {
- accessId = getDefaultAccessId(userPrincipals.get(0));
- } else if (userPrincipals.size() > 1) {
- err().println("Manually specifying accessId is only supported when there "
- + "is one user principal in the command line. Reduce the number of "
- + "principal to one and try again.");
- return;
+ accessId = getDefaultAccessId(userPrincipal);
}
- for (int i = 0; i < userPrincipals.size(); i++) {
- final String principal = userPrincipals.get(i);
- try {
- if (i >= 1) {
- accessId = getDefaultAccessId(principal);
- }
- final S3SecretValue resp =
- objStore.tenantAssignUserAccessId(principal, tenantId, accessId);
- err().println("Assigned '" + principal + "' to '" + tenantId +
- "' with accessId '" + accessId + "'.");
- out().println("export AWS_ACCESS_KEY_ID='" +
- resp.getAwsAccessKey() + "'");
- out().println("export AWS_SECRET_ACCESS_KEY='" +
- resp.getAwsSecret() + "'");
- } catch (IOException e) {
- err().println("Failed to assign '" + principal + "' to '" +
- tenantId + "': " + e.getMessage());
- if (e instanceof OMException) {
- final OMException omException = (OMException) e;
- if (omException.getResult().equals(
- OMException.ResultCodes.TENANT_NOT_FOUND)) {
- // If tenant does not exist, don't bother continuing the loop
- break;
- }
- }
- }
+ final S3SecretValue resp = client.getObjectStore()
+ .tenantAssignUserAccessId(userPrincipal, tenantId, accessId);
+
+ out().println(
+ "export AWS_ACCESS_KEY_ID='" + resp.getAwsAccessKey() + "'");
+ out().println(
+ "export AWS_SECRET_ACCESS_KEY='" + resp.getAwsSecret() + "'");
+
+ if (isVerbose()) {
+ err().println("Assigned '" + userPrincipal + "' to '" + tenantId +
Review Comment:
We can move this to `stdout()`, Your thoughts?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877577463
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantDeleteHandler.java:
##########
@@ -35,30 +38,36 @@ public class TenantDeleteHandler extends TenantHandler {
@CommandLine.Parameters(description = "Tenant name", arity = "1..1")
private String tenantId;
- // TODO: HDDS-6340. Add an option to print JSON result
-
@Override
protected void execute(OzoneClient client, OzoneAddress address)
throws IOException {
- try {
- final DeleteTenantState resp =
- client.getObjectStore().deleteTenant(tenantId);
- out().println("Deleted tenant '" + tenantId + "'.");
- long volumeRefCount = resp.getVolRefCount();
- assert (volumeRefCount >= 0L);
- final String volumeName = resp.getVolumeName();
- final String extraPrompt =
- "But the associated volume '" + volumeName + "' is not removed. ";
- if (volumeRefCount == 0L) {
- out().println(extraPrompt + "To delete it, run"
- + "\n ozone sh volume delete " + volumeName + "\n");
- } else {
- out().println(extraPrompt + "And it is still referenced by some other "
- + "Ozone features (refCount is " + volumeRefCount + ").");
- }
- } catch (IOException e) {
- // Throw exception to make client exit code non-zero
- throw new IOException("Failed to delete tenant '" + tenantId + "'", e);
+
+ final DeleteTenantState resp =
+ client.getObjectStore().deleteTenant(tenantId);
+
+ err().println("Deleted tenant '" + tenantId + "'.");
Review Comment:
Yeah this was discussed earlier with @errose28 last week.
The point is to have a nice prompt that informs the admin to delete the volume manually.
stderr doesn't necessarily have to only contain error message though (e.g. try `wget google.com 2> /dev/null`). The point here is to seperate the JSON output (printed to stdout) and user prompts.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877107059
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantRevokeUserAccessIdHandler.java:
##########
@@ -33,26 +30,16 @@
description = "Revoke user accessId to tenant")
public class TenantRevokeUserAccessIdHandler extends TenantHandler {
- @CommandLine.Spec
- private CommandLine.Model.CommandSpec spec;
-
- @CommandLine.Parameters(description = "List of user accessIds", arity = "1..")
- private List<String> accessIds = new ArrayList<>();
-
- // TODO: HDDS-6340. Add an option to print JSON result
+ @CommandLine.Parameters(description = "Access ID", arity = "1..1")
+ private String accessId;
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
- accessIds.forEach(accessId -> {
- try {
- objStore.tenantRevokeUserAccessId(accessId);
- err().format("Revoked accessId '%s'.%n", accessId);
- } catch (IOException e) {
- err().format("Failed to revoke accessId '%s': %s%n",
- accessId, e.getMessage());
- }
- });
+ client.getObjectStore().tenantRevokeUserAccessId(accessId);
+ if (isVerbose()) {
+ err().format("Revoked accessId '%s'.%n", accessId);
Review Comment:
Actually I initially did print this to `stdout`.
But then I don't want to mix the output in `TenantAssignUserAccessIdHandler` (with verbose flag on). In that case, `export AWS_ACCESS_KEY_ID=...` goes to stdout while "Assigned user to tenant" message goes to stderr.
So here in the case of revoke user, I'm trying to keep the behavior consistent with assign user (i.e. verbose success message both goes to stderr).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877107059
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantRevokeUserAccessIdHandler.java:
##########
@@ -33,26 +30,16 @@
description = "Revoke user accessId to tenant")
public class TenantRevokeUserAccessIdHandler extends TenantHandler {
- @CommandLine.Spec
- private CommandLine.Model.CommandSpec spec;
-
- @CommandLine.Parameters(description = "List of user accessIds", arity = "1..")
- private List<String> accessIds = new ArrayList<>();
-
- // TODO: HDDS-6340. Add an option to print JSON result
+ @CommandLine.Parameters(description = "Access ID", arity = "1..1")
+ private String accessId;
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
- accessIds.forEach(accessId -> {
- try {
- objStore.tenantRevokeUserAccessId(accessId);
- err().format("Revoked accessId '%s'.%n", accessId);
- } catch (IOException e) {
- err().format("Failed to revoke accessId '%s': %s%n",
- accessId, e.getMessage());
- }
- });
+ client.getObjectStore().tenantRevokeUserAccessId(accessId);
+ if (isVerbose()) {
+ err().format("Revoked accessId '%s'.%n", accessId);
Review Comment:
Actually I initially did print this to `stdout`.
But then I don't want to mix the "export" output in `TenantAssignUserAccessIdHandler` (with verbose flag). In that case `export AWS_ACCESS_KEY_ID=..` goes to stdout while "Assigned user to tenant" message goes to stderr.
In the case of revoke user, I'm trying to keep the behavior consistent with assign user (i.e. verbose success message both goes to stderr).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877107059
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantRevokeUserAccessIdHandler.java:
##########
@@ -33,26 +30,16 @@
description = "Revoke user accessId to tenant")
public class TenantRevokeUserAccessIdHandler extends TenantHandler {
- @CommandLine.Spec
- private CommandLine.Model.CommandSpec spec;
-
- @CommandLine.Parameters(description = "List of user accessIds", arity = "1..")
- private List<String> accessIds = new ArrayList<>();
-
- // TODO: HDDS-6340. Add an option to print JSON result
+ @CommandLine.Parameters(description = "Access ID", arity = "1..1")
+ private String accessId;
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
- accessIds.forEach(accessId -> {
- try {
- objStore.tenantRevokeUserAccessId(accessId);
- err().format("Revoked accessId '%s'.%n", accessId);
- } catch (IOException e) {
- err().format("Failed to revoke accessId '%s': %s%n",
- accessId, e.getMessage());
- }
- });
+ client.getObjectStore().tenantRevokeUserAccessId(accessId);
+ if (isVerbose()) {
+ err().format("Revoked accessId '%s'.%n", accessId);
Review Comment:
Actually I initially did print this to `stdout`.
But then I don't want to mix the "export" output in `TenantAssignUserAccessIdHandler` (with verbose flag). In that case `export AWS_ACCESS_KEY_ID=..` goes to stdout while "Assigned user to tenant" message goes to stderr.
In the case of revoke user I'm trying to keep the behavior consistent with assign user (i.e. verbose success message both goes to stderr).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl merged pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl merged PR #3381:
URL: https://github.com/apache/ozone/pull/3381
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877107059
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantRevokeUserAccessIdHandler.java:
##########
@@ -33,26 +30,16 @@
description = "Revoke user accessId to tenant")
public class TenantRevokeUserAccessIdHandler extends TenantHandler {
- @CommandLine.Spec
- private CommandLine.Model.CommandSpec spec;
-
- @CommandLine.Parameters(description = "List of user accessIds", arity = "1..")
- private List<String> accessIds = new ArrayList<>();
-
- // TODO: HDDS-6340. Add an option to print JSON result
+ @CommandLine.Parameters(description = "Access ID", arity = "1..1")
+ private String accessId;
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
- accessIds.forEach(accessId -> {
- try {
- objStore.tenantRevokeUserAccessId(accessId);
- err().format("Revoked accessId '%s'.%n", accessId);
- } catch (IOException e) {
- err().format("Failed to revoke accessId '%s': %s%n",
- accessId, e.getMessage());
- }
- });
+ client.getObjectStore().tenantRevokeUserAccessId(accessId);
+ if (isVerbose()) {
+ err().format("Revoked accessId '%s'.%n", accessId);
Review Comment:
Actually I initially did print this to `stdout`.
But then I don't want to mix the output in `TenantAssignUserAccessIdHandler` (with verbose flag on). In that case, `export AWS_ACCESS_KEY_ID=...` goes to stdout while "Assigned user to tenant" message goes to stderr.
So here in the case of revoke user, I'm trying to keep the behavior consistent with assign user (i.e. verbose success message always goes to stderr).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on PR #3381:
URL: https://github.com/apache/ozone/pull/3381#issuecomment-1133137110
Thanks @aswinshakil for reviewing this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877107059
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantRevokeUserAccessIdHandler.java:
##########
@@ -33,26 +30,16 @@
description = "Revoke user accessId to tenant")
public class TenantRevokeUserAccessIdHandler extends TenantHandler {
- @CommandLine.Spec
- private CommandLine.Model.CommandSpec spec;
-
- @CommandLine.Parameters(description = "List of user accessIds", arity = "1..")
- private List<String> accessIds = new ArrayList<>();
-
- // TODO: HDDS-6340. Add an option to print JSON result
+ @CommandLine.Parameters(description = "Access ID", arity = "1..1")
+ private String accessId;
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
- accessIds.forEach(accessId -> {
- try {
- objStore.tenantRevokeUserAccessId(accessId);
- err().format("Revoked accessId '%s'.%n", accessId);
- } catch (IOException e) {
- err().format("Failed to revoke accessId '%s': %s%n",
- accessId, e.getMessage());
- }
- });
+ client.getObjectStore().tenantRevokeUserAccessId(accessId);
+ if (isVerbose()) {
+ err().format("Revoked accessId '%s'.%n", accessId);
Review Comment:
Actually I initially did print this to `stdout`.
But then I don't want to mix the output in `TenantAssignUserAccessIdHandler` (with verbose flag on). In that case, `export AWS_ACCESS_KEY_ID=..` goes to stdout while "Assigned user to tenant" message goes to stderr.
In the case of revoke user, I'm trying to keep the behavior consistent with assign user (i.e. verbose success message both goes to stderr).
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantRevokeUserAccessIdHandler.java:
##########
@@ -33,26 +30,16 @@
description = "Revoke user accessId to tenant")
public class TenantRevokeUserAccessIdHandler extends TenantHandler {
- @CommandLine.Spec
- private CommandLine.Model.CommandSpec spec;
-
- @CommandLine.Parameters(description = "List of user accessIds", arity = "1..")
- private List<String> accessIds = new ArrayList<>();
-
- // TODO: HDDS-6340. Add an option to print JSON result
+ @CommandLine.Parameters(description = "Access ID", arity = "1..1")
+ private String accessId;
@Override
- protected void execute(OzoneClient client, OzoneAddress address) {
- final ObjectStore objStore = client.getObjectStore();
+ protected void execute(OzoneClient client, OzoneAddress address)
+ throws IOException {
- accessIds.forEach(accessId -> {
- try {
- objStore.tenantRevokeUserAccessId(accessId);
- err().format("Revoked accessId '%s'.%n", accessId);
- } catch (IOException e) {
- err().format("Failed to revoke accessId '%s': %s%n",
- accessId, e.getMessage());
- }
- });
+ client.getObjectStore().tenantRevokeUserAccessId(accessId);
+ if (isVerbose()) {
+ err().format("Revoked accessId '%s'.%n", accessId);
Review Comment:
Actually I initially did print this to `stdout`.
But then I don't want to mix the output in `TenantAssignUserAccessIdHandler` (with verbose flag on). In that case, `export AWS_ACCESS_KEY_ID=...` goes to stdout while "Assigned user to tenant" message goes to stderr.
In the case of revoke user, I'm trying to keep the behavior consistent with assign user (i.e. verbose success message both goes to stderr).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877577463
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantDeleteHandler.java:
##########
@@ -35,30 +38,36 @@ public class TenantDeleteHandler extends TenantHandler {
@CommandLine.Parameters(description = "Tenant name", arity = "1..1")
private String tenantId;
- // TODO: HDDS-6340. Add an option to print JSON result
-
@Override
protected void execute(OzoneClient client, OzoneAddress address)
throws IOException {
- try {
- final DeleteTenantState resp =
- client.getObjectStore().deleteTenant(tenantId);
- out().println("Deleted tenant '" + tenantId + "'.");
- long volumeRefCount = resp.getVolRefCount();
- assert (volumeRefCount >= 0L);
- final String volumeName = resp.getVolumeName();
- final String extraPrompt =
- "But the associated volume '" + volumeName + "' is not removed. ";
- if (volumeRefCount == 0L) {
- out().println(extraPrompt + "To delete it, run"
- + "\n ozone sh volume delete " + volumeName + "\n");
- } else {
- out().println(extraPrompt + "And it is still referenced by some other "
- + "Ozone features (refCount is " + volumeRefCount + ").");
- }
- } catch (IOException e) {
- // Throw exception to make client exit code non-zero
- throw new IOException("Failed to delete tenant '" + tenantId + "'", e);
+
+ final DeleteTenantState resp =
+ client.getObjectStore().deleteTenant(tenantId);
+
+ err().println("Deleted tenant '" + tenantId + "'.");
Review Comment:
Yeah this was debated earlier with @errose28 last week.
The point is to have a nice prompt that informs the admin to delete the volume manually.
stderr doesn't necessarily have to only contain error message though (e.g. try `wget google.com 2> /dev/null`). The point here is to seperate the JSON output (printed to stdout) and user prompts.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3381:
URL: https://github.com/apache/ozone/pull/3381#discussion_r877112958
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantGetSecretHandler.java:
##########
@@ -57,14 +53,11 @@ protected void execute(OzoneClient client, OzoneAddress address)
try {
final S3SecretValue accessIdSecretKeyPair =
objectStore.getS3Secret(accessId, false);
- if (export) {
- out().println("export AWS_ACCESS_KEY_ID='" +
- accessIdSecretKeyPair.getAwsAccessKey() + "'");
- out().println("export AWS_SECRET_ACCESS_KEY='" +
- accessIdSecretKeyPair.getAwsSecret() + "'");
- } else {
- out().println(accessIdSecretKeyPair);
- }
+ // Always print export format
+ out().println("export AWS_ACCESS_KEY_ID='" +
+ accessIdSecretKeyPair.getAwsAccessKey() + "'");
+ out().println("export AWS_SECRET_ACCESS_KEY='" +
+ accessIdSecretKeyPair.getAwsSecret() + "'");
} catch (OMException omEx) {
Review Comment:
Good point. Done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #3381: HDDS-6340. [Multi-Tenant] Add tenant CLI option to print results in JSON
Posted by GitBox <gi...@apache.org>.
smengcl commented on PR #3381:
URL: https://github.com/apache/ozone/pull/3381#issuecomment-1126340997
@aswinshakil Would you take a look at this? Thx!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org