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

[WIP] AWS SDK Cross Version Compatability: env-var http_proxy & https_proxy support #4165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mythra
Copy link

@Mythra Mythra commented Jul 6, 2023

Motivation and Context

note: I'm opening this early because as far as I can tell it requires changing the public API, but I'm opening to see if others have ideas, or how we can handle this.

fixes #2958

The goal of this is to allow like many other SDKs (including aws-sdk-java v1) support for specifying proxies through environment variables. Using http_proxy & https_proxy for environment variables. http_proxy will be used for connections to HTTP sites, and https_proxy for HTTPS sites.

While making this change, I also noticed a bug in how we were selecting the http.proxyHost & https.proxyHost System Properties. I've gotten them to follow: https://docs.oracle.com/javase/8/docs/technotes/guides/net/proxies.html correctly now. Before we weren't passing the scheme in and only selecting based on explicit configuration (which you can still do, and doesn't break)!

I've changed all the HTTP clients that are in the source tree.

Modifications

  • Every single builder now has support for allowing reading from environment variables (by default this is true like system properties), and is not overriding of anything that exists (not breaking, and also like other SDKs).
    • There is now a way to set whether you want an explicitly configured process to handle HTTP or not HTTPS. By default explicit overrides everything like always. This allows it to play with system properties and environment variables too.
  • Every proxy configuration type now takes in the scheme it's connecting too, to change between http & https proxies without manual configuration, or multiple clients at all.

Testing

  • I've been running tests locally to confirm unit tests continue to pass.
  • When I take this out of WIP I will run integration tests through a proxy to confirm it doesn't break anything.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This is also a breaking change -- i cannot see a way to avoid it

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@millems millems marked this pull request as ready for review August 7, 2023 17:31
@millems millems requested a review from a team as a code owner August 7, 2023 17:31
return Optional.of(trimmed);
}

public static Optional<String> parsePasswordFromUrl(URL value) {
Copy link

Choose a reason for hiding this comment

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

Should this handle passwords that are URLEncoded with special characters? Maybe after grabbing it out of the parts array, pass it through urlDecode?

Copy link
Author

Choose a reason for hiding this comment

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

Hey, great question. To be honest I'm not sure, I didn't test with any that required it. I'll write some tests and see how the client libraries we use interact with it, and add a boolean parameter if needed!

@debora-ito debora-ito added the needs-review This issue or PR needs review from the team. label Aug 15, 2023
@debora-ito debora-ito assigned debora-ito and joviegas and unassigned debora-ito Aug 18, 2023
@debora-ito debora-ito removed the needs-review This issue or PR needs review from the team. label Aug 24, 2023
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.

Use environment variables to configure HTTP proxies
4 participants