micronaut-projects / micronaut-core

Micronaut Application Framework
http://micronaut.io
Apache License 2.0
6.01k stars 1.05k forks source link

Kubernetes environmental variable not being processed #6667

Open javiertoja opened 2 years ago

javiertoja commented 2 years ago

Expected Behavior

When I defined a configuration property with the name my.system.service.port, and an environmental variable with the name MY_SYSTEM_SERVICE_PORT. In both my local environment and inside a kubernetes cluster the configuration property is overwritten at runtime with the value of the environmental variable.

Actual Behaviour

Because the sufix SERVICE_PORT, micronaut decides that the environmental variable is not user generated, rather than automatically expanded by kubernetes and decides to do not process the variable, defaulting the configuration property value to the value present in the configuration property file.

Without a warning, a log entry, or a note in the official documentation.

Steps To Reproduce

Don't needed, just don't exclude sufixes here

https://github.com/micronaut-projects/micronaut-core/blob/3.2.x/inject/src/main/java/io/micronaut/context/env/KubernetesEnvironmentPropertySource.java

Environment Information

No response

Example Application

No response

Version

2.5.12, but more are affected

graemerocher commented 2 years ago

This is by design, since k8s clusters can add thousands of environment variables. If you need k8s service discovery use the Kubernetes module https://github.com/micronaut-projects/micronaut-kubernetes/

javiertoja commented 2 years ago

This is by design, since k8s clusters can add thousands of environment variables. If you need k8s service discovery use the Kubernetes module https://github.com/micronaut-projects/micronaut-kubernetes/

I don't agree with this design, but I accept it.

I think that at least the official documentation should reflect this because i was not able to find anything related inside the official documentation and lost a lot of time debugging internally the core of micronaut to find the cause.

graemerocher commented 2 years ago

sounds like a good idea, could be a good improvement for https://docs.micronaut.io/latest/guide/#propertySource

PRs welcome of course.

ianroberts commented 2 years ago

And besides, the SERVICE_PORT environment variable isn't necessarily the port on which your pod should be listening, it's the listening port for the service that wraps your pods. It's perfectly fine - and indeed common - in k8s for the two to be different, e.g. a deployment of pods running as an unprivileged user (which can't bind low numbered ports) with each pod listening on port 8888, then a Service in front of those pods exposing them to the rest of the cluster on port 80.

graemerocher commented 2 years ago

Indeed, there are multiple reasons to favour the kubernete modules' service discovery feature because also the environment variables can become out of date as the cluster changes which breaks service to service communication. With the kubernetes module and service discovery any changes to IPs and ports are refreshed automatically thus preventing the need for a pod restart

javiertoja commented 2 years ago

The issue was not related to service discovery, the issue was about why some environmental variables with certain suffixes where not being processed due to their name, without a hint in the doc. I can have a concept within the domain of my application that contain any of those suffixes, not related to routing or service discovery and variables with that suffix will not be read in a kubernetes environment while in any other environment it will work as expected.

I understand your point about service discovery and your kubernetes module, but banning all the environmental variables with those suffixes assuming that they will always be used for routing and connecting to an internal service module its something that i don't agree with. Specially because there are a lot of more services outside a kubernetes environment, legacy API's, Third-party service providers ..., and in all of those scenarios developers are being force to don't use those suffixes due to this design decision.

I lost my time tracking the problem within my service because it was working perfectly in my local environment (non k8s) and in testing. It only failed when we deployed to the dev k8s cluster, so it was a non trivial bug hunting due to me not knowing about the design decision because it is not documented. According to this section the name of the variables that i was using was valid and fare.

For example, banning _TCP is preventing developer from using variables like MAX_TCP that could mean in a context, maximum threshold counting projects or whatever.

graemerocher commented 2 years ago

I agree, though it is a tricky problem to solve since in large clusters reading all those environment variables results in performance issues and an impact to application startup time. See https://github.com/micronaut-projects/micronaut-kubernetes/issues/209 for the origin of the change.

ianroberts commented 2 years ago

A sensible solution to me seems like

javiertoja commented 2 years ago

A sensible solution to me seems like

  • ignore those k8s-type environment variables by default
  • provide some sort of bootstrap option to allow that filtering to be disabled by the user if they know what they're doing
  • document this clearly, perhaps with a note that if you do want to run under k8s with the filtering disabled then it's a good idea to set enableServiceLinks: false in your Deployment/DaemonSet/StatefulSet manifest

I think that these will be enough if had know about the naming scheme I could have use a different one, also putting a switch to enable/disable this filtering promotes portability from other frameworks/platforms that might be accepting at the moment environment variable with this suffixes. By this means developers are allowed to have a choice.