Quarkus Redis extension does not allow empty redis.hosts property

[The-Funk]

Describe the bug

Redis DevServices have been fixed to start if the redis.hosts property is null OR empty. However in the case of empty, even though the dev services have started, the Redis extension throws an exception because the hosts value is empty.

This is needed for people like me who have ephemeral GitHub Actions runners for their CI that live inside a K8s cluster and cannot run docker themselves. These runners must be given an actual host value when running tests, and so the CI environment has been given real redis instances. However when testing locally on a laptop, or on a VM, docker is available, and as such there's no need to give redis.hosts a value or point to a remote instance of redis. This is useful because of corporate firewalling which makes accessing a remote service rather painful.

All of this leads to an interesting conundrum. What if you need a test container for local dev and a physical instance for remote CI, and both tests should run under the same profile for guaranteed consistency?

Expected behavior

If Redis hosts property is null OR empty, dev services should start
If redis hosts present OR dev services started, Quarkus Redis extension should not throw.

Actual behavior

Redis DevServices have been fixed to start if the redis.hosts property is null OR empty. However in the case of empty, even though the dev services have started, the Redis extension throws an exception because the hosts value is empty. This causes the Quarkus application to fail to start.

How to Reproduce?

No response

Output of uname -a or ver

Windows 11

Output of java -version

21

Quarkus version or git rev

2.45.1

Build tool (ie. output of mvnw --version or gradlew --version)

3.9.8

Additional information

No response

[quarkus-bot]

/cc @Ladicek (redis), @cescoffier (redis), @machi1990 (redis)

[cescoffier]

Fancy a PR?

[The-Funk]

@cescoffier will give it the old college try!

[The-Funk]

@cescoffier This might be a little outside my pay range but wanted to get your thoughts.

quarkus/extensions/redis-client/runtime/src/main/java/io/quarkus/redis/runtime/client/VertxRedisClientFactory.java

Line 68 in 7aa6f66

throw new ConfigurationException("Redis host not configured - you must either configure 'quarkus.redis.hosts` or" +

I found the specific area in code where this happens. All of this is in the quarkus-redis-client extension, under runtime/client.

In the file linked above, if redis hosts or redis.hosts-provider-name is configured, even if it is empty, this factory is invoked in order to create the client(s). You could have one client or many named clients or annotated clients.

In the RedisClientProcessor, lines 138 to 158 gather those client names and config details to be sent to the factory for creation, followed by line 161 which then initializes the client.

My thought is, if there's an easy way to determine if a Redis client bean exists already with a given name, signifying that the bean was already created by dev services, we would simply ignore calling initialize on line 161 for those names, this includes the default client too.

We could make this change even more intentional if this information could be directly queried from dev services. That would probably be best because then we could check if dev services is enabled prior to filtering out names of clients that already exist, reducing the risk of a regression where a bean might exist temporarily in the container but is then removed for some reason, and thus...failure.

Is there an easy way in the RedisClientProcessor class that we could query dev services to determine if it has initialized any named beans/default beans for a class or is the only solution for that a direct call to the ArC?

[The-Funk]

@cescoffier any thoughts on this?

[cescoffier]

Is there an easy way in the RedisClientProcessor class that we could query dev services to determine if it has initialized any named beans/default beans for a class or is the only solution for that a direct call to the ArC?

Unfortunately, not really. The dev service injects the configuration as a user would; nothing indicates it comes from a dev service. So, I don't believe you can detect it at runtime.

Also, knowing if a bean of certain type has been created can be tricky, as the bean instance will be created lazily. @mkouba knows more, so it may explain if it's possible.

[mkouba]

Is there an easy way in the RedisClientProcessor class that we could query dev services to determine if it has initialized any named beans/default beans for a class or is the only solution for that a direct call to the ArC?

Unfortunately, not really. The dev service injects the configuration as a user would; nothing indicates it comes from a dev service. So, I don't believe you can detect it at runtime.

Also, knowing if a bean of certain type has been created can be tricky, as the bean instance will be created lazily. @mkouba knows more, so it may explain if it's possible.

I'm not quite sure I understand the question. The RedisClientProcessor is a build time construct. Dev service is a runtime thing.

query dev services to determine if it has initialized any named beans/default beans for a class

To query what exactly where exactly (e.g. in a build step or from some code at runtime)?

[The-Funk]

Is there an easy way in the RedisClientProcessor class that we could query dev services to determine if it has initialized any named beans/default beans for a class or is the only solution for that a direct call to the ArC?

Unfortunately, not really. The dev service injects the configuration as a user would; nothing indicates it comes from a dev service. So, I don't believe you can detect it at runtime.
Also, knowing if a bean of certain type has been created can be tricky, as the bean instance will be created lazily. @mkouba knows more, so it may explain if it's possible.

I'm not quite sure I understand the question. The RedisClientProcessor is a build time construct. Dev service is a runtime thing.

query dev services to determine if it has initialized any named beans/default beans for a class

To query what exactly where exactly (e.g. in a build step or from some code at runtime)?

@mkouba My mistake. The issue is, at runtime, the redis extension detects whether redis hosts is null or empty and then shuts down if redis hosts is empty. The actual behavior should be, if dev services is not already providing the host and host is null or empty, then shut down.

I'm running into an issue where if I set my redis host value to something effectively empty, my application fails to start, even though dev services starts and my redis container should be responding to requests.

If the value is null, there is no problem, however effectively empty, there is a problem.

I have different CI environments with different requirements. For instance I have some ephemeral GitHub Actions runners in Kubernetes that can't run Docker. These containers need to be able to run my tests and so I provide them with a legitimate redis hosts value via an environment variable.

quarkus.redis.hosts=${MY_REDIS_HOST_FROM_ENV}

At the same time, I have developers off site that need to work on this same project and need to be able to test that given any redis instance, their code will work. We want these tests to run with the same profile so there is exactly zero bias per environment. The same exact test running under the same exact circumstances should run in both environments.

Enter dev services. If the value above is empty, dev services fires up, and as long as the developer has Docker Desktop on their machine, they can then work and run their unit tests against a real redis instance.

Except something at runtime is checking that redis.hosts is empty and then the entire application fails to start.

The desired behavior is, if hosts null or empty AND dev services not running for this instance, fail to start, else start.

[mkouba]

The desired behavior is, if hosts null or empty AND dev services not running for this instance, fail to start, else start.

I think that I understand now. Thanks ;-).

My thought is, if there's an easy way to determine if a Redis client bean exists already with a given name, signifying that the bean was already created by dev services, we would simply ignore calling initialize on line 161 for those names, this includes the default client too.

I was able to reproduce the problem with redis-quickstart - just added quarkus.redis.hosts=${MY_REDIS_HOST_FROM_ENV} in application.properties and set the env variable to an empty value. And I think that I found the real cause. The Redis dev service is started correctly and it also sets the quarkus.redis.hosts to the corresponding values (host:port of the running container) but under the hood a RunTimeConfigurationDefaultBuildItem is produced. The problem is that in case of an empty value the default value is not used at all. Instead, an empty value is interpreted as an empty list null and thus VertxRedisClientFactory#create() fails; unlike when no property is set at all - in this case, the default value provided by the dev service is used.

In other words, it's not a Redis-specific issue but more a general Dev service/config problem.

For now, I would recommend to unset the env variable instead of using an empty value.

I'm not quite sure how to solve this problem in general. @cescoffier @radcortez

[radcortez]

I'm not quite sure how to solve this problem in general. @cescoffier @radcortez

This is expected. Setting an empty value unsets the configuration. Configuration always worked this way. In this case, because Env has a higher ordinal than defaults, it clears the configuration of quarkus.redis.host.

From the Config perspective, having a null property or empty property are two different things. If this is not the desired behavior for the Redis extension, then the DevServices configuration has to be set in another namespace so it can be used as a fallback default.

[mkouba]

I'm not quite sure how to solve this problem in general. @cescoffier @radcortez

This is expected. Setting an empty value unsets the configuration. Configuration always worked this way. In this case, because Env has a higher ordinal than defaults, it clears the configuration of quarkus.redis.host.

From the Config perspective, having a null property or empty property are two different things. If this is not the desired behavior for the Redis extension, then the DevServices configuration has to be set in another namespace so it can be used as a fallback default.

Hm, I would intuitively expect that an empty value will result in an empty collection and not null. But that wouldn't help in this case anyway.

FYI the config property in question is actually Optional<Set<URI>> hosts(). And it returns an empty optional in this particular case.

In any case, I agree that "empty" is not the same as "absent".

[The-Funk]

@mkouba @radcortez

From a config perspective I'd agree, empty and absent should be two different things. Maybe this is a question of context with dev services then? Maybe each extension that offers dev services needs to know if dev services was bootstrapped or maybe an explicit config flag is needed per extension to indicate that the programmer is requesting dev services in a given environment regardless of other configuration?

My end goal was for dev services to take over without errors on empty or null

But now that I think about it, I'm sure there's a slew of other scenarios where someone might explicitly want to say, "use the dev services, don't look at this config" and I envision that as an all extensions kind of thing, not just the Redis extension. An explicit flag might be the answer here. This is mainly due to the environment I described above. I was working on a large corporate network and getting developer laptops connected to cloud resources was annoying and difficult, so we started using test containers, and that worked great until our CI environment was containerized and started failing because you can't fire up docker inside of docker.

One thing we could have done to work around the issue is spin up a separate testing profile for that environment but this kind of breaks the spirit of testing. The ideal scenario is, the test profile is the only profile used to run tests and therefore the tests run in a deterministic fashion everywhere.

[The-Funk]

I apologize, this is potentially shifting into partially a feature request. This might be breaking for people that rely on the automated launch of dev services on null values currently, but you could also gate this behind a control flag.

Here's what I envision:

  # Should dev services do what it has always done, or do you, the developer, want manual control?
  quarkus.devservices.transmission= oneof [automatic || manual]
  # Global, set this to true to tell all extensions you want to use dev services regardless of each individual extensions config values
  quarkus.devservices.requested= oneof [true || false]
  # Per extension, in case any of those dev containers are troublesome, these settings could override the global setting, per extension. These flags could also be used as an alternative to setting the flag globally.
  quarkus.redis.devservices.requested= oneof [true || false] 

[radcortez]

DevServices work by setting up the relevant connection configuration for the services if no connection configuration can be found. The problem is that the DevServices config evaluation happens at build time, not runtime. Please check this other comment about a related issue: #42392 (comment)

[The-Funk]

DevServices work by setting up the relevant connection configuration for the services if no connection configuration can be found. The problem is that the DevServices config evaluation happens at build time, not runtime. Please check this other comment about a related issue: #42392 (comment)

If I understand correctly, in this case dev services is starting but because the environment variable in application.properties takes precedence over the provided dev services value, we wind up in the situation where Quarkus fails to start because the extension thinks there's nothing to connect to, even though just a moment before it fired up a test container for that extension.

This discrepancy in actual behavior vs expected behavior is likely my fault.

Changing the order of precedence in the config would be a big breaking change, and there's no way to do that. But what if there was a opt-in flag in the config which could be checked by the extensions prior to the normal configuration evaluation, and that one config could just say, "ignore everything else and just use dev services to get these config values, trust me bro."

Such a flag would need to be read and considered on a per extension basis but it would enable the developer to say, "trust me I am asking you to use test containers regardless of any other config you might find."

This would provide a singular place to evaluate if the user wants dev services running and then dev services check becomes externalized from the typical config evaluation, which given their scope I think makes sense.

[radcortez]

If I understand correctly, in this case dev services is starting but because the environment variable in application.properties takes precedence over the provided dev services value, we wind up in the situation where Quarkus fails to start because the extension thinks there's nothing to connect to, even though just a moment before it fired up a test container for that extension.

Mostly. Things are a bit more tricky, because the evaluated configuration is the build time configuration, and we ignore Env in build time.

This discrepancy in actual behavior vs expected behavior is likely my fault.

Ahahah! I did mention that was a bad ideia :)

Changing the order of precedence in the config would be a big breaking change, and there's no way to do that. But what if there was a opt-in flag in the config which could be checked by the extensions prior to the normal configuration evaluation, and that one config could just say, "ignore everything else and just use dev services to get these config values, trust me bro."

Such a flag would need to be read and considered on a per extension basis but it would enable the developer to say, "trust me I am asking you to use test containers regardless of any other config you might find."

This would provide a singular place to evaluate if the user wants dev services running and then dev services check becomes externalized from the typical config evaluation, which given their scope I think makes sense.

Yes, that is my recommendation. In the same way we have a property to disable DevServices, we could have one to force them, no matter what.

[geoand]

What's the status of this one?

[radcortez]

My proposal was to have a way to force DevServices. Do we see other alternatives?