cmd/cue: get go should support optional field comments

[uhthomas]

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

The Kuberentes ecosystem use comments to denote whether a field is optional or required. The comment has a direct effect on the produced OpenAPI schema, and often means otherwise correct Go "schemas" will be represented incorrectly with CUE.

https://github.com/kubernetes/community/blob/e977e6ea355f26130ca555d1e8704893727ee024/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Describe the solution you'd like

CUE should consider the +optional comment when evaluating whether a field is optional.

https://github.com/cue-lang/cue/blob/273602f2266fd12b2aa6f42a6f3119c1294b7fb9/cmd/cue/cmd/get_go.go#L1397C21-L1397C31

Describe alternatives you've considered

Additional context

• Optional fields are missing omitempty json tag external-secrets/external-secrets#2853
• postgres instance set name is not actually optional CrunchyData/postgres-operator#3684

[mvdan]

Even though this is something that the kubernetes ecosystem invented themselves, and is thus relatively niche, I'm fine with adding support. It shouldn't affect any other users.

The heuristic should look for // +optional - that is, it shouldn't match // foo+optional, // +optional more, nor /* +optional */. Otherwise we could easily run into false positives like // Foo is an integer representing the sum of bar+optional.

And, since we're adding support for +optional, I think we should do +required as well, resulting in exactly the opposite result. It seems like k8s only support fields which are either optional or required, so the binary choice is still OK.

[uhthomas]

I believe this is the underlying implementation to evaluate whether a field is optional, so it may be worth emulating this behaviour.

[justenstall]

Since the // +optional comments are parsed by kubebuilder when generating a Kubernetes CustomResourceDefinition, maybe this should be supported with an alternate mode for cue get go, something like cue get go+kubebuilder. The alternate mode could defer to kubebuilder to generate the CRD, then convert the CRD itself to CUE.

This implies a cue get crd functionality which could be used on the generated CRD file. The current OpenAPI implementation would need tweaked to support the Kubernetes-specific OpenAPI extensions.

[mvdan]

@justenstall the change that Thomas is proposing here makes sense regardless, I think, and we should do it. Adding another cue get mode tailored to Kubernetes might be a good idea, but should be considered separately. If you think it's still a good idea after the change here is merged, I'd encourage you to file a new issue :)

[justenstall]

@justenstall the change that Thomas is proposing here makes sense regardless, I think, and we should do it. Adding another cue get mode tailored to Kubernetes might be a good idea, but should be considered separately. If you think it's still a good idea after the change here is merged, I'd encourage you to file a new issue :)

What I'm trying to argue is that cue get go shouldn't support these marker comments. The marker comments are not Go, are not utilized by the Go packages they show up in, and are specifically defined as part of the "kubebuilder" SDK. They mean nothing until parsed by the controller-gen CLI when the developer is generating a Kubernetes CRD. In my opinion, marker comments are so context-specific that they should only be supported in a command specific to that context.

Either way, the implementation in 67ea9cf isn't complete. There are a range of marker comments defined by kubebuilder/controller-gen to describe validation rules: https://book.kubebuilder.io/reference/markers/crd-validation. I think supporting a subset in cue get go is confusing, but supporting them all in cue get go is out of its scope. I am willing to start work on a Kubernetes-specific command to support this use case.

[uhthomas]

To be clear, this is not specific to kubebuilder. This is a convention for the Kubernetes API, as outlined by the Kubernetes Architecture SIG. This directive has a direct impact on the OpenAPI schemas generated by kube-openapi.

https://github.com/kubernetes/community/blob/e977e6ea355f26130ca555d1e8704893727ee024/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Implementation of kubebuilder specific directives is definitely out of scope.

[mvdan]

I don't think it's an all or nothing for what it's worth - cue get go is in part a heuristic, because some of the information we want to extract out of Go types like "is this field optional or required" is simply not something that Go as a language or toolchain supports. So I think leaning on existing comment directives used in the wild such as // +optional is fine, as long as it's only part of the heuristic.

In the same vein, I don't think we should reject other cue get modes - I don't know enough about Kubernetes to have a good idea, but perhaps a cue get kubebuilder would make sense. @justenstall I would encourage you to file a new issue about that, because even if this issue is related, its scope was also much smaller, so I don't think it would be right to repurpose it for a larger feature request :)

[myitcv]

Let me just add that it's great to have these sorts of discussion, so thanks for contributing @uhthomas and @justenstall. In situations like this especially, we need to rely on the experience and expertise of others to help shape things. And often it's the case that we need/want patterns to emerge to help steer how we should do things.

[uhthomas]

@mvdan @myitcv I came across another example today, in Cilium. It uses the // +kubebuilder:validation:Optional comment. Do you think it would be appropriate to add support for this one also?

https://github.com/cilium/cilium/blob/3bd0f5caa3a22e1bfffe5df2686953e66e63fcae/pkg/k8s/apis/cilium.io/v2alpha1/lbipam_types.go#L91-L93