Issue with helm v3 and `--reset-values`

[andrew-kurin]

I am using Helm v3.0.0-beta.5 and helm-diff 3.0.0-rc.2, and it appears that when I'm running helm diff upgrade --reset-values it completely ignores variable files that I'm passing to it. See for comparison.

helm diff upgrade --reset-values ingx stable/nginx-ingress -f ../../../helmfiles/nginx-ingress/nginx-ingress-values.yaml --set controller.service.loadBalancerIP=35.223.250.53 --detailed-exitcode
default, ingx-nginx-ingress-controller, HorizontalPodAutoscaler (autoscaling) has been removed:
- # Source: nginx-ingress/templates/controller-hpa.yaml
- apiVersion: autoscaling/v2beta1
- kind: HorizontalPodAutoscaler
- metadata:
-   labels:
-     app: nginx-ingress
-     chart: nginx-ingress-1.24.4
-     component: "controller"
-     heritage: Helm
-     release: ingx
-   name: ingx-nginx-ingress-controller
- spec:
-   scaleTargetRef:
-     apiVersion:
-     kind: Deployment
-     name: ingx-nginx-ingress-controller
-   minReplicas: 2
-   maxReplicas: 100
-   metrics:
-     - type: Resource
-       resource:
-         name: cpu
-         targetAverageUtilization: 70
-     - type: Resource
-       resource:
-         name: memory
-         targetAverageUtilization: 70
+
default, ingx-nginx-ingress, ServiceAccount (v1) has been removed:
- # Source: nginx-ingress/templates/controller-serviceaccount.yaml
- apiVersion: v1
- kind: ServiceAccount
- metadata:
-   labels:
-     app: nginx-ingress
-     chart: nginx-ingress-1.24.4
-     heritage: Helm
-     release: ingx
-   name: ingx-nginx-ingress
+
default, ingx-nginx-ingress-controller, Deployment (apps) has changed:
  # Source: nginx-ingress/templates/controller-deployment.yaml
  apiVersion: apps/v1
  kind: Deployment
  metadata:
    labels:
      app: nginx-ingress
      chart: nginx-ingress-1.24.4
      component: "controller"
      heritage: Helm
      release: ingx
    name: ingx-nginx-ingress-controller
  spec:
    selector:
      matchLabels:
        app: nginx-ingress
        release: ingx
    replicas: 1
    revisionHistoryLimit: 10
    strategy:
      {}
    minReadySeconds: 0
    template:
      metadata:
        labels:
          app: nginx-ingress
          component: "controller"
          release: ingx
      spec:
        dnsPolicy: ClusterFirst
        containers:
          - name: nginx-ingress-controller
            image: "quay.io/kubernetes-ingress-controller/nginx-ingress-controller:0.26.1"
            imagePullPolicy: "IfNotPresent"
            args:
              - /nginx-ingress-controller
              - --default-backend-service=default/ingx-nginx-ingress-default-backend
              - --election-id=ingress-controller-leader
              - --ingress-class=nginx
              - --configmap=default/ingx-nginx-ingress-controller
            securityContext:
              capabilities:
                  drop:
                  - ALL
                  add:
                  - NET_BIND_SERVICE
              runAsUser: 33
              allowPrivilegeEscalation: true
            env:
              - name: POD_NAME
                valueFrom:
                  fieldRef:
                    fieldPath: metadata.name
              - name: POD_NAMESPACE
                valueFrom:
                  fieldRef:
                    fieldPath: metadata.namespace
            livenessProbe:
              httpGet:
                path: /healthz
                port: 10254
                scheme: HTTP
              initialDelaySeconds: 10
              periodSeconds: 10
              timeoutSeconds: 1
              successThreshold: 1
              failureThreshold: 3
            ports:
              - name: http
                containerPort: 80
                protocol: TCP
              - name: https
                containerPort: 443
                protocol: TCP
-             - name: metrics
-               containerPort: 10254
-               protocol: TCP
            readinessProbe:
              httpGet:
                path: /healthz
                port: 10254
                scheme: HTTP
              initialDelaySeconds: 10
              periodSeconds: 10
              timeoutSeconds: 1
              successThreshold: 1
              failureThreshold: 3
            resources:
              {}
        hostNetwork: false
        serviceAccountName: ingx-nginx-ingress
        terminationGracePeriodSeconds: 60
default, ingx-nginx-ingress-controller-metrics, Service (v1) has been removed:
- # Source: nginx-ingress/templates/controller-metrics-service.yaml
- apiVersion: v1
- kind: Service
- metadata:
-   labels:
-     app: nginx-ingress
-     chart: nginx-ingress-1.24.4
-     component: "controller"
-     heritage: Helm
-     release: ingx
-   name: ingx-nginx-ingress-controller-metrics
- spec:
-   ports:
-     - name: metrics
-       port: 9913
-       targetPort: metrics
-   selector:
-     app: nginx-ingress
-     component: "controller"
-     release: ingx
-   type: "ClusterIP"
+
default, ingx-nginx-ingress-controller, Service (v1) has changed:
  # Source: nginx-ingress/templates/controller-service.yaml
  apiVersion: v1
  kind: Service
  metadata:
    labels:
      app: nginx-ingress
      chart: nginx-ingress-1.24.4
      component: "controller"
      heritage: Helm
      release: ingx
    name: ingx-nginx-ingress-controller
  spec:
-   loadBalancerIP: "35.223.250.53"
-   externalTrafficPolicy: "Local"
+   clusterIP: ""
    ports:
      - name: http
        port: 80
        protocol: TCP
        targetPort: http
      - name: https
        port: 443
        protocol: TCP
        targetPort: https
    selector:
      app: nginx-ingress
      component: "controller"
      release: ingx
    type: "LoadBalancer"
default, ingx-nginx-ingress-default-backend, Service (v1) has changed:
  # Source: nginx-ingress/templates/default-backend-service.yaml
  apiVersion: v1
  kind: Service
  metadata:
    labels:
      app: nginx-ingress
      chart: nginx-ingress-1.24.4
      component: "default-backend"
      heritage: Helm
      release: ingx
    name: ingx-nginx-ingress-default-backend
  spec:
+   clusterIP: ""
    ports:
      - name: http
        port: 80
        protocol: TCP
        targetPort: http
    selector:
      app: nginx-ingress
      component: "default-backend"
      release: ingx
    type: "ClusterIP"
Error: identified at least one change, exiting with non-zero exit code (detailed-exitcode parameter enabled)
identified at least one change, exiting with non-zero exit code (detailed-exitcode parameter enabled)
Error: plugin "diff" exited with error

It is the same result when I'm not passing any values. Meanwhile, if I run the same command without --reset-values, it will give me empty answer and 0 exit code.
In addition, exit code returned is 1, while exit code 2 is expected.
It prevents helmfile diff from functioning properly roboll/helmfile#877

[andrew-kurin]

It seems like here https://github.com/databus23/helm-diff/blob/master/cmd/helm3.go#L74, the other value flags are only passed through if --reset-values flag is not set

[mumoshu]

@zen4ever Ah, good catch! Assuming there was no breaking change made on --reset-values, it shouldn't be mutually exclusive with other flags 🤔 Let's try removing that if block.

cc/ @jonasrutishauser

[mumoshu]

Maybe a more correct behaviour would be to use the current values only when d.reuseValues && !d.resetValues and NOT ignore other flags like -set, --set-string, --values, --set-file.

[mumoshu]

Also, shouldn't we enable --reuse-values automatically when anynone of --set-, --set-string, --values, --set-file is specified?
https://medium.com/@kcatstack/understand-helm-upgrade-flags-reset-values-reuse-values-6e58ac8f127e

[jonasrutishauser]

@mumoshu you are right, the current implementation is wrong.
The if around reuse values should be !d.resetValues && (d.reuseValues || !<any --set, --set-string, --value, --set-file specified>) and no if around the other flags.

Additionally i just saw that the real call to get the old values (existingValues) is missing in the code.
This should be added too.

I think i made this wrong when i changed the implementation to use all values (old chart and old given) in --reuseValues.

[mumoshu]

@jonasrutishauser Thanks for confirming! Let me submit a pull request for that tonight.

[mumoshu]

I've just submitted #159 for this