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 59623ece1359..f3b089d330b6 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 @@ -328,6 +328,20 @@ public DeleteResult isDeleted(Cell cell) { return DeleteResult.NOT_DELETED; } + /** + * The inherited {@link ScanDeleteTracker#isRedundantDelete(Cell)} is label-blind: it tracks + * deleteCell/deleteType/deleteTimestamp (and familyStamp) without regard to the visibility tags + * of the delete markers. Reusing it during minor compaction could drop a delete marker that only + * shadows differently-labeled data, resurrecting cells that should stay deleted. So on + * cell-visibility tables we conservatively report no delete as redundant (matching the + * {@link org.apache.hadoop.hbase.regionserver.querymatcher.DeleteTracker} default), keeping every + * marker (tracked + included). + */ + @Override + public boolean isRedundantDelete(Cell cell) { + return false; + } + @Override public void reset() { super.reset(); 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 c1cd137cb99c..1548b9874ce7 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,15 +17,18 @@ */ package org.apache.hadoop.hbase.security.visibility; +import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; import java.io.InterruptedIOException; import java.security.PrivilegedExceptionAction; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Random; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellScanner; import org.apache.hadoop.hbase.CellUtil; @@ -45,6 +48,7 @@ import org.apache.hadoop.hbase.client.Table; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.protobuf.generated.VisibilityLabelsProtos.VisibilityLabelsResponse; +import org.apache.hadoop.hbase.regionserver.HRegion; import org.apache.hadoop.hbase.testclassification.LargeTests; import org.apache.hadoop.hbase.testclassification.SecurityTests; import org.apache.hadoop.hbase.util.Bytes; @@ -1734,6 +1738,96 @@ public Void run() throws Exception { } } + /** + * On a cell-visibility table, two DeleteColumn markers carrying different labels shadow disjoint + * cells, so neither is redundant w.r.t. the other. A minor compaction must not drop the + * lower-timestamp marker just because a higher-timestamp marker of a different label was tracked + * first; doing so would resurrect data that must stay deleted. This is the end-to-end regression + * guard for {@link VisibilityScanDeleteTracker#isRedundantDelete}. + *

+ * The big put is larger than {@code hbase.hstore.compaction.max.size}, so minor compaction + * excludes its file and merges only the two small delete-marker files (isAllFiles=false, hence + * COMPACT_RETAIN_DELETES and MinorCompactionScanQueryMatcher, the path that calls + * isRedundantDelete). The put's data stays in its own file, so dropping the SECRET marker would + * make it visible again. + */ + @Test + public void testDifferentLabelDeleteMarkersSurviveMinorCompaction(TestInfo testInfo) + throws Exception { + setAuths(); + final TableName tableName = TableName + .valueOf(TableNameTestExtension.cleanUpTestName(testInfo.getTestMethod().get().getName())); + // Minor compaction merges at most two files and skips any file larger than 64KB. + ColumnFamilyDescriptorBuilder cfd = ColumnFamilyDescriptorBuilder.newBuilder(fam) + .setMaxVersions(5).setConfiguration("hbase.hstore.compaction.min", "2") + .setConfiguration("hbase.hstore.compaction.max", "2") + .setConfiguration("hbase.hstore.compaction.max.size", "65536"); + // Disable automatic compaction while the three HFiles are laid down, so the minor compaction + // under test is exactly the one triggered explicitly below (no racing background compaction). + TEST_UTIL.getAdmin().createTable(TableDescriptorBuilder.newBuilder(tableName) + .setCompactionEnabled(false).setColumnFamily(cfd.build()).build()); + + SUPERUSER.runAs((PrivilegedExceptionAction) () -> { + try (Connection connection = ConnectionFactory.createConnection(conf); + Table table = connection.getTable(tableName)) { + // Big SECRET-visible put at ts=40. Random bytes keep the file above max.size so it is + // excluded from the minor compaction and retains the data the SECRET delete shadows. + byte[] big = new byte[200 * 1024]; + new Random(1).nextBytes(big); + Put put = new Put(row1); + put.addColumn(fam, qual, 40L, big); + put.setCellVisibility(new CellVisibility(SECRET)); + table.put(put); + TEST_UTIL.getAdmin().flush(tableName); + // SECRET DeleteColumn at ts=50 shadows the SECRET put. + Delete dSecret = new Delete(row1); + dSecret.setCellVisibility(new CellVisibility(SECRET)); + dSecret.addColumns(fam, qual, 50L); + table.delete(dSecret); + TEST_UTIL.getAdmin().flush(tableName); + // Newer CONFIDENTIAL DeleteColumn at ts=100 shadows disjoint (CONFIDENTIAL) cells. + Delete dConf = new Delete(row1); + dConf.setCellVisibility(new CellVisibility(CONFIDENTIAL)); + dConf.addColumns(fam, qual, 100L); + table.delete(dConf); + TEST_UTIL.getAdmin().flush(tableName); + } + return null; + }); + + // Before compaction the SECRET put is correctly hidden by the SECRET delete. + assertEquals(0, countCells(tableName, SECRET)); + + // Synchronously minor-compact the two small delete files (the big put file is excluded). + HRegion region = TEST_UTIL.getHBaseCluster().getRegions(tableName).get(0); + region.compact(false); + // Two files remain: the untouched big put file plus the single merged delete file. This both + // confirms a minor compaction happened and that the big file was not swept into a major one. + await().atMost(Duration.ofSeconds(60)) + .untilAsserted(() -> assertEquals(2, region.getStore(fam).getStorefilesCount())); + + // The SECRET delete must still shadow the SECRET put: no resurrection. + assertEquals(0, countCells(tableName, SECRET)); + } + + private int countCells(TableName tableName, String... auths) throws Exception { + return SUPERUSER.runAs((PrivilegedExceptionAction) () -> { + try (Connection connection = ConnectionFactory.createConnection(conf); + Table table = connection.getTable(tableName)) { + Scan s = new Scan(); + s.readVersions(5); + s.setAuthorizations(new Authorizations(auths)); + try (ResultScanner scanner = table.getScanner(s)) { + int count = 0; + for (Result r : scanner) { + count += r.size(); + } + return count; + } + } + }); + } + @Test public void testDeleteFamilySpecificTimeStampWithMulipleVersionsDoneTwice(TestInfo testInfo) throws Exception { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityScanDeleteTracker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityScanDeleteTracker.java new file mode 100644 index 000000000000..4dcef04517ac --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityScanDeleteTracker.java @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.security.visibility; + +import static org.junit.jupiter.api.Assertions.assertFalse; + +import java.util.Collections; +import java.util.List; +import org.apache.hadoop.hbase.ArrayBackedTag; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellComparatorImpl; +import org.apache.hadoop.hbase.KeyValue; +import org.apache.hadoop.hbase.Tag; +import org.apache.hadoop.hbase.TagType; +import org.apache.hadoop.hbase.regionserver.querymatcher.DeleteTracker; +import org.apache.hadoop.hbase.testclassification.SecurityTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.jupiter.api.Test; + +/** + * Tests that {@link VisibilityScanDeleteTracker} never reports a delete marker as redundant. + *

+ * HBASE-30036 added {@link DeleteTracker#isRedundantDelete(Cell)} so that minor compaction can drop + * a delete marker already covered by a previously tracked delete of equal or broader scope. The + * base {@link org.apache.hadoop.hbase.regionserver.querymatcher.ScanDeleteTracker} implements it + * purely from delete type / timestamp / qualifier, with no regard to visibility labels. + *

+ * On cell-visibility tables a delete marker only shadows cells whose visibility expression matches + * (see {@link VisibilityScanDeleteTracker#isDeleted}). Two markers carrying different labels cover + * disjoint cells, so neither is redundant w.r.t. the other. Reusing the label-blind base logic + * would wrongly declare such a marker redundant; minor compaction would then drop it and resurrect + * cells that must stay deleted. {@link VisibilityScanDeleteTracker} must therefore conservatively + * report no delete as redundant. + */ +@org.junit.jupiter.api.Tag(SecurityTests.TAG) +@org.junit.jupiter.api.Tag(SmallTests.TAG) +public class TestVisibilityScanDeleteTracker { + + private static final byte[] ROW = Bytes.toBytes("row"); + private static final byte[] FAMILY = Bytes.toBytes("f"); + private static final byte[] QUALIFIER = Bytes.toBytes("q"); + + // Placeholder visibility-tag payloads. The exact bytes are irrelevant here: the tracker only + // needs the tag to be of type VISIBILITY_TAG_TYPE, and the two markers to carry different ones. + private static final byte[] LABEL_A = new byte[] { 1 }; + private static final byte[] LABEL_B = new byte[] { 2 }; + + /** + * A column-level delete must not be considered redundant just because a same-qualifier + * DeleteColumn with a higher timestamp was already tracked: if they carry different labels they + * shadow disjoint cells. The base tracker's {@code coveredByColumn} branch would return true. + */ + @Test + public void differentLabelColumnDeleteIsNotRedundant() { + VisibilityScanDeleteTracker tracker = + new VisibilityScanDeleteTracker(CellComparatorImpl.COMPARATOR); + // Tracked first (newest ts, as a compaction scan would present it). + tracker.add(deleteMarker(QUALIFIER, 100L, KeyValue.Type.DeleteColumn, LABEL_A)); + + assertFalse( + tracker.isRedundantDelete(deleteMarker(QUALIFIER, 50L, KeyValue.Type.DeleteColumn, LABEL_B))); + assertFalse( + tracker.isRedundantDelete(deleteMarker(QUALIFIER, 50L, KeyValue.Type.Delete, LABEL_B))); + } + + /** + * A family-level delete must not be considered redundant just because a DeleteFamily with a + * higher timestamp set the family stamp: a label-less family delete only shadows label-less + * cells, while a labeled one shadows that label's cells. The base tracker's + * {@code coveredByFamily} branch would return true. + */ + @Test + public void differentLabelFamilyDeleteIsNotRedundant() { + VisibilityScanDeleteTracker tracker = + new VisibilityScanDeleteTracker(CellComparatorImpl.COMPARATOR); + // A label-less DeleteFamily is the only thing that advances familyStamp (see add()). + tracker.add(deleteMarker(null, 100L, KeyValue.Type.DeleteFamily, null)); + + assertFalse( + tracker.isRedundantDelete(deleteMarker(null, 50L, KeyValue.Type.DeleteFamily, LABEL_B))); + assertFalse(tracker + .isRedundantDelete(deleteMarker(null, 50L, KeyValue.Type.DeleteFamilyVersion, LABEL_B))); + } + + /** + * Even a same-label marker is kept: the base {@code coveredByColumn} branch would (safely) treat + * a same-qualifier, same-label DeleteColumn of lower timestamp as redundant, but the visibility + * tracker forgoes that optimization so the label-blind base logic can never run. This pins the + * deliberately conservative contract against a future change that re-enables it. + */ + @Test + public void sameLabelColumnDeleteIsNotRedundant() { + VisibilityScanDeleteTracker tracker = + new VisibilityScanDeleteTracker(CellComparatorImpl.COMPARATOR); + tracker.add(deleteMarker(QUALIFIER, 100L, KeyValue.Type.DeleteColumn, LABEL_A)); + + assertFalse( + tracker.isRedundantDelete(deleteMarker(QUALIFIER, 50L, KeyValue.Type.DeleteColumn, LABEL_A))); + } + + /** + * Even where the base answer would actually be correct (a label-less DeleteColumn covered by a + * label-less one of higher timestamp), the tracker still reports no redundancy, keeping the + * contract uniform across labeled and label-less markers. + */ + @Test + public void labelLessColumnDeleteIsNotRedundant() { + VisibilityScanDeleteTracker tracker = + new VisibilityScanDeleteTracker(CellComparatorImpl.COMPARATOR); + tracker.add(deleteMarker(QUALIFIER, 100L, KeyValue.Type.DeleteColumn, null)); + + assertFalse( + tracker.isRedundantDelete(deleteMarker(QUALIFIER, 50L, KeyValue.Type.DeleteColumn, null))); + } + + private static Cell deleteMarker(byte[] qualifier, long ts, KeyValue.Type type, byte[] label) { + List tags = label == null + ? null + : Collections.singletonList(new ArrayBackedTag(TagType.VISIBILITY_TAG_TYPE, label)); + return new KeyValue(ROW, FAMILY, qualifier, ts, type, null, tags); + } +}