kube-logging / logging-operator

Logging operator for Kubernetes
https://kube-logging.dev
Apache License 2.0
1.52k stars 326 forks source link

Provide CRDs in a subchart #1732

Open cmontemuino opened 2 months ago

cmontemuino commented 2 months ago

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

Helm 3 does not manage CRDs (see https://helm.sh/docs/chart_best_practices/custom_resource_definitions/). helm uninstall won't remove CRDs, and helm updgrade won't upgrade them. Manual intervention is required with the current setup. It is not GitOps friendly at all.

Additionally, it is not possible to template CRDs in Helm 3 in the crds folder.

The following comes from helm best practices around CRDs:

https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

There is no support at this time for upgrading or deleting CRDs using Helm. This was an explicit decision after much community discussion due to the danger for unintentional data loss.

Describe the solution you'd like

Put the CRDs into a subchart.

Describe alternatives you've considered

Our current approach is publishing a chart in our private registry with the CRDs alone. Then, we install the CRDs first, and logging-operator chart later. This is done with Argo CD, having two applications (one per each chart).

Additional context

A (debatable) better approach here would be to publish CRDs in a separate chart. This is exactly what Prometheus is doing: https://github.com/prometheus-community/helm-charts/tree/main/charts/prometheus-operator-crds

Prometheus is following https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-2-separate-charts:

Another way to do this is to put the CRD definition in one chart, and then put any resources that use that CRD in another chart.

In this method, each chart must be installed separately. However, this workflow may be more useful for cluster operators who have admin access to a cluster

I do think having CRDs in a subchart might be convenient enough. Indeed, this approach is also available in the prometheus-stack chart: https://github.com/prometheus-community/helm-charts/tree/main/charts/kube-prometheus-stack/charts/crds

cmontemuino commented 2 months ago

I can open a PR with proposed changes if that's ok.

pepov commented 2 months ago

Can't you install crds using kustomize from argocd?

See:

 kubectl apply -k github.com/kube-logging/logging-operator/config/crd\?ref=4.6.0 --dry-run
cmontemuino commented 2 months ago

Can't you install crds using kustomize from argocd?

See:

 kubectl apply -k github.com/kube-logging/logging-operator/config/crd\?ref=4.6.0 --dry-run

I should have been be more clear in the description. Apologies for that:

Having CRDs in a subchart comes with an extra feature: it's possible to template them. One interesting case is adding optional annotations. For Argo CD users, the following is pretty standard I'd say:

...
metadata:
  annotations:
    argocd.argoproj.sync-options: Delete=false,Replace=true,ServerSideApply=true
...

Is there something I might be missing here? I mean to not follow the suggested approach to CRDs installation.

pepov commented 2 months ago

I see. I'm not against the subchart as long as the content is generated and is integrated with the current workflow, contribution is of course absolutely welcome!

pepov commented 2 months ago

Please make it completely optional

stale[bot] commented 3 weeks ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

pepov commented 1 week ago

@cmontemuino are you still planning to implement this?