-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added opensearch serverless support #92
added opensearch serverless support #92
Conversation
371a713
to
a30f48d
Compare
I would like to mention that I'm fully aware that dragging hash header creation logic into this solution is a bad idea from maintenance perspective, but, since neither elastic client nor awsv4 signing client don't support this, it seems like the only way besides either creating a set of related PRs in dependant packages repositories or, which would probably be THE BEST WAY, completely switching to native go-opensearch client maintained by opensearch team which seems to support aforementioned requirement by default. |
We've tested this changes against our private opensearch serverless collection and it successfully created resources. |
I've run tests locally and they all passed. Can we re-try? It seems strange that flavor was set to 0 (Unknown) as it seems like docker hosted opensearch instance returned a valid version info response |
a30f48d
to
f526e6f
Compare
@prudhvigodithi could you please approve test run? |
9546c59
to
63a3b59
Compare
Thanks for your contribution @vasyaxparfenov. |
@vasyaxparfenov Thank you for this. Any idea when this could be released? |
Hey @vasyaxparfenov can you please add some unit tests to the change. |
0278ef7
to
d42fac5
Compare
Done |
@vasyaxparfenov I encounter an error while using this branch in a project. The creation of an index works fine, but when tags are updated in other resources, the It looks like there are some scenarios where the setting The steps are:
Actual Result:
Expected Result:Update tags and do nothing in relation to the index. My current configsTerraform version:
|
Thank you for a feedback, @lcapillera. By looking at source code of provider and elasticsearch client used in this solution I've noticed that aforementioned error is returned by provider itself while hiding actual error during client creation process. Could you please enable info level logging and run your setup? We should get a real error which happens during setup rather than generic one. |
@vasyaxparfenov The client error is
The OpenSearch Serverless cluster is up and running because I can recreate the index by doing:
|
Thank you, @lcapillera ! So this happens mainly when terraform performs requests to refresh its state? I made this assumption based on your original scenario and based on a fact you were able to remove resource from state and re-create it. Are you using this exact branch? |
@vasyaxparfenov It looks like the refreshing state only fails during the For instance, if I add this
But the tag change can be on any resource, not related to OpenSearch, for the index refreshing to fail. It's pretty strange. |
@vasyaxparfenov I narrowed down the problem a bit more, my current setup is this:
The index error started to happen when I added
to
I did that because it wasn't waiting for the collection to be created in the first run, so the provider was getting a null url. If I remove So it looks like I have 2 bugs and I can't fix both at the same time :D. |
Thank you for a detailed explanation. It seems like issue is related to aws provider resources. Did you try using collection_endpoint attribute? It will be same as using data_source from logical perspective, but it might impose better dependency relationship between opensearch provider and collection and help terraform understand your intention better. |
@lcapillera I would actually recommend you to use terragrunt to resolve this issue. On our project we use terragrunt and separate opensearch configuration and setup in two different modules with terragrunt explicit dependency which makes this modules run in specific order due to chicken and egg problem. Even despite my suggestion to use direct dependency to resource rather than using data_source, it won't solve a first run problem due to terraform not being able to handle such thing since it needs all provider to be configured before running refresh (unless you run terraform with target argument first time avoiding opensearch provider configuration until collection is created). One of the reasons why your configuration starts to fail after you add depends_on might be related to a lack of terraform knowledge when running refresh of data_source data due to pending update (new tags for example). |
@vasyaxparfenov Thank you for the links and suggestions. What you said makes sense. But I wonder if the issue I'm facing does occur with OpenSearch Service (as opposed to Serverless), because I don't see any issues in this repo and I guess not everybody uses terragrunt. |
@lcapillera could you please try using attribute from collection resource rather than data_source? Based on what is mentioned here we can't be sure that it provides previously used url to opensearch provider when you change inputs on dependant resource. I'm actually curious what it configure provider with. Did your run with logging enabled had any details regarding provider setup? Maybe if you try log level debug we could get some information. Nevertheless I would first try using resource reference explicitly |
@vasyaxparfenov I just tried using the attribute from the collection resource and it works in both cases, first run and later updates. The reason I added the data resource was because this wasn't working, but maybe I had something in a inconsistent state while trying different things. Thanks for your time, I'll let you know if I find anything else, but I guess this PR is ready to go 👏 |
@lcapillera I actually have an explanation why it doesn't work when you use data_source. So, it can't determine what attributes data_source is going to have because it depends on collection which is about to change - so it just passes empty string (default value) to provider and golang url.Parse API is ok with empty string being passed and returns url object with default fields. Then when you try to refresh your resource it calls getClient which when creating client checks url, if it doesn't have valid schema it's not added to connection list and then it has logic which checks active connection and that's where it fails because previous step skips empty url object. Mystery solved 🤣 |
b62f986
to
7d22515
Compare
Thanks for the feedback and the contribution, I will review this PR soon and lets this merged so that this feature is part of the upcoming release. Thanks |
Hey @vasyaxparfenov do you think its useful to add anything in the documentation? https://github.com/opensearch-project/terraform-provider-opensearch/blob/main/CONTRIBUTING.md#generate-documentation-using-tfplugindocs |
Signed-off-by: vasyaxparfenov <[email protected]>
Signed-off-by: vasyaxparfenov <[email protected]>
7d22515
to
cce6435
Compare
tfplugindocs generate didn't produce any changes, so I guess there is nothing important to add, the usage pattern hasn't changed so to use serverless variant you would just provide url which normally contains all necessary information |
Thanks @vasyaxparfenov, please make sure the CI runs are green and we can proceed to merge the PR.
|
@prudhvigodithi I've been running tests locally and I don't have similar issue. It might be a bug in tests logic since I can't find a logic path which would lead to Flavor being set to 0. |
provider/awsv4.go
Outdated
"net/http" | ||
) | ||
|
||
var emptyStringSHA256 = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, can we have the logic so that emptyStringSHA256
is generated dynamically ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @vasyaxparfenov, just one comment rest the PR LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Signed-off-by: vasyaxparfenov <[email protected]>
LGTM, @vasyaxparfenov, thanks for your contribution. |
Description
Added Opensearch Serverless support by:
Issues Resolved
Closes #90
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.