Stricter and false positive env variables validation after upgrade to 3.7.0

[goldmann]

Describe the bug

This is not really a bug per se, but more a gotcha. The 3.7.0 version ensures now that when an environment variable matches the application configuration key -- it will assume that this env variable is for the application itself and it will try to map it within the application.

In my case this prevented the app to start.

Jan 30, 2024 10:53:22 AM org.hibernate.resource.beans.container.spi.AbstractCdiBeanContainer stop
INFO: HHH10005004: Stopping BeanContainer : io.quarkus.hibernate.orm.runtime.cdi.QuarkusArcBeanContainer@3a709cc7
Configuration validation failed:
	SRCFG00050: sbomer.service.service.host in EnvConfigSource does not map to any root
	SRCFG00050: sbomer.cache.port[7070]tcp in EnvConfigSource does not map to any root
	SRCFG00050: sbomer.cache.port[7070]tcp.proto in EnvConfigSource does not map to any root
	SRCFG00050: sbomer.service.port[80]tcp.addr in EnvConfigSource does not map to any root
	SRCFG00050: sbomer.postgres.service.port.http in EnvConfigSource does not map to any root
	SRCFG00050: sbomer.service.port in EnvConfigSource does not map to any root
	SRCFG00050: sbomer.route.host in EnvConfigSource does not map to any root
	SRCFG00050: sbomer.postgres.port[5432]tcp.proto in EnvConfigSource does not map to any root

The problem was that the env variables were not under my control as these were generated by Kubernetes based on service names.

It may be common for applications to behave similarly after upgrade to 3.7.0.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

[quarkus-bot]

/cc @geoand (kubernetes), @iocanel (kubernetes)

[ia3andy]

I had a similar issue on OpenShift, removing the unused environment fixed the issue for me.

[gsmet]

@radcortez could you clarify what's happening here? Should these env vars be ignored instead?

[gsmet]

/cc @maxandersen

[radcortez]

@radcortez could you clarify what's happening here? Should these env vars be ignored instead?

I looked, and there was an unnoticed bug in the previous version that did not correctly validate unmapped env vars. The new implementation fixed this as a side effect. Validations are applied to a prefix namespace set in a @ConfigMapping annotated class.

I recommend any of the following fixes:

• Remove unmapped env vars from the config.
• Add the corresponding mapping to the unmapped configuration.
• Ignore the missing mapping programmatically as Programmatically ignore unmapped properties project-ncl/sbomer#351
• Disable quarkus.config.mapping.validate-unknown=false (not totally recommended since its global)

[gsmet]

What I think might happen is that if you have a service named myservice, there's a good chance you might have env var starting with myservice together with a config mapping whose root is myservice.

I'm wondering if being very strict is a position we can keep. But maybe I have misunderstood the problem.

[radcortez]

What I think might happen is that if you have a service named myservice, there's a good chance you might have env var starting with myservice together with a config mapping whose root is myservice.

Yes, this may be a possibility. It really depends on how you structure your application.

I'm wondering if being very strict is a position we can keep. But maybe I have misunderstood the problem.

The strictness is there for all the other sources since the first version of mappings (I can't remember when it got to Quarkus, but probably it was in 1.x). An unfortunate bug, was not applying this correctly to the EnvSource. I can understand that this may start causing undesirable failures. We do have some easy fixes for users to get rid of the problem and we can think in some other options:

• We could relax the validation and make it a warning only as a default (with a configuration to fail - current behaviour)
• We could add the validation mode in the annotation
• We could ignore everything coming from EnvSources (effectively replicating the old behaviour, but turn it into a documented feature :))

For Quarkus, I have this disabled and unmapped properties are reported as unknown properties. It is probably useful to reconcile all of these.

[gsmet]

Yeah, my guess is that it's going to be a problem but I suppose we will see how Quarkus 3.7 is received once it is released.

[gsmet]

By the way, I think my vote goes for:

We could ignore everything coming from EnvSources (effectively replicating the old behaviour, but turn it into a documented feature :))

at least by default. Your environment might not be something you can adjust.

[goldmann]

Other option is to mention this in release notes, with some examples. I think there will be more people like me.

[gsmet]

Yeah, my problem is that if we end up having all our users implementing workaround to ignore env vars, we have a problem.

[radcortez]

Let's monitor it, and we change it if it becomes too problematic.

[maxandersen]

what is the trigger to make this validation fail? It doesn't happen on all quarkus apps if I read it right - just those that decided to have @ConfigMapping(prefix="myservice") somewhere correct ?

And then our logic is that if you have such config mapping then any config source must be validated before we launch and if some key exist that starts with "myservice." we treat that as an error?

If yes, then I definitely think this is problematic as the full environment is not necessarily in your apps full control.

I don't fully grok what "We could ignore everything coming from EnvSources" means - does that mean we won't even read the environment for these config mappings?

Sounds to me that "relax the validation and make it a warning only as a default (with a configuration to fail - current behaviour)" would be the better lenient but informative while still possible to configure the warnings away options.

[yrodiere]

Hey, FWIW failures also happen in scenarios where environment variables are perfectly valid and actually map to Quarkus config. The validation stuff is apparently yielding false positives.

For example if I have those configuration properties in my app:

quarkusio.web-uri
indexing.on-startup.when
indexing.error-reporting.type

... and use this in .env:

_DEV_QUARKUSIO_WEB_URI=http://localhost:4000
_DEV_INDEXING_ON_STARTUP_WHEN=indexes-empty
_DEV_INDEXING_ERROR_REPORTING_TYPE=github-issue

I get this (notice how e.g. QUARKUSIO_WEB_URI was interpreted as quarkusio.web.uri instead of quarkusio.web-uri):

Configuration validation failed:
        SRCFG00050: indexing.on.startup.when in EnvConfigSource[source=file:/home/yrodiere/workspaces/main/search.quarkus.io/.env] does not map to any root
        SRCFG00050: indexing.error.reporting.type in EnvConfigSource[source=file:/home/yrodiere/workspaces/main/search.quarkus.io/.env] does not map to any root
        SRCFG00050: quarkusio.web.uri in EnvConfigSource[source=file:/home/yrodiere/workspaces/main/search.quarkus.io/.env] does not map to any root
java.lang.RuntimeException: Failed to start quarkus
        at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
        at io.quarkus.runtime.Application.start(Application.java:101)
...

Setting quarkus.config.mapping.validate-unknown=false makes the error disappear, and env variables are taken into account, proving that they were not, in fact, unmapped.

See https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Env.20var.20translation.20changed.20in.203.2E7.3F

If I had to guess, I'd say that env variable mapping probably generates all possible interpretations of _ characters (since they can be translated to _, -, %, ...); some of those (duplicate) interpretations end up being caught by the validation stuff and prevent the application from starting.

[radcortez]

what is the trigger to make this validation fail? It doesn't happen on all quarkus apps if I read it right - just those that decided to have @ConfigMapping(prefix="myservice") somewhere correct ?

And then our logic is that if you have such config mapping then any config source must be validated before we launch and if some key exist that starts with "myservice." we treat that as an error?

Consider:

@ConfigMapping(prefix = "my.service")
interface MyService {
  String value();
}

The configuration expects my.service.value to be set, which you can set via properties file, system property, env, etc. Now consider the following environment:

# this works and validates 
export MY_SERVICE_VALUE=value

export MY_SERVICE_VALUE=value
# this fails and throws the unmapped error
export MY_SERVICE_ANOTHER=value

The goal of the validation is to make sure that all properties for the mapping are mapped in the interface class. Now, due to a bug in previous versions the environment variable validation was not being applied correctly. If you try this in previous versions of Quarkus:

my.service.value=value
my.service.another=value

It would also fail. With the new version, it also started to consider the environment variables. The interesting thing is that we never noticed the bug before, and when I redid the implementation, I've fixed it by accident :)

An additional thing to consider is that the quarkus namespace is except for validation, because we are going to encounter configuration that may be runtime in build time or vice-versa.

I don't fully grok what "We could ignore everything coming from EnvSources" means - does that mean we won't even read the environment for these config mappings?

Not ignore the values, but ignore the validation part, effectively replicating the behaviour we had before, but not as a bug but as a documented feature :)

Sounds to me that "relax the validation and make it a warning only as a default (with a configuration to fail - current behaviour)" would be the better lenient but informative while still possible to configure the warnings away options.

Yes, I think that this is the best option. The validation can turn into three types: ERROR, WARNING, NONE, where WARNING becomes the default.

[radcortez]

For example if I have those configuration properties in my app:

quarkusio.web-uri
indexing.on-startup.when
indexing.error-reporting.type

... and use this in .env:

_DEV_QUARKUSIO_WEB_URI=http://localhost:4000
_DEV_INDEXING_ON_STARTUP_WHEN=indexes-empty
_DEV_INDEXING_ERROR_REPORTING_TYPE=github-issue

I get this (notice how e.g. QUARKUSIO_WEB_URI was interpreted as quarkusio.web.uri instead of quarkusio.web-uri):

There is an issue where profiled env names where not being correctly matched with the mapping. This should fix it: smallrye/smallrye-config#1100

[mschorsch]

We have the same problem. A warning should be sufficient instead of an error.

[gsmet]

Yeah, we can start with a warning with an option to raise it either to error or none. But I suspect the warnings will get extremely annoying very fast as people can't really act on their env.

@radcortez I think this is top priority given how many people it concerns. Also we need to be able to backport it.

[radcortez]

Yeah, we can start with a warning with an option to raise it either to error or none. But I suspect the warnings will get extremely annoying very fast as people can't really act on their env.

@radcortez I think this is top priority given how many people it concerns. Also we need to be able to backport it.

Yes, I'm working on this. Yesterday, I fixed an issue with the profiles. I'll be adding the warning capability next.

[gsmet]

Perfect, thanks for the heads up.

[ctomc]

problem with this is that env variables that it complains about are the ones that kubernetes automatically injects in every container so they can be used as service discovery, see https://kubernetes.io/docs/concepts/services-networking/service/#environment-variables for reference.
There is no way to not get those env variables in your container.

[radcortez]

problem with this is that env variables that it complains about are the ones that kubernetes automatically injects in every container so they can be used as service discovery, see https://kubernetes.io/docs/concepts/services-networking/service/#environment-variables for reference.

We are already working in a relaxed validation. If this is a blocker for you, please use any of the alternatives described in #38479 (comment)

It is OK to use quarkus.config.mapping.validate-unknown=false. You do lose the validation of configurations that are not mapped, which may prevent a few bugs, but turning this to false is not going to affect your application in any way and it will allow you to move forward past this issue.

[gsmet]

There is no way to not get those env variables in your container.

Yeah, that's why I'm not sure having warnings will fly. They are going to be annoying. @radcortez we should probably advertise how to get rid of the warnings in the message.

(@ctomc hey, nice to see you here!)

[radcortez]

Maybe then we need to take a step back and discuss if these errors / warnings are useful at all.

To summarize:

The validation exists to detect mismatches between the provided configuration and the mapping. It may help in the following scenarios:

• Typos in the configuration
• Removed configuration not updated yet

This is the same goal with Quarkus unknown configuration and the same feature applied to mappings (but mappings are stricter). I guess we never had such an issue with Quarkus because we control the namespace, and users fully control what they set in quarkus

Moving forward, completely disabling the feature for mappings is like not having it because users must remember to turn it on. Still, let's discuss different alternatives:

1. Remove the feature completely
2. Remove the feature for Env Vars only (bugged behavior from previous versions, but now documented)
3. Keep the feature for everything; change it to warn by default, with additional none and error modes. Provide a way to override the global behavior in the mapping itself.

Probably 2 is the less annoying, while 3 gives you more flexibility but requires you to fine-tune the mappings.

[yrodiere]

I would add a fourth alternative: make it configurable per configuration source, defaulting to error for e.g. application.properties but warning/disabled for environment variables.

[gsmet]

I think it has a lot of values for application.properties and we should keep it.

For environment, the discussion is a bit different as it's not something you can always handle. Now I can also see the value of having a warning when you try to configure something via environment and it doesn't work.

[maxandersen]

The warning/validations has value but it is not something that is obvious good being default on or off for all.

I think default on for sources users can control makes sense but for env where you don't have full control we should not be strict and yes if Kubernetes default inject then fail and warn is bad default.

[radcortez]

While less likely, you may not have full control of application.properties either. Configurations can pollute your config system by properties files coming from a classpath resource or even external files set up for you in the deployment. We can certainly add this behavior per source, but deciding which sources this should be on or off may be tricky.

I think option 3 is still better. Even if you turn it off for specific namespaces, you can turn it on occasionally to check how misaligned you are from the mapping and make adjustments if possible. With a per-source approach, it would possibly require multiple toggles to get 100% validation, and you may even miss sources that you may not be aware of.

[maxandersen]

If I grok it right we've been running with option 2 (fail everywhere except env) the last 3 years; is that true?

Would say that's pretty good evidence that eager validation outside of env is workable ?

[radcortez]

If I grok it right we've been running with option 2 (fail everywhere except env) the last 3 years; is that true?

Correct.

Would say that's pretty good evidence that eager validation outside of env is workable ?

It is. The question is if we should make it per configuration source as suggested by option 4. I think it is tricky to do it.

My recommendation, for now, is to provide a hotfix to have the behavior of 2 and then think about what we should do for the next major release.

[yrodiere]

My recommendation, for now, is to provide a hotfix to have the behavior of 2 and then think about what we should do for the next major release.

+1. Sorry if I gave the impression I was requesting that feature: I was just suggesting another way forward for later, when the rush is behind us. I completely agree with you that option 2 is the most reasonable approach for a hotfix.

[juandiii]

Same issue here :(

[radcortez]

Same issue here :(

Please check #38479 (comment). Sorry for the inconvenience.

[juandiii]

Same issue here :(

Please check #38479 (comment). Sorry for the inconvenience.

Thank u !!