Enhance robustness of InternalOpenSearchSink tests through scenario-driven coverage, refactoring, and regression prevention

src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java

@@ -0,0 +1,186 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ *
+ * Modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */
+package org.opensearch.security.auditlog.sink;
+
+import java.util.Map;
+import java.util.Objects;
+
+import org.junit.Test;
+
+import org.opensearch.action.search.SearchRequest;
+import org.opensearch.action.search.SearchResponse;
+import org.opensearch.index.query.QueryBuilders;
+import org.opensearch.search.builder.SearchSourceBuilder;
+import org.opensearch.search.sort.SortOrder;
+import org.opensearch.test.framework.cluster.LocalCluster;
+import org.opensearch.test.framework.cluster.TestRestClient;
+import org.opensearch.transport.client.Client;
+
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
+import static org.hamcrest.Matchers.is;
+import static org.awaitility.Awaitility.await;
+
+/**
+ * Base class for {@link InternalOpenSearchSink} integration tests.
+ *
+ * <p>Holds test methods that are valid regardless of whether the audit target
+ * is a plain concrete index or a write alias. Each subclass provides its own
+ * {@code @ClassRule} {@link LocalCluster} and overrides
+ * {@link #cluster()} and {@link #auditTarget()} to supply the correct
+ * cluster and query pattern for that configuration variant.</p>
+ *
+ * <p>Using a shared base class avoids duplicating {@code testPersistsAuditEventsToTarget}
+ * and {@code testAuditDocumentContainsMandatoryFields} across the two concrete
+ * integration-test classes that differ only in cluster setup.</p>
+ *
+ * @see InternalOpenSearchSinkIntegrationTestConcreteIndex concrete date-based index variant
+ * @see InternalOpenSearchSinkIntegrationTestAuditAlias    write-alias variant
+ */
+abstract class AbstractInternalOpenSearchSinkIntegrationTest {
+
+    /**
+     * Returns the cluster configured for this test variant.
+     * Implementations must expose the {@code @ClassRule}-annotated cluster field.
+     */
+    abstract LocalCluster cluster();
+
+    /**
+     * Returns the index name or alias used to query audit documents.
+     *
+     * <p>Examples:</p>
+     * <ul>
+     *   <li>{@code "security-auditlog-*"} — default date-based variant</li>
+     *   <li>{@code "security-audit-write-alias"} — alias variant</li>
+     * </ul>
+     */
+    abstract String auditTarget();
+
+    // -----------------------------------------------------------------------
+    // Shared helpers
+    // -----------------------------------------------------------------------
+
+    /** Counts all audit documents reachable through {@link #auditTarget()}. */
+    long countAuditDocs(Client client) {
+        return Objects.requireNonNull(
+            client.search(new SearchRequest(auditTarget()).source(new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0)))
+                .actionGet()
+                .getHits()
+                .getTotalHits()
+        ).value();
+    }
+
+    /** Refreshes the audit target so newly indexed documents become searchable. */
+    void refreshAuditTarget(Client client) {
+        client.admin().indices().prepareRefresh(auditTarget()).get();
+    }
+
+    /** Issues an authenticated REST GET that triggers one {@code GRANTED_PRIVILEGES} audit event. */
+    void generateAuditEvent(String path) {
+        try (TestRestClient restClient = cluster().getRestClient(cluster().getAdminCertificate())) {
+            restClient.get(path);
+        }
+    }
+
+    // -----------------------------------------------------------------------
+    // Shared tests — valid for both date-based index and alias configurations
+    // -----------------------------------------------------------------------
+
+    /**
+     * Verifies that multiple audit events are successfully persisted to the audit target.
+     *
+     * <p><b>Tested Code Path:</b> From the second event onward,
+     * {@code createIndexIfAbsent()} detects the target already exists
+     * (via {@code metadata.hasIndex()} for concrete indices or
+     * {@code metadata.hasAlias()} for aliases) and returns {@code true}
+     * without attempting recreation. Successful persistence of both events
+     * confirms the early-return branch works correctly.</p>
+     */
+    @Test
+    public void testPersistsAuditEventsToTarget() {
+        try (Client client = cluster().getInternalNodeClient()) {
+            long before = countAuditDocs(client);
+
+            generateAuditEvent("_cluster/health");
+            generateAuditEvent("_cluster/stats");
+            generateAuditEvent("_nodes");
+
+            await().atMost(10, SECONDS).pollInterval(200, MILLISECONDS).untilAsserted(() -> {
+                refreshAuditTarget(client);
+                assertThat("At least 3 new audit events must be persisted", countAuditDocs(client) - before, greaterThanOrEqualTo(3L));
+            });
+        }
+    }
+
+    /**
+     * Validates that audit documents contain all mandatory fields defined by the
+     * audit log specification.
+     *
+     * <p><b>Core Fields (all events):</b></p>
+     * <ul>
+     *   <li>{@code audit_category} — event classification (e.g., {@code GRANTED_PRIVILEGES})</li>
+     *   <li>{@code audit_request_layer} — processing layer; expected value: {@code REST}</li>
+     *   <li>{@code audit_request_origin} — origin layer; expected value: {@code REST}</li>
+     *   <li>{@code @timestamp} — event timestamp</li>
+     * </ul>
+     *
+     * <p><b>REST-Specific Fields:</b></p>
+     * <ul>
+     *   <li>{@code audit_rest_request_method} — HTTP method (e.g., {@code GET})</li>
+     *   <li>{@code audit_rest_request_path} — request path (e.g., {@code /_cluster/health})</li>
+     * </ul>
+     *
+     * <p><b>Absent Fields (transport audit disabled):</b></p>
+     * <ul>
+     *   <li>{@code audit_transport_request_type} — must not be present</li>
+     * </ul>
+     */
+    @Test
+    public void testAuditDocumentContainsMandatoryFields() {
+        try (Client client = cluster().getInternalNodeClient()) {
+            long before = countAuditDocs(client);
+
+            generateAuditEvent("_cluster/health");
+
+            await().atMost(10, SECONDS).pollInterval(200, MILLISECONDS).untilAsserted(() -> {
+                refreshAuditTarget(client);
+                assertThat("Test must generate at least one new event", countAuditDocs(client) - before, greaterThan(0L));
+            });
+
+            SearchResponse response = client.search(
+                new SearchRequest(auditTarget()).source(
+                    new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(1).sort("@timestamp", SortOrder.DESC)
+                )
+            ).actionGet();
+
+            assertThat(
+                "At least one audit document must exist",
+                Objects.requireNonNull(response.getHits().getTotalHits()).value(),
+                greaterThan(0L)
+            );
+
+            Map<String, Object> doc = response.getHits().getAt(0).getSourceAsMap();
+
+            assertThat("Missing field: audit_category", doc.containsKey("audit_category"), is(true));
+            assertThat("Missing field: audit_request_layer", doc.containsKey("audit_request_layer"), is(true));
+            assertThat("Wrong value: audit_request_layer", doc.get("audit_request_layer"), is("REST"));
+            assertThat("Missing field: audit_request_origin", doc.containsKey("audit_request_origin"), is(true));
+            assertThat("Wrong value: audit_request_origin", doc.get("audit_request_origin"), is("REST"));
+            assertThat("Missing field: @timestamp", doc.containsKey("@timestamp"), is(true));
+            assertThat("Missing field: audit_rest_request_method", doc.containsKey("audit_rest_request_method"), is(true));
+            assertThat("Missing field: audit_rest_request_path", doc.containsKey("audit_rest_request_path"), is(true));
+            assertThat("Unexpected field: audit_transport_request_type", doc.containsKey("audit_transport_request_type"), is(false));
+        }
+    }
+}

src/integrationTest/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkIntegrationTestAuditAlias.java

@@ -29,7 +29,6 @@
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
 import static org.awaitility.Awaitility.await;
 
@@ -42,8 +41,14 @@
  * Tests share a single cluster and use a before/after delta pattern so that
  * execution order does not matter.</p>
  *
+ * <p>Shared tests ({@code testPersistsAuditEventsToTarget} and
+ * {@code testAuditDocumentContainsMandatoryFields}) are inherited from
+ * {@link AbstractInternalOpenSearchSinkIntegrationTest}.</p>
+ *
+ * @see InternalOpenSearchSinkIntegrationTestConcreteIndex for the concrete date-based index variant
+ * @see InternalOpenSearchSinkTest for unit tests covering exception and race-condition branches
  */
-public class InternalOpenSearchSinkIntegrationTestAuditAlias {
+public class InternalOpenSearchSinkIntegrationTestAuditAlias extends AbstractInternalOpenSearchSinkIntegrationTest {
 
     private static final String AUDIT_ALIAS = "security-audit-write-alias";
     private static final String BACKING_INDEX = "security-audit-backend-000001";
@@ -52,27 +57,21 @@ public class InternalOpenSearchSinkIntegrationTestAuditAlias {
     static final TestAlias auditAlias = new TestAlias(AUDIT_ALIAS).on(backingIndex).writeIndex(backingIndex);
 
     @ClassRule
-    public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE)
+    public static final LocalCluster CLUSTER = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE)
         .nodeSettings(Map.of("plugins.security.audit.config.index", AUDIT_ALIAS))
         .internalAudit(new AuditConfiguration(true).filters(new AuditFilters().enabledRest(true).enabledTransport(false)))
         .indices(backingIndex)
         .aliases(auditAlias)
         .build();
 
-    /** Counts all audit documents reachable through the write alias. */
-    private long countAuditDocs(TestRestClient client) {
-        HttpResponse response = client.postJson(AUDIT_ALIAS + "/_search", """
-            {"query": {"match_all": {}}, "size": 0}
-            """);
-        response.assertStatusCode(200);
-        return response.getLongFromJsonBody("/hits/total/value");
+    @Override
+    LocalCluster cluster() {
+        return CLUSTER;
     }
 
-    /** Issues an authenticated REST GET that triggers an {@code AUTHENTICATED} audit event. */
-    private void generateAuditEvent(String path) {
-        try (TestRestClient restClient = cluster.getRestClient(cluster.getAdminCertificate())) {
-            restClient.get(path);
-        }
+    @Override
+    String auditTarget() {
+        return AUDIT_ALIAS;
     }
 
     /**
@@ -81,13 +80,23 @@ private void generateAuditEvent(String path) {
      *
      * <p>Generates one event, then checks that the alias still resolves to the
      * backing index and no spurious concrete index was created.</p>
+     *
+     * <p><b>Tested Code Path:</b> {@code metadata.hasAlias(indexName)} returns
+     * {@code true}, so the method logs a debug message and returns {@code true}
+     * immediately without attempting index creation.</p>
      */
     @Test
     public void testRecognizesAuditTargetAsWriteAlias() {
-        try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) {
+        try (TestRestClient client = CLUSTER.getRestClient(CLUSTER.getAdminCertificate())) {
             generateAuditEvent("_cluster/health");
 
-            await().until(() -> countAuditDocs(client) > 0);
+            await().until(() -> {
+                HttpResponse countResponse = client.postJson(AUDIT_ALIAS + "/_search", """
+                    {"query": {"match_all": {}}, "size": 0}
+                    """);
+                countResponse.assertStatusCode(200);
+                return countResponse.getLongFromJsonBody("/hits/total/value") > 0;
+            });
 
             HttpResponse aliasResponse = client.get("_alias/" + AUDIT_ALIAS);
             aliasResponse.assertStatusCode(200);
@@ -106,55 +115,4 @@ public void testRecognizesAuditTargetAsWriteAlias() {
             assertThat("Backing index must exist physically", indexExistsResponse.getStatusCode(), is(200));
         }
     }
-
-    /**
-     * The alias branch is invoked on every {@code doStore} call.
-     * Generates three distinct events and asserts all are persisted, confirming
-     * that repeated writes through the alias succeed.
-     */
-    @Test
-    public void testWritesEventsToAliasSuccessfully() {
-        try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) {
-            long before = countAuditDocs(client);
-
-            generateAuditEvent("_cluster/health");
-            generateAuditEvent("_cluster/stats");
-            generateAuditEvent("_nodes/info");
-
-            await().untilAsserted(
-                () -> assertThat("At least 3 events must be written through alias", countAuditDocs(client) - before, greaterThan(2L))
-            );
-        }
-    }
-
-    /**
-     * Documents written via the alias must contain the same mandatory audit fields
-     * as those written to a concrete index (category, timestamp, REST method/path,
-     * layer and origin). Transport-specific fields must be absent since transport
-     * audit is disabled.
-     */
-    @Test
-    public void testAuditDocumentsViaAliasContainMandatoryFields() {
-        try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) {
-            long before = countAuditDocs(client);
-            generateAuditEvent("_cluster/health");
-
-            await().until(() -> countAuditDocs(client) > before);
-
-            HttpResponse response = client.postJson(AUDIT_ALIAS + "/_search", """
-                {"query": {"match_all": {}}, "size": 1, "sort": [{"@timestamp": "desc"}]}
-                """);
-            response.assertStatusCode(200);
-
-            JsonNode source = response.bodyAsJsonNode().get("hits").get("hits").get(0).get("_source");
-
-            assertThat(source.has("audit_category"), is(true));
-            assertThat(source.has("@timestamp"), is(true));
-            assertThat(source.has("audit_rest_request_method"), is(true));
-            assertThat(source.has("audit_rest_request_path"), is(true));
-            assertThat(source.get("audit_request_layer").asText(), is("REST"));
-            assertThat(source.get("audit_request_origin").asText(), is("REST"));
-            assertThat(source.has("audit_transport_request_type"), is(false));
-        }
-    }
 }