🌱 crd gen: handle type any #1252

brekelj1 · 2025-08-05T01:31:40Z

Handle Go1.18's any type in CRD generator.

linux-foundation-easycla · 2025-08-05T01:31:44Z

The committers listed above are authorized under a signed CLA.

✅ login: brekelj1 / name: Jasper (7049f31, 2ebad8e)

k8s-ci-robot · 2025-08-05T01:31:49Z

Welcome @brekelj1!

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 · 2025-08-05T01:31:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brekelj1
Once this PR has been reviewed and has the lgtm label, please assign sbueringer 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 · 2025-08-05T01:31:50Z

Hi @brekelj1. 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 · 2025-08-05T15:22:05Z

pkg/crd/testdata2/go.mod

Do we need a whole new testdata to be able to test this change? testdata is a well known path, I'm not sure if you can just create a testdata2 and have it have the same grace/ignoring that is done for testdata

You're right. Changed tests to use existing testdata directory to follow existing approach.

JoelSpeed · 2025-08-05T15:23:17Z

pkg/crd/gen_integration_test.go

@@ -171,6 +171,44 @@ var _ = Describe("CRD Generation proper defaulting", func() {
 	})
 })

+var _ = Describe("CRD Generation with any", func() {
+	It("Works", func() {


Does this need a separate suite or can it be integrated into the existing testdata?

I introduced a separate Describe because the original Describe said "CRD Generation proper defaulting" which doesn't quite fit what is being tested.

Having said that "CRD Generation proper defaulting" was already testing many more behaviours than proper defaulting. I merged the test into the existing structure.

JoelSpeed · 2025-08-05T15:24:04Z

pkg/crd/testdata2/v1/example.com_foos.yaml

+            type: object
+          spec:
+            properties:
+              bar: {}


What does this mean from an API perspective? What will the API server do when it sees an empty schema for bar?

Applying CRD to a cluster gives this error:

The CustomResourceDefinition "foos.example.com" is invalid: spec.validation.openAPIV3Schema.properties[spec].properties[bar].type: Required value: must not be empty for specified object fields

I haven't been able to find a reference in docs/code, but I think we can conclude these kind of schemas are invalid in K8S world.

Looks like this officially states type is required here.
Ref: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/

A structural schema is an OpenAPI v3.0 validation schema which:

specifies a non-empty type (via type in OpenAPI) for the root, for each specified field of an object node (via properties or additionalProperties in OpenAPI) and for each item in an array node (via items in OpenAPI), with the exception of:
1a. a node with x-kubernetes-int-or-string: true
1b. a node with x-kubernetes-preserve-unknown-fields: true
...

JoelSpeed · 2025-08-05T15:25:33Z

It appears at the moment that we are generating a schema with the any type, I was somewhat expecting to return an error from the generator to say you can't use any or something more helpful than that tbh

I'm not sure if returning an empty schema is correct here, I don't know how the API server reacts to that

/hold we need to explore what the correct behaviour is here

brekelj1 · 2025-08-06T02:05:50Z

I updated the approach to always error on interface types (instead of also allowing empty interfaces): #1252 (comment)

Also made updates for the other comments.

@JoelSpeed please rereview

JoelSpeed · 2025-08-07T10:06:58Z

pkg/crd/testdata/gen/iface/iface.example.com_kindwithifaces.yaml

If the generator errors for this type, do we still expect it to spit out a yaml file? Not sure I was expecting to see a yaml output

1
This is how the existing CRD generator behaves for similar kinds of errors. For example, this existing code follows the same approach for error handling on interface types:

ctx.pkg.AddError(loader.ErrFromNode(fmt.Errorf("unsupported AST kind %T", expr), rawType)) // NB(directxman12): we explicitly don't handle interfaces return &apiext.JSONSchemaProps{}

I think reassessing the failure behaviour should be left out of scope of this pull request, in favour of more holistically revisiting that in another pull request.

2
More to the point: I'm not sure how much of an issue outputting a YAML file is when there are errors like this, since the user can see those errors in the output anyway (did anyone over the years raise issues about this?), and even if the user overlooked the errors the CRD will be rejected by the API server, so that would incentivise the user to look closer anyway.

It also looks like func main/controller-gen exits with non-zero code when errors are added to *"pkg/loader".Package's (which makes it more difficult for users to overlook):

https://github.com/kubernetes-sigs/controller-tools/blob/main/cmd/controller-gen/main.go#L187

https://github.com/kubernetes-sigs/controller-tools/blob/main/pkg/genall/genall.go#L282

3
I added this YAML file in this PR since the existing "should have deterministic output" test load everything in testdata/gen as ./... to test group-kinds are sorted/deterministic. This also includes iface_types.go.

JoelSpeed · 2025-08-08T09:09:38Z

/lgtm

@sbueringer @alvaroaleman I think this is a step in the right direction for avoiding panics when we come across any types. I know there are other open threads about potentially support any without an error, but until we come to a conclusion on the appropriate course there, I think this helps

Any concerns?

k8s-ci-robot · 2025-08-08T09:09:46Z

LGTM label has been added.

Git tree hash: c98381fff1ecc0b9ab610c6ae079b545f433ae78

JoelSpeed · 2025-08-08T09:09:53Z

/ok-to-test
/hold cancel

sbueringer · 2025-08-11T19:09:44Z

/assign

I'll take a look

sbueringer · 2025-08-12T09:06:19Z

pkg/crd/schema.go

@@ -294,6 +294,10 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema
 			Format: fmt,
 		}
 	}
+	if _, isInterface := typeInfo.(*types.Interface); isInterface {


Does types.Interface only capture any? Or do we actually want to block all interface types even if they would work?

What about changing the logic below to simply not panic on the type cast?

typeInfoWithObjMethod, ok := typeInfo.(interface{ Obj() *types.TypeName }) if ok { typeNameInfo := typeInfoWithObjMethod.Obj() pkg := typeNameInfo.Pkg() pkgPath := loader.NonVendorPath(pkg.Path()) if pkg == ctx.pkg.Types { pkgPath = "" } ctx.requestSchema(pkgPath, typeNameInfo.Name()) link := TypeRefLink(pkgPath, typeNameInfo.Name()) return &apiext.JSONSchemaProps{ Ref: &link, } }

Then we can still handle various cases below that

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 5, 2025

k8s-ci-robot requested review from chrischdi and JoelSpeed August 5, 2025 01:31

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 5, 2025

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 5, 2025

brekelj1 force-pushed the handle-any-type branch from 779349b to 2ebad8e Compare August 5, 2025 01:36

crd gen: handle type any

2ebad8e

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2025

brekelj1 mentioned this pull request Aug 5, 2025

panic: interface conversion: *types.Interface is not interface { Obj() *types.TypeName }: missing method Obj #1251

Open

JoelSpeed reviewed Aug 5, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2025

always error on iface tests, avoid separate testdata dir

7049f31

brekelj1 requested a review from JoelSpeed August 6, 2025 09:07

JoelSpeed reviewed Aug 7, 2025

View reviewed changes

k8s-ci-robot assigned JoelSpeed Aug 8, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 8, 2025

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 8, 2025

k8s-ci-robot assigned sbueringer Aug 11, 2025

sbueringer reviewed Aug 12, 2025

View reviewed changes

🌱 crd gen: handle type any #1252

Are you sure you want to change the base?

🌱 crd gen: handle type any #1252

Conversation

brekelj1 commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 5, 2025

Uh oh!

k8s-ci-robot commented Aug 5, 2025

Uh oh!

k8s-ci-robot commented Aug 5, 2025

Uh oh!

JoelSpeed Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

brekelj1 Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

brekelj1 Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

brekelj1 Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brekelj1 Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Aug 5, 2025

Uh oh!

brekelj1 commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

brekelj1 Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Aug 8, 2025

Uh oh!

k8s-ci-robot commented Aug 8, 2025

Uh oh!

JoelSpeed commented Aug 8, 2025

Uh oh!

sbueringer commented Aug 11, 2025

Uh oh!

sbueringer Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brekelj1 commented Aug 5, 2025 •

edited

Loading

linux-foundation-easycla bot commented Aug 5, 2025 •

edited

Loading

brekelj1 Aug 6, 2025 •

edited

Loading

brekelj1 Aug 8, 2025 •

edited

Loading

brekelj1 commented Aug 6, 2025 •

edited

Loading

brekelj1 Aug 8, 2025 •

edited

Loading

sbueringer Aug 12, 2025 •

edited

Loading