helm / helm

The Kubernetes Package Manager
https://helm.sh
Apache License 2.0
27.06k stars 7.12k forks source link

helm ignores alias property on a chart dependency, if Chart.lock does not match chart directory #11936

Open jkroepke opened 1 year ago

jkroepke commented 1 year ago

Output of helm version:

version.BuildInfo{Version:"v3.11.2", GitCommit:"912ebc1cd10d38d340f048efaf0abda047c3468e", GitTreeState:"clean", GoVersion:"go1.20.2"}

On some local machines we identity the issue that some helm dependencies are completely ignoring helm values. After some investigation, I could identity that the problem appears, after some dependencies update maintenance tasks.

Looking deeper into it, I could identity that only charts with the alias property are affected.

In our setup, we have the (sub-)charts directory always on .gitignore to prevent that .tar.gz in out git repository. In conclusion, if someone changes the dependency on the Chart.yaml runs helm dep up on local system and check in, the problem will occurs on all systems after a git pull. (Example Commit: https://github.com/jkroepke/homelab/commit/31a5edef38f5776b7fb50cd247f9c6c591002bf8)

If the Chart.lock and the charts/ directory of a helm chart are out of sync, then helm will ignore the alias property entirely which prevents that values are pass from the umbrella chart to the sub chart.

A potential workaround is running helm dep up.

How to reproduce this?

git clone https://github.com/jkroepke/homelab.git

# Checkout a git state, before maintenance
% git checkout bcc28ead96c32c83a013ddffce23f4c3785eccd8 
% cd helm-chart-examples/helm-dependendency-issue
% helm dep up
% helm template .
---
# Source: helm-dependency-issue/charts/yaml/templates/values.yaml
apiVersion: v1
data:
  key: value
global: {}
kind: ConfigMap
metadata:
  name: test

# Checkout next git commit after to simulate that someone from team to maintaince tasks
% git checkout 31a5edef38f5776b7fb50cd247f9c6c591002bf8
% helm template .
---
# Source: helm-dependency-issue/charts/values/templates/values.yaml
global: {}

# Workaround: Running helm dep up again.
% helm dep up
% helm template .
---
# Source: helm-dependency-issue/charts/yaml/templates/values.yaml
apiVersion: v1
data:
  key: value
global: {}
kind: ConfigMap
metadata:
  name: test

Proposed fix

If there is a Chart.lock file in helm directory and the Chart.lock is not in sync with the charts/ directory, helm should refuse commands like helm template and helm upgrade. Helm does this already, if the charts is empty:

% rm -rf charts/*
% helm template .
Error: An error occurred while checking for chart dependencies. You may need to run `helm dependency build` to fetch missing dependencies: found in Chart.yaml, but missing in charts/ directory: values

Helm checks, if the sub chart is present on the charts directory but it does not validate, if the specific version if present in the charts directory. If the version specific helm chart version different from the Chart.lock file, the alias property gets ignored.

joejulian commented 1 year ago

I'm pretty sure this is mostly a duplicate of https://github.com/helm/helm/issues/11876

https://helm.sh/docs/topics/charts/#managing-dependencies-manually-via-the-charts-directory provides a method for including charts outside of the Chart.yaml/Chart.lock control. This is intentional.

jkroepke commented 1 year ago

But does it make sense? It's a different behavior compared to all known package mangers?

I would not expect that the manual dependency management hits here, since there is a Chart.lock which opt-in into automatic dependency management.

jkroepke commented 1 year ago

@joejulian After-rethink about this. There is a conflict.

I re-read https://helm.sh/docs/topics/charts/#managing-dependencies-manually-via-the-charts-directory which documents the manual dependency management. And do some local tests.

Manually dependency must be extracted otherwise, the helm dep command would remove them. In conclusion, packaged dependency should never considered as manually managed dependency, since helm dep build and helm dep up would remove them otherwise.

The documented describes that manually dependencies should be extracted which means that dependency which packaged as tar.gz should be never assumed as manually managed dependency. helm dep build and helm dep up behaving like this. They assume, any tar.gz in the charts directory coming form earlier helm dep up run.

From my point of view, it is a bug that helm is loading outdated .tar.gz packaged dependency. Helm is able to validate, if the .tar.gz does not match the Chart.lock file, since a packaged dependency should never considered as manual managed dependency.

joejulian commented 1 year ago

You make a fair argument but the fact is, it's always worked this way and it's possible that with the millions of users out there, some may be using this to their advantage.

If you'd like to offer a PR to change this behavior, it would need to be governed by a helm improvement proposal (HIP) and guarded by a flag to prevent breaking existing use cases.

jkroepke commented 1 year ago

I may ask, why a bug fix need a HIP? Provisusly #11876 is mentioned as duplicate and marked as bug. Now, a bug is mentioned as proposal?

would need to be governed by a helm improvement proposal

Well, no one will pass the very slow process of the HIP for a bugfix.

jkroepke commented 1 year ago

Hi @joejulian

I would like ask you again, if you consider this as bug. I recently figure out that dependencies.import-values works fine, even the version constrain does not met. This issue is wrongly marked as proposal

If dependencies.import-values works, dependencies.alias should works fine.

dependencies:
  - name: kube-prometheus-stack
    alias: prom-stack
    condition: prom-stack.enabled
    # https://artifacthub.io/packages/helm/prometheus-community/kube-prometheus-stack
    version: 45.7.1
    repository: https://prometheus-community.github.io/helm-charts
    import-values:
      - child: alertmanager
        parent: dependencyCheck.kubePrometheusStack

I'm very unhappy with the current situation here.

github-actions[bot] commented 1 year ago

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

jkroepke commented 1 year ago

Still a bug

github-actions[bot] commented 1 year ago

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

applejag commented 1 year ago

Still a bug

github-actions[bot] commented 9 months ago

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

jkroepke commented 9 months ago

Relevant

MrBuddyCasino commented 9 months ago

I just wasted half a day on this. The chart versions didn't match the downloaded ones anymore, thus the chart alias didn't work anymore, and so values didn't get applied, and I had no clue as to why.

jkroepke commented 9 months ago

and I had no clue as to why.

Because the error behavior is 'is intentional' by helm.

github-actions[bot] commented 6 months ago

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

jkroepke commented 6 months ago

Still an issue

github-actions[bot] commented 2 months ago

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

MrBuddyCasino commented 2 months ago

bump