ref(crdconvert): respond with errors from all requested resources (#4601

)

Currently, CRD conversion webhooks only respond with the error returned
by the first failed resource converted, but there may be several. This
change aggregates errors from all resources sent in a request.

Signed-off-by: Jon Huhn <[email protected]>

pkg/crdconversion/config_meshconfig_conversion.go

@@ -3,7 +3,7 @@ package crdconversion
 import (
 	"net/http"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -14,12 +14,12 @@ func serveMeshConfigConversion(w http.ResponseWriter, r *http.Request) {
 
 // convertMeshConfig contains the business logic to convert meshconfigs.config.openservicemesh.io CRD
 // Example implementation reference : https://github.com/kubernetes/kubernetes/blob/release-1.22/test/images/agnhost/crd-conversion-webhook/converter/example_converter.go
-func convertMeshConfig(obj *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, metav1.Status) {
+func convertMeshConfig(obj *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, error) {
 	convertedObject := obj.DeepCopy()
 	fromVersion := obj.GetAPIVersion()
 
 	if toVersion == fromVersion {
-		return nil, statusErrorWithMessage("MeshConfig: conversion from a version to itself should not call the webhook: %s", toVersion)
+		return nil, errors.Errorf("MeshConfig: conversion from a version to itself should not call the webhook: %s", toVersion)
 	}
 
 	log.Debug().Msgf("MeshConfig conversion request: from-version=%s, to-version=%s", fromVersion, toVersion)
@@ -32,7 +32,7 @@ func convertMeshConfig(obj *unstructured.Unstructured, toVersion string) (*unstr
 			// necessary at this moment.
 
 		default:
-			return nil, statusErrorWithMessage("Unexpected conversion to-version for MeshConfig resource: %s", toVersion)
+			return nil, errors.Errorf("Unexpected conversion to-version for MeshConfig resource: %s", toVersion)
 		}
 
 	case "config.openservicemesh.io/v1alpha2":
@@ -55,13 +55,13 @@ func convertMeshConfig(obj *unstructured.Unstructured, toVersion string) (*unstr
 				}
 			}
 		default:
-			return nil, statusErrorWithMessage("Unexpected conversion to-version for MeshConfig resource: %s", toVersion)
+			return nil, errors.Errorf("Unexpected conversion to-version for MeshConfig resource: %s", toVersion)
 		}
 
 	default:
-		return nil, statusErrorWithMessage("Unexpected conversion from-version for MeshConfig resource: %s", fromVersion)
+		return nil, errors.Errorf("Unexpected conversion from-version for MeshConfig resource: %s", fromVersion)
 	}
 
 	log.Debug().Msg("MeshConfig: successfully converted object")
-	return convertedObject, statusSucceed()
+	return convertedObject, nil
 }

pkg/crdconversion/config_meshconfig_conversion_test.go

@@ -17,7 +17,7 @@ func TestConvertMeshConfig(t *testing.T) {
 		name      string
 		request   runtime.Object
 		toVersion string
-		verifyFn  func(*assert.Assertions, *unstructured.Unstructured, metav1.Status)
+		verifyFn  func(*assert.Assertions, *unstructured.Unstructured, error)
 	}{
 		{
 			name: "v1alpha2 -> v1alpha1 should remove additional field",
@@ -39,8 +39,8 @@ func TestConvertMeshConfig(t *testing.T) {
 				},
 			},
 			toVersion: "config.openservicemesh.io/v1alpha1",
-			verifyFn: func(a *assert.Assertions, converted *unstructured.Unstructured, status metav1.Status) {
-				a.Equal(status, statusSucceed())
+			verifyFn: func(a *assert.Assertions, converted *unstructured.Unstructured, err error) {
+				a.NoError(err)
 
 				unsupportedFields := [][]string{
 					{"spec", "traffic", "outboundIPRangeInclusionList"},
@@ -66,8 +66,8 @@ func TestConvertMeshConfig(t *testing.T) {
 				Spec: configv1alpha1.MeshConfigSpec{},
 			},
 			toVersion: "config.openservicemesh.io/v1alpha2",
-			verifyFn: func(a *assert.Assertions, converted *unstructured.Unstructured, status metav1.Status) {
-				a.Equal(status, statusSucceed())
+			verifyFn: func(a *assert.Assertions, converted *unstructured.Unstructured, err error) {
+				a.NoError(err)
 			},
 		},
 	}

pkg/crdconversion/config_multiclusterservice_conversion.go

@@ -3,7 +3,7 @@ package crdconversion
 import (
 	"net/http"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	"github.com/openservicemesh/osm/pkg/constants"
@@ -16,14 +16,14 @@ func serveMultiClusterServiceConversion(w http.ResponseWriter, r *http.Request)
 
 // convertMultiClusterService contains the business logic to convert multiclusterservices.config.openservicemesh.io CRD
 // Example implementation reference : https://github.com/kubernetes/kubernetes/blob/release-1.22/test/images/agnhost/crd-conversion-webhook/converter/example_converter.go
-func convertMultiClusterService(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, metav1.Status) {
+func convertMultiClusterService(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, error) {
 	convertedObject := Object.DeepCopy()
 	fromVersion := Object.GetAPIVersion()
 
 	if toVersion == fromVersion {
-		return nil, statusErrorWithMessage("MultiClusterService: conversion from a version to itself should not call the webhook: %s", toVersion)
+		return nil, errors.Errorf("MultiClusterService: conversion from a version to itself should not call the webhook: %s", toVersion)
 	}
 
 	log.Debug().Str(constants.LogFieldContext, constants.LogContextMulticluster).Msg("MultiClusterService: successfully converted object")
-	return convertedObject, statusSucceed()
+	return convertedObject, nil
 }

pkg/crdconversion/framework.go

@@ -30,7 +30,7 @@ import (
 
 // convertFunc is the user defined function for any conversion. The code in this file is a
 // template that can be use for any CR conversion given this function.
-type convertFunc func(Object *unstructured.Unstructured, version string) (*unstructured.Unstructured, metav1.Status)
+type convertFunc func(Object *unstructured.Unstructured, version string) (*unstructured.Unstructured, error)
 
 // conversionResponseFailureWithMessagef is a helper function to create an AdmissionResponse
 // with a formatted embedded error message.
@@ -43,43 +43,46 @@ func conversionResponseFailureWithMessagef(msg string, params ...interface{}) *v
 	}
 }
 
-func statusErrorWithMessage(msg string, params ...interface{}) metav1.Status {
-	return metav1.Status{
-		Message: fmt.Sprintf(msg, params...),
-		Status:  metav1.StatusFailure,
-	}
-}
-
-func statusSucceed() metav1.Status {
-	return metav1.Status{
-		Status: metav1.StatusSuccess,
-	}
-}
-
 // doConversion converts the requested object given the conversion function and returns a conversion response.
 // failures will be reported as Reason in the conversion response.
 func doConversion(convertRequest *v1beta1.ConversionRequest, convert convertFunc) *v1beta1.ConversionResponse {
 	var convertedObjects []runtime.RawExtension
+	// aggregate errors from all objects in the request vs. only returning the
+	// first so errors from all objects are sent in the request and metrics are
+	// recorded as accurately as possible.
+	var errs []string
 	for _, obj := range convertRequest.Objects {
 		cr := unstructured.Unstructured{}
 		if err := cr.UnmarshalJSON(obj.Raw); err != nil {
 			log.Error().Err(err).Msg("error unmarshalling object JSON")
-			return conversionResponseFailureWithMessagef("failed to unmarshall object (%v) with error: %v", string(obj.Raw), err)
+			errs = append(errs, fmt.Sprintf("failed to unmarshal object (%v) with error: %v", string(obj.Raw), err))
+			continue
 		}
-		convertedCR, status := convert(&cr, convertRequest.DesiredAPIVersion)
-		if status.Status != metav1.StatusSuccess {
-			log.Error().Msgf(status.String())
-			return &v1beta1.ConversionResponse{
-				Result: status,
-			}
+		convertedCR, err := convert(&cr, convertRequest.DesiredAPIVersion)
+		if err != nil {
+			log.Error().Err(err).Msg("conversion failed")
+			errs = append(errs, err.Error())
+			continue
 		}
 		convertedCR.SetAPIVersion(convertRequest.DesiredAPIVersion)
 		convertedObjects = append(convertedObjects, runtime.RawExtension{Object: convertedCR})
 	}
-	return &v1beta1.ConversionResponse{
+
+	resp := &v1beta1.ConversionResponse{
 		ConvertedObjects: convertedObjects,
-		Result:           statusSucceed(),
+		Result: metav1.Status{
+			Status: metav1.StatusSuccess,
+		},
+	}
+	if len(errs) > 0 {
+		resp = &v1beta1.ConversionResponse{
+			Result: metav1.Status{
+				Message: strings.Join(errs, "; "),
+				Status:  metav1.StatusFailure,
+			},
+		}
 	}
+	return resp
 }
 
 func serve(w http.ResponseWriter, r *http.Request, convert convertFunc) {

pkg/crdconversion/framework_test.go

@@ -0,0 +1,97 @@
+package crdconversion
+
+import (
+	"bytes"
+	"encoding/json"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+func TestServe(t *testing.T) {
+	runServe := func(req *v1beta1.ConversionRequest, convert convertFunc) (*httptest.ResponseRecorder, *v1beta1.ConversionReview) {
+		j, err := json.Marshal(&v1beta1.ConversionReview{
+			Request: req,
+		})
+		require.NoError(t, err)
+		body := bytes.NewBuffer(j)
+		r := httptest.NewRequest(http.MethodPost, "http://this.doesnt/matter", body)
+		r.Header.Add("Content-Type", "application/json")
+		r.Header.Add("Accept", "application/json")
+		w := httptest.NewRecorder()
+
+		serve(w, r, convert)
+
+		res := new(v1beta1.ConversionReview)
+		err = json.Unmarshal(w.Body.Bytes(), res)
+		require.NoError(t, err)
+
+		return w, res
+	}
+
+	req := &v1beta1.ConversionRequest{
+		DesiredAPIVersion: "any.group/v1",
+		Objects: []runtime.RawExtension{
+			{
+				Raw: []byte(`{
+						"apiVersion": "any.group/v2",
+						"kind": "SomeKind"
+					}`),
+			},
+			{
+				Raw: []byte(`{
+						"apiVersion": "any.group/v3",
+						"kind": "SomeKind"
+					}`),
+			},
+		},
+	}
+	failConvert := func(*unstructured.Unstructured, string) (*unstructured.Unstructured, error) {
+		return nil, errors.New("fail")
+	}
+	okConvert := func(in *unstructured.Unstructured, _ string) (*unstructured.Unstructured, error) {
+		return in.DeepCopy(), nil
+	}
+	v2FailV3OkConvert := func(in *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, error) {
+		switch in.GetAPIVersion() {
+		case "any.group/v2":
+			return failConvert(in, toVersion)
+		case "any.group/v3":
+			return okConvert(in, toVersion)
+		}
+		panic("unexpected API version")
+	}
+
+	a := assert.New(t)
+
+	// ok conversion
+	w, res := runServe(req, okConvert)
+
+	a.Equal(http.StatusOK, w.Result().StatusCode)
+	a.Equal(metav1.StatusSuccess, res.Response.Result.Status)
+	a.Len(res.Response.ConvertedObjects, 2)
+
+	// failing conversion
+	w, res = runServe(req, failConvert)
+
+	a.Equal(http.StatusOK, w.Result().StatusCode)
+	a.Equal(metav1.StatusFailure, res.Response.Result.Status)
+	a.Len(res.Response.ConvertedObjects, 0)
+	a.Equal("fail; fail", res.Response.Result.Message)
+
+	// partially successful conversion
+	w, res = runServe(req, v2FailV3OkConvert)
+
+	a.Equal(http.StatusOK, w.Result().StatusCode)
+	a.Equal(metav1.StatusFailure, res.Response.Result.Status)
+	a.Len(res.Response.ConvertedObjects, 0)
+	a.Equal("fail", res.Response.Result.Message)
+}

pkg/crdconversion/policy_egress_conversion.go

@@ -3,7 +3,7 @@ package crdconversion
 import (
 	"net/http"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -14,14 +14,14 @@ func serveEgressPolicyConversion(w http.ResponseWriter, r *http.Request) {
 
 // convertEgressPolicy contains the business logic to convert egresses.policy.openservicemesh.io CRD
 // Example implementation reference : https://github.com/kubernetes/kubernetes/blob/release-1.22/test/images/agnhost/crd-conversion-webhook/converter/example_converter.go
-func convertEgressPolicy(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, metav1.Status) {
+func convertEgressPolicy(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, error) {
 	convertedObject := Object.DeepCopy()
 	fromVersion := Object.GetAPIVersion()
 
 	if toVersion == fromVersion {
-		return nil, statusErrorWithMessage("EgressPolicy: conversion from a version to itself should not call the webhook: %s", toVersion)
+		return nil, errors.Errorf("EgressPolicy: conversion from a version to itself should not call the webhook: %s", toVersion)
 	}
 
 	log.Debug().Msg("EgressPolicy: successfully converted object")
-	return convertedObject, statusSucceed()
+	return convertedObject, nil
 }

pkg/crdconversion/policy_ingressbackends_conversion.go

@@ -3,7 +3,7 @@ package crdconversion
 import (
 	"net/http"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -14,14 +14,14 @@ func serveIngressBackendsPolicyConversion(w http.ResponseWriter, r *http.Request
 
 // convertIngressBackendsPolicy contains the business logic to convert ingressbackends.policy.openservicemesh.io CRD
 // Example implementation reference : https://github.com/kubernetes/kubernetes/blob/release-1.22/test/images/agnhost/crd-conversion-webhook/converter/example_converter.go
-func convertIngressBackendsPolicy(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, metav1.Status) {
+func convertIngressBackendsPolicy(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, error) {
 	convertedObject := Object.DeepCopy()
 	fromVersion := Object.GetAPIVersion()
 
 	if toVersion == fromVersion {
-		return nil, statusErrorWithMessage("IngressBackendsPolicy: conversion from a version to itself should not call the webhook: %s", toVersion)
+		return nil, errors.Errorf("IngressBackendsPolicy: conversion from a version to itself should not call the webhook: %s", toVersion)
 	}
 
 	log.Debug().Msg("IngressBackendsPolicy: successfully converted object")
-	return convertedObject, statusSucceed()
+	return convertedObject, nil
 }

pkg/crdconversion/policy_retry_conversion.go

@@ -3,7 +3,7 @@ package crdconversion
 import (
 	"net/http"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -14,14 +14,14 @@ func serveRetryPolicyConversion(w http.ResponseWriter, r *http.Request) {
 
 // convertRetryPolicy contains the business logic to convert retries.policy.openservicemesh.io CRD
 // Example implementation reference : https://github.com/kubernetes/kubernetes/blob/release-1.22/test/images/agnhost/crd-conversion-webhook/converter/example_converter.go
-func convertRetryPolicy(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, metav1.Status) {
+func convertRetryPolicy(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, error) {
 	convertedObject := Object.DeepCopy()
 	fromVersion := Object.GetAPIVersion()
 
 	if toVersion == fromVersion {
-		return nil, statusErrorWithMessage("RetryPolicy: conversion from a version to itself should not call the webhook: %s", toVersion)
+		return nil, errors.Errorf("RetryPolicy: conversion from a version to itself should not call the webhook: %s", toVersion)
 	}
 
 	log.Debug().Msg("RetryPolicy: successfully converted object")
-	return convertedObject, statusSucceed()
+	return convertedObject, nil
 }

pkg/crdconversion/smi_httproutegroup_conversion.go

@@ -3,7 +3,7 @@ package crdconversion
 import (
 	"net/http"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -14,14 +14,14 @@ func serveHTTPRouteGroupConversion(w http.ResponseWriter, r *http.Request) {
 
 // convertEgressPolicy contains the business logic to convert httproutegroups.specs.smi-spec.io CRD
 // Example implementation reference : https://github.com/kubernetes/kubernetes/blob/release-1.22/test/images/agnhost/crd-conversion-webhook/converter/example_converter.go
-func convertHTTPRouteGroup(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, metav1.Status) {
+func convertHTTPRouteGroup(Object *unstructured.Unstructured, toVersion string) (*unstructured.Unstructured, error) {
 	convertedObject := Object.DeepCopy()
 	fromVersion := Object.GetAPIVersion()
 
 	if toVersion == fromVersion {
-		return nil, statusErrorWithMessage("HTTPRouteGroup: conversion from a version to itself should not call the webhook: %s", toVersion)
+		return nil, errors.Errorf("HTTPRouteGroup: conversion from a version to itself should not call the webhook: %s", toVersion)
 	}
 
 	log.Debug().Msg("HTTPRouteGroup: successfully converted object")
-	return convertedObject, statusSucceed()
+	return convertedObject, nil
 }