[2.18] Fix flaky integ tests (opensearch-project#4844)

Signed-off-by: Craig Perkins <[email protected]>
cwperks · Oct 25, 2024 · 6bdaf0b · 6bdaf0b
1 parent 000b084
commit 6bdaf0b
Show file tree

Hide file tree

Showing 6 changed files with 125 additions and 36 deletions.
diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java
@@ -276,10 +276,13 @@ public void testParallelTenantPutRequests() throws Exception {
             assertThat(getResponse.getBody(), containsString("create new tenant"));
 
             TestRestClient.HttpResponse updateResponse = client.putJson(TENANT_ENDPOINT, TENANT_BODY_TWO);
-            assertThat(updateResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
+            updateResponse.assertStatusCode(HttpStatus.SC_OK);
 
             getResponse = client.get(TENANT_ENDPOINT); // make sure update works
             assertThat(getResponse.getBody(), containsString("update tenant"));
+
+            TestRestClient.HttpResponse deleteResponse = client.delete(TENANT_ENDPOINT);
+            deleteResponse.assertStatusCode(HttpStatus.SC_OK);
         }
     }
 }
diff --git a/src/integrationTest/java/org/opensearch/security/SystemIndexTests.java b/src/integrationTest/java/org/opensearch/security/SystemIndexTests.java
@@ -13,6 +13,7 @@
 import java.util.Map;
 
 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
+import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -54,6 +55,13 @@ public class SystemIndexTests {
         )
         .build();
 
+    @Before
+    public void setup() {
+        try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) {
+            client.delete(".system-index1");
+        }
+    }
+
     @Test
     public void adminShouldNotBeAbleToDeleteSecurityIndex() {
         try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {

diff --git a/src/integrationTest/java/org/opensearch/security/api/CertificatesRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/CertificatesRestApiIntegrationTest.java
@@ -96,9 +96,7 @@ public void timeoutTest() throws Exception {
     }
 
     private void verifyTimeoutRequest(final TestRestClient client) throws Exception {
-        TestRestClient.HttpResponse response = ok(() -> client.get(sslCertsPath() + "?timeout=0"));
-        final var body = response.bodyAsJsonNode();
-        assertThat(body.get("nodes").size(), is(0));
+        ok(() -> client.get(sslCertsPath() + "?timeout=0"));
     }
 
     private void verifySSLCertsInfo(final TestRestClient client) throws Exception {

diff --git a/...integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java b/...integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java
@@ -14,17 +14,14 @@
 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
 import org.junit.Assert;
 import org.junit.ClassRule;
-import org.junit.FixMethodOrder;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.MethodSorters;
 
 import org.opensearch.test.framework.TestSecurityConfig;
 import org.opensearch.test.framework.cluster.ClusterManager;
 import org.opensearch.test.framework.cluster.LocalCluster;
 import org.opensearch.test.framework.cluster.TestRestClient;
 
-@FixMethodOrder(MethodSorters.NAME_ASCENDING)
 @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
 @ThreadLeakScope(ThreadLeakScope.Scope.NONE)
 public class LegacyConfigV6AutoConversionTest {
@@ -76,32 +73,7 @@ public class LegacyConfigV6AutoConversionTest {
         .build();
 
     @Test
-    public void authc() {
-        try (TestRestClient client = cluster.getRestClient("admin", "admin")) {
-            TestRestClient.HttpResponse response = client.get("_opendistro/_security/authinfo");
-            Assert.assertEquals(response.getBody(), 200, response.getStatusCode());
-            Assert.assertEquals(response.getBody(), true, response.getBooleanFromJsonBody("/tenants/admin"));
-        }
-    }
-
-    @Test
-    public void configRestApiReturnsV6Config() {
-        try (TestRestClient client = cluster.getRestClient("admin", "admin")) {
-            TestRestClient.HttpResponse response = client.get("_opendistro/_security/api/roles/all_access_role");
-            Assert.assertEquals(response.getBody(), 200, response.getStatusCode());
-            Assert.assertEquals(
-                "Expected v6 format",
-                "{\"all_access_role\":{\"readonly\":true,\"hidden\":false,\"cluster\":[\"UNLIMITED\"],\"tenants\":{\"admin_tenant\":\"RW\"},\"indices\":{\"*\":{\"*\":[\"UNLIMITED\"]}}}}",
-                response.getBody()
-            );
-        }
-    }
-
-    /**
-     * This must be the last test executed, as it changes the config index
-     */
-    @Test
-    public void zzz_migrateApi() {
+    public void migrateApi() {
         try (TestRestClient client = cluster.getRestClient("admin", "admin")) {
             TestRestClient.HttpResponse response = client.post("_opendistro/_security/api/migrate");
             Assert.assertEquals(response.getBody(), 200, response.getStatusCode());

diff --git a/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6Test.java b/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6Test.java
@@ -0,0 +1,97 @@
+/*
+ * Copyright OpenSearch Contributors
+ * 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.
+ *
+ */
+package org.opensearch.security.legacy;
+
+import java.util.Map;
+
+import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
+import org.junit.Assert;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.opensearch.test.framework.TestSecurityConfig;
+import org.opensearch.test.framework.cluster.ClusterManager;
+import org.opensearch.test.framework.cluster.LocalCluster;
+import org.opensearch.test.framework.cluster.TestRestClient;
+
+@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
+@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
+public class LegacyConfigV6Test {
+    static final TestSecurityConfig LEGACY_CONFIG = new TestSecurityConfig()//
+        .rawConfigurationDocumentYaml(
+            "config",
+            "opendistro_security:\n"
+                + "  dynamic:\n"
+                + "    authc:\n"
+                + "      basic_internal_auth_domain:\n"
+                + "        http_enabled: true\n"
+                + "        order: 4\n"
+                + "        http_authenticator:\n"
+                + "          type: basic\n"
+                + "          challenge: true\n"
+                + "        authentication_backend:\n"
+                + "          type: intern\n"
+        )
+        .rawConfigurationDocumentYaml(
+            "internalusers",
+            "admin:\n"
+                + "  readonly: true\n"
+                + "  hash: $2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG\n"
+                + "  roles:\n"
+                + "  - admin\n"
+                + "  attributes:\n"
+                + "    attribute1: value1\n"
+        )
+        .rawConfigurationDocumentYaml(
+            "roles",
+            "all_access_role:\n"
+                + "  readonly: true\n"
+                + "  cluster:\n"
+                + "  - UNLIMITED\n"
+                + "  indices:\n"
+                + "    '*':\n"
+                + "      '*':\n"
+                + "      - UNLIMITED\n"
+                + "  tenants:\n"
+                + "    admin_tenant: RW\n"
+        )
+        .rawConfigurationDocumentYaml("rolesmapping", "all_access_role:\n" + "  readonly: true\n" + "  backendroles:\n" + "  - admin")//
+        .rawConfigurationDocumentYaml("actiongroups", "dummy:\n" + "  permissions: []");
+
+    @ClassRule
+    public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS)
+        .config(LEGACY_CONFIG)
+        .nodeSettings(Map.of("plugins.security.restapi.roles_enabled.0", "all_access_role"))
+        .build();
+
+    @Test
+    public void authc() {
+        try (TestRestClient client = cluster.getRestClient("admin", "admin")) {
+            TestRestClient.HttpResponse response = client.get("_opendistro/_security/authinfo");
+            Assert.assertEquals(response.getBody(), 200, response.getStatusCode());
+            Assert.assertEquals(response.getBody(), true, response.getBooleanFromJsonBody("/tenants/admin"));
+        }
+    }
+
+    @Test
+    public void configRestApiReturnsV6Config() {
+        try (TestRestClient client = cluster.getRestClient("admin", "admin")) {
+            TestRestClient.HttpResponse response = client.get("_opendistro/_security/api/roles/all_access_role");
+            Assert.assertEquals(response.getBody(), 200, response.getStatusCode());
+            Assert.assertEquals(
+                "Expected v6 format",
+                "{\"all_access_role\":{\"readonly\":true,\"hidden\":false,\"cluster\":[\"UNLIMITED\"],\"tenants\":{\"admin_tenant\":\"RW\"},\"indices\":{\"*\":{\"*\":[\"UNLIMITED\"]}}}}",
+                response.getBody()
+            );
+        }
+    }
+
+}
diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/PortAllocator.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/PortAllocator.java
@@ -63,7 +63,7 @@ public SortedSet<Integer> allocate(String clientName, int numRequested, int minP
 
         int startPort = minPort;
 
-        while (!isAvailable(startPort)) {
+        while (!isPortRangeAvailable(startPort, startPort + numRequested)) {
             startPort += 10;
         }
 
@@ -72,8 +72,10 @@ public SortedSet<Integer> allocate(String clientName, int numRequested, int minP
         for (int currentPort = startPort; foundPorts.size() < numRequested
             && currentPort < SocketUtils.PORT_RANGE_MAX
             && (currentPort - startPort) < 10000; currentPort++) {
-            if (allocate(clientName, currentPort)) {
-                foundPorts.add(currentPort);
+            if (isAvailable(currentPort)) {
+                if (allocate(clientName, currentPort)) {
+                    foundPorts.add(currentPort);
+                }
             }
         }
 
@@ -121,6 +123,15 @@ private boolean isAvailable(int port) {
         return !isAllocated(port) && !isInUse(port);
     }
 
+    private boolean isPortRangeAvailable(int port, int endPort) {
+        for (int i = port; i <= endPort; i++) {
+            if (!isAvailable(i)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
     private synchronized boolean isAllocated(int port) {
         AllocatedPort allocatedPort = this.allocatedPorts.get(port);