Merge pull request #7 from qiujian16/update-unstruct

Update unstructured object

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)
+}

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)
+			}
+		})
+	}
+}