Open Sovietaced opened 10 months ago
Just tested this locally and it works fine for basic auth workflows.
Thanks @Sovietaced for your contribution, is this a similar problem https://github.com/opensearch-project/terraform-provider-opensearch/issues/102 that can be solved with this PR? @bbarani
Thanks @Sovietaced for your contribution, is this a similar problem https://github.com/opensearch-project/terraform-provider-opensearch/issues/102 that can be solved with this PR?
@bbarani
Possibly. I'm not sure if #102 has been root caused
Thanks @Sovietaced, Can you please add some unit tests to test this change? Thanks
I see one of your another PR https://github.com/opensearch-project/terraform-provider-opensearch/pull/114 (~ same subject), any co-relation between these 2 PR's?
I assume this PR https://github.com/opensearch-project/terraform-provider-opensearch/pull/117 is to disable sigv4 (signAWSRequests) automatically when basic auth is enabled and this one https://github.com/opensearch-project/terraform-provider-opensearch/pull/114 is to tell the user to disable the signAWSRequests
manually ?
I see one of your another PR #114 (~ same subject), any co-relation between these 2 PR's?
They are related. I made them separate pull requests because I wasn't sure how receptive you would be to a code change while a doc change is pretty straightforward.
Thanks @Sovietaced, Can you please add some unit tests to test this change? Thanks
I had looked into adding unit tests but I don't see any existing coverage for basic authentication. I'm not even sure how I would observe that the options have been set on an HTTP client (besides setting up a mock HTTP server) which seems like quite a bit of work.
Looks to me like if this PR goes in we dont have to explicitly disable the request signing right ? This PR https://github.com/opensearch-project/terraform-provider-opensearch/pull/114 mentions to disable it. @Sovietaced
Looks to me like if this PR goes in we dont have to explicitly disable the request signing right ? This PR #114 mentions to disable it. @Sovietaced
That is correct. If the documentation PR merges I will rebase this. If we'd like to merge this (and close the documentation PR), I can update the documentation accordingly.
I will just prepare this pull request to remove the option to disable request signing.
Thanks @Sovietaced I will go ahead and merge the documentation PR https://github.com/opensearch-project/terraform-provider-opensearch/pull/114, please try to add some unit tests for this PR and then we can proceed to merge this. WDYT? Thanks
Thanks @Sovietaced I will go ahead and merge the documentation PR #114, please try to add some unit tests for this PR and then we can proceed to merge this. WDYT? Thanks
Sounds good. Taking a look at things now.
@prudhvigodithi I took a look at the test code here and I don't think there is much I can really do. There exists zero unit test code that targets basic authentication, and it makes sense because provider.getClient
is not very unit-testable. The return value of provider.getClient
is a struct that we have no visibility into. The area where I am modifying code is reasoning about ClientOptionFunc
s which are not even comparable as far as I know so I cannot refactor that into a method that is callable from unit tests. Since we have no visibility into elastic7.Client
all we could really do is observe its behavior against a test HTTP server which isn't great.
It looks like the existing code had resorted to one of the following:
provider.getClient
into functions like awsSession
that can be called within unit tests and reasonable asserted against. If we really care about preventing regressions against AWS Opensearch, there should be a way to run acceptance/integration tests against AWS Opensearch. Given that AWS Opensearch is closed source and cannot be run locally by the public, the only reasonable way to run acceptance/integration tests is to run actual integration tests against an ephemeral instance of AWS Opensearch on AWS, IMO. And that is something I don't think I should be responsible for setting up.
Please let me know if you had any other ideas around unit testing.
@prudhvigodithi The changes are working for me, the following terraform code worked using this PR https://github.com/rblcoder/terraform-opensearch-samples/blob/main/aws_opensearch_basic_auth_auto_PR117/main.tf
@prudhvigodithi Can we merge this PR?
Hey @Sovietaced can you please take care of the conflicts, we can proceed to merge this PR. Thanks
Description
Disable request signing when basic auth is enabled so that AWS request signing doesn't have to be explicitly enabled. Also removes the option to configure whether requests should be signed since it should always be disabled when basic auth is enabled.
Issues Resolved
116
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.