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

ACMPCA CertificateIssuedWaiter default "retryable" behavior possibly incorrect #1585

Closed
3 tasks done
azdagron opened this issue Feb 9, 2022 · 5 comments
Closed
3 tasks done
Labels
bug This issue is a bug. m Effort estimation: medium p3 This is a minor priority issue

Comments

@azdagron
Copy link

azdagron commented Feb 9, 2022

Documentation

Describe the bug

When the CertificateIssuedWaiter sees an error come back from GetCertificate, it queries a callback set on the options to determine if it should retry based on the error seen.

The default implementation of this option callback is the certificateIssuedStateRetryable function.

This function has the following behavior:

  • If no error, function returns false, nil which results in the WaitOutput function returning the certificate ✅
  • if there is an error, and it is a RequestInProgressException error, the function returns true, nil, which results in the WaitOutput function retrying the operation ✅
  • if there is an error and it is not a RequestInProgressException error, the function still returns true, nil, which results in the WaitOutput function retrying the operation ❌

This behavior seems to differ from the Java SDK, which will only retry on RequestInProgressException, and otherwise rethrow other exceptions.

Seems like the function should return the error if it isn't a RequestInProgressException so that WaitOutput will bubble the error up.

Expected behavior

Waiter should only retry on RequestInProgressException and not for every error.

Current behavior

Waiter retries on any error.

Steps to Reproduce

N/A

Possible Solution

No response

AWS Go SDK version used

github.com/aws/aws-sdk-go-v2/service/acmpca v1.14.0

Compiler and Version used

go version go1.17.6 darwin/amd64

Operating System and version

N/A

@azdagron azdagron added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 9, 2022
@vudh1 vudh1 self-assigned this Mar 9, 2022
@vudh1
Copy link
Contributor

vudh1 commented Mar 25, 2022

Hi @azdagron thanks for reaching out. Can you provide more information on how we can reproduce this behavior?

@vudh1 vudh1 added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Mar 25, 2022
@github-actions
Copy link

This issue has not received a response in 1 week. If you want to keep this issue open, please just leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Mar 28, 2022
@azdagron
Copy link
Author

Hi @vudh1. I didn't discover this by having a problem firsthand. Rather, I was looking through the SDK code when we migrated to v2 to make double check some usage of ours and noticed this code and thought it looked wrong. I looked up the counterpart in the Java library to see what happened there and noticed the behavior difference.

I don't know if it is a bug, but does seem to represent an inconsistency in behavior between the Go and Java SDKs.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Mar 29, 2022
@vudh1 vudh1 removed their assignment Aug 25, 2022
@RanVaknin RanVaknin added p3 This is a minor priority issue m Effort estimation: medium labels Nov 10, 2022
@RanVaknin
Copy link
Contributor

Hi there,

The waiter definition is what controls which error is retried on, and which error is not. Since the waiter is generated based on that model definition, we have decided that at this point since this is not blocking you, its a 2 year old issue we are going to close this.

If new customers are seeing this issue and still think this should be fixed, please open a new separate issue.

Thanks,
Ran~

@RanVaknin RanVaknin closed this as not planned Won't fix, can't repro, duplicate, stale May 2, 2024
Copy link

github-actions bot commented May 2, 2024

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. m Effort estimation: medium p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

3 participants