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:

• [X] Code compiles correctly
• [X] Created/updated tests
• [X] 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 https://github.com/dapr/java-sdk/pull/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

[willtsai]

@mukundansundar @artursouza -- this is ready for review now, can you please take a look and if ok can remove the do-not-merge label?

[mukundansundar]

@willtsai The integration tests are failing due to compilation error ...

[willtsai]

@willtsai The integration tests are failing due to compilation error ...

@mukundansundar - weird, I was able to compile and pass tests locally...looks like it's one specific failure package Properties does not exist - I'll investigate it. Probably a dependency issue due to missing import statement in the integration test class.

[willtsai]

@willtsai The integration tests are failing due to compilation error ...

@mukundansundar - weird, I was able to compile and pass tests locally...looks like it's one specific failure package Properties does not exist - I'll investigate it. Probably a dependency issue due to missing import statement in the integration test class.

@mukundansundar - looks like it's passed integration tests and compiling successfully now. Also, thanks for restarting the validator jobs, looks like the second runs succeeded. I think this PR is ready for review now, you can remove the do-not-merge label if appropriate.

[codecov[bot]]

Codecov Report

Merging #649 (59dca39) into master (cff90c0) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #649      +/-   ##
============================================
+ Coverage     76.34%   76.35%   +0.01%     
- Complexity     1117     1118       +1     
============================================
  Files           101      101              
  Lines          3500     3502       +2     
  Branches        407      407              
============================================
+ Hits           2672     2674       +2     
  Misses          630      630              
  Partials        198      198              

Impacted Files	Coverage Δ
sdk/src/main/java/io/dapr/config/Properties.java	82.60% <100.00%> (+0.79%)	:arrow_up:
sdk/src/main/java/io/dapr/utils/NetworkUtils.java	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cff90c0...59dca39. Read the comment docs.

[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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[willtsai]

woo! thanks @cicoyle @artursouza 🥳