fix(dify): de-duplicate container env vars (fixes declarative apply /…#174
fix(dify): de-duplicate container env vars (fixes declarative apply /…#174this-is-eis wants to merge 1 commit into
Conversation
… GitOps)
The chart emits duplicate env-var names in some containers, relying on the
container runtime's "last value wins." This happens even with default values
(plugin-daemon gets two DB_DATABASE entries) and whenever a user sets a var
already present in a chart default via global.extraBackendEnvs.
A container's env is a strategic-merge list keyed by name
(patchStrategy:"merge", patchMergeKey:"name"), so duplicate keys break the
strategic-merge diff used by kubectl apply, helm upgrade, and ArgoCD:
failed to construct strategic merge patch:
The order in patch list [...] doesn't match $setElementOrder list [...]
Add a dify.dedupeEnv helper (last occurrence wins, first-occurrence order
preserved) plus dify.backendEnvs / dify.frontendEnvs wrappers that assemble
each container's env in the existing upstream order and then de-dupe. Replace
the inline env concatenation in the api, worker, worker-beat, plugin-daemon
(backend) and frontend containers; inline preambles and the sandbox container
are untouched. Bump chart version 0.10.0 -> 0.10.1.
De-dup applies the same last-wins rule the runtime already used, so the
effective environment of every container is unchanged; only the redundant list
entries are removed. Verified via before/after helm template (default values
and a values set that intentionally duplicates DB_DATABASE/CODE_MAX_STRING_LENGTH):
identical env-name set and last-wins values per container, valueFrom intact.
|
@LeoQuote Please help review this PR when you're free 🙇 . |
LeoQuote
left a comment
There was a problem hiding this comment.
感谢你的 PR, 我相信这个 PR 可以解决这个问题, 但是我目前还在考虑这个方案, 原因是这个实现中, 出现了一两层的代码嵌套, 并且有不少的复杂循环, 这在一般的编程语言中不是很大的问题, 但是在模板语言中, 可能会影响到代码的可读性和可维护性.
我目前想到的一个方案是用 dict 来替换当前的 list 实现, 比如在 values 就提供 dict, 我们在 template 中就可以直接用 mergeOverwrite 函数来进行简单的去重, 去重完成后使用一个函数将 key 转化为 name, value 转化为其他属性, 就可以完成当前的需求.
上面我说的方案需要去更改 values 的格式, 如果需要保留格式, 我们可以先将列表转化为 dict, 然后用 mergeOverwrite 将其合并, 最后将其转换回 list, 这样的方式来进行实现.
以上两个使用 mergeOverwrite 的方案都是 last-win 的方案, 只要注意优先级顺序就可以了.
Thank you for your PR. I believe this PR can address the issue, but I am still considering the approach. The reason is that in this implementation, there are one or two levels of code nesting and a number of complex loops. While this is not a major problem in general programming languages, it could affect code readability and maintainability in a template language.
One approach I am currently considering is to replace the current list implementation with a dict. For example, if we provide dicts in the values, we can directly use the mergeOverwrite function in the template to perform simple deduplication. After deduplication, we can use a function to convert keys into name and values into other attributes, thereby achieving the current requirement.
The approach I described above requires changing the format of the values. If we need to preserve the format, we can first convert the list to a dict, then use mergeOverwrite to merge them, and finally convert it back to a list. This approach can be used to achieve the goal.
Both of the above approaches using mergeOverwrite are last-win strategies. You just need to pay attention to the priority order.
Summary
The chart emits duplicate env-var names in some containers, relying on the container runtime's "last value wins." This happens even with default values — the
plugin-daemongets twoDB_DATABASEentries (one fromglobal.extraBackendEnvsplumbing /commonBackendEnvs, one frompluginDaemon.envs) — and any user who sets a var already present in a chart default (e.g.CODE_MAX_*viaglobal.extraBackendEnvs) gets duplicates too.Duplicate names are tolerated at runtime, but a container's
envis a strategic-merge list keyed byname(patchStrategy:"merge", patchMergeKey:"name"). Any tool that computes a strategic-merge diff —kubectl apply,helm upgrade(three-way merge), ArgoCD — fails on duplicate keys with:So the chart's rendered manifests are effectively incompatible with GitOps / declarative apply. A first-time
helm installworks (no diff), which is why it often goes unnoticed.fix #115
fix #173
Changes
dify.dedupeEnvhelper (last occurrence wins, first-occurrence order preserved) intemplates/_helpers.tpl.dify.backendEnvsanddify.frontendEnvs, that assemble each container's env in the existing upstream order (commonEnvs→commonBackendEnvs→global.extraEnvs→global.extraBackendEnvs→ componentenvs) and then de-dupe.templates/deployment.yamlforapi,worker,worker-beat,plugin-daemon(allbackendEnvs) andfrontend(frontendEnvs). Inline preambles (- name: MODE; the plugin-daemon'sDIFY_INNER_API_URL/PLUGIN_WORKING_PATH/SERVER_KEY/DIFY_INNER_API_KEYblock) stay inline — their names don't collide with the tail. Thesandboxcontainer has nocommonEnvsand is untouched.0.10.0→0.10.1.Backward compatibility / behavior
De-dup applies the same last-wins rule the runtime already used, so the effective environment of every container is unchanged — it only removes the duplicate list entries. Verified against a before/after
helm template(default values and a values set that intentionally duplicatesDB_DATABASE/CODE_MAX_STRING_LENGTH): identical env-name set per container, each surviving var keeps its last-wins value,valueFromentries intact, no var dropped. The only externally visible change is that the rendered manifests are now well-formed (no duplicate keys), sokubectl apply/helm upgrade/ ArgoCD can diff them.Requires Helm's
fromYamlArray(Helm 3.x) — already the chart's minimum.Testing
envhas no duplicatename.master— identical for both the default values and the duplicating values set.