AWS SDK Compatibility: Check for http_proxy & https_proxy environment variables
#2991
+231
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this brings us in line with many other AWS SDKs which check for these environment variables to modify client behavior. it does not change behavior if you've already been setting your web proxy explicitly, or in properties (the same as the java (and other) sdks). we do introduce this with two new methods in ClientConfig to not break any existing callers using the methods that are public. while all of our callers have been updated to properly check web proxy, and then if that's unset check the protocol being used and optionally call `GetHttpProxy`/`GetHttpsProxy` as needed. If we're okay with breaking ever IT MAY be worthwhile to break `GetWebProxy` by taking in the protocol, and then just having the one method to prevent any accidental mis-use, and to ensure everyone is using those environment variables. it's also important to note: <dotnet/runtime#31113> meaning a user will just get an error if they try to proxy to something that is using `HTTPS`. i figured just letting it error on construction is probably the 'safest' option. letting the user know that their setting isn't being respected, as opposed to just silently 'ignoring' it. this is also a decision we might want to change.
|
Thanks a lot for the PR, we're discussing internally what should be done for the other SDKs that don't support these environment variables (aws/aws-sdk#538 (comment)). |
ashishdhingra
added
feature-request
dscpinheiro
requested review from
normj and
dscpinheiro
and removed request for
peterrsongg
dscpinheiro
reviewed
Nov 28, 2023
sdk/src/Core/Amazon.Runtime/Pipeline/HttpHandler/_bcl/HttpWebRequestFactory.cs
Show resolved
Hide resolved
dscpinheiro
approved these changes
Nov 30, 2023
boblodgett
reviewed
Dec 1, 2023
normj
requested changes
Dec 5, 2023
sdk/src/Core/Amazon.Runtime/Pipeline/HttpHandler/_bcl/HttpWebRequestFactory.cs
Show resolved
Hide resolved
Ensure we read explicitly configured proxies through code first, and then fallback to reading the proxy from environment variables. next add some tests with the recommended environment variable mocking test setup provided.
normj
requested changes
Dec 8, 2023
|
@Mythra Could you please have a look at review comments and help address these in order to proceed further. |
|
Hey @ashishdhingra yep, I saw the comments and will get to it, but it's also the holidays and I'm otherwise preoccupied at the moment. I'll get to it as soon as I can as it is really important I've been watching it like a hawk. Thanks! |
|
I've come back from holidays, and made the changes! |
normj
approved these changes
Jan 3, 2024
boblodgett
approved these changes
Jan 3, 2024
|
Thanks again for the contribution, we really appreciate it! This will be included in the next SDK release (which should happen tomorrow). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
this brings us in line with many other AWS SDKs which check for these environment variables to modify client behavior. it does not change behavior if you've already been setting your web proxy explicitly, or in properties (the same as the java (and other) sdks).
we do introduce this with two new methods in ClientConfig to not break any existing callers using the methods that are public. while all of our callers have been updated to properly check web proxy, and then if that's unset check the protocol being used and optionally call
GetHttpProxy/GetHttpsProxyas needed. If we're okay with breaking ever IT MAY be worthwhile to breakGetWebProxyby taking in the protocol, and then just having the one method to prevent any accidental mis-use, and to ensure everyone is using those environment variables.it's also important to note:
dotnet/runtime#31113 meaning a user will just get an error if they try to proxy to something that is using
HTTPS. i figured just letting it error on construction is probably the 'safest' option. letting the user know that their setting isn't being respected, as opposed to just silently 'ignoring' it. this is also a decision we might want to change.Motivation and Context
fixes #1977
Testing
Types of changes
Checklist
License