Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HelmRelease: CRDs of disabled subcharts get installed anyway #938

Open
1 task done
MBrouns opened this issue Apr 12, 2024 · 9 comments
Open
1 task done

HelmRelease: CRDs of disabled subcharts get installed anyway #938

MBrouns opened this issue Apr 12, 2024 · 9 comments
Labels
blocked/upstream Blocked by an upstream dependency or issue

Comments

@MBrouns
Copy link

MBrouns commented Apr 12, 2024

Describe the bug

Assume a helm chart with the following structure

.
├── Chart.yaml
├── charts
│   └── subchart
│       ├── Chart.yaml
│       ├── crds
│       │   └── crd.yaml
│       ├── templates
│       │   └── subchartpod.yaml
│       └── values.yaml
├── templates
│   └── mainchartpod.yaml
└── values.yaml

where the mainchart's Chart.yaml says:

dependencies:
  - name: subchart
    condition: subchart.enabled

When I create a helm release with the following details (specifically note that the subchart is not enabled)

apiVersion: helm.toolkit.fluxcd.io/v2beta2
kind: HelmRelease
metadata:
  name: fluxcd-test
  namespace: default
spec:
  interval: 10m
  timeout: 5m
  chart:
    spec:
      chart: mainchart
      version: "0.1.0"
      sourceRef:
        kind: GitRepository
        name: fluxcd-subchart-crd-test
      interval: 5m
  releaseName: crd-test
  values:
    replicaCount: 1
    subchart:
      enabled: false

I still see the crd from the subchart created in the cluster. This is only the case for the crd in the subchart, not the resources defined in the subchartpod.yaml. This is not what I expect, and also not what happens when installing the helm chart directly into the cluster

I have tested this for all combinations of Kubernetes 1.26.8, 1.27.8 and 1.28.3 and flux 2.2.0 and 2.2.3

Running helm install test . --set 'subchart.enabled=false' directly does not install the subcharts crd

Steps to reproduce

  1. install flux using the CLI
  2. apply the following manifest to your cluster
apiVersion: source.toolkit.fluxcd.io/v1
kind: GitRepository
metadata:
  name: fluxcd-subchart-crd-test
  namespace: default
spec:
  interval: 5m0s
  url: https://github.com/mbrouns/fluxcd-subchart-crd-test
  ref:
    branch: main
---
apiVersion: helm.toolkit.fluxcd.io/v2beta2
kind: HelmRelease
metadata:
  name: fluxcd-test
  namespace: default
spec:
  interval: 10m
  timeout: 5m
  chart:
    spec:
      chart: mainchart
      version: "0.1.0"
      sourceRef:
        kind: GitRepository
        name: fluxcd-subchart-crd-test
      interval: 5m
  releaseName: crd-test
  values:
    replicaCount: 1
    subchart:
      enabled: false

  1. run kubectl get crd and observe the crd called crontabs.stable.example.com being created

Expected behavior

I expect no crd to be created for disabled helm subcharts

Screenshots and recordings

No response

OS / Distro

MacOS running Minikube, but also on ranchers K8s

Flux version

v2.2.3

Flux check

► checking prerequisites
✔ Kubernetes 1.28.3 >=1.26.0-0
► checking version in cluster
✔ distribution: flux-v2.2.3
✔ bootstrapped: false
► checking controllers
✔ helm-controller: deployment ready
► ghcr.io/fluxcd/helm-controller:v0.37.4
✔ kustomize-controller: deployment ready
► ghcr.io/fluxcd/kustomize-controller:v1.2.2
✔ notification-controller: deployment ready
► ghcr.io/fluxcd/notification-controller:v1.2.4
✔ source-controller: deployment ready
► ghcr.io/fluxcd/source-controller:v1.2.4
► checking crds
✔ alerts.notification.toolkit.fluxcd.io/v1beta3
✔ buckets.source.toolkit.fluxcd.io/v1beta2
✔ gitrepositories.source.toolkit.fluxcd.io/v1
✔ helmcharts.source.toolkit.fluxcd.io/v1beta2
✔ helmreleases.helm.toolkit.fluxcd.io/v2beta2
✔ helmrepositories.source.toolkit.fluxcd.io/v1beta2
✔ kustomizations.kustomize.toolkit.fluxcd.io/v1
✔ ocirepositories.source.toolkit.fluxcd.io/v1beta2
✔ providers.notification.toolkit.fluxcd.io/v1beta3
✔ receivers.notification.toolkit.fluxcd.io/v1
✔ all checks passed

Git provider

GitHub

Container Registry provider

N/A

Additional context

If anyone's able to point me in the right direction I'd be happy to help fixing this

Code of Conduct

  • I agree to follow this project's Code of Conduct
@stefanprodan stefanprodan transferred this issue from fluxcd/flux2 Apr 12, 2024
@stefanprodan
Copy link
Member

stefanprodan commented Apr 12, 2024

Ok turns out this is a bug upstream in Helm and never fixed helm/helm#8508

@stefanprodan stefanprodan added the blocked/upstream Blocked by an upstream dependency or issue label Apr 12, 2024
@stefanprodan
Copy link
Member

@MBrouns if your main chart contains no CRDs, you could skip all CRDs with:

apiVersion: helm.toolkit.fluxcd.io/v2beta2
kind: HelmRelease
spec:
  install:
    crds: Skip
  upgrade:
    crds: Skip

Besides this there is no way we can fix this issue in Flux. In Flux we use the Helm SDK to get the CRDs and the SDK returns all CRDs from all sub-charts before values are compiled.

@MBrouns
Copy link
Author

MBrouns commented Apr 12, 2024

Thanks for that lightning-quick response! Unfortunately we do enable other subcharts in the mainchart that have CRDs, so I don't think that'll fix it for us.

The weird thing is that if I do a helm install directly of the same manifests, the CRD is not installed. Only when I install the chart through flux it is. Are you suggesting there are differences between the helm cli and helm sdk?

@stefanprodan
Copy link
Member

What version of the Helm CLI are you using?

@MBrouns
Copy link
Author

MBrouns commented Apr 12, 2024

version.BuildInfo{Version:"v3.13.3", GitCommit:"c8b948945e52abba22ff885446a1486cb5fd3474", GitTreeState:"clean", GoVersion:"go1.20.11"}

@stefanprodan
Copy link
Member

I don't see how the Helm CLI can do anything different, both the controller and the CLI call this code where all CRDs are merged from all sub-charts and applied:

https://github.com/helm/helm/blob/14d0c13e9eefff5b4a1b511cf50643529692ec94/pkg/action/install.go#L254-L263

@MBrouns
Copy link
Author

MBrouns commented Apr 16, 2024

I agree that it's weird, but it does seem to be what's happening. Running helm install test . --set 'subchart.enabled=false' results in the CRD not being installed for the chart defined in https://github.com/MBrouns/fluxcd-subchart-crd-test/tree/main/mainchart

Let me preface this by saying I'm not very familiar with helm's codebase, but I've done some digging to understand the issue better. I can confirm that when I put a breakpoint here https://github.com/helm/helm/blob/14d0c13e9eefff5b4a1b511cf50643529692ec94/pkg/action/install.go#L256 the length of chrt.CRDObjects() is zero if the subchart is disabled and one if it is enabled.

The crd is initially in the chrt.CRDObjects() at the start of the RunWithContext but gets removed in the call to

if err := chartutil.ProcessDependenciesWithMerge(chrt, vals); err != nil {
    return nil, err
}

As far as I can tell though, Flux calls the same RunWithContext here:

return install.RunWithContext(ctx, chrt, vals.AsMap())
, but the lines above it (
if err := applyCRDs(config, policy, chrt, setOriginVisitor(v2.GroupVersion.Group, obj.Namespace, obj.Name)); err != nil {
return nil, fmt.Errorf("failed to apply CustomResourceDefinitions: %w", err)
}
) do mention something about installing CRDs anyway. Isn't that possibly where the issue is?

@MBrouns
Copy link
Author

MBrouns commented Apr 16, 2024

I looked into it a bit further and took the very blunt approach of calling helmchartutil.ProcessDependenciesWithMerge before applyCRDs is called and that does result in the crd of the subchart not being installed.

Can you confirm whether this makes sense? If so, I'll make the linked PR proper and add stuff like test cases

@artem-nefedov
Copy link

artem-nefedov commented Jun 14, 2024

Got bitten by this as well with splunk-otel-collector chart.
@stefanprodan Helm cli does not reproduce this problem (can be tested with helm template --include-crds command), so this is purely a flux helm-controller issue.

A workaround with spec.install.crds=Skip helped at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked/upstream Blocked by an upstream dependency or issue
Projects
None yet
Development

No branches or pull requests

3 participants