Quarkus: Kubernetes: simplify env-vars injection

[metacosm]

Description
Currently, injecting environment variables into containers is possible using several mechanisms (see https://quarkus.io/guides/kubernetes#environment-variables). However, the way this is performed is less than ideal in particular when wanting to inject all values from a Secret or ConfigMap as the syntax quarkus.kubernetes.env-vars.my-env-var.secret=my-secret needs to be used. Note the presence of my-env-var is that property, which doesn't really make sense when injecting a secret and that is actually ignored as the result of using this property is to inject all the key/value pairs present in the secret without taking my-env-var into account at all. Using this syntax is currently required to distinguish between the different options, in particular for the field injection mechanism where the value of the variable is taken from another field of the resource. This current situation is confusing and could be made simpler.

Implementation ideas
Proposed solution would be to add reverse how the source of the value is identified: instead of using quarkus.kubernetes.env-vars.<variable name>.<source type>=<value>, we'd use quarkus.kubernetes.env-vars.<source type>(.<variable name>)?=<value>.

This would result in:

• quarkus.kubernetes.env-vars.var.foo=value to inject an arbitrary single value
• quarkus.kubernetes.env-vars.field.foo=fieldName to inject in the foo env var the value of the resource field named fieldName
• quarkus.kubernetes.env-vars.secret=secretName to inject all values from Secret named secretName
• quarkus.kubernetes.env-vars.configmap=configName to inject all values from ConfigMap named configName

This would also have the advantage of making the behavior more consistent with how labels and annotations are defined.

Another option could be to use the following syntax:

• quarkus.kubernetes.env-vars.foo=value to inject an arbitrary single value
• quarkus.kubernetes.env-from-fields.foo=fieldName to inject in the foo env var the value of the resource field named fieldName
• quarkus.kubernetes.env-from-secret=secretName to inject all values from Secret named secretName
• quarkus.kubernetes.env-from-configmap=configName to inject all values from ConfigMap named configName

Another option could be to drop field injection altogether and protect secret and configmap as reserved env-vars "names" so that we could do:

• quarkus.kubernetes.env-vars.foo=value to inject an arbitrary single value
• quarkus.kubernetes.env-vars.secret=secretName to inject all values from Secret named secretName
• quarkus.kubernetes.env-vars.configmap=configName to inject all values from ConfigMap named configName

There are, of course, other possible solutions but these seem to be the most reasonable ones to me, with option 1 having the advantage of coherence while keeping support existing features.

[iocanel]

I am not sure if the first suggestion is technically possible. As in some cases env-vars would map to a Map<String, String> and in some other cases to a Map<String, Map<String, String>>.

Maybe @dmlloyd has an idea on if and how it can be done.

I like the second suggestion though. It's a little more verbose, but it looks neat.

cc @geoand

[geoand]

I agree entirely with @iocanel. Options 2 seems interesting because Option 1 is pretty much off the table (AFAIK)

[dmlloyd]

I think it's possible to have a Map<String, String> alongside a Map<String, Map<String, String>>. I don't know if I specifically tested that but the system should be able to accommodate it: the matcher would essentially consider the number of segments to be significant and realise that the Map<String,String> cannot match if more property name segments exist.

[metacosm]

I will look into it to see what's feasible…

[metacosm]

From @iocanel:

I am a bit skeptic about it as it completely changes behaviour (breaking change). I am wondering if we could deprecate existing env-vars and introduce your idea under another keyword e.g. env. So then we would have:

quarkus.kubernetes.env.var.bar=value
quarkus.kubernetes.env.field.foo=fieldName
quarkus.kubernetes.env.secret=secretName
quarkus.kubernetes.env.configmap=configName

[metacosm]

From @iocanel:

I am a bit skeptic about it as it completely changes behaviour (breaking change). I am wondering if we could deprecate existing env-vars and introduce your idea under another keyword e.g. env.

I agree with the suggestion. However, what should be the conflict resolution procedure in case an application.properties mixes new and older versions? Should we assume that if an env variable is defined in both styles, the newer should replace the older one?

Meaning if we had this situation:

quarkus.kubernetes.env.var.bar=newValue
quarkus.kubernetes.env-vars.bar.value=oldValue

Quarkus would generate only one env var with the newValue as its value?

[geoand]

I agree that the newer one should take precedence. That is what we did IIRC when we moved the Kubernetes configuration under the Quarkus Config system.

We could also add logging that the old properties are now deprecated when we detect that they are being used.

[metacosm]

Another question: what should happen in this situation?

quarkus.kubernetes.env-vars.foo.value=bar
quarkus.kubernetes.env.fields.foo=fieldName

The new style defines an env var coming from a field whereas we still have an old-style definition, defining an env var coming from a direct value with the same name. This seems to indicate some user error: re-defining an env variable with the same name in a different way. Should that be raised as an error?

[geoand]

I believe we should raise an error. Best to be clear and provide actionable information than to end up in a confusing state, especially when mixing old and new style configurations.

[metacosm]

To be clear, I think that should also be an error when using only one style.

[geoand]

I don't get that, WDYM?

[metacosm]

If you do something like (so only using the new style):

quarkus.kubernetes.env.vars.foo=bar
quarkus.kubernetes.env.fields.foo=fieldName

an error should also be raised, which I don't think is the case at the moment, regardless of which style you use.

[iocanel]

Our goal is to guarantee forward compatibility. So throwing an error feels a little bit off for me.

My preference is the following:

• priority to new style over the old style.
• mixing both styles should be feasible.

which is also the approach we followed for old-style vs new style.

[metacosm]

Our goal is to guarantee forward compatibility. So throwing an error feels a little bit off for me.

My preference is the following:

* priority to new style over the old style.

* mixing both styles should be feasible.

which is also the approach we followed for old-style vs new style.

I'd argue that the goal should be to improve usability, regardless of how it's achieved. In this case, regardless of which style is used, I believe an error should be thrown when a user redefines an env variable name instead of silently erasing one.

Note also, that what you're saying is not incompatible with what I'm saying as this is what the implementation is already doing. The issue here is what should occur when a user redefines an env var, regardless of which style is used.

[geoand]

Here is my take:

• Mixing old and new when it doesn't make sense should throw an error - that way users can migrate quickly to the new config
• Adding seemingly incompatible fields should probably just result in a warning

[metacosm]

Why would we treat differently both cases? In both cases, it doesn't make sense and I'd argue it's even more an error when not dealing with backwards compatibility because you don't have the excuse of having leftover configuration that you haven't migrated yet… :)

[geoand]

Fair point, but then my take becomes that it should be a warning, not an error.

[metacosm]

What's the reasoning behind using a warning and not an error? Genuinely asking because it seems to me that this will result in something that is most definitively not something the user intended. Shouldn't this fail the build so that the error can be fixed instead of risking the warning being ignored and the user realizing at runtime that the configuration is not what they expected? Seems to me that an error would fit better with the Quarkus philosophy of addressing as much as possible at build time…

[geoand]

It just seems wrong to me to hard fail because of such a misconfiguration.
In my view, if the user wants to do something wrong despite being told that it probably won't work, then so be it.

If users want to ignore a warning then it's their fault.

Also, let's turn this on its head: If we were to make this an error, what sort of case would you classify as being worthy of a warning?

[metacosm]

It just seems wrong to me to hard fail because of such a misconfiguration.
In my view, if the user wants to do something wrong despite being told that it probably won't work, then so be it.
If users want to ignore a warning then it's their fault.

That doesn't sound right at all. Developer joy is about being proactive. Not letting people do dumb stuff we know won't work.

Also, let's turn this on its head: If we were to make this an error, what sort of case would you classify as being worthy of a warning?

Let me ask you the same question: what would be worthy of being an error then? :)

To me, warnings should be reserved to things that might be wrong but might also be intended. I don't have such case in mind here.

In the particular case we're discussing, though, this is definitely an error because you shouldn't be redeclaring a variable. If you redeclare a variable in your code, you expect the compiler to tell you made a mistake. I'd expect Quarkus to not let me ignore a mistake in my configuration because 1. it's easy to ignore warnings and 2. the error might go undiagnosed for a long time and cause weird behavior at runtime.

[metacosm]

To nuance this a bit, I could see it being a warning when a new version replaces an old version (so the migration case) but not when you create a new configuration that has conflicts.

[geoand]

Let me ask you the same question: what would be worthy of being an error then? :)

In my book, a worthy error is something that doesn't let me run or deploy my application.

[geoand]

Also from experience so far, users do pay attention to warnings

[metacosm]

Let me ask you the same question: what would be worthy of being an error then? :)

In my book, a worthy error is something that doesn't let me run or deploy my application.

Wouldn't a wrong set of environment variables qualify then? If your app expects some env vars to be set but they're not, your app won't run and might not even deploy. Would you rather wait for the app to be on the cluster to see it fail or fail at build time where you can fix it quickly instead of having to hunt down the cause of the error looking at your pods' logs?

[geoand]

Let's put it this way, currently if I do:

QUARKUS_HTTP_PORT=999999 mvn clean package

I won't get an error. I messed up the config and it will clearly never work as I intended, but I don't get an error. And I would argue that it's a much more severe error that what you are describing.

So yeah, if we were being super strict about evething then throwing an error would make sense. Given the current state, I think a warning is more than enough.

[metacosm]

I beg to differ as I see this is as an opportunity to improve, i.e. because something doesn't fail elsewhere when it probably should, shouldn't cause everything else to follow the same pattern, in my opinion. Otherwise, you're just lowering the bar everywhere.

That said, if folks think a warning is enough, warning it will be! :)

[maxandersen]

about warning and error I follow @metacosm 's concerns and I'm struggling to see why one wouldn't want a hard error (i.e. not see kubernetes extensions try deploy/configuration) in case of duplicate configured variablres.

My only concern is to be sure in devmode it will still survive edits to these.

[metacosm]

I've implemented a version that throws an error at the moment, pending a decision.

[geoand]

I'm not gonna debate this any more, I've been worn down...

Whatever you folks think is best I'll roll with

[geoand]

@iocanel would you like to do a first review of this?

[metacosm]

Identified a case where a warning make sense (adding several times the same configmap/secret source for example since doing so is idempotent and therefore not liable to cause future issues) so modified the implementation to support this.