-
Couldn't load subscription status.
- Fork 947
Fix bug in using nonProxyHosts with CRT http/s3 clients #6511
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
Conversation
Quality Gate Passed Quality Gate passed
Issues
2 New issues
0 Accepted issues
Measures
0 Security Hotspots
83.3% Coverage on New Code
0.0% Duplication on New Code
core/crt-core/src/main/java/software/amazon/awssdk/crtcore/CrtConfigurationUtils.java
Show resolved
Hide resolved
core/crt-core/src/main/java/software/amazon/awssdk/crtcore/CrtConfigurationUtils.java
Show resolved
Hide resolved
My only concern is that if users were setting non-proxy hosts before and it had no effect, this PR would enable non-proxy-host and could bypass the proxy for some requests that weren't bypassing it previously, correct?
My only concern is that if users were setting non-proxy hosts before and it had no effect, this PR would enable non-proxy-host and could bypass the proxy for some requests that weren't bypassing it previously, correct?
Yeah - this is true and I think a fair concern, but I think I'd consider it a reasonable break fix - if a user has explicitly configured a non-proxy-host we should use it. Existing system/environment uses of nonProxyHost should remain unchanged, so its only explicit in code configurations that should now start working.
We could mitigate that risk by introducing a new configuration (nonProxyHost_butThisTimeIMeanIt) and deprecating the old one, but I think I'd argue that this is a reasonable fix of existing broken behavior.
My only concern is that if users were setting non-proxy hosts before and it had no effect, this PR would enable non-proxy-host and could bypass the proxy for some requests that weren't bypassing it previously, correct?
Yeah - this is true and I think a fair concern, but I think I'd consider it a reasonable break fix - if a user has explicitly configured a non-proxy-host we should use it. Existing system/environment uses of nonProxyHost should remain unchanged, so its only explicit in code configurations that should now start working.
We could mitigate that risk by introducing a new configuration (
nonProxyHost_butThisTimeIMeanIt) and deprecating the old one, but I think I'd argue that this is a reasonable fix of existing broken behavior.
Yeah, I agree that if a user has explicitly configured a non-proxy-host we should use it. I'm just thinking of the situation where user would have set a non-proxy-host for a dev/local setup environment, but then inadvertently leaked that same config to their prod environment (unknowingly because it didn't take effect previously). I don't know how much of a concern that would be, it does seems unlikely.
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.
Uh oh!
There was an error while loading. Please reload this page.
Fix bug in using nonProxyHosts with CRT http/s3 clients.
Fixes #6414
Motivation and Context
#4974 added the
nonProxyHostsconfiguration to theCrtProxyConfigurationused by the CRT HTTP Client and S3 Client. However, the implementation had a bug and was checking the nonProxyHosts against the proxy host instead of against the request's endpoint.Checking against the request's endpoint is tricky and for the S3 Client, not possible, since the endpoint is resolved per request. Rather than implementing the no proxy hosts logic in Java, this PR uses the
noProxyHostsCRT configuration.There is a difference in the format/support for no proxy hosts configuration between CRT and pure Java (apache/netty clients): the Java implementation allows arbitrary regex but does not support CIDR style ip addresses. CRT's implementation follows the format used by curl and the only wild card supported is a single "*". It also supports CIDR style ip addresses.
These differences exist today in the use of no proxy environment/system properties.
Despite this behavior difference, this PR is NOT a breaking change since the nonProxyHosts configuration was not actually being applied before.
Modifications
noProxyHostson CRT'sHttpProxyOptions, allowing it to be used by CRT.Testing
Modified existing tests + manual testing of crt http client and s3 client.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License