emissary
emissary copied to clipboard
emissary-ingress
Improve Emissary-Ingress HelmChart and CRDs file
In my opinion the HelmChart and the CRDs file have some options to improve. First the apiext deployment is included in the CRDs file and not the HelmChart, this should be a template there. Also there is a namespace definition in the CRDs file, which is unexpected.
The problem we came across:
Our problem is that we use a custom namespace called "ingress" for our ingress resource and emissary-ingress (they have to be in the same namespace), which is defined by a HelmRelease. We use flux2+ kustomize to apply those resources. The namespace located in the CRDs file will be overridden by the defined namespace in a kustomization and applied, but there shouldn't be a namespace resource in the CRDs file in the first place. According to your documentation, emissary-ingress does not have to be installed in the emissary-system namespace. Also when we applied the HelmRelease we saw an "apiext" deployment which had no mention in the HelmChart and we only found it when we looked into the CRDs file. This deployment also has no securityContext and resource requests, which our policies enforce, so we had to patch the CRDs file, which is something we don't want to do.
Solution:
So in my opinion there shouldn't be an apiext deployment and no namespace in the CRDs file, it should only contain CRDs and the apiext deployment should be a template in the HelmChart, with a securityContext and resource requests. The namespace in the CRDs shouldn't exist at all as it already exists as a template in the chart.
Helm supports CRDs: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you and https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-2-separate-charts. Paired with flux2: https://fluxcd.io/docs/components/helm/helmreleases/#crds See linkerd as an example: https://artifacthub.io/packages/helm/linkerd2/linkerd2?modal=template&template=serviceprofile-crd.yaml
What is your opinion on that?
@cindymullins-dw is there any update? Unfortunately I can't see that ticket. Do you need help? Should I create a PR maybe?
HI @kharf, sorry that is a private repo so am just linking them for our reference. It looks like we're evaluating the options.
+1 to this request. I understand Helm doesn't manage CRDs (updating/deleting). It only installs CRDs which is very useful for folks using something like Terraform to automate cluster deployments. It is a pain to apply kubectl manifests via Terraform and they have their own limitations. However Terraform works pretty well with helm releases. It is a common practice among helm community to install CRDs during a helm release.
@kharf writes:
In my opinion the HelmChart and the CRDs file have some options to improve. First the apiext deployment is included in the CRDs file and not the HelmChart, this should be a template there. Also there is a namespace definition in the CRDs file, which is unexpected.
I agree that the emissary-crds file should only contain CRDs.
I'm uncertain the best way to provide these CRDs, that is, via helm or as raw manifests. Strictly speaking, I think raw manifests may be "best practice" but either approach is acceptable to me.
However, if users are expected to install CRDs from manifests, I would like these additional manifests to be included in the GitHub release, the same way that many (most?) Kubernetes assets are released:
https://github.com/emissary-ingress/emissary/releases/download/v3.3.1/emissary-crds.yaml
If not part of the release, then I'd still like usable (valid) versions to be available from GitHub:
https://github.com/emissary-ingress/emissary/raw/v3.3.1//manifests/emissary-crds.yaml
This deployment also has no securityContext and resource requests, which our policies enforce, so we had to patch the CRDs file, which is something we don't want to do.
Aside: @kharf can you create a PR to patch the apiext deployment securityContext? Regardless of where this manifest is, I would like these changes to be applied ASAP.
@kharf writes:
In my opinion the HelmChart and the CRDs file have some options to improve. First the apiext deployment is included in the CRDs file and not the HelmChart, this should be a template there. Also there is a namespace definition in the CRDs file, which is unexpected.
I agree that the emissary-crds file should only contain CRDs.
I'm uncertain the best way to provide these CRDs, that is, via helm or as raw manifests. Strictly speaking, I think raw manifests may be "best practice" but either approach is acceptable to me.
However, if users are expected to install CRDs from manifests, I would like these additional manifests to be included in the GitHub release, the same way that many (most?) Kubernetes assets are released:
https://github.com/emissary-ingress/emissary/releases/download/v3.3.1/emissary-crds.yaml
If not part of the release, then I'd still like usable (valid) versions to be available from GitHub:
https://github.com/emissary-ingress/emissary/raw/v3.3.1//manifests/emissary-crds.yaml
If its raw manifests, I don't exactly know what it means for the GitOps people. I think they would have to manually manage them then, but I have to test it out, because all other tools we use ship it via Helm I think.
This deployment also has no securityContext and resource requests, which our policies enforce, so we had to patch the CRDs file, which is something we don't want to do.
Aside: @kharf can you create a PR to patch the apiext deployment securityContext? Regardless of where this manifest is, I would like these changes to be applied ASAP.
I will try my best!
Issue is part of multiple other issues, so closing it for: securityContext: https://github.com/emissary-ingress/emissary/pull/4665 resource requests and limits: https://github.com/emissary-ingress/emissary/issues/4771 helm quality of life: https://github.com/emissary-ingress/emissary/issues/4772