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

Adjust forward auth to avoid connection leak #10096

Merged
merged 1 commit into from Sep 9, 2023

Conversation

wdhongtw
Copy link
Contributor

@wdhongtw wdhongtw commented Aug 28, 2023

What does this PR do?

This MR provides a minor bug fix for forward auth middleware implementation.

During the sub-request of forward auth process, If error happens on io.ReadAll(forwardResponse.Body), current implement won't close the body, the underline http connection can not be reuse for next request, and results in connection leak in HTTP 1.1 environment.

The only change in this MR is to register the defer statement earlier, covering all possible execution path.

Although it’s unlikely for a working connection to return error on body read, it’s still a good practice to defer body.Close() immediately after client.Do if it does not return error.

See also:

Motivation

It's not a serious bug, just nitpicking some implementation details. :D

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

I grep the code base with client.Do usage, another issue I found is in integration/try/try.go.
Since that package is only used in test, it’s should be ok to keep it untouched. :D

@rtribotte
Copy link
Member

Hello @wdhongtw,

Thanks for this contribution!

Can you please rebase the PR on the v2.10 branch?
While we think it is worth doing this change as a better pattern, we are unsure that the problem could really happen.
Nonetheless, this is more of a bug fix and hence it should be merged on the current v2.10.

Should always close response body on successful client.Do

Signed-off-by: Weida Hong <wdhongtw@gmail.com>
@wdhongtw
Copy link
Contributor Author

wdhongtw commented Sep 6, 2023

@rtribotte No problem :D

@wdhongtw wdhongtw changed the base branch from master to v2.10 September 6, 2023 14:01
@ldez ldez added this to the 2.10 milestone Sep 6, 2023
@rtribotte rtribotte closed this Sep 7, 2023
@rtribotte rtribotte reopened this Sep 7, 2023
Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM thanks 👍

Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks!

@traefiker traefiker merged commit 3216c8a into traefik:v2.10 Sep 9, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants