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

CRDs upgrade support #4001

Open
shahar-h opened this issue Aug 5, 2024 · 10 comments
Open

CRDs upgrade support #4001

shahar-h opened this issue Aug 5, 2024 · 10 comments
Labels
area/installation area/upgrade kind/decision A record of a decision made by the community.
Milestone

Comments

@shahar-h
Copy link
Contributor

shahar-h commented Aug 5, 2024

Description:
Currently Gateway API and EG CRDs are located in EG gateway-helm chart crds folder. This means that CRDs are not upgraded on chart upgrade, and need to be manually upgraded beforehand.

Possible solutions:

  1. Move crds inside templates.
    Real world example: cert-manager chart has CRDs as part of templates folder and exposes crds.enabled flag.
    It also allows to decide if we want to keep the CRDs on chart uninstallation with crds.keep flag, by leveraging "helm.sh/resource-policy": keep helm annotation.
    See: https://cert-manager.io/v1.13-docs/installation/helm/#crd-considerations.
    However, this is a breaking change for consumers that use EG chart as a sub chart and have custom resources as part of the main chart.
  2. Split the current chart into crds chart and application chart, and instruct to install the crds chart first.
    Real world example: Istio provides a base chart that installs CRDs: https://istio.io/latest/docs/setup/install/helm/#installation-steps.

Both 1 and 2 can also solve #3094 by providing a separate flags(1)/charts(2) for Gateway API and EG CRDs.

Also Related:

WDYT?

@shahar-h
Copy link
Contributor Author

shahar-h commented Sep 4, 2024

While trying to split the chart into 2 charts(app chart and CRDs chart) internally CRDs chart installation failed because helm release secret exceeded max 1MB size limit. In order to resolve that issue I split the CRDs chart into 2 more charts - eg CRDS and gateway api CRDs.

@arkodg
Copy link
Contributor

arkodg commented Sep 11, 2024

thinking out loud, we could add two additional templates (w/o deleting the /crds dir) for the CRDs - one for gateway-api and one for envoy-gateway and use knobs such as applyCrds.all applyCrds.gateway-api and applyCrds.envoy-gateway

This solves 2 use cases

  • For cases where the cloud provider like GKE installs the gateway-api resources, a user could install EG with helm install --skip-crds --set applyCrds.envoy-gateway=true to selectively install EG CRDs
  • For upgrade cases , users could run helm upgrade --set applyCrds.all=true to update all CRDs or with --set applyCrds.envoy-gateway=true to only update EG CRDs

the naming for the helm knobs can be improved, of course

@shahar-h
Copy link
Contributor Author

shahar-h commented Sep 17, 2024

@arkodg installing both EG and gateway-api CRDs from the same chart won't work since helm release secret will exceed 1MB limit as I mentioned here.
In addition, installing CRDs from EG chart templates would not work for umbrella charts that contain some CRD instance.

What about keeping the crds folder and create 2 additional CRDs charts?

@arkodg arkodg added the kind/decision A record of a decision made by the community. label Sep 17, 2024
@arkodg
Copy link
Contributor

arkodg commented Sep 17, 2024

ah thanks for trying it out @shahar-h .

Looks like we have a decision to make, neither being ideal

  1. Move CRDs from /crds to templates/ with an opt in / opt out approach similar to what cert-manager does (opt in)
    Pros
  • Users - Makes sure versions of CRD and controller are same, so there's implicit order and support during version upgrades
  • Maintainers - One less chart to maintain
  1. Add another gateway-crds-helm chart similar to what linked does https://linkerd.io/2-edge/tasks/install-helm/#linkerd-crds
    Pros
  • Explicit split up of responsibilities b/w cluster admin (installing CRDs) and platform admin (installing and running EG)
    Cons
  • Users need to ensure they are installing compatible/same versions

Common issues

  • Users may forget to opt in or opt out of installing CRDs during install and upgrade, which is probably also true for a gateway-crds-helm chart where users may forget to install it.

@arkodg
Copy link
Contributor

arkodg commented Sep 17, 2024

argo also puts crds in templates

@arkodg
Copy link
Contributor

arkodg commented Sep 20, 2024

@shahar-h can you help investigate who is the consuming most of the space ?

@shahar-h
Copy link
Contributor Author

@shahar-h can you help investigate who is the consuming most of the space ?

Sure, will do.

@arkodg
Copy link
Contributor

arkodg commented Sep 20, 2024

thanks ! I did a little digging, one way to reduce size could be to rm the API descriptions, but that would break kubectl explain

@shahar-h
Copy link
Contributor Author

shahar-h commented Sep 23, 2024

I computed eg CRDS & gwapi CRDs size by creating separate charts and installing each chart. Then I computed helm secret release size for each one of them with the following command:

kubectl -n envoy-gateway-system get secret <helm-release-secret-name> -o jsonpath='{.data.release}' | base64 -d | wc -c 

Results:

  • gwapi crds chart release secret size - 494132 Bytes (0.49MB)
  • eg crds chart release secret size - 685360 Bytes (0.68MB)

Total: 1.17MB > 1MB size limit

Note that the helm release secret size is lower that real CRDs size since helm uses gzip compression.

BTW there is an open PR to split helm releases into multiple k8s secrets.

@arkodg
Copy link
Contributor

arkodg commented Sep 23, 2024

thanks for analyzing this !
the linked PR and the approach of using multiple k8s secrets looks promising, we could wait it out until that feature goes in and then move this the CRDs into templates

@arkodg arkodg modified the milestones: v1.2.0-rc1, Backlog Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/installation area/upgrade kind/decision A record of a decision made by the community.
Projects
None yet
Development

No branches or pull requests

3 participants