Revert "CR conversion: protect from converter input edits"

This reverts commit f14cc7fdfcfcedafc7910f043ec6eb74930cfee7. Kubernetes-commit: 1da781e29b36e3e3938df4fab13d2986cf728cc4
kubernetes · Apr 13, 2023 · 3735b65 · 3735b65
1 parent 2c580f6
commit 3735b65
Show file tree

Hide file tree

Showing 2 changed files with 3 additions and 88 deletions.
diff --git a/pkg/apiserver/conversion/converter.go b/pkg/apiserver/conversion/converter.go
@@ -245,11 +245,6 @@ func (c *delegatingCRConverter) ConvertToVersion(in runtime.Object, target runti
 		return converted, nil
 	}
 
-	// Deep copy the list before we invoke the converter to ensure that if the converter does mutate the
-	// list (which it shouldn't, but you never know), it doesn't have any impact.
-	convertedList := list.DeepCopy()
-	convertedList.SetAPIVersion(desiredAPIVersion)
-
 	convertedObjects, err := c.converter.Convert(list, toGVK.GroupVersion())
 	if err != nil {
 		return nil, fmt.Errorf("conversion for %v failed: %w", in.GetObjectKind().GroupVersionKind(), err)
@@ -258,8 +253,10 @@ func (c *delegatingCRConverter) ConvertToVersion(in runtime.Object, target runti
 		return nil, fmt.Errorf("conversion for %v returned %d objects, expected %d", in.GetObjectKind().GroupVersionKind(), len(convertedObjects.Items), len(objectsToConvert))
 	}
 
-	// Fill in the converted objects from the response at the right spots.
+	// start a deepcopy of the input and fill in the converted objects from the response at the right spots.
 	// The response list might be sparse because objects had the right version already.
+	convertedList := list.DeepCopy()
+	convertedList.SetAPIVersion(desiredAPIVersion)
 	convertedIndex := 0
 	for i := range convertedList.Items {
 		original := &convertedList.Items[i]

diff --git a/pkg/apiserver/conversion/converter_test.go b/pkg/apiserver/conversion/converter_test.go
@@ -46,15 +46,13 @@ func TestConversion(t *testing.T) {
 			SourceObject: &unstructured.Unstructured{
 				Object: map[string]interface{}{
 					"apiVersion": "example.com/v1",
-					"metadata":   map[string]interface{}{},
 					"other":      "data",
 					"kind":       "foo",
 				},
 			},
 			ExpectedObject: &unstructured.Unstructured{
 				Object: map[string]interface{}{
 					"apiVersion": "example.com/v2",
-					"metadata":   map[string]interface{}{},
 					"other":      "data",
 					"kind":       "foo",
 				},
@@ -88,15 +86,13 @@ func TestConversion(t *testing.T) {
 					{
 						Object: map[string]interface{}{
 							"apiVersion": "example.com/v1",
-							"metadata":   map[string]interface{}{},
 							"kind":       "foo",
 							"other":      "data",
 						},
 					},
 					{
 						Object: map[string]interface{}{
 							"apiVersion": "example.com/v1",
-							"metadata":   map[string]interface{}{},
 							"kind":       "foo",
 							"other":      "data2",
 						},
@@ -112,15 +108,13 @@ func TestConversion(t *testing.T) {
 					{
 						Object: map[string]interface{}{
 							"apiVersion": "example.com/v2",
-							"metadata":   map[string]interface{}{},
 							"kind":       "foo",
 							"other":      "data",
 						},
 					},
 					{
 						Object: map[string]interface{}{
 							"apiVersion": "example.com/v2",
-							"metadata":   map[string]interface{}{},
 							"kind":       "foo",
 							"other":      "data2",
 						},
@@ -294,79 +288,3 @@ func TestGetObjectsToConvert(t *testing.T) {
 		})
 	}
 }
-
-func TestConverterMutatesInput(t *testing.T) {
-	testCRD := apiextensionsv1.CustomResourceDefinition{
-		Spec: apiextensionsv1.CustomResourceDefinitionSpec{
-			Conversion: &apiextensionsv1.CustomResourceConversion{
-				Strategy: apiextensionsv1.NoneConverter,
-			},
-			Group: "test.k8s.io",
-			Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
-				{
-					Name:   "v1alpha1",
-					Served: true,
-				},
-				{
-					Name:   "v1alpha2",
-					Served: true,
-				},
-			},
-		},
-	}
-
-	safeConverter, _, err := NewDelegatingConverter(&testCRD, &inputMutatingConverter{})
-	if err != nil {
-		t.Fatalf("Cannot create converter: %v", err)
-	}
-
-	input := &unstructured.UnstructuredList{
-		Object: map[string]interface{}{
-			"apiVersion": "test.k8s.io/v1alpha1",
-		},
-		Items: []unstructured.Unstructured{
-			{
-				Object: map[string]interface{}{
-					"apiVersion": "test.k8s.io/v1alpha1",
-					"metadata": map[string]interface{}{
-						"name": "item1",
-					},
-				},
-			},
-			{
-				Object: map[string]interface{}{
-					"apiVersion": "test.k8s.io/v1alpha1",
-					"metadata": map[string]interface{}{
-						"name": "item2",
-					},
-				},
-			},
-		},
-	}
-
-	toVersion, _ := schema.ParseGroupVersion("test.k8s.io/v1alpha2")
-	toVersions := schema.GroupVersions{toVersion}
-	converted, err := safeConverter.ConvertToVersion(input, toVersions)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	convertedList := converted.(*unstructured.UnstructuredList)
-	if e, a := 2, len(convertedList.Items); e != a {
-		t.Fatalf("length: expected %d, got %d", e, a)
-	}
-}
-
-type inputMutatingConverter struct{}
-
-func (i *inputMutatingConverter) Convert(in *unstructured.UnstructuredList, targetGVK schema.GroupVersion) (*unstructured.UnstructuredList, error) {
-	out := &unstructured.UnstructuredList{}
-	for _, obj := range in.Items {
-		u := obj.DeepCopy()
-		u.SetAPIVersion(targetGVK.String())
-		out.Items = append(out.Items, *u)
-	}
-
-	in.Items = nil
-
-	return out, nil
-}