From 7fd20e8d464d32dc332878f46eca26e2b546d707 Mon Sep 17 00:00:00 2001 From: Christian Lorenz Date: Wed, 23 Oct 2019 11:08:07 +0200 Subject: [PATCH 1/4] add option to configure Consul check status considered as healthy --- .../discovery/ConsulDiscoveryProperties.java | 19 ++++++ .../cloud/consul/discovery/ConsulServer.java | 9 ++- .../consul/discovery/ConsulServerList.java | 14 ++-- .../ConsulDiscoveryPropertiesTests.java | 14 ++++ .../consul/discovery/ConsulServerTest.java | 68 +++++++++++++++++++ .../HealthServiceServerListFilterTests.java | 32 ++------- 6 files changed, 124 insertions(+), 32 deletions(-) create mode 100644 spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulServerTest.java diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryProperties.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryProperties.java index e180f228d..dbf3e89a1 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryProperties.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryProperties.java @@ -17,11 +17,15 @@ package org.springframework.cloud.consul.discovery; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import com.ecwid.consul.v1.ConsistencyMode; +import com.ecwid.consul.v1.health.model.Check; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.context.properties.ConfigurationProperties; @@ -160,6 +164,12 @@ public class ConsulDiscoveryProperties { */ private boolean queryPassing = false; + /** + * List of status considered as healthy. Contains "passing" by default. + */ + private Set statusConsideredAsHealthy = new HashSet( + Collections.singleton(Check.CheckStatus.PASSING)); + /** Register as a service in consul. */ private boolean register = true; @@ -470,10 +480,19 @@ public boolean isQueryPassing() { return this.queryPassing; } + public Set getStatusConsideredAsHealthy() { + return statusConsideredAsHealthy; + } + public void setQueryPassing(boolean queryPassing) { this.queryPassing = queryPassing; } + public void setStatusConsideredAsHealthy( + Set statusConsideredAsHealthy) { + this.statusConsideredAsHealthy = statusConsideredAsHealthy; + } + public boolean isRegister() { return this.register; } diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java index 5f0fdae4e..be50d81fc 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java @@ -17,6 +17,7 @@ package org.springframework.cloud.consul.discovery; import java.util.Map; +import java.util.Set; import com.ecwid.consul.v1.health.model.Check; import com.ecwid.consul.v1.health.model.HealthService; @@ -33,11 +34,15 @@ public class ConsulServer extends Server { private final HealthService service; + private final Set statusConsideredAsHealthy; + private final Map metadata; - public ConsulServer(final HealthService healthService) { + public ConsulServer(final HealthService healthService, + Set statusConsideredAsHealthy) { super(findHost(healthService), healthService.getService().getPort()); this.service = healthService; + this.statusConsideredAsHealthy = statusConsideredAsHealthy; this.metadata = ConsulServerUtils.getMetadata(this.service); this.metaInfo = new MetaInfo() { @Override @@ -79,7 +84,7 @@ public Map getMetadata() { public boolean isPassingChecks() { for (Check check : this.service.getChecks()) { - if (check.getStatus() != Check.CheckStatus.PASSING) { + if (!statusConsideredAsHealthy.contains(check.getStatus())) { return false; } } diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java index a34d06fb2..92fd35dcd 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java @@ -19,10 +19,12 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Set; import com.ecwid.consul.v1.ConsulClient; import com.ecwid.consul.v1.QueryParams; import com.ecwid.consul.v1.Response; +import com.ecwid.consul.v1.health.model.Check; import com.ecwid.consul.v1.health.model.HealthService; import com.netflix.client.config.IClientConfig; import com.netflix.loadbalancer.AbstractServerList; @@ -83,20 +85,24 @@ private List getServers() { if (response.getValue() == null || response.getValue().isEmpty()) { return Collections.emptyList(); } - return transformResponse(response.getValue()); + return transformResponse(response.getValue(), + this.properties.getStatusConsideredAsHealthy()); } /** * Transforms the response from Consul in to a list of usable {@link ConsulServer}s. * @param healthServices the initial list of servers from Consul. Guaranteed to be * non-empty list + * @param statusConsideredAsHealthy list of status that are considered as + * healthy/passing * @return ConsulServer instances - * @see ConsulServer#ConsulServer(HealthService) + * @see ConsulServer#ConsulServer(HealthService, java.util.Set) */ - protected List transformResponse(List healthServices) { + protected List transformResponse(List healthServices, + Set statusConsideredAsHealthy) { List servers = new ArrayList<>(); for (HealthService service : healthServices) { - ConsulServer server = new ConsulServer(service); + ConsulServer server = new ConsulServer(service, statusConsideredAsHealthy); if (server.getMetadata() .containsKey(this.properties.getDefaultZoneMetadataName())) { server.setZone(server.getMetadata() diff --git a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java index a9b99fcc8..02df4541d 100644 --- a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java +++ b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java @@ -19,6 +19,7 @@ import java.util.Collections; import java.util.Map; +import com.ecwid.consul.v1.health.model.Check; import org.junit.Before; import org.junit.Test; @@ -95,4 +96,17 @@ public void testAddManagementTag() { .containsOnly(ConsulDiscoveryProperties.MANAGEMENT, "newTag"); } + @Test + public void testConsiderStatusAsHealthyContainsPassingByDefault() { + assertThat(this.properties.getStatusConsideredAsHealthy()) + .containsExactly(Check.CheckStatus.PASSING); + } + + @Test + public void testConsiderWarningStatusAsHealthy() { + this.properties.getStatusConsideredAsHealthy().add(Check.CheckStatus.WARNING); + assertThat(this.properties.getStatusConsideredAsHealthy()) + .containsExactly(Check.CheckStatus.WARNING, Check.CheckStatus.PASSING); + } + } diff --git a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulServerTest.java b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulServerTest.java new file mode 100644 index 000000000..86094ac90 --- /dev/null +++ b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulServerTest.java @@ -0,0 +1,68 @@ +/* + * Copyright 2013-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.consul.discovery; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +import com.ecwid.consul.v1.health.model.Check; +import com.ecwid.consul.v1.health.model.HealthService; +import org.junit.Test; + +import static com.ecwid.consul.v1.health.model.Check.CheckStatus.CRITICAL; +import static com.ecwid.consul.v1.health.model.Check.CheckStatus.PASSING; +import static com.ecwid.consul.v1.health.model.Check.CheckStatus.WARNING; +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Christian Lorenz + */ +public class ConsulServerTest { + + @Test + public void testIsPassingChecks() { + Set acceptedStatus = new HashSet<>( + Arrays.asList(PASSING, WARNING)); + assertThat(newServer(PASSING, acceptedStatus).isPassingChecks()).isTrue(); + assertThat(newServer(WARNING, acceptedStatus).isPassingChecks()).isTrue(); + assertThat(newServer(CRITICAL, acceptedStatus).isPassingChecks()).isFalse(); + } + + static ConsulServer newServer(Check.CheckStatus checkStatus, + Set acceptedStatus) { + HealthService healthService = new HealthService(); + HealthService.Node node = new HealthService.Node(); + node.setAddress("nodeaddr" + checkStatus.name()); + node.setNode("nodenode" + checkStatus.name()); + healthService.setNode(node); + HealthService.Service service = new HealthService.Service(); + service.setAddress("serviceaddr" + checkStatus.name()); + service.setId("serviceid" + checkStatus.name()); + service.setPort(8080); + service.setService("serviceservice" + checkStatus.name()); + healthService.setService(service); + ArrayList checks = new ArrayList<>(); + Check check = new Check(); + check.setStatus(checkStatus); + checks.add(check); + healthService.setChecks(checks); + return new ConsulServer(healthService, acceptedStatus); + } + +} diff --git a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/HealthServiceServerListFilterTests.java b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/HealthServiceServerListFilterTests.java index 25e162f3b..62964bf71 100644 --- a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/HealthServiceServerListFilterTests.java +++ b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/HealthServiceServerListFilterTests.java @@ -17,10 +17,9 @@ package org.springframework.cloud.consul.discovery; import java.util.ArrayList; +import java.util.Collections; import java.util.List; -import com.ecwid.consul.v1.health.model.Check; -import com.ecwid.consul.v1.health.model.HealthService; import com.netflix.loadbalancer.Server; import org.junit.Test; @@ -28,6 +27,7 @@ import static com.ecwid.consul.v1.health.model.Check.CheckStatus.PASSING; import static com.ecwid.consul.v1.health.model.Check.CheckStatus.WARNING; import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.cloud.consul.discovery.ConsulServerTest.newServer; /** * @author Spencer Gibb @@ -39,33 +39,13 @@ public void testGetFilteredListOfServers() { HealthServiceServerListFilter filter = new HealthServiceServerListFilter(); ArrayList servers = new ArrayList<>(); - servers.add(newServer(PASSING)); - servers.add(newServer(PASSING)); - servers.add(newServer(CRITICAL)); - servers.add(newServer(WARNING)); + servers.add(newServer(PASSING, Collections.singleton(PASSING))); + servers.add(newServer(PASSING, Collections.singleton(PASSING))); + servers.add(newServer(CRITICAL, Collections.singleton(PASSING))); + servers.add(newServer(WARNING, Collections.singleton(PASSING))); List filtered = filter.getFilteredListOfServers(servers); assertThat(filtered).as("wrong # of filtered servers").hasSize(2); } - private ConsulServer newServer(Check.CheckStatus checkStatus) { - HealthService healthService = new HealthService(); - HealthService.Node node = new HealthService.Node(); - node.setAddress("nodeaddr" + checkStatus.name()); - node.setNode("nodenode" + checkStatus.name()); - healthService.setNode(node); - HealthService.Service service = new HealthService.Service(); - service.setAddress("serviceaddr" + checkStatus.name()); - service.setId("serviceid" + checkStatus.name()); - service.setPort(8080); - service.setService("serviceservice" + checkStatus.name()); - healthService.setService(service); - ArrayList checks = new ArrayList<>(); - Check check = new Check(); - check.setStatus(checkStatus); - checks.add(check); - healthService.setChecks(checks); - return new ConsulServer(healthService); - } - } From 78f67452ad55781f7c1a2ba0a914e9898782faf3 Mon Sep 17 00:00:00 2001 From: Christian Lorenz Date: Thu, 12 Dec 2019 11:52:14 +0100 Subject: [PATCH 2/4] * document new property * restore backwards-compatibility of changed classes --- docs/src/main/asciidoc/_configprops.adoc | 1 + docs/src/main/asciidoc/spring-cloud-consul.adoc | 14 ++++++++++++++ .../cloud/consul/discovery/ConsulServer.java | 5 +++++ .../cloud/consul/discovery/ConsulServerList.java | 11 ++++------- 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/docs/src/main/asciidoc/_configprops.adoc b/docs/src/main/asciidoc/_configprops.adoc index 57be0be47..d101a8db4 100644 --- a/docs/src/main/asciidoc/_configprops.adoc +++ b/docs/src/main/asciidoc/_configprops.adoc @@ -54,6 +54,7 @@ |spring.cloud.consul.discovery.scheme | http | Whether to register an http or https service. |spring.cloud.consul.discovery.server-list-query-tags | | Map of serviceId's -> tag to query for in server list. This allows filtering services by a single tag. |spring.cloud.consul.discovery.service-name | | Service name. +|spring.cloud.consul.discovery.status-considered-as-healthy | passing | The accepted health check states of instances used for routing in load balancer. |spring.cloud.consul.discovery.tags | | Tags to use when registering service. |spring.cloud.consul.enabled | true | Is spring cloud consul enabled. |spring.cloud.consul.host | localhost | Consul agent hostname. Defaults to 'localhost'. diff --git a/docs/src/main/asciidoc/spring-cloud-consul.adoc b/docs/src/main/asciidoc/spring-cloud-consul.adoc index 9d417dce0..6275cd418 100644 --- a/docs/src/main/asciidoc/spring-cloud-consul.adoc +++ b/docs/src/main/asciidoc/spring-cloud-consul.adoc @@ -279,6 +279,20 @@ public String serviceUrl() { } ---- +==== Instances used by Load-balancer + +The load balancer will only use instances of a service with health checks in "passing" state by default. However Consul also offers the "warning" state, which you might still want to accept for routing. You can configure the accepted states via property `spring.cloud.consul.discovery.status-considered-as-healthy`, e.g. +.application.yml +---- +spring: + cloud: + consul: + discovery: + status-considered-as-healthy: + - PASSING + - WARNING +---- + === Consul Catalog Watch The Consul Catalog Watch takes advantage of the ability of consul to https://www.consul.io/docs/agent/watches.html#services[watch services]. The Catalog Watch makes a blocking Consul HTTP API call to determine if any services have changed. If there is new service data a Heartbeat Event is published. diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java index be50d81fc..bf98409a9 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java @@ -16,6 +16,7 @@ package org.springframework.cloud.consul.discovery; +import java.util.Collections; import java.util.Map; import java.util.Set; @@ -38,6 +39,10 @@ public class ConsulServer extends Server { private final Map metadata; + public ConsulServer(final HealthService healthService) { + this(healthService, Collections.singleton(Check.CheckStatus.PASSING)); + } + public ConsulServer(final HealthService healthService, Set statusConsideredAsHealthy) { super(findHost(healthService), healthService.getService().getPort()); diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java index 92fd35dcd..9e3f2d9f0 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java @@ -85,24 +85,21 @@ private List getServers() { if (response.getValue() == null || response.getValue().isEmpty()) { return Collections.emptyList(); } - return transformResponse(response.getValue(), - this.properties.getStatusConsideredAsHealthy()); + return transformResponse(response.getValue()); } /** * Transforms the response from Consul in to a list of usable {@link ConsulServer}s. * @param healthServices the initial list of servers from Consul. Guaranteed to be * non-empty list - * @param statusConsideredAsHealthy list of status that are considered as - * healthy/passing * @return ConsulServer instances * @see ConsulServer#ConsulServer(HealthService, java.util.Set) */ - protected List transformResponse(List healthServices, - Set statusConsideredAsHealthy) { + protected List transformResponse(List healthServices) { List servers = new ArrayList<>(); for (HealthService service : healthServices) { - ConsulServer server = new ConsulServer(service, statusConsideredAsHealthy); + ConsulServer server = new ConsulServer(service, + this.properties.getStatusConsideredAsHealthy()); if (server.getMetadata() .containsKey(this.properties.getDefaultZoneMetadataName())) { server.setZone(server.getMetadata() From 5974bbcbdb3e9365ca66a4dafbbbebf742f97d64 Mon Sep 17 00:00:00 2001 From: Christian Lorenz Date: Thu, 12 Dec 2019 13:55:47 +0100 Subject: [PATCH 3/4] fix imports for checkstyle --- .../cloud/consul/discovery/ConsulServerList.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java index 9e3f2d9f0..7c9c92596 100644 --- a/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java +++ b/spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java @@ -19,12 +19,10 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Set; import com.ecwid.consul.v1.ConsulClient; import com.ecwid.consul.v1.QueryParams; import com.ecwid.consul.v1.Response; -import com.ecwid.consul.v1.health.model.Check; import com.ecwid.consul.v1.health.model.HealthService; import com.netflix.client.config.IClientConfig; import com.netflix.loadbalancer.AbstractServerList; From 29893c72b933318f2b066b8dccd0e1f00f694443 Mon Sep 17 00:00:00 2001 From: Christian Lorenz Date: Thu, 12 Dec 2019 14:08:41 +0100 Subject: [PATCH 4/4] fix unit test by ignoring order in result --- .../cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java index 02df4541d..85865ead0 100644 --- a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java +++ b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryPropertiesTests.java @@ -106,7 +106,7 @@ public void testConsiderStatusAsHealthyContainsPassingByDefault() { public void testConsiderWarningStatusAsHealthy() { this.properties.getStatusConsideredAsHealthy().add(Check.CheckStatus.WARNING); assertThat(this.properties.getStatusConsideredAsHealthy()) - .containsExactly(Check.CheckStatus.WARNING, Check.CheckStatus.PASSING); + .containsExactlyInAnyOrder(Check.CheckStatus.WARNING, Check.CheckStatus.PASSING); } }