Change "127.0.0.1" to SIDECAR_IP to dynamically account for IPv4 vs. IPv6 environments

[willtsai]

Description

I've made some changes to move away from hardcoded "127.0.0.1" IP addresses (representing localhost) to make the SDK and Tests compatible with exclusively IPv6 environments:

• Replaced hardcoded "127.0.0.1" in various URL strings with the SIDECAR_IP property constant
• Changed the DEFAULT_SIDECAR_IP property constant's value from the hardcoded "127.0.0.1" to instead use the Java API getLoopbackAddress(). This would make the IP default value more dynamic and account for situations where users operate in an exclusively ipv6 environment and the logic ends up falling back to the default value.
• Added a test util to detect IPv6 addresses and format them accordingly for HTTP URLs using the square brackets representation in unit tests

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #218

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation (N/A)

[mukundansundar]

@artursouza Wasn't there was a particular reason why 127.0.0.1 was used instead of localhost?

[willtsai]

@artursouza Wasn't there was a particular reason why 127.0.0.1 was used instead of localhost?

@mukundansundar according to #213, it was changed from localhost to 127.0.0.1 "to avoid local OS specific DNS lookups that might hinder performance". My changes here don't reintroduce the use of localhost, but rather uses the dynamically sourced SIDECAR_IP constant from the system.

[willtsai]

Work in progress, please do not merge

[artursouza]

I will give another pass tomorrow. Thanks.

[artursouza]

Just a minor change.

[artursouza]

Can this method automatically return the Ipv6 between "[]"?

[willtsai]

@artursouza Sorry for the delay, finally had some time to look into this. I changed some unit tests locally to validate IPv6 sidecar IP constants contained within "[]" and it seems like your suggestion is viable - for both URL endpoint invocation formats and Dapr client instantiation. Great recommendation!

However, before I proceed, I need to validate an implementation detail with you. To test for whether an address is IPv6, it would be convenient to leverage the Apache Commons Validator, though you have recommended against bringing in 3P libraries beyond the test scope. Is it possible to get an exception for this case, or do I have to write a custom IPv6 validator within NetworkUtils?

[mukundansundar]

@willtsai Is it complex to write a validator for IPv6 ? both standard pattern and hex compressed pattern along with IPv4 validation?
I am wary of adding a new dependency specifically for one validation ....

[willtsai]

Understood - should not be too complex but I wanted to see if the work (and subsequent unit tests) could be avoided. Seems like adding a new dependency is not worth it here, so I'll work on the custom validator.

[willtsai]

I started the implementation for this and have a working validator, but then realized that this would introduce inconsistencies into the Property<String> SIDECAR_IP config object since the result would be that the special IPv6 formatting would only get applied when DEFAULT_SIDECAR_IP is used in fallback scenarios, so the IPv6 address would not get "[]" applied to them when they are sourced from the system property name or env variable.

Thus, I think there are two options:

1. Leave the implementation of adding "[]" to the http address to the client and thus just modifying our test clients accordingly, i.e. code in this PR remain as is.
2. Make modifications to the Property and/or Properties objects to introduce a new object class IpProperty.java or equivalent that would apply the required IPv6 formatting to the IP address it returns.

@artursouza @mukundansundar - what are your thoughts here? which option would you prefer? (I personally think option 1 introduces less surprises to the client who may not be expecting IPv6 address to be pre-formatted for http URLs)

[cicoyle]

@willtsai mind resolving the conflicts?

[willtsai]

@willtsai mind resolving the conflicts?

@cicoyle done, though now i need to investigate the test failures...

[willtsai]

@cicoyle i've addressed all merge conflicts and test failures, can you please take another look? (i think the build tests just need to be re-run)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (14cc3f8) 77.60% compared to head (d9fd2c0) 77.61%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #649   +/-   ##
=========================================
  Coverage     77.60%   77.61%           
- Complexity     1570     1571    +1     
=========================================
  Files           144      144           
  Lines          4765     4767    +2     
  Branches        554      554           
=========================================
+ Hits           3698     3700    +2     
  Misses          781      781           
  Partials        286      286           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cicoyle]

lgtm - nice to see this coming to a closing 🎉

[willtsai]

woo! thanks @cicoyle @artursouza 🥳