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

FIX: extract reason from error in AWSSdk2Transport #493

Merged

Conversation

chenqi0805
Copy link
Contributor

Description

This is for resolving missing cause message when there is no proper permission to access Fine grained Access Control Amazon Opensearch domain.

Exception in thread "main" org.opensearch.client.opensearch._types.OpenSearchException: Request failed: [security_exception] authentication/authorization failure
	at org.opensearch.client.transport.aws.AwsSdk2Transport.parseResponse(AwsSdk2Transport.java:517)
	at org.opensearch.client.transport.aws.AwsSdk2Transport.executeSync(AwsSdk2Transport.java:438)
	at org.opensearch.client.transport.aws.AwsSdk2Transport.performRequest(AwsSdk2Transport.java:241)
	at org.opensearch.client.opensearch.OpenSearchClient.bulk(OpenSearchClient.java:211)
	at org.opensearch.dataprepper.plugins.sink.opensearch.index.TestClient.testBulk(TestClient.java:215)
	at org.opensearch.dataprepper.plugins.sink.opensearch.index.TestClient.main(TestClient.java:80)

Issues Resolved

Closes #473

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.

Signed-off-by: George Chen <[email protected]>
@wbeckler
Copy link

Thank you for submitting this! Can you think of a scenario in which an application might be relying on the previous behavior? As in, could this possibly be a breaking change?

@chenqi0805
Copy link
Contributor Author

@wbeckler The only scenario I can think of is when both reason and message is present but so far it is hypothetical as I have not found such case. Do you have other scenario in your head?

@wbeckler
Copy link

@wbeckler The only scenario I can think of is when both reason and message is present but so far it is hypothetical as I have not found such case. Do you have other scenario in your head?

Not really. What you're saying makes sense, but I ask the question in case there's something I'm not thinking of.

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

LGTM! Just a minor comment.

CHANGELOG.md Outdated

>>>>>>> main
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Remove this line.

@chenqi0805 chenqi0805 requested a review from VachaShah May 22, 2023 19:56
VachaShah
VachaShah previously approved these changes May 22, 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, but needs tests please.

Signed-off-by: George Chen <[email protected]>
@chenqi0805
Copy link
Contributor Author

@dblock Thanks for the suggestion. I do not find a good strategy to unit test so I put a AwsSdk2SecurityIT that needs to be run against a FGAC without proper permission.

Signed-off-by: George Chen <[email protected]>
@dblock
Copy link
Member

dblock commented May 23, 2023

Without looking at the code, do we not have a way to mock a response? If it's really hard, care to open an issue to add tests that exercise all these paths and we'll merge as is? Also @reta wdyt?

@chenqi0805
Copy link
Contributor Author

@dblock I agree we should open issue to use unit test to cover paths in Awssdk2Transport. ref: #503

@reta
Copy link
Collaborator

reta commented May 23, 2023

Without looking at the code, do we not have a way to mock a response? If it's really hard, care to open an issue to add tests that exercise all these paths and we'll merge as is? Also @reta wdyt?

Sounds reasonable @dblock , we may need to come up with some mocks (like test fixtures we do in core) but it may take some efforts.

@reta
Copy link
Collaborator

reta commented May 31, 2023

LGMT! @chenqi0805 there are conflicts, could you please resolve them? thank you

@chenqi0805
Copy link
Contributor Author

@reta resolved conflict

@reta
Copy link
Collaborator

reta commented Jun 1, 2023

@dblock LGTY? thanks!

@chenqi0805
Copy link
Contributor Author

@dblock Need your approval for unblocking merge

@dblock dblock merged commit 89d154d into opensearch-project:main Jun 1, 2023
@dblock dblock added the backport 2.x Backport to 2.x branch label Jun 1, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 1, 2023
* MAINT: extract reason from error

Signed-off-by: George Chen <[email protected]>

* MAINT: add change log

Signed-off-by: George Chen <[email protected]>

* MAINT: remove unwanted string

Signed-off-by: George Chen <[email protected]>

* TST: AwsSdk2SecurityIT

Signed-off-by: George Chen <[email protected]>

* MAINT: header

Signed-off-by: George Chen <[email protected]>

---------

Signed-off-by: George Chen <[email protected]>
(cherry picked from commit 89d154d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request Jun 1, 2023
* MAINT: extract reason from error



* MAINT: add change log



* MAINT: remove unwanted string



* TST: AwsSdk2SecurityIT



* MAINT: header



---------


(cherry picked from commit 89d154d)

Signed-off-by: George Chen <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@chenqi0805 chenqi0805 deleted the fix/cause-reason-for-fgac-domain branch June 1, 2023 14:30
@BrendonFaleiro BrendonFaleiro mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unhelpful error message when failing to authenticate with Amazon OpenSearch Service
5 participants