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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
