From d42b2543fa9f65e537b35b944faf845b60a35a87 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Fri, 3 Jun 2022 16:22:02 -0400 Subject: [PATCH] Do not supply discovery client when Consul discovery client disabled (#786) * Do not supply discovery client when Consul discovery client disabled. Fixes #785 * Fixes tests --- .../ConditionalOnConsulDiscoveryEnabled.java | 7 +++- .../ConsulConfigServerBootstrapper.java | 9 ++++- .../ConsulConfigServerBootstrapperTests.java | 40 +++++++++++++++++-- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConditionalOnConsulDiscoveryEnabled.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConditionalOnConsulDiscoveryEnabled.java index 658cf8bf..f53df9ca 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConditionalOnConsulDiscoveryEnabled.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConditionalOnConsulDiscoveryEnabled.java @@ -36,7 +36,12 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; @Retention(RetentionPolicy.RUNTIME) @Documented @Inherited -@ConditionalOnProperty(value = "spring.cloud.consul.discovery.enabled", matchIfMissing = true) +@ConditionalOnProperty(value = ConditionalOnConsulDiscoveryEnabled.PROPERTY, matchIfMissing = true) public @interface ConditionalOnConsulDiscoveryEnabled { + /** + * Property key. + */ + String PROPERTY = "spring.cloud.consul.discovery.enabled"; + } diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/configclient/ConsulConfigServerBootstrapper.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/configclient/ConsulConfigServerBootstrapper.java index 814020c4..e8062b03 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/configclient/ConsulConfigServerBootstrapper.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/configclient/ConsulConfigServerBootstrapper.java @@ -16,6 +16,8 @@ package org.springframework.cloud.consul.discovery.configclient; +import java.util.Collections; + import com.ecwid.consul.v1.ConsulClient; import org.springframework.boot.BootstrapRegistry; @@ -29,6 +31,7 @@ import org.springframework.cloud.config.client.ConfigClientProperties; import org.springframework.cloud.config.client.ConfigServerInstanceProvider; import org.springframework.cloud.consul.ConsulAutoConfiguration; import org.springframework.cloud.consul.ConsulProperties; +import org.springframework.cloud.consul.discovery.ConditionalOnConsulDiscoveryEnabled; import org.springframework.cloud.consul.discovery.ConsulDiscoveryClient; import org.springframework.cloud.consul.discovery.ConsulDiscoveryProperties; import org.springframework.util.ClassUtils; @@ -83,7 +86,7 @@ public class ConsulConfigServerBootstrapper implements BootstrapRegistryInitiali }); registry.registerIfAbsent(ConfigServerInstanceProvider.Function.class, context -> { if (!isDiscoveryEnabled(context.get(Binder.class))) { - return null; + return (id) -> Collections.emptyList(); } ConsulDiscoveryClient discoveryClient = context.get(ConsulDiscoveryClient.class); return discoveryClient::getInstances; @@ -96,7 +99,9 @@ public class ConsulConfigServerBootstrapper implements BootstrapRegistryInitiali } private boolean isDiscoveryEnabled(Binder binder) { - return binder.bind(ConfigClientProperties.CONFIG_DISCOVERY_ENABLED, Boolean.class).orElse(false); + return binder.bind(ConfigClientProperties.CONFIG_DISCOVERY_ENABLED, Boolean.class).orElse(false) + && binder.bind(ConditionalOnConsulDiscoveryEnabled.PROPERTY, Boolean.class).orElse(true) + && binder.bind("spring.cloud.discovery.enabled", Boolean.class).orElse(true); } } diff --git a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/configclient/ConsulConfigServerBootstrapperTests.java b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/configclient/ConsulConfigServerBootstrapperTests.java index 5e6eeba9..3c1a7b2d 100644 --- a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/configclient/ConsulConfigServerBootstrapperTests.java +++ b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/configclient/ConsulConfigServerBootstrapperTests.java @@ -16,8 +16,10 @@ package org.springframework.cloud.consul.discovery.configclient; +import java.util.Collections; import java.util.concurrent.atomic.AtomicReference; +import com.ecwid.consul.transport.TransportException; import org.junit.jupiter.api.Test; import org.springframework.boot.BootstrapRegistry; @@ -34,6 +36,7 @@ import org.springframework.cloud.consul.discovery.ConsulDiscoveryClient; import org.springframework.context.ConfigurableApplicationContext; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class ConsulConfigServerBootstrapperTests { @@ -44,11 +47,40 @@ public class ConsulConfigServerBootstrapperTests { .addBootstrapRegistryInitializer(registry -> registry.addCloseListener(event -> { ConfigServerInstanceProvider.Function providerFn = event.getBootstrapContext() .get(ConfigServerInstanceProvider.Function.class); - assertThat(providerFn).as("ConfigServerInstanceProvider.Function was created when it shouldn't") - .isNull(); + assertThat(providerFn.apply("id")) + .as("ConfigServerInstanceProvider.Function should return empty list") + .isEqualTo(Collections.EMPTY_LIST); })).run().close(); } + @Test + public void consulDiscoveryClientNotEnabledProvidesEmptyList() { + new SpringApplicationBuilder(TestConfig.class) + .properties("--server.port=0", "spring.cloud.service-registry.auto-registration.enabled=false", + "spring.cloud.config.discovery.enabled=true", "spring.cloud.consul.discovery.enabled=false") + .addBootstrapRegistryInitializer(registry -> registry.addCloseListener(event -> { + ConfigServerInstanceProvider.Function providerFn = event.getBootstrapContext() + .get(ConfigServerInstanceProvider.Function.class); + assertThat(providerFn.apply("id")) + .as("ConfigServerInstanceProvider.Function should return empty list") + .isEqualTo(Collections.EMPTY_LIST); + })).run().close(); + } + + @Test + public void springCloudDiscoveryClientNotEnabledProvidesEmptyList() { + new SpringApplicationBuilder(TestConfig.class) + .properties("--server.port=0", "spring.cloud.service-registry.auto-registration.enabled=false", + "spring.cloud.config.discovery.enabled=true", "spring.cloud.discovery.enabled=false") + .addBootstrapRegistryInitializer(registry -> registry.addCloseListener(event -> { + ConfigServerInstanceProvider.Function providerFn = event.getBootstrapContext() + .get(ConfigServerInstanceProvider.Function.class); + assertThat(providerFn.apply("id")) + .as("ConfigServerInstanceProvider.Function should return empty list") + .isEqualTo(Collections.EMPTY_LIST); + })).run().close(); + } + @Test public void enabledAddsInstanceProviderFn() { AtomicReference bootstrapDiscoveryClient = new AtomicReference<>(); @@ -63,7 +95,9 @@ public class ConsulConfigServerBootstrapperTests { bootstrapDiscoveryClient.set(event.getBootstrapContext().get(ConsulDiscoveryClient.class)); ConfigServerInstanceProvider.Function providerFn = event.getBootstrapContext() .get(ConfigServerInstanceProvider.Function.class); - assertThat(providerFn).as("ConfigServerInstanceProvider.Function was not created when it should.") + assertThatThrownBy(() -> providerFn.apply("id")).isInstanceOf(TransportException.class) + .hasMessageContaining("org.apache.http.conn.HttpHostConnectException: Connect to localhost:8500") + .as("Should have tried to reach out to Consul to get config server instance") .isNotNull(); })).run(); ConsulDiscoveryClient discoveryClient = context.getBean(ConsulDiscoveryClient.class);