Generate CRDs using controller-tools

[tfussell]

Unhappy with our current setup around CRDs, I thought it was time to try upstream Kubernetes controller-tools to generate CRDs from CRs (and extra metadata from comments, see https://book.kubebuilder.io/reference/generating-crd.html). For now, I would just like a review on the resulting CRDs and whether we could migrate to these CRDs in the near future without breaking anything.

This PR does not include the additional step of converting CRD YAMLs to functions that return apiextensionsv1beta1.CustomResourceDefinition such as NewG8sControlPlaneCRD(). This can be done in a separate PR since this PR doesn't actually affect any code yet, just the docs/ directory. Until then there would be duplicate CRD definitions, those in the multiline strings in source code and the generated CRDs in docs/. This would be a temporary state.

An extra benefit of this approach is that we can have full schemata for all types which is required for moving to apiextensionsv1.CustomResourceDefinition due to enforced pruning. See https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#controlling-pruning for more info. This is currently a blocker for spot instances as I understand it.

Checklist

• Consider SIG UX feedback.
• Update changelog in CHANGELOG.md.

[tfussell]

It would be preferable to use a released binary of controller-gen somehow to avoid this label.

[kopiczko]

Can we maybe dockerize release version in some (separate or this one) repository? Maybe docker image already exists? then we can generate stuff with docker run and volume bindings.

[tfussell]

This would be nice, but I tend to worry about the added complexity of keeping another image updated. What do you think about the cluster-api approach of installing the tool before build? https://github.com/kubernetes-sigs/cluster-api/blob/master/Makefile#L164-L165 In this case it's using a Makefile, but I guess we can just add it to scripts/gen.sh.

[tfussell]

This is improved in the following PR (#410) so I'm not too worried about this being perfect.

[kopiczko]

Awesome work man! It would be cool if we somehow could use those yaml directly to install them in CPs (I saw what you wrote in the description). Would be good to try to align with https://github.com/giantswarm/opsctl/pull/596.

[xh3b4sd]

I think this is a step into the right direction. Really cool. But we need to figure out how to extend the CRDs based on the CR related go types/comments. With this approach it would then maybe make sense to simply download the YAML files in opsctl in order to ensure them.

[tfussell]

According to kubernetes-sigs/controller-tools#245 (comment), float64 is not a good type to use in Kubernetes due to round-tripping issues. I'm not sure what the implications of changing this type would be though.

[marians]

Property descriptions apparently are now coming from Go comments. This has several ramifications:

1. All the descriptions from our YAML schema are missing. For an example, look at CertConfig.

2. There is a mix between Go and JSON/YAML casing, as the description always repeats the Go name of the field as a first word. Example:

  properties:
    configMap:
      description: ConfigMap references a config map containing catalog
        values that should be applied to apps in this catalog.

From a UX perspective, that's less than ideal.

3. Also not ideal in terms of readability is that the descriptions are repeating the field name.

I see the benefit of having descriptions only in one place. However, using the Golang one is a trade-off for where UX loses.

[marians]

As it is this PR means we are loosing descriptions.

[xh3b4sd]

Let's not mix docs with internal tooling. In this PR the docs are actually overwritten which we could change. It may not be reasonable to try to solve our documentation needs in one solution together with our internal CRD management. So separating this would probably be better.

[kopiczko]

Let's not mix docs with internal tooling. In this PR the docs are actually overwritten which we could change. It may not be reasonable to try to solve our documentation needs in one solution together with our internal CRD management. So separating this would probably be better.

I don't get that. Can you elaborate here? Or maybe tell what you are referring to?

[kopiczko]

I see the benefit of having descriptions only in one place. However, using the Golang one is a trade-off for where UX loses.

Is the lost bigger than the benefit in your opinion? I'd say people who read those are kind of used to Go style comments. For me not copying stuff over and having simpler workflows by far outweighs a problem of description starting with a repeated name.

[xh3b4sd]

Marian and Thomas started to generate CRDs for documentation reasons and put them in the docs/ folder. This is what is used to display in https://docs.giantswarm.io/reference/cp-k8s-api. Thomas now wanted to generate the CRDs we use with our tooling instead of having these multi line string definitions in go variables. His result as per this pull request is written to the docs/ folder, which now mixes two different purposes. I was questioning if it is good to mix these two use cases which both have likely totally different requirements. So maybe we want to thrive for one approach to rule them all but maybe this is more complicated than necessary. Just wanted to nudge on that a bit.

[xh3b4sd]

For me not copying stuff over and having simpler workflows by far outweighs a problem of description starting with a repeated name.

Which is you, the developer speaking. Marian thinks about the users. See my comment about mixing use cases above.

[marians]

@xh3b4sd Everything in the docs/ folder is generated automatically. The CRD content with all the schema property description texts is contained in YAML strings in Go code. Example

The problem is not that we overwrite docs/. The problem is that we

• have some descriptions in there coming from Go code. This means the outcome is a little strange for end users (but probably still usable).
• lose descriptions which are currently present only in YAML. This means we have to at least make sure we transfer the descriptions from the YAML strings to Go comments before re-generating the docs content.

[marians]

@kopiczko

Is the lost bigger than the benefit in your opinion?

That's hard to judge and to compare, mainly as the gains and losses happen on different ends (internal vs. external).

I think most users will be able to cope with the strangeness and still be able to make the right conclusions. If this helps us getting more complete, up-to-date and accurate property docs, fine. I'm not in the way.

[tfussell]

Thanks for the review and comments. I'll try to address each concern.

[tfussell]

For me not copying stuff over and having simpler workflows by far outweighs a problem of description starting with a repeated name.

Which is you, the developer speaking. Marian thinks about the users. See my comment about mixing use cases above.

I think we have the opportunity to address both use cases with this change. Here's how:

• As you know, with the old method, we had multiline strings in source code containing manually defined CRDs. These CRDs were unmarshalled into runtime.Objects and then marshalled back into standalone YAML files in the docs/crd directory using bespoke tooling. Aligning with upstream Kubernetes means our libraries are more easily understood by internal developers and external technical users.
• The old method required us to write CRDs by hand without syntax highlighting or validation because they were YAML documents in Go source which provided many opportunities for mistakes or outdated schemata/documentation. By generating CRDs from CR structs, we know that our CRDs are valid Kubernetes objects and that they are in sync with CRs.
• We now have a CRD with a schema for every type "for free" which means more documentation and validation for computers and humans even if it's computer generated. It also gives us the ability to move to v1 CustomResourceDefinition which does not allow disabling pruning.
• When we add GoDoc comments to our CR fields, we get them in the CRD field descriptions automatically. This means improving DX (developer experience) also improves UX and vice versa.

[xh3b4sd]

This is cool. We discussed a little. Here are some final thoughts.

• When the go types changed, but the CRDs are not re-generated, CI should fail in an idea world. Not a big deal in the first step, but let's consider this for the upcoming PRs.
• I already made the experience that the files are hard to determine in the Github file tree UI because the generated file names are super long. This makes it super hard for humans to identify where the file you are looking for is. Maybe we could work more with a directory structure. Just an idea.
• It is amazing that now all CR status structures are also defined and that the whole schemas are actually structural and valid. This makes so much sense.
• We should test this somehow on a Control Plane to see if the CRDs are still fine. We can maybe talk about it and try something out together. It is a bit dangerous to deal with so we should make sure this does not break production environments.

[kopiczko]

Another thought which I don't know why I didn't propose in the first place. I'd be fine with breaking godoc conventions for there files to have CRDs properly generated. We can add // nolint:godoc (don't know if this is godoc or something else) for those files and treat them exceptionally.

[tfussell]

Another thought which I don't know why I didn't propose in the first place. I'd be fine with breaking godoc conventions for there files to have CRDs properly generated. We can add // nolint:godoc (don't know if this is godoc or something else) for those files and treat them exceptionally.

I checked cluster-api and it uses normal GoDoc style comments for CRs (field name in CamelCase at the beginning of each one). Both ways seem reasonable to me.

[marians]

Anyway, I'd leave the comment changes out of this PR. We can tackle that later.

[tfussell]

This PR isn't perfect, but most of the issues are resolved in the next PR, #410. If it's okay with everyone, I would merge this to master after some approvals and tie up any loose ends in the next PR.

[tfussell]

The only remaining task for this PR is figuring out what to do about the float64 fields. I will work on that tomorrow. Open to ideas.

[xh3b4sd]

I did not fully understand the problem and we can talk about it in more detail if you like. I just found kubebuilder:validation:Type which may help to enforce the right type?

[tfussell]

Specifying the type here for validation. This will still be a breaking change for the kvm-operator but should be able to work around it with strconv.ParseFloat(MemorySizeGB, 64). I'll test in kvm-operator now.

[tfussell]

Now that float64 has been fixed, this PR has been effectively subsumed by #410 so I'm going to merge it and continue work in the next PR with an announcement that apiextensions@master is a WIP.

[xh3b4sd]

Closing in favour of #410.