HBASE-30267 Backport HBASE-30266 Do not drop visibility delete marker…

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();

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}.
+   * <p>
+   * 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<Void>) () -> {
+      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<Integer>) () -> {
+      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 {

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.
+ * <p>
+ * 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.
+ * <p>
+ * 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<Tag> tags = label == null
+      ? null
+      : Collections.singletonList(new ArrayBackedTag(TagType.VISIBILITY_TAG_TYPE, label));
+    return new KeyValue(ROW, FAMILY, qualifier, ts, type, null, tags);
+  }
+}