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 2020/12/16 13:09:48 UTC
[GitHub] [ozone] masstter opened a new pull request #1716: HDDS-4606. Got wrong acls when set default Acl(s) on volume for repeatedly
masstter opened a new pull request #1716:
URL: https://github.com/apache/ozone/pull/1716
HDDS-4606. Got wrong acls when set default Acl(s) on volume for repeatedly
## What changes were proposed in this pull request?
add check step when adding volume Acl(s)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4606
## How was this patch tested?
updating Acl(s) repeatedly, then using ozone CLI get target Acl(s) -- passed
----------------------------------------------------------------
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.
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] ChenSammi commented on pull request #1716: HDDS-4606. Detect and handle duplicate ACLs when set default ACLs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1716:
URL: https://github.com/apache/ozone/pull/1716#issuecomment-750721415
+1. Thanks @masstter for the contribution and @xiaoyuyao for the code 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.
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] ChenSammi commented on a change in pull request #1716: HDDS-4606. Detect and handle duplicate ACLs when set default ACLs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1716:
URL: https://github.com/apache/ozone/pull/1716#discussion_r547724814
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmOzoneAclMap.java
##########
@@ -100,28 +97,58 @@ private BitSet getAcl(OzoneAclType type, String user) {
// Add a new acl to the map
public void addAcl(OzoneAcl acl) throws OMException {
Objects.requireNonNull(acl, "Acl should not be null.");
+ OzoneAclType aclType = OzoneAclType.valueOf(acl.getType().name());
if (acl.getAclScope().equals(OzoneAcl.AclScope.DEFAULT)) {
- defaultAclList.add(OzoneAcl.toProtobuf(acl));
+ addDefaultAcl(acl);
return;
}
- OzoneAclType aclType = OzoneAclType.valueOf(acl.getType().name());
if (!getAccessAclMap(aclType).containsKey(acl.getName())) {
getAccessAclMap(aclType).put(acl.getName(), acl.getAclBitSet());
} else {
- // Check if we are adding new rights to existing acl.
- BitSet temp = (BitSet) acl.getAclBitSet().clone();
- BitSet curRights = (BitSet) getAccessAclMap(aclType).
- get(acl.getName()).clone();
- temp.or(curRights);
-
- if (temp.equals(curRights)) {
- // throw exception if acl is already added.
- throw new OMException("Acl " + acl + " already exist.",
- INVALID_REQUEST);
+ BitSet currBitSet = getAccessAclMap(aclType).get(acl.getName());
Review comment:
currBitSet -> curBitSet
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmOzoneAclMap.java
##########
@@ -100,28 +97,58 @@ private BitSet getAcl(OzoneAclType type, String user) {
// Add a new acl to the map
public void addAcl(OzoneAcl acl) throws OMException {
Objects.requireNonNull(acl, "Acl should not be null.");
+ OzoneAclType aclType = OzoneAclType.valueOf(acl.getType().name());
if (acl.getAclScope().equals(OzoneAcl.AclScope.DEFAULT)) {
- defaultAclList.add(OzoneAcl.toProtobuf(acl));
+ addDefaultAcl(acl);
return;
}
- OzoneAclType aclType = OzoneAclType.valueOf(acl.getType().name());
if (!getAccessAclMap(aclType).containsKey(acl.getName())) {
getAccessAclMap(aclType).put(acl.getName(), acl.getAclBitSet());
} else {
- // Check if we are adding new rights to existing acl.
- BitSet temp = (BitSet) acl.getAclBitSet().clone();
- BitSet curRights = (BitSet) getAccessAclMap(aclType).
- get(acl.getName()).clone();
- temp.or(curRights);
-
- if (temp.equals(curRights)) {
- // throw exception if acl is already added.
- throw new OMException("Acl " + acl + " already exist.",
- INVALID_REQUEST);
+ BitSet currBitSet = getAccessAclMap(aclType).get(acl.getName());
+ BitSet bitSet = checkAndGet(acl, currBitSet);
+ getAccessAclMap(aclType).replace(acl.getName(), bitSet);
+ }
+ }
+
+ private void addDefaultAcl(OzoneAcl acl) throws OMException {
+ OzoneAclInfo ozoneAclInfo = OzoneAcl.toProtobuf(acl);
+ if (defaultAclList.contains(ozoneAclInfo)) {
+ aclExistsError(acl);
+ } else {
+ for (int i = 0; i < defaultAclList.size(); i++) {
+ OzoneAclInfo old = defaultAclList.get(i);
+ if (old.getType() == ozoneAclInfo.getType() && old.getName().equals(
+ ozoneAclInfo.getName())) {
+ BitSet currBitSet = BitSet.valueOf(old.getRights().toByteArray());
+ BitSet bitSet = checkAndGet(acl, currBitSet);
+ ozoneAclInfo = OzoneAclInfo.newBuilder(ozoneAclInfo).setRights(
+ ByteString.copyFrom(bitSet.toByteArray())).build();
+ defaultAclList.remove(i);
+ defaultAclList.add(ozoneAclInfo);
+ return;
+ }
}
- getAccessAclMap(aclType).replace(acl.getName(), temp);
}
+ defaultAclList.add(ozoneAclInfo);
+ }
+
+ private void aclExistsError(OzoneAcl acl) throws OMException {
+ // throw exception if acl is already added.
+ throw new OMException("Acl " + acl + " already exist.", INVALID_REQUEST);
+ }
+
+ private BitSet checkAndGet(OzoneAcl acl, BitSet currBitSet)
+ throws OMException {
Review comment:
New line indent is 2. Here second line of a long line is 4.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmOzoneAclMap.java
##########
@@ -100,28 +97,58 @@ private BitSet getAcl(OzoneAclType type, String user) {
// Add a new acl to the map
public void addAcl(OzoneAcl acl) throws OMException {
Objects.requireNonNull(acl, "Acl should not be null.");
+ OzoneAclType aclType = OzoneAclType.valueOf(acl.getType().name());
if (acl.getAclScope().equals(OzoneAcl.AclScope.DEFAULT)) {
- defaultAclList.add(OzoneAcl.toProtobuf(acl));
+ addDefaultAcl(acl);
return;
}
- OzoneAclType aclType = OzoneAclType.valueOf(acl.getType().name());
if (!getAccessAclMap(aclType).containsKey(acl.getName())) {
getAccessAclMap(aclType).put(acl.getName(), acl.getAclBitSet());
} else {
- // Check if we are adding new rights to existing acl.
- BitSet temp = (BitSet) acl.getAclBitSet().clone();
- BitSet curRights = (BitSet) getAccessAclMap(aclType).
- get(acl.getName()).clone();
- temp.or(curRights);
-
- if (temp.equals(curRights)) {
- // throw exception if acl is already added.
- throw new OMException("Acl " + acl + " already exist.",
- INVALID_REQUEST);
+ BitSet currBitSet = getAccessAclMap(aclType).get(acl.getName());
+ BitSet bitSet = checkAndGet(acl, currBitSet);
+ getAccessAclMap(aclType).replace(acl.getName(), bitSet);
+ }
+ }
+
+ private void addDefaultAcl(OzoneAcl acl) throws OMException {
+ OzoneAclInfo ozoneAclInfo = OzoneAcl.toProtobuf(acl);
+ if (defaultAclList.contains(ozoneAclInfo)) {
+ aclExistsError(acl);
+ } else {
+ for (int i = 0; i < defaultAclList.size(); i++) {
+ OzoneAclInfo old = defaultAclList.get(i);
+ if (old.getType() == ozoneAclInfo.getType() && old.getName().equals(
+ ozoneAclInfo.getName())) {
+ BitSet currBitSet = BitSet.valueOf(old.getRights().toByteArray());
+ BitSet bitSet = checkAndGet(acl, currBitSet);
+ ozoneAclInfo = OzoneAclInfo.newBuilder(ozoneAclInfo).setRights(
+ ByteString.copyFrom(bitSet.toByteArray())).build();
+ defaultAclList.remove(i);
+ defaultAclList.add(ozoneAclInfo);
+ return;
+ }
}
- getAccessAclMap(aclType).replace(acl.getName(), temp);
}
+ defaultAclList.add(ozoneAclInfo);
+ }
+
+ private void aclExistsError(OzoneAcl acl) throws OMException {
+ // throw exception if acl is already added.
+ throw new OMException("Acl " + acl + " already exist.", INVALID_REQUEST);
+ }
+
+ private BitSet checkAndGet(OzoneAcl acl, BitSet currBitSet)
Review comment:
currBitSet -> curBitSet
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmOzoneAclMap.java
##########
@@ -100,28 +97,58 @@ private BitSet getAcl(OzoneAclType type, String user) {
// Add a new acl to the map
public void addAcl(OzoneAcl acl) throws OMException {
Objects.requireNonNull(acl, "Acl should not be null.");
+ OzoneAclType aclType = OzoneAclType.valueOf(acl.getType().name());
if (acl.getAclScope().equals(OzoneAcl.AclScope.DEFAULT)) {
- defaultAclList.add(OzoneAcl.toProtobuf(acl));
+ addDefaultAcl(acl);
return;
}
- OzoneAclType aclType = OzoneAclType.valueOf(acl.getType().name());
if (!getAccessAclMap(aclType).containsKey(acl.getName())) {
getAccessAclMap(aclType).put(acl.getName(), acl.getAclBitSet());
} else {
- // Check if we are adding new rights to existing acl.
- BitSet temp = (BitSet) acl.getAclBitSet().clone();
- BitSet curRights = (BitSet) getAccessAclMap(aclType).
- get(acl.getName()).clone();
- temp.or(curRights);
-
- if (temp.equals(curRights)) {
- // throw exception if acl is already added.
- throw new OMException("Acl " + acl + " already exist.",
- INVALID_REQUEST);
+ BitSet currBitSet = getAccessAclMap(aclType).get(acl.getName());
+ BitSet bitSet = checkAndGet(acl, currBitSet);
+ getAccessAclMap(aclType).replace(acl.getName(), bitSet);
+ }
+ }
+
+ private void addDefaultAcl(OzoneAcl acl) throws OMException {
+ OzoneAclInfo ozoneAclInfo = OzoneAcl.toProtobuf(acl);
+ if (defaultAclList.contains(ozoneAclInfo)) {
+ aclExistsError(acl);
+ } else {
+ for (int i = 0; i < defaultAclList.size(); i++) {
+ OzoneAclInfo old = defaultAclList.get(i);
+ if (old.getType() == ozoneAclInfo.getType() && old.getName().equals(
+ ozoneAclInfo.getName())) {
+ BitSet currBitSet = BitSet.valueOf(old.getRights().toByteArray());
+ BitSet bitSet = checkAndGet(acl, currBitSet);
+ ozoneAclInfo = OzoneAclInfo.newBuilder(ozoneAclInfo).setRights(
+ ByteString.copyFrom(bitSet.toByteArray())).build();
+ defaultAclList.remove(i);
+ defaultAclList.add(ozoneAclInfo);
+ return;
+ }
}
- getAccessAclMap(aclType).replace(acl.getName(), temp);
}
+ defaultAclList.add(ozoneAclInfo);
+ }
+
+ private void aclExistsError(OzoneAcl acl) throws OMException {
Review comment:
Could you replace other same expection codes in this class using this aclExistsError function?
----------------------------------------------------------------
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.
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] ChenSammi merged pull request #1716: HDDS-4606. Detect and handle duplicate ACLs when set default ACLs
Posted by GitBox <gi...@apache.org>.
ChenSammi merged pull request #1716:
URL: https://github.com/apache/ozone/pull/1716
----------------------------------------------------------------
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.
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] masstter commented on pull request #1716: HDDS-4606. Got wrong ACL(s) when set default ACL(s) on volume for repeatedly
Posted by GitBox <gi...@apache.org>.
masstter commented on pull request #1716:
URL: https://github.com/apache/ozone/pull/1716#issuecomment-748769807
@xiaoyuyao Thanks for reviews, it's helped me a lot, added handle adding different acl rights to existing acl entry in my new changes
----------------------------------------------------------------
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.
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