Create `sidecar.hostname` and `sidecar.ipAddress` #1561

Merged
merged 4 commits into from Jan 9, 2017

Projects

None yet

5 participants

@kakawait
Contributor

In addition to eureka.instance.hostname property you can now defined sidecar.hostname in order to set different hostname from application and its sidecar.
However if eureka.instance.hostname is present it will override sidecar.hostname (due to @ConfigurationProperties("eureka.instance"))

Moreover we also now re-calculate ip address from sidecar.hostname (except if sidecar.ipAddress is defined) to correctly have ip address on eureka registry.

Fixes #981


PS: About tests I can't really test ip recalculation because I'm not able to mock InetAddress.getByName() without PowerMock or something else.

@pivotal-issuemaster

@kakawait Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@kakawait kakawait Create `sidecar.hostname` and `sidecar.ipAddress`
In addition to `eureka.instance.hostname` property you can now defined `sidecar.hostname` in order to set different hostname from application and its sidecar.
However if `eureka.instance.hostname` is present it will override `sidecar.hostname`.

Moreover we also now re-calculate ip address from `sidecar.hostname` (except if `sidecar.ipAddress` is defined) to correctly have ip address on eureka registry.

Fixes #981
ebc29cc
@codecov-io
codecov-io commented Dec 19, 2016 edited

Current coverage is 74.19% (diff: 63.63%)

Merging #1561 into master will decrease coverage by 0.03%

@@             master      #1561   diff @@
==========================================
  Files           193        193          
  Lines          5937       5945     +8   
  Methods           0          0          
  Messages          0          0          
  Branches        893        895     +2   
==========================================
+ Hits           4407       4411     +4   
- Misses         1206       1208     +2   
- Partials        324        326     +2   

Powered by Codecov. Last update c380039...9096e53

@@ -80,13 +97,29 @@ public EurekaInstanceConfigBean eurekaInstanceConfigBean() {
int port = this.sidecarProperties.getPort();
config.setNonSecurePort(port);
config.setInstanceId(getDefaultInstanceId(this.env));
- if(StringUtils.hasText(springAppName)) {
+ if (StringUtils.hasText(springAppName)) {
@ryanjbaxter
ryanjbaxter Dec 19, 2016 Contributor

rougue space before if, same with the ones below

@kakawait
kakawait Dec 20, 2016 Contributor

What do you mean? Sorry I can't understand since https://github.com/spring-cloud/spring-cloud-netflix/blob/master/eclipse/eclipse-code-formatter.xml describe that if must be followed by space before brace

@ryanjbaxter
ryanjbaxter Dec 20, 2016 Contributor

You are right, sorry my mistake

+ if (StringUtils.hasText(hostname)) {
+ if (!StringUtils.hasText(ipAddress)) {
+ try {
+ ipAddress = inetUtils.convertAddress(InetAddress.getByName(hostname)).getIpAddress();
@ryanjbaxter
ryanjbaxter Dec 19, 2016 Contributor

This is going to make a network request correct? Have you thought about the performance impacts of this? I guess there isn't a better way of doing this if you really need the information. Maybe we want to consider only doing this if the user configures the sidecar app to do so?

@spencergibb
spencergibb Dec 19, 2016 Member

We already have code that attempts to detect the hostname and ipaddress in spring cloud commons. Please remove this and stick to just setting the value if it is available.

@kakawait
kakawait Dec 20, 2016 edited Contributor

I did that because if you are registering sidecar using previous trick eureka.instance.hostname (in order to target application on different hostname) we get following result on Eureka server http://localhost:8761/eureka/apps

Using following configuration (tested on Camden.SR3)

spring:
  application:
    name: sidecar

server:
  port: 5678

sidecar:
  port: 8080

eureka:
  instance:
    hostname: google.com

returns

<applications>
    <versions__delta>1</versions__delta>
    <apps__hashcode>UP_1_</apps__hashcode>
    <application>
        <name>SIDECAR</name>
        <instance>
            <instanceId>172.16.123.1:sidecar:5678</instanceId>
            <hostName>google.com</hostName>
            <app>SIDECAR</app>
            <ipAddr>172.16.123.1</ipAddr>
            <status>UP</status>
            ...
        </instance>
    </application>
</applications>

As you can see hostName is correctly set but ip address does not correspond to the hostname.

Thus if you are using eureka.instance.preferIpAddress or other way can fail.

So if I removed the hostname resolution and client do not provide ip address via sidecar.ipAddress (example: Docker you can determine hostname but generally not the ip address that will be generated when container will be run) you assume that Eureka information will be partially wrong?

@kakawait
kakawait Dec 20, 2016 Contributor

@ryanjbaxter Yes performance will be impacted (and can fail if hostname can't be resolve when sidecar will be starting) but as I said before I favored consistency rather than performance. But let's discuss together!

Since I'm not mastering Eureka yet I may miss-understand something about its concepts, or even my sidecar vision may not be the same.

@ryanjbaxter
ryanjbaxter Dec 20, 2016 Contributor

example: Docker you can determine hostname but generally not the ip address that will be generated when container will be run

@kakawait I could be wrong, but I thought that Docker sets the IP address of the container in an environment variable. I am pretty sure that was the case when using Docker compose.

Is there another situation besides Docker where this is an issue?

@kakawait
kakawait Dec 20, 2016 Contributor

@ryanjbaxter What can I propose: remove hostname resolution but update documentation that inform user sidecar.ipAddress should be fill in addition to sidecar.hostname to prevent any futur error when trying to use ipAddress from Eureka meta informations? Like eureka.instance.preferIpAddress

@kakawait
kakawait Dec 20, 2016 Contributor

About docker by default (with basic run param) there is no exposed variable that contain ipAddress

$ docker run -it --rm alpine env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=60db1f4ab975
TERM=xterm
HOME=/root
@spencergibb
spencergibb Dec 20, 2016 Member

We already have code to do this here. It is already used in the constructor of EurekaInstanceConfigBean. Please remove this code.

@kakawait
kakawait Dec 20, 2016 Contributor

Yes but I will apply on server that instanciate the bean so the sidecar itself and not the application referenced by the sidecar. I'm OK to remove it but I think is important to inform user that it should specify IP address himself else sidecar IP will be referenced and not application

@spencergibb
spencergibb Dec 20, 2016 edited Member

Sidecar was built to run as a process next to the application itself. They should share the same IP address.

@kakawait
kakawait Dec 20, 2016 Contributor

Thus this PR is no sense, since they should share same IP address so technically same hostname

+ config.setHostname(hostname);
+ }
+ if (!StringUtils.hasText(ipAddress)) {
+ config.setIpAddress(ipAddress);
@ryanjbaxter
ryanjbaxter Dec 19, 2016 Contributor

why set the IP address if there is nothing to set?

@kakawait
kakawait Dec 20, 2016 Contributor

Lol I may fail something 👍 I will check that

+ if (StringUtils.hasText(hostname)) {
+ if (!StringUtils.hasText(ipAddress)) {
+ try {
+ ipAddress = inetUtils.convertAddress(InetAddress.getByName(hostname)).getIpAddress();
@spencergibb
spencergibb Dec 19, 2016 Member

We already have code that attempts to detect the hostname and ipaddress in spring cloud commons. Please remove this and stick to just setting the value if it is available.

- EurekaInstanceConfigBean config;
+ @RunWith(SpringJUnit4ClassRunner.class)
+ @SpringBootTest(classes = SidecarApplication.class, webEnvironment = WebEnvironment.RANDOM_PORT, value = {
+ "spring.application.name=mytest", "spring.cloud.client.hostname=mhhost",
@spencergibb
spencergibb Dec 19, 2016 Member

spring.cloud.client.hostname doesn't map to any config. My guess is these tests work find without it.

@kakawait
kakawait Dec 20, 2016 Contributor

Yes just copied from original test. Didn't ask myself the usage of that property. I will remove it

@kakawait
kakawait Dec 20, 2016 Contributor

After removed it used for instanceId generation if eureka.instance.hostname is not present so NewPropertyEurekaTestConfigBeanTest

Expected: "mhhost:mytest:1"
     but: was "127.0.0.1:mytest:1"
Expected :mhhost:mytest:1
Actual   :127.0.0.1:mytest:1

Thus I can't change assertion to match regex (because ip can be different depending on environment, in my case I have an alias for lo0) or simply rollback?

kakawait added some commits Dec 20, 2016
@kakawait kakawait Fix incorrect condition for ipAddress assignment
Previously

```java
if (!StringUtils.hasText(ipAddress)) {
    config.setIpAddress(ipAddress);
}
```

was no sense, condition was negate...

Just fix that stupid error
3a6579b
@kakawait kakawait Remove useless property on tests
`spring.cloud.client.hostname` do nothing, remove it
f1c05e1
@kakawait kakawait Remove IpAddress resolution code
9096e53
@spencergibb spencergibb merged commit fccbe1a into spring-cloud:master Jan 9, 2017

2 of 4 checks passed

codecov/patch 63.63% of diff hit (target 74.22%)
Details
codecov/project 74.19% (-0.04%) compared to c380039
Details
ci/circleci Your tests passed on CircleCI!
Details
ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment