From 6bdaf0b20dea94265c09525f8f170c9ff4ff450e Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 25 Oct 2024 16:01:03 -0400 Subject: [PATCH] [2.18] Fix flaky integ tests (#4844) Signed-off-by: Craig Perkins --- .../security/SecurityConfigurationTests.java | 5 +- .../opensearch/security/SystemIndexTests.java | 8 ++ .../CertificatesRestApiIntegrationTest.java | 4 +- .../LegacyConfigV6AutoConversionTest.java | 30 +----- .../security/legacy/LegacyConfigV6Test.java | 97 +++++++++++++++++++ .../test/framework/cluster/PortAllocator.java | 17 +++- 6 files changed, 125 insertions(+), 36 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6Test.java diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java index dc2c82c188..6d7675abc5 100644 --- 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 index add98ca572..ae068255da 100644 --- 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 index 8a69406bff..cfcf404ffa 100644 --- 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/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java b/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java index c3ef83528a..8a088c615d 100644 --- a/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java +++ b/src/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 new file mode 100644 index 0000000000..24ab41597a --- /dev/null +++ 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 index 139378fd22..d9384dda61 100644 --- 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 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 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);