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

Provide support for CRD upgrades natively in zarf #2555

Open
mjnagel opened this issue May 28, 2024 · 5 comments
Open

Provide support for CRD upgrades natively in zarf #2555

mjnagel opened this issue May 28, 2024 · 5 comments
Labels
enhancement ✨ New feature or request

Comments

@mjnagel
Copy link
Contributor

mjnagel commented May 28, 2024

Is your feature request related to a problem? Please describe.

Today zarf's posture for CRD upgrades is identical to helm's - https://docs.zarf.dev/ref/deploy/#handling-customresourcedefinitions-crds. While this does make sense given zarf is using helm under the hood it means that the end user is left to figure out how to upgrade CRDs on their own. Typically this means either the chart needs to handle upgrades (I've seen this done with k8s jobs that apply the CRD manifests) or it has to be handled via the zarf package. Handling via the zarf package is certainly possible, but not ideal (I think it would have to be done with files + actions, since even manifests get wrapped in a helm chart and would rely on helm's behavior).

Describe the solution you'd like

I would like zarf to provide config options for CRDs per chart. By default this would align with the upstream helm default behavior (install only, don't touch upgrades), but could be configured based on package needs. This could mean something like a new field in the helm chart component spec to specify whether to do CRD upgrades or not:

components:
  - name: my-chart
    charts:
      - name: my-chart
        url: https://mychart.com
        version: 2.0.0
        namespace: myns
        crdUpgrade: true

When this field is set Zarf would handle CRD upgrades itself (this would have to be done via k8s api/sdk directly rather than helm given the helm posture on CRDs).

Describe alternatives you've considered

Continue down the current path, shifting the CRD upgrade responsibility to the end user to handle either via the helm chart or some zarf package changes. Perhaps to ease the burden on end users and example package could be provided to show how CRD upgrades would be expected to take place.

Additional context

Another project in a similar space of "managing helm installs" is Flux - they have build a similar spec around their HelmRelease which allows for configuring CRD upgrades. I'm not sure if their project/code would be a useful place to look for inspiration on how to approach this problem differently.

I do think CRD upgrades are likely to be a common problem/need in the future - many upstream charts leave this to the end user as a manual upgrade (ex: kube-prom-stack). It would be great if zarf could provide something to lessen this burden.

@mjnagel mjnagel added the enhancement ✨ New feature or request label May 28, 2024
@phillebaba
Copy link
Contributor

I think we should just follow a similar method to what Flux does. I was partially involved in the discussions at the time, and it became clear that managing CRDs was required. Implementing this should be possible for Zarf as we already have the chart so getting the CRDs from the crd directory in the chart should be simple. We should also keep to Flux choice to not delete CRDs and only create or update them.

@mjnagel
Copy link
Contributor Author

mjnagel commented Jun 13, 2024

@phillebaba yeah I think that would be a great path to go down. One thing I was always curious on was the decision to only "manage" CRDs if they are in the crd directory. In my experience with a number of the opensource charts we use for uds-core there's definitely a handful of charts that use templates for CRDs instead. I wonder if it would possible/reasonable to use the kind only (and not consider directory) to determine what is a CRD we want to manage?

@phillebaba
Copy link
Contributor

Here is a user example that does not involve the Helm crd directory.

kind: ZarfPackageConfig
metadata:
  name: knative
  description: Deploys Knative using Manifest files
  version: 0.0.1
  authors: Justin Bailey
  documentation: https://knative.dev/docs/install/yaml-install/serving/install-serving-with-yaml/
  source: https://github.com/knative/serving

components:
  - name: knative-serving
    required: true
    description: "Knative Serving"
    manifests:
      - name: knative-crds
        files:
          - https://github.com/knative/serving/releases/download/knative-v1.14.1/serving-crds.yaml
      - name: knative-serving
        namespace: knative-serving
        kustomizations:
          - manifests/
    images:
      - gcr.io/knative-releases/knative.dev/serving/cmd/activator:v1.14.1
      - gcr.io/knative-releases/knative.dev/serving/cmd/autoscaler:v1.14.1
      - gcr.io/knative-releases/knative.dev/serving/cmd/controller:v1.14.1
      - gcr.io/knative-releases/knative.dev/serving/cmd/queue:v1.14.1
      - gcr.io/knative-releases/knative.dev/serving/cmd/webhook:v1.14.1
      - gcr.io/knative-releases/knative.dev/net-kourier/cmd/kourier:v1.14.0
      - docker.io/envoyproxy/envoy:v1.26-latest

@salaxander salaxander added this to Zarf Jul 22, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Zarf Jul 22, 2024
@salaxander salaxander removed the status in Zarf Jul 22, 2024
@salaxander salaxander moved this to Triage in Zarf Sep 10, 2024
@Miaoxiang-philips
Copy link
Contributor

I also think it would be very meaningful if Zarf could support this feature.

If there isn't a good solution for now, could we consider having a dedicated document on Zarf CRDs upgrades to provide some suggestions or alternatives?

@mjnagel
Copy link
Contributor Author

mjnagel commented Dec 17, 2024

I did some digging and reading today and believe my initial understanding of Helm and CRDs was a bit flawed. It seems like "helm does not upgrade CRDs" only applies if using the special crds folder within a chart. Based on what I'm seeing. CRDs are actually upgraded as expected when they are in the templates folder. Depending on how a chart is architected, CRD upgrades may actually be possible with Zarf (for example I've successfully upgraded CRDs on the Istio base chart which uses the templates folder). I think these findings also line up with where the upgrade caveat is placed in the helm doc (note that it's nested under Method 1 and not mentioned under Method 2).

This still does not account for CRDs in the crds folder but I think this could still solve a number of scenarios. I'm unsure on the behavior if CRDs are included as zarf manifests. May be worth a doc update to explain this assuming my findings are correct here.

A few upstream helm code links that seem to backup that the special "skip upgrade" logic only applies to resources in the crds folder:

@AustinAbro321 AustinAbro321 moved this from Triage to Backlog in Zarf Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
Status: Backlog
Development

No branches or pull requests

3 participants