Merge pull request #3278 from sbueringer/pr-conversion-panics

✨ Handle panics during conversion more gracefully

Author: k8s-ci-robot

pkg/webhook/conversion/conversion.go

@@ -22,7 +22,9 @@ See pkg/conversion for interface definitions required to ensure an API Type is c
 package conversion
 
 import (
+	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -31,8 +33,10 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
+	conversionmetrics "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/metrics"
 )
 
 var (
@@ -53,6 +57,8 @@ type webhook struct {
 var _ http.Handler = &webhook{}
 
 func (wh *webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	ctx := r.Context()
+
 	convertReview := &apix.ConversionReview{}
 	err := json.NewDecoder(r.Body).Decode(convertReview)
 	if err != nil {
@@ -69,7 +75,7 @@ func (wh *webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	// TODO(droot): may be move the conversion logic to a separate module to
 	// decouple it from the http layer ?
-	resp, err := wh.handleConvertRequest(convertReview.Request)
+	resp, err := wh.handleConvertRequest(ctx, convertReview.Request)
 	if err != nil {
 		log.Error(err, "failed to convert", "request", convertReview.Request.UID)
 		convertReview.Response = errored(err)
@@ -87,7 +93,18 @@ func (wh *webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 }
 
 // handles a version conversion request.
-func (wh *webhook) handleConvertRequest(req *apix.ConversionRequest) (*apix.ConversionResponse, error) {
+func (wh *webhook) handleConvertRequest(ctx context.Context, req *apix.ConversionRequest) (_ *apix.ConversionResponse, retErr error) {
+	defer func() {
+		if r := recover(); r != nil {
+			conversionmetrics.WebhookPanics.WithLabelValues().Inc()
+
+			for _, fn := range utilruntime.PanicHandlers {
+				fn(ctx, r)
+			}
+			retErr = errors.New("internal error occurred during conversion")
+			return
+		}
+	}()
 	if req == nil {
 		return nil, fmt.Errorf("conversion request is nil")
 	}

pkg/webhook/conversion/conversion_test.go

@@ -296,6 +296,29 @@ var _ = Describe("Conversion Webhook", func() {
 		Expect(convReview.Response.ConvertedObjects).To(BeEmpty())
 	})
 
+	It("should return error on panic in conversion", func() {
+
+		v1Obj := makeV1Obj()
+		v1Obj.Spec.PanicInConversion = true
+
+		convReq := &apix.ConversionReview{
+			TypeMeta: metav1.TypeMeta{},
+			Request: &apix.ConversionRequest{
+				DesiredAPIVersion: "jobs.testprojects.kb.io/v3",
+				Objects: []runtime.RawExtension{
+					{
+						Object: v1Obj,
+					},
+				},
+			},
+		}
+
+		convReview := doRequest(convReq)
+
+		Expect(convReview.Response.ConvertedObjects).To(HaveLen(0))
+		Expect(convReview.Response.Result.Status).To(Equal(metav1.StatusFailure))
+		Expect(convReview.Response.Result.Message).To(Equal("internal error occurred during conversion"))
+	})
 })
 
 var _ = Describe("IsConvertible", func() {

pkg/webhook/conversion/metrics/metrics.go

@@ -0,0 +1,39 @@
+/*
+Copyright 2025 The Kubernetes Authors.
+
+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.
+*/
+
+package metrics
+
+import (
+	"github.com/prometheus/client_golang/prometheus"
+	"sigs.k8s.io/controller-runtime/pkg/metrics"
+)
+
+var (
+	// WebhookPanics is a prometheus counter metrics which holds the total
+	// number of panics from conversion webhooks.
+	WebhookPanics = prometheus.NewCounterVec(prometheus.CounterOpts{
+		Name: "controller_runtime_conversion_webhook_panics_total",
+		Help: "Total number of conversion webhook panics",
+	}, []string{})
+)
+
+func init() {
+	metrics.Registry.MustRegister(
+		WebhookPanics,
+	)
+	// Init metric.
+	WebhookPanics.WithLabelValues().Add(0)
+}

pkg/webhook/conversion/testdata/api/v1/externaljob_types.go

@@ -17,6 +17,7 @@ package v1
 
 import (
 	"fmt"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 
@@ -31,6 +32,9 @@ type ExternalJobSpec struct {
 	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
 	// Important: Run "make" to regenerate code after modifying this file
 	RunAt string `json:"runAt"`
+
+	// PanicInConversion triggers a panic during conversion when set to true.
+	PanicInConversion bool `json:"panicInConversion"`
 }
 
 // ExternalJobStatus defines the observed state of ExternalJob
@@ -66,6 +70,9 @@ func init() {
 // ConvertTo implements conversion logic to convert to Hub type (v2.ExternalJob
 // in this case)
 func (ej *ExternalJob) ConvertTo(dst conversion.Hub) error {
+	if ej.Spec.PanicInConversion {
+		panic("PanicInConversion field set to true")
+	}
 	switch t := dst.(type) {
 	case *v2.ExternalJob:
 		jobv2 := dst.(*v2.ExternalJob)
@@ -80,6 +87,9 @@ func (ej *ExternalJob) ConvertTo(dst conversion.Hub) error {
 // ConvertFrom implements conversion logic to convert from Hub type (v2.ExternalJob
 // in this case)
 func (ej *ExternalJob) ConvertFrom(src conversion.Hub) error {
+	if ej.Spec.PanicInConversion {
+		panic("PanicInConversion field set to true")
+	}
 	switch t := src.(type) {
 	case *v2.ExternalJob:
 		jobv2 := src.(*v2.ExternalJob)

pkg/webhook/conversion/testdata/api/v1/zz_generated.deepcopy.go

@@ -27,6 +27,9 @@ type ExternalJobSpec struct {
 	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
 	// Important: Run "make" to regenerate code after modifying this file
 	ScheduleAt string `json:"scheduleAt"`
+
+	// PanicInConversion triggers a panic during conversion when set to true.
+	PanicInConversion bool `json:"panicInConversion"`
 }
 
 // ExternalJobStatus defines the observed state of ExternalJob

pkg/webhook/conversion/testdata/api/v2/externaljob_types.go

@@ -17,6 +17,7 @@ package v3
 
 import (
 	"fmt"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 
@@ -31,6 +32,9 @@ type ExternalJobSpec struct {
 	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
 	// Important: Run "make" to regenerate code after modifying this file
 	DeferredAt string `json:"deferredAt"`
+
+	// PanicInConversion triggers a panic during conversion when set to true.
+	PanicInConversion bool `json:"panicInConversion"`
 }
 
 // ExternalJobStatus defines the observed state of ExternalJob
@@ -66,6 +70,9 @@ func init() {
 // ConvertTo implements conversion logic to convert to Hub type (v2.ExternalJob
 // in this case)
 func (ej *ExternalJob) ConvertTo(dst conversion.Hub) error {
+	if ej.Spec.PanicInConversion {
+		panic("PanicInConversion field set to true")
+	}
 	switch t := dst.(type) {
 	case *v2.ExternalJob:
 		jobv2 := dst.(*v2.ExternalJob)
@@ -80,6 +87,9 @@ func (ej *ExternalJob) ConvertTo(dst conversion.Hub) error {
 // ConvertFrom implements conversion logic to convert from Hub type (v2.ExternalJob
 // in this case)
 func (ej *ExternalJob) ConvertFrom(src conversion.Hub) error {
+	if ej.Spec.PanicInConversion {
+		panic("PanicInConversion field set to true")
+	}
 	switch t := src.(type) {
 	case *v2.ExternalJob:
 		jobv2 := src.(*v2.ExternalJob)