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/07/02 00:24:49 UTC
[GitHub] [ozone] aswinshakil opened a new pull request, #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
aswinshakil opened a new pull request, #3578:
URL: https://github.com/apache/ozone/pull/3578
This is an addendum to #3566
Fix the Short Principle and Full Principle check for `GetS3SecretHandler` and `TenantGetSecretHandler`
CI run: https://github.com/aswinshakil/ozone/actions/runs/2599527648
--
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 #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3578:
URL: https://github.com/apache/ozone/pull/3578#discussion_r912576423
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -378,7 +377,7 @@ public void testGetSecretWithTenant() throws IOException {
new OMTenantAssignUserAccessIdRequest(
new OMTenantAssignUserAccessIdRequest(
assignUserToTenantRequest(TENANT_ID,
- USER_BOB_SHORT, ACCESS_ID_BOB)
Review Comment:
We should revert the "bob" changes here and keep the "alice" changes. Because "bob" is used in tenant accessId testing while "alice" is for legacy s3v accessId.
--
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 #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3578:
URL: https://github.com/apache/ozone/pull/3578#discussion_r912573368
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -90,11 +90,10 @@ public class TestS3GetSecretRequest {
// Multi-tenant related vars
private static final String USER_ALICE = "alice@EXAMPLE.COM";
- private static final String USER_ALICE_SHORT = "alice";
private static final String TENANT_ID = "finance";
- private static final String USER_BOB_SHORT = "bob";
+ private static final String USER_BOB = "bob@EXAMPLE.COM";
private static final String ACCESS_ID_BOB =
- OMMultiTenantManager.getDefaultAccessId(TENANT_ID, USER_BOB_SHORT);
Review Comment:
Hmm, I'm not sure I get the intention of this change?
Access ID in tenants is supposed to look like `tenant1$bob`, not `tenant1$bob@EXAMPLE.COM`
--
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 #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3578:
URL: https://github.com/apache/ozone/pull/3578#discussion_r912577834
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -378,7 +377,7 @@ public void testGetSecretWithTenant() throws IOException {
new OMTenantAssignUserAccessIdRequest(
new OMTenantAssignUserAccessIdRequest(
assignUserToTenantRequest(TENANT_ID,
- USER_BOB_SHORT, ACCESS_ID_BOB)
Review Comment:
e.g. when we run tenant assign user CLI ([doc](https://ci-hadoop.apache.org/view/Hadoop%20Ozone/job/ozone-doc-master/lastSuccessfulBuild/artifact/hadoop-hdds/docs/public/feature/s3-tenant-commands.html#:~:text=Assign%20a%20user%20to%20a%20tenant)):
```bash
ozone tenant user assign testuser --tenant=tenantone
```
`testuser` is the `userPrincipal` in [handler](https://github.com/apache/ozone/blob/f0b30130911f4d7bacbe711ce51be798f8d8ba80/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantAssignUserAccessIdHandler.java#L70), and is passed to OM as [`username`](https://github.com/apache/ozone/blob/f3dc1580c5dfd56634120c0863f5c297ec78842e/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java#L1051-L1056). The point is `userPrincipal` input here is supposed to be a short name, not a full principal.
--
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 #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3578:
URL: https://github.com/apache/ozone/pull/3578#discussion_r915252120
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -378,7 +377,7 @@ public void testGetSecretWithTenant() throws IOException {
new OMTenantAssignUserAccessIdRequest(
new OMTenantAssignUserAccessIdRequest(
assignUserToTenantRequest(TENANT_ID,
- USER_BOB_SHORT, ACCESS_ID_BOB)
Review Comment:
Thanks for the review. Understood. I'll update the PR.
--
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 #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3578:
URL: https://github.com/apache/ozone/pull/3578#discussion_r912577834
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -378,7 +377,7 @@ public void testGetSecretWithTenant() throws IOException {
new OMTenantAssignUserAccessIdRequest(
new OMTenantAssignUserAccessIdRequest(
assignUserToTenantRequest(TENANT_ID,
- USER_BOB_SHORT, ACCESS_ID_BOB)
Review Comment:
e.g. when we run tenant assign user CLI ([doc](https://ci-hadoop.apache.org/view/Hadoop%20Ozone/job/ozone-doc-master/lastSuccessfulBuild/artifact/hadoop-hdds/docs/public/feature/s3-tenant-commands.html#:~:text=Assign%20a%20user%20to%20a%20tenant)):
```bash
ozone tenant user assign testuser --tenant=tenantone
```
`testuser` is the `userPrincipal` in [handler](https://github.com/apache/ozone/blob/f0b30130911f4d7bacbe711ce51be798f8d8ba80/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantAssignUserAccessIdHandler.java#L70), and is passed to OM as [`username`](https://github.com/apache/ozone/blob/f3dc1580c5dfd56634120c0863f5c297ec78842e/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java#L1051-L1056).
--
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 #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3578:
URL: https://github.com/apache/ozone/pull/3578#discussion_r912573368
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -90,11 +90,10 @@ public class TestS3GetSecretRequest {
// Multi-tenant related vars
private static final String USER_ALICE = "alice@EXAMPLE.COM";
- private static final String USER_ALICE_SHORT = "alice";
private static final String TENANT_ID = "finance";
- private static final String USER_BOB_SHORT = "bob";
+ private static final String USER_BOB = "bob@EXAMPLE.COM";
private static final String ACCESS_ID_BOB =
- OMMultiTenantManager.getDefaultAccessId(TENANT_ID, USER_BOB_SHORT);
Review Comment:
Hmm, I'm not sure I get the intention of this change?
Access ID is supposed to look like `tenant1$bob`, not `tenant1$bob@EXAMPLE.COM`
--
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 #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3578:
URL: https://github.com/apache/ozone/pull/3578#discussion_r912577834
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -378,7 +377,7 @@ public void testGetSecretWithTenant() throws IOException {
new OMTenantAssignUserAccessIdRequest(
new OMTenantAssignUserAccessIdRequest(
assignUserToTenantRequest(TENANT_ID,
- USER_BOB_SHORT, ACCESS_ID_BOB)
Review Comment:
e.g. when we run tenant assign user CLI ([doc](https://ci-hadoop.apache.org/view/Hadoop%20Ozone/job/ozone-doc-master/lastSuccessfulBuild/artifact/hadoop-hdds/docs/public/feature/s3-tenant-commands.html#:~:text=Assign%20a%20user%20to%20a%20tenant)):
```bash
ozone tenant user assign testuser --tenant=tenantone
```
`testuser` is the `userPrincipal` in [handler](https://github.com/apache/ozone/blob/f0b30130911f4d7bacbe711ce51be798f8d8ba80/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/tenant/TenantAssignUserAccessIdHandler.java#L70), and is passed to OM as [`username`](https://github.com/apache/ozone/blob/f3dc1580c5dfd56634120c0863f5c297ec78842e/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java#L1051-L1056). The point is `userPrincipal` input here is supposed to be a short name, not a full principal.
This is intentionally different from the legacy [`GetS3SecretHandler`](https://github.com/apache/ozone/blob/19116fe1f14a779505147bf5687dbd6239dbd800/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/GetS3SecretHandler.java#L55) where the input is full principal **by default**, which doesn't make sense in the first place IMO (because Kerberos principal can contain HOST making it non-portable) but we are stuck with it for compatibility.
--
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 pull request #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on PR #3578:
URL: https://github.com/apache/ozone/pull/3578#issuecomment-1176753367
Thanks for the review @smengcl, I have updated the patch with the suggestions.
--
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 #3578: HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user.
Posted by GitBox <gi...@apache.org>.
smengcl merged PR #3578:
URL: https://github.com/apache/ozone/pull/3578
--
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