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

Bugfix to support comma separated values in no_proxy environment settings #4735

Merged
merged 5 commits into from
Dec 5, 2023

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Dec 2, 2023

Motivation and Context

4703

Root Cause

  • The logic to parse no_proxy environment variables for multiple host used logic same as that of System property NO_PROXY setting which treated "|" as separator

  • Why was this missed in testing ?

    • ProxyConfigurationTest had added a test case to test multiple hosts scenario. Unfortunately the test had a typo error where hosts names were added in double quotes instead of separate string.

Modifications

  • Added logic to split string based on "," instead of "|".
  • Added logic to trim extra spaces
  • We are not adding greedy to non-greedy since it can cause issues if user already sends a non-greedy string. Wild cards are passed as it is.

New revision

After offline Discussion with @zoewangg updated this to be consistent as V1-here and V1-Here supporting both commas and pipes.

Testing

  • Junits added

Screenshots (if appropriate)

Types of changes

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

License

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

@joviegas joviegas requested a review from a team as a code owner December 2, 2023 01:44
@joviegas joviegas changed the title Bugfix to support comma separated values in no_http environment settings Bugfix to support comma separated values in no_proxy environment settings Dec 2, 2023
@joviegas joviegas force-pushed the joviegas/non_proxy_host_issue branch from 5ef809a to 01af6b2 Compare December 2, 2023 01:46
@joviegas joviegas force-pushed the joviegas/non_proxy_host_issue branch from 01af6b2 to 0dec65d Compare December 2, 2023 02:03
@joviegas joviegas enabled auto-merge (squash) December 4, 2023 21:57
Copy link
Contributor

@zoewangg zoewangg left a comment

Choose a reason for hiding this comment

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

Can we update Javadocs for ProxyConfiguration.Builder#useEnvironmentVariableValues to mention this behavior? We have proxy config class for every http client, hopefully it's not too much copy paste

.changes/next-release/bugfix-AWSSDKforJavav2-02e443d.json Outdated Show resolved Hide resolved
@joviegas joviegas enabled auto-merge (squash) December 5, 2023 02:29
@joviegas joviegas merged commit 668bbe2 into master Dec 5, 2023
13 checks passed
Copy link

sonarcloud bot commented Dec 5, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 15 Code Smells

78.1% 78.1% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@joviegas joviegas deleted the joviegas/non_proxy_host_issue branch May 15, 2024 20:31
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.

3 participants