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

Skip applying CRDs in main traefik helm chart. #1315

Open
2 tasks done
PurseChicken opened this issue Jan 20, 2025 · 17 comments
Open
2 tasks done

Skip applying CRDs in main traefik helm chart. #1315

PurseChicken opened this issue Jan 20, 2025 · 17 comments
Labels
kind/question Further information is requested

Comments

@PurseChicken
Copy link

PurseChicken commented Jan 20, 2025

Welcome!

  • Yes, I've searched similar issues on GitHub and didn't find any.
  • Yes, I've searched similar issues on the Traefik community forum and didn't find any.

What did you expect to see?

Now that traefik-crds exist as a separate chart, we need another way to "skip" applying the CRDs from the main traefik helm chart.

Currently the only option is to use the helm option of --skip-crds. This is not ideal as there may be other ways that the chart is applied beyond using helm directly. ArgoCD specifically allows you to use .spec.source.helm.skipCrds: true but using this in an application set means that it applies to every application manifest derived from the application set. As you can imagine this is not ideal.

There are a few options that I think can be considered:

1. Completely remove the CRDs from the traefik chart and require applying CRDs with the separate chart (bonus here is that you only have to manage CRDs in one place):

helm install traefik-crds traefik/traefik-crds
helm install traefik traefik/traefik

2. Convert existing traefik/crds directory to a sub-chart. Reference it as a dependency chart enabled by a key value pair.

Example:

values.yaml:

crds:
  enabled: true

Chart.yaml:

dependencies:
    - name: crds
      version: "1.2.0"
      condition: crds.enabled

traefik/crds/Chart.yaml:

apiVersion: v2
name: crds
version: 1.2.0

3. Change CRD manifests to include if block based on key value pair in values:
STRIKED, as this is not a valid option. You can't template manifests in the default CRD folder.

exampleCrdManifest.yaml:

{{- if .Values.crds.enabled }}
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
........
......
....
..
{{- end }}

values.yaml:

crds:
  enabled: true
@PurseChicken PurseChicken changed the title Add ability to completely skip applying CRDs Skip applying CRDs in main traefik helm chart. Jan 20, 2025
@grantcarthew
Copy link

More context from a Google Cloud Platform perspective.

Googles documentation states to enable the Gateway API as either an option on the deployment of a new cluster or executing a command against an existing cluster:

https://cloud.google.com/kubernetes-engine/docs/how-to/deploying-gateways

This gives you the Gateway API CRDs to support the GKE Gateway Controller:

https://cloud.google.com/kubernetes-engine/docs/concepts/gateway-api#gateway_controller

The Gateway API CRDs should not be deployed as part of a cluster add-on such as Traefik as has been stated.

That said, the CRDs for Traefik use such as the IngressRoute, Middleware, etc, should be deployed with Traefik.

Maybe a thirdPartyCrds.enabled option, or include the Gateway API CRDs with gateway.enabled?

@PurseChicken
Copy link
Author

PurseChicken commented Jan 23, 2025

@grantcarthew There is already a separate chart for traefik CRDs in which you can disable gateway-api crds when deploying in GKE:

https://github.com/traefik/traefik-helm-chart/tree/master/traefik-crds

The current recommended approach for using this separate chart is to deploy traefik using the --skip-crds helm flag. E.G.

helm install traefik-crds traefik/traefik-crds
helm install traefik traefik/traefik --skip-crds

This Github Issue was created because not everyone uses helm directly so using --skip-crds is not a valid option. Or at least not one that addresses different ways of deploying traefik. In my case, with a broad ArgoCD application set.

I believe the best approach here is to remove the CRDs altogether from the main traefik chart and require the use of the traefik-crds chart. If that's not acceptable then we need a way to remove the CRD's from the main chart beyond using the --skip-crds helm flag. Some ways to do this are outlined in the OP.

@darkweaver87
Copy link
Contributor

Hello @PurseChicken ,

Thanks for this feedback. I'll will check what we can do but while reading your proposition:

  1. is not really an option as traefik installation should work out of the box for the "standard" case
  2. cf Json schema validation fails when using traefik-crds as chart dependency #1311, we will open a documentation PR to explain why this is not satisfactory and why we chose to not go that way
  3. I'm more about going with something like this but I'll check with other maintainers because this basically override the current role of traefik-crds chart

Rémi

@PurseChicken
Copy link
Author

Hello @PurseChicken ,

Thanks for this feedback. I'll will check what we can do but while reading your proposition:

  1. is not really an option as traefik installation should work out of the box for the "standard" case
  2. cf Json schema validation fails when using traefik-crds as chart dependency #1311, we will open a documentation PR to explain why this is not satisfactory and why we chose to not go that way
  3. I'm more about going with something like this but I'll check with other maintainers because this basically override the current role of traefik-crds chart

Rémi

@darkweaver87 If you like I can submit a PR using option 3.

@darkweaver87
Copy link
Contributor

darkweaver87 commented Jan 31, 2025

@PurseChicken, feel free to open a PR. Thanks.

@mloiseleur mloiseleur added kind/question Further information is requested and removed status/0-needs-triage labels Jan 31, 2025
@grantcarthew
Copy link

grantcarthew commented Feb 7, 2025

Argo CD Workaround - App-of-Apps

I was having trouble with this issue. Fortunately, I'm not the only one and the Argo CD fantastic people have come to the rescue.

There were two problems in my case:

  1. Disable the CRDs deployment with the Traefik Helm Chart
  2. Use a Parent Chart to deploy.

skipCrds

The first problem is easy to fix using the skipCrds feature of the Argo CD Application custom resources:

This stopped the Traefik Helm Chart from trying to clobber the GKE deployed Kubernetes Gateway API Customer Resource Definitions.

We still need to deploy the Traefik CRDs though. Thanks to the this brilliant repo, this is easy enough:

skipSchemaValidation

A general pattern we have at work is to deploy charts from a local git repository. In the case the Traefik CRDs, this caused an issue.

When you subchart a repo, Helm injects the global variable. The owners of this repo have made a choice not to include the global variable in the schema of the variables. This causes a schema validation error in Argo CD when trying to deploy the Chart:

This is easy enough to bypass by using the Helm command argument --skip-schema-validation. Unless you are using Argo CD and not running the Helm command directly.

Again, the Argo CD champions have come to the rescue.

They have updated Argo CD to support the Helm skipSchemaValidation feature:

This feature was release 3 days ago, and I deployed Argo CD v2.14.1 yesterday. We now have had success deploying the traefik-crds Helm Chart via Argo CD as a Subchart.

Here is the release that supports the new feature:

Examples

Good developers copy, great developers paste.
~ Kelsey Hightower

App-of-Apps Templates

_helpers.tpl

---
{{- define "applicationTemplate"}}
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: {{.name}}
  namespace: argocd
spec:
  destination:
    namespace: {{.namespace}}
    server: https://kubernetes.default.svc
  project: default
  source:
    repoURL: {{ or .repoURL .defaultRepoURL }}
    path: {{ or .path (printf "k8s/%s/%s/charts/%s" .environment .cluster .name) }}
    targetRevision: {{ or .targetRevision "HEAD" }}
    {{- if or .skipCrds .skipSchemaValidation }}
    helm:
      {{- if .skipCrds }}
      skipCrds: {{.skipCrds}}
      {{- end}}
      {{- if .skipSchemaValidation }}
      skipSchemaValidation: {{.skipSchemaValidation}}
      {{- end}}
    {{- end}}
{{- end}}

applications.tpl

---
{{- range $service := .Values.services}}
{{- include "applicationTemplate" (
        dict
        "name" $service.name
        "namespace" $service.namespace
        "environment" $.Values.environment
        "cluster" $.Values.cluster
        "defaultRepoURL" $.Values.defaultRepoURL
        "repoURL" $service.repoURL
        "path" $service.path
        "targetRevision" $service.targetRevision
        "skipCrds" $service.skipCrds
        "skipSchemaValidation" $service.skipSchemaValidation
        "Values" $
    )}}
---
{{- end}}

App-of-Apps Values

---
environment: staging
cluster: xyz

defaultRepoURL: "https://some-service.com/somepath"

services:
  - name: argocd
    namespace: argocd

  - name: foo
    namespace: foo

  - name: traefik
    namespace: traefik
    skipCrds: true

  - name: traefik-crds
    namespace: traefik
    skipSchemaValidation: true

Edit: Changed title and added App-of-Apps.

@PurseChicken
Copy link
Author

PurseChicken commented Feb 7, 2025

The above can be a valid approach if applying a single application manifest per application. You are basically adding:

helm:
  skipCrds: true

To the application manifest which is the same as using the helm option of --skip-crds.

That being said, if you use broad applicationsets which end up generating multiple applications based on the applicationset template, its not viable. Using the skipCrds Argo option works in an applicationset, however every application then generated from that applicationset will have that option applied. That is not ideal.

@grantcarthew
Copy link

Good point. I change the title of the last post.

@j
Copy link

j commented Feb 8, 2025

This just bit us today. Using GKE and ArgoCD would force traefik through a sync loop.

Doing these workarounds feels dirty.

Opting out via values makes the most sense:

'''yaml
crds:

disable all

enabled: false

just gateway standard

gateway_standard: false
'''

@grantcarthew
Copy link

I agree with @j, third party CRDs should be optional and disabled by default depending on the other options selected.
I now have two apps in Argo CD to deploy Traefik being "traefik" and "traefik-crds". Not to mention the time I wasted to get this working when a values option is the first place anyone would look.

It's a great project though, and I don't want to come across as putting anything or one down. Keep up the fantastic work. It is appreciated. Whatever the contributors decide :-)

@j
Copy link

j commented Feb 8, 2025

Plus if other proxy's do the same then there will be same-cluster conflicts. What I install in a helm chart should ideally not be affected by other tools and very least, be able to opt out of them.

This is an easy PR though if I had time I'd throw one together

@PurseChicken
Copy link
Author

PurseChicken commented Feb 10, 2025

@darkweaver87 So because Traefik uses helms default crd folder, the crd's are not templated. This means that there is no option to use values like .Values.crds.enabled to either enable or disable them. The only option we have here is to modify the way that crds are installed by putting them in the templates folder, use crds-files like the traefik-crds chart does, or using a sub chart like originally mentioned.

I was working on a PR for this and realised that this crd methodology blocks templating as noted above. I could move forward with a PR that moves the CRDs to a folder, or we can go with one of the original options I noted in the OP.

I tested quickly by making the new traefik-crds chart a dependency chart and removing the crds folder from the main traefik chart. That worked exactly as expected if the traefik-crds chart is changed to allow additional properties

treafik-crds chart:
values.schema.json

"additionalProperties": true,

traefik chart:
Chart.yaml: (must run helm dep update)

dependencies:
  - name: traefik-crds
    version: 1.3.0
    repository: "file://../traefik-crds"

By default, this will install the traefik crds only (not hub or gateway-api) since by default the traefik-crds chart has traefik: true set by default. If someone then wants to override this or enable other crd's then they can just add the traefik-crds map to the traefik values file:

traefik chart:
values.yaml:

traefik-crds:
  traefik: false
  gatewayAPI: true

I feel like this is the best way and then allows the traefik maintainers to only have to worry about CRD's in one location.

Here is a repository that shows the changes:

PurseChicken@c4c6d7e

@mloiseleur
Copy link
Member

@PurseChicken There is a PR opened to explain why it's designed that way, see #1321.

Image

Does that answer your questions about current implementation ?

@PurseChicken
Copy link
Author

@mloiseleur If the goal is to not have the current separated CRD chart as a sub-chart to the main traefik chart, then the only other option that I can think of to resolve this issue is to turn the CRD's in the main traefik chart into something that can be templated similar to what is done in the traefik-crds chart using the crds-files directory and crds.yaml file in the templates folder.

Are you saying that the approach in my last post with using traefik-crds as a dependency chart is not a valid approach by the maintainers? If so then a.) I think that should be reconsidered. b.) I will change the proposal to use the same crds.yaml approach in the traefik-crds chart.

@mloiseleur
Copy link
Member

As said in the PR, the objectives are the following:

  1. Support the nominal installation case following official Helm GuideLines
  2. Stay conservative about CRDs to protect resource removal : that's actually one of the reasons why helm doesn't support CRDs upgrades
  3. Allow users to install multiple instances of Traefik chart along with helm managed CRDs

We have tried to put the CRD as template inside the main Chart. It does not work: the IngressRoute or TLSOption cannot be loaded.

@PurseChicken
Copy link
Author

Ill change my PR then to use the same crds-files pattern that the traefik-crds chart uses. This will then allow us to template the crd manifests and enable or disable based on a key\value pair.

@PurseChicken
Copy link
Author

PR is now ready:

#1340

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants