Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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
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 {
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
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

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
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"`
}