You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2023/01/18 18:14:17 UTC
[GitHub] [ozone] xichen01 opened a new pull request, #4188: HDDS-7791. Support display and persist owner info to DB
xichen01 opened a new pull request, #4188:
URL: https://github.com/apache/ozone/pull/4188
## What changes were proposed in this pull request?
current support persists the owner info to DB and reads the owner info from DB,
the owner will be the user that creates the Object, for `ozone fs` and `ozone sh key`, it will be the system user that executes the command, for `aws s3` it will be `AWS Access Key ID`
### supported command:
#### write:
support `ozone fs` and `ozone sh key` and `aws s3/s3api` related Object creation commands. will present owner info to DB when creating an object.
#### read (support display real owner info):
if the `owner `field is `null` will fall back to the old logic, use the OS login user to fill the `owner `field. This is possible during an upgrade from the previous version
```
ozone fs -ls
ozone fs stat
ozone sh key ls
ozone sh key info
aws s3api list-objects
```
`aws s3api get-object-acl` not support current, will be support next
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7791
## How was this patch tested?
1. create some key/file/dir through `ozone fs`, `ozone sh key`, `aws s3`
environment
```shell
[pony.chen@linux /root/ozone]% aws configure
AWS Access Key ID [****************chen]:
// xxxxxx
[pony.chen@linux /root/ozone]% whoami
pony.chen
```
```shell
[pony.chen@linux /root/ozone]% ozone sh key put /s3v/bucket1/putfile ~/testfile.img
[pony.chen@linux /root/ozone]% ozone fs -mkdir ofs://localhost/s3v/bucket1/dir1
[pony.chen@linux /root/ozone]% ozone fs -touch ofs://localhost/s3v/bucket1/file1
[pony.chen@linux /root/ozone]% aws s3 --endpoint http://localhost:9878 cp ~/testfile s3://bucket1/s3file
upload: ../../testfile to s3://bucket1/s3file
[pony.chen@linux /root/ozone]%
```
2. switch to another user and display `key/file` info about `owner`, the `owner` info will be real info created above
environment.
You can see that the owner is `pony.chen` instead of `root` (before this PR it will be root)
```
linux:~ root# whoami
root
```
```shell
linux:~ root# ozone sh key ls s3v/bucket1 | grep "name\|owner"
"name" : "dir1/",
"ownerName" : "pony.chen",
"name" : "file1",
"ownerName" : "pony.chen",
"name" : "putfile",
"ownerName" : "pony.chen",
"name" : "s3file",
"ownerName" : "pony.chen",
linux:~ root#
linux:~ root# ozone fs -ls ofs://localhost/s3v/bucket1/
Found 4 items
drwxrwxrwx - pony.chen root 0 2022-12-06 15:06 ofs://localhost/s3v/bucket1/dir1
-rw-rw-rw- 3 pony.chen root 0 2022-12-06 15:06 ofs://localhost/s3v/bucket1/file1
-rw-rw-rw- 3 pony.chen root 1048576 2022-12-06 15:05 ofs://localhost/s3v/bucket1/putfile
-rw-rw-rw- 3 pony.chen root 1048576 2022-12-06 15:07 ofs://localhost/s3v/bucket1/s3file
linux:~ root#
linux:~ root# aws s3api --endpoint http://localhost:9878 list-objects --bucket bucket1
None
CONTENTS 2022-12-06T07:06:07.468Z dir1/ 2022-12-06T07:06:07.468000+00:00 0 STANDARD
OWNER pony.chen pony.chen
CONTENTS 2022-12-06T07:06:22.887Z file1 2022-12-06T07:06:22.887000+00:00 0 STANDARD
OWNER pony.chen pony.chen
CONTENTS 2022-12-06T07:05:39.955Z putfile 2022-12-06T07:05:39.955000+00:00 1048576 STANDARD
OWNER pony.chen pony.chen
CONTENTS 2022-12-06T07:07:00.776Z s3file 2022-12-06T07:07:00.776000+00:00 1048576 STANDARD
OWNER pony.chen pony.chen
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #4188: HDDS-7791. Support display and persist owner info to DB
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-1421126306
I just had some minor comments and questions posted here and on HDDS-7577. I probably won't have time to review this PR, at least not any time soon.
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559090113
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -66,13 +66,18 @@ Verify Bucket 1 Owner
${result} = Execute ozone sh bucket info /tenantone/bucket-test1 | jq -r '.owner'
Should Be Equal ${result} testuser
-Put, get and delete a key in the tenant bucket
+Put a key in the tenant bucket
Execute echo "Randomtext" > /tmp/testfile
Execute and checkrc aws s3api --endpoint-url ${S3G_ENDPOINT_URL} put-object --bucket bucket-test1 --key mykey --body /tmp/testfile 0
+
+Verify Object Owner
+ ${result} = Execute ozone sh key info /tenantone/bucket-test1/mykey | jq -r '.owner'
+ Should Be Equal ${result} testuser
Review Comment:
I am fine with this addition. Just curious, IIUC because Ozone was faking key owner before, this check would still pass without key owner right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #4188: HDDS-7791. Support display and persist owner info to DB
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1085720123
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java:
##########
@@ -45,6 +45,10 @@ public class OzoneKey {
* Name of the Key.
*/
private final String name;
+ /**
+ * Name of the Key owner.
+ */
+ private final String ownerName;
Review Comment:
In `OzoneVolume` and `OzoneBucket` this field is called `owner`. We should probably use the same name throughout this change.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketList.java:
##########
@@ -108,6 +109,37 @@ public void listSubDir() throws OS3Exception, IOException {
}
+ @Test
+ public void listObjectOwner() throws OS3Exception, IOException {
+
+ UserGroupInformation user1 = UserGroupInformation
+ .createUserForTesting("user1", new String[] {"user1"});
+ UserGroupInformation user2 = UserGroupInformation
+ .createUserForTesting("user2", new String[] {"user2"});
+
+ BucketEndpoint getBucket = new BucketEndpoint();
+ OzoneClient client = new OzoneClientStub();
+ client.getObjectStore().createS3Bucket("b1");
+ OzoneBucket bucket = client.getObjectStore().getS3Bucket("b1");
+
+ UserGroupInformation.setLoginUser(user1);
Review Comment:
In an actual cluster wouldn't the "logged in" user on s3 gateway be s3 gateway itself, not the user making the request? I think what we need to test that s3g is forwarding the user info it was given from the s3 client and OM is storing it correctly.
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
Arguably we could just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner. This way we don't bloat every single key with this extra owner string in its proto message. This can also make `chown` on the bucket/dir non-recursive (much better performance).
Also when a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well (unless we add a lengthy step during OM upgrade to add owner to all existing keys which I don't think is the best idea).
But on the other hand I get that the the owner field in key info is required for "sticky bit" behavior to work with properly Ranger authorizer (previously there is a workaround, HDDS-9701).
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559099986
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -1004,7 +1004,7 @@ private FileStatusAdapter toFileStatusAdapter(OzoneFileStatus status,
keyInfo.getModificationTime(),
keyInfo.getModificationTime(),
status.isDirectory() ? (short) 00777 : (short) 00666,
- owner,
+ StringUtils.defaultIfEmpty(keyInfo.getOwnerName(), owner),
owner,
Review Comment:
Same. `group` should also have the same treatment?
```suggestion
StringUtils.defaultIfEmpty(keyInfo.getOwnerName(), owner),
StringUtils.defaultIfEmpty(keyInfo.getOwnerName(), owner),
```
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1558981420
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Thanks @xichen01 for explanation. Got it. So:
1. The current PR hasn't changed the ACL part yet. Thus it doesn't really use the new **key owner** field (yet) during ACL checks.
2. In this PR, the sole purpose of key owner field as of now is to be able to write and read the owner field.
Is my understanding correct so far?
Now I do have a follow-up question:
What is the plan on using the key owner field for ACL checks later on? (Previously I was incorrectly assuming this PR already does what Aswin's bucket owner PR does to ACL checks)
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-2073651636
Thanks @xichen01 for merging master to this PR.
But there is build failure after the conflict resolution. Would you 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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-2080343753
Thanks @xichen01 for working on this!
Also thanks @sumitagrawl @errose28 for the reviews.
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1554990590
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -736,17 +736,24 @@ private static void verifySpaceQuota(long quota) throws OMException {
* @return listOfAcls
* */
private List<OzoneAcl> getAclList() {
+ UserGroupInformation realUserInfo = getRealUserInfo();
+ return OzoneAclUtil.getAclList(realUserInfo.getUserName(),
+ realUserInfo.getGroupNames(), userRights, groupRights);
+ }
+
+ /**
+ * Helper function to get the actual operating user.
+ *
+ * @return listOfAcls
+ * */
+ private UserGroupInformation getRealUserInfo() {
+ // After HDDS-5881 the user will not be different,
+ // as S3G uses single RpcClient. So we should be checking thread-local
+ // S3Auth and use it during proxy.
if (ozoneManagerClient.getThreadLocalS3Auth() != null) {
- UserGroupInformation aclUgi =
- UserGroupInformation.createRemoteUser(
- ozoneManagerClient.getThreadLocalS3Auth().getAccessID());
- return OzoneAclUtil.getAclList(
- aclUgi.getUserName(),
- aclUgi.getGroupNames(),
- userRights, groupRights);
+ return UserGroupInformation.createRemoteUser(getThreadLocalS3Auth().getUserPrincipal());
}
Review Comment:
updated, the result of the `UserGroupInformation.createRemoteUser()` will be cached for each `RPCClient`
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
Arguably we could just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner. This way we don't bloat every single key with this extra owner string in its proto message.
Cons of setting `owner` for every single new key:
1. Slight perf hit for every new key/dir creation.
2. This also implies `chown` on a bucket/dir would be recursive by default. Makes `chown` much slower. (Where in the case of not setting `owner` for every key, we don't need to recurse by default because the sub-keys and sub-dirs should just inherit the parent owner)
3. Because `owner` is a proto string, it can be wasting quite a lot of space repeating the same name string. (numeric user ID mapping can solve that)
4. When a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well. Unless we add a lengthy step during OM upgrade to add owner to all existing keys, which I don't think is the best idea).
Pro:
1. On the other hand I get that the the owner field in key info is required for "sticky bit" behavior to work properly with Ranger authorizer (previously this requires a workaround, HDDS-9701).
2. Writer user is not necessarily the same as the parent owner.
Discussions:
1. Is there some other reasons we must set this owner field for new keys and dirs?
3. How much perf hit would there actually be to add this owner field for every key? Try this in a micro benchmark with and without S3?
4. Is it reasonable to add an config to allow not setting owner field for new keys and dirs by default? Or, only add owner field if the parent dir have the "sticky bit".
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1561142532
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1415,7 +1415,7 @@ long generateDiffReport(
*/
private boolean isKeyModified(OmKeyInfo fromKey, OmKeyInfo toKey) {
return !fromKey.isKeyInfoSame(toKey,
- false, false, false, false)
+ false, false, false, false, false)
Review Comment:
Makes sense, I see the ACL was compared, and the owner may like ACL, how do you think? @hemantk-12 @swamirishi @smengcl
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559090113
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -66,13 +66,18 @@ Verify Bucket 1 Owner
${result} = Execute ozone sh bucket info /tenantone/bucket-test1 | jq -r '.owner'
Should Be Equal ${result} testuser
-Put, get and delete a key in the tenant bucket
+Put a key in the tenant bucket
Execute echo "Randomtext" > /tmp/testfile
Execute and checkrc aws s3api --endpoint-url ${S3G_ENDPOINT_URL} put-object --bucket bucket-test1 --key mykey --body /tmp/testfile 0
+
+Verify Object Owner
+ ${result} = Execute ozone sh key info /tenantone/bucket-test1/mykey | jq -r '.owner'
+ Should Be Equal ${result} testuser
Review Comment:
I am fine with this addition. Just curious, IIUC because Ozone was faking key owner before, this check would still pass without key owner right? Or `sh key` result would not return fake key owner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on pull request #4188: HDDS-7791. Support display and persist owner info to DB
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-1419988333
@errose28 can you please review the latest pull?
@xichen01 Can you please look into the workflow failures?
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550048016
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -2026,10 +2045,12 @@ public OzoneFileStatus getOzoneFileStatus(String volumeName,
@Override
public void createDirectory(String volumeName, String bucketName,
String keyName) throws IOException {
+ String ownerName = getRealUserInfo().getShortUserName();
OmKeyArgs keyArgs = new OmKeyArgs.Builder().setVolumeName(volumeName)
.setBucketName(bucketName)
.setKeyName(keyName)
.setAcls(getAclList())
+ .setOwnerName(ownerName)
Review Comment:
My previous [comment](https://github.com/apache/ozone/pull/4188/files#r1550044937) also applies here, as well as to others like `createFile()`.
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559277107
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
> The current PR hasn't changed the ACL part yet. Thus it doesn't really use the new key owner field (yet) during ACL checks.
Right, this PR does not change the ACL check and does not affect the results of the current ACL check because the current ACL check does not even know the key owner existing.
> In this PR, the sole purpose of key owner field as of now is to be able to write and read the owner field.
Right.
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-2021919958
@smengcl 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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-1956382067
> > @smengcl @errose28 Is it sufficient for S3 to use the AWS `Access ID` directly as the `Owner` of the Object (one-to-one `Access ID` and `Owner` of the Object)
>
> @xichen01 Uh preferrably not. `accessId` should be mapped to a user name (similar idea to `ugi.getShortUserName()`) before it can be used in the `owner` field:
>
> https://github.com/apache/ozone/blob/cce2f969a85323441c476aaeaf27d45b081b0c2f/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java#L417-L420
@sumitagrawl. Thank for your suggestion.
How to map the `accessId` to a `user name`. Are you saying we should use `Displayname` as the `owner` of the Ozone key?
The AWS S3 Owner is the `ID` and `Displayname`, while the Ozone Owner is simply a string.
```java
public class S3Owner {
public static final S3Owner
NOT_SUPPORTED_OWNER = new S3Owner("NOT-SUPPORTED", "Not Supported");
@XmlElement(name = "DisplayName")
private String displayName;
@XmlElement(name = "ID")
private String id;
}
```
--
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-7791. Support display and persist owner info to DB [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-1925791067
@smengcl @errose28 Is it sufficient for S3 to use the AWS `Access ID` directly as the `Owner` of the Object (one-to-one `Access ID` and `Owner` of the Object)
--
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-7791. Support display and persist owner info to DB [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-1925789516
@adoroszlai @smengcl @errose28 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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
I think it might actually better to just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner.
Also when a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well (unless we add a lengthy step during OM upgrade to add owner to all existing keys which I don't think is the best idea).
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
Cons:
1. Arguably we could just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner. This way we don't bloat every single key with this extra owner string in its proto message. This can also make `chown` on the bucket/dir non-recursive (much better performance).
2. Also when a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well (unless we add a lengthy step during OM upgrade to add owner to all existing keys which I don't think is the best idea).
Pro:
1. On the other hand I get that the the owner field in key info is required for "sticky bit" behavior to work with properly Ranger authorizer (previously there is a workaround, HDDS-9701).
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1558786330
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Hi @xichen01 , good to know that the perf isn't visibly degraded by this.
1. Back to the `chown` subcommand, the point of the problem I'm raising is that the behavior becomes inconsistent **before and after** an upgrade to an Ozone version with key ownership.
When this key owner addition gets in, and admin `chown` a directory in a bucket, because `chown` is non-recursive by default, the keys inside that dir would still have the old owner field set. This can break some existing Ozone Ranger ACLs that is previously set by admins. e.g. users can be denied access to some keys while in the previous Ozone version they would be allowed acesss, given the same Ranger ACL policy with `{owner}` permission set to ALLOW.
There should be a caveat after Ozone upgrade.
This at least deserves a mention in the **Release Notes**. Please add that to the Apache JIRA accordingly so that the release manager can collect and publish it.
2. Another relevant questions is: How are existing keys WITHOUT an owner field set going to behave after Ozone upgrade to one with key ownership support? Do they inherit parent's owner with the current implementation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xichen01 commented on a diff in pull request #4188: HDDS-7791. Support display and persist owner info to DB
Posted by GitBox <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1082183785
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
"add owner" has been added, I add this when fixed the ci test
--
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-7791. Support display and persist owner info to DB [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-1913250841
@xichen01 Do you plan to update this PR? There are lots of conflicts, unfortunately.
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1558983275
##########
hadoop-ozone/integration-test/src/test/resources/core-site.xml:
##########
@@ -20,5 +20,16 @@
<!-- Put site-specific property overrides in this file. -->
<configuration>
-
+ <property>
+ <name>hadoop.proxyuser.proxyuser.users</name>
+ <value>*</value>
+ </property>
+ <property>
+ <name>hadoop.proxyuser.proxyuser.groups</name>
+ <value>*</value>
+ </property>
+ <property>
+ <name>hadoop.proxyuser.proxyuser.hosts</name>
+ <value>*</value>
+ </property>
</configuration>
Review Comment:
Is this required for new added 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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559099498
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java:
##########
@@ -514,7 +514,7 @@ private FileStatusAdapter toFileStatusAdapter(OzoneFileStatus status,
keyInfo.getModificationTime(),
keyInfo.getModificationTime(),
status.isDirectory() ? (short) 00777 : (short) 00666,
- owner,
+ StringUtils.defaultIfEmpty(keyInfo.getOwnerName(), owner),
owner,
Review Comment:
`group` should also have the same treatment?
```suggestion
StringUtils.defaultIfEmpty(keyInfo.getOwnerName(), owner),
StringUtils.defaultIfEmpty(keyInfo.getOwnerName(), owner),
```
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
Arguably we could just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner. This way we don't bloat every single key with this extra owner string in its proto message.
Cons of setting `owner` for every single new key:
1. Slight perf hit for every new key/dir creation.
2. This also implies `chown` on a bucket/dir would be recursive by default. Makes `chown` much slower. (Where in the case of not setting `owner` for every key, we don't need to recurse by default because the sub-keys and sub-dirs should just inherit the parent owner)
3. Because `owner` is a proto string, it can be wasting quite a lot of space repeating the same name string. (numeric user ID mapping can solve that)
4. When a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well. Unless we add a lengthy step during OM upgrade to add owner to all existing keys, which I don't think is the best idea).
Pros:
1. On the other hand I get that the the owner field in key info is required for "sticky bit" behavior to work properly with Ranger authorizer (previously this requires a workaround, HDDS-9701).
2. Writer user is not necessarily the same as the parent owner.
Discussions:
1. Is there some other reasons we must set this owner field for new keys and dirs?
3. How much perf hit would there actually be to add this owner field for every key? Try this in a micro benchmark with and without S3?
4. Is it reasonable to add an config to allow not setting owner field for new keys and dirs by default? Or, only add owner field if the parent dir have the "sticky bit".
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1551354188
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
@smengcl Thank for your suggestion.
- Necessity for the Owner
"Owner" is a fundamental feature of file systems, including S3 and Filesystem, and is used to explicitly record who created the current Object. This is important for permissions management (Linux UGO and ACL Permission).
So it is necessary to store a real Owner for each Object. This is the only way to achieve better permissions control and so on.
- For the `chown` command
When we `chown` a directory, we do not recursively `chown` every file in the directory by default, unless the -R option is specified.
- For the performance
In fact, we ran a performance test after adding an Owner, and there was no observable performance degradation.
- For the owner configuration
Creating a file in Linux, or creating an Object via S3, doesn't have an option to create a file/object with an empty owner, so I think we probably don't need 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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-2072693283
> Thanks @xichen01 for the big patch. lgtm
>
> Only concern left from my side is the one regarding snapshot diff: [#4188 (comment)](https://github.com/apache/ozone/pull/4188#discussion_r1559096505)
Have modified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4188: HDDS-7791. Support display and persist owner info to DB
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1082114410
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Also Missing createStreamFile, createMultipartStremKey for add owner
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xichen01 commented on a diff in pull request #4188: HDDS-7791. Support display and persist owner info to DB
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1089167889
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java:
##########
@@ -45,6 +45,10 @@ public class OzoneKey {
* Name of the Key.
*/
private final String name;
+ /**
+ * Name of the Key owner.
+ */
+ private final String ownerName;
Review Comment:
I will change this, but my feedback may be slow, since I am still in holiday until 30th Jan.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketList.java:
##########
@@ -108,6 +109,37 @@ public void listSubDir() throws OS3Exception, IOException {
}
+ @Test
+ public void listObjectOwner() throws OS3Exception, IOException {
+
+ UserGroupInformation user1 = UserGroupInformation
+ .createUserForTesting("user1", new String[] {"user1"});
+ UserGroupInformation user2 = UserGroupInformation
+ .createUserForTesting("user2", new String[] {"user2"});
+
+ BucketEndpoint getBucket = new BucketEndpoint();
+ OzoneClient client = new OzoneClientStub();
+ client.getObjectStore().createS3Bucket("b1");
+ OzoneBucket bucket = client.getObjectStore().getS3Bucket("b1");
+
+ UserGroupInformation.setLoginUser(user1);
Review Comment:
For s3g, if you put an object, the object's owner will be the AWS "logged in" user, not the S3 Gateway user. this test only for `Ozone client`.
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
Arguably we could just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner. This way we don't bloat every single key with this extra owner string in its proto message.
Cons of setting `owner` for every single new key:
1. Slight perf hit for every new key/dir creation.
2. This also implies `chown` on a bucket/dir would be recursive by default. Makes `chown` much slower. (Where in the case of not setting `owner` for every key, we don't need to recurse by default because the sub-keys and sub-dirs should just inherit the parent owner)
3. Because `owner` is a proto string, it can be wasting quite a lot of space repeating the same name string. (numeric user ID mapping can solve that)
4. When a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well. Unless we add a lengthy step during OM upgrade to add owner to all existing keys, which I don't think is the best idea).
Pro:
1. On the other hand I get that the the owner field in key info is required for "sticky bit" behavior to work properly with Ranger authorizer (previously this requires a workaround, HDDS-9701).
Discussions:
1. Is there some other reasons we must set this owner field for new keys and dirs?
2. How much perf hit would there actually be to add this owner field for every key? Try this in a micro benchmark with and without S3?
3. Is it reasonable to add an config to allow not setting owner field for new keys and dirs by default? Or, only add owner field if the parent dir have the "sticky bit".
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
Arguably we could just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner. This way we don't bloat every single key with this extra owner string in its proto message.
Also when a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well (unless we add a lengthy step during OM upgrade to add owner to all existing keys which I don't think is the best idea).
But on the other hand I get that the the owner field in key info is required for "sticky bit" behavior to work with properly Ranger authorizer (previously there is a workaround, HDDS-9701).
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550048016
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -2026,10 +2045,12 @@ public OzoneFileStatus getOzoneFileStatus(String volumeName,
@Override
public void createDirectory(String volumeName, String bucketName,
String keyName) throws IOException {
+ String ownerName = getRealUserInfo().getShortUserName();
OmKeyArgs keyArgs = new OmKeyArgs.Builder().setVolumeName(volumeName)
.setBucketName(bucketName)
.setKeyName(keyName)
.setAcls(getAclList())
+ .setOwnerName(ownerName)
Review Comment:
My previous [comment](https://github.com/apache/ozone/pull/4188/files#r1550044937) also applies here, as well as others like `createFile()`.
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550048016
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -2026,10 +2045,12 @@ public OzoneFileStatus getOzoneFileStatus(String volumeName,
@Override
public void createDirectory(String volumeName, String bucketName,
String keyName) throws IOException {
+ String ownerName = getRealUserInfo().getShortUserName();
OmKeyArgs keyArgs = new OmKeyArgs.Builder().setVolumeName(volumeName)
.setBucketName(bucketName)
.setKeyName(keyName)
.setAcls(getAclList())
+ .setOwnerName(ownerName)
Review Comment:
My previous [comment](https://github.com/apache/ozone/pull/4188/files#r1550044937) also applies 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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-2046346324
@smengcl PTAL. 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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559096505
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1415,7 +1415,7 @@ long generateDiffReport(
*/
private boolean isKeyModified(OmKeyInfo fromKey, OmKeyInfo toKey) {
return !fromKey.isKeyInfoSame(toKey,
- false, false, false, false)
+ false, false, false, false, false)
Review Comment:
Should key owner change show up in snapshot diffs? I guess so (should be `true` here)? @hemantk-12 @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-7791. Support display and persist owner info to DB [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-1927719092
@sumitagrawl please 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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1490605905
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java:
##########
@@ -772,6 +793,7 @@ public boolean isKeyInfoSame(OmKeyInfo omKeyInfo, boolean checkPath,
replicationConfig.equals(omKeyInfo.replicationConfig) &&
Objects.equals(metadata, omKeyInfo.metadata) &&
Objects.equals(acls, omKeyInfo.acls) &&
+ Objects.equals(ownerName, omKeyInfo.ownerName) &&
Review Comment:
Done.
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1551359152
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -736,17 +736,24 @@ private static void verifySpaceQuota(long quota) throws OMException {
* @return listOfAcls
* */
private List<OzoneAcl> getAclList() {
+ UserGroupInformation realUserInfo = getRealUserInfo();
+ return OzoneAclUtil.getAclList(realUserInfo.getUserName(),
+ realUserInfo.getGroupNames(), userRights, groupRights);
+ }
+
+ /**
+ * Helper function to get the actual operating user.
+ *
+ * @return listOfAcls
+ * */
+ private UserGroupInformation getRealUserInfo() {
+ // After HDDS-5881 the user will not be different,
+ // as S3G uses single RpcClient. So we should be checking thread-local
+ // S3Auth and use it during proxy.
if (ozoneManagerClient.getThreadLocalS3Auth() != null) {
- UserGroupInformation aclUgi =
- UserGroupInformation.createRemoteUser(
- ozoneManagerClient.getThreadLocalS3Auth().getAccessID());
- return OzoneAclUtil.getAclList(
- aclUgi.getUserName(),
- aclUgi.getGroupNames(),
- userRights, groupRights);
+ return UserGroupInformation.createRemoteUser(getThreadLocalS3Auth().getUserPrincipal());
}
Review Comment:
Yes, this is a good idea, I'll update 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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
I think it might be better to just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner.
Also when a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well (unless we add a lengthy step during OM upgrade to add owner to all existing keys which I don't think is the best idea).
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559090113
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -66,13 +66,18 @@ Verify Bucket 1 Owner
${result} = Execute ozone sh bucket info /tenantone/bucket-test1 | jq -r '.owner'
Should Be Equal ${result} testuser
-Put, get and delete a key in the tenant bucket
+Put a key in the tenant bucket
Execute echo "Randomtext" > /tmp/testfile
Execute and checkrc aws s3api --endpoint-url ${S3G_ENDPOINT_URL} put-object --bucket bucket-test1 --key mykey --body /tmp/testfile 0
+
+Verify Object Owner
+ ${result} = Execute ozone sh key info /tenantone/bucket-test1/mykey | jq -r '.owner'
+ Should Be Equal ${result} testuser
Review Comment:
I am fine with this addition. Just curious, IIUC because Ozone was faking key owner before, this check would still pass without key owner right? Or `sh key` would not fake owner.
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559306108
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -66,13 +66,18 @@ Verify Bucket 1 Owner
${result} = Execute ozone sh bucket info /tenantone/bucket-test1 | jq -r '.owner'
Should Be Equal ${result} testuser
-Put, get and delete a key in the tenant bucket
+Put a key in the tenant bucket
Execute echo "Randomtext" > /tmp/testfile
Execute and checkrc aws s3api --endpoint-url ${S3G_ENDPOINT_URL} put-object --bucket bucket-test1 --key mykey --body /tmp/testfile 0
+
+Verify Object Owner
+ ${result} = Execute ozone sh key info /tenantone/bucket-test1/mykey | jq -r '.owner'
+ Should Be Equal ${result} testuser
Review Comment:
Prior to this PR, the Ozone key info command would not display the `owner`, and if it did, it would display `hadoop`
Because docker's startup user is hadoop, so this would have returned hadoop
```bash
[root@centos ~/ozone]$ docker exec ozonesecure-scm-1 whoami
hadoop
[root@centos ~/ozone]$
```
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1558842693
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Hi @smengcl
> When this key owner addition gets in, and admin chown a directory in a bucket...
The current Ozone key does not have an `owner` field, so the current ACL implementation only checks the `Volume owner` and `Bucket owner`. Adding an `owner` field to the Key will not change this behavior.
So, I don't think it will affect the current ACL, because the current ACL checking process doesn't involve the key owner.
https://github.com/apache/ozone/blob/06c0d81af229d66cbb0464e74dda67b73095d106/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneAclUtils.java#L97-L102
---
> Another relevant questions is: How are existing keys WITHOUT an owner field set going to behave after Ozone upgrade...
The current `owner` only affects the read APIs such as `listKey`, `getKeyInfo`, etc. before `owner` in the result of these API were a fake (showing the user who executed the command), now it can show the real `owner`. For old version keys, which didn't have an `owner` field before, it will backoff to the previous logic and populate the `owner` field with the current user executing the command.
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl merged PR #4188:
URL: https://github.com/apache/ozone/pull/4188
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4188: HDDS-7791. Support display and persist owner info to DB
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1081174463
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Missing add owner for createStreamKey method in same class where also create a key
--
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-7791. Support display and persist owner info to DB [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1483854486
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java:
##########
@@ -772,6 +793,7 @@ public boolean isKeyInfoSame(OmKeyInfo omKeyInfo, boolean checkPath,
replicationConfig.equals(omKeyInfo.replicationConfig) &&
Objects.equals(metadata, omKeyInfo.metadata) &&
Objects.equals(acls, omKeyInfo.acls) &&
+ Objects.equals(ownerName, omKeyInfo.ownerName) &&
Review Comment:
ownerName comparision can be based on flag, as this is used for snapdiff for snapshot, where ownername can be different but key will be same
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4188:
URL: https://github.com/apache/ozone/pull/4188#issuecomment-1959002351
> > > @smengcl @errose28 Is it sufficient for S3 to use the AWS `Access ID` directly as the `Owner` of the Object (one-to-one `Access ID` and `Owner` of the Object)
> >
> >
> > @xichen01 Uh preferrably not. `accessId` should be mapped to a user name (similar idea to `ugi.getShortUserName()`) before it can be used in the `owner` field:
> > https://github.com/apache/ozone/blob/cce2f969a85323441c476aaeaf27d45b081b0c2f/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java#L417-L420
>
> @smengcl. Thank for your suggestion. How to map the `accessId` to a `user name`. Are you saying we should use `Displayname` as the `owner` of the Ozone key? The AWS S3 Owner is the `ID` and `Displayname`, while the Ozone Owner is simply a string.
>
> ```java
> public class S3Owner {
>
> public static final S3Owner
> NOT_SUPPORTED_OWNER = new S3Owner("NOT-SUPPORTED", "Not Supported");
>
> @XmlElement(name = "DisplayName")
> private String displayName;
>
> @XmlElement(name = "ID")
> private String id;
>
> }
> ```
@xichen01
> > > @smengcl @errose28 Is it sufficient for S3 to use the AWS `Access ID` directly as the `Owner` of the Object (one-to-one `Access ID` and `Owner` of the Object)
> >
> >
> > @xichen01 Uh preferrably not. `accessId` should be mapped to a user name (similar idea to `ugi.getShortUserName()`) before it can be used in the `owner` field:
> > https://github.com/apache/ozone/blob/cce2f969a85323441c476aaeaf27d45b081b0c2f/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java#L417-L420
>
> @smengcl. Thank for your suggestion. How to map the `accessId` to a `user name`. Are you saying we should use `Displayname` as the `owner` of the Ozone key? The AWS S3 Owner is the `ID` and `Displayname`, while the Ozone Owner is simply a string.
>
> ```java
> public class S3Owner {
>
> public static final S3Owner
> NOT_SUPPORTED_OWNER = new S3Owner("NOT-SUPPORTED", "Not Supported");
>
> @XmlElement(name = "DisplayName")
> private String displayName;
>
> @XmlElement(name = "ID")
> private String id;
>
> }
> ```
We can add a helper method to do the conversion. For `accessId -> user name` conversion there are two cases to be considered:
1. When accessId is generated with [`ozone s3 getsecret`](https://ozone.apache.org/docs/1.3.0/interface/s3.html#security) (not managed under an Ozone tenant), the accessId **is** the Kerberos principal, e.g. `testuser/om@EXAMPLE.COM`. The conversion can be done with `ugi.getShortUserName()` (it automatically applies the conversion rules specified in `hadoop.security.auth_to_local`), where `ugi` can be created from `String` like this:
https://github.com/apache/ozone/blob/2ae531b0f6a069db5a46bd486bb50225a168485d/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/http/HttpServer2.java#L1502-L1503
2. When accessId is generated with [`ozone tenant user assign`](https://ozone.apache.org/docs/1.3.0/feature/s3-tenant-commands.html#assign-a-user-to-a-tenant) (managed by an Ozone tenant), the accessId to username mapping is stored in [`tenantAccessIdTable`](https://github.com/apache/ozone/blob/2f2234c7b61714404399ada8f31b3fb4772b613a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java#L166) table. So the conversion can be done by first getting the `OmDBAccessIdInfo` associated with the `accessId`, then getting its `userPrincipal`, which should already be the (short) user name we need for the owner field:
https://github.com/apache/ozone/blob/6d7ba130cf5a660780aceb773bb17d738df33905/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDBAccessIdInfo.java#L47-L50
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
Arguably we could just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner. This way we don't bloat every single key with this extra owner string in its proto message.
Cons of setting `owner` for every single new key:
1. Slight perf hit for every new key/dir creation.
2. This also implies `chown` on a bucket/dir would be recursive by default. Makes `chown` much slower. (Where in the case of not setting `owner` for every key, we don't need to recurse by default because the sub-keys and sub-dirs should just inherit the parent owner)
4. When a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well. Unless we add a lengthy step during OM upgrade to add owner to all existing keys, which I don't think is the best idea).
Pro:
1. On the other hand I get that the the owner field in key info is required for "sticky bit" behavior to work with properly Ranger authorizer (previously there is a workaround, HDDS-9701).
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1551359152
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -736,17 +736,24 @@ private static void verifySpaceQuota(long quota) throws OMException {
* @return listOfAcls
* */
private List<OzoneAcl> getAclList() {
+ UserGroupInformation realUserInfo = getRealUserInfo();
+ return OzoneAclUtil.getAclList(realUserInfo.getUserName(),
+ realUserInfo.getGroupNames(), userRights, groupRights);
+ }
+
+ /**
+ * Helper function to get the actual operating user.
+ *
+ * @return listOfAcls
+ * */
+ private UserGroupInformation getRealUserInfo() {
+ // After HDDS-5881 the user will not be different,
+ // as S3G uses single RpcClient. So we should be checking thread-local
+ // S3Auth and use it during proxy.
if (ozoneManagerClient.getThreadLocalS3Auth() != null) {
- UserGroupInformation aclUgi =
- UserGroupInformation.createRemoteUser(
- ozoneManagerClient.getThreadLocalS3Auth().getAccessID());
- return OzoneAclUtil.getAclList(
- aclUgi.getUserName(),
- aclUgi.getGroupNames(),
- userRights, groupRights);
+ return UserGroupInformation.createRemoteUser(getThreadLocalS3Auth().getUserPrincipal());
}
Review Comment:
Yes, this is a good idea, I'll update 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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559299839
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java:
##########
@@ -514,7 +514,7 @@ private FileStatusAdapter toFileStatusAdapter(OzoneFileStatus status,
keyInfo.getModificationTime(),
keyInfo.getModificationTime(),
status.isDirectory() ? (short) 00777 : (short) 00666,
- owner,
+ StringUtils.defaultIfEmpty(keyInfo.getOwnerName(), owner),
owner,
Review Comment:
The current change only involves `owner`, and `owner` and `group` are two separate attributes, so I think we're better off leaving `group` unchanged.
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1559282699
##########
hadoop-ozone/integration-test/src/test/resources/core-site.xml:
##########
@@ -20,5 +20,16 @@
<!-- Put site-specific property overrides in this file. -->
<configuration>
-
+ <property>
+ <name>hadoop.proxyuser.proxyuser.users</name>
+ <value>*</value>
+ </property>
+ <property>
+ <name>hadoop.proxyuser.proxyuser.groups</name>
+ <value>*</value>
+ </property>
+ <property>
+ <name>hadoop.proxyuser.proxyuser.hosts</name>
+ <value>*</value>
+ </property>
</configuration>
Review Comment:
This is used for tests. Because I found that adding proxyuser to the specific test case doesn't work.
--
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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550036354
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -736,17 +736,24 @@ private static void verifySpaceQuota(long quota) throws OMException {
* @return listOfAcls
* */
private List<OzoneAcl> getAclList() {
+ UserGroupInformation realUserInfo = getRealUserInfo();
+ return OzoneAclUtil.getAclList(realUserInfo.getUserName(),
+ realUserInfo.getGroupNames(), userRights, groupRights);
+ }
+
+ /**
+ * Helper function to get the actual operating user.
+ *
+ * @return listOfAcls
+ * */
+ private UserGroupInformation getRealUserInfo() {
+ // After HDDS-5881 the user will not be different,
+ // as S3G uses single RpcClient. So we should be checking thread-local
+ // S3Auth and use it during proxy.
if (ozoneManagerClient.getThreadLocalS3Auth() != null) {
- UserGroupInformation aclUgi =
- UserGroupInformation.createRemoteUser(
- ozoneManagerClient.getThreadLocalS3Auth().getAccessID());
- return OzoneAclUtil.getAclList(
- aclUgi.getUserName(),
- aclUgi.getGroupNames(),
- userRights, groupRights);
+ return UserGroupInformation.createRemoteUser(getThreadLocalS3Auth().getUserPrincipal());
}
Review Comment:
Can we cache the result of `UserGroupInformation.createRemoteUser()` in the case of S3?
Every single call `getRealUserInfo()` creates a new UGI object, and that is being called every single time in `createKey()` now.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -736,17 +736,24 @@ private static void verifySpaceQuota(long quota) throws OMException {
* @return listOfAcls
* */
private List<OzoneAcl> getAclList() {
+ UserGroupInformation realUserInfo = getRealUserInfo();
+ return OzoneAclUtil.getAclList(realUserInfo.getUserName(),
+ realUserInfo.getGroupNames(), userRights, groupRights);
+ }
+
+ /**
+ * Helper function to get the actual operating user.
+ *
+ * @return listOfAcls
+ * */
+ private UserGroupInformation getRealUserInfo() {
+ // After HDDS-5881 the user will not be different,
+ // as S3G uses single RpcClient. So we should be checking thread-local
+ // S3Auth and use it during proxy.
if (ozoneManagerClient.getThreadLocalS3Auth() != null) {
- UserGroupInformation aclUgi =
- UserGroupInformation.createRemoteUser(
- ozoneManagerClient.getThreadLocalS3Auth().getAccessID());
- return OzoneAclUtil.getAclList(
- aclUgi.getUserName(),
- aclUgi.getGroupNames(),
- userRights, groupRights);
+ return UserGroupInformation.createRemoteUser(getThreadLocalS3Auth().getUserPrincipal());
}
Review Comment:
Can we cache the result of `UserGroupInformation.createRemoteUser()` in the case of S3?
Every single `getRealUserInfo()` call creates a new UGI object, and that is being called every single time in `createKey()` 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-7791. Support key ownership [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1550044937
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1195,7 +1196,8 @@ public OzoneOutputStream createKey(
.setReplicationConfig(replicationConfig)
.addAllMetadata(metadata)
.setAcls(getAclList())
- .setLatestVersionLocation(getLatestVersionLocation);
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setOwnerName(ownerName);
Review Comment:
Actually I wonder if we really need to add this owner field to every single new key created in Ozone.
Arguably we could just leave this as unset (empty) to indicate the default behavior, which is to inherit parent's owner. This way we don't bloat every single key with this extra owner string in its proto message.
Cons of setting `owner` for every single new key:
1. Slight perf hit for every new key/dir creation.
2. This also implies `chown` on a bucket/dir would be recursive by default. Makes `chown` much slower. (Where in the case of not setting `owner` for every key, we don't need to recurse by default because the sub-keys and sub-dirs should just inherit the parent owner)
3. Because `owner` is a proto string, it can be wasting quite a lot of space repeating the same name string. (numeric user ID mapping can solve that)
4. When a cluster is upgraded to a new Ozone version with key ownership support, all existing keys would natually have empty owners as well. Unless we add a lengthy step during OM upgrade to add owner to all existing keys, which I don't think is the best idea).
Pros:
1. On the other hand I get that the the owner field in key info is required for "sticky bit" behavior to work properly with Ranger authorizer (previously this requires a workaround, HDDS-9701).
2. Writer user is not necessarily the same as the parent owner.
Discussions:
1. Is there some other reasons we must set this owner field for new keys and dirs?
2. How much perf hit would there actually be to add this owner field for every key? Try this in a micro benchmark with and without S3?
3. Is it reasonable to add an config to allow not setting owner field for new keys and dirs by default? Or, only add owner field if the parent dir have the "sticky bit". Or, only add key owner field if the writer is not the same user as the parent owner?
--
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-7791. Support key ownership [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4188:
URL: https://github.com/apache/ozone/pull/4188#discussion_r1576458299
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1415,7 +1415,7 @@ long generateDiffReport(
*/
private boolean isKeyModified(OmKeyInfo fromKey, OmKeyInfo toKey) {
return !fromKey.isKeyInfoSame(toKey,
- false, false, false, false)
+ false, false, false, false, false)
Review Comment:
Modify to true for Key owner..
--
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