litellm icon indicating copy to clipboard operation
litellm copied to clipboard

chore(helm-chart): use default environment variable for master key

Open debdutdeb opened this issue 1 year ago • 5 comments

Sometimes these manifests are used as a reference to some documentation. Idea is to use a the default environment variable instead of something specific and different baked into just the chart.

It may be that somebody doesn't want to put key in values file. They can set the key something like

master_key: os.environ/MY_KEY

Then have a secret referenced with the key

type: Opaque
data:
  MY_KEY: xxxxxx

debdutdeb avatar Mar 10 '24 09:03 debdutdeb

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 14, 2024 5:23pm
litellm-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 14, 2024 5:23pm

vercel[bot] avatar Mar 10 '24 09:03 vercel[bot]

This makes sense. Or even just bypass the file as an option. Some logic like:

  1. is the master key present in the file as env var reference or value?
  2. The env var "MASTER_KEY" is set?

Otherwise fallback to disable master key security.

GuillermoBlasco avatar Mar 10 '24 13:03 GuillermoBlasco

good to merge @debdutdeb @GuillermoBlasco ?

ishaan-jaff avatar Mar 11 '24 13:03 ishaan-jaff

@GuillermoBlasco you're right .. we should put an {{- if .masterKey }} before.

So if there is the value in Values file, we set LITELLM_MASTER_KEY.

If it's in config like os.environ/SOMETHING_ELSE , in theory user would need to add the secret mnaully anyway since how'd we know what the value is? whether it's a literal or a valueFrom.

And if somebody put it inside the config as a literal, well, if Im not mistaken you're saying we do something like

{{- range lines -}}
{{- if contains . "master_key" -}}
{{- $s := split . ": " -}}
{{- $value := index $s 1 -}}
{{- if contains $value "os.environ" | not -}}
{{/* use $value for a secret then mount, maybe delete the line from config? */}}
...

If that's the case, IMO we let them do it as is? 🙈 this is the kind of helm magic that in my experience creates more pain when multiple team members are considered, and honestly there are better ways to handle it within chart wth envFrom, and whomever deploying should take care of that themselves?

IDK if you folks think we should do it that way, I can add those couple lines in :)

Sorry @ishaan-jaff did not receive the notification first time. Please let's wait for Gui's input.

debdutdeb avatar Mar 11 '24 14:03 debdutdeb

what're next steps on this? @debdutdeb @GuillermoBlasco @ishaan-jaff

krrishdholakia avatar Mar 17 '24 02:03 krrishdholakia

@ishaan-jaff this should be ready to merge :)

debdutdeb avatar Apr 14 '24 17:04 debdutdeb

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Apr 23 '25 00:04 github-actions[bot]

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Apr 23 '25 00:04 CLAassistant

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Jul 23 '25 00:07 github-actions[bot]