From 33cc85cd0c52a4efd533f39e20216d0e242a795a Mon Sep 17 00:00:00 2001 From: Nolan Brubaker Date: Thu, 29 Mar 2018 14:50:30 -0400 Subject: [PATCH] Compare backup and cluster objects before logging When restoring resources that raise an already exists error, check their equality before logging a message on the restore. If they're the same except for some metadata, don't generate a message. The restore process was modified so that if an object had an empty namespace string, no namespace key is created on the object. This was to avoid manipulating the copy of the current cluster's object by adding the target namespace. There are some cases right now that are known to not be equal via this method: - The `default` ServiceAccount in a namespace will not match, primarily because of differing default tokens. These will be handled in their own patch - IP addresses for Services are recorded in the backup object, but are either not present on the cluster object, or different. An issue for this already exists at https://github.com/heptio/ark/issues/354 - Endpoints have differing values for `renewTime`. This may be insubstantial, but isn't currently handled by the resetMetadataAndStatus function. - PersistentVolume objects do not match on spec fields, such as claimRef and cloud provider persistent disk info Signed-off-by: Nolan Brubaker --- pkg/restore/restore.go | 49 ++++++++++++++++++++--- pkg/restore/restore_test.go | 80 +++++++++++++++++++++++++++++++++++-- 2 files changed, 119 insertions(+), 10 deletions(-) diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 8a01b2781..41e61f07b 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -31,6 +31,7 @@ import ( "github.com/sirupsen/logrus" "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -634,20 +635,37 @@ func (ctx *context) restoreResource(resource, namespace, resourcePath string) (a } // necessary because we may have remapped the namespace - obj.SetNamespace(namespace) + // if the namespace is blank, don't create the key + if namespace != "" { + obj.SetNamespace(namespace) + } // add an ark-restore label to each resource for easy ID addLabel(obj, api.RestoreLabelKey, ctx.restore.Name) ctx.infof("Restoring %s: %v", obj.GroupVersionKind().Kind, obj.GetName()) - _, err = resourceClient.Create(obj) - if apierrors.IsAlreadyExists(err) { - addToResult(&warnings, namespace, err) + _, restoreErr := resourceClient.Create(obj) + if apierrors.IsAlreadyExists(restoreErr) { + equal := false + if fromCluster, err := resourceClient.Get(obj.GetName(), metav1.GetOptions{}); err == nil { + equal, err = objectsAreEqual(fromCluster, obj) + // Log any errors trying to check equality + if err != nil { + ctx.infof("error checking %s against cluster: %v", obj.GetName(), err) + } + } else { + ctx.infof("Error retrieving cluster version of %s: %v", obj.GetName(), err) + } + if !equal { + e := errors.Errorf("not restored: %s and is different from backed up version.", restoreErr) + addToResult(&warnings, namespace, e) + } continue } - if err != nil { + // Error was something other than an AlreadyExists + if restoreErr != nil { ctx.infof("error restoring %s: %v", obj.GetName(), err) - addToResult(&errs, namespace, fmt.Errorf("error restoring %s: %v", fullPath, err)) + addToResult(&errs, namespace, fmt.Errorf("error restoring %s: %v", fullPath, restoreErr)) continue } @@ -722,6 +740,25 @@ func (ctx *context) executePVAction(obj *unstructured.Unstructured) (*unstructur return updated2, nil } +// objectsAreEqual takes two unstructured objects and checks for equality. +// The fromCluster object is mutated to remove any insubstantial runtime +// information that won't match +func objectsAreEqual(fromCluster, fromBackup *unstructured.Unstructured) (bool, error) { + // Remove insubstantial metadata + fromCluster, err := resetMetadataAndStatus(fromCluster) + if err != nil { + return false, err + } + + // We know the cluster won't have the restore name label, so + // copy it over from the backup + restoreName := fromBackup.GetLabels()[api.RestoreLabelKey] + addLabel(fromCluster, api.RestoreLabelKey, restoreName) + + // If there are no specific actions needed based on the type, simply check for equality. + return equality.Semantic.DeepEqual(fromBackup, fromCluster), nil +} + func isPVReady(obj runtime.Unstructured) bool { phase, err := collections.GetString(obj.UnstructuredContent(), "status.phase") if err != nil { diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 39d02322e..187cfcf6b 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -676,6 +676,61 @@ func TestResetMetadataAndStatus(t *testing.T) { } } +func TestObjectsAreEqual(t *testing.T) { + tests := []struct { + name string + backupObj *unstructured.Unstructured + clusterObj *unstructured.Unstructured + expectedErr bool + expectedRes bool + }{ + { + name: "objects are already equal", + backupObj: NewTestUnstructured().WithName("obj").WithArkLabel("test").Unstructured, + clusterObj: NewTestUnstructured().WithName("obj").Unstructured, + expectedErr: false, + expectedRes: true, + }, + { + name: "objects reset correctly", + backupObj: NewTestUnstructured().WithName("obj").WithArkLabel("test").Unstructured, + clusterObj: NewTestUnstructured().WithMetadata("blah", "foo").WithName("obj").Unstructured, + expectedErr: false, + expectedRes: true, + }, + { + name: "cluster object has no metadata to reset", + backupObj: NewTestUnstructured().WithName("obj").WithArkLabel("test").Unstructured, + clusterObj: NewTestUnstructured().Unstructured, + expectedErr: true, + expectedRes: false, + }, + { + name: "Test JSON objects", + backupObj: unstructuredOrDie(`{"apiVersion":"v1","kind":"ServiceAccount","metadata":{"name":"default","namespace":"nginx-example", "labels": {"ark-restore": "test"}},"secrets":[{"name":"default-token-xhjjc"}]}`), + clusterObj: unstructuredOrDie(`{"apiVersion":"v1","kind":"ServiceAccount","metadata":{"creationTimestamp":"2018-04-05T20:12:21Z","name":"default","namespace":"nginx-example","resourceVersion":"650","selfLink":"/api/v1/namespaces/nginx-example/serviceaccounts/default","uid":"a5a3d2a2-390d-11e8-9644-42010a960002"},"secrets":[{"name":"default-token-xhjjc"}]}`), + expectedErr: false, + expectedRes: true, + }, + { + name: "Test ServiceAccount secrets mismatch", + backupObj: unstructuredOrDie(`{"apiVersion":"v1","kind":"ServiceAccount","metadata":{"name":"default","namespace":"nginx-example", "labels": {"ark-restore": "test"}},"secrets":[{"name":"default-token-abcde"}]}`), + clusterObj: unstructuredOrDie(`{"apiVersion":"v1","kind":"ServiceAccount","metadata":{"creationTimestamp":"2018-04-05T20:12:21Z","name":"default","namespace":"nginx-example","resourceVersion":"650","selfLink":"/api/v1/namespaces/nginx-example/serviceaccounts/default","uid":"a5a3d2a2-390d-11e8-9644-42010a960002"},"secrets":[{"name":"default-token-xhjjc"}]}`), + expectedErr: false, + expectedRes: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + res, err := objectsAreEqual(test.clusterObj, test.backupObj) + if assert.Equal(t, test.expectedErr, err != nil) { + assert.Equal(t, test.expectedRes, res) + } + }) + } +} + func TestExecutePVAction(t *testing.T) { iops := int64(1000) @@ -844,6 +899,16 @@ func TestIsPVReady(t *testing.T) { } } +// Copied from backup/backup_test.go for JSON testing. +// TODO: move this into util/test for re-use. +func unstructuredOrDie(data string) *unstructured.Unstructured { + o, _, err := unstructured.UnstructuredJSONScheme.Decode([]byte(data), nil, nil) + if err != nil { + panic(err) + } + return o.(*unstructured.Unstructured) +} + type testUnstructured struct { *unstructured.Unstructured } @@ -897,6 +962,17 @@ func (obj *testUnstructured) WithName(name string) *testUnstructured { return obj.WithMetadataField("name", name) } +func (obj *testUnstructured) WithArkLabel(restoreName string) *testUnstructured { + ls := obj.GetLabels() + if ls == nil { + ls = make(map[string]string) + } + ls[api.RestoreLabelKey] = restoreName + obj.SetLabels(ls) + + return obj +} + func (obj *testUnstructured) withMap(name string, fields ...string) *testUnstructured { m := make(map[string]interface{}) obj.Object[name] = m @@ -942,10 +1018,6 @@ func toUnstructured(objs ...runtime.Object) []unstructured.Unstructured { delete(metadata, "creationTimestamp") - if _, exists := metadata["namespace"]; !exists { - metadata["namespace"] = "" - } - delete(unstructuredObj.Object, "status") res = append(res, unstructuredObj)