Some charts having diffs where there are no changes

[mahgo]

I'm having a problem with some charts when calling helm diff upgrade; it is detecting changes but when viewing the outputted diff, it appears to be the same. For example:

default, foo-bar-service, Service (v1) has changed:
  # Source: foo-bar/templates/service.yaml
- apiVersion: v1
- kind: Service
- metadata:
-   name: foo-bar-service
- spec:
-   type: NodePort
-   selector:
-     app: foo-bar-frontend
-   ports:
-   - protocol: TCP
-     port: 80
+ apiVersion: v1
+ kind: Service
+ metadata:
+   name: foo-bar-service
+ spec:
+   type: NodePort
+   selector:
+     app: foo-bar-frontend
+   ports:
+   - protocol: TCP
+     port: 80
      targetPort:

[mahgo]

This seems to only occur when the previous upgrade on a chart was from a different computer.

[jonerer]

Could it be a difference in newlines? Like windows vs linux \r\n and \n? If you could check the content of the secrets, that would be interesting

[mahgo]

Using --normalize-manifests (not yet in a release) seems to fix this issue. But is this something that you should need to use a parameter on or should this be the sort of thing that is fixed by default?

[mahgo]

@jonerer I have mainly been using Linux and WSL2 Linux, which probably shouldn't be different. What would be the best way to check this?

[jonerer]

Send the output of helm template to a file and have a look at it. And then try to see what's bee written in the helm secret in your cluster. Kubectl get secrets and have a look around to find it

[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]

Could anyone buy me a Windows laptop for testing or contribute a fix? 😃

Anyway, if it's a line-ending issue, it would have something to do with the fact that we call helm get manifest RELEASE and helm template to get the current and the desired manifests to be diffed. Maybe normalize line-endings by default there ould fix it.

[mahgo]

I can test this in the next few days. I'd definitely be up for normalizing the line endings by default though. Are there any use cases for not wanting to normalize line endings?

[mumoshu]

@mahgo Thanks. The point here is that helm-diff should be normalizing line-endings already. --normalize-manifests isn't intended for normalizing line-endings only so enabling it by default isn't an option imho.

[mahgo]

@mumoshu I've done a diff on the manifest and template using git diff --no-index --word-diff=color --word-diff-regex=. manifest.txt template.txt. Here's a snippet of the output:

[mumoshu]

@mahgo Assuming ^M represents a carriage-return, does it mean that you have carriage-returns in the result only in the output of helm-template, right?

[mumoshu]

@mahgo Would you mind checking how the result looks like when you used helm upgrade --dry-run instead of helm template to generate template.txt?

[mahgo]

@mumoshu Yes they are in the template and not the manifest. And the dry-run result is the same as the template.

[mumoshu]

@mahgo Thanks- Gotcha. How did you prepare the chart? I thought any templates in a chart will get carriage-returns added if you use certain tool on Windows, which is affecting helm's rendering output.

I'm also wondering if how helm is handling carriage-returns in a fetched chart and a new chart.
What if you run helm create yourchart and see files under yourchart/templates? Does each file gets a carriage-return at the end of every line?

[mahgo]

This chart was originally created by a 3rd party.

Here's how the pdb yaml file looks via cat -e pdb.yaml

apiVersion: policy/v1^M$
kind: PodDisruptionBudget^M$
metadata:^M$
  name: {{ include "iam-app.fullname" . }}^M$
  labels:^M$
    {{- include "iam-app.labels" . | nindent 4 }}^M$
spec:^M$
  minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}^M$
  selector:^M$
    matchLabels:^M$
      app: {{ include "iam-app.fullname" . }}

This doesn't seem to exist on a new chart though (both if created in powershell or under WSL2):

{{- if .Values.ingress.enabled -}}$
{{- $fullName := include "yourchart.fullname" . -}}$
{{- $svcPort := .Values.service.port -}}$
{{- if semverCompare ">=1.14-0" .Capabilities.KubeVersion.GitVersion -}}$
apiVersion: networking.k8s.io/v1beta1$
{{- else -}}$
apiVersion: extensions/v1beta1$
{{- end }}$
kind: Ingress$
metadata:$
  name: {{ $fullName }}$
  labels:$
    {{- include "yourchart.labels" . | nindent 4 }}$
  {{- with .Values.ingress.annotations }}$
  annotations:$
    {{- toYaml . | nindent 4 }}$
  {{- end }}$
spec:$
  {{- if .Values.ingress.tls }}$
  tls:$
    {{- range .Values.ingress.tls }}$
    - hosts:$
        {{- range .hosts }}$
        - {{ . | quote }}$
        {{- end }}$
      secretName: {{ .secretName }}$
    {{- end }}$
  {{- end }}$
  rules:$
    {{- range .Values.ingress.hosts }}$
    - host: {{ .host | quote }}$
      http:$
        paths:$
          {{- range .paths }}$
          - path: {{ .path }}$
            backend:$
              serviceName: {{ $fullName }}$
              servicePort: {{ $svcPort }}$
          {{- end }}$
    {{- end }}$
  {{- end }}$

[mumoshu]

@mahgo Thanks for the response!

This chart was originally created by a 3rd party.

How did you fetched the chart exactly? helm fetch and then tar zxvf, or a totally different tool?

[mahgo]

@mumoshu Was given access to their git repository, to collaborate with them.

[mumoshu]

@mahgo Thanks. You used git clone to fetched the chart, right?

[mahgo]

@mumoshu Yes that is correct.

Should helm-diff ignore this character by default maybe? Or is there a reason not to?

[mumoshu]

@mahgo Hmmm, maybe..? Honestly, I'm not yet sure why helm-diff needs to that even though helm doesn't do it (in helm-template and helm-upgrade-dry-run)

A workaround would be to set your git config correctly, so that it won't automatically add carriage-returns.
Does it sound too terrible?

[mahgo]

@mumoshu I think it would be good for helm-diff to ignore this by default. Would be keen to hear what others think.

Looking into why git adds these carraige-returns too is probably not a bad idea as well, thanks.

[mahgo]

Maybe make --normalize-manifests default but give an option to turn it off? Is there anyone that has a use case in which using --normalize-manifests is a problem?

[mumoshu]

anyone that has a use case in which using --normalize-manifests is a problem?

@mahgo AFAIK, no. But that doesn't mean it is battle-tested to work for every use case. That's why I'm not eager to turn it on by default. Flipping a big feature flag just to resolve one issue that happens only on Windows might not be a great idea in terms of reliability.

[mahgo]

That's fair enough. What do you think about showing these hidden characters in the diff, when they are the cause of the diff line?

[mumoshu]

@mahgo Sounds like a nice-to-have from the user's perspective, but as a maintainer, I would prefer a mechanism like that to be implemented outside of helm-diff.

We use aryann/difflib for taking diff between two inputs so probably anyone could contribute to the library for the addition? 🤔

[mumoshu]

Also, we're going to enable helm-diff to toggle --normalize-manifests via an envvar. See #358.
If your issue can be rephrased to be concern about repeating --normalize-manifests for every helm-diff run, would the ability to toggle it via the envvar helps?

[jack1902]

Potentially related, but i'm using ansible to run my helm-charts which has helm diff installed for better detection of changes.

For some of the charts, it all works perfectly but for one in particular its behaving as if there is a change on every run, even when nothing on the file system has changed. To elaborate, the helm-chart consumes values which I am reading in from a file on disk using lookup('file', 'FILEPATH') to get the value of the file into helm as an example:

ansible snippet

---
# Get linkerd charts
- name: Add linkerd chart repo
  kubernetes.core.helm_repository:
    name: linkerd
    repo_url: "https://helm.linkerd.io/stable"

- name: Deploy linkerd2 chart
  kubernetes.core.helm:
    name: linkerd2
    chart_ref: linkerd/linkerd2
    chart_version: 2.11.1
    release_namespace: linkerd
    wait: true
    release_values:
      installNamespace: false
      # ca.crt
      identityTrustAnchorsPEM: "{{ lookup('file', 'ca.crt') }}"
      identity:
        issuer:
          tls:
            # issuer.crt
            crtPEM: "{{ lookup('file', 'issuer.crt') }}"
            # issuer.key
            keyPEM: "{{ lookup('file', 'issuer.key') }}"

The files being read-in are certificate files which are strings over multiple lines. No matter what I do, I constantly have ansible re-install the chart which is wholly unnecessary. Would --nomalize-manifests work in this use-case since the values are set as secrets.

Happy to attempt any debugging

[mumoshu]

@jack1902 To be extra sure what's going on- are you running ansible on Windows/WSL/cygwin/etc?

[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.