You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "chungen0126 (via GitHub)" <gi...@apache.org> on 2026/10/12 02:16:06 UTC
[PR] HDDS-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
chungen0126 opened a new pull request, #11479:
URL: https://github.com/apache/ozone/pull/11479
## What changes were proposed in this pull request?
S3 Gateway resolves the S3 volume with a separate `GetS3VolumeContext` call (`getS3Volume`) before looking up a bucket. Following HDDS-7324, which did the same for `GetKeyInfo`, this patch lets OM resolve the S3 volume inside the `InfoBucket` call, so a bucket lookup takes one RPC instead of two.
- **Protocol:** `OMRequest` gets an optional `assumeS3Context` flag. When it is set on `InfoBucket`, OM resolves the S3 volume itself (multi-tenancy aware) and returns the volume info and user principal in `OMResponse`.
- **Client:** `RpcClient.getS3BucketDetails` uses this in a single `InfoBucket` call and updates the S3 principal from the response. Older OMs ignore `assumeS3Context` and would look up the bucket in the default S3 volume. To stay correct under multi-tenancy, the client is gated on a new `OzoneManagerVersion.S3_BUCKET_INFO_WITH_VOLUME_CONTEXT`; for older OMs it falls back to `GetS3VolumeContext` + `InfoBucket`.
- **S3 Gateway:**
- `S3RequestContext` caches the bucket for the request, so the owner check and the handler share one lookup.
- `getVolumeName()` reuses the cached bucket's volume when the bucket has already been looked up.
- Bucket lookups now use `getS3BucketDetails`: HEAD bucket, list objects, multi-delete, bucket ACL, bucket tagging, bucket lifecycle, delete bucket and list multipart uploads (owner checks), HEAD object, multipart upload init/complete, put object, and copy source owner checks.
Object tagging is unchanged: it still makes one `getS3Volume` call (see HDDS-16603). Removing that call would need OM to resolve the S3 volume for tagging requests too, which is left for a follow-up.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16681
## How was this patch tested?
- New unit test `TestRpcClientGetS3BucketDetails` covers both cases:
- **New OM:** only `InfoBucket` is sent, with `assumeS3Context`.
- **Older OM:** the client falls back to `GetS3VolumeContext` + `InfoBucket`.
- Extended `TestMultiTenantVolume` to cover `getS3BucketDetails` for the default S3 volume and for tenant volumes, including the principal update and bucket-not-found.
- Updated the affected S3 Gateway unit tests and added lookup-count checks for lifecycle PUT and list multipart uploads.
- Ran the `ozone-client` and `ozone-s3gateway` unit tests, `TestMultiTenantVolume`, `checkstyle.sh` and `rat.sh` locally.
Generated-by: Claude Code (Claude Opus 5.5)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6128146477
@chungen0126 — I pushed the review feedback to a branch on @jojochuang's fork so you can pull it locally:
**Branch:** `HDDS-16681-review-fixes`
**Remote:** https://github.com/jojochuang/ozone/tree/HDDS-16681-review-fixes
**Changes (commit `a8e74823e61`):**
- Lifecycle GET uses `context.getVolumeName()` again so GetBucketLifecycle does not always call `getS3BucketDetails` when `x-amz-expected-bucket-owner` is absent.
- Restored `testGetLifecycleOnlyLooksUpBucketForOwner` (with `getS3BucketDetails` stubs) and fixed the protocol-error test to expect no bucket lookup without owner header.
- Updated `proto.lock` for `OMRequest.assumeS3Context` (field 8) and `OMResponse.userPrincipal` (field 162).
**To apply on your branch:**
```bash
git remote add jojochuang https://github.com/jojochuang/ozone.git # if needed
git fetch jojochuang HDDS-16681-review-fixes
git checkout HDDS-16681
git cherry-pick a8e74823e61
# or: git merge jojochuang/HDDS-16681-review-fixes
```
Note: `proto.lock` is still behind the full `.proto` for other fields (same as master); only the new HDDS-16681 fields were added 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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4349203673
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java:
##########
@@ -103,6 +103,8 @@ public void setup() {
S3GatewayMetrics.create(conf);
when(client.getProxy()).thenReturn(clientProtocol);
when(objectStore.getClientProxy()).thenReturn(clientProtocol);
+ when(clientProtocol.getS3BucketDetails(anyString())).thenReturn(bucket);
Review Comment:
Done, dropped them. Now if any of these tests accidentally call volume.getBucket(...) and introduce an extra RPC, they will fail immediately.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4312768390
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getS3BucketDetails(name);
} else {
Preconditions.assertEquals(bucket.getName(), name, "Multiple buckets in one request are not supported");
}
return bucket;
}
+ /**
+ * Returns the S3 volume name, reusing the bucket already looked up in this request to avoid another call.
+ */
+ String getVolumeName() throws IOException {
Review Comment:
Add tests.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6289113741
@Russole @chihsuan PTAL
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub) via issues" <is...@ozone.apache.org>.
chungen0126 commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6656480080
Merged. Thanks @jojochuang @chihsuan @rich7420 for the review.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4349208228
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/BucketInfoWithVolumeContext.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.helpers;
+
+import java.io.IOException;
+import java.util.Optional;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
+
+/**
+ * Wrapper class to return BucketInfo and optionally the S3 user principal.
+ */
+public final class BucketInfoWithVolumeContext {
Review Comment:
Updated
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java:
##########
@@ -1045,4 +1057,77 @@ public void getKeyInfoServesObjectStoreLayoutWhenNotRequested(boolean assumeS3Co
verify(keyManager).getKeyInfo(any(OmKeyArgs.class), any(ResolvedBucket.class), any());
}
+
+ @Test
+ public void getBucketInfoResolvesS3VolumeWhenAssumeS3Context() throws Exception {
+ BucketManager bucketManager = mock(BucketManager.class);
+ OmMetadataReader reader = mock(OmMetadataReader.class);
+ OMMetrics metrics = mock(OMMetrics.class);
+ OzoneManager ozoneManager = newBucketInfoOzoneManager(bucketManager, reader, metrics);
+ OmBucketInfo bucket = bucketInfo(TENANT_VOLUME_NAME);
+ when(bucketManager.getBucketInfo(TENANT_VOLUME_NAME, BUCKET_NAME)).thenReturn(bucket);
+
+ BucketInfoWithVolumeContext result = ozoneManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME, true);
+
+ assertSame(bucket, result.getBucketInfo());
+ assertEquals(Optional.of("user"), result.getUserPrincipal());
+ verify(ozoneManager).getS3VolumeContext(true);
+ verify(reader).checkAcls(BUCKET, OZONE, READ, TENANT_VOLUME_NAME, BUCKET_NAME, null);
+ verify(metrics).incNumBucketInfos();
+ }
+
+ @Test
+ public void getBucketInfoUsesRequestedVolumeWithoutAssumeS3Context() throws Exception {
+ BucketManager bucketManager = mock(BucketManager.class);
+ OmMetadataReader reader = mock(OmMetadataReader.class);
+ OzoneManager ozoneManager = newBucketInfoOzoneManager(bucketManager, reader, mock(OMMetrics.class));
+ OmBucketInfo bucket = bucketInfo(VOLUME_NAME);
+ when(bucketManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME)).thenReturn(bucket);
+
+ BucketInfoWithVolumeContext result = ozoneManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME, false);
+
+ assertSame(bucket, result.getBucketInfo());
+ assertFalse(result.getUserPrincipal().isPresent());
+ verify(ozoneManager, never()).getS3VolumeContext(anyBoolean());
+ verify(reader).checkAcls(BUCKET, OZONE, READ, VOLUME_NAME, BUCKET_NAME, null);
+ }
+
+ @Test
+ public void getBucketInfoReturnsBucketNotFoundWithoutVolumeCheck() throws Exception {
Review Comment:
Updated
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4312752486
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-sts-multitenant.robot:
##########
@@ -269,7 +269,7 @@ Verify Limited-Scoped Token Accesses
Get Object Should Fail ${TENANT_ONE_LINKED_BUCKET} ${ANOTHER_BUCKET_TESTFILE} AccessDenied
Put Object Should Fail ${TENANT_ONE_LINKED_BUCKET} ${ANOTHER_BUCKET_TESTFILE} AccessDenied
Get Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET} ${ANOTHER_BUCKET_TESTFILE} NoSuchBucket
- Put Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET} ${ANOTHER_BUCKET_TESTFILE} AccessDenied
+ Put Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET} ${ANOTHER_BUCKET_TESTFILE} NoSuchBucket
Review Comment:
Yes, it is intentional. InfoBucket with assumeS3Context resolves the S3 volume with getS3VolumeContext(true), the same as getKeyInfo for GetObject, so a missing bucket now returns NoSuchBucket for PutObject as well.
This matches the existing GetObject expectation on line 271 and the role-token PutObject case on line 203.
It’s a bit of a shortcut/compromise, but I believe it’s harmless. Here’s what’s happening under the hood:
- Previously, the workflow called getS3VolumeContext first before looking up the bucket. It would fail at the first step and throw AccessDenied due to lacking volume permissions.
- Now, since we’ve consolidated everything into a single RPC, it goes straight to the bucket lookup. The catch here is that instead of checking volume permissions upfront, it fetches the bucket and volume info first before performing the permission checks. Because the bucket isn't found during the lookup, it throws NoSuchBucket instead.
I think this difference in error message is harmless, and trying to preserve the exact AccessDenied behavior here would introduce unnecessary overhead.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4424620863
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -284,9 +284,6 @@ public RpcClient(ConfigurationSource conf, String omServiceId)
dtService = omTransport.getDelegationTokenService();
ServiceInfoEx serviceInfoEx = ozoneManagerClient.getServiceInfo();
omVersion = getOmVersion(serviceInfoEx);
- resolveS3VolumeOnOm = conf.getBoolean(S3Auth.S3_AUTH_CHECK, false)
- && OzoneManagerVersion.S3_VOLUME_CONTEXT_RESOLUTION
- .isSupportedBy(omVersion);
Review Comment:
Since omVersion is now updated periodically, we should fetch the latest omVersion dynamically instead of fixing it at startup.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub) via issues" <is...@ozone.apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4479383227
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getBucketDetails(volume != null ? volume.getName() : null, name);
Review Comment:
Thanks for pointing this out. I believe we already have dedicated JIRAs tracking these individual operations. I think it would be better to optimize the round-trip calls under their JIRAs to keep the scope of this PR focused.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4313318259
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3288,19 +3289,33 @@ private OmBucketInfo getResolvedSourceBucket(
@Override
public OmBucketInfo getBucketInfo(String volume, String bucket)
throws IOException {
+ return getBucketInfo(volume, bucket, false).getBucketInfo();
+ }
+
+ @Override
+ public BucketInfoWithVolumeContext getBucketInfo(String volume, String bucket, boolean assumeS3Context)
+ throws IOException {
+ String resolvedVolumeName = volume;
+ BucketInfoWithVolumeContext.Builder builder = BucketInfoWithVolumeContext.newBuilder();
+ if (assumeS3Context) {
+ S3VolumeContext context = getS3VolumeContext(true);
Review Comment:
nit: Would it be worth an OM-side unit test here, like #11449 did? It's only covered by `TestMultiTenantVolume`.
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/TestRpcClientGetS3BucketDetails.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.client;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import jakarta.annotation.Nonnull;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hadoop.hdds.conf.InMemoryConfigurationForTesting;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeType;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.ozone.OzoneManagerVersion;
+import org.apache.hadoop.ozone.client.rpc.RpcClient;
+import org.apache.hadoop.ozone.om.helpers.ServiceInfoEx;
+import org.apache.hadoop.ozone.om.protocol.S3Auth;
+import org.apache.hadoop.ozone.om.protocolPB.OmTransport;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.GetS3VolumeContextResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.InfoBucketRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.InfoVolumeRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.ServiceInfo;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.ServiceListResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Verifies that {@link RpcClient#getS3BucketDetails} resolves the S3 volume on OM in the InfoBucket call
+ * (HDDS-16681), and falls back to a separate GetS3VolumeContext call for an older OM.
+ */
+public class TestRpcClientGetS3BucketDetails {
+
+ private static final String TENANT_VOLUME = "tenantvol";
+ private static final String BUCKET = "bucket1";
+ private static final String ACCESS_ID = "tenant$user";
+ private static final String USER_PRINCIPAL = "user";
+
+ private final List<OMRequest> requests = new ArrayList<>();
+
+ /**
+ * Creates a client against a mock OM reporting {@code omVersion}. The mock OM maps every S3 request to
+ * {@link #TENANT_VOLUME}, like OM does for a tenant user, but only honors assumeS3Context in InfoBucket when
+ * {@code omVersion} supports it.
+ */
+ private OzoneClient newClient(OzoneManagerVersion omVersion) throws IOException {
+ InMemoryConfigurationForTesting conf = new InMemoryConfigurationForTesting();
+ conf.setFromObject(conf.getObject(OzoneClientConfig.class));
+ boolean resolvesS3Context = OzoneManagerVersion.S3_BUCKET_INFO_WITH_VOLUME_CONTEXT.isSupportedBy(omVersion);
+ RpcClient rpcClient = new RpcClient(conf, null) {
+ @Override
+ protected OmTransport createOmTransport(String omServiceId) {
+ return new MockOmTransport() {
+ @Override
+ public OMResponse submitRequest(OMRequest payload) throws IOException {
+ requests.add(payload);
+ switch (payload.getCmdType()) {
+ case ServiceList:
+ return OMResponse.newBuilder()
+ .setCmdType(Type.ServiceList)
+ .setStatus(Status.OK)
+ .setServiceListResponse(ServiceListResponse.newBuilder()
+ .addServiceInfo(ServiceInfo.newBuilder()
+ .setNodeType(NodeType.OM)
+ .setHostname("om")
+ .setOMVersion(omVersion.serialize())))
+ .build();
+ case GetS3VolumeContext:
+ OMResponse volumeResponse = super.submitRequest(OMRequest.newBuilder(payload)
+ .setCmdType(Type.InfoVolume)
+ .setInfoVolumeRequest(InfoVolumeRequest.newBuilder().setVolumeName(TENANT_VOLUME))
+ .build());
+ return OMResponse.newBuilder()
+ .setCmdType(Type.GetS3VolumeContext)
+ .setStatus(Status.OK)
+ .setGetS3VolumeContextResponse(GetS3VolumeContextResponse.newBuilder()
+ .setVolumeInfo(volumeResponse.getInfoVolumeResponse().getVolumeInfo())
+ .setUserPrincipal(USER_PRINCIPAL))
+ .build();
+ case InfoBucket:
+ if (resolvesS3Context && payload.getAssumeS3Context()) {
+ OMResponse bucketResponse = super.submitRequest(OMRequest.newBuilder(payload)
+ .setInfoBucketRequest(InfoBucketRequest.newBuilder(payload.getInfoBucketRequest())
+ .setVolumeName(TENANT_VOLUME))
+ .build());
+ return OMResponse.newBuilder(bucketResponse).setUserPrincipal(USER_PRINCIPAL).build();
+ }
+ return super.submitRequest(payload);
+ default:
+ return super.submitRequest(payload);
+ }
+ }
+ };
+ }
+
+ @Nonnull
+ @Override
+ protected XceiverClientFactory createXceiverClientFactory(ServiceInfoEx serviceInfo) {
+ return new MockXceiverClientFactory();
+ }
+ };
+ OzoneClient client = new OzoneClient(conf, rpcClient);
+ client.getObjectStore().createVolume(TENANT_VOLUME);
+ client.getObjectStore().getVolume(TENANT_VOLUME).createBucket(BUCKET);
+ requests.clear();
+ rpcClient.setThreadLocalS3Auth(new S3Auth("unused1", "unused2", ACCESS_ID, ACCESS_ID));
+ return client;
+ }
+
+ @Test
+ public void resolvesS3VolumeInInfoBucket() throws IOException {
+ try (OzoneClient client = newClient(OzoneManagerVersion.S3_BUCKET_INFO_WITH_VOLUME_CONTEXT)) {
+ OzoneBucket bucket = client.getProxy().getS3BucketDetails(BUCKET);
+
+ assertEquals(TENANT_VOLUME, bucket.getVolumeName());
+ assertEquals(BUCKET, bucket.getName());
+ assertThat(requests).extracting(OMRequest::getCmdType).containsExactly(Type.InfoBucket);
+ assertTrue(requests.get(0).getAssumeS3Context());
+ assertEquals(USER_PRINCIPAL, client.getProxy().getThreadLocalS3Auth().getUserPrincipal());
+ }
+ }
+
+ @Test
+ public void fallsBackToS3VolumeContextForOlderOm() throws IOException {
+ try (OzoneClient client = newClient(OzoneManagerVersion.GET_FILE_STATUS_REJECTS_OBS)) {
Review Comment:
nit: Could we cover all versions here, like in #11449? A gate on 102 would still pass.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4332247684
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -499,6 +499,7 @@ message OMResponse {
optional StartFinalizeUpgradeResponse startFinalizeUpgradeResponse = 159;
optional QueryUpgradeStatusResponse queryUpgradeStatusResponse = 160;
optional CompleteFinalizeUpgradeResponse completeFinalizeUpgradeResponse = 161;
+ optional string userPrincipal = 162;
Review Comment:
I'll decide to place it in OMResponse. For a few reasons:
- Symmetry: #11449 put the flag on OMRequest. Keeping both the request flag and response principal at the envelope level is consistent.
- Reusability: Other write-path PRs (PutObject, MPU) can reuse this field directly without modifying individual response protos.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub) via issues" <is...@ozone.apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4440304870
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -3162,7 +3172,7 @@ public Map<String, String> getObjectTagging(
throw new IOException("OzoneManager does not support S3 object tagging API");
}
- boolean assumeS3Context = volumeName == null && resolveS3VolumeOnOm;
+ boolean assumeS3Context = volumeName == null;
Review Comment:
Thanks @rich7420 for reviewing. You're right. Updated, RpcClient will fallback 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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub) via issues" <is...@ozone.apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4440525024
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getBucketDetails(null, name);
Review Comment:
Updated
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub) via issues" <is...@ozone.apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4431101785
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -3162,7 +3172,7 @@ public Map<String, String> getObjectTagging(
throw new IOException("OzoneManager does not support S3 object tagging API");
}
- boolean assumeS3Context = volumeName == null && resolveS3VolumeOnOm;
+ boolean assumeS3Context = volumeName == null;
Review Comment:
The version check is already handled on the `S3ClientProtocol` side before calling this method. It checks whether the OM supports it via `resolveVolumeName` and falls back if 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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4334708135
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java:
##########
@@ -103,6 +103,8 @@ public void setup() {
S3GatewayMetrics.create(conf);
when(client.getProxy()).thenReturn(clientProtocol);
when(objectStore.getClientProxy()).thenReturn(clientProtocol);
+ when(clientProtocol.getS3BucketDetails(anyString())).thenReturn(bucket);
Review Comment:
nit: Could we drop the `volume.getBucket(...)` stubs below too? They're no longer hit.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java:
##########
@@ -1045,4 +1057,77 @@ public void getKeyInfoServesObjectStoreLayoutWhenNotRequested(boolean assumeS3Co
verify(keyManager).getKeyInfo(any(OmKeyArgs.class), any(ResolvedBucket.class), any());
}
+
+ @Test
+ public void getBucketInfoResolvesS3VolumeWhenAssumeS3Context() throws Exception {
+ BucketManager bucketManager = mock(BucketManager.class);
+ OmMetadataReader reader = mock(OmMetadataReader.class);
+ OMMetrics metrics = mock(OMMetrics.class);
+ OzoneManager ozoneManager = newBucketInfoOzoneManager(bucketManager, reader, metrics);
+ OmBucketInfo bucket = bucketInfo(TENANT_VOLUME_NAME);
+ when(bucketManager.getBucketInfo(TENANT_VOLUME_NAME, BUCKET_NAME)).thenReturn(bucket);
+
+ BucketInfoWithVolumeContext result = ozoneManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME, true);
+
+ assertSame(bucket, result.getBucketInfo());
+ assertEquals(Optional.of("user"), result.getUserPrincipal());
+ verify(ozoneManager).getS3VolumeContext(true);
+ verify(reader).checkAcls(BUCKET, OZONE, READ, TENANT_VOLUME_NAME, BUCKET_NAME, null);
+ verify(metrics).incNumBucketInfos();
+ }
+
+ @Test
+ public void getBucketInfoUsesRequestedVolumeWithoutAssumeS3Context() throws Exception {
+ BucketManager bucketManager = mock(BucketManager.class);
+ OmMetadataReader reader = mock(OmMetadataReader.class);
+ OzoneManager ozoneManager = newBucketInfoOzoneManager(bucketManager, reader, mock(OMMetrics.class));
+ OmBucketInfo bucket = bucketInfo(VOLUME_NAME);
+ when(bucketManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME)).thenReturn(bucket);
+
+ BucketInfoWithVolumeContext result = ozoneManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME, false);
+
+ assertSame(bucket, result.getBucketInfo());
+ assertFalse(result.getUserPrincipal().isPresent());
+ verify(ozoneManager, never()).getS3VolumeContext(anyBoolean());
+ verify(reader).checkAcls(BUCKET, OZONE, READ, VOLUME_NAME, BUCKET_NAME, null);
+ }
+
+ @Test
+ public void getBucketInfoReturnsBucketNotFoundWithoutVolumeCheck() throws Exception {
Review Comment:
nit: Could we drop this test? It only asserts what its own mock throws.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/BucketInfoWithVolumeContext.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.helpers;
+
+import java.io.IOException;
+import java.util.Optional;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
+
+/**
+ * Wrapper class to return BucketInfo and optionally the S3 user principal.
+ */
+public final class BucketInfoWithVolumeContext {
Review Comment:
nit: Since it no longer holds volume args, maybe `BucketInfoWithS3Context` fits better?
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chihsuan (via GitHub) via issues" <is...@ozone.apache.org>.
chihsuan commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4431181815
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -3162,7 +3172,7 @@ public Map<String, String> getObjectTagging(
throw new IOException("OzoneManager does not support S3 object tagging API");
}
- boolean assumeS3Context = volumeName == null && resolveS3VolumeOnOm;
+ boolean assumeS3Context = volumeName == null;
Review Comment:
Thanks, agreed on S3G! I meant non-S3G clients. They don't go through `S3ClientProtocol`, so with an older OM, they may read from the wrong volume? For example, in an edge case where the S3 volume is renamed, the older OM still looks in s3v.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "rich7420 (via GitHub) via issues" <is...@ozone.apache.org>.
rich7420 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4432498397
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -3162,7 +3172,7 @@ public Map<String, String> getObjectTagging(
throw new IOException("OzoneManager does not support S3 object tagging API");
}
- boolean assumeS3Context = volumeName == null && resolveS3VolumeOnOm;
+ boolean assumeS3Context = volumeName == null;
Review Comment:
I also reproduced this in `getBucketDetails` with a non-S3G client and a mock older OM: it returns the bucket from `s3v` even when `GetS3VolumeContext` points elsewhere. Could we keep the fallback for direct callers too?
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getBucketDetails(null, name);
Review Comment:
nit: Could we reuse the cached volume here? `BucketAclHandler` calls `getVolume()` first, so the old-OM fallback ends up calling `GetS3VolumeContext` a second time.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4312648975
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneManagerVersion.java:
##########
@@ -73,6 +73,8 @@ public enum OzoneManagerVersion implements ComponentVersion {
+ "server-side, so file system clients no longer need the "
+ "client-side InfoBucket layout check for getFileChecksum"),
ZDU(100, "OzoneManager version that supports zero downtime upgrade"),
+ S3_BUCKET_INFO_WITH_VOLUME_CONTEXT(101,
Review Comment:
Updated
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "Russole (via GitHub)" <gi...@apache.org>.
Russole commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4327209444
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java:
##########
@@ -342,6 +342,15 @@ void checkBucketAccess(String volumeName, String bucketName)
OzoneBucket getBucketDetails(String volumeName, String bucketName)
throws IOException;
+ /**
+ * Returns Bucket Information.
Review Comment:
nit: Could the Javadoc clarify that this resolves the S3 volume from the current S3 context?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3288,19 +3289,33 @@ private OmBucketInfo getResolvedSourceBucket(
@Override
public OmBucketInfo getBucketInfo(String volume, String bucket)
throws IOException {
+ return getBucketInfo(volume, bucket, false).getBucketInfo();
+ }
+
+ @Override
+ public BucketInfoWithVolumeContext getBucketInfo(String volume, String bucket, boolean assumeS3Context)
+ throws IOException {
+ String resolvedVolumeName = volume;
+ BucketInfoWithVolumeContext.Builder builder = BucketInfoWithVolumeContext.newBuilder();
+ if (assumeS3Context) {
+ S3VolumeContext context = getS3VolumeContext(true);
+ resolvedVolumeName = context.getOmVolumeArgs().getVolume();
+ builder.setUserPrincipal(context.getUserPrincipal());
+ }
+
Review Comment:
nit: remove trailing whitespace
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6128118510
proto.lock change can ignore. We usually do that during a release.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4422793320
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -3162,7 +3172,7 @@ public Map<String, String> getObjectTagging(
throw new IOException("OzoneManager does not support S3 object tagging API");
}
- boolean assumeS3Context = volumeName == null && resolveS3VolumeOnOm;
+ boolean assumeS3Context = volumeName == null;
Review Comment:
I think this is a bug caused by https://github.com/apache/ozone/pull/11574#issue-5892754928 .
`assumeS3Context` shouldn't only be passed when S3_AUTH_CHECK is true. My understanding is that as long as this feature (S3_VOLUME_CONTEXT_RESOLUTION) is supported, null should always be passed (i.e. assumeS3Context = true). The server side will then handle the resolution—falling back to the default s3v for non-multitenant cases, or mapping to the corresponding tenant volume if multitenancy is enabled.
cc @chihsuan @Russole WDYT
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "rich7420 (via GitHub) via issues" <is...@ozone.apache.org>.
rich7420 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4479287528
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getBucketDetails(volume != null ? volume.getName() : null, name);
Review Comment:
nit: Could ListParts, DeleteObject and PUT/DELETE object tagging use `getVolumeName()` too? With an expected-owner header and OM advertising 101, each calls `GetS3VolumeContext` twice (base: once).
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub) via issues" <is...@ozone.apache.org>.
chungen0126 merged PR #11479:
URL: https://github.com/apache/ozone/pull/11479
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4301178188
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java:
##########
@@ -341,6 +341,15 @@ void checkBucketAccess(String volumeName, String bucketName)
OzoneBucket getBucketDetails(String volumeName, String bucketName)
throws IOException;
+ /**
+ * Returns Bucket Information.
+ * @param bucketName Name of the Bucket.
+ * @return OzoneBucket
+ * @throws IOException
+ */
+ OzoneBucket getS3BucketDetails(String bucketName)
Review Comment:
Just a heads-up, #11458 also adds `getS3BucketDetails`. If it merges first, could we reuse its implementation and drop the duplicate here?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneManagerVersion.java:
##########
@@ -73,6 +73,8 @@ public enum OzoneManagerVersion implements ComponentVersion {
+ "server-side, so file system clients no longer need the "
+ "client-side InfoBucket layout check for getFileChecksum"),
ZDU(100, "OzoneManager version that supports zero downtime upgrade"),
+ S3_BUCKET_INFO_WITH_VOLUME_CONTEXT(101,
Review Comment:
A gentle reminder, master already took 101 in the meantime. Might be worth double-checking the next free number before merging.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -199,6 +199,7 @@ message OMRequest {
optional LayoutVersion layoutVersion = 6;
optional ReadConsistencyHint readConsistencyHint = 7;
+ optional bool assumeS3Context = 8;
Review Comment:
Heads-up rather than a change request: #11458 puts `assumeS3Context` on `InfoBucketRequest` instead. I left a comment there to align on one place, would love your thoughts too.
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-sts-multitenant.robot:
##########
@@ -269,7 +269,7 @@ Verify Limited-Scoped Token Accesses
Get Object Should Fail ${TENANT_ONE_LINKED_BUCKET} ${ANOTHER_BUCKET_TESTFILE} AccessDenied
Put Object Should Fail ${TENANT_ONE_LINKED_BUCKET} ${ANOTHER_BUCKET_TESTFILE} AccessDenied
Get Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET} ${ANOTHER_BUCKET_TESTFILE} NoSuchBucket
- Put Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET} ${ANOTHER_BUCKET_TESTFILE} AccessDenied
+ Put Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET} ${ANOTHER_BUCKET_TESTFILE} NoSuchBucket
Review Comment:
Just curious, is the switch to NoSuchBucket here intentional? I found it comes from skipping the volume check with `getS3VolumeContext(true)`, while #11458 keeps that check and still returns AccessDenied. Either way works for me, as long as we pick the same one.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getS3BucketDetails(name);
} else {
Preconditions.assertEquals(bucket.getName(), name, "Multiple buckets in one request are not supported");
}
return bucket;
}
+ /**
+ * Returns the S3 volume name, reusing the bucket already looked up in this request to avoid another call.
+ */
+ String getVolumeName() throws IOException {
Review Comment:
nit: Could we verify `getS3Volume()` isn't called once the bucket is cached? Reverting this still passes the tests.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "adoroszlai (via GitHub) via issues" <is...@ozone.apache.org>.
adoroszlai commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6664301865
Proposed fix: https://github.com/adoroszlai/ozone/commit/18f6aa9e09ebc761e4158b4927a1f439ab5949e3
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "adoroszlai (via GitHub) via issues" <is...@ozone.apache.org>.
adoroszlai commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6663908878
@chungen0126 @Gargi-jais11 This causes failures in `TestInitiateMultipartUpload` test cases added yesterday (#11922, #11900), which were not run as part of this PR's CI. I'm checking it, but if you can fix it faster, please feel free to take HDDS-17713.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4324068601
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3288,19 +3289,33 @@ private OmBucketInfo getResolvedSourceBucket(
@Override
public OmBucketInfo getBucketInfo(String volume, String bucket)
throws IOException {
+ return getBucketInfo(volume, bucket, false).getBucketInfo();
+ }
+
+ @Override
+ public BucketInfoWithVolumeContext getBucketInfo(String volume, String bucket, boolean assumeS3Context)
+ throws IOException {
+ String resolvedVolumeName = volume;
+ BucketInfoWithVolumeContext.Builder builder = BucketInfoWithVolumeContext.newBuilder();
+ if (assumeS3Context) {
+ S3VolumeContext context = getS3VolumeContext(true);
Review Comment:
Updated. Add tests to TestOmMetadataReader.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4326047940
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMMetadataReader.java:
##########
@@ -1045,4 +1057,79 @@ public void getKeyInfoServesObjectStoreLayoutWhenNotRequested(boolean assumeS3Co
verify(keyManager).getKeyInfo(any(OmKeyArgs.class), any(ResolvedBucket.class), any());
}
+
+ @Test
+ public void getBucketInfoResolvesS3VolumeWhenAssumeS3Context() throws Exception {
+ BucketManager bucketManager = mock(BucketManager.class);
+ OmMetadataReader reader = mock(OmMetadataReader.class);
+ OMMetrics metrics = mock(OMMetrics.class);
+ OzoneManager ozoneManager = newBucketInfoOzoneManager(bucketManager, reader, metrics);
+ OmBucketInfo bucket = bucketInfo(TENANT_VOLUME_NAME);
+ when(bucketManager.getBucketInfo(TENANT_VOLUME_NAME, BUCKET_NAME)).thenReturn(bucket);
+
+ BucketInfoWithVolumeContext result = ozoneManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME, true);
+
+ assertSame(bucket, result.getBucketInfo());
+ assertEquals(Optional.of("user"), result.getUserPrincipal());
+ verify(ozoneManager).getS3VolumeContext(true);
+ verify(reader).checkAcls(BUCKET, OZONE, READ, TENANT_VOLUME_NAME, BUCKET_NAME, null);
+ verify(reader, never()).checkAcls(eq(VOLUME), any(), any(), any(), any(), any());
+ verify(metrics).incNumBucketInfos();
+ }
+
+ @Test
+ public void getBucketInfoUsesRequestedVolumeWithoutAssumeS3Context() throws Exception {
+ BucketManager bucketManager = mock(BucketManager.class);
+ OmMetadataReader reader = mock(OmMetadataReader.class);
+ OzoneManager ozoneManager = newBucketInfoOzoneManager(bucketManager, reader, mock(OMMetrics.class));
+ OmBucketInfo bucket = bucketInfo(VOLUME_NAME);
+ when(bucketManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME)).thenReturn(bucket);
+
+ BucketInfoWithVolumeContext result = ozoneManager.getBucketInfo(VOLUME_NAME, BUCKET_NAME, false);
+
+ assertSame(bucket, result.getBucketInfo());
+ assertFalse(result.getUserPrincipal().isPresent());
+ verify(ozoneManager, never()).getS3VolumeContext(anyBoolean());
+ verify(reader).checkAcls(BUCKET, OZONE, READ, VOLUME_NAME, BUCKET_NAME, null);
+ }
+
+ @Test
+ public void getBucketInfoReturnsBucketNotFoundWithoutVolumeCheck() throws Exception {
Review Comment:
nit: Would it be worth dropping the `never()` checks on VOLUME `checkAcls`? `getBucketInfo` doesn't check volume ACLs before or after this change, so they can't fail.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -499,6 +499,7 @@ message OMResponse {
optional StartFinalizeUpgradeResponse startFinalizeUpgradeResponse = 159;
optional QueryUpgradeStatusResponse queryUpgradeStatusResponse = 160;
optional CompleteFinalizeUpgradeResponse completeFinalizeUpgradeResponse = 161;
+ optional string userPrincipal = 162;
Review Comment:
Just curious, are we planning to put `userPrincipal` on `OMResponse` for the other subtasks too? `GetKeyInfoResponse` has it in its own response, so it'd be good to pick one way before this ships.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4426272317
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -284,9 +284,6 @@ public RpcClient(ConfigurationSource conf, String omServiceId)
dtService = omTransport.getDelegationTokenService();
ServiceInfoEx serviceInfoEx = ozoneManagerClient.getServiceInfo();
omVersion = getOmVersion(serviceInfoEx);
- resolveS3VolumeOnOm = conf.getBoolean(S3Auth.S3_AUTH_CHECK, false)
- && OzoneManagerVersion.S3_VOLUME_CONTEXT_RESOLUTION
- .isSupportedBy(omVersion);
Review Comment:
Are you referring to #11789? If so, I agree reading omVersion per call seems safer.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4426262733
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -3162,7 +3172,7 @@ public Map<String, String> getObjectTagging(
throw new IOException("OzoneManager does not support S3 object tagging API");
}
- boolean assumeS3Context = volumeName == null && resolveS3VolumeOnOm;
+ boolean assumeS3Context = volumeName == null;
Review Comment:
Thanks for raising this! @chungen0126 From what I can see, only S3G passes a `null` volume, which is why I thought the gate was intentional. But since OM already resolves the default S3 volume for clients without an S3 identity, dropping it sounds good to me.
I noticed the OM version check is also dropped here, though. I think we should keep 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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub) via issues" <is...@ozone.apache.org>.
chungen0126 commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6578746193
@jojochuang Would you like to 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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "rich7420 (via GitHub) via issues" <is...@ozone.apache.org>.
rich7420 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4444258222
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1220,13 +1217,14 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
if (S3Owner.hasBucketOwnershipVerificationConditions(getHeaders())) {
final String sourceBucketOwner = runWithS3ActionString(
- "GetObject", () -> volume.getBucket(sourceBucket).getOwner());
+ "GetObject", () -> getClientProtocol().getBucketDetails(null, sourceBucket).getOwner());
Review Comment:
nit: Could we reuse the destination volume here and in UploadPartCopy? Tested full requests with owner headers at apparent version 101: `GetS3VolumeContext` calls drop from two to one.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getBucketDetails(volume != null ? volume.getName() : null, name);
Review Comment:
nit: Could the remaining handlers use `getVolumeName()` too? DELETE bucket with an expected-owner header resolves the volume twice on older OMs.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "rich7420 (via GitHub) via issues" <is...@ozone.apache.org>.
rich7420 commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4444258209
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getBucketDetails(volume != null ? volume.getName() : null, name);
Review Comment:
nit: I think could the remaining handlers use `getVolumeName()` too? DELETE bucket with an expected-owner header resolves the volume twice on older OMs.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "chungen0126 (via GitHub) via issues" <is...@ozone.apache.org>.
chungen0126 commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6664378079
Thanks @adoroszlai for catching this! The proposed commit looks good to me.
--
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-16681. S3G: Avoid calling getS3Volume by using getBucketDetails API [ozone]
Posted by "rich7420 (via GitHub) via issues" <is...@ozone.apache.org>.
rich7420 commented on PR #11479:
URL: https://github.com/apache/ozone/pull/11479#issuecomment-6664188451
@adoroszlai I’ll take a look at HDDS-17713 and the failing tests.
--
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