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

go/types: sigs.k8s.io/controller-tools/cmd/controller-gen crashes on 1.22 #65637

Closed
mikkeloscar opened this issue Feb 9, 2024 · 14 comments · Fixed by hashicorp/consul#20692
Closed
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@mikkeloscar
Copy link

Go version

go version go1.22.0 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/<private>/.cache/go-build'
GOENV='/home/<private>/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/<private>/projects/go/pkg/mod'
GONOPROXY='<private>'
GONOSUMDB='<private>'
GOOS='linux'
GOPATH='/home/<private>/projects/go'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2025307450=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Run sigs.k8s.io/controller-tools built with go1.22:

How to replicate:

git clone https://github.com/zalando-incubator/stackset-controller.git
cd stackset-controller
go run sigs.k8s.io/controller-tools/cmd/controller-gen crd:crdVersions=v1,allowDangerousTypes=true paths=./pkg/apis/... output:crd:dir=docs

Also reported here by other users: kubernetes-sigs/controller-tools#880

What did you see happen?

The command:

go run sigs.k8s.io/controller-tools/cmd/controller-gen crd:crdVersions=v1,allowDangerousTypes=true paths=./pkg/apis/... output:crd:dir=docs

crashes with:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa046be]

goroutine 28 [running]:
go/types.(*Checker).handleBailout(0xc0001aae00, 0xc001125d40)
	/usr/lib/go/src/go/types/check.go:367 +0x88
panic({0xbda960?, 0x12c3820?})
	/usr/lib/go/src/runtime/panic.go:770 +0x132
go/types.(*StdSizes).Sizeof(0x0, {0xdd40b8, 0x12cc080})
	/usr/lib/go/src/go/types/sizes.go:228 +0x31e
go/types.(*Config).sizeof(...)
	/usr/lib/go/src/go/types/sizes.go:333
go/types.representableConst.func1({0xdd40b8?, 0x12cc080?})
	/usr/lib/go/src/go/types/const.go:76 +0x9e
go/types.representableConst({0xdda3f8, 0x1297198}, 0xc0001aae00, 0x12cc080, 0xc001121b40)
	/usr/lib/go/src/go/types/const.go:106 +0x2c7
go/types.(*Checker).representation(0xc0001aae00, 0xc00111d4c0, 0x12cc080)
	/usr/lib/go/src/go/types/const.go:256 +0x65
go/types.(*Checker).representable(0xc0001aae00, 0xc00111d4c0, 0x12cc080)
	/usr/lib/go/src/go/types/const.go:239 +0x26
go/types.(*Checker).shift(0xc0001aae00, 0xc00111d440, 0xc00111d4c0, {0xdd8228, 0xc0005b2c00}, 0x14)
	/usr/lib/go/src/go/types/expr.go:650 +0x1eb
go/types.(*Checker).binary(0xc0001aae00, 0xc00111d440, {0xdd8228, 0xc0005b2c00}, {0xdd8738, 0xc0007bc5c0}, {0xdd8738, 0xc0007bc5e0}, 0x14, 0x306bd0)
	/usr/lib/go/src/go/types/expr.go:796 +0x150
go/types.(*Checker).exprInternal(0xc0001aae00, 0x0, 0xc00111d440, {0xdd8228, 0xc0005b2c00}, {0x0, 0x0})
	/usr/lib/go/src/go/types/expr.go:1416 +0x206
go/types.(*Checker).rawExpr(0xc0001aae00, 0x0, 0xc00111d440, {0xdd8228?, 0xc0005b2c00?}, {0x0?, 0x0?}, 0x0)
	/usr/lib/go/src/go/types/expr.go:979 +0x19e
go/types.(*Checker).exprInternal(0xc0001aae00, 0x0, 0xc00111d440, {0xdd80d8, 0xc0007bc600}, {0x0, 0x0})
	/usr/lib/go/src/go/types/expr.go:1320 +0x178
go/types.(*Checker).rawExpr(0xc0001aae00, 0x0, 0xc00111d440, {0xdd80d8?, 0xc0007bc600?}, {0x0?, 0x0?}, 0x0)
	/usr/lib/go/src/go/types/expr.go:979 +0x19e
go/types.(*Checker).expr(0xc0001aae00, 0x0?, 0xc00111d440, {0xdd80d8?, 0xc0007bc600?})
	/usr/lib/go/src/go/types/expr.go:1513 +0x30
go/types.(*Checker).binary(0xc0001aae00, 0xc00111d440, {0xdd8228, 0xc0005b2c30}, {0xdd80d8, 0xc0007bc600}, {0xdd8738, 0xc0007bc620}, 0xd, 0x306bd5)
	/usr/lib/go/src/go/types/expr.go:783 +0xa5
go/types.(*Checker).exprInternal(0xc0001aae00, 0x0, 0xc00111d440, {0xdd8228, 0xc0005b2c30}, {0x0, 0x0})
	/usr/lib/go/src/go/types/expr.go:1416 +0x206
go/types.(*Checker).rawExpr(0xc0001aae00, 0x0, 0xc00111d440, {0xdd8228?, 0xc0005b2c30?}, {0x0?, 0x0?}, 0x1)
	/usr/lib/go/src/go/types/expr.go:979 +0x19e
go/types.(*Checker).use1(0xc0001aae00, {0xdd8228, 0xc0005b2c30}, 0x0)
	/usr/lib/go/src/go/types/call.go:1043 +0x1a5
go/types.(*Checker).useN(...)
	/usr/lib/go/src/go/types/call.go:1004
go/types.(*Checker).use(...)
	/usr/lib/go/src/go/types/call.go:994
go/types.(*Checker).callExpr(0xc0001aae00, 0xc00111d380, 0xc0007be2c0)
	/usr/lib/go/src/go/types/call.go:196 +0x1770
go/types.(*Checker).exprInternal(0xc0001aae00, 0x0, 0xc00111d380, {0xdd87f8, 0xc0007be2c0}, {0x0, 0x0})
	/usr/lib/go/src/go/types/expr.go:1374 +0xf8
go/types.(*Checker).rawExpr(0xc0001aae00, 0x0, 0xc00111d380, {0xdd87f8?, 0xc0007be2c0?}, {0x0?, 0x0?}, 0x0)
	/usr/lib/go/src/go/types/expr.go:979 +0x19e
go/types.(*Checker).expr(0xc0001aae00, 0xc0000ea310?, 0xc00111d380, {0xdd87f8?, 0xc0007be2c0?})
	/usr/lib/go/src/go/types/expr.go:1513 +0x30
go/types.(*Checker).exprInternal(0xc0001aae00, 0x0, 0xc00111d380, {0xdd8198, 0xc0007be300}, {0x0, 0x0})
	/usr/lib/go/src/go/types/expr.go:1190 +0x1f1f
go/types.(*Checker).rawExpr(0xc0001aae00, 0x0, 0xc00111d380, {0xdd8198?, 0xc0007be300?}, {0x0?, 0x0?}, 0x0)
	/usr/lib/go/src/go/types/expr.go:979 +0x19e
go/types.(*Checker).expr(0xc0001aae00, 0x0?, 0xc00111d380, {0xdd8198?, 0xc0007be300?})
	/usr/lib/go/src/go/types/expr.go:1513 +0x30
go/types.(*Checker).varDecl(0xc0001aae00, 0xc000d2c960, {0xc000377020, 0x1, 0x1}, {0x0, 0x0}, {0xdd8198, 0xc0007be300})
	/usr/lib/go/src/go/types/decl.go:521 +0x17b
go/types.(*Checker).objDecl(0xc0001aae00, {0xddfc58, 0xc000d2c960}, 0x0)
	/usr/lib/go/src/go/types/decl.go:194 +0x9e5
go/types.(*Checker).packageObjects(0xc0001aae00)
	/usr/lib/go/src/go/types/resolver.go:693 +0x4dd
go/types.(*Checker).checkFiles(0xc0001aae00, {0xc000384400, 0x7, 0x7})
	/usr/lib/go/src/go/types/check.go:408 +0x1a5
go/types.(*Checker).Files(...)
	/usr/lib/go/src/go/types/check.go:372
sigs.k8s.io/controller-tools/pkg/loader.(*loader).typeCheck(0xc000381380, 0xc0002a5e80)
	/home/<private>/projects/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:286 +0x36a
sigs.k8s.io/controller-tools/pkg/loader.(*Package).NeedTypesInfo(0xc0002a5e80)
	/home/<private>/projects/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:99 +0x39
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check(0xc000601b60, 0xc0002a5e80)
	/home/<private>/projects/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:268 +0x2b7
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check.func1(0x0?)
	/home/<private>/projects/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:262 +0x53
created by sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check in goroutine 1
	/home/<private>/projects/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:260 +0x1c5
exit status 2

What did you expect to see?

With go1.21.7 there is no panic.

I have bisected between go1.21.7 and go.1.22.0 and found this commit to cause the issue: 5fa4aac

@findleyr
Copy link
Contributor

findleyr commented Feb 9, 2024

Indeed, this looks like a breakage due to the change in #61035. A number of tools broke that were relying on undocumented behavior of types.SizesFor.

From kubernetes-sigs/controller-tools#880 this appears to have been fixed downstream. In that case, I'm not sure there's anything more we can do.

CC @griesemer @cuonglm

@seankhliao seankhliao changed the title go1.22: go run sigs.k8s.io/controller-tools/cmd/controller-gen crashes on Go 1.22 go/types: sigs.k8s.io/controller-tools/cmd/controller-gen crashes on 1.22 Feb 9, 2024
@seankhliao seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 11, 2024
AlexanderYastrebov added a commit to szuecs/routegroup-client that referenced this issue Mar 6, 2024
This fixes panic on go1.22, see kubernetes-sigs/controller-tools#880
and golang/go#65637

Since `go get sigs.k8s.io/controller-tools` updates transitive Kubernetes dependencies
this change removes it from dependencies in favor of fixed version in the Makefile.

Signed-off-by: Alexander Yastrebov <[email protected]>
@silverwind
Copy link

silverwind commented Mar 8, 2024

Why is golang doing breaking changes in minor releases? Reserve these for major releases please. Something being undocumented is no excuse, it's still part of the API contract.

@cuonglm
Copy link
Member

cuonglm commented Mar 8, 2024

Why is golang doing breaking changes in minor releases? Reserve these for major releases please. Something being undocumented is no excuse, it's still part of the API contract.

I don't see why it's part of the API contract. Those clients are assuming wrong thing, which types.SizeFor never guarantees any thing on that.

@ianlancetaylor
Copy link
Contributor

@findleyr Sorry for the dumb question, but what is the undocumented behavior that changed? I don't understand how we get a nil pointer to StdSizes here. As far as I can tell everything is coming from go/types and x/tools/go/packages.

@findleyr
Copy link
Contributor

findleyr commented Mar 8, 2024

@ianlancetaylor prior to https://go.dev/cl/516917, go/packages would set a nil types.Config.Sizes because it assumed that types.SizesFor returned a *types.StdSizes. This caused quite a bit of breakage, both in our tools and in third party tools, as it meant that tools would start panicking if they upgraded to Go 1.22 without also updating their x/tools dependency. Although tools using go/packages were most affected, I've also seen this panic without go/packages, because there are other places (e.g. gopls) that assumed types.SizesFor returned a *types.StdSizes.

I don't think I tracked down exactly what happened in this particular case: I've seen that stack many times, and assumed it followed the same pattern. Here's another example: google/wire#400

There are several lessons to be learned here.

@ianlancetaylor
Copy link
Contributor

That makes it sound like the fix is for controller-gen to update to a newer version of golang.org/x/tools. If that is so it seems OK, though hardly ideal, for an upgrade to a new version of Go to require an upgrade to newer versions of supporting packages.

@mikkeloscar
Copy link
Author

My motivation for raising this is that we're currently not in a position where we can update sigs.k8s.io/controller-tools to the latest version because of Kubernetes version dependency and ofc. my implicit assumption (and experience) that Go would not be breaking the backwards compatibility (I understand that this point can be argued whether this is breaking the API contract or not).

@ianlancetaylor
Copy link
Contributor

The change does not break the documented API. And there is a straightforward workaround that doesn't involve changing any code. This change is permitted by our backward compatibility guidelines (https://go.dev/doc/go1compat). I agree of course that this case is not ideal, and we would have avoided it if it were possible. But it wasn't.

It's not clear to me why it is OK for you to update to a new version of Go but it is not OK to update to a new version of golang.org/x/tools.

I'm going to close this issue because while it is unfortunate I don't think there is anything we can do. Please comment if you disagree.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Mar 14, 2024
@silverwind
Copy link

It's not clear to me why it is OK for you to update to a new version of Go but it is not OK to update to a new version of golang.org/x/tools.

The problem is that golang.org/x/tools is imported inside dependencies which is out of control of many users unless they fork those dependency.

@ianlancetaylor
Copy link
Contributor

You should be able to go get golang.org/x/[email protected] in your main module to pick up the latest version of golang.org/x/tools for the entire build. (You may need to add an explicit blank import to keep go mod tidy from removing your desired version.)

@silverwind
Copy link

silverwind commented Mar 14, 2024

Yeah that will help if the dependency is pictured in go.mod, but in the current case we ran the dependency via go run dependency@version, so no way to update x/tools there when the maintainer is unresponsive, except forking, of course.

@zikaeroh
Copy link
Contributor

#48429 will make this easier, but you could use the tools.go method to pin the executable in your module, then upgrade the transitive dep, which will affect the tool when run within the module. i.e.:

//go:build tools

package tools

import (
	"github.com/go-swagger/go-swagger/cmd/swagger"
)

Then run without a version number:

$ go run github.com/go-swagger/go-swagger/cmd/swagger

@silverwind
Copy link

silverwind commented Mar 15, 2024

Yeah, I'm waiting for something like #48429 as that workaround with the go file seems too "dirty" 😆. But yes, it's good to have this option in worst case.

@mikkeloscar
Copy link
Author

I didn't quite realize that updating golang.org/x/tools was the fix, since we already use the tools.go trick, this was easy for us to address for now.

reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 12, 2024
Fix a nil pointer issue after bumping up to go1.22
More details see golang/go#65637

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 12, 2024
Fix a nil pointer issue after bumping up to go1.22
More details see golang/go#65637

Also update the way protoc is called,
more details see:
golang/protobuf#1070

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 12, 2024
Fix a nil pointer issue after bumping up to go1.22
More details see golang/go#65637

Also update the way protoc is called,
more details see:
golang/protobuf#1070

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 12, 2024
Bump up controller-gen to v0.14.0 to fix a nil pointer issue after bumping up to go1.22
More details see golang/go#65637

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 12, 2024
This commit bumps up the golang for building and testing velero to v1.22

It also updates controller-gen to v0.14.0 to fix an issue under new
versino of go.
More details see golang/go#65637

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 12, 2024
This commit bumps up the golang for building and testing velero to v1.22

It also updates controller-gen to v0.14.0 to fix an issue under new
versino of go.
More details see golang/go#65637

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 12, 2024
This commit bumps up the golang for building and testing velero to v1.22

It also updates controller-gen to v0.14.0 to fix an issue under new
versino of go.
More details see golang/go#65637

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 12, 2024
This commit bumps up the golang for building and testing velero to v1.22

It also updates controller-gen to v0.14.0 to fix an issue under new
versino of go.
More details see golang/go#65637

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 14, 2024
This commit bumps up the golang for building and testing velero to v1.22

It also updates controller-gen to v0.14.0 to fix an issue under new
versino of go.
More details see golang/go#65637

Signed-off-by: Daniel Jiang <[email protected]>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Apr 14, 2024
This commit bumps up the golang for building and testing velero to v1.22

It also updates controller-gen to v0.14.0 to fix an issue under new
versino of go.
More details see golang/go#65637

Signed-off-by: Daniel Jiang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants