Support both dynamic mgmt & http ports

chriswhite199 commented 5 years ago

Using both port 0 for management.server and server causes the following stack trace in 2.1.1.RELEASE:

Stacktrace

java.lang.IllegalArgumentException: createCheck port must be greater than 0
    at org.springframework.util.Assert.isTrue(Assert.java:118) ~[spring-core-5.1.6.RELEASE.jar:5.1.6.RELEASE]
    at org.springframework.cloud.consul.serviceregistry.ConsulAutoRegistration.createCheck(ConsulAutoRegistration.java:236) ~[spring-cloud-consul-discovery-2.1.1.RELEASE.jar:2.1.1.RELEASE]
    at org.springframework.cloud.consul.serviceregistry.ConsulAutoRegistration.setCheck(ConsulAutoRegistration.java:129) ~[spring-cloud-consul-discovery-2.1.1.RELEASE.jar:2.1.1.RELEASE]
    at org.springframework.cloud.consul.serviceregistry.ConsulAutoRegistration.initializePort(ConsulAutoRegistration.java:336) ~[spring-cloud-consul-discovery-2.1.1.RELEASE.jar:2.1.1.RELEASE]
    at org.springframework.cloud.consul.serviceregistry.ConsulAutoServiceRegistration.getRegistration(ConsulAutoServiceRegistration.java:58) ~[spring-cloud-consul-discovery-2.1.1.RELEASE.jar:2.1.1.RELEASE]
    at org.springframework.cloud.consul.serviceregistry.ConsulAutoServiceRegistration.getRegistration(ConsulAutoServiceRegistration.java:33) ~[spring-cloud-consul-discovery-2.1.1.RELEASE.jar:2.1.1.RELEASE]
    at org.springframework.cloud.client.serviceregistry.AbstractAutoServiceRegistration.start(AbstractAutoServiceRegistration.java:137) ~[spring-cloud-commons-2.1.1.RELEASE.jar:2.1.1.RELEASE]
    at org.springframework.cloud.consul.serviceregistry.ConsulAutoServiceRegistration.start(ConsulAutoServiceRegistration.java:73) ~[spring-cloud-consul-discovery-2.1.1.RELEASE.jar:2.1.1.RELEASE]
    at org.springframework.cloud.consul.serviceregistry.ConsulAutoServiceRegistrationListener.onApplicationEvent(ConsulAutoServiceRegistrationListener.java:63) ~[spring-cloud-consul-discovery-2.1.1.RELEASE.jar:2.1.1.RELEASE]

Issue tracing

Tracing through, the issue lies in org.springframework.cloud.consul.serviceregistry.ConsulAutoRegistration#getManagementPort which defers to org.springframework.cloud.client.discovery.ManagementServerPortUtils#getPort, which in turn returns 0 as ManagementServerProperties.port is 0.

Appreciate this is more likely an issue with spring-cloud-commons (where ManagementServerPortUtils is defined), but raising here for visibility for others who may stumble down this path

application.yml

management:
  server:
    port: 0

server:
  port: 0

Application

@SpringBootApplication
@EnableDiscoveryClient
public class DemoWebServicesApplication {
    public static void main(String[] args) {
        SpringApplication.run(DemoWebServicesApplication.class, args);
    }
}

chriswhite199 commented 5 years ago

A simple config based patch:

@EnableDiscoveryClient
@Configuration
public class DynamicMgmtPortConsulConfig implements ApplicationListener<WebServerInitializedEvent> {
    private ConsulDiscoveryProperties discoveryProperties;

    public DynamicMgmtPortConsulConfig(ConsulDiscoveryProperties discoveryProperties) {
        this.discoveryProperties = discoveryProperties;
    }

    @Override
    public void onApplicationEvent(WebServerInitializedEvent event) {
        if (StringUtils.equals(event.getApplicationContext().getServerNamespace(), "management")) {
            int mgmtPort = event.getWebServer().getPort();
            this.discoveryProperties.setManagementPort(mgmtPort);
        }
    }
}

ryanjbaxter commented 5 years ago

Would you be interested in submitting a PR based on that patch?

chriswhite199 commented 5 years ago

I meant that patch to useful to developers while an official version is patched. As i mentioned, i think the ManagementServerPortUtils needs to be fixed to address this issue, rather than an AOP based hack.

That being said, i'll raise another ticket for ManagementServerPortUtils and reference it here. Hopefully when that is closed out - this can be closed too

chriswhite199 commented 5 years ago

An update - it appears that the order of web server startup (web vs mgmt) is not well defined. I'm still seeing random failures on linux nodes where web comes up before mgmt, so my AOP patch above isn't always going to work. I have other patches that work when you disable lifecycle.enabled and manually start the config after the ApplicationStartEvent is fired, but it all seem hacky. Will investigate further.

spencergibb commented 4 years ago

Reclassifying as an enhancement as support for dynamic management port is not trivial.

spring-cloud / spring-cloud-consul