From 72de5d1594f0449fc05a6eba915cae4dd150c7bf Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Thu, 16 Sep 2021 15:37:38 +0800 Subject: [PATCH] Fix owners merge in apply unstructured Signed-off-by: Jian Qiu --- deploy/webhook/manifestworks.crd.yaml | 12 +- go.mod | 2 +- go.sum | 4 +- .../manifestwork_controller.go | 11 +- .../manifestwork_controller_test.go | 143 ++++++++++++++++++ vendor/modules.txt | 2 +- ...uster-management.io_manifestworks.crd.yaml | 12 +- .../api/work/v1/types.go | 4 +- .../v1/zz_generated.swagger_doc_generated.go | 4 +- 9 files changed, 168 insertions(+), 26 deletions(-) diff --git a/deploy/webhook/manifestworks.crd.yaml b/deploy/webhook/manifestworks.crd.yaml index 6ee4d706e..617c767f7 100644 --- a/deploy/webhook/manifestworks.crd.yaml +++ b/deploy/webhook/manifestworks.crd.yaml @@ -71,18 +71,18 @@ spec: included in this manifestwork type: object properties: - Name: + group: + description: Group is the api group of the resources + in the workload that the strategy is applied + type: string + name: description: Name is the names of the resources in the workload that the strategy is applied type: string - Namespace: + namespace: description: Namespace is the namespaces of the resources in the workload that the strategy is applied type: string - group: - description: Group is the api group of the resources - in the workload that the strategy is applied - type: string resource: description: Resource is the resources in the workload that the strategy is applied diff --git a/go.mod b/go.mod index 06ac75aff..1f3659a6e 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,7 @@ require ( k8s.io/klog/v2 v2.9.0 k8s.io/kube-aggregator v0.22.1 k8s.io/utils v0.0.0-20210722164352-7f3ee0f31471 - open-cluster-management.io/api v0.0.0-20210823013037-9667ae902e4b + open-cluster-management.io/api v0.0.0-20210916013819-2e58cdb938f9 sigs.k8s.io/controller-runtime v0.9.6 ) diff --git a/go.sum b/go.sum index 3e3e504b4..87be1f64e 100644 --- a/go.sum +++ b/go.sum @@ -1132,8 +1132,8 @@ modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk= modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k= modernc.org/strutil v1.0.0/go.mod h1:lstksw84oURvj9y3tn8lGvRxyRC1S2+g5uuIzNfIOBs= modernc.org/xc v1.0.0/go.mod h1:mRNCo0bvLjGhHO9WsyuKVU4q0ceiDDDoEeWDJHrNx8I= -open-cluster-management.io/api v0.0.0-20210823013037-9667ae902e4b h1:B2vuGyZdIULcE704+81Q4Kisd2ANzlv/+d38GVdcw6w= -open-cluster-management.io/api v0.0.0-20210823013037-9667ae902e4b/go.mod h1:9qiA5h/8kvPQnJEOlAPHVjRO9a1jCmDhGzvgMBvXEaE= +open-cluster-management.io/api v0.0.0-20210916013819-2e58cdb938f9 h1:ySrjJFbSuPbHEN0OvzTeQO8Bt93rjgvbce7lo2cQeZY= +open-cluster-management.io/api v0.0.0-20210916013819-2e58cdb938f9/go.mod h1:9qiA5h/8kvPQnJEOlAPHVjRO9a1jCmDhGzvgMBvXEaE= rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8= rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0= rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= diff --git a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go index ec77e06a3..927b95adf 100644 --- a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go @@ -264,7 +264,7 @@ func (m *ManifestWorkController) applyOneManifest( // TODO we should check the certain error. // Use dynamic client when scheme cannot decode manifest or typed client cannot handle the object if isDecodeError(result.Error) || isUnhandledError(result.Error) || isUnsupportedError(result.Error) { - result.Result, result.Changed, result.Error = m.applyUnstructured(ctx, manifest.Raw, owner, deleteOption, gvr, recorder) + result.Result, result.Changed, result.Error = m.applyUnstructured(ctx, manifest.Raw, owner, gvr, recorder) } return result @@ -288,7 +288,6 @@ func (m *ManifestWorkController) applyUnstructured( ctx context.Context, data []byte, owner metav1.OwnerReference, - deleteOption *workapiv1.DeleteOption, gvr schema.GroupVersionResource, recorder events.Recorder) (*unstructured.Unstructured, bool, error) { @@ -297,6 +296,8 @@ func (m *ManifestWorkController) applyUnstructured( return nil, false, err } + required.SetOwnerReferences([]metav1.OwnerReference{owner}) + existing, err := m.spokeDynamicClient. Resource(gvr). Namespace(required.GetNamespace()). @@ -314,15 +315,13 @@ func (m *ManifestWorkController) applyUnstructured( } // Merge OwnerRefs. - required.SetOwnerReferences([]metav1.OwnerReference{owner}) existingOwners := existing.GetOwnerReferences() modified := resourcemerge.BoolPtr(false) resourcemerge.MergeOwnerRefs(modified, &existingOwners, required.GetOwnerReferences()) - if *modified { - required.SetOwnerReferences(existingOwners) - } + // Always overwrite required ownerrefs from existing, since ownerrefs of required has been merged to existing + required.SetOwnerReferences(existingOwners) // Compare and update the unstrcuctured. if isSameUnstructured(required, existing) { diff --git a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go index 367a99071..b66f32f88 100644 --- a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go +++ b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go @@ -1,6 +1,8 @@ package manifestcontroller import ( + "context" + "encoding/json" "fmt" "testing" "time" @@ -733,3 +735,144 @@ func TestManageOwner(t *testing.T) { }) } } + +func TestApplyUnstructred(t *testing.T) { + cases := []struct { + name string + owner metav1.OwnerReference + existingObject []runtime.Object + required *unstructured.Unstructured + gvr schema.GroupVersionResource + validateActions func(t *testing.T, actions []clienttesting.Action) + }{ + { + name: "create a new object with owner", + existingObject: []runtime.Object{}, + owner: metav1.OwnerReference{Name: "test", UID: "testowner"}, + required: spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"), + gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + validateActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) != 2 { + t.Errorf("Expect 2 actions, but have %d", len(actions)) + } + + spoketesting.AssertAction(t, actions[0], "get") + spoketesting.AssertAction(t, actions[1], "create") + + obj := actions[1].(clienttesting.CreateActionImpl).Object.(*unstructured.Unstructured) + owners := obj.GetOwnerReferences() + if len(owners) != 1 { + t.Errorf("Expect 2 owners, but have %d", len(owners)) + } + + if owners[0].UID != "testowner" { + t.Errorf("Owner UId is not correct, got %s", owners[0].UID) + } + }, + }, + { + name: "create a new object without owner", + existingObject: []runtime.Object{}, + owner: metav1.OwnerReference{Name: "test", UID: "testowner-"}, + required: spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"), + gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + validateActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) != 2 { + t.Errorf("Expect 2 actions, but have %d", len(actions)) + } + + spoketesting.AssertAction(t, actions[0], "get") + spoketesting.AssertAction(t, actions[1], "create") + + obj := actions[1].(clienttesting.CreateActionImpl).Object.(*unstructured.Unstructured) + owners := obj.GetOwnerReferences() + if len(owners) != 0 { + t.Errorf("Expect 1 owners, but have %d", len(owners)) + } + }, + }, + { + name: "update an object owner", + existingObject: []runtime.Object{spoketesting.NewUnstructured("v1", "Secret", "ns1", "test", metav1.OwnerReference{Name: "test1", UID: "testowner1"})}, + owner: metav1.OwnerReference{Name: "test", UID: "testowner"}, + required: spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"), + gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + validateActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) != 2 { + t.Errorf("Expect 2 actions, but have %d", len(actions)) + } + + spoketesting.AssertAction(t, actions[0], "get") + spoketesting.AssertAction(t, actions[1], "update") + + obj := actions[1].(clienttesting.UpdateActionImpl).Object.(*unstructured.Unstructured) + owners := obj.GetOwnerReferences() + if len(owners) != 2 { + t.Errorf("Expect 2 owners, but have %d", len(owners)) + } + + if owners[0].UID != "testowner1" { + t.Errorf("Owner UId is not correct, got %s", owners[0].UID) + } + if owners[1].UID != "testowner" { + t.Errorf("Owner UId is not correct, got %s", owners[1].UID) + } + }, + }, + { + name: "update an object without owner", + existingObject: []runtime.Object{spoketesting.NewUnstructured("v1", "Secret", "ns1", "test", metav1.OwnerReference{Name: "test1", UID: "testowner1"})}, + owner: metav1.OwnerReference{Name: "test", UID: "testowner-"}, + required: spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"), + gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + validateActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) != 1 { + t.Errorf("Expect 1 actions, but have %d", len(actions)) + } + + spoketesting.AssertAction(t, actions[0], "get") + }, + }, + { + name: "remove an object owner", + existingObject: []runtime.Object{spoketesting.NewUnstructured("v1", "Secret", "ns1", "test", metav1.OwnerReference{Name: "test", UID: "testowner"})}, + owner: metav1.OwnerReference{Name: "test", UID: "testowner-"}, + required: spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"), + gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + validateActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) != 2 { + t.Errorf("Expect 2 actions, but have %d", len(actions)) + } + + spoketesting.AssertAction(t, actions[0], "get") + spoketesting.AssertAction(t, actions[1], "update") + + obj := actions[1].(clienttesting.UpdateActionImpl).Object.(*unstructured.Unstructured) + owners := obj.GetOwnerReferences() + if len(owners) != 0 { + t.Errorf("Expect 0 owner, but have %d", len(owners)) + } + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + work, workKey := spoketesting.NewManifestWork(0) + work.Finalizers = []string{controllers.ManifestWorkFinalizer} + controller := newController(work, nil, spoketesting.NewFakeRestMapper()). + withUnstructuredObject(c.existingObject...) + syncContext := spoketesting.NewFakeSyncContext(t, workKey) + + data, _ := json.Marshal(c.required) + _, _, err := controller.controller.applyUnstructured( + context.TODO(), data, c.owner, c.gvr, syncContext.Recorder()) + + if err != nil { + t.Errorf("expect no error, but got %v", err) + } + + c.validateActions(t, controller.dynamicClient.Actions()) + }) + } +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 3ff3a214f..52d14a812 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1075,7 +1075,7 @@ k8s.io/utils/net k8s.io/utils/path k8s.io/utils/pointer k8s.io/utils/trace -# open-cluster-management.io/api v0.0.0-20210823013037-9667ae902e4b +# open-cluster-management.io/api v0.0.0-20210916013819-2e58cdb938f9 ## explicit open-cluster-management.io/api/client/work/clientset/versioned open-cluster-management.io/api/client/work/clientset/versioned/fake diff --git a/vendor/open-cluster-management.io/api/work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml b/vendor/open-cluster-management.io/api/work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml index 6ee4d706e..617c767f7 100644 --- a/vendor/open-cluster-management.io/api/work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml +++ b/vendor/open-cluster-management.io/api/work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml @@ -71,18 +71,18 @@ spec: included in this manifestwork type: object properties: - Name: + group: + description: Group is the api group of the resources + in the workload that the strategy is applied + type: string + name: description: Name is the names of the resources in the workload that the strategy is applied type: string - Namespace: + namespace: description: Namespace is the namespaces of the resources in the workload that the strategy is applied type: string - group: - description: Group is the api group of the resources - in the workload that the strategy is applied - type: string resource: description: Resource is the resources in the workload that the strategy is applied diff --git a/vendor/open-cluster-management.io/api/work/v1/types.go b/vendor/open-cluster-management.io/api/work/v1/types.go index f21fd4945..d003a386c 100644 --- a/vendor/open-cluster-management.io/api/work/v1/types.go +++ b/vendor/open-cluster-management.io/api/work/v1/types.go @@ -98,10 +98,10 @@ type OrphaningRule struct { Resource string `json:"resource"` // Namespace is the namespaces of the resources in the workload that the strategy is applied // +optional - Namespace string `json:"Namespace"` + Namespace string `json:"namespace"` // Name is the names of the resources in the workload that the strategy is applied // +required - Name string `json:"Name"` + Name string `json:"name"` } // ManifestResourceMeta represents the group, version, kind, as well as the group, version, resource, name and namespace of a resoure. diff --git a/vendor/open-cluster-management.io/api/work/v1/zz_generated.swagger_doc_generated.go b/vendor/open-cluster-management.io/api/work/v1/zz_generated.swagger_doc_generated.go index edc90bc74..7e0214a10 100644 --- a/vendor/open-cluster-management.io/api/work/v1/zz_generated.swagger_doc_generated.go +++ b/vendor/open-cluster-management.io/api/work/v1/zz_generated.swagger_doc_generated.go @@ -168,8 +168,8 @@ var map_OrphaningRule = map[string]string{ "": "OrphaningRule identifies a single resource included in this manifestwork", "group": "Group is the api group of the resources in the workload that the strategy is applied", "resource": "Resource is the resources in the workload that the strategy is applied", - "Namespace": "Namespace is the namespaces of the resources in the workload that the strategy is applied", - "Name": "Name is the names of the resources in the workload that the strategy is applied", + "namespace": "Namespace is the namespaces of the resources in the workload that the strategy is applied", + "name": "Name is the names of the resources in the workload that the strategy is applied", } func (OrphaningRule) SwaggerDoc() map[string]string {