Skip to content

Support loading config files (YAML and properties) embedded in env vars via spring.config.import #41609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chicobento opened this issue Jul 24, 2024 · 8 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@chicobento
Copy link
Contributor

chicobento commented Jul 24, 2024

Use case

Providing a Helm Chart for a Spring Boot application, typically requires translating input from values.yaml to application properties.
There are many ways to achieve this, one of the most straightforward ways being using environment variables.
However, there are a few drawbacks with such strategy:

  • limitations with case-sensitive properties such as Document logger environment variable restrictions #17958
  • helm / kubernetes resources are yaml-based and dynamically translating yaml snippets to environment variables requires some helm templating wizardry
  • support for defining property source / document order priority

Converting yaml snippet to json and injecting it in the SPRING_APPLICATION_JSON almost fits the purpose, but helm's fromYaml function has limitations such as support for multi-documents (see helm/helm#11647) and SPRING_APPLICATION_JSON doesnt support much flexibility in terms of document order priority.

The proposal

To improve developer experience when creating helm charts for Spring Boot apps, the proposal is to allow loading via spring.config.import a full config yaml file encoded in a single environment variable.

Given the following application.yaml (burned in the container image):

logging:
  level:
    acme.MyClass: ERROR

spring:
  config:
    import:       
    - optional:env:HELM_APPLICATION_YAML[.yaml]

When the multi-line environment variable HELM_APPLICATION_YAML is defined for the container (rendered by helm):

logging:
  level:
    acme.MyClass: DEBUG
---
acme:
  property1: 1

Then, the final configured level for acme.MyClass should be DEBUG.

Optionally we can support base64-encoded env vars for simplifying creation of multi-line env vars, via something like optional:env:base64:HELM_APPLICATION_YAML[.yaml]

Ps: In the examples I mentioned yaml, but it would support properties files as well via the [.properties] hint

I already have a PoC working code for supporting this and Im willing to contribute if the feature is acceptable.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 24, 2024
@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jul 25, 2024
@philwebb
Copy link
Member

This is an interesting proposal. I've flagged it for team discussion (hopefully sometime next week)

@scottfrederick
Copy link
Contributor

@chicobento Environment variables are often used to map input from values.yaml into properties in the application container, but if there are a lot of properties then using ConfigMaps can be simpler.

For example, if you have something like this in values.yaml:

config:
  spring:
    application:
      name: helm-demo
  app:
    greeting: "Hello"
    name: "World!"

and create a new template in templates/configMap.yaml like this:

apiVersion: v1
kind: ConfigMap
metadata:
  name: app-configmap
data:
  helm-values.yaml: |-
    {{ .Values.config | toYaml | nindent 4 }}

then you can mount the ConfigMap into the container in templates/deployment.yaml:

spec:
  template:
    spec:
      containers:
          volumeMounts:
            - name: app-config-volume
              mountPath: /config
      volumes:
        - name: app-config-volume
          configMap:
            name: app-configmap

and directly import the contents of this configuration from the mounted helm-values.yaml file in the application's main application.yaml file:

spring:
  application:
    name: demo
  config:
    import: optional:/config/helm-values.yaml

This will work now, without any changes to Spring Boot, and without mapping from YAML to the environment. Is this approach something you have explored, and if so is there a reason this won't work for your use cases? If there is a reason why this won't work, can you give us a more complete example of what you are doing?

@scottfrederick scottfrederick added the status: waiting-for-feedback We need additional information before we can continue label Jul 31, 2024
@chicobento
Copy link
Contributor Author

chicobento commented Aug 1, 2024

Hi @scottfrederick , thank you for the very detailed feedback.

We have being doing exactly this since spring.config.import was added to SB and we want to move away from it due to several reasons, the main one being the problem with upgrades and backward compatibility of the spring properties.
Sorry for the very long blog post 😆, but follows some reasoning.

Upgrade problem

Lets say that the v1 of your app has the following config:

config:
  app:
    should-say-hello: true # boolean property

Now, in the v2 version you changed the property to an enum:

config:
  app:
    should-say-hello: mornings-only # always, never or mornings-only

When you run the helm upgrade command, one of the first resources to be upgraded will be the configmap.
Thats fine, the new v2 pods will take the new configmap and assuming that the old v1 ones doesnt have any hot-reload facility (which in fact we have), they will not react to the change and will still keep serving the traffic until they are removed.

What happens if something goes wrong during the upgrade ?

If the v2 pods doesn't become ready or take too much time - for whatever reason, and in this timeframe the v1 pods needs to be restarted (either because of a crash or due to scaling up), kubernetes will spawn new v1 pods for replacing the crashed ones using the deployment created from the v1 chart. At this point, the configmap has the v2 chart contents but kubernetes is trying to spawn new v1 pods which will obviously crash because they cannot recognize the new config format and we at the risk of not being able to serve any traffic.

We have reproduced this by creating a simple app and adding a delay in the startup of the v2 and manually deleting the old pods. The delay was added by placing a Thread.sleep(600000) in Spring Boot main class.

Possible solutions

We have tried to version the configmap by applying a suffix to its name, i.e: name: app-configmap-v1 and name: app-configmap-v2 but then we figured out that helm will uninstall the app-configmap-v1 and install the app-configmap-v2 in the very beginning of the upgrade, so if the v1 pods needs to be restarted they will get stuck because of the absence of the app-configmap-v1 resource. This could be solved by using helm helm.sh/resource-policy: keep but then the application would have to take care of the housekeeping of the resources - or leave it to the ops team.

Im afraid that almost every single spring boot application out there using this type of solution may be exposed to this type of bug, since it is tricky to setup this scenario for testing.

Other issues

Besides the upgrade problem, I personally dislike exposing this internal type of config as a config map. Users may be tempted to update the config via configmap rather than via the helm chart values and then the config map would become one additional interface to be honored and maintained. I'd rather use configmaps for cases where I'm fully commited to maintain as an exposed interface, or cases where for example multiple services needs to share/use the same configuration snippet that would be provisioned by another system or process, i.e operators or integration charts.

While I'd love to hear about solutions and alternatives around those problems, the bottom line is that they should not be there in first place if env variables are used instead of configmaps and for this reason Im requesting this feature to simplify config loading via env variables.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 1, 2024
@scottfrederick
Copy link
Contributor

@chicobento Can you show an example of how the Helm chart converts values.yaml properties to the YAML environment variable, and an example of the environment variable contents?

@chicobento
Copy link
Contributor Author

chicobento commented Aug 9, 2024

Sure.
I created this example https://github.com/chicobento/spring-boot-config-env-example/ based on https://github.com/helm/examples/tree/main/charts/hello-world:

Added config to values.yaml

config:
  spring:
    application:
      name: helm-demo
  app:
    greeting: "Hello"
    name: "World!"

Added envs to deployment.yaml

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      containers:
        - name: {{ .Chart.Name }}
          env:
          - name: SPRING_APPLICATION_YAML
            value: |-
            {{- .Values.config | toYaml | nindent 14 }}
          - name: SPRING_APPLICATION_YAML_BASE64
            value: "{{- .Values.config | toYaml | b64enc }}"

Now if you install the chart (I did with kind), shell into the pod and echo "$SPRING_APPLICATION_YAML" or echo $SPRING_APPLICATION_YAML_BASE64 | base64 -d you'll get the following:

app:
  greeting: Hello
  name: World!
spring:
  application:
    name: helm-demo

Then it should be able to be loaded in boot app by doing something like:

spring:
  config:
    import:       
    - optional:env:SPRING_APPLICATION_YAML[.yaml]
    - optional:env:base64:SPRING_APPLICATION_YAML_BASE64[.yaml]

@philwebb philwebb added for: team-meeting An issue we'd like to discuss as a team to make progress status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement and removed for: team-meeting An issue we'd like to discuss as a team to make progress status: waiting-for-triage An issue we've not yet triaged labels Dec 11, 2024
@philwebb philwebb added this to the 3.5.x milestone Dec 18, 2024
@mhalbritter
Copy link
Contributor

I already have a PoC working code for supporting this and Im willing to contribute if the feature is acceptable.

Hey @chicobento, could you please create a PR with your code? That'd be much appreciated. Thanks!

@chicobento
Copy link
Contributor Author

Greetings, @mhalbritter. Happy new year! Sorry for the delay, I was out for vacation. I have an initial draft, please let me know your thoughts.

@mhalbritter mhalbritter removed status: feedback-provided Feedback has been provided status: pending-design-work Needs design work before any code can be developed labels Jan 31, 2025
@mhalbritter mhalbritter modified the milestones: 3.5.x, 3.5.0-M2 Jan 31, 2025
@chicobento
Copy link
Contributor Author

Thanks a lot, @mhalbritter and team!

philwebb added a commit that referenced this issue Feb 1, 2025
Rename classes to align with existing `SystemEnvironment...` classes
and extract common `FileExtensionHint` logic.

See gh-41609
@wilkinsona wilkinsona changed the title Support loading config yaml files embedded in env vars via spring.config.import Support loading config files (YAML and properties) embedded in env vars via spring.config.import Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants