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

Add experimental SOCKS5 support for S3 #524

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ivanyu
Copy link
Contributor

@ivanyu ivanyu commented Mar 28, 2024

Technically, I found a way to have proxy-side host name resolution, but it requires rewriting quite a bit of the AWS client machinery. Leaving it as is for now.

@ivanyu ivanyu force-pushed the ivanyu/socks5-aws branch 4 times, most recently from 352d559 to 87fdbde Compare March 28, 2024 16:26
Technically, I found a way to have proxy-side host name resolution, but it requires rewriting quite a bit of the AWS client machinery. Leaving it as is for now.
@ivanyu ivanyu marked this pull request as ready for review March 28, 2024 17:08
@ivanyu ivanyu requested a review from a team as a code owner March 28, 2024 17:08
@jeqo jeqo self-assigned this Apr 2, 2024
jeqo
jeqo previously approved these changes Apr 3, 2024
Copy link
Contributor

@jeqo jeqo left a comment

Choose a reason for hiding this comment

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

Thanks @ivanyu. LGTM overall.

nit: I wonder if there's any consideration/reminder we should be comment about the usage of reflection. As this is an experimental feature it may be ok as is though.

@ivanyu
Copy link
Contributor Author

ivanyu commented Apr 3, 2024

@jeqo I isolated all the reflection operations in a separate package + added a comment on this. Even when this becomes non-experimental, most likely the reflection magic stays: the AWS SDK isn't just configurable enough to do what we want as we did with the other clouds.

Isolate the proxy installation and related reflection operations
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.

None yet

2 participants