Failure to honor lookup function

[jglick]

Unpack diffdemo.zip whose interesting part is

apiVersion: v1
kind: Secret
metadata:
  name: x
type: Opaque
data:
{{- $existing := (lookup "v1" "Secret" .Release.Namespace "x" ) }}
{{- if $existing }}
  text: {{ index $existing.data "text" }}
{{- else }}
  text: {{ (randAlpha 10) | b64enc}}
{{- end }}

and then run

$ helm install test .
NAME: test
LAST DEPLOYED: Mon Mar  1 09:13:24 2021
NAMESPACE: difftest
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
kubectl get secret --namespace difftest x -o go-template="{{.data.text | base64decode}}{{\"\n\"}}"
$ kubectl get secret --namespace difftest x -o go-template="{{.data.text | base64decode}}{{\"\n\"}}"
eLbQJJESrO
$ helm upgrade test .
Release "test" has been upgraded. Happy Helming!
NAME: test
LAST DEPLOYED: Mon Mar  1 09:13:37 2021
NAMESPACE: difftest
STATUS: deployed
REVISION: 2
TEST SUITE: None
NOTES:
kubectl get secret --namespace difftest x -o go-template="{{.data.text | base64decode}}{{\"\n\"}}"
$ kubectl get secret --namespace difftest x -o go-template="{{.data.text | base64decode}}{{\"\n\"}}"
eLbQJJESrO
$ helm diff upgrade test .
difftest, x, Secret (v1) has changed:
  # Source: diffdemo/templates/secret.yaml
  apiVersion: v1
  kind: Secret
  metadata:
    name: x
  data:
-   text: '-------- # (10 bytes)'
+   text: '++++++++ # (10 bytes)'
  type: Opaque

helm diff upgrade claims that secret/x would change if helm upgrade were run, when in fact it would not. See #176 and https://github.com/helm/charts/issues/5167#issuecomment-641558251.

(Helm 3.5.2, Microk8s 1.20.2, kubectl 1.20.4, helm-diff freshly installed so I suppose 3.1.3.)

[jglick]

https://github.com/bacongobbler/helm-patchdiff does not seem to have this problem by the way.

[unfor19]

@jglick - Have you solved this one? I have the exact same case, though I'm using .Release.IsInstall flag to make sure the secrets are generated only once, but the results are still the same as yours. Seems like lookup function is not working.

[jglick]

@unfor19 no I have no particular workaround other than to tell people looking at the diff to please ignore apparent Secret changes.

[unfor19]

Argh 😞 I intend to use it as part of a pipeline, so it's impossible to tell the pipeline "just ignore it" since I do want to take it into consideration.

Btw, if you want to suppress messages about secrets, you can add the following flag

$ helm diff --help
...
-q, --suppress-secrets             suppress secrets in the output

Though it still means you'll get

$ helm diff upgrade my-app . --values values.yaml --suppress-secrets

devops-sandbox, my-app-mysql-secret, Secret (v1) has changed:
+ Changes suppressed on sensitive content of type Secret

Update- I've just checked and the lookup function returns an empty map[], and it appears to be the default behavior according to the docs

Keep in mind that Helm is not supposed to contact the Kubernetes API Server during a helm template or a helm install|update|delete|rollback --dry-run, so the lookup function will return an empty list (i.e. dict) in such a case.

I guess helm diff is no different than helm template, it simply cannot execute the lookup function. Adding the --show-secrets flag to helm diff shows the correct values of the existing secrets, so it means the lookup function works but the diff is wrong, so maybe it's doable.

[jglick]

--suppress-secrets does not seem like a safe workaround, as other Secrets not defined via lookup may actually have changed, or their metadata might have changed, etc.

I guess helm diff is no different than helm template, it simply cannot execute the lookup function.

I do not think so. helm template runs offline. helm diff upgrade, like helm upgrade, contacts the cluster (unless you pass --dry-run).

Helm is not supposed to contact the Kubernetes API Server during a helm template or a helm install|update|delete|rollback --dry-run

Not actually true, by the way: helm/helm#6901 (comment)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jglick]

I do not think this should be considered stale as it is an ongoing problem with no known workaround.

[yelhouti]

Facing the same problem here, while using helmfile which uses helm diff under the hood.

[rickardp]

Are there any plans to actually fix this? I didn't take it if you thought it went agains the design goals or if this is to be treated as a bug?
I suppose this could be viewed as blocked by helm/helm#8137? What is the opionion of the maintainers here?

[databus23]

I suppose this could be viewed as blocked by helm/helm#8137? What is the opionion of the maintainers here?

Exactly, while I would love to have support for the lookup function there is no support for that in the upstream helm binary.
I'm super hesitant to reimplement the template render logic in this plugin because then this needs to be kept up-to-date with template functions provided by newer helm versions. In fact this plugin would have to release matching versions for each version of helm that has a change in its "template language". Also using newer versions of this plugin with older helm versions would lead to inaccurate results as functions supported by the diff plugin would not be when performing the upgrade (using the helm binary).
It was a conscious decision to not "vendor" the rendering of the template into this plugin when adding support for helm3 when everything went client-side: #149 (comment).

So as long as there is no support for extracting the correctly rendered output from the helm binary I don't see how to make progress on this issue.

[rickardp]

That makes sense. Let's all help out getting the dry-run fix merged then, looks like a PR has been published but not yet reviewed.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mumoshu]

We now have the ability to use helm upgrade --dry-run instead of helm template to render the chart (#330). And upgrade --dry-run seems to have a chance to get cluster access(even though it doesn't look exactly like what I'd expect for a dry-run operation...

So, this should be possible with HELM_DIFF_USE_UPGARDE_DRY_RUN=true once helm/helm#9426 gets merged upstream.

[mumoshu]

Exactly, while I would love to have support for the lookup function there is no support for that in the upstream helm binary.
I'm super hesitant to reimplement the template render logic in this plugin because then this needs to be kept up-to-date with template functions provided by newer helm versions.

@databus23 I agere and support this. And I have a slightly related discussion to bring in here.

In #304, we did introduce dependencies to helm and k8s.io related packages in our three-way merge work. This requires us to keep those dependencies up-to-date. We rely on those dependencies only for converting YAML doc to K8s resource objects and doing three-wary-merging, but template rendering.

The part that converts YAML doc to K8s resource objects won't change that often as long as you mostly use stable K8s APIs(apps/v1, ingress/v1, core/v1, etc). Probably it's similar for the three-way merge logic, too. So I think it's fine to introduce helm and k8s deps.

But the template renderer changes often in Helm v3, when e.g. a new helm version gets new template functions. So reimplementing the renderer in helm-diff would be a maintenance nightmare.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[scubbo]

Commenting to make this not-stale (I came here from here - if there's a better place to track this, please let me know!) - I'm still encountering this issue on Helm commit 414ff28d4029ae8c8b05d62aa06c7fe3dee2bc58, and it looks like the PR has still not been merged.

EDIT: Oh, it's closed, which appears to be irreversible. I'm still getting used to Github Issues, sorry!

[MichaelMorrisEst]

FYI, I have created a new issue to progress this and plan to submit a PR once the helm PR is merged
#449