hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From reidc...@apache.org
Subject [hbase] branch branch-2.4 updated: HBASE-26001 When turn on access control, the cell level TTL of Increment and Append operations is invalid (#3403)
Date Mon, 21 Jun 2021 07:36:03 GMT
This is an automated email from the ASF dual-hosted git repository.

reidchan pushed a commit to branch branch-2.4
in repository https://gitbox.apache.org/repos/asf/hbase.git


The following commit(s) were added to refs/heads/branch-2.4 by this push:
     new 02714cb  HBASE-26001 When turn on access control, the cell level TTL of Increment
and Append operations is invalid (#3403)
02714cb is described below

commit 02714cbc18d59965b55e94c63debe8f456deb5ab
Author: YutSean <33572832+YutSean@users.noreply.github.com>
AuthorDate: Mon Jun 21 15:00:50 2021 +0800

    HBASE-26001 When turn on access control, the cell level TTL of Increment and Append operations
is invalid (#3403)
    
    Signed-off-by: Reid Chan <reidchan@apache.org>
    (cherry-picked from 029dc81b39aa25f0b1e81aa885df8c2449cbe284)
---
 .../hbase/security/access/AccessController.java    |  45 ++-----
 .../TestPostIncrementAndAppendBeforeWAL.java       | 139 ++++++++++++++++++++-
 2 files changed, 147 insertions(+), 37 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
index f67d6af..1b2fb20 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
@@ -135,7 +135,6 @@ import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import org.apache.hbase.thirdparty.com.google.common.collect.ArrayListMultimap;
 import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableSet;
 import org.apache.hbase.thirdparty.com.google.common.collect.ListMultimap;
 import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
@@ -1744,7 +1743,7 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
       List<Pair<Cell, Cell>> cellPairs) throws IOException {
     // If the HFile version is insufficient to persist tags, we won't have any
     // work to do here
-    if (!cellFeaturesEnabled) {
+    if (!cellFeaturesEnabled || mutation.getACL() == null) {
       return cellPairs;
     }
     return cellPairs.stream().map(pair -> new Pair<>(pair.getFirst(),
@@ -1758,7 +1757,7 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
       List<Pair<Cell, Cell>> cellPairs) throws IOException {
     // If the HFile version is insufficient to persist tags, we won't have any
     // work to do here
-    if (!cellFeaturesEnabled) {
+    if (!cellFeaturesEnabled || mutation.getACL() == null) {
       return cellPairs;
     }
     return cellPairs.stream().map(pair -> new Pair<>(pair.getFirst(),
@@ -1767,50 +1766,28 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
   }
 
   private Cell createNewCellWithTags(Mutation mutation, Cell oldCell, Cell newCell) {
-    // Collect any ACLs from the old cell
+    // As Increment and Append operations have already copied the tags of oldCell to the
newCell,
+    // there is no need to rewrite them again. Just extract non-acl tags of newCell if we
need to
+    // add a new acl tag for the cell. Actually, oldCell is useless here.
     List<Tag> tags = Lists.newArrayList();
-    List<Tag> aclTags = Lists.newArrayList();
-    ListMultimap<String,Permission> perms = ArrayListMultimap.create();
-    if (oldCell != null) {
-      Iterator<Tag> tagIterator = PrivateCellUtil.tagsIterator(oldCell);
+    if (newCell != null) {
+      Iterator<Tag> tagIterator = PrivateCellUtil.tagsIterator(newCell);
       while (tagIterator.hasNext()) {
         Tag tag = tagIterator.next();
         if (tag.getType() != PermissionStorage.ACL_TAG_TYPE) {
           // Not an ACL tag, just carry it through
           if (LOG.isTraceEnabled()) {
-            LOG.trace("Carrying forward tag from " + oldCell + ": type " + tag.getType()
+            LOG.trace("Carrying forward tag from " + newCell + ": type " + tag.getType()
                 + " length " + tag.getValueLength());
           }
           tags.add(tag);
-        } else {
-          aclTags.add(tag);
-        }
-      }
-    }
-
-    // Do we have an ACL on the operation?
-    byte[] aclBytes = mutation.getACL();
-    if (aclBytes != null) {
-      // Yes, use it
-      tags.add(new ArrayBackedTag(PermissionStorage.ACL_TAG_TYPE, aclBytes));
-    } else {
-      // No, use what we carried forward
-      if (perms != null) {
-        // TODO: If we collected ACLs from more than one tag we may have a
-        // List<Permission> of size > 1, this can be collapsed into a single
-        // Permission
-        if (LOG.isTraceEnabled()) {
-          LOG.trace("Carrying forward ACLs from " + oldCell + ": " + perms);
         }
-        tags.addAll(aclTags);
       }
     }
 
-    // If we have no tags to add, just return
-    if (tags.isEmpty()) {
-      return newCell;
-    }
-
+    // We have checked the ACL tag of mutation is not null.
+    // So that the tags could not be empty.
+    tags.add(new ArrayBackedTag(PermissionStorage.ACL_TAG_TYPE, mutation.getACL()));
     return PrivateCellUtil.createCell(newCell, tags);
   }
 
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java
index 031960b..a845826 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java
@@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.io.IOException;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -29,10 +30,15 @@ import java.util.stream.Collectors;
 import org.apache.hadoop.hbase.Cell;
 import org.apache.hadoop.hbase.CellBuilderType;
 import org.apache.hadoop.hbase.CellUtil;
+import org.apache.hadoop.hbase.DoNotRetryIOException;
 import org.apache.hadoop.hbase.ExtendedCellBuilderFactory;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.PrivateCellUtil;
 import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.Tag;
+import org.apache.hadoop.hbase.TagBuilderFactory;
+import org.apache.hadoop.hbase.TagType;
 import org.apache.hadoop.hbase.client.Append;
 import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
 import org.apache.hadoop.hbase.client.Connection;
@@ -45,6 +51,8 @@ import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
 import org.apache.hadoop.hbase.client.TestFromClientSide;
 import org.apache.hadoop.hbase.regionserver.NoSuchColumnFamilyException;
+import org.apache.hadoop.hbase.security.access.AccessController;
+import org.apache.hadoop.hbase.security.access.Permission;
 import org.apache.hadoop.hbase.testclassification.CoprocessorTests;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
 import org.apache.hadoop.hbase.util.Bytes;
@@ -64,14 +72,14 @@ import org.slf4j.LoggerFactory;
  * {@link RegionObserver#postIncrementBeforeWAL(ObserverContext, Mutation, List)} and
  * {@link RegionObserver#postAppendBeforeWAL(ObserverContext, Mutation, List)}. These methods
may
  * change the cells which will be applied to memstore and WAL. So add unit test for the case
which
- * change the cell's column family.
+ * change the cell's column family and tags.
  */
 @Category({CoprocessorTests.class, MediumTests.class})
 public class TestPostIncrementAndAppendBeforeWAL {
 
   @ClassRule
   public static final HBaseClassTestRule CLASS_RULE =
-      HBaseClassTestRule.forClass(TestPostIncrementAndAppendBeforeWAL.class);
+    HBaseClassTestRule.forClass(TestPostIncrementAndAppendBeforeWAL.class);
 
   @Rule
   public TestName name = new TestName();
@@ -92,6 +100,10 @@ public class TestPostIncrementAndAppendBeforeWAL {
   private static final byte[] CQ1 = Bytes.toBytes("cq1");
   private static final byte[] CQ2 = Bytes.toBytes("cq2");
   private static final byte[] VALUE = Bytes.toBytes("value");
+  private static final byte[] VALUE2 = Bytes.toBytes("valuevalue");
+  private static final String USER = "User";
+  private static final Permission PERMS =
+    Permission.newBuilder().withActions(Permission.Action.READ).build();
 
   @BeforeClass
   public static void setupBeforeClass() throws Exception {
@@ -161,6 +173,94 @@ public class TestPostIncrementAndAppendBeforeWAL {
     }
   }
 
+  @Test
+  public void testIncrementTTLWithACLTag() throws Exception {
+    TableName tableName = TableName.valueOf(name.getMethodName());
+    createTableWithCoprocessor(tableName, ChangeCellWithACLTagObserver.class.getName());
+    try (Table table = connection.getTable(tableName)) {
+      // Increment without TTL
+      Increment firstIncrement = new Increment(ROW).addColumn(CF1_BYTES, CQ1, 1)
+        .setACL(USER, PERMS);
+      Result result = table.increment(firstIncrement);
+      assertEquals(1, result.size());
+      assertEquals(1, Bytes.toLong(result.getValue(CF1_BYTES, CQ1)));
+
+      // Check if the new cell can be read
+      Get get = new Get(ROW).addColumn(CF1_BYTES, CQ1);
+      result = table.get(get);
+      assertEquals(1, result.size());
+      assertEquals(1, Bytes.toLong(result.getValue(CF1_BYTES, CQ1)));
+
+      // Increment with TTL
+      Increment secondIncrement = new Increment(ROW).addColumn(CF1_BYTES, CQ1, 1).setTTL(1000)
+        .setACL(USER, PERMS);
+      result = table.increment(secondIncrement);
+
+      // We should get value 2 here
+      assertEquals(1, result.size());
+      assertEquals(2, Bytes.toLong(result.getValue(CF1_BYTES, CQ1)));
+
+      // Wait 4s to let the second increment expire
+      Thread.sleep(4000);
+      get = new Get(ROW).addColumn(CF1_BYTES, CQ1);
+      result = table.get(get);
+
+      // The value should revert to 1
+      assertEquals(1, result.size());
+      assertEquals(1, Bytes.toLong(result.getValue(CF1_BYTES, CQ1)));
+    }
+  }
+
+  @Test
+  public void testAppendTTLWithACLTag() throws Exception {
+    TableName tableName = TableName.valueOf(name.getMethodName());
+    createTableWithCoprocessor(tableName, ChangeCellWithACLTagObserver.class.getName());
+    try (Table table = connection.getTable(tableName)) {
+      // Append without TTL
+      Append firstAppend = new Append(ROW).addColumn(CF1_BYTES, CQ2, VALUE).setACL(USER,
PERMS);
+      Result result = table.append(firstAppend);
+      assertEquals(1, result.size());
+      assertTrue(Bytes.equals(VALUE, result.getValue(CF1_BYTES, CQ2)));
+
+      // Check if the new cell can be read
+      Get get = new Get(ROW).addColumn(CF1_BYTES, CQ2);
+      result = table.get(get);
+      assertEquals(1, result.size());
+      assertTrue(Bytes.equals(VALUE, result.getValue(CF1_BYTES, CQ2)));
+
+      // Append with TTL
+      Append secondAppend = new Append(ROW).addColumn(CF1_BYTES, CQ2, VALUE).setTTL(1000)
+        .setACL(USER, PERMS);
+      result = table.append(secondAppend);
+
+      // We should get "valuevalue""
+      assertEquals(1, result.size());
+      assertTrue(Bytes.equals(VALUE2, result.getValue(CF1_BYTES, CQ2)));
+
+      // Wait 4s to let the second append expire
+      Thread.sleep(4000);
+      get = new Get(ROW).addColumn(CF1_BYTES, CQ2);
+      result = table.get(get);
+
+      // The value should revert to "value"
+      assertEquals(1, result.size());
+      assertTrue(Bytes.equals(VALUE, result.getValue(CF1_BYTES, CQ2)));
+    }
+  }
+
+  private static boolean checkAclTag(byte[] acl, Cell cell) {
+    Iterator<Tag> iter = PrivateCellUtil.tagsIterator(cell);
+    while (iter.hasNext()) {
+      Tag tag = iter.next();
+      if (tag.getType() == TagType.ACL_TAG_TYPE) {
+        Tag temp = TagBuilderFactory.create().
+          setTagType(TagType.ACL_TAG_TYPE).setTagValue(acl).build();
+        return Tag.matchingValue(tag, temp);
+      }
+    }
+    return false;
+  }
+
   public static class ChangeCellWithDifferntColumnFamilyObserver
       implements RegionCoprocessor, RegionObserver {
     @Override
@@ -232,4 +332,37 @@ public class TestPostIncrementAndAppendBeforeWAL {
           .collect(Collectors.toList());
     }
   }
-}
\ No newline at end of file
+
+  public static class ChangeCellWithACLTagObserver extends AccessController {
+    @Override
+    public Optional<RegionObserver> getRegionObserver() {
+      return Optional.of(this);
+    }
+
+    @Override
+    public List<Pair<Cell, Cell>> postIncrementBeforeWAL(
+        ObserverContext<RegionCoprocessorEnvironment> ctx, Mutation mutation,
+        List<Pair<Cell, Cell>> cellPairs) throws IOException {
+      List<Pair<Cell, Cell>> result = super.postIncrementBeforeWAL(ctx, mutation,
cellPairs);
+      for (Pair<Cell, Cell> pair : result) {
+        if (mutation.getACL() != null && !checkAclTag(mutation.getACL(), pair.getSecond()))
{
+          throw new DoNotRetryIOException("Unmatched ACL tag.");
+        }
+      }
+      return result;
+    }
+
+    @Override
+    public List<Pair<Cell, Cell>> postAppendBeforeWAL(
+        ObserverContext<RegionCoprocessorEnvironment> ctx, Mutation mutation,
+        List<Pair<Cell, Cell>> cellPairs) throws IOException {
+      List<Pair<Cell, Cell>> result = super.postAppendBeforeWAL(ctx, mutation,
cellPairs);
+      for (Pair<Cell, Cell> pair : result) {
+        if (mutation.getACL() != null && !checkAclTag(mutation.getACL(), pair.getSecond()))
{
+          throw new DoNotRetryIOException("Unmatched ACL tag.");
+        }
+      }
+      return result;
+    }
+  }
+}

Mime
View raw message