feat: add extraEnv configuration for additional environment variables #221
Conversation
|
|
There was a problem hiding this comment.
I believe this serves the same purpose as the extraEnvVars property no?
For the other PR mentioned, the benefit is mostly around a secret value being kept hidden from the users, which is a good practice.
I am personally not a fan of the extra<Thing> properties as they tend to potentially abstract too many variables away, I'd rather have something properly defined in the schema to reduce the chances of users errors.
If you have some specific use case that makes this a good addition to the extraEnvVars could you add it to the description so that we can take it into account?
For instance if you are blocked on a deployment because you're missing something that this would fix, maybe we can figure out if this is indeed needed or if we have another way to accomplish it?
Yes, but
I need exactly what other PR provides: |
|
BTW, I still believe this PR is better solution. This PR uses: {{- tpl . $ | nindent 12 }}while {{- toYaml . | nindent 12 }}difference is that with extraEnv: |
- name: OPENFGA_AUTHN_PRESHARED_KEYS
valueFrom:
secretKeyRef:
name: {{ .Release.Name }}-authn
key: preshared_keyscurrently with #212 merged I'm forced to use |
mkyc
force-pushed
the
elastic-env-var-provisioning
branch
from
013cd83 to
bd1049c
Compare
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes
participant OpenFGA Deployment
User->>Helm: Set extraEnv in values.yaml
Helm->>Helm: Process deployment.yaml template with tpl(extraEnv)
Helm->>Kubernetes: Deploy OpenFGA with extra environment variables
Kubernetes->>OpenFGA Deployment: Start containers with injected env vars
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
charts/openfga/values.yaml (1)
372-389: Clarify & harden the newextraEnvinterface
extraEnvis defined as a free-form string that istpl-rendered and inlined verbatim. While flexible, it:
- Places the burden of producing valid YAML (correct indentation, list syntax, etc.) on end-users—one malformed space will break the render.
- Can silently create duplicate
name:keys when combined withextraEnvVars, causing the Deployment to be rejected by the Kubernetes API (spec.template.spec.containers[*].env.name: Duplicate value).Consider one or more of the following refinements:
- extraEnv: "" + # extraEnvTpl: "" # keep the name self-describing + # extraEnvVarsTpl: [] # mirror existing array option but evaluated via tplor introduce simple validation logic (e.g., a
requiredUniqueEnvboolean that errors on duplicates), and highlight these caveats in the chart’s README.charts/openfga/values.schema.json (1)
1149-1153: Schema could convey “YAML blob” intentDeclaring the field as just
type: "string"gives consumers (e.g., editors with JSON-schema support) zero hints about the expected payload.
You can take advantage of the optionalcontentMediaTypeto signal that the string holds YAML:{ "type": "string", "description": "Extra environment variables … (evaluated with Helm tpl)", "contentMediaType": "text/x-yaml", "default": "" }Small win for editor UX; no effect on chart runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
charts/openfga/templates/deployment.yaml(2 hunks)charts/openfga/values.schema.json(1 hunks)charts/openfga/values.yaml(1 hunks)
| {{- with .Values.extraEnvVars }} | ||
| {{- toYaml . | nindent 12 }} | ||
| {{- end }} | ||
| {{- with .Values.extraEnv }} | ||
| {{- tpl . $ | nindent 12 }} | ||
| {{- end }} | ||
| resources: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
extraEnv injection works, but duplicate-name collision is still possible
The two blocks correctly use tpl + nindent, 👍.
However, nothing prevents a user from declaring the same env name in:
extraEnvVarsextraEnv(init container)extraEnv(main container)
Kubernetes will reject the resulting manifest. A defensive guard can avoid surprising failures:
{{- $seen := dict -}}
{{- range $idx, $env := append .Values.extraEnvVars (tpl .Values.extraEnv $ | fromYaml) }}
{{- if hasKey $seen $env.name }}
{{- fail (printf "duplicate env var '%s' detected in extraEnv/extraEnvVars" $env.name) }}
{{- end }}
{{- $_ := set $seen $env.name true }}
{{- end }}
Even a simple Helm required/fail with hasKey suffices.
Also applies to: 390-393
🤖 Prompt for AI Agents
In charts/openfga/templates/deployment.yaml around lines 70 to 76, there is a
risk of duplicate environment variable names between extraEnvVars and extraEnv
blocks, which causes Kubernetes manifest rejection. To fix this, add a guard
using a dictionary to track seen environment variable names before rendering
them. Iterate over the combined list of extraEnvVars and the parsed extraEnv,
check if a name already exists in the dictionary, and if so, use the fail
function to stop rendering with a clear duplicate name error. Apply the same
duplicate check logic also around lines 390 to 393 where similar environment
variables are injected.
2 participants
Description
as much as I like PR #212 I think it doesn't solve more general problem which is "there is so many options people need to provide secrets".
Review Checklist
mainhelm template -n test-01 openfga .with example configuration uncommented and I got expected result:Summary by CodeRabbit
New Features
Documentation