Skip to content

🌱 crd gen: handle type any #1252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+119 −5
Open

🌱 crd gen: handle type any #1252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ebad8e
crd gen: handle type any
brekelj1 Aug 5, 2025
7049f31
always error on iface tests, avoid separate testdata dir
brekelj1 Aug 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions pkg/crd/gen_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ import (

var _ = Describe("CRD Generation proper defaulting", func() {
var (
ctx, ctx2 *genall.GenerationContext
out *outputRule
ctx, ctx2, ctx3 *genall.GenerationContext
out *outputRule

genDir = filepath.Join("testdata", "gen")
)
Expand All @@ -53,7 +53,10 @@ var _ = Describe("CRD Generation proper defaulting", func() {
Expect(pkgs).To(HaveLen(1))
pkgs2, err := loader.LoadRoots("./...")
Expect(err).NotTo(HaveOccurred())
Expect(pkgs2).To(HaveLen(2))
Expect(pkgs2).To(HaveLen(3))
pkgs3, err := loader.LoadRoots("./iface")
Expect(err).NotTo(HaveOccurred())
Expect(pkgs3).To(HaveLen(1))

By("setup up the context")
reg := &markers.Registry{}
Expand All @@ -73,6 +76,12 @@ var _ = Describe("CRD Generation proper defaulting", func() {
Checker: &loader.TypeChecker{},
OutputRule: out,
}
ctx3 = &genall.GenerationContext{
Collector: &markers.Collector{Registry: reg},
Roots: pkgs3,
Checker: &loader.TypeChecker{},
OutputRule: out,
}
})

It("should fail to generate v1beta1 CRDs", func() {
Expand Down Expand Up @@ -106,13 +115,15 @@ var _ = Describe("CRD Generation proper defaulting", func() {
Expect(gen.Generate(ctx2)).NotTo(HaveOccurred())

By("loading the desired YAMLs")
expectedFileIfaces, err := os.ReadFile(filepath.Join(genDir, "iface", "iface.example.com_kindwithifaces.yaml"))
Expect(err).NotTo(HaveOccurred())
expectedFileFoos, err := os.ReadFile(filepath.Join(genDir, "bar.example.com_foos.yaml"))
Expect(err).NotTo(HaveOccurred())
expectedFileZoos, err := os.ReadFile(filepath.Join(genDir, "zoo", "bar.example.com_zoos.yaml"))
Expect(err).NotTo(HaveOccurred())

By("comparing the two, output must be deterministic because groupKinds are sorted")
expectedOut := string(expectedFileFoos) + string(expectedFileZoos)
By("comparing the three, output must be deterministic because groupKinds are sorted")
expectedOut := string(expectedFileFoos) + string(expectedFileIfaces) + string(expectedFileZoos)
Expect(out.buf.String()).To(Equal(expectedOut), cmp.Diff(out.buf.String(), expectedOut))
})

Expand Down Expand Up @@ -169,6 +180,26 @@ var _ = Describe("CRD Generation proper defaulting", func() {
By("comparing the two")
Expect(out.buf.String()).To(Equal(string(expectedFile)), cmp.Diff(out.buf.String(), string(expectedFile)))
})

It("should gracefully error on interface types", func() {
gen := &crd.Generator{}
err := gen.Generate(ctx3)
Expect(err).NotTo(HaveOccurred())

wd, err := os.Getwd()
Expect(err).NotTo(HaveOccurred())
matches := 0
for _, pkg := range ctx3.Roots {
for _, pkgError := range pkg.Errors {
posRel, err := filepath.Rel(filepath.Join(wd, genDir), pkgError.Pos)
Expect(err).NotTo(HaveOccurred())
Expect(posRel).To(Equal("iface/iface_types.go:32:6"))
Expect(pkgError.Msg).To(Equal("cannot generate schema for interface type any"))
matches++
}
}
Expect(matches).To(Equal(1))
})
})

type outputRule struct {
Expand Down
4 changes: 4 additions & 0 deletions pkg/crd/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,10 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema
Format: fmt,
}
}
if _, isInterface := typeInfo.(*types.Interface); isInterface {
Copy link
Member

@sbueringer sbueringer Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@brekelj1 brekelj1 Aug 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does types.Interface only capture any?

It captures all interface types (that are named).

Or do we actually want to block all interface types even if they would work?

The intent here is to error on all interface types.
This aligns with what the existing code is doing here:

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

This PR is mainly about avoiding a panic.

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

We could do that.
I would prefer the style of a type switch here, since there is also an earlier if: if basicInfo, isBasic := typeInfo.(*types.Basic); isBasic {.

So it would be something like this:

switch typeInfo := typeInfo.(type) {
case *types.Basic:
   ...
case interface{Obj()*types.TypeName}:
   ...
case *types.Interface:
   ...
default:
   ...
}

I did it this way because existing code was already doing if basicInfo, isBasic := typeInfo.(*types.Basic); isBasic {, so it aligns with that, and I don't want to make the diff bigger and scarier (by indenting/reordering existing code).

ctx.pkg.AddError(loader.ErrFromNode(fmt.Errorf("cannot generate schema for interface type %s", ident.Name), ident))
return &apiext.JSONSchemaProps{}
}
// NB(directxman12): if there are dot imports, this might be an external reference,
// so use typechecking info to get the actual object
typeNameInfo := typeInfo.(interface{ Obj() *types.TypeName }).Obj()
Expand Down
46 changes: 46 additions & 0 deletions pkg/crd/testdata/gen/iface/iface.example.com_kindwithifaces.yaml
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@brekelj1 brekelj1 Aug 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: (devel)
name: kindwithifaces.iface.example.com
spec:
group: iface.example.com
names:
kind: KindWithIFace
listKind: KindWithIFaceList
plural: kindwithifaces
singular: kindwithiface
scope: Namespaced
versions:
- name: iface
schema:
openAPIV3Schema:
properties:
apiVersion:
description: |-
APIVersion defines the versioned schema of this representation of an object.
Servers should convert recognized schemas to the latest internal value, and
may reject unrecognized values.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
type: string
kind:
description: |-
Kind is a string value representing the REST resource this object represents.
Servers may infer this from the endpoint the client submits requests to.
Cannot be updated.
In CamelCase.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
type: string
metadata:
type: object
spec:
properties:
bar: {}
type: object
required:
- metadata
type: object
served: true
storage: true
33 changes: 33 additions & 0 deletions pkg/crd/testdata/gen/iface/iface_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

//go:generate ../../../../../.run-controller-gen.sh crd:crdVersions=v1 paths=. output:dir=.

// +groupName=iface.example.com
package iface

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

type KindWithIFace struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata"`
Spec KindWithIFaceSpec `json:"spec,omitempty"`
}

type KindWithIFaceSpec struct {
Bar any `json:"bar,omitempty"`
}