[BUG] Return HTTP 409 on version confict when creating & updating tenants #1402
Labels
enhancement
New feature or request
good first issue
These are recommended starting points for newcomers looking to make their first contributions.
triaged
Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Comments
phillbaker
added
Beta
bug
vrozov
removed
Beta
untriaged
davidlago
added
enhancement
davidlago
added
the
triaged
|
[Triage] Issue is still relevant and being labelled as a "good first issue". Thank you. |
stephen-crawford
added
the
good first issue
|
This issue might be deprecated because when I try to test it I get a 409 error, let me know if I'm doing anything wrong here: |
|
From the code, it does look like this may have been resolved at one point. Thank you for confirming. The relevant API is TenantsApiAction which is now a subclass of AbstractApiAction which has support for handling conflicts. FYI HTTP status code 409 is the status code for conflict. |
|
@prabhask5 if a test doesn't exist around this behavior, would it make sense to create one? I see a similar test for the Users API endpoints here, but I don't see any around tenants. |
Sure! I can make a PR to add this test |
3 tasks
3 tasks
3 tasks
cwperks
pushed a commit
that referenced
this issue
Jan 19, 2024
#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 <[email protected]>
opensearch-trigger-bot bot
pushed a commit
that referenced
this issue
Jan 19, 2024
#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 <[email protected]> (cherry picked from commit 15f5b7b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dlin2028
pushed a commit
to dlin2028/security
that referenced
this issue
May 1, 2024
opensearch-project#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 - opensearch-project#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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
Hello, I opened #1095 earlier, which has recently been fixed (thanks!), but it looks like a HTTP 500 (instead of 409) is being returned for the tenant endpoint as well. This is related to phillbaker/terraform-provider-elasticsearch#206.
When OpenDistro receives parallel requests to create or update tenants (
/_opendistro/_security/api/tenants/{name}), it returns an HTTP 500 response.Similar to Elasticsearch itself, these conflict exceptions should return a http 409 instead of a 500 to signal that this is an expect error case and a retry is expected.
To Reproduce
Steps to reproduce the behavior:
/_opendistro/_security/api/tenants/tenant_1,/_opendistro/_security/api/tenants/tenant_2Expected behavior
A HTTP 409 instead of HTTP 500.
Plugins
Opendistro
Screenshots
N/A
Host/Environment (please complete the following information):
Additional context
N/A
The text was updated successfully, but these errors were encountered: