You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "0lai0 (via GitHub)" <gi...@apache.org> on 2025/11/11 09:54:28 UTC
[PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
0lai0 opened a new pull request, #9279:
URL: https://github.com/apache/ozone/pull/9279
## What changes were proposed in this pull request?
This PR relocates ACL checks in OMTenantCreateRequest and OMTenantDeleteRequest from the validateAndUpdateCache (apply/RAFT) phase to the preExecute (RPC) phase.
- Aligns with OM design guidance to avoid external dependencies (e.g., Ranger) in validateAndUpdateCache.
- Matches existing patterns used by volume/bucket create requests.
- Fails fast before RAFT, with proper audit logging on preExecute failures.
## What is the link to the Apache JIRA
[HDDS-13864](https://issues.apache.org/jira/browse/HDDS-13864)
## How was this patch tested?
mvn -DskipTests -DskipShade -Pdist checkstyle:check
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3531624880
Thanks @Gargi-jais11 for pointing this out. I'll push a fix right away.
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3556145158
Thanks @adoroszlai and @sarvekshayr for pointing this out. I'll look into this and fix it. Thanks.
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3525230568
Thanks @0lai0 for updating the patch, LGTM !
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9279: HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest
URL: https://github.com/apache/ozone/pull/9279
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3737372075
Thanks @jojochuang and @sarvekshayr for the review! Please let me know if any changes are needed.
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2523730868
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +96,31 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ // Perform ACL check during preExecute (WRITE_ACL on volume if applicable)
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState != null) {
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ if (volumeName != null && !volumeName.isEmpty()) {
+ checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+ OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL,
+ volumeName, null, null);
+ }
Review Comment:
Thanks @jojochuang and @Gargi-jais11 suggestion. I'll push a fix right away.
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2596624682
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +98,38 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState == null) {
+ LOG.debug("tenant: '{}' does not exist", tenantId);
+ throw new OMException("Tenant '" + tenantId + "' does not exist",
+ TENANT_NOT_FOUND);
+ }
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ Preconditions.checkNotNull(volumeName);
Review Comment:
Please use `Objects.requireNonNull`.
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3551100298
Unit tests should usually be in the same package as the class being tested. So for these two tests, package should be `org.apache.hadoop.ozone.om.request.s3.tenant`.
https://github.com/apache/ozone/blob/72771127d4287663353b29aae752d78a6d79c9ec/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantCreateRequest.java#L18
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3641867054
@swamirishi please take another look
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3740087580
Merged. Thanks @0lai0 @sarvekshayr @adoroszlai @chungen0126 @Gargi-jais11 @swamirishi
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "sarvekshayr (via GitHub)" <gi...@apache.org>.
sarvekshayr commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3551276204
> Unit tests should usually be in the same package as the class being tested. So for these two tests, package should be `org.apache.hadoop.ozone.om.request.s3.tenant`.
>
> https://github.com/apache/ozone/blob/72771127d4287663353b29aae752d78a6d79c9ec/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantCreateRequest.java#L18
This package already exists under test. We can move the tests directly under it.
Ref: [[hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/tenant/TestSetRangerServiceVersionRequest.java](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/tenant/TestSetRangerServiceVersionRequest.java)](https://github.com/apache/ozone/blob/b6c742e162fe873e1d90c665ca0880ea743907d0/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/tenant/TestSetRangerServiceVersionRequest.java#L18)
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #9279:
URL: https://github.com/apache/ozone/pull/9279
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3731083342
Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen 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: 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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2527371678
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +96,36 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState == null) {
+ LOG.debug("tenant: '{}' does not exist", tenantId);
+ throw new OMException("Tenant '" + tenantId + "' does not exist",
+ TENANT_NOT_FOUND);
+ }
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ Preconditions.checkNotNull(volumeName);
+
+ // Perform ACL check during preExecute (WRITE_ACL on volume if applicable)
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ if (!volumeName.isEmpty()) {
Review Comment:
This check can have a different behaviour. The earlier implementation in validateAndUpdateCache was going ahead with the acl check for volumeName as an empty string. The authorizer may have thrown an IOException saying the resource name is empty. However we are gonna let the request through for empty volumeName now. From looking at the implementation of both the request and response classes I believe we can throw an exception here
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3528149745
Sure, I'll add a unit test in next commit. Thanks @chungen0126 .
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3542998169
I think it would be better to create a new package called 'Tenant' in 'org.apache.hadoop.ozone.om.request' and move both tests into 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: 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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2527774483
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +96,36 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState == null) {
+ LOG.debug("tenant: '{}' does not exist", tenantId);
+ throw new OMException("Tenant '" + tenantId + "' does not exist",
+ TENANT_NOT_FOUND);
+ }
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ Preconditions.checkNotNull(volumeName);
+
+ // Perform ACL check during preExecute (WRITE_ACL on volume if applicable)
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ if (!volumeName.isEmpty()) {
Review Comment:
Thanks @swamirishi for pointing this out. I'm working on it now.
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3551085619
Thanks @chungen0126 suggestion. Just to clarify, do you mean creating a sub-package tenant inside the current package (so the path becomes org.apache.hadoop.ozone.om.request.tenant) and moving the files there? Thank you.
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2522022036
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +96,31 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ // Perform ACL check during preExecute (WRITE_ACL on volume if applicable)
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState != null) {
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ if (volumeName != null && !volumeName.isEmpty()) {
+ checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+ OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL,
+ volumeName, null, null);
+ }
Review Comment:
> IMO if (dbTenantState == null) it should throw an exception to abort the request. Of course it will fail later during validateAndUpdateCache anyway.
Correct, its better to catch this exception early in the preExecute itself.
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2516805987
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +96,29 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ // Perform ACL check during preExecute (WRITE_ACL on volume if applicable)
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState != null) {
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ if (volumeName != null && !volumeName.isEmpty()) {
+ checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+ OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL,
+ volumeName, null, null);
+ }
+ }
+ } catch (IOException ex) {
+ // Ensure audit log captures preExecute failures
+ markForAudit(ozoneManager.getAuditLogger(),
+ buildAuditMessage(OMAction.DELETE_TENANT, new HashMap<>(), ex,
+ omRequest.getUserInfo()));
+ throw ex;
Review Comment:
Okay, I'll modify it . Thanks.
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3530789216
@0lai0 Please resolve the checkstyle error https://github.com/0lai0/ozone/actions/runs/19337172176/job/55315613270#step:16:17
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2516739506
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +96,29 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ // Perform ACL check during preExecute (WRITE_ACL on volume if applicable)
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState != null) {
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ if (volumeName != null && !volumeName.isEmpty()) {
+ checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+ OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL,
+ volumeName, null, null);
+ }
+ }
+ } catch (IOException ex) {
+ // Ensure audit log captures preExecute failures
+ markForAudit(ozoneManager.getAuditLogger(),
+ buildAuditMessage(OMAction.DELETE_TENANT, new HashMap<>(), ex,
+ omRequest.getUserInfo()));
+ throw ex;
Review Comment:
The audit map is empty when logging preExecute failures. The tenant ID should be included
```suggestion
Map<String, String> auditMap = new HashMap<>();
auditMap.put(OzoneConsts.TENANT, tenantId);
markForAudit(ozoneManager.getAuditLogger(),
buildAuditMessage(OMAction.DELETE_TENANT, auditMap, ex,
omRequest.getUserInfo()));
```
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2516805987
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +96,29 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ // Perform ACL check during preExecute (WRITE_ACL on volume if applicable)
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState != null) {
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ if (volumeName != null && !volumeName.isEmpty()) {
+ checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+ OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL,
+ volumeName, null, null);
+ }
+ }
+ } catch (IOException ex) {
+ // Ensure audit log captures preExecute failures
+ markForAudit(ozoneManager.getAuditLogger(),
+ buildAuditMessage(OMAction.DELETE_TENANT, new HashMap<>(), ex,
+ omRequest.getUserInfo()));
+ throw ex;
Review Comment:
Okay, I'll modify it . When the ACL check fails during the preExecute phase, the audit log includes the tenant ID for easier tracking and debugging. Is this the right direction for modification?
--
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
Re: [PR] HDDS-13864. Move ACL check in OMTenantCreateRequest and OMTenantDelete Request to pre execute [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9279:
URL: https://github.com/apache/ozone/pull/9279#discussion_r2521779104
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantDeleteRequest.java:
##########
@@ -96,6 +96,31 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// Get tenant object by tenant name
final Tenant tenantObj = multiTenantManager.getTenantFromDBById(tenantId);
+ // Perform ACL check during preExecute (WRITE_ACL on volume if applicable)
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ final OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ final OmDBTenantState dbTenantState =
+ omMetadataManager.getTenantStateTable().get(tenantId);
+ if (dbTenantState != null) {
+ final String volumeName = dbTenantState.getBucketNamespaceName();
+ if (volumeName != null && !volumeName.isEmpty()) {
+ checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME,
+ OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL,
+ volumeName, null, null);
+ }
Review Comment:
IMO if (dbTenantState == null) it should throw an exception to abort the request.
Of course it will fail later during validateAndUpdateCache anyway.
Also if volumeName is null it should throw exception too.
`Preconditions.checkNotNull(volumeName);`
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3627569145
Thanks @adoroszlai for review. I've addressed your comments. Please take another look.
--
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
Re: [PR] HDDS-13864. Move ACL check to preExecute in OMTenantCreateRequest and OMTenantDeleteRequest [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9279:
URL: https://github.com/apache/ozone/pull/9279#issuecomment-3704251990
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.
--
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