Skip to content

HDDS-15961. Resolve linked bucket source properties consistently#10859

Draft
smengcl wants to merge 4 commits into
apache:masterfrom
smengcl:HDDS-15961
Draft

HDDS-15961. Resolve linked bucket source properties consistently#10859
smengcl wants to merge 4 commits into
apache:masterfrom
smengcl:HDDS-15961

Conversation

@smengcl

@smengcl smengcl commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Generated-by: Codex (GPT-5.6 Sol)

What changes were proposed in this pull request?

HDDS-9117 changed getBucketInfo so linked buckets return selected operational properties from their resolved source bucket. HDDS-15624 extracted this behavior into shared logic and applied it to listBuckets.

This patch addresses the remaining inconsistencies:

  • Check READ access on the resolved source bucket before copying its operational properties into a linked bucket response. Propagate PERMISSION_DENIED from getBucketInfo when the caller cannot read the resolved source bucket. (When source READ access is denied during listBuckets enrichment, the current logic leaves the stored link entry unchanged and continue processing the remaining entries.)
  • Reuse successfully authorized source bucket information within one listBuckets call when multiple links resolve to the same source.
  • Copy BucketEncryptionKeyInfo from the resolved source bucket.
  • Keep the operational property setter order consistent with OmBucketInfo#toBuilder.

The link bucket's identity fields—including its volume, bucket name, owner, source path, ACLs, timestamps, and object/update IDs—remain unchanged. listBuckets also continues processing subsequent entries when a link cannot be enriched.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15961

How was this patch tested?

The following test cases were added:

  • TestOmBucketInfo#testWithOperationalPropertiesFromCopiesEncryptionInfo
  • TestBucketManagerImpl#testGetBucketInfoDoesNotCopySourcePropertiesWithoutReadAccess
  • TestBucketManagerImpl#testListBucketsDoesNotCopySourcePropertiesWithoutReadAccess
  • TestBucketManagerImpl#testListBucketsChecksSourceReadAccessOncePerSource

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens how OM enriches linked bucket responses by gating the copy of operational properties on READ access to the resolved source bucket, and extends the set of copied operational fields to include bucket encryption key info. It also optimizes listBuckets by reusing authorized source information within a single call when multiple links resolve to the same source.

Changes:

  • Add a source-bucket READ ACL check before enriching link buckets; if denied, return the stored link info without enrichment.
  • Cache resolved source bucket info during listBuckets enrichment to avoid repeated source READ checks for the same source.
  • Extend OmBucketInfo.withOperationalPropertiesFrom(...) to copy BucketEncryptionKeyInfo, with unit tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java Gate link enrichment on source READ ACL; reuse resolved source info during listBuckets.
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java Include bucket encryption key info among copied operational properties.
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmBucketInfo.java Add unit coverage ensuring encryption info is copied during operational enrichment.
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestBucketManagerImpl.java Add ACL-focused tests for link enrichment behavior and per-source READ check reuse.
Comments suppressed due to low confidence (1)

hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestBucketManagerImpl.java:600

  • Similar to the getBucketInfo case, this test currently asserts object identity (assertSame) rather than the behavioral contract. To make the test resilient and more meaningful, assert the returned linked bucket does not include source metadata/operational properties when source READ is denied, and verify the source bucket is not fetched.
    List<OmBucketInfo> result = omSpy.listBuckets(linkVolume, "", "", 100, false);

    assertSame(link, result.get(0));
    assertSame(regular, result.get(1));
    verify(metadataReader).checkAcls(BUCKET, OZONE, READ, sourceVolume, "source", null);
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

return cachedSource;
}
}
if (getAclsEnabled()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we didn't verify ACL of the target bucket even before HDDS-15624.

Linux symlink does verify the target permission, so it makes sense to check it.

resolvedBucket.realBucket(), null);
} catch (OMException e) {
if (e.getResult() == PERMISSION_DENIED) {
return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this -- wouldn't it make more sense to throw PERMISSION_DENIED exception in this case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Fixed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason Codex did that was because of this:

for (int i = 0; i < buckets.size(); i++) {
try {
buckets.set(i, enrichLinkBucketInfo(buckets.get(i), resolvedSourceCache));
} catch (IOException e) {
LOG.debug("Failed to enrich listBuckets entry for {}/{}; returning raw entry",
volumeName, buckets.get(i).getBucketName(), e);
}
}

listBuckets is doing a best-effort resolution of the links. If it hits PERMISSION_DENIED, it will leave that OmBucketInfo untouched

Copilot AI review requested due to automatic review settings July 24, 2026 20:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants