feat: Make generated CRDs directly available for testing and development

[erikgb]

I think the CRDs generated by controller-gen deserves to be available in our projects, and not just temporary files used as input to Helm CRD templates. While this will semi-duplicate the CRDs in project repositories, I think it has a lot of benefits (not limited to):

• Making CRD manifests available in projects, and not just wrapped in a Helm chart.
• Easier to publish CRDs as a release artifact on releases, which is requested in several issues across the cert-manager org.
• Avoiding pitfalls such as https://github.com/cert-manager/trust-manager/blob/main/test/integration/bundle/integration.go#L41, which is a semi-blocker for running integration tests for IDEs.

I have suggested the default kubebuilder path for CRD manifests, even if we don't use/require kubebuilder in our projects. Open for other suggestions, but I think the term "bases" is important - as I think we can use kustomize to transform CRD in other use cases.

[erikgb]

/cc @inteon
/cc @ThatsMrTalbot

[ThatsMrTalbot]

but I think the term "bases" is important

Im not sure, if we just want to publish the CRDs I don't see why we need to pollute the path, I would opt for something like deploy/crds

[erikgb]

but I think the term "bases" is important

Im not sure, if we just want to publish the CRDs I don't see why we need to pollute the path, I would opt for something like deploy/crds

I am ok with any path, as long as we get the plain CRD manifests under source control. 😸 I don't think we currently have any conversion webhooks active, but kustomize patches are useful to fix/enrich the CRDs. Example: https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v4/config/crd. Another interesting approach is using kustomize to "templatify" the CRDs into Helm templates. Example: https://github.com/cybozu-go/accurate/tree/main/config/kustomize-to-helm/overlays/crds. In addition, I believe using kubebuilder "standard directory layout" will make contributor "feel more home" in general.

[SgtCoDFish]

Adding here what I said on slack: I think I'm supportive of this as long as there's automation in place to ensure that these CRDs are updated correctly!

[ThatsMrTalbot]

I'm unsure if this should be done in the helm module, or should be part of a different module. Generating the CRDs on their own is not helm specific.

Another option:

• Move generate-crds to the controller-gen module, it would just generate the CRDs not do any helm stuff
• Replace the generate-crds target in the helm module with generate-helm-crds which depends on generate-crds that can take the CRDs and do the helm specific magic.

This would create a dependency between the two modules, but that should be fine as long as its commnted

[erikgb]

I agree this should be refactored. But I wanted to make the diff as small as possible in this initial PR.

[inteon]

We currently use the Helm chart to generate the CRDs: https://github.com/cert-manager/trust-manager/blob/main/make/test-integration.mk#L15-L19.
This makes sure that the CRDs used for testing match what we output as release artifact. Additionally, this will perform all the templating that was injected.

It's not clear why we want to have the CRDs in a separate file (why being wrapped in a Helm chart is a bad thing).
The unit, integration and e2e tests often have dependencies that have to be generated by Make before running the test. CRDs are just one of the dependencies. I normally run the make target & re-run the go test using the environment variables that were generated for the make target (I agree this is non-ideal, but it will also account for other missing dependencies).

[erikgb]

The unit, integration and e2e tests often have dependencies that have to be generated by Make before running the test. CRDs are just one of the dependencies.

As discussed in our stand-up, I don't this is a valid argument. Why shouldn't we fix this problem for the CRDs when possible, even if we have similar issues to fix later on? 😕

I had a look at the cert-manager CRDs mentioned in the stand-up, and they are currently not the same thing as I am suggesting here. They seem to be "temporary" Helm templates, and not usable to solve my use-case (example). They are also updated (not fully generated) by another controller-gen command. I believe this could be greatly improved by migrating to the standard controller-gen crd command, even in cert-manager, but that remains to prove. 😉 I also found kubernetes-sigs/controller-tools#571 registered by @maelvls.

I am currently in doubt if this PR will be accepted - in any shape, based on some of the feedback so far. 😢 How do we get to a conclusion (hopefully a consensus) here?

[SgtCoDFish]

How do we get to a conclusion (hopefully a consensus) here?

I feel like there are a few things which make me supportive of this PR even though I wouldn't personally use the generated CRDs in my workflow:

1. It's a fairly small and simple change
2. We can easily tell if these files are "up-to-date" in the repo and have CI to enforce that
3. We can leave the CRDs as an implementation detail of setting up tests and warn people away from using them
4. It's easy to revert if we regret it (especially if we warn people away from using them for now).

I think it's worth a go from that basis. The key concerns I can see are:

1. Adam asking about if this should be in the Helm module, which seems valid on the face of it - this doesn't really relate to Helm. I can see the motivation for wanting the simplest possible change, but I think both of Adam's suggestions make sense and don't seem to be disruptive? An alternative would be to keep this logic in the Helm module for now with a big comment saying it'll be moved elsewhere later, but tbh it seems reasonable to move the logic now, in this PR.

2. Tim's comment:

   It's not clear why we want to have the CRDs in a separate file (why being wrapped in a Helm chart is a bad thing).

I think the description of this PR could do with a line explaining why having the CRDs usable without Helm is desirable. As I understand it I think the reason is "it makes it possible / easier to use those CRDs more easily with other tooling such as IDEs" which is not a bad reason. But it wouldn't hurt to spell it out!

I don't see a reason why if those two concerns are addressed we couldn't give this a go - again, it's easy to revert it we think it was a mistake. Generally, I'm supportive of checking generated files into git where it's not too much of a burden, because there's value in being able to visually look at the file in a checkout / on GitHub, and it removes a step for people to get started contributing and we want it to be easy to get started!

[erikgb]

I had a long talk with @inteon (since he seems most negative to do this) last Thursday, hoping we can reach a consensus around it. And we did, at least we came to a shared understanding about the pros and cons. This resulted in a proposed alternative local fix in trust-manager. I am not too happy with just a local fix when we have established makefile-modules. In addition, the proposed alternative has an issue (that probably could be fixed - if we decide to move forward with it):

• make generate-crds does not update the static CRDs

After thinking a bit more around this, I still believe generating static CRDs from the "master" in Go sources (as we already do, but to a "temp" location) is better than re-generating and filtering them out from the Helm chart. And I don't want this to be a local fix in trust-manager. I also work on approver-policy, and I have plans to work more in cert-manager.

So without trying to start a "fight" with Tim, I am going to raise this as a decision point to our next bi-weekly meeting agenda. 🧨

To support my case, I browsed some other popular open source projects to see how they do it. It is certainly possible to find examples supporting the opposite view, but here is what I found:

• Flux kustomization-controller (same in other Flux controller projects)
• Prometheus Operator
• Kyverno
• Cillium
• OPA Gatekeeper
• SPIRE Controller Manager

Many projects seem to also have adopted the kubebuilder standard layout, which is something I also prefer, but this is currently a non-goal for me to introduce this into cert-manager projects. Even if I think it would make any drive-by cloud engineer feel more "home" in our projects. For me, the primary goal now is just to be able to work more effectively and with less friction on cert-manager projects.

[maelvls]

I'm in favor of approving this change. Ash already explained really well the pros and cons, so I will refer to his comment.

As I understand it: with this change, developers of projects using Makefile Modules will no longer need to run some make command before being able to run "envtest" tests, that we also call "integration tests". I see myself benefiting from this change since I've often bumped into the issue when running the envtest tests with the Delve debugger and had to configure env vars for it. The slight duplication (if we can call it that) is frankly not a problem to me.

The unit, integration and e2e tests often have dependencies that have to be generated by Make before running the test. CRDs are just one of the dependencies.

Yes, theoretically, all tests may have dependencies. But I'd like to emphazise that envtest tests are to my eyes just like unit tests: fast and can be run offline. I don't like calling them "integration tests". Thus, striving to make these tests easier to debug (using Delve or whichever) is a great idea. Erik shared with me that he uses envtests a lot because (1) he can't use Kind on his laptop and (2) it makes it possible to run the debugger and see what's going. I'm all in favor of helping him.

[inteon]

@erikgb I further discussed this PR with @maelvls this morning and we agreed that we want to empower you to make these kind of changes. Based on our past collaboration, we also know that we can rely on you taking ownership of this solution and making sure the solution is consistent across all projects. Additionally, this solution can still improve in the future.
My alternative solution cert-manager/trust-manager#482 can be closed after we finish and merge this PR.
I'm looking forward to better understand this approach and the next steps it will unlock.

/approve

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[SgtCoDFish]

/lgtm

If Tim has approved, I'm happy to LGTM on that basis! Looking forward to seeing more on this 🚀

[erikgb]

I further discussed this PR with @maelvls this morning and we agreed that we want to empower you to make these kind of changes. Based on our past collaboration, we also know that we can rely on you taking ownership of this solution and making sure the solution is consistent across all projects. Additionally, this solution can still improve in the future.

Thanks for the faith @inteon! Appreciated! ❤️

I think this PR was merged a bit quickly. We got lost in principal discussions and forgot to discuss the following details: 🤠

• Where should the generated files be located (by default) in our projects?
• Should we try to refactor the generation out from the Helm makefile-module? Or just keep them in Helm, at least for now?

Any input here would be appreciated!

[inteon]

Indeed. I should have specified: I /approve the changes but have not tested nor reviewed the code ( I think the /lgtm command indicates that normally ).

[SgtCoDFish]

Sorry, I thought this was good to merge! We'll revert and re-review a followup. I don't think we've ever formalised what the different review actions mean and maybe that's worth writing down some time.

Where should the generated files be located (by default) in our projects?

deploy/crds/... IMO, to match cert-manager?

Should we try to refactor the generation out from the Helm makefile-module? Or just keep them in Helm, at least for now?

It doesn't belong in a module called "Helm" long term unless it depends on Helm (which I don't think it does). We could keep it in the helm module short-term I guess if we needed to - but I don't think it should be too much work to move this elsewhere? I'm not really sure on that though.

[erikgb]

It was continued in #214.