Skip to content
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.

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

🐛 pkg/crd: fix type casting panic with new default *types.Alias with Go 1.23 #1061

Merged
merged 1 commit into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
pkg/crd: fix type casting panic with new default *types.Alias
In the latest version of go, a change was made to the generation of
Alias types by default. From the release notes:

> By default, go/types now produces Alias type nodes for type aliases.
> This behavior can be controlled by the GODEBUG gotypesalias flag. Its
> default has changed from 0 in Go 1.22 to 1 in Go 1.23.

This provoked a panic in the localNamedToSchema function when processing
any type alias becaused it was expecting only a *types.Named and not a
*types.Alias. This can be fixed by using an anonymous interface to more
broadly "ask" if the object supports the Obj() function instead of
asking it to be a specific type. Types *types.Named and *types.Alias
share this method so it can be used directly.

Note that it would be better (and would have made this easier to debug)
to retrieve the "ok" second return value and print some error on failure
to cast but this is a minimal working patch.

For example you can reproduce a panic, switching the project to use Go
1.23 (and thus gotypesalias to 1), putting the following file in
repro/repro.go:

	// +groupName=repro.io
	package repro

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

	type Repro struct {
		metav1.TypeMeta   `json:",inline"`
		metav1.ObjectMeta `json:"metadata"`

		Reproducer StringAlias `json:"reproducer"`
	}

	type StringAlias = string

Then run:

	go run ./cmd/controller-gen/ crd paths=./repro

You should see something similar to:

	panic: interface conversion: types.Type is *types.Alias, not *types.Named

	goroutine 1 [running]:
	sigs.k8s.io/controller-tools/pkg/crd.localNamedToSchema(0x4001315f50, 0x40003918e0)
		/home/mtardy.linux/controller-tools/pkg/crd/schema.go:258 +0x3bc
	sigs.k8s.io/controller-tools/pkg/crd.typeToSchema(0x4001315f50, {0xa391e8, 0x40003918e0})
		/home/mtardy.linux/controller-tools/pkg/crd/schema.go:197 +0xd0
	sigs.k8s.io/controller-tools/pkg/crd.structToSchema(0x40009ee5f8, 0x400000e300)
		/home/mtardy.linux/controller-tools/pkg/crd/schema.go:436 +0x7d8
	sigs.k8s.io/controller-tools/pkg/crd.typeToSchema(0x40009ee5f8, {0xa391b8, 0x400000e300})
		/home/mtardy.linux/controller-tools/pkg/crd/schema.go:207 +0x90
	sigs.k8s.io/controller-tools/pkg/crd.infoToSchema(0x40000505f8)
		/home/mtardy.linux/controller-tools/pkg/crd/schema.go:125 +0xcc
	sigs.k8s.io/controller-tools/pkg/crd.(*Parser).NeedSchemaFor(0x400009a120, {0x4000207b80, {0x40002c2540, 0x5}})
		/home/mtardy.linux/controller-tools/pkg/crd/parser.go:193 +0x1e8
	sigs.k8s.io/controller-tools/pkg/crd.(*Parser).NeedFlattenedSchemaFor(0x400009a120, {0x4000207b80, {0x40002c2540, 0x5}})
		/home/mtardy.linux/controller-tools/pkg/crd/parser.go:205 +0x9c
	sigs.k8s.io/controller-tools/pkg/crd.(*Parser).NeedCRDFor(0x400009a120, {{0x4000185a06, 0x8}, {0x40002c2540, 0x5}}, 0x0)
		/home/mtardy.linux/controller-tools/pkg/crd/spec.go:93 +0x3d8
	sigs.k8s.io/controller-tools/pkg/crd.Generator.Generate({0x0, 0x0, 0x0, {0x0, 0x0, 0x0}, 0x0, {0x0, 0x0}, {0x0, ...}, ...}, ...)
		/home/mtardy.linux/controller-tools/pkg/crd/gen.go:182 +0x464
	sigs.k8s.io/controller-tools/pkg/genall.(*Runtime).Run(0x400015cbd0)
		/home/mtardy.linux/controller-tools/pkg/genall/genall.go:272 +0x21c
	main.main.func1(0x40002a2200?, {0x40002d9ad0?, 0x4?, 0x8e519c?})
		/home/mtardy.linux/controller-tools/cmd/controller-gen/main.go:176 +0x64
	github.com/spf13/cobra.(*Command).execute(0x40002aec08, {0x4000136090, 0x3, 0x3})
		/home/mtardy.linux/go/pkg/mod/github.com/spf13/[email protected]/command.go:985 +0x834
	github.com/spf13/cobra.(*Command).ExecuteC(0x40002aec08)
		/home/mtardy.linux/go/pkg/mod/github.com/spf13/[email protected]/command.go:1117 +0x344
	github.com/spf13/cobra.(*Command).Execute(...)
		/home/mtardy.linux/go/pkg/mod/github.com/spf13/[email protected]/command.go:1041
	main.main()
		/home/mtardy.linux/controller-tools/cmd/controller-gen/main.go:200 +0x290
	exit status 2

Also added a test case to reproduce the error thanks to sbueringer.

Signed-off-by: Mahe Tardy <[email protected]>
  • Loading branch information
mtardy committed Sep 23, 2024
commit 0a52475258f07af5b41fa79d7d6da1096ce68ab8
2 changes: 1 addition & 1 deletion pkg/crd/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema
}
// NB(directxman12): if there are dot imports, this might be an external reference,
// so use typechecking info to get the actual object
typeNameInfo := typeInfo.(*types.Named).Obj()
typeNameInfo := typeInfo.(interface{ Obj() *types.TypeName }).Obj()

Choose a reason for hiding this comment

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

wow this is a nice one 😲

pkg := typeNameInfo.Pkg()
pkgPath := loader.NonVendorPath(pkg.Path())
if pkg == ctx.pkg.Types {
Expand Down
5 changes: 5 additions & 0 deletions pkg/crd/testdata/cronjob_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,9 @@ type CronJobSpec struct {

HostsAlias Hosts `json:"hostsAlias,omitempty"`

// This tests that string alias is handled correctly.
StringAlias StringAlias `json:"stringAlias,omitempty"`

// This tests string slice validation.
// +kubebuilder:validation:MinItems=2
// +kubebuilder:validation:MaxItems=2
Expand All @@ -351,6 +354,8 @@ type CronJobSpec struct {
Protocol corev1.Protocol `json:"protocol,omitempty" protobuf:"bytes,4,opt,name=protocol,casttype=Protocol"`
}

type StringAlias = string

type ContainsNestedMap struct {
InnerMap map[string]string `json:"innerMap,omitempty"`
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8972,6 +8972,9 @@ spec:
time for any reason. Missed jobs executions will be counted as failed ones.
format: int64
type: integer
stringAlias:
description: This tests that string alias is handled correctly.
type: string
stringPair:
description: This tests string slice validation.
items:
Expand Down
Loading