Skip to content
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

Example for using env var secrets is impossible #22

Open
AndrewFarley opened this issue Sep 26, 2024 · 6 comments
Open

Example for using env var secrets is impossible #22

AndrewFarley opened this issue Sep 26, 2024 · 6 comments

Comments

@AndrewFarley
Copy link

AndrewFarley commented Sep 26, 2024

  additionalEnv:
    - name: DATABASE_URL
      valueFrom:
        secretKeyRef:
          name: my-secrets
          key: langfuse-database-url

This is impossible (via this example) because of how this has been defined.

            {{- range $key, $value := .Values.langfuse.additionalEnv }}
            - name: {{ $key }}
              value: {{ $value | quote }}
            {{- end }}

See: https://github.com/langfuse/langfuse-k8s/blob/main/charts/langfuse/templates/deployment.yaml#L96-L98

I don't want to knock this Helm chart too much because it's saving me time deploying to Kubernetes and I really appreciate that. Through installing this though, it seems many of the common conventions in Helm charts, and doesn't have sane defaults or example configurations. You basically end up having to extract the zip and reverse engineer via the Helm chart's code what the values are expect (eg: inside of an ingress) which to me is not a successful or well-written Helm chart. This chart needs a lot of love to get to a good/better place, if I have lots of free time and if I end up using this extensively, I'll likely invest some time fixing up this chart to get us collectively to a happier/easier place.

@AndrewFarley
Copy link
Author

AndrewFarley commented Sep 26, 2024

Ahh nevermind I must have an old version of the chart... the latest on main has the fix...

            {{- if .Values.langfuse.additionalEnv }}
              {{- toYaml .Values.langfuse.additionalEnv | nindent 12 }}
            {{- end }}

Well, in that case you may need to re-release/update your Helm chart in the repo so people can get this fix?

@AndrewFarley
Copy link
Author

AndrewFarley commented Sep 26, 2024

Nevermind even the new code gives a different error with that example, it improperly formats the variables. :(

            - name: 0
              value: "map[name:DATABASE_URL valueFrom:map[secretKeyRef:map[key:database_uri name:langfuse-db]]]"
            - name: 1
              value: "map[name:DATABASE_USERNAME valueFrom:map[secretKeyRef:map[key:username name:langfuse-db]]]"
            - name: 2
              value: "map[name:DATABASE_PASSWORD valueFrom:map[secretKeyRef:map[key:password name:langfuse-db]]]"
            - name: 3
              value: "map[name:LANGFUSE_LOG_LEVEL value:info]"
            - name: 4
              value: "map[name:LANGFUSE_LOG_FORMAT value:json]"

@AndrewFarley AndrewFarley reopened this Sep 26, 2024
@Steffen911
Copy link
Contributor

@AndrewFarley We released a couple fixes for the additionalEnv with version 0.4.0. Could you validate whether you still observe the same behaviour with the new chart?

@otanasiichuk-altera
Copy link

I have langfuse installed using version 0.3.0

Tried to check my installation with latest version and received an error:
helm diff upgrade dev-langfuse langfuse/langfuse \

-f helm/langfuse.yaml  \
--namespace devspace  \
--allow-unreleased

_Error: Failed to render chart: exit status 1: coalesce.go:289: warning: destination for langfuse.langfuse.additionalEnv is a table. Ignoring non-table value ([])
coalesce.go:289: warning: destination for langfuse.langfuse.additionalEnv is a table. Ignoring non-table value ([])
Error: YAML parse error on langfuse/templates/deployment.yaml: error converting YAML to JSON: yaml: line 64: did not find expected '-' indicator

Use --debug flag to render out invalid YAML_

Version 0.3.0 looks fine

@marcklingen
Copy link
Member

@Steffen911 did the behavior change of additionalEnv?

@Steffen911
Copy link
Contributor

@marcklingen @otanasiichuk-altera No, it should support everything we've had before, but handles non-string values correctly now. So it should be fully backwards compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants