spring-cloud / spring-cloud-vault

Configuration Integration with HashiCorp Vault
http://cloud.spring.io/spring-cloud-vault/
Apache License 2.0
270 stars 151 forks source link

Reactive auto-configuration - Both health indicators VaultHealthIndicator and VaultReactiveHealthIndicator are registered in the application context #676

Closed raestio closed 1 year ago

raestio commented 1 year ago

Describe the bug Hi, I'm using Spring Cloud Vault with the Spring WebFlux stack:

As it is a reactive stack, Vault’s reactive auto-configuration is enabled. What seems to be a bug is that there are now 2 Vault health indicators registered in the application context - the reactive one and non-reactive one. When /actuator/health endpoint is called both health indicators (VaultHealthIndicator and VaultReactiveHealthIndicator) are executed.

Or is it meant to be like that because you can have Spring Cloud Vault on Servlet Stack (Spring MVC) and be using Spring's WebClient from org.springframework:spring-webflux dependency?

Sample

I've created a sample test in which we can see the both indicators are registered:

public class VaultReactiveAutoConfigurationIssueDemonstrationSampleTests {

    private final ApplicationContextRunner contextRunner = new ApplicationContextRunner()
            .withConfiguration(AutoConfigurations.of(
                    VaultReactiveAutoConfiguration.class,
                    VaultAutoConfiguration.class,
                    VaultHealthIndicatorAutoConfiguration.class));

    @Test
    void bothReactiveAndNonReactiveHealthIndicatorsAreConfigured() {
        this.contextRunner
                .withPropertyValues("spring.cloud.vault.token=foo")
                .run(context -> {
            assertThat(context).hasBean("vaultHealthIndicator");
            assertThat(context).hasBean("vaultReactiveHealthIndicator");
        });
    }
}

Or you can check it out directly here: VaultReactiveAutoConfigurationIssueDemonstrationSampleTests

Suggestion

I've checked other Spring Boot auto-configurations and for example here is the auto-configuration for Redis:

RedisHealthContributorAutoConfiguration RedisReactiveHealthContributorAutoConfiguration

We can see that in RedisHealthContributorAutoConfiguration

@Bean
@ConditionalOnMissingBean(name = { "redisHealthIndicator", "redisHealthContributor" })
public HealthContributor redisHealthContributor(Map<String, RedisConnectionFactory> redisConnectionFactories) {
    return createContributor(redisConnectionFactories);
}

they use the same @ConditionalOnMissingBean name as in RedisReactiveHealthContributorAutoConfiguration:

@Bean
@ConditionalOnMissingBean(name = { "redisHealthIndicator", "redisHealthContributor" })
public ReactiveHealthContributor redisHealthContributor() {
    return createContributor(this.redisConnectionFactories);
}

Which would prevent the creation of the non-reactive health indicator (we would need to change the @Import order here as well VaultHealthIndicatorAutoConfiguration)

What do you think? I would be happy to prepare and submit a pull request.

mp911de commented 1 year ago

Imperative endpoints can be exposed via JMX. Endpoints are registered based on the presence of a VaultOperations bean respective the presence of the Flux type on the classpath. We do not differentiate between the availability of WebMVC vs. WebFlux.

This setup is common in Spring Boot itself.

raestio commented 1 year ago

Thanks for the comment. Yes, so just to confirm, is it OK that both VaultHealthIndicator and VaultReactiveHealthIndicator are registered and contribute to health at the same time even though they do basically the same check?

{
  "vault": {
    "status": "UP",
    "details": {
      "version": "1.12.3"
    }
  },
  "vaultReactive": {
    "status": "UP",
    "details": {
      "state": "Vault in standby",
      "version": "1.12.3"
    }
  }
}

As opposed to the example I've provided where in e.g. Redis they configure only one health contributor (redisHealthIndicator) and the reactive one has precedence.

mp911de commented 1 year ago

As opposed to the example I've provided where in e.g. Redis they configure only one health contributor (redisHealthIndicator) and the reactive one has precedence.

Thanks for pointing this out. You're right, I totally missed that one indicator's configuration backs off in case of such an relationship between the beans. Feel free to submit a pull request to align the conditions, precedence, and bean names. We should also keep the vaultReactiveHealthIndicator bean name as an alias to avoid breaking references that use the bean by name.

raestio commented 1 year ago

No problem, I've just submitted the pull request and it's ready for review 👍