From b789e7faeb930100703010c86cf82ba92eeb4f7a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 19 Jan 2024 14:48:57 +0000 Subject: [PATCH] Validate 409s occur when multiple config updates happen simultaneously (#3909) ### Description Added new TenantAsyncActionTest.java file to put api unit tests for the /api/tenants api call- additionally wrote new unit test to test for parallel PUT /tenant/{name} calls, that they successfully return a 409 call and retry the call. * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Test fix * Why these changes are required? There was no test that checked for the behavior mentioned in the linked ticket, so previously we didn't know if the issue still existed or not (it apparently was fixed before), to make sure this doesn't happen again for this particular case, I decided to write a concrete test for it. * What is the old behavior before changes and new behavior after changes? Test was added, there's no behavior change? ### Issues Resolved - #1402 - added unit test to cover bug in this issue ### Testing Added new tenant action unit test to test parallel tenant api calls. ### Check List - [x] New functionality includes testing - [x] New functionality has been documented - [x] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Prabhas Kurapati (cherry picked from commit 15f5b7b8514c5ef6ba4771e4c182086cf5c21362) Signed-off-by: github-actions[bot] --- .../security/SecurityConfigurationTests.java | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java index 6b04737d18..3889fa2a3c 100644 --- a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java @@ -13,8 +13,13 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.http.HttpStatus; import org.awaitility.Awaitility; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -24,6 +29,7 @@ import org.junit.runner.RunWith; import org.opensearch.client.Client; +import org.opensearch.test.framework.AsyncActions; import org.opensearch.test.framework.TestSecurityConfig.Role; import org.opensearch.test.framework.TestSecurityConfig.User; import org.opensearch.test.framework.certificate.TestCertificates; @@ -33,6 +39,8 @@ import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; import static org.opensearch.security.support.ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST; @@ -229,4 +237,39 @@ public void shouldUseSecurityAdminTool() throws Exception { .until(() -> client.get("_plugins/_security/api/rolesmapping/readall").getStatusCode(), equalTo(200)); } } + + @Test + public void testParallelTenantPutRequests() throws Exception { + final String TENANT_ENDPOINT = "_plugins/_security/api/tenants/tenant1"; + final String TENANT_BODY = "{\"description\":\"create new tenant\"}"; + final String TENANT_BODY_TWO = "{\"description\":\"update tenant\"}"; + + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + + final CountDownLatch countDownLatch = new CountDownLatch(1); + final List> conflictingRequests = AsyncActions.generate(() -> { + countDownLatch.await(); + return client.putJson(TENANT_ENDPOINT, TENANT_BODY); + }, 4, 4); + + // Make sure all requests start at the same time + countDownLatch.countDown(); + + AtomicInteger numCreatedResponses = new AtomicInteger(); + AsyncActions.getAll(conflictingRequests, 1, TimeUnit.SECONDS).forEach((response) -> { + assertThat(response.getStatusCode(), anyOf(equalTo(HttpStatus.SC_CREATED), equalTo(HttpStatus.SC_CONFLICT))); + if (response.getStatusCode() == HttpStatus.SC_CREATED) numCreatedResponses.getAndIncrement(); + }); + assertThat(numCreatedResponses.get(), equalTo(1)); // should only be one 201 + + TestRestClient.HttpResponse getResponse = client.get(TENANT_ENDPOINT); // make sure the one 201 works + assertThat(getResponse.getBody(), containsString("create new tenant")); + + TestRestClient.HttpResponse updateResponse = client.putJson(TENANT_ENDPOINT, TENANT_BODY_TWO); + assertThat(updateResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); + + getResponse = client.get(TENANT_ENDPOINT); // make sure update works + assertThat(getResponse.getBody(), containsString("update tenant")); + } + } }