Backup entire v1beta1 CRD instead of just changing version string (#2478)

* Switch to backing up v1beta1 CRDs from API server

Instead of simply switching out the APIVersion string on a v1
CustomResourceDefinition object, re-download the object from the API
server entirely to get the correct fields.

This should fix validation errors upon restore.

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

* Fix existing tests

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

* Add full example CRDs to automated tests

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

* Move beta CRD lookup into helper function

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

* Add case for preserveUnknownFields CRDs

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

* Add PreserveUnknownFields case and refactor execute

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

* Add older prometheus CRD test cases

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

* Add changelog

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

changelogs/unreleased/2478-nrb

@@ -0,0 +1 @@
+At backup time, if a CustomResourceDefinition appears to have been created via the v1beta1 endpoint, retrieve it from the v1beta1 endpoint instead of simply changing the APIVersion.

pkg/backup/remap_crd_version_action.go

@@ -22,22 +22,28 @@ import (
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+
+	apiextv1beta1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 
 	v1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	"github.com/vmware-tanzu/velero/pkg/kuberesource"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
 )
 
 // RemapCRDVersionAction inspects CustomResourceDefinition and decides if it is a v1
 // CRD that needs to be backed up as v1beta1.
 type RemapCRDVersionAction struct {
-	logger logrus.FieldLogger
+	logger        logrus.FieldLogger
+	betaCRDClient apiextv1beta1client.CustomResourceDefinitionInterface
 }
 
 // NewRemapCRDVersionAction instantiates a new RemapCRDVersionAction plugin.
-func NewRemapCRDVersionAction(logger logrus.FieldLogger) *RemapCRDVersionAction {
-	return &RemapCRDVersionAction{logger: logger}
+func NewRemapCRDVersionAction(logger logrus.FieldLogger, betaCRDClient apiextv1beta1client.CustomResourceDefinitionInterface) *RemapCRDVersionAction {
+	return &RemapCRDVersionAction{logger: logger, betaCRDClient: betaCRDClient}
 }
 
 // AppliesTo selects the resources the plugin should run against. In this case, CustomResourceDefinitions.
@@ -80,6 +86,62 @@ func (a *RemapCRDVersionAction) Execute(item runtime.Unstructured, backup *v1.Ba
 
 	log := a.logger.WithField("plugin", "RemapCRDVersionAction").WithField("CRD", crd.Name)
 
+	switch {
+	case hasSingleVersion(crd), hasNonStructuralSchema(crd), hasPreserveUnknownFields(crd):
+		log.Infof("CustomResourceDefinition %s appears to be v1beta1, fetching the v1beta version", crd.Name)
+		item, err = fetchV1beta1CRD(crd.Name, a.betaCRDClient)
+		if err != nil {
+			return nil, nil, err
+		}
+	default:
+		log.Infof("CustomResourceDefinition %s does not appear to be v1beta1, backing up as v1", crd.Name)
+	}
+
+	return item, nil, nil
+}
+
+func fetchV1beta1CRD(name string, betaCRDClient apiextv1beta1client.CustomResourceDefinitionInterface) (*unstructured.Unstructured, error) {
+	betaCRD, err := betaCRDClient.Get(name, metav1.GetOptions{})
+	if err != nil {
+		return nil, errors.Wrapf(err, "error fetching v1beta1 version of %s", name)
+	}
+
+	// Individual items fetched from the API don't always have the kind/API version set
+	// See https://github.com/kubernetes/kubernetes/issues/3030. Unsure why this is happening here and not in main Velero;
+	// probably has to do with List calls and Dynamic client vs typed client
+	// Set these all the time, since they shouldn't ever be different, anyway
+	betaCRD.Kind = kuberesource.CustomResourceDefinitions.Resource
+	betaCRD.APIVersion = apiextv1beta1.SchemeGroupVersion.String()
+
+	m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&betaCRD)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error converting v1beta1 version of %s to unstructured", name)
+	}
+	item := &unstructured.Unstructured{Object: m}
+
+	return item, nil
+
+}
+
+// hasPreserveUnknownFields determines whether or not a CRD is set to preserve unknown fields or not.
+func hasPreserveUnknownFields(crd apiextv1.CustomResourceDefinition) bool {
+	return crd.Spec.PreserveUnknownFields
+}
+
+// hasNonStructuralSchema determines whether or not a CRD has had a nonstructural schema condition applied.
+func hasNonStructuralSchema(crd apiextv1.CustomResourceDefinition) bool {
+	var ret bool
+	for _, c := range crd.Status.Conditions {
+		if c.Type == apiextv1.NonStructuralSchema {
+			ret = true
+			break
+		}
+	}
+	return ret
+}
+
+// hasSingleVersion checks a CRD to see if it has a single version with no schema information.
+func hasSingleVersion(crd apiextv1.CustomResourceDefinition) bool {
 	// Looking for 1 version should be enough to tell if it's a v1beta1 CRD, as all v1beta1 CRD versions share the same schema.
 	// v1 CRDs can have different schemas per version
 	// The silently upgraded versions will often have a `versions` entry that looks like this:
@@ -88,39 +150,11 @@ func (a *RemapCRDVersionAction) Execute(item runtime.Unstructured, backup *v1.Ba
 	//     served:  true
 	//     storage: true
 	// This is acceptable when re-submitted to a v1beta1 endpoint on restore.
+	var ret bool
 	if len(crd.Spec.Versions) > 0 {
 		if crd.Spec.Versions[0].Schema == nil || crd.Spec.Versions[0].Schema.OpenAPIV3Schema == nil {
-			log.Debug("CRD is a candidate for v1beta1 backup")
-
-			if err := setV1beta1Version(item); err != nil {
-				return nil, nil, err
-			}
+			ret = true
 		}
 	}
-
-	// If the NonStructuralSchema condition was applied, be sure to back it up as v1beta1.
-	for _, c := range crd.Status.Conditions {
-		if c.Type == apiextv1.NonStructuralSchema {
-			log.Debug("CRD is a non-structural schema")
-
-			if err := setV1beta1Version(item); err != nil {
-				return nil, nil, err
-			}
-
-			break
-		}
-	}
-
-	return item, nil, nil
-}
-
-// setV1beta1Version updates the apiVersion field of an Unstructured CRD to be the v1beta1 string instead of v1.
-func setV1beta1Version(u runtime.Unstructured) error {
-	// Since we can't manipulate an Unstructured's Object map directly, get a copy to manipulate before setting it back
-	tempMap := u.UnstructuredContent()
-	if err := unstructured.SetNestedField(tempMap, "apiextensions.k8s.io/v1beta1", "apiVersion"); err != nil {
-		return errors.Wrap(err, "unable to set apiversion to v1beta1")
-	}
-	u.SetUnstructuredContent(tempMap)
-	return nil
+	return ret
 }

pkg/backup/remap_crd_version_action_test.go

@@ -18,11 +18,16 @@ package backup
 
 import (
 	"encoding/json"
+	"fmt"
+	"io/ioutil"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	apiextfakes "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 
@@ -33,16 +38,28 @@ import (
 
 func TestRemapCRDVersionAction(t *testing.T) {
 	backup := &v1.Backup{}
-	a := NewRemapCRDVersionAction(velerotest.NewLogger())
+	clientset := apiextfakes.NewSimpleClientset()
+	betaClient := clientset.ApiextensionsV1beta1().CustomResourceDefinitions()
+
+	// build a v1beta1 CRD with the same name and add it to the fake client that the plugin is going to call.
+	// keep the same one for all 3 tests, since there's little value in recreating it
+	b := builder.ForCustomResourceDefinition("test.velero.io")
+	c := b.Result()
+	_, err := betaClient.Create(c)
+	require.NoError(t, err)
+
+	a := NewRemapCRDVersionAction(velerotest.NewLogger(), betaClient)
 
 	t.Run("Test a v1 CRD without any Schema information", func(t *testing.T) {
 		b := builder.ForV1CustomResourceDefinition("test.velero.io")
 		// Set a version that does not include and schema information.
 		b.Version(builder.ForV1CustomResourceDefinitionVersion("v1").Served(true).Storage(true).Result())
 		c := b.Result()
+
 		obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&c)
 		require.NoError(t, err)
 
+		// Execute the plugin, which will call the fake client
 		item, _, err := a.Execute(&unstructured.Unstructured{Object: obj}, backup)
 		require.NoError(t, err)
 		assert.Equal(t, "apiextensions.k8s.io/v1beta1", item.UnstructuredContent()["apiVersion"])
@@ -79,4 +96,97 @@ func TestRemapCRDVersionAction(t *testing.T) {
 		_, _, err = a.Execute(&u, backup)
 		require.NoError(t, err)
 	})
+
+	t.Run("Having Spec.PreserveUnknownFields set to true will return a v1beta1 version of the CRD", func(t *testing.T) {
+		b := builder.ForV1CustomResourceDefinition("test.velero.io")
+		b.PreserveUnknownFields(true)
+		c := b.Result()
+		obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&c)
+		require.NoError(t, err)
+
+		item, _, err := a.Execute(&unstructured.Unstructured{Object: obj}, backup)
+		require.NoError(t, err)
+		assert.Equal(t, "apiextensions.k8s.io/v1beta1", item.UnstructuredContent()["apiVersion"])
+	})
+}
+
+// TestRemapCRDVersionActionData tests the RemapCRDVersionAction plugin against actual CRD to confirm that the v1beta1 version is returned when the v1 version is passed in to the plugin.
+func TestRemapCRDVersionActionData(t *testing.T) {
+	backup := &v1.Backup{}
+	clientset := apiextfakes.NewSimpleClientset()
+	betaClient := clientset.ApiextensionsV1beta1().CustomResourceDefinitions()
+
+	a := NewRemapCRDVersionAction(velerotest.NewLogger(), betaClient)
+
+	tests := []struct {
+		crd                     string
+		expectAdditionalColumns bool
+	}{
+		{
+			crd:                     "elasticsearches.elasticsearch.k8s.elastic.co",
+			expectAdditionalColumns: true,
+		},
+		{
+			crd:                     "kibanas.kibana.k8s.elastic.co",
+			expectAdditionalColumns: true,
+		},
+		{
+			crd: "gcpsamples.gcp.stacks.crossplane.io",
+		},
+		{
+			crd: "alertmanagers.monitoring.coreos.com",
+		},
+		{
+			crd: "prometheuses.monitoring.coreos.com",
+		},
+	}
+
+	for _, test := range tests {
+		tName := fmt.Sprintf("%s CRD passed in as v1 should be returned as v1beta1", test.crd)
+		t.Run(tName, func(t *testing.T) {
+			// We don't need a Go struct of the v1 data, just an unstructured to pass into the plugin.
+			v1File := fmt.Sprintf("testdata/v1/%s.json", test.crd)
+			f, err := ioutil.ReadFile(v1File)
+			require.NoError(t, err)
+
+			var obj unstructured.Unstructured
+			err = json.Unmarshal([]byte(f), &obj)
+			require.NoError(t, err)
+
+			// Load a v1beta1 struct into the beta client to be returned
+			v1beta1File := fmt.Sprintf("testdata/v1beta1/%s.json", test.crd)
+			f, err = ioutil.ReadFile(v1beta1File)
+			require.NoError(t, err)
+
+			var crd apiextv1beta1.CustomResourceDefinition
+			err = json.Unmarshal([]byte(f), &crd)
+			require.NoError(t, err)
+
+			_, err = betaClient.Create(&crd)
+			require.NoError(t, err)
+
+			// Run method under test
+			item, _, err := a.Execute(&obj, backup)
+			require.NoError(t, err)
+
+			assert.Equal(t, "apiextensions.k8s.io/v1beta1", item.UnstructuredContent()["apiVersion"])
+			name, _, err := unstructured.NestedString(item.UnstructuredContent(), "metadata", "name")
+			require.NoError(t, err)
+			assert.Equal(t, crd.Name, name)
+			uid, _, err := unstructured.NestedString(item.UnstructuredContent(), "metadata", "uid")
+			require.NoError(t, err)
+			assert.Equal(t, string(crd.UID), uid)
+
+			// For ElasticSearch and Kibana, problems manifested when additionalPrinterColumns was moved from the top-level spec down to the
+			// versions slice.
+			if test.expectAdditionalColumns {
+				_, ok := item.UnstructuredContent()["spec"].(map[string]interface{})["additionalPrinterColumns"]
+				assert.True(t, ok)
+			}
+
+			// Clean up the item created in the test.
+			betaClient.Delete(crd.Name, &metav1.DeleteOptions{})
+		})
+	}
+
 }

pkg/backup/testdata/v1/gcpsamples.gcp.stacks.crossplane.io.json

@@ -0,0 +1,63 @@
+{
+    "apiVersion": "apiextensions.k8s.io/v1",
+    "kind": "CustomResourceDefinition",
+    "metadata": {
+        "annotations": {
+            "kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"apiextensions.k8s.io/v1beta1\",\"kind\":\"CustomResourceDefinition\",\"metadata\":{\"annotations\":{},\"creationTimestamp\":\"2020-04-20T16:57:37Z\",\"generation\":1,\"name\":\"gcpsamples.gcp.stacks.crossplane.io\",\"resourceVersion\":\"549\",\"selfLink\":\"/apis/apiextensions.k8s.io/v1/customresourcedefinitions/gcpsamples.gcp.stacks.crossplane.io\",\"uid\":\"db5f4321-3226-44b0-8247-66fd7ef59dc8\"},\"spec\":{\"conversion\":{\"strategy\":\"None\"},\"group\":\"gcp.stacks.crossplane.io\",\"names\":{\"kind\":\"GCPSample\",\"listKind\":\"GCPSampleList\",\"plural\":\"gcpsamples\",\"singular\":\"gcpsample\"},\"preserveUnknownFields\":true,\"scope\":\"Cluster\",\"versions\":[{\"name\":\"v1alpha1\",\"served\":true,\"storage\":true}]},\"status\":{\"acceptedNames\":{\"kind\":\"GCPSample\",\"listKind\":\"GCPSampleList\",\"plural\":\"gcpsamples\",\"singular\":\"gcpsample\"},\"conditions\":[{\"lastTransitionTime\":\"2020-04-20T16:57:37Z\",\"message\":\"no conflicts found\",\"reason\":\"NoConflicts\",\"status\":\"True\",\"type\":\"NamesAccepted\"},{\"lastTransitionTime\":\"2020-04-20T16:57:37Z\",\"message\":\"the initial names have been accepted\",\"reason\":\"InitialNamesAccepted\",\"status\":\"True\",\"type\":\"Established\"}],\"storedVersions\":[\"v1alpha1\"]}}\n"
+        },
+        "creationTimestamp": "2020-04-20T17:27:56Z",
+        "generation": 1,
+        "name": "gcpsamples.gcp.stacks.crossplane.io",
+        "resourceVersion": "5567",
+        "selfLink": "/apis/apiextensions.k8s.io/v1/customresourcedefinitions/gcpsamples.gcp.stacks.crossplane.io",
+        "uid": "c0bbac74-acab-4620-b628-1d5f91b19040"
+    },
+    "spec": {
+        "conversion": {
+            "strategy": "None"
+        },
+        "group": "gcp.stacks.crossplane.io",
+        "names": {
+            "kind": "GCPSample",
+            "listKind": "GCPSampleList",
+            "plural": "gcpsamples",
+            "singular": "gcpsample"
+        },
+        "preserveUnknownFields": true,
+        "scope": "Cluster",
+        "versions": [
+            {
+                "name": "v1alpha1",
+                "served": true,
+                "storage": true
+            }
+        ]
+    },
+    "status": {
+        "acceptedNames": {
+            "kind": "GCPSample",
+            "listKind": "GCPSampleList",
+            "plural": "gcpsamples",
+            "singular": "gcpsample"
+        },
+        "conditions": [
+            {
+                "lastTransitionTime": "2020-04-20T17:27:56Z",
+                "message": "no conflicts found",
+                "reason": "NoConflicts",
+                "status": "True",
+                "type": "NamesAccepted"
+            },
+            {
+                "lastTransitionTime": "2020-04-20T17:27:56Z",
+                "message": "the initial names have been accepted",
+                "reason": "InitialNamesAccepted",
+                "status": "True",
+                "type": "Established"
+            }
+        ],
+        "storedVersions": [
+            "v1alpha1"
+        ]
+    }
+}

pkg/backup/testdata/v1beta1/gcpsamples.gcp.stacks.crossplane.io.json

@@ -0,0 +1,64 @@
+{
+  "kind": "CustomResourceDefinition",
+  "apiVersion": "apiextensions.k8s.io/v1beta1",
+  "metadata": {
+    "name": "gcpsamples.gcp.stacks.crossplane.io",
+    "selfLink": "/apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions/gcpsamples.gcp.stacks.crossplane.io",
+    "uid": "c0bbac74-acab-4620-b628-1d5f91b19040",
+    "resourceVersion": "5567",
+    "generation": 1,
+    "creationTimestamp": "2020-04-20T17:27:56Z",
+    "annotations": {
+      "kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"apiextensions.k8s.io/v1beta1\",\"kind\":\"CustomResourceDefinition\",\"metadata\":{\"annotations\":{},\"creationTimestamp\":\"2020-04-20T16:57:37Z\",\"generation\":1,\"name\":\"gcpsamples.gcp.stacks.crossplane.io\",\"resourceVersion\":\"549\",\"selfLink\":\"/apis/apiextensions.k8s.io/v1/customresourcedefinitions/gcpsamples.gcp.stacks.crossplane.io\",\"uid\":\"db5f4321-3226-44b0-8247-66fd7ef59dc8\"},\"spec\":{\"conversion\":{\"strategy\":\"None\"},\"group\":\"gcp.stacks.crossplane.io\",\"names\":{\"kind\":\"GCPSample\",\"listKind\":\"GCPSampleList\",\"plural\":\"gcpsamples\",\"singular\":\"gcpsample\"},\"preserveUnknownFields\":true,\"scope\":\"Cluster\",\"versions\":[{\"name\":\"v1alpha1\",\"served\":true,\"storage\":true}]},\"status\":{\"acceptedNames\":{\"kind\":\"GCPSample\",\"listKind\":\"GCPSampleList\",\"plural\":\"gcpsamples\",\"singular\":\"gcpsample\"},\"conditions\":[{\"lastTransitionTime\":\"2020-04-20T16:57:37Z\",\"message\":\"no conflicts found\",\"reason\":\"NoConflicts\",\"status\":\"True\",\"type\":\"NamesAccepted\"},{\"lastTransitionTime\":\"2020-04-20T16:57:37Z\",\"message\":\"the initial names have been accepted\",\"reason\":\"InitialNamesAccepted\",\"status\":\"True\",\"type\":\"Established\"}],\"storedVersions\":[\"v1alpha1\"]}}\n"
+    }
+  },
+  "spec": {
+    "group": "gcp.stacks.crossplane.io",
+    "version": "v1alpha1",
+    "names": {
+      "plural": "gcpsamples",
+      "singular": "gcpsample",
+      "kind": "GCPSample",
+      "listKind": "GCPSampleList"
+    },
+    "scope": "Cluster",
+    "versions": [
+      {
+        "name": "v1alpha1",
+        "served": true,
+        "storage": true
+      }
+    ],
+    "conversion": {
+      "strategy": "None"
+    },
+    "preserveUnknownFields": true
+  },
+  "status": {
+    "conditions": [
+      {
+        "type": "NamesAccepted",
+        "status": "True",
+        "lastTransitionTime": "2020-04-20T17:27:56Z",
+        "reason": "NoConflicts",
+        "message": "no conflicts found"
+      },
+      {
+        "type": "Established",
+        "status": "True",
+        "lastTransitionTime": "2020-04-20T17:27:56Z",
+        "reason": "InitialNamesAccepted",
+        "message": "the initial names have been accepted"
+      }
+    ],
+    "acceptedNames": {
+      "plural": "gcpsamples",
+      "singular": "gcpsample",
+      "kind": "GCPSample",
+      "listKind": "GCPSampleList"
+    },
+    "storedVersions": [
+      "v1alpha1"
+    ]
+  }
+}

pkg/cmd/server/plugin/plugin.go

@@ -20,6 +20,8 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 
+	apiextensions "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
+
 	"github.com/vmware-tanzu/velero/pkg/backup"
 	"github.com/vmware-tanzu/velero/pkg/client"
 	velerodiscovery "github.com/vmware-tanzu/velero/pkg/discovery"
@@ -38,7 +40,7 @@ func NewCommand(f client.Factory) *cobra.Command {
 				RegisterBackupItemAction("velero.io/pv", newPVBackupItemAction).
 				RegisterBackupItemAction("velero.io/pod", newPodBackupItemAction).
 				RegisterBackupItemAction("velero.io/service-account", newServiceAccountBackupItemAction(f)).
-				RegisterBackupItemAction("velero.io/crd-remap-version", newRemapCRDVersionAction).
+				RegisterBackupItemAction("velero.io/crd-remap-version", newRemapCRDVersionAction(f)).
 				RegisterRestoreItemAction("velero.io/job", newJobRestoreItemAction).
 				RegisterRestoreItemAction("velero.io/pod", newPodRestoreItemAction).
 				RegisterRestoreItemAction("velero.io/restic", newResticRestoreItemAction(f)).
@@ -93,8 +95,20 @@ func newServiceAccountBackupItemAction(f client.Factory) veleroplugin.HandlerIni
 	}
 }
 
-func newRemapCRDVersionAction(logger logrus.FieldLogger) (interface{}, error) {
-	return backup.NewRemapCRDVersionAction(logger), nil
+func newRemapCRDVersionAction(f client.Factory) veleroplugin.HandlerInitializer {
+	return func(logger logrus.FieldLogger) (interface{}, error) {
+		config, err := f.ClientConfig()
+		if err != nil {
+			return nil, err
+		}
+
+		client, err := apiextensions.NewForConfig(config)
+		if err != nil {
+			return nil, err
+		}
+
+		return backup.NewRemapCRDVersionAction(logger, client.ApiextensionsV1beta1().CustomResourceDefinitions()), nil
+	}
 }
 
 func newJobRestoreItemAction(logger logrus.FieldLogger) (interface{}, error) {