From e9143d61585b6ea6e1944512891ce11f5611174f Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Wed, 6 May 2020 15:03:56 +0800 Subject: [PATCH] Update unstructured object --- .../controllers/manifestwork_controller.go | 49 +++++++- .../manifestwork_controller_test.go | 109 +++++++++++++++--- 2 files changed, 137 insertions(+), 21 deletions(-) diff --git a/pkg/spoke/controllers/manifestwork_controller.go b/pkg/spoke/controllers/manifestwork_controller.go index 86a67044c..48269c71d 100644 --- a/pkg/spoke/controllers/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestwork_controller.go @@ -8,6 +8,7 @@ import ( "time" apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -268,7 +269,6 @@ func (m *ManifestWorkController) applyUnstructrued(data []byte, recorder events. Resource(gvr). Namespace(required.GetNamespace()). Get(context.TODO(), required.GetName(), metav1.GetOptions{}) - if errors.IsNotFound(err) { actual, err := m.spokeDynamicClient.Resource(gvr).Namespace(required.GetNamespace()).Create( context.TODO(), required, metav1.CreateOptions{}) @@ -280,9 +280,16 @@ func (m *ManifestWorkController) applyUnstructrued(data []byte, recorder events. return nil, false, err } - // Do not update unstructured type right now. Need to figure out how to merge. - - return existing, false, err + // Compare and update the unstrcuctured. + if isSameUnstructured(required, existing) { + return existing, false, nil + } + required.SetResourceVersion(existing.GetResourceVersion()) + actual, err := m.spokeDynamicClient.Resource(gvr).Namespace(required.GetNamespace()).Update( + context.TODO(), required, metav1.UpdateOptions{}) + recorder.Eventf(fmt.Sprintf( + "%s Updated", required.GetKind()), "Updated %s/%s", required.GetNamespace(), required.GetName()) + return actual, true, err } func (m *ManifestWorkController) generateUpdateStatusFunc(manifestStatus workapiv1.ManifestResourceStatus) helper.UpdateManifestWorkStatusFunc { @@ -304,3 +311,37 @@ func isDecodeError(err error) bool { func isUnhandledError(err error) bool { return err != nil && strings.HasPrefix(err.Error(), "cannot decode") } + +// isSameUnstructured compares the two unstructured object. +// The comparison ignores the metadata and status field, and check if the two objects are sementically equal. +func isSameUnstructured(obj1, obj2 *unstructured.Unstructured) bool { + obj1Copy := obj1.DeepCopy() + obj2Copy := obj2.DeepCopy() + + // Comapre gvk, name, namespace at first + if obj1Copy.GroupVersionKind() != obj2Copy.GroupVersionKind() { + return false + } + if obj1Copy.GetName() != obj2Copy.GetName() { + return false + } + if obj1Copy.GetNamespace() != obj2Copy.GetNamespace() { + return false + } + + // Compare label and annotations + if !equality.Semantic.DeepEqual(obj1Copy.GetLabels(), obj2Copy.GetLabels()) { + return false + } + if !equality.Semantic.DeepEqual(obj1Copy.GetAnnotations(), obj2Copy.GetAnnotations()) { + return false + } + + // Compare sementically after removing metadata and status field + delete(obj1Copy.Object, "metadata") + delete(obj2Copy.Object, "metadata") + delete(obj1Copy.Object, "status") + delete(obj2Copy.Object, "status") + + return equality.Semantic.DeepEqual(obj1Copy.Object, obj2Copy.Object) +} diff --git a/pkg/spoke/controllers/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestwork_controller_test.go index f5044b6d7..1eb0bdbe2 100644 --- a/pkg/spoke/controllers/manifestwork_controller_test.go +++ b/pkg/spoke/controllers/manifestwork_controller_test.go @@ -74,10 +74,12 @@ func newUnstructured(apiVersion, kind, namespace, name string) *unstructured.Uns } } -func newUnstructuredWithSpec( - apiVersion, kind, namespace, name string, spec map[string]interface{}) *unstructured.Unstructured { +func newUnstructuredWithContent( + apiVersion, kind, namespace, name string, content map[string]interface{}) *unstructured.Unstructured { object := newUnstructured(apiVersion, kind, namespace, name) - object.Object["spec"] = spec + for key, val := range content { + object.Object[key] = val + } return object } @@ -117,7 +119,7 @@ func newFakeMapper() *resource.Mapper { "v1": { {Name: "secrets", Namespaced: true, Kind: "Secret"}, {Name: "pods", Namespaced: true, Kind: "Pod"}, - {Name: "newobject", Namespaced: true, Kind: "NewObject"}, + {Name: "newobjects", Namespaced: true, Kind: "NewObject"}, }, }, }, @@ -127,18 +129,13 @@ func newFakeMapper() *resource.Mapper { } } -func newController(work *workapiv1.ManifestWork, mapper *resource.Mapper, objects ...runtime.Object) *testController { +func newController(work *workapiv1.ManifestWork, mapper *resource.Mapper) *testController { fakeWorkClient := fakeworkclient.NewSimpleClientset(work) - scheme := runtime.NewScheme() - dynamicClient := fakedynamic.NewSimpleDynamicClient(scheme) - kubeClient := fakekube.NewSimpleClientset(objects...) workInformerFactory := workinformers.NewSharedInformerFactoryWithOptions(fakeWorkClient, 5*time.Minute, workinformers.WithNamespace("cluster1")) controller := &ManifestWorkController{ manifestWorkClient: fakeWorkClient.WorkV1().ManifestWorks("cluster1"), manifestWorkLister: workInformerFactory.Work().V1().ManifestWorks().Lister().ManifestWorks("cluster1"), - spokeDynamicClient: dynamicClient, - spokeKubeclient: kubeClient, restMapper: mapper, } @@ -146,13 +143,26 @@ func newController(work *workapiv1.ManifestWork, mapper *resource.Mapper, object store.Add(work) return &testController{ - controller: controller, - dynamicClient: dynamicClient, - workClient: fakeWorkClient, - kubeClient: kubeClient, + controller: controller, + workClient: fakeWorkClient, } } +func (t *testController) withKubeObject(objects ...runtime.Object) *testController { + kubeClient := fakekube.NewSimpleClientset(objects...) + t.controller.spokeKubeclient = kubeClient + t.kubeClient = kubeClient + return t +} + +func (t *testController) withUnstructuredObject(objects ...runtime.Object) *testController { + scheme := runtime.NewScheme() + dynamicClient := fakedynamic.NewSimpleDynamicClient(scheme, objects...) + t.controller.spokeDynamicClient = dynamicClient + t.dynamicClient = dynamicClient + return t +} + func assertAction(t *testing.T, actual clienttesting.Action, expected string) { if actual.GetVerb() != expected { t.Errorf("expected %s action but got: %#v", expected, actual) @@ -191,6 +201,7 @@ type testCase struct { name string workManifest []*unstructured.Unstructured spokeObject []runtime.Object + spokeDynamicObject []runtime.Object expectedWorkAction []string expectedKubeAction []string expectedDynamicAction []string @@ -207,6 +218,7 @@ func newTestCase(name string) *testCase { name: name, workManifest: []*unstructured.Unstructured{}, spokeObject: []runtime.Object{}, + spokeDynamicObject: []runtime.Object{}, expectedWorkAction: []string{}, expectedKubeAction: []string{}, expectedDynamicAction: []string{}, @@ -224,6 +236,11 @@ func (t *testCase) withSpokeObject(objects ...runtime.Object) *testCase { return t } +func (t *testCase) withSpokeDynamicObject(objects ...runtime.Object) *testCase { + t.spokeDynamicObject = objects + return t +} + func (t *testCase) withExpectedWorkAction(actions ...string) *testCase { t.expectedWorkAction = actions return t @@ -301,6 +318,12 @@ func TestSync(t *testing.T) { withExpectedWorkAction("get", "update"). withExpectedDynamicAction("get", "create"). withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), + newTestCase("update single unstructured resource"). + withWorkManifest(newUnstructuredWithContent("v1", "NewObject", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val1"}})). + withSpokeDynamicObject(newUnstructuredWithContent("v1", "NewObject", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val2"}})). + withExpectedWorkAction("get", "update"). + withExpectedDynamicAction("get", "update"). + withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), newTestCase("multiple create&update resource"). withWorkManifest(newUnstructured("v1", "Secret", "ns1", "test"), newUnstructured("v1", "Secret", "ns2", "test")). withSpokeObject(newSecret("test", "ns1", "value2")). @@ -313,7 +336,9 @@ func TestSync(t *testing.T) { t.Run(c.name, func(t *testing.T) { work, workKey := newManifestWork(0, c.workManifest...) work.Finalizers = []string{manifestWorkFinalizer} - controller := newController(work, newFakeMapper(), c.spokeObject...) + controller := newController(work, newFakeMapper()). + withKubeObject(c.spokeObject...). + withUnstructuredObject(c.spokeDynamicObject...) syncContext := newFakeSyncContext(t, workKey) err := controller.controller.sync(nil, syncContext) if err != nil { @@ -337,7 +362,7 @@ func TestDeleteWork(t *testing.T) { work.Finalizers = []string{manifestWorkFinalizer} now := metav1.Now() work.ObjectMeta.SetDeletionTimestamp(&now) - controller := newController(work, newFakeMapper(), tc.spokeObject...) + controller := newController(work, newFakeMapper()).withKubeObject(tc.spokeObject...).withUnstructuredObject() syncContext := newFakeSyncContext(t, workKey) err := controller.controller.sync(nil, syncContext) if err != nil { @@ -369,7 +394,7 @@ func TestFailedToApplyResource(t *testing.T) { work, workKey := newManifestWork(0, tc.workManifest...) work.Finalizers = []string{manifestWorkFinalizer} - controller := newController(work, newFakeMapper(), tc.spokeObject...) + controller := newController(work, newFakeMapper()).withKubeObject(tc.spokeObject...).withUnstructuredObject() // Add a reactor on fake client to throw error when creating secret on namespace ns2 controller.kubeClient.PrependReactor("create", "secrets", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { @@ -394,3 +419,53 @@ func TestFailedToApplyResource(t *testing.T) { tc.validate(t, controller.dynamicClient, controller.workClient, controller.kubeClient) } + +// Test unstructured compare +func TestIsSameUnstructured(t *testing.T) { + cases := []struct { + name string + obj1 *unstructured.Unstructured + obj2 *unstructured.Unstructured + expected bool + }{ + { + name: "different kind", + obj1: newUnstructured("v1", "Kind1", "ns1", "n1"), + obj2: newUnstructured("v1", "Kind2", "ns1", "n1"), + expected: false, + }, + { + name: "different namespace", + obj1: newUnstructured("v1", "Kind1", "ns1", "n1"), + obj2: newUnstructured("v1", "Kind1", "ns2", "n1"), + expected: false, + }, + { + name: "different name", + obj1: newUnstructured("v1", "Kind1", "ns1", "n1"), + obj2: newUnstructured("v1", "Kind1", "ns1", "n2"), + expected: false, + }, + { + name: "different spec", + obj1: newUnstructuredWithContent("v1", "Kind1", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val1"}}), + obj2: newUnstructuredWithContent("v1", "Kind1", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val2"}}), + expected: false, + }, + { + name: "same spec, different status", + obj1: newUnstructuredWithContent("v1", "Kind1", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val1"}, "status": "status1"}), + obj2: newUnstructuredWithContent("v1", "Kind1", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val1"}, "status": "status2"}), + expected: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + actual := isSameUnstructured(c.obj1, c.obj2) + if c.expected != actual { + t.Errorf("expected %t, but %t", c.expected, actual) + } + }) + } +}