-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Broadcast public ipv4 addresses on override #1275
Conversation
* Indicates if the public ipv4 address of the instance should be advertised. | ||
* @return true if the public ipv4 address of the instance should be advertised, false otherwise . | ||
*/ | ||
boolean shouldBroadcastPublicIpv4Addr(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to go in a public api? Looks like it could be in PropertiesInstanceConfig and used by CloudInstanceConfig without being in the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was planning to throw in a default implementation as I realized it breaks the interface, but I could keep it to PropertiesInstanceConfig
which was my initial plan. LMK what you prefer.
Would still want to keep a sane default in AbstractInstanceConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spencergibb PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does ptal mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please Take A(nother) Look
@@ -128,8 +128,17 @@ public String getHostName(boolean refresh) { | |||
|
|||
@Override | |||
public String getIpAddress() { | |||
String ipAddr = amazonInfoHolder.get().get(MetaDataKey.localIpv4); | |||
return ipAddr == null ? super.getIpAddress() : ipAddr; | |||
String publicIpv4Addr = amazonInfoHolder.get().get(MetaDataKey.publicIpv4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code.
* @return true if the public ipv4 address of the instance should be advertised, false otherwise . | ||
*/ | ||
public boolean shouldBroadcastPublicIpv4Addr() { | ||
return configInstance.getBooleanProperty(namespace + ADVERTISE_PUBLIC_IPV4_ADDR, super.shouldBroadcastPublicIpv4Addr()).get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_KEY to stay consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some nits.
This is a quick and dirty PR to get Eureka to return public ipv4 addresses when overriden by a new config flag
broadcastPublicIpv4
.However, it doesn't fundamentally alter the behavior of
resolveDefaultAddress
as part of this change./cc @drobertduke