✨ +enum validation marker

[MishimaPorte]

As per issue #933, it seems to be beneficial to support upstream +enum validation marker that infers enum members for a type from constant declarations of that type.

So, given

// +enum
type MyEnumType string

const (
    MyEnumType_One MyEnumType = "one"
    MyEnumType_Two MyEnumType = "two"
)

every time the MyEnumType is used, an enum block is rendered in the schema as follows:

enum:
- one
- two

This mechanism only handles the string-based types and does not handle numerical types.

A testcase for the generator is added as well.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: MishimaPorte (410a208, ccd6c36, b338e3f, 1bf22cb, bb6e2b6, 040149f)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MishimaPorte
Once this PR has been reviewed and has the lgtm label, please assign alvaroaleman for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[k8s-ci-robot]

Welcome @MishimaPorte!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @MishimaPorte. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[JoelSpeed]

What happens if an existing API has the +enum marker AND a +kubebuilder:validation:enum marker? Can we add a test?

[JoelSpeed]

There are clearly a lot of types that have enum already that we've ignored up til now. This will impact existing CRDs for users where they've copied core kube types. Are we to assume that where they've copied the built-in type, this is a safe change? The general use case would be to pass through the core kube type to the underlying pod or whatever it is that is being created

Assuming a recent version of K8s, the change should ratchet invalid values anyway 🤔

[MishimaPorte]

I think it is? Probably for core types that have enum tags the values outside of enum-defined set are anyway invalid.

[MishimaPorte]

What happens if an existing API has the +enum marker AND a +kubebuilder:validation:enum marker?

When both +enum and +kubebuilder:validation:enum are defined the +enum takes precedence

[JoelSpeed]

When both +enum and +kubebuilder:validation:enum are defined the +enum takes precedence

Lets add a test case for this one please

[MishimaPorte]

Lets add a test case for this one please

Done

[JoelSpeed]

/ok-to-test

[JoelSpeed]

Are there any other markers that do this kind of thing? Is there precedence?

[JoelSpeed]

Are there any other markers that do this kind of thing? Is there precedence?

[MishimaPorte]

I don't think so. But enum markers are kind of the only ones that do the same thing differently so some decision on precedence here is needed in any case.

[JoelSpeed]

CC @alvaroaleman @sbueringer do you have an opinion on this?

At present, when you have the kubebuilder:validation:enum on the field and a type alias, I believe you get a struct like

allOf:
- enum: []
- enum: []

I wonder if, to be consistent, this case should also do the same? It would encourage developers to deduplicate their markers which I think is probably a positive

[MishimaPorte]

It would encourage developers to deduplicate their markers

I believe this is less of a usecase here since both markers (+enum and +kubebuilder:validation:enum) are at the same place - on the type alias declaration, not in distant places (one on the field and the other on the type). So it is close to impossible to have accidental duplicates, like with the aforementioned example. I personally would prefer this to be an error right away, but it can probably harm backward compatibility.

[JoelSpeed]

I wonder if we want to make the preference configurable. The default behaviour though I think needs to be that kubebuilder:validation:enum takes precedence over the naked enum though. That way we won't break existing users. I'm worried that built in types that already have the enum will get broken when this change rolls out

[MishimaPorte]

I think configurable precedence would complicate things too much - we can just make the kubebuilder:validation:enum take precedence over +enum and call it a day.

For known core types it can be made configurable, but, honestly, the now-generated enum block should have always been there in the schema. I can think of zero example where such a change would complicate something - on the contrary, probably additional validation would make the generated crds less error-prone.

[JoelSpeed]

should have always been there in the schema

Yes, but depending on the use case, this is likely a breaking change for the people using the CRDs built by this tooling

we can just make the kubebuilder:validation:enum take precedence over +enum and call it a day.

I would lean towards this for now to be safe

[MishimaPorte]

this is likely a breaking change for the people using the CRDs built by this tooling

so, a configuration flag? but an opt-in or opt-out one is what we need? I personally would argue for the latter since the change seems to be beneficial in most of the cases (at least when something like PodTemplate is used)