✨ Add support for AllowDangerousTypes crd flag (enables float support)

[matthchr]

This can be used to enable float32/float64 (go type) -> number (json schema) for CRDs. It is off by default because floats are not recommended in CRDs.

Discussion about this originated in #245 (which is now closed). @DirectXMan12 had suggested using a flag by this name to allow conditional supporting of floats. There's also at least one other issue asking about this (indirectly) at #93.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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/test-infra repository. I understand the commands that are listed here.

[k8s-ci-robot]

Welcome @matthchr!

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. 😃

[matthchr]

Note(s) for maintainers:

1. I used AllowDangerousTypes as the flag name even though really it's only allowing floats right now. If you'd prefer, I can update this to be more clearly just AllowFloats or something.
2. I looked at existing options for the CRD generator and didn't see any existing options (like MaxDescLength being tested via unit test, so I didn't add a test (I did test manually). If there is some place I should be putting a test feel free to point it out (I tried to find someplace but couldn't).
3. I passed the flag down to the required place in the code (builtinToType) as reasonably as I could given my extremely limited knowledge of the code base - please feel free to suggest an alternative route if the one I came up with isn't clean/clear enough (currently it flows: Generator -> Parser (set at construction in Generate()) -> schemaContext -> builtinToType().

[matthchr]

I signed it

[DirectXMan12]

/hold

thanks for the contribution. This is neat, and I like that it explicitly marks particular types as "this is a known problematic type". I think it fits nicely with #443 as well, but I think we'll want to think about cases where we use one vs the other -- this feels like "types that we don't use" vs 443, which is "types that we shouldn't use", but I'm not sure how clear the distinction is. Thoughts?

[DirectXMan12]

/kind feature
/priority important-longterm

[matthchr]

@DirectXMan12 - it seems like there's a continuum here:

1. Types that are always supported.
2. Types that are allowed by default, but not recommended (warning emitted when they are encountered as per ✨ Warn about things that we strongly recommend avoiding #443). Possibly they are allowed by default for historical reasons and may even be "on their way out" at some point in the future.
3. Types that are not allowed by default, not recommended, but there are some legitimate reasons to need them in certain corner cases. Possibly these types should also emit a warning as per ✨ Warn about things that we strongly recommend avoiding #443 even when they are "switched on" (an integration point between this PR and ✨ Warn about things that we strongly recommend avoiding #443 if desired.
4. Types that are not allowed and will not be allowed, possibly because it just "doesn't make sense" or possibly because the implementation is too difficult/clunky to promote them to category 3.

I do think the two ideas actually play nicely together because if you wanted to say demote a type from category 2 to 3, you can do that and then hide it behind the allowDangerousTypes flag so that people can turn it back on if they so desire.

*I don't actually know what all types are "not recommended" and presumably the recommended practices are relatively static so I wouldn't imagine much release-to-release change in these once they're solidified.

Thoughts?

[matthchr]

@DirectXMan12 - Are there changes that you'd like to see for this PR (and/or its interactions with #443)?

I'm wondering what the process is to get from where it is today to a state where it could be merged.

[DirectXMan12]

I'm wondering what the process is to get from where it is today to a state where it could be merged.

It's largely mergable now, just wanted to make sure we had that conversation and that there was a general plan. The one thing I'd do is maybe copy what you wrote in that comment (it's well-written :-) ) into a comment in the source code, and maybe add a TODO to make a more formal mechanism for putting "type patterns" in those categories.

[DirectXMan12]

then we can get this merged

[matthchr]

@DirectXMan12 thanks for the guidance. I've put the text I wrote as a comment on the AllowDangerousTypes field in Parser.go (I didn't want to put it in gen.go because that shows up in customer facing help and this seemed more of an internal thing than something we want in the help).

Let me know if that looks good or if you'd like any modifications to it - thanks for your time!

[DirectXMan12]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, matthchr

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 [DirectXMan12]

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

[matthchr]

@DirectXMan12 thanks for the approval. Based on the PR approval process it looks like the do-not-merge/hold label is blocking automatic merging. If that's intentional no worries, just wanted to check though as the label may have been left on unintentionally.

[DirectXMan12]

ah, yeah, I forgot to

/hold cancel

[DirectXMan12]

that should do it.

[whalecold]

hi， how can I use the field allowDangerousTypes ?

[matthchr]

Specify it on the cmdline, something like: crd:crdVersions=v1,allowDangerousTypes=true