From e9bafeb091549cce35f25a56688b6632578de74b Mon Sep 17 00:00:00 2001 From: Chia-Ping Tsai Date: Sat, 19 Aug 2017 01:55:45 +0800 Subject: [PATCH] HBASE-18572 Delete can't remove the cells which have no visibility label --- .../DefaultVisibilityLabelServiceImpl.java | 12 +- .../visibility/VisibilityController.java | 4 + .../VisibilityScanDeleteTracker.java | 19 +- ...ExpAsStringVisibilityLabelServiceImpl.java | 4 + .../TestVisibilityLabelsWithDeletes.java | 191 ++++++++++++++++++ 5 files changed, 222 insertions(+), 8 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/DefaultVisibilityLabelServiceImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/DefaultVisibilityLabelServiceImpl.java index d4a5627bec6..672da8afd08 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/DefaultVisibilityLabelServiceImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/DefaultVisibilityLabelServiceImpl.java @@ -551,12 +551,16 @@ public class DefaultVisibilityLabelServiceImpl implements VisibilityLabelService @Override public boolean matchVisibility(List putVisTags, Byte putTagsFormat, List deleteVisTags, Byte deleteTagsFormat) throws IOException { + // Early out if there are no tags in both of cell and delete + if (putVisTags.isEmpty() && deleteVisTags.isEmpty()) { + return true; + } + // Early out if one of the tags is empty + if (putVisTags.isEmpty() ^ deleteVisTags.isEmpty()) { + return false; + } if ((deleteTagsFormat != null && deleteTagsFormat == SORTED_ORDINAL_SERIALIZATION_FORMAT) && (putTagsFormat == null || putTagsFormat == SORTED_ORDINAL_SERIALIZATION_FORMAT)) { - if (putVisTags.isEmpty()) { - // Early out if there are no tags in the cell - return false; - } if (putTagsFormat == null) { return matchUnSortedVisibilityTags(putVisTags, deleteVisTags); } else { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java index 23f058373da..130587a152f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java @@ -1074,6 +1074,10 @@ public class VisibilityController implements MasterObserver, RegionObserver, public ReturnCode filterKeyValue(Cell cell) throws IOException { List putVisTags = new ArrayList<>(); Byte putCellVisTagsFormat = VisibilityUtils.extractVisibilityTags(cell, putVisTags); + if (putVisTags.isEmpty() && deleteCellVisTags.isEmpty()) { + // Early out if there are no tags in the cell + return ReturnCode.INCLUDE; + } boolean matchFound = VisibilityLabelServiceManager .getInstance().getVisibilityLabelService() .matchVisibility(putVisTags, putCellVisTagsFormat, deleteCellVisTags, diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityScanDeleteTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityScanDeleteTracker.java index 67181e1299a..f62e8d03a13 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityScanDeleteTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityScanDeleteTracker.java @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.security.visibility; import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import org.apache.commons.logging.Log; @@ -45,6 +46,10 @@ public class VisibilityScanDeleteTracker extends ScanDeleteTracker { private static final Log LOG = LogFactory.getLog(VisibilityScanDeleteTracker.class); + /** + * This tag is used for the DELETE cell which has no visibility label. + */ + private static final List EMPTY_TAG = Collections.EMPTY_LIST; // Its better to track the visibility tags in delete based on each type. Create individual // data structures for tracking each of them. This would ensure that there is no tracking based // on time and also would handle all cases where deletefamily or deletecolumns is specified with @@ -110,9 +115,8 @@ public class VisibilityScanDeleteTracker extends ScanDeleteTracker { private boolean extractDeleteCellVisTags(Cell delCell, Type type) { // If tag is present in the delete boolean hasVisTag = false; - if (delCell.getTagsLength() > 0) { - Byte deleteCellVisTagsFormat = null; - switch (type) { + Byte deleteCellVisTagsFormat = null; + switch (type) { case DeleteFamily: List delTags = new ArrayList<>(); if (visibilityTagsDeleteFamily == null) { @@ -122,6 +126,8 @@ public class VisibilityScanDeleteTracker extends ScanDeleteTracker { if (!delTags.isEmpty()) { visibilityTagsDeleteFamily.add(new Triple<>(delTags, deleteCellVisTagsFormat, delCell.getTimestamp())); hasVisTag = true; + } else { + visibilityTagsDeleteFamily.add(new Triple<>(EMPTY_TAG, deleteCellVisTagsFormat, delCell.getTimestamp())); } break; case DeleteFamilyVersion: @@ -133,6 +139,8 @@ public class VisibilityScanDeleteTracker extends ScanDeleteTracker { if (!delTags.isEmpty()) { visibilityTagsDeleteFamilyVersion.add(new Triple<>(delTags, deleteCellVisTagsFormat, delCell.getTimestamp())); hasVisTag = true; + } else { + visibilityTagsDeleteFamilyVersion.add(new Triple<>(EMPTY_TAG, deleteCellVisTagsFormat, delCell.getTimestamp())); } break; case DeleteColumn: @@ -144,6 +152,8 @@ public class VisibilityScanDeleteTracker extends ScanDeleteTracker { if (!delTags.isEmpty()) { visibilityTagsDeleteColumns.add(new Pair<>(delTags, deleteCellVisTagsFormat)); hasVisTag = true; + } else { + visibilityTagsDeleteColumns.add(new Pair<>(EMPTY_TAG, deleteCellVisTagsFormat)); } break; case Delete: @@ -155,11 +165,12 @@ public class VisibilityScanDeleteTracker extends ScanDeleteTracker { if (!delTags.isEmpty()) { visiblityTagsDeleteColumnVersion.add(new Pair<>(delTags, deleteCellVisTagsFormat)); hasVisTag = true; + } else { + visiblityTagsDeleteColumnVersion.add(new Pair<>(EMPTY_TAG, deleteCellVisTagsFormat)); } break; default: throw new IllegalArgumentException("Invalid delete type"); - } } return hasVisTag; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/ExpAsStringVisibilityLabelServiceImpl.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/ExpAsStringVisibilityLabelServiceImpl.java index d8d6f1ec5ee..043d08bb38c 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/ExpAsStringVisibilityLabelServiceImpl.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/ExpAsStringVisibilityLabelServiceImpl.java @@ -418,6 +418,10 @@ public class ExpAsStringVisibilityLabelServiceImpl implements VisibilityLabelSer private static boolean checkForMatchingVisibilityTagsWithSortedOrder(List putVisTags, List deleteVisTags) { + // Early out if there are no tags in both of cell and delete + if (putVisTags.isEmpty() && deleteVisTags.isEmpty()) { + return true; + } boolean matchFound = false; // If the size does not match. Definitely we are not comparing the equal // tags. diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityLabelsWithDeletes.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityLabelsWithDeletes.java index dfc48bfcd6e..4f48236c0f1 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityLabelsWithDeletes.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityLabelsWithDeletes.java @@ -17,9 +17,12 @@ */ package org.apache.hadoop.hbase.security.visibility; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellScanner; +import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.HColumnDescriptor; import org.apache.hadoop.hbase.HConstants; @@ -41,6 +44,9 @@ import org.apache.hadoop.hbase.security.User; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.testclassification.SecurityTests; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.DefaultEnvironmentEdge; +import org.apache.hadoop.hbase.util.EnvironmentEdge; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.Threads; import org.junit.After; import org.junit.AfterClass; @@ -67,6 +73,7 @@ import static org.junit.Assert.assertTrue; */ @Category({SecurityTests.class, MediumTests.class}) public class TestVisibilityLabelsWithDeletes { + private static final Log LOG = LogFactory.getLog(TestVisibilityLabelsWithDeletes.class); private static final String TOPSECRET = "TOPSECRET"; private static final String PUBLIC = "PUBLIC"; private static final String PRIVATE = "PRIVATE"; @@ -3285,4 +3292,188 @@ public class TestVisibilityLabelsWithDeletes { public static List createList(T... ts) { return new ArrayList<>(Arrays.asList(ts)); } + + + private enum DeleteMark { + ROW, + FAMILY, + FAMILY_VERSION, + COLUMN, + CELL + } + + private static Delete addDeleteMark(Delete d, DeleteMark mark, long now) { + switch (mark) { + case ROW: + break; + case FAMILY: + d.addFamily(fam); + break; + case FAMILY_VERSION: + d.addFamilyVersion(fam, now); + break; + case COLUMN: + d.addColumns(fam, qual); + break; + case CELL: + d.addColumn(fam, qual); + break; + default: + break; + } + return d; + } + + @Test + public void testDeleteCellWithoutVisibility() throws IOException, InterruptedException { + for (DeleteMark mark : DeleteMark.values()) { + testDeleteCellWithoutVisibility(mark); + } + } + + private void testDeleteCellWithoutVisibility(DeleteMark mark) throws IOException, InterruptedException { + setAuths(); + final TableName tableName = TableName.valueOf("testDeleteCellWithoutVisibility-" + mark.name()); + Admin hBaseAdmin = TEST_UTIL.getAdmin(); + HColumnDescriptor colDesc = new HColumnDescriptor(fam); + colDesc.setMaxVersions(5); + HTableDescriptor desc = new HTableDescriptor(tableName); + desc.addFamily(colDesc); + hBaseAdmin.createTable(desc); + long now = EnvironmentEdgeManager.currentTime(); + List puts = new ArrayList<>(1); + Put put = new Put(row1); + if (mark == DeleteMark.FAMILY_VERSION) { + put.addColumn(fam, qual, now, value); + } else { + put.addColumn(fam, qual, value); + } + + puts.add(put); + try (Table table = TEST_UTIL.getConnection().getTable(tableName)){ + table.put(puts); + Result r = table.get(new Get(row1)); + assertEquals(1, r.size()); + assertEquals(Bytes.toString(value), Bytes.toString(CellUtil.cloneValue(r.rawCells()[0]))); + + Delete d = addDeleteMark(new Delete(row1), mark, now); + table.delete(d); + r = table.get(new Get(row1)); + assertEquals(0, r.size()); + } + } + + @Test + public void testDeleteCellWithVisibility() throws IOException, InterruptedException { + for (DeleteMark mark : DeleteMark.values()) { + testDeleteCellWithVisibility(mark); + testDeleteCellWithVisibilityV2(mark); + } + } + + private void testDeleteCellWithVisibility(DeleteMark mark) throws IOException, InterruptedException { + setAuths(); + final TableName tableName = TableName.valueOf("testDeleteCellWithVisibility-" + mark.name()); + Admin hBaseAdmin = TEST_UTIL.getAdmin(); + HColumnDescriptor colDesc = new HColumnDescriptor(fam); + colDesc.setMaxVersions(5); + HTableDescriptor desc = new HTableDescriptor(tableName); + desc.addFamily(colDesc); + hBaseAdmin.createTable(desc); + long now = EnvironmentEdgeManager.currentTime(); + List puts = new ArrayList<>(2); + Put put = new Put(row1); + if (mark == DeleteMark.FAMILY_VERSION) { + put.addColumn(fam, qual, now, value); + } else { + put.addColumn(fam, qual, value); + } + puts.add(put); + put = new Put(row1); + if (mark == DeleteMark.FAMILY_VERSION) { + put.addColumn(fam, qual, now, value1); + } else { + put.addColumn(fam, qual, value1); + } + put.setCellVisibility(new CellVisibility(PRIVATE)); + puts.add(put); + try (Table table = TEST_UTIL.getConnection().getTable(tableName)) { + table.put(puts); + Result r = table.get(new Get(row1)); + assertEquals(0, r.size()); + r = table.get(new Get(row1).setAuthorizations(new Authorizations(PRIVATE))); + assertEquals(1, r.size()); + assertEquals(Bytes.toString(value1), Bytes.toString(CellUtil.cloneValue(r.rawCells()[0]))); + + Delete d = addDeleteMark(new Delete(row1), mark, now); + table.delete(d); + + r = table.get(new Get(row1)); + assertEquals(0, r.size()); + r = table.get(new Get(row1).setAuthorizations(new Authorizations(PRIVATE))); + assertEquals(1, r.size()); + assertEquals(Bytes.toString(value1), Bytes.toString(CellUtil.cloneValue(r.rawCells()[0]))); + + d = addDeleteMark(new Delete(row1).setCellVisibility(new CellVisibility(PRIVATE)), mark, now); + table.delete(d); + + r = table.get(new Get(row1)); + assertEquals(0, r.size()); + r = table.get(new Get(row1).setAuthorizations(new Authorizations(PRIVATE))); + assertEquals(0, r.size()); + } + } + + private void testDeleteCellWithVisibilityV2(DeleteMark mark) throws IOException, InterruptedException { + setAuths(); + final TableName tableName = TableName.valueOf("testDeleteCellWithVisibilityV2-" + mark.name()); + Admin hBaseAdmin = TEST_UTIL.getAdmin(); + HColumnDescriptor colDesc = new HColumnDescriptor(fam); + colDesc.setMaxVersions(5); + HTableDescriptor desc = new HTableDescriptor(tableName); + desc.addFamily(colDesc); + hBaseAdmin.createTable(desc); + long now = EnvironmentEdgeManager.currentTime(); + List puts = new ArrayList<>(2); + Put put = new Put(row1); + put.setCellVisibility(new CellVisibility(PRIVATE)); + if (mark == DeleteMark.FAMILY_VERSION) { + put.addColumn(fam, qual, now, value); + } else { + put.addColumn(fam, qual, value); + } + puts.add(put); + put = new Put(row1); + if (mark == DeleteMark.FAMILY_VERSION) { + put.addColumn(fam, qual, now, value1); + } else { + put.addColumn(fam, qual, value1); + } + puts.add(put); + try (Table table = TEST_UTIL.getConnection().getTable(tableName)){ + table.put(puts); + Result r = table.get(new Get(row1)); + assertEquals(1, r.size()); + assertEquals(Bytes.toString(value1), Bytes.toString(CellUtil.cloneValue(r.rawCells()[0]))); + r = table.get(new Get(row1).setAuthorizations(new Authorizations(PRIVATE))); + assertEquals(1, r.size()); + assertEquals(Bytes.toString(value1), Bytes.toString(CellUtil.cloneValue(r.rawCells()[0]))); + + Delete d = addDeleteMark(new Delete(row1), mark, now); + table.delete(d); + + r = table.get(new Get(row1)); + assertEquals(0, r.size()); + r = table.get(new Get(row1).setAuthorizations(new Authorizations(PRIVATE))); + assertEquals(0, r.size()); + + d = addDeleteMark(new Delete(row1).setCellVisibility(new CellVisibility(PRIVATE)), mark, now); + table.delete(d); + + r = table.get(new Get(row1)); + assertEquals(0, r.size()); + r = table.get(new Get(row1).setAuthorizations(new Authorizations(PRIVATE))); + assertEquals(0, r.size()); + } + } }