Skip to content

✨ Handle panics during conversion more gracefully #3278

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

Jump to bottom
Merged
merged 1 commit into from
Aug 13, 2025
Merged

✨ Handle panics during conversion more gracefully #3278

merged 1 commit into from
Aug 13, 2025
+113 −11

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Aug 13, 2025 •
edited
Loading

Signed-off-by: Stefan Büringer [email protected]

Found an issue in conversion-gen a while ago that lead to panics in generated conversion code: kubernetes/kubernetes#132883

While hitting that issue I noticed that we don't handle panics in conversion code well today.

Behavior before this PR:

kubectl get

k get machinedeployments.v1beta1.cluster.x-k8s.io    capi-quickstart-md-0
Warning: cluster.x-k8s.io/v1beta1 MachineDeployment is deprecated; use cluster.x-k8s.io/v1beta2 MachineDeployment
Error from server: conversion webhook for cluster.x-k8s.io/v1beta2, Kind=MachineDeployment failed: Post "https://capi-webhook-service.capi-system.svc:443/convert?timeout=30s": EOF

controller logs

I0813 17:29:43.322918      73 ???:1] "http: panic serving 10.244.0.1:45849: test\ngoroutine 1121 [running]:\nnet/http.(*conn).serve.func1()\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:1947 +0x118\npanic({0x26dc660?, 0x2eb2590?})\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/runtime/panic.go:792 +0xf0\nsigs.k8s.io/cluster-api/api/core/v1beta1.(*MachineDeployment).ConvertFrom(0x4000768308, {0x2ed5560, 0x4000473888})\n\t/Users/buringerst/code/src/sigs.k8s.io/cluster-api/api/core/v1beta1/conversion.go:483 +0x5c\nsigs.k8s.io/controller-runtime/pkg/webhook/conversion.(*webhook).convertObject(0x400080ecc0, {0x2ec6060, 0x4000473888}, {0x2ec5de0, 0x4000768308})\n\t/Users/buringerst/code/pkg/mod/sigs.k8s.io/[email protected]/pkg/webhook/conversion/conversion.go:140 +0x3d4\nsigs.k8s.io/controller-runtime/pkg/webhook/conversion.(*webhook).handleConvertRequest(0x400080ecc0, 0x4000d90840)\n\t/Users/buringerst/code/pkg/mod/sigs.k8s.io/[email protected]/pkg/webhook/conversion/conversion.go:105 +0x290\nsigs.k8s.io/controller-runtime/pkg/webhook/conversion.(*webhook).ServeHTTP(0x400080ecc0, {0xf430a8242e10, 0x4000244af0}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/sigs.k8s.io/[email protected]/pkg/webhook/conversion/conversion.go:72 +0x1a0\ngithub.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerInFlight.func1({0xf430a8242e10, 0x4000244af0}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:60 +0x104\nnet/http.HandlerFunc.ServeHTTP(0x4000809b30, {0xf430a8242e10, 0x4000244af0}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2294 +0x40\ngithub.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerCounter.func1({0x2edff50, 0x40017fe700}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:147 +0xd0\nnet/http.HandlerFunc.ServeHTTP(0x4000809d40, {0x2edff50, 0x40017fe700}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2294 +0x40\ngithub.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerDuration.func2({0x2edff50, 0x40017fe700}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:109 +0x9c\nnet/http.HandlerFunc.ServeHTTP(0x400080db80, {0x2edff50, 0x40017fe700}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2294 +0x40\nnet/http.(*ServeMux).ServeHTTP(0x40005dd980, {0x2edff50, 0x40017fe700}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2822 +0x2dc\nnet/http.serverHandler.ServeHTTP({0x4000abc000}, {0x2edff50, 0x40017fe700}, 0x40002a32c0)\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:3301 +0x2b0\nnet/http.(*conn).serve(0x40011358c0, {0x2ee3b88, 0x4000244a00})\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2102 +0x16bc\ncreated by net/http.(*Server).Serve in goroutine 163\n\t/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:3454 +0x88c"

Behavior after this PR:

kubectl get

k get machinedeployments.v1beta1.cluster.x-k8s.io    capi-quickstart-md-0
Warning: cluster.x-k8s.io/v1beta1 MachineDeployment is deprecated; use cluster.x-k8s.io/v1beta2 MachineDeployment
Error from server: conversion webhook for cluster.x-k8s.io/v1beta2, Kind=MachineDeployment failed: panic occurred during conversion: test

controller logs

E0813 17:31:15.457852     127 runtime.go:140] "Observed a panic" panic="test" stacktrace=<
	goroutine 1121 [running]:
	k8s.io/apimachinery/pkg/util/runtime.logPanic({0x2ee3c88, 0x40004e41e0}, {0x26dc660, 0x2eb2680})
		/Users/buringerst/code/pkg/mod/k8s.io/[email protected]/pkg/util/runtime/runtime.go:132 +0xe4
	sigs.k8s.io/controller-runtime/pkg/webhook/conversion.(*webhook).handleConvertRequest.func1()
		/Users/buringerst/code/src/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion.go:101 +0xf8
	panic({0x26dc660?, 0x2eb2680?})
		/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/runtime/panic.go:792 +0xf0
	sigs.k8s.io/cluster-api/api/core/v1beta1.(*MachineDeployment).ConvertFrom(0x4000860308, {0x2ed5660, 0x4001124008})
		/Users/buringerst/code/src/sigs.k8s.io/cluster-api/api/core/v1beta1/conversion.go:483 +0x5c
	sigs.k8s.io/controller-runtime/pkg/webhook/conversion.(*webhook).convertObject(0x400089aa70, {0x2ec6160, 0x4001124008}, {0x2ec5ee0, 0x4000860308})
		/Users/buringerst/code/src/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion.go:156 +0x3d4
	sigs.k8s.io/controller-runtime/pkg/webhook/conversion.(*webhook).handleConvertRequest(0x400089aa70, {0x2ee3c88, 0x40004e41e0}, 0x400088c480)
		/Users/buringerst/code/src/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion.go:121 +0x3b4
	sigs.k8s.io/controller-runtime/pkg/webhook/conversion.(*webhook).ServeHTTP(0x400089aa70, {0xeb8c6a30e820, 0x40004e4280}, 0x4000a66000)
		/Users/buringerst/code/src/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion.go:77 +0x1b8
	github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerInFlight.func1({0xeb8c6a30e820, 0x40004e4280}, 0x4000a66000)
		/Users/buringerst/code/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:60 +0x104
	net/http.HandlerFunc.ServeHTTP(0x4000895860, {0xeb8c6a30e820, 0x40004e4280}, 0x4000a66000)
		/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2294 +0x40
	github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerCounter.func1({0x2ee0050, 0x400156c0e0}, 0x4000a66000)
		/Users/buringerst/code/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:147 +0xd0
	net/http.HandlerFunc.ServeHTTP(0x4000895a70, {0x2ee0050, 0x400156c0e0}, 0x4000a66000)
		/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2294 +0x40
	github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerDuration.func2({0x2ee0050, 0x400156c0e0}, 0x4000a66000)
		/Users/buringerst/code/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:109 +0x9c
	net/http.HandlerFunc.ServeHTTP(0x400089c600, {0x2ee0050, 0x400156c0e0}, 0x4000a66000)
		/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2294 +0x40
	net/http.(*ServeMux).ServeHTTP(0x4000315080, {0x2ee0050, 0x400156c0e0}, 0x4000a66000)
		/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2822 +0x2dc
	net/http.serverHandler.ServeHTTP({0x40001bc600}, {0x2ee0050, 0x400156c0e0}, 0x4000a66000)
		/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:3301 +0x2b0
	net/http.(*conn).serve(0x4000ad1560, {0x2ee3c88, 0x40004e4190})
		/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:2102 +0x16bc
	created by net/http.(*Server).Serve in goroutine 107
		/Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/net/http/server.go:3454 +0x88c
 >
E0813 17:31:15.457878     127 conversion.go:79] "failed to convert" err="panic occurred during conversion: test" logger="conversion-webhook" request="ba642d45-8ffb-4ec8-8484-64a65ea60807"

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 13, 2025
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 13, 2025
@sbueringer sbueringer changed the title [WIP] ✨ Handle panics during conversion more gracefully ✨ Handle panics during conversion more gracefully Aug 13, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2025
@sbueringer
Copy link
Member Author

/assign @alvaroaleman

/cc @fabriziopandini

alvaroaleman
alvaroaleman reviewed Aug 13, 2025
View reviewed changes
@sbueringer sbueringer force-pushed the pr-conversion-panics branch from c7cb69d to 4947ab9 Compare August 13, 2025 17:48
alvaroaleman
alvaroaleman reviewed Aug 13, 2025
View reviewed changes
@sbueringer sbueringer force-pushed the pr-conversion-panics branch from 4947ab9 to 336ab90 Compare August 13, 2025 17:56
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: c1aab0b5f823e1a89e968fdb27c3bfe57ddcd12f

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [alvaroaleman,sbueringer]

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 k8s-ci-robot merged commit c7df6d0 into kubernetes-sigs:main Aug 13, 2025
9 checks passed
@sbueringer sbueringer deleted the pr-conversion-panics branch August 13, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@FillZpp FillZpp Awaiting requested review from FillZpp

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@alvaroaleman alvaroaleman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sbueringer @k8s-ci-robot @alvaroaleman