Feature: Move CRDs into separate contour-crds chart #21

[Sefiris]

Move CRDs into separate contour-crds chart #21

[Sefiris]

For good measure, tested all these changes locally using minikube

[deepy]

I'm going to note a blocker here but I'm afraid I'll have to elaborate at a later point

Starting with the easy part, I think outright removing it from the contour chart is going to confuse users
Ideally as a default, but I'm not opposed to having it disabled by default
But from the perspective of a user, I'd expect that after installing the Helm chart I have everything need

And now the part I'll need to fill in more details on later, but Helm's CRD management is incredibly easy to get burned by and I don't think it should be used at all
The short version of this is that for a crd-only chart you need to handle them as normal templated files rather than CRDs, so the gateway-api parts here are fine but not the contour ones

[Sefiris]

Starting with the easy part, I think outright removing it from the contour chart is going to confuse users Ideally as a default, but I'm not opposed to having it disabled by default But from the perspective of a user, I'd expect that after installing the Helm chart I have everything need

We disagree on this part, I have been burnt more by CRDs being included within a controllers deployments than i can count. as someone that has bootstrapped hundreds of clusters with different controllers I always found it odd to have a single controller be the "primary" deployment that all the others relied on because of the CRD deployment.

Correct me if I am wrong here but from my testing the Contour ingress controller works perfectly fine without the CRDs, so from my point of view at least I have everything I need and the CRDs are optional. but for someone that uses the gateway api spec I can understand that these are a "requirement".

And now the part I'll need to fill in more details on later, but Helm's CRD management is incredibly easy to get burned by and I don't think it should be used at all The short version of this is that for a crd-only chart you need to handle them as normal templated files rather than CRDs, so the gateway-api parts here are fine but not the contour ones

Learned something new here thanks!, i've built a lot of internal helm charts but never a CRD so i was not aware of this, I will keep this in mind and make this change.

[deepy]

We disagree on this part, I have been burnt more by CRDs being included within a controllers deployments than i can count. as someone that has bootstrapped hundreds of clusters with different controllers I always found it odd to have a single controller be the "primary" deployment that all the others relied on because of the CRD deployment.

I think we maybe agree on it, but on different points :-)

To me I expect the chart to be self-sufficient, and when I run multiple instances in the cluster the first thing I do is disable CRD deployment so I can manage that centrally
So for a single-instance cluster everything is all peachy, and I still have the option to opt-out if I ever need another instance

But I don't know how many contour instances the average user has, so I think having the option to install CRDs but defaulting to not would be perfectly fine
The reason I prefer to have it enabled by default is that if you just run helm install without providing any values you'd get a somewhat useable deployment

Learned something new here thanks!, i've built a lot of internal helm charts but never a CRD so i was not aware of this, I will keep this in mind and make this change.

Yeah this is a well-hidden pitfall and one of the more awkward points in Helm that I know of, they have at least documented it and the tradeoffs here https://helm.sh/docs/chart_best_practices/custom_resource_definitions/

It gets especially weird when Helm has specific support and functionality for CRDs but then hits you with "There is no support at this time for upgrading or deleting CRDs using Helm"
And while it's fair to point to the lack of consensus around managing CRDs and the inherent risks with incorrectly upgrading or deleting CRDs I think Helm really could've allow for an opinionated way of doing this and helped build that consensus

Luckily for us, we don't need to use the CRDs that the charts provides in the chart itself, so none of the caveats apply to us and we can manage them just as normal resources to allow helm template and tools that rely on it to work :-)

TL;DR
This is a very long-winded way of me saying I'd like to see support retained in the contour chart for installing CRDs
And that we need to move the contour CRDs out of the special Helm crds directory

[Sefiris]

@deepy entirely fair enough! I have slightly come around to your side on this, if I am a cluster operator I can just as well disable the management of the CRDs in the main chart and use the CRD specific chart.

And i wholly agree with you that the defaults should give you a usable and as much as possible production ready deployment.

I have made the changes and tested them once again all locally, and opted for just putting a good CRD management disclaimer in the readme of the contour chart

[tsaarni]

Would it be feasible to use a chart dependency so that the CRD chart is installed automatically when manageCRDs is set to true #21 (comment), or is it necessary to duplicate the CRDs in both charts? I’m mainly wondering whether there’s a way to avoid maintaining the same CRDs in multiple places.

[Sefiris]

Would it be feasible to use a chart dependency so that the CRD chart is installed automatically when manageCRDs is set to true #21 (comment), or is it necessary to duplicate the CRDs in both charts? I’m mainly wondering whether there’s a way to avoid maintaining the same CRDs in multiple places.

@tsaarni, call me crazy but I would avoid this like the plague(just an opinion ofc), Helm in and off itself is already stupidly complex to maintain and test for and I do believe that adding subcharts and dependencies just increases your possible blast radius when it comes to testing changes in the future.

That said because i do love diving into things i've never done before I have created a possible setup for it, see my last push, maybe you guys do really like the way this works, I myself think it's too much added complexity vs just updating 2 files in 2 separate charts :)

[deepy]

@Sefiris I'm not sure why this would need a third chart?
Is there something that'd prevent having the CRDs in the CRDs chart and then conditionally depend on it from the contour chart?

Like for example how Grafana does it in their alloy chart: https://github.com/grafana/alloy/blob/main/operations/helm/charts/alloy/Chart.yaml

I caught a cold this weekend so didn't get a chance to look into it as I had planned, but if you want to I can try doing the final bits to bring this PR over the finishing line tonight

[deepy]

I don't think file:// references will work with a remote repository, and sadly if I read the documentation right this'll need to point to the actual repository

[Sefiris]

for type: library charts these file references should work?.
The Chart.yaml should only resolve when building from the repo.

The following commands worked locally.

# Package
  helm package charts/contour-crds --destination dist

# Install
  helm install contour-crds ./dist/contour-crds-0.1.0.tgz \
    --namespace contour-crds --create-namespace

unpacking the tgz one can see the following inside of it then as well

[deepy]

After some testing, the helm packaging will sort this out regardless of what type it is

[tsaarni]

I do see that Helm dependencies are discouraged but I don’t really have enough experience to say whether the concept is considered fundamentally flawed, or if dependency mechanism is still OK towards project's own CRD chart.

But one big practical implication I do see: the manageCRDs option in the main chart used to refer to templates/crds, which meant those CRDs remain outside Helm’s upgrade/downgrade/delete lifecycle. With the dependency, the CRDs would come from the CRD chart’s templates/, meaning upgrades, downgrades, and deletions of the Contour chart would also affect the CRDs (something Helm otherwise tries to avoid).

[Sefiris]

@Sefiris I'm not sure why this would need a third chart? Is there something that'd prevent having the CRDs in the CRDs chart and then conditionally depend on it from the contour chart?

Like for example how Grafana does it in their alloy chart: https://github.com/grafana/alloy/blob/main/operations/helm/charts/alloy/Chart.yaml

I've actually found examples like these before, but these are not exposed to the world as a chart themselves are they?

When i look for the alloy charts I can find the following

...
grafana/alloy                            	1.5.0                     	v1.12.0    	Grafana Alloy
grafana/alloy-crd                        	1.0.0                     	1.0.0      	A Helm chart the Alloy CustomResourceDefinition...
...

They might be using a CI pipeline for this?, if this option works I'm all for it, but i do not quite understand it myself yet when comparing it to our repo.

I do see that Helm dependencies are discouraged but I don’t really have enough experience to say whether the concept is considered fundamentally flawed, or if dependency mechanism is still OK towards project's own CRD chart.

Thats always up to the maintainers ofc, my strong opinion is that it should be avoided until there is a really good use case for it, and even then limit it as much as possible.

But one big practical implication I do see: the manageCRDs option in the main chart used to refer to templates/crds, which meant those CRDs remain outside Helm’s upgrade/downgrade/delete lifecycle. With the dependency, the CRDs would come from the CRD chart’s templates/, meaning upgrades, downgrades, and deletions of the Contour chart would also affect the CRDs (something Helm otherwise tries to avoid).

Yeah, now that you mention it, I reverted back to the problem we had at the start, and that's not something we want, entirely fair point!

[deepy]

I need to double-check this, but iirc ./templates/crds is not treated the same as ./crds and the latter is the one that includes the (unwanted) Helm CRD support

[Sefiris]

I need to double-check this, but iirc ./templates/crds is not treated the same as ./crds and the latter is the one that includes the (unwanted) Helm CRD support

The "special" crd directory is fairly dumb in Helm and it does not allow includes or any other templating.

The distinction here would be that this is not unwanted in the application chart contour correct?

In most application charts i've seen, they use that crd directory to avoid the application chart from also managing the lifecycle of the CRDs

And in all separated CRD charts i've seen them use templated files, because thats the only way in helm to get it to manage the full lifecycle of the resource.

It puts us in a very difficult spot if you want to "library" the CRDs between the 2, which is why my preference goes out to just duplicating.

[tsaarni]

Ah, but then it was just me mixing things up. The Bitnami chart we inherit from never actually used Helm’s crds support. It simply shipped its CRDs under templates/crds, which looked to me like special handling because of the directory name, but it is just ordinary template.

Given that, the CRDs were always upgraded/downgraded/deleted together with the software. I don't know how well that worked for people, but from that point of view, adding the dependency from main chart towards CRD chart would not be a change like I thought in #22 (comment).

[Sefiris]

Indeed, glad we could clear it up!

The final design question we need answered then is if:

1. We want to keep it in "library" form using the file references(which should resolve themselves when packaging) and is pretty clean from a maintainer perspective.

Or

2. We make the contour chart dependent on the contour-crd chart, this does make maintaining slightly more difficult as an update to the CRD chart would have to happen before you could package and update the dependency in the application chart.

But the end result should be the same as helm will package them at build time.

[deepy]

It's possible to use file references even in 2, it just makes it a bit awkward in that you need to reference the correct version, e.g. https://github.com/deepy/contour-chart/blob/contour-0.3.1/charts/contour/Chart.yaml#L20-L23

Which in reality means that if the CRD version is bumped the dependency also needs to be bumped, or it'll prevent releasing new versions of the main chart.
I imagine this is relatively unlikely to happen, since the CRD update should be happening at the same time as a new release

The other scenario I can see is if the automation creates the new versions but doesn't release them automatically, in which case a human would need to remember to bump both versions to avoid getting weird results

So I think an easy way forward would be moving the CRD files back into contour-crd and make it a conditional dependency with a file:// repository
And then updating #18 to take the additional line into account