Fix int/float conversion issues in CRD version remapping plugin (#2322)

* Add builders for CRD schemas

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add test case for #2319

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add failing test case

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Remove unnecessary print and temporary variable

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add some options for fixing the test case

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Switch to a JSON middle step to "fix" conversions

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add comment and changelog

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

changelogs/unreleased/2322-nrb

@@ -0,0 +1 @@
+Fix CRD backup failures when fields contained a whole number.

pkg/backup/remap_crd_version_action.go

@@ -17,6 +17,8 @@ limitations under the License.
 package backup
 
 import (
+	"encoding/json"
+
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -62,8 +64,18 @@ func (a *RemapCRDVersionAction) Execute(item runtime.Unstructured, backup *v1.Ba
 
 	// We've got a v1 CRD, so proceed.
 	var crd apiextv1.CustomResourceDefinition
-	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(item.UnstructuredContent(), &crd); err != nil {
-		return nil, nil, errors.Wrap(err, "unable to convert unstructured item to a v1 CRD")
+
+	// Do not use runtime.DefaultUnstructuredConverter.FromUnstructured here because it has a bug when converting integers/whole
+	// numbers in float fields (https://github.com/kubernetes/kubernetes/issues/87675).
+	// Using JSON as a go-between avoids this issue, without adding a bunch of type conversion by using unstructured helper functions
+	// to inspect the fields we want to look at.
+	js, err := json.Marshal(item.UnstructuredContent())
+	if err != nil {
+		return nil, nil, errors.Wrap(err, "unable to convert unstructured item to JSON")
+	}
+
+	if err = json.Unmarshal(js, &crd); err != nil {
+		return nil, nil, errors.Wrap(err, "unable to convert JSON to CRD Go type")
 	}
 
 	log := a.logger.WithField("plugin", "RemapCRDVersionAction").WithField("CRD", crd.Name)

pkg/backup/remap_crd_version_action_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package backup
 
 import (
+	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -58,4 +59,24 @@ func TestRemapCRDVersionAction(t *testing.T) {
 		require.NoError(t, err)
 		assert.Equal(t, "apiextensions.k8s.io/v1beta1", item.UnstructuredContent()["apiVersion"])
 	})
+
+	t.Run("Having an integer on a float64 field should work (issue 2319)", func(t *testing.T) {
+		b := builder.ForV1CustomResourceDefinition("test.velero.io")
+		// 5 here is just an int value, it could be any other whole number.
+		schema := builder.ForJSONSchemaPropsBuilder().Maximum(5).Result()
+		b.Version(builder.ForV1CustomResourceDefinitionVersion("v1").Served(true).Storage(true).Schema(schema).Result())
+		c := b.Result()
+
+		// Marshall in and out of JSON because the problem doesn't manifest when we use ToUnstructured directly
+		// This should simulate the JSON passing over the wire in an HTTP request/response with a dynamic client
+		js, err := json.Marshal(c)
+		require.NoError(t, err)
+
+		var u unstructured.Unstructured
+		err = json.Unmarshal(js, &u)
+		require.NoError(t, err)
+
+		_, _, err = a.Execute(&u, backup)
+		require.NoError(t, err)
+	})
 }

pkg/builder/json_schema_props_builder.go

@@ -0,0 +1,43 @@
+/*
+Copyright 2020 the Velero contributors.
+
+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 builder
+
+import (
+	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+)
+
+// JSONSchemaPropsBuilder builds JSONSchemaProps objects.
+type JSONSchemaPropsBuilder struct {
+	object *apiextv1.JSONSchemaProps
+}
+
+// ForJSONSchemaPropsBuilder is the constructor for a JSONSchemaPropsBuilder.
+func ForJSONSchemaPropsBuilder() *JSONSchemaPropsBuilder {
+	return &JSONSchemaPropsBuilder{
+		object: &apiextv1.JSONSchemaProps{},
+	}
+}
+
+// Maximum sets the Maximum field on a JSONSchemaPropsBuilder object.
+func (b *JSONSchemaPropsBuilder) Maximum(f float64) *JSONSchemaPropsBuilder {
+	b.object.Maximum = &f
+	return b
+}
+
+// Result returns the built object.
+func (b *JSONSchemaPropsBuilder) Result() *apiextv1.JSONSchemaProps {
+	return b.object
+}

pkg/builder/v1_customresourcedefinition_builder.go

@@ -126,6 +126,14 @@ func (b *V1CustomResourceDefinitionVersionBuilder) Storage(s bool) *V1CustomReso
 	return b
 }
 
+func (b *V1CustomResourceDefinitionVersionBuilder) Schema(s *apiextv1.JSONSchemaProps) *V1CustomResourceDefinitionVersionBuilder {
+	if b.object.Schema == nil {
+		b.object.Schema = new(apiextv1.CustomResourceValidation)
+	}
+	b.object.Schema.OpenAPIV3Schema = s
+	return b
+}
+
 // Result returns the built CustomResourceDefinitionVersion.
 func (b *V1CustomResourceDefinitionVersionBuilder) Result() apiextv1.CustomResourceDefinitionVersion {
 	return b.object

pkg/util/kube/utils.go

@@ -157,6 +157,9 @@ func IsCRDReady(crd *apiextv1beta1.CustomResourceDefinition) bool {
 // TODO: Delete this function and use IsCRDReady when the upstream runtime.FromUnstructured function properly handles int64 field conversions.
 // Duplicated function because the velero install package uses IsCRDReady with the beta types.
 // See https://github.com/kubernetes/kubernetes/issues/87675
+// This is different from the fix for https://github.com/vmware-tanzu/velero/issues/2319 because here,
+// we need to account for *both* v1beta1 and v1 CRDs, so doing marshalling into JSON to convert to a Go type may not be as useful here, unless we do
+// type switching.
 func IsUnstructuredCRDReady(crd *unstructured.Unstructured) (bool, error) {
 	var isEstablished, namesAccepted bool