helm-diff icon indicating copy to clipboard operation
helm-diff copied to clipboard

Input stringData gets stripped completely

Open danopia opened this issue 3 years ago • 6 comments

It appears as though the redactSecrets method is using stringData as a behind-the-scenes crutch to print a redacted data diff: https://github.com/databus23/helm-diff/blob/818e596fa6e9cb928985043c1b0a96b9b4a02299/diff/diff.go#L94-L129

The issue is that if the chart being diffed uses stringData then all those fields are completely hidden from the diff output. For example, this template:

---
apiVersion: v1
kind: Secret
metadata:
  name: {{ $config.name }}
type: Opaque
stringData:
  kongCredType: acl
  group: inbound-traffic
  otherField: |
    Lorem ipsum dolor sit amet, consectetur adipiscing elit.
    Sed sed felis id ex ultricies tempor.

shows as this in helm-diff:

my-namespace, my-name, Secret (v1) has been added:
+ # Source: secret.yaml
+ apiVersion: v1
+ kind: Secret
+ metadata:
+   name: my-name
+ type: Opaque

This effectively defeats the diffing for any secret values that do not need to be redacted.

danopia avatar Aug 23 '21 12:08 danopia

I see. Thanks for reporting this. So I think what should happen here is that we need to consider that stringData might not be empty. If its not empty we need to redact the fields in place and only add fields from data that are not already present in stringData. Would you agree?

databus23 avatar Aug 25 '21 10:08 databus23

In my opinion, I don't actually care about redaction for stringData fields; it's basically a statement that something else expects the data in a Secret even though it's human readable plaintext and I don't want to deal with obfuscation.

Disregarding my opinion, your plan looks sound :) The distinction between stringData and data would get lost in the diff, which sounds reasonable because the distinction also gets lost once in the cluster.

I'd be happy either way (with or without stringData redaction) as long as the diff stops incorrectly hiding entire fields.

danopia avatar Aug 25 '21 14:08 danopia

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.

stale[bot] avatar Jan 08 '22 23:01 stale[bot]

I also have a similar problem with stringData. I have a chart that only deploy a secret containing a stringData When I changed the content of the stringData the diff output no modification instead of saying that the secret is modify, meaning that if I do a helmfile apply nothing is done. The workaround I found is to add an annotation to my secret and change the value when I change the data.

To reproduce:

  1. Deploy a secret containing stringData with the chart incubator/raw
  2. Modify the secret stringData
  3. Execute helm diff

Nothing is displayed.

guillomep avatar Feb 09 '22 10:02 guillomep

experiencing this as well with stringData secrets

ccodreanu avatar Jun 03 '22 13:06 ccodreanu

Hey, guys! I had the same problem, to work around it, I added this to my template file:

---
apiVersion: v1
kind: Secret
metadata:
  name: {{ include "alertmanager.config.secret" . }}
  labels:
    {{ include "alertmanager.labels" . | indent 4 | trim }}
    release: {{ .Release.Name }}
  annotations:
    rollme: {{ randAlphaNum 5 | quote }}

mayconritzmann avatar Aug 04 '22 22:08 mayconritzmann

In my opinion, I don't actually care about redaction for stringData fields; it's basically a statement that something else expects the data in a Secret even though it's human readable plaintext and I don't want to deal with obfuscation.

Counter-point: After running into this issue in our own Helm toolchain, I surveyed our Helm charts for usage of stringData and found a fair amount of secrets that use stringData for actual credentials. My guess is that it's just more convenient and/or more obvious for chart authors to write:

stringData:
  foo_password: "{{ .Values.foo_service.password }}"
  bar_token: "{{ .Values.bar_service.token }}"

instead of:

data:
  foo_password: "{{ .Values.foo_service.password | b64enc }}"
  bar_token: "{{ .Values.bar_service.token | b64enc }}"

majewsky avatar Sep 21 '22 14:09 majewsky