Skip to content
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

Adding support and examples for AOSS #216

Merged
merged 5 commits into from
Jan 22, 2023

Conversation

VachaShah
Copy link
Collaborator

@VachaShah VachaShah commented Jan 17, 2023

Signed-off-by: Vacha Shah vachshah@amazon.com

Description

Adding support and examples for AOSS.

Issues Resolved

#204

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.

@dblock
Copy link
Member

dblock commented Jan 17, 2023

I fully expect this not to be needed soon for AOSS, this is work in progress server-side.

@dblock
Copy link
Member

dblock commented Jan 17, 2023

The root API will be (re)added to OpenSearch Serverless.

I propose we remove the product check altogether, similar to opensearch-project/opensearch-ruby#130. We try to do a version check to see if the client is compatible with the server. I believe this was inherited from Elastic that was trying to maintain some sense of version compatibility without semver, but since we follow semver it's easy to know now whether your client will work against a server or not. My concern is that a version request every single time a new instance of a client is created is not insignificant overhead (in a web app example this could end up making a root request every time a web app serves a client search request!), and is an unexpected side effect when users try to use the client.

@VachaShah VachaShah force-pushed the aoss branch 2 times, most recently from 426f540 to 64e573a Compare January 18, 2023 23:36
@VachaShah
Copy link
Collaborator Author

I fully expect this not to be needed soon for AOSS, this is work in progress server-side.

Sounds good! I removed the content-length change from this PR.

@VachaShah
Copy link
Collaborator Author

The root API will be (re)added to OpenSearch Serverless.

I propose we remove the product check altogether, similar to opensearch-project/opensearch-ruby#130. We try to do a version check to see if the client is compatible with the server. I believe this was inherited from Elastic that was trying to maintain some sense of version compatibility without semver, but since we follow semver it's easy to know now whether your client will work against a server or not. My concern is that a version request every single time a new instance of a client is created is not insignificant overhead (in a web app example this could end up making a root request every time a web app serves a client search request!), and is an unexpected side effect when users try to use the client.

Sure! I removed the product info check altogether.

@VachaShah VachaShah changed the title [WIP] Removing content-length for signed headers to support AOSS [WIP] Adding support and examples for AOSS Jan 18, 2023
@VachaShah VachaShah changed the title [WIP] Adding support and examples for AOSS Adding support and examples for AOSS Jan 18, 2023
@VachaShah VachaShah marked this pull request as ready for review January 18, 2023 23:40
@VachaShah VachaShah requested a review from svencowart as a code owner January 18, 2023 23:40
@VachaShah
Copy link
Collaborator Author

@dblock This is ready for review.

@VachaShah VachaShah requested a review from dblock January 18, 2023 23:50
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make a separate PR with an entry that removes the version check, so we can make it very visible. Should we major increment version, too?

For docs I'd say managed and serverless work the same and the only thing you want to change is the service signing name. Let's say that in the user guide?

USER_GUIDE.md Outdated
- [How to use IAMs as authentication method](#how-to-use-iams-as-authentication-method)
- [AWS SDK V1](#aws-sdk-v1)
- [AWS SDK V2](#aws-sdk-v2)
- [AWS SDK V2 with OpenSearch Serverless](#aws-sdk-v2-with-opensearch-serverless)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tabs vs. spaces? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, fixed

USER_GUIDE.md Outdated
@@ -283,3 +286,91 @@ func main() {
}

```

#### AWS SDK V2 with OpenSearch Serverless
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should merge the managed and serverless portions, these are the same now no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combined them.

@VachaShah
Copy link
Collaborator Author

VachaShah commented Jan 20, 2023

I think we should make a separate PR with an entry that removes the version check, so we can make it very visible. Should we major increment version, too?

For docs I'd say managed and serverless work the same and the only thing you want to change is the service signing name. Let's say that in the user guide?

Sure! Created a separate PR #219 for removing the version check and incrementing the version.
Updated the user guide.

@VachaShah VachaShah requested a review from dblock January 20, 2023 06:41
dblock
dblock previously approved these changes Jan 20, 2023
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I have some potential nits for docs, feel free to ignore them and merge.

USER_GUIDE.md Show resolved Hide resolved
USER_GUIDE.md Outdated Show resolved Hide resolved
USER_GUIDE.md Outdated Show resolved Hide resolved
USER_GUIDE.md Outdated Show resolved Hide resolved
Signed-off-by: Vacha Shah <vachshah@amazon.com>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
@VachaShah
Copy link
Collaborator Author

@dblock I addressed the comments for documentation, it needs your approval once more.

dblock
dblock previously approved these changes Jan 21, 2023
Comment on lines +275 to +281
mapping := strings.NewReader(`{
"settings": {
"index": {
"number_of_shards": 4
}
}
}`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this mapping used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I added its usage

@@ -68,6 +68,7 @@ func (s *awsSdkV2Signer) SignRequest(r *http.Request) error {
}

hash, err := hexEncodedSha256OfRequest(r)
r.Header.Set("X-Amz-Content-Sha256", hash)
Copy link
Member

@VijayanB VijayanB Jan 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Shall we move this to constant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since its just used here, I kept it as a string

Signed-off-by: Vacha Shah <vachshah@amazon.com>
@VachaShah
Copy link
Collaborator Author

@VijayanB @dblock Review again please?

@VachaShah VachaShah merged commit d38a466 into opensearch-project:main Jan 22, 2023
@VachaShah VachaShah deleted the aoss branch January 22, 2023 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants