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

pull retry loop forward to cover everything from resolving auth scheme onward #2966

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

lucix-aws
Copy link
Contributor

@lucix-aws lucix-aws commented Jan 16, 2025

Closes #2965

  • Moves the retry loop forward to cover credential retrieval, which is the intended behavior per Smithy client reference architecture and is how we behaved before our auth refactor in feat: sra identity&auth refactor #2364.
  • Also adjusts the middleware insert logic for checksums to ensure that the header and trailer checksums (which are separate middlewares) happen after one another. The latter requires the former to determine whether to operate through context signals. In the future we should just combine these, see defragment HTTP checksum middlewares #2968.
  • Fixes tests in service/s3 and service/route53 to correctly inspect the final request. These were technically retrieving the request at the wrong point in time before, it just happened to work because the retry loop hadn't kicked in and cloned the request yet (which it now does, making the captured request pointers stale).

This change is verified by our integration tests.

@lucix-aws lucix-aws requested a review from a team as a code owner January 16, 2025 19:16
Copy link
Contributor

@Madrigal Madrigal left a comment

Choose a reason for hiding this comment

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

LGMT as soon as integ tests pass

@lucix-aws lucix-aws force-pushed the fix-retrygetidentity branch from 0e53ad6 to 46147c8 Compare January 16, 2025 20:43
@lucix-aws
Copy link
Contributor Author

@Madrigal You'll want to re-inspect the 1st commit

@lucix-aws
Copy link
Contributor Author

lucix-aws commented Jan 16, 2025

Integ tests failed again but it looked transient that time - service/wafv2 operation timeouts - retried

@@ -53,29 +53,6 @@ type InputMiddlewareOptions struct {
// AddInputMiddleware adds the middleware for performing checksum computing
// of request payloads, and checksum validation of response payloads.
func AddInputMiddleware(stack *middleware.Stack, options InputMiddlewareOptions) (err error) {
// TODO ensure this works correctly with presigned URLs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(it does)

@@ -53,29 +53,6 @@ type InputMiddlewareOptions struct {
// AddInputMiddleware adds the middleware for performing checksum computing
// of request payloads, and checksum validation of response payloads.
func AddInputMiddleware(stack *middleware.Stack, options InputMiddlewareOptions) (err error) {
// TODO ensure this works correctly with presigned URLs

// Middleware stack:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is so beyond out-of-date at this point I've just removed it

@lucix-aws lucix-aws force-pushed the fix-retrygetidentity branch from 42d3a05 to a0fcc5d Compare January 17, 2025 02:47
}
rm.request = req
return next.HandleSerialize(ctx, in)
func (c *captureRequest) Do(r *http.Request) (*http.Response, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was just something cool to do, right? Just want to make sure I'm not missing anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that was for

Fixes tests in service/s3 and service/route53 to correctly inspect the final request. These were technically retrieving the request at the wrong point in time before, it just happened to work because the retry loop hadn't kicked in and cloned the request yet (which it now does, making the captured request pointers stale).

return err
}

stack.Build.Remove("ContentChecksum")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we had this in place since before you made this change, but is it because ContentChecksum is the older way of doing checksums?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, ContentChecksum is the old MD5, which the recent changes to S3 object integrity effectively removes.

@lucix-aws lucix-aws merged commit 74b33ba into main Jan 17, 2025
13 checks passed
@lucix-aws lucix-aws deleted the fix-retrygetidentity branch January 17, 2025 15:19
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.

Retry loop does not refresh credentials after recent SRA identity&auth refactor
3 participants