Fix bug in using nonProxyHosts with CRT http/s3 clients

[alextwoods]

Fix bug in using nonProxyHosts with CRT http/s3 clients.

Fixes #6414

Motivation and Context

#4974 added the nonProxyHosts configuration to the CrtProxyConfiguration used 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 noProxyHosts CRT 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

• Update to latest aws-crt dependency (which includes the noProxyHosts binding)
• Set the noProxyHosts on CRT's HttpProxyOptions, 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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

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

[sonarqubecloud]

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

See analysis details on SonarQube Cloud

[L-Applin]

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?

[alextwoods]

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.

[L-Applin]

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.

[github-actions]

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.