From df6d748cd374c559ea0b920c0099bbd243181e63 Mon Sep 17 00:00:00 2001 From: Tom Gianos Date: Fri, 7 Dec 2018 11:00:08 -0800 Subject: [PATCH] Fix Zookeeper health endpoint bean registration (#185) There was a bug in the evaluation order for ZookeeperHealthIndicator bean where its conditions for creation, particularly whether there was a CuratorFramework bean, were evaluated too early causing it to always fail. This commit moves the inner ZookeeperAutoConfiguration.ZookeeperHealthConfig class into its own public class ZookeeperHealthAutoConfiguration and makes sure its configured as auto configuration after ZookeeperAutoConfiguration. This seems to resolve the order problem in testing. Unit tests added for conditions on ZookeeperHealthIndicator bean. Minor change to ZookeeperHealthIndicator to only call curator getState method once rather than twice. --- .../zookeeper/ZookeeperAutoConfiguration.java | 16 ----- .../ZookeeperHealthAutoConfiguration.java | 56 +++++++++++++++ .../zookeeper/ZookeeperHealthIndicator.java | 5 +- .../main/resources/META-INF/spring.factories | 3 +- ...ZookeeperHealthAutoConfigurationTests.java | 70 +++++++++++++++++++ 5 files changed, 131 insertions(+), 19 deletions(-) create mode 100644 spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperHealthAutoConfiguration.java create mode 100644 spring-cloud-zookeeper-core/src/test/java/org/springframework/cloud/zookeeper/ZookeeperHealthAutoConfigurationTests.java diff --git a/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperAutoConfiguration.java b/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperAutoConfiguration.java index ee5b2351..a57162ab 100644 --- a/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperAutoConfiguration.java +++ b/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperAutoConfiguration.java @@ -24,10 +24,6 @@ import org.apache.curator.framework.CuratorFramework; import org.apache.curator.framework.CuratorFrameworkFactory; import org.apache.curator.retry.ExponentialBackoffRetry; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.actuate.autoconfigure.health.ConditionalOnEnabledHealthIndicator; -import org.springframework.boot.actuate.endpoint.annotation.Endpoint; -import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; -import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Bean; @@ -81,16 +77,4 @@ public class ZookeeperAutoConfiguration { properties.getMaxRetries(), properties.getMaxSleepMs()); } - - @Configuration - @ConditionalOnClass(Endpoint.class) - protected static class ZookeeperHealthConfig { - @Bean - @ConditionalOnMissingBean - @ConditionalOnBean(CuratorFramework.class) - @ConditionalOnEnabledHealthIndicator("zookeeper") - public ZookeeperHealthIndicator zookeeperHealthIndicator(CuratorFramework curator) { - return new ZookeeperHealthIndicator(curator); - } - } } diff --git a/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperHealthAutoConfiguration.java b/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperHealthAutoConfiguration.java new file mode 100644 index 00000000..7c70ac45 --- /dev/null +++ b/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperHealthAutoConfiguration.java @@ -0,0 +1,56 @@ +/* + * Copyright 2013-2018 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 + * + * http://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.zookeeper; + +import org.apache.curator.framework.CuratorFramework; +import org.springframework.boot.actuate.autoconfigure.health.ConditionalOnEnabledHealthIndicator; +import org.springframework.boot.actuate.endpoint.annotation.Endpoint; +import org.springframework.boot.autoconfigure.AutoConfigureAfter; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +/** + * Auto {@link Configuration} for adding a Zookeeper health endpoint to actuator if + * required. + * + * @author tgianos + * @since 2.0.1 + */ +@Configuration +@ConditionalOnZookeeperEnabled +@ConditionalOnClass(Endpoint.class) +@AutoConfigureAfter({ ZookeeperAutoConfiguration.class }) +public class ZookeeperHealthAutoConfiguration { + + /** + * If there is an active curator, if the zookeeper health endpoint is enabled and if a + * health indicator hasn't already been added by a user add one. + * + * @param curator The curator connection to zookeeper to use + * @return An instance of {@link ZookeeperHealthIndicator} to add to actuator health + * report + */ + @Bean + @ConditionalOnMissingBean(ZookeeperHealthIndicator.class) + @ConditionalOnBean(CuratorFramework.class) + @ConditionalOnEnabledHealthIndicator("zookeeper") + public ZookeeperHealthIndicator zookeeperHealthIndicator(CuratorFramework curator) { + return new ZookeeperHealthIndicator(curator); + } +} diff --git a/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperHealthIndicator.java b/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperHealthIndicator.java index 36cd7b74..aeb77dd6 100644 --- a/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperHealthIndicator.java +++ b/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/ZookeeperHealthIndicator.java @@ -38,7 +38,8 @@ public class ZookeeperHealthIndicator extends AbstractHealthIndicator { @Override protected void doHealthCheck(Health.Builder builder) throws Exception { try { - if (this.curator.getState() != CuratorFrameworkState.STARTED) { + CuratorFrameworkState state = this.curator.getState(); + if (state != CuratorFrameworkState.STARTED) { builder.down().withDetail("error", "Client not started"); } else if (this.curator.checkExists().forPath("/") == null) { @@ -49,7 +50,7 @@ public class ZookeeperHealthIndicator extends AbstractHealthIndicator { } builder.withDetail("connectionString", this.curator.getZookeeperClient().getCurrentConnectionString()) - .withDetail("state", this.curator.getState()); + .withDetail("state", state); } catch (Exception e) { builder.down(e); diff --git a/spring-cloud-zookeeper-core/src/main/resources/META-INF/spring.factories b/spring-cloud-zookeeper-core/src/main/resources/META-INF/spring.factories index ae022cd4..1dbe5c5b 100644 --- a/spring-cloud-zookeeper-core/src/main/resources/META-INF/spring.factories +++ b/spring-cloud-zookeeper-core/src/main/resources/META-INF/spring.factories @@ -1,3 +1,4 @@ # Auto Configuration org.springframework.boot.autoconfigure.EnableAutoConfiguration=\ -org.springframework.cloud.zookeeper.ZookeeperAutoConfiguration +org.springframework.cloud.zookeeper.ZookeeperAutoConfiguration,\ +org.springframework.cloud.zookeeper.ZookeeperHealthAutoConfiguration diff --git a/spring-cloud-zookeeper-core/src/test/java/org/springframework/cloud/zookeeper/ZookeeperHealthAutoConfigurationTests.java b/spring-cloud-zookeeper-core/src/test/java/org/springframework/cloud/zookeeper/ZookeeperHealthAutoConfigurationTests.java new file mode 100644 index 00000000..baf2d023 --- /dev/null +++ b/spring-cloud-zookeeper-core/src/test/java/org/springframework/cloud/zookeeper/ZookeeperHealthAutoConfigurationTests.java @@ -0,0 +1,70 @@ +/* + * Copyright 2013-2018 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 + * + * http://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.zookeeper; + +import org.apache.curator.framework.CuratorFramework; +import org.assertj.core.api.Assertions; +import org.junit.Test; +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.context.annotation.Bean; + +/** + * Tests for {@link ZookeeperHealthAutoConfiguration}. + * + * @author tgianos + * @since 2.0.1 + */ +public class ZookeeperHealthAutoConfigurationTests { + private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() + .withConfiguration(AutoConfigurations.of(ZookeeperAutoConfiguration.class, + ZookeeperHealthAutoConfiguration.class)) + .withUserConfiguration(ZookeeperAutoConfigurationTests.TestConfig.class); + + @Test + public void testDefaultPropertiesCreateZookeeperHealthIndicator() { + this.contextRunner.run((context) -> Assertions.assertThat(context) + .hasSingleBean(ZookeeperHealthIndicator.class)); + } + + @Test + public void testZookeeperHealthIndicatorDisabled() { + this.contextRunner.withPropertyValues("management.health.zookeeper.enabled=false") + .run((context) -> Assertions.assertThat(context) + .doesNotHaveBean(ZookeeperHealthIndicator.class)); + } + + @Test + public void testZookeeperHealthIndicatorAlreadyAdded() { + this.contextRunner.withUserConfiguration(HealthIndicatorCustomConfig.class) + .run((context) -> { + Assertions.assertThat(context) + .hasSingleBean(ZookeeperHealthIndicator.class); + Assertions.assertThat(context) + .doesNotHaveBean("zookeeperHealthIndicator"); + Assertions.assertThat(context) + .hasBean("customZookeeperHealthIndicator"); + }); + } + + static class HealthIndicatorCustomConfig { + @Bean + ZookeeperHealthIndicator customZookeeperHealthIndicator( + CuratorFramework curatorFramework) { + return new ZookeeperHealthIndicator(curatorFramework); + } + } +}