fluxcd / helm-controller

The GitOps Toolkit Helm reconciler, for declarative Helming
https://fluxcd.io
Apache License 2.0
411 stars 163 forks source link

Helm unset value seems not working #319

Open mecseid opened 3 years ago

mecseid commented 3 years ago

I wanted to upgrade my Rook installation with unset CPU limitation, but after the upgrade, the limitation still exists with the default value (500m). Am I doing something wrong, or it's maybe a bug?

Helm Controller version: v0.11.1 Chart: https://github.com/rook/rook/tree/v1.6.9/cluster/charts/rook-ceph

HelmRelease:

apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: rook-ceph
spec:
  chart:
    spec:
      chart: rook-ceph
      version: v1.6.9
      sourceRef:
        kind: HelmRepository
        name: rook
  interval: 24h
  maxHistory: 3
  releaseName: rook-ceph
  storageNamespace: rook-ceph
  targetNamespace: rook-ceph
  values:
    csi:
      enableCephfsSnapshotter: false
      enableRBDSnapshotter: false
      kubeletDirPath: /opt/rke/var/lib/kubelet
      pluginTolerations:
        - key: node.netfone.hu/environment
          operator: Exists
    pspEnable: false
    resources:
      limits:
        cpu: null
        memory: 256Mi
      requests:
        memory: 128Mi

Helm values (got with helm CLI v3.2.4):

# helm -n rook-ceph get values rook-ceph
USER-SUPPLIED VALUES:
csi:
  enableCephfsSnapshotter: false
  enableRBDSnapshotter: false
  kubeletDirPath: /opt/rke/var/lib/kubelet
  pluginTolerations:
  - key: node.netfone.hu/environment
    operator: Exists
pspEnable: false
resources:
  limits:
    memory: 256Mi
  requests:
    memory: 128Mi
hiddeco commented 3 years ago

Helm as a pretty rich history around this bug, and there is a possibility that the merging code we borrowed and/or drew inspiration from for our own merging operations wasn't patched at the time.

somtochiama commented 3 years ago

Hey @mecseid , I have been unable to reproduce this on the latest version of flux.

flux: v0.20.1
helm-controller: v0.12.1

Can you try upgrading to flux?

mecseid commented 3 years ago

Hi @SomtochiAma . Sorry for late response, I didn't have so much time to upgrade flux, until now. :) After upgraded to v0.22.1 and helm-controller to v0.12.2, I don't see any difference in helm values.

To check maybe an upgrade helps it, I pushed my chart to next patch version (rook v1.6.10), but I got the same result for helm -n rook-ceph get values rook-ceph.

Maybe am I doing something wrong?

somtochiama commented 2 years ago

Hey @mecseid, I have been able to reproduce this on my end and currently looking at a fix.

paprickar commented 2 years ago

@somtochiama was you able to look into this further?

applike-ss commented 2 years ago

also waiting for a fix of this

Xplouder commented 1 year ago

Still happening with flux version 0.38.3 and helm-controller v0.28.1. Managed to workaround this by using postRenderers with delete patches.

torbjornvatn commented 1 year ago

Still happening with flux version 0.38.3 and helm-controller v0.28.1. Managed to workaround this by using postRenderers with delete patches.

@Xplouder can you give a short example of how you did this?

@somtochiama any news on a potential fix for this? I also got bitten by a value that didn't fall back to the Chart default when I removed my override from the values list in my HelmRelease definition. So maybe my issue is slightly different from that of the original poster 🤔

Xplouder commented 1 year ago

@torbjornvatn sure, needed to do this to remove the annotations, the chart did not allow to disable them in my use case:

apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: matomo
spec:
  releaseName: matomo
  chart:
    spec:
      chart: matomo
      version: 0.2.18
      sourceRef:
        kind: HelmRepository
        name: bitnami
        namespace: flux-system
  interval: 5m0s
  postRenderers:
    - kustomize:
        patches:
          - target:
              name: matomo
              kind: Deployment
            patch: |-
              - op: remove
                path: /spec/template/metadata/annotations/prometheus.io~1port
              - op: remove
                path: /spec/template/metadata/annotations/prometheus.io~1scrape

...
sebastiangaiser commented 1 year ago

Also ran into this issue when trying to override an object like the security context:

I tried the following options:

securityContext: {}
securityContext: ~
securityContext: null

Workaround (I know that a postRenderer also works but I want to post that option here, too):

securityContext:
  fsGroup: 0
  runAsGroup: 0
  runAsNonRoot: false
  runAsUser: 0

Can we do something to get this solved? It's very hard to figure out that this a issue with the helm-controller.

sebastiangaiser commented 1 year ago

It's a bit sad to see this issue not being popular enough because the use case seams to be pretty common. @somtochiama @hiddeco can we do something to get this fixed after >2 years?

hiddeco commented 11 months ago

I suspect this will be solved in the next minor release, as Helm v3.13.x included changes to properly reset values when they have been set to null.

msd-olo commented 1 month ago

I believe you have more problems here. Currently, if you set a key under values to null it will get stripped because the CRD has preserve-unknown-fields but that still prunes unknown fields with a null value. I think you would need another way to indicate that a value needs to be replaced with a null when helm is invoked.