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

helmCharts doesn't support --include-crds #3819

Closed
drivelikebrazil opened this issue Apr 20, 2021 · 16 comments
Closed

helmCharts doesn't support --include-crds #3819

drivelikebrazil opened this issue Apr 20, 2021 · 16 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@drivelikebrazil
Copy link

Just to add onto the chain for #3816 and #3815:

The new Helm chart inflator implementation should provide a way to set the --include-crds flag for the helm template step

@drivelikebrazil drivelikebrazil added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 20, 2021
@Shell32-Natsu Shell32-Natsu added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Apr 20, 2021
@monopole
Copy link
Contributor

Can you add some more justification for this? Does helm squash crds by default?

See also #3815 (comment)

@ChristianCiach
Copy link

ChristianCiach commented Apr 28, 2021

I think the CRDs are just part of the helm chart and should not be omitted. When you execute helm install, helm will also install the CRDs if they are not already present.

Our CI pipeline takes a single kustomization.yaml as input and outputs a single kubernetes manifest (as a single file) that must include everything that is necessary to deploy the application. For this example, we try to deploy Traefik. Obviously, Traefik (when using the IngressRoute provider) doesn't work without CRDs, so there is currently no way to build a complete kubernetes manifest using Kustomize when using helm charts as sources.

By the way, our CI system is not connected to the internet. The chart file charts/traefik-9.18.3.tgz is committed to the repo, so there are no security implications to consider.

@ChristianCiach
Copy link

ChristianCiach commented Apr 28, 2021

I tried to use helmChartInflationGenerator instead, but the extraArgs seem to be ignored completely:

helmChartInflationGenerator:
- chartName: traefik
  #valuesInline: {}
  releaseName: traefik
  chartVersion: 9.18.3
  chartRepoUrl: https://helm.traefik.io/traefik
  extraArgs: 
    - '--include-crds'

No matter what I specify as extraArgs, there is no error.

Using kustomize v4.1.2 and Helm v3.5.4.

EDIT: Sorry, I've just noticed that extraArgs is ignored intentionally, according to the release notes.

@drivelikebrazil
Copy link
Author

Helm v3 changed how CRDs are installed. Helm v3 charts now have a crds directory that helm applies when you run helm install. Since they are applied as-is and not templated, helm template doesn't generate them unless you use the new --install-crds flag that adds them to the templated output.

See helm docs: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/

@KnVerey
Copy link
Contributor

KnVerey commented May 14, 2021

Under what circumstances will folks using these charts via Kustomize (so not running helm install) want to exclude CRDs that the chart includes? In other words, do we need to expose the option to enable them, or do we just need to enable them globally in the generator?

@dmizelle
Copy link
Contributor

hey @monopole, thinking about submitting a PR for this. saw on the contributor's guide for kustomize that the process was to e-mail sig-cli and join the biweekly Zoom call. For something like this that's not a big feature, would you still like me to do that? Or is it safe to submit the PR and have the review / discussion happen there?

@dmizelle
Copy link
Contributor

@KnVerey -- I was thinking about having it set to false by default, just so when / if we enable this, it doesn't cause surprises for users. It'd be a new field on the HelmChart struct so that users can set it like:

apiVersion: builtin
kind: HelmChartInflationGenerator
metadata:
  name: argo-applicationset
name: argo-applicationset
version: 3.1.3
repo: https://argoproj.github.io/argo-helm
releaseName: octopus
includeCRDs: true # <--- here
valuesInline:
  pspEnabled: false
  serviceAccount:
    create: false

Thoughts?

@dmizelle
Copy link
Contributor

/assign

@dmizelle
Copy link
Contributor

Opened up #3898 . If you'd like me to go through the more formal process for this, please let me know! I have some further comments in here.

@hans-d
Copy link

hans-d commented Jun 30, 2021

Seeing that #3898 has been merged in late may, and the (current) latest release kustomize/v4.1.3
was released this on 20 May (just 8 days before the merge): when can we expect a new release that includes this?

@natasha41575
Copy link
Contributor

natasha41575 commented Jun 30, 2021

@hans-d We are trying to do a release this week. Fingers crossed that it goes well!

@natasha41575
Copy link
Contributor

I am also closing this issue because #3898 has been merged.

@rkulow
Copy link

rkulow commented Jul 8, 2021

Is there already any update on when this feature will be released or what release number will contain it?

@natasha41575
Copy link
Contributor

natasha41575 commented Jul 8, 2021

The most recent release should have the feature (v4.2.0).

@rkulow
Copy link

rkulow commented Jul 8, 2021

All right, thanks - I was just wondering because the release notes didn't say anything specifically about this feature.

@natasha41575
Copy link
Contributor

natasha41575 commented Jul 8, 2021

api/v0.8.11 has a release note about it and kustomize v4.2.0 has a release note that it depends on api v0.8.11. This is very confusing and unintuitive so I don't blame you for not seeing it, we are hoping to improve our release process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

9 participants