From 79a51090f9582c9b67576c180d77b95214d1cd3e Mon Sep 17 00:00:00 2001 From: Manu Gupta Date: Fri, 5 Nov 2021 06:33:52 -0700 Subject: [PATCH] fix: 81134: fix unsafe json for ReleaseControllerRevision (#104049) * fix: 81134: fix unsafe json for ReleaseControllerRevision 1. Ensures that ReleaseControllerRevision returns a proper json by marshalling an object into bytes. Otherwise, it returns an error. 2. Also, refactors the code to commonize the merge type GenerateDeleteOwnerRefStrategicMergeBytes that returns a byte and is used across ReleasePod, ReleaseControllerRevison ReleaseReplicaSet. * Move GeneratePatchBytesForDelete to controller_ref_manager --- pkg/controller/controller_ref_manager.go | 72 ++++++++++--------- pkg/controller/controller_ref_manager_test.go | 57 ++++++++++++++- .../garbagecollector/garbagecollector.go | 16 +++-- .../garbagecollector/garbagecollector_test.go | 8 ++- pkg/controller/garbagecollector/patch.go | 27 ------- pkg/controller/history/controller_history.go | 9 ++- .../history/controller_history_test.go | 5 +- 7 files changed, 122 insertions(+), 72 deletions(-) diff --git a/pkg/controller/controller_ref_manager.go b/pkg/controller/controller_ref_manager.go index 3fc94626805..eca5c3a8fd5 100644 --- a/pkg/controller/controller_ref_manager.go +++ b/pkg/controller/controller_ref_manager.go @@ -232,7 +232,7 @@ func (m *PodControllerRefManager) AdoptPod(ctx context.Context, pod *v1.Pod) err func (m *PodControllerRefManager) ReleasePod(pod *v1.Pod) error { klog.V(2).Infof("patching pod %s_%s to remove its controllerRef to %s/%s:%s", pod.Namespace, pod.Name, m.controllerKind.GroupVersion(), m.controllerKind.Kind, m.Controller.GetName()) - patchBytes, err := deleteOwnerRefStrategicMergePatch(pod.UID, m.Controller.GetUID(), m.finalizers...) + patchBytes, err := GenerateDeleteOwnerRefStrategicMergeBytes(pod.UID, []types.UID{m.Controller.GetUID()}, m.finalizers...) if err != nil { return err } @@ -357,7 +357,7 @@ func (m *ReplicaSetControllerRefManager) AdoptReplicaSet(ctx context.Context, rs func (m *ReplicaSetControllerRefManager) ReleaseReplicaSet(replicaSet *apps.ReplicaSet) error { klog.V(2).Infof("patching ReplicaSet %s_%s to remove its controllerRef to %s/%s:%s", replicaSet.Namespace, replicaSet.Name, m.controllerKind.GroupVersion(), m.controllerKind.Kind, m.Controller.GetName()) - patchBytes, err := deleteOwnerRefStrategicMergePatch(replicaSet.UID, m.Controller.GetUID()) + patchBytes, err := GenerateDeleteOwnerRefStrategicMergeBytes(replicaSet.UID, []types.UID{m.Controller.GetUID()}) if err != nil { return err } @@ -495,7 +495,7 @@ func (m *ControllerRevisionControllerRefManager) AdoptControllerRevision(ctx con func (m *ControllerRevisionControllerRefManager) ReleaseControllerRevision(history *apps.ControllerRevision) error { klog.V(2).Infof("patching ControllerRevision %s_%s to remove its controllerRef to %s/%s:%s", history.Namespace, history.Name, m.controllerKind.GroupVersion(), m.controllerKind.Kind, m.Controller.GetName()) - patchBytes, err := deleteOwnerRefStrategicMergePatch(history.UID, m.Controller.GetUID()) + patchBytes, err := GenerateDeleteOwnerRefStrategicMergeBytes(history.UID, []types.UID{m.Controller.GetUID()}) if err != nil { return err } @@ -517,36 +517,6 @@ func (m *ControllerRevisionControllerRefManager) ReleaseControllerRevision(histo return err } -type objectForDeleteOwnerRefStrategicMergePatch struct { - Metadata objectMetaForMergePatch `json:"metadata"` -} - -type objectMetaForMergePatch struct { - UID types.UID `json:"uid"` - OwnerReferences []map[string]string `json:"ownerReferences"` - DeleteFinalizers []string `json:"$deleteFromPrimitiveList/finalizers,omitempty"` -} - -func deleteOwnerRefStrategicMergePatch(dependentUID types.UID, ownerUID types.UID, finalizers ...string) ([]byte, error) { - patch := objectForDeleteOwnerRefStrategicMergePatch{ - Metadata: objectMetaForMergePatch{ - UID: dependentUID, - OwnerReferences: []map[string]string{ - { - "$patch": "delete", - "uid": string(ownerUID), - }, - }, - DeleteFinalizers: finalizers, - }, - } - patchBytes, err := json.Marshal(&patch) - if err != nil { - return nil, err - } - return patchBytes, nil -} - type objectForAddOwnerRefPatch struct { Metadata objectMetaForPatch `json:"metadata"` } @@ -582,3 +552,39 @@ func ownerRefControllerPatch(controller metav1.Object, controllerKind schema.Gro } return patchBytes, nil } + +type objectForDeleteOwnerRefStrategicMergePatch struct { + Metadata objectMetaForMergePatch `json:"metadata"` +} + +type objectMetaForMergePatch struct { + UID types.UID `json:"uid"` + OwnerReferences []map[string]string `json:"ownerReferences"` + DeleteFinalizers []string `json:"$deleteFromPrimitiveList/finalizers,omitempty"` +} + +func GenerateDeleteOwnerRefStrategicMergeBytes(dependentUID types.UID, ownerUIDs []types.UID, finalizers ...string) ([]byte, error) { + var ownerReferences []map[string]string + for _, ownerUID := range ownerUIDs { + ownerReferences = append(ownerReferences, ownerReference(ownerUID, "delete")) + } + patch := objectForDeleteOwnerRefStrategicMergePatch{ + Metadata: objectMetaForMergePatch{ + UID: dependentUID, + OwnerReferences: ownerReferences, + DeleteFinalizers: finalizers, + }, + } + patchBytes, err := json.Marshal(&patch) + if err != nil { + return nil, err + } + return patchBytes, nil +} + +func ownerReference(uid types.UID, patchType string) map[string]string { + return map[string]string{ + "$patch": patchType, + "uid": string(uid), + } +} diff --git a/pkg/controller/controller_ref_manager_test.go b/pkg/controller/controller_ref_manager_test.go index e06ec35bc8f..ab475fb6a87 100644 --- a/pkg/controller/controller_ref_manager_test.go +++ b/pkg/controller/controller_ref_manager_test.go @@ -18,6 +18,7 @@ package controller import ( "context" + "reflect" "strings" "testing" @@ -202,10 +203,64 @@ func TestClaimPods(t *testing.T) { } for _, f := range test.manager.finalizers { if !strings.Contains(patch, f) { - t.Errorf("Patch doesn't contain finalizer %q", f) + t.Errorf("Patch doesn't contain finalizer %s, %q", patch, f) } } } }) } } + +func TestGeneratePatchBytesForDelete(t *testing.T) { + tests := []struct { + name string + ownerUID []types.UID + dependentUID types.UID + finalizers []string + want []byte + }{ + { + name: "check the structure of patch bytes", + ownerUID: []types.UID{"ss1"}, + dependentUID: "ss2", + finalizers: []string{}, + want: []byte(`{"metadata":{"uid":"ss2","ownerReferences":[{"$patch":"delete","uid":"ss1"}]}}`), + }, + { + name: "check if parent uid is escaped", + ownerUID: []types.UID{`ss1"hello`}, + dependentUID: "ss2", + finalizers: []string{}, + want: []byte(`{"metadata":{"uid":"ss2","ownerReferences":[{"$patch":"delete","uid":"ss1\"hello"}]}}`), + }, + { + name: "check if revision uid uid is escaped", + ownerUID: []types.UID{`ss1`}, + dependentUID: `ss2"hello`, + finalizers: []string{}, + want: []byte(`{"metadata":{"uid":"ss2\"hello","ownerReferences":[{"$patch":"delete","uid":"ss1"}]}}`), + }, + { + name: "check the structure of patch bytes with multiple owners", + ownerUID: []types.UID{"ss1", "ss2"}, + dependentUID: "ss2", + finalizers: []string{}, + want: []byte(`{"metadata":{"uid":"ss2","ownerReferences":[{"$patch":"delete","uid":"ss1"},{"$patch":"delete","uid":"ss2"}]}}`), + }, + { + name: "check the structure of patch bytes with a finalizer and multiple owners", + ownerUID: []types.UID{"ss1", "ss2"}, + dependentUID: "ss2", + finalizers: []string{"f1"}, + want: []byte(`{"metadata":{"uid":"ss2","ownerReferences":[{"$patch":"delete","uid":"ss1"},{"$patch":"delete","uid":"ss2"}],"$deleteFromPrimitiveList/finalizers":["f1"]}}`), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, _ := GenerateDeleteOwnerRefStrategicMergeBytes(tt.dependentUID, tt.ownerUID, tt.finalizers...) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("generatePatchBytesForDelete() got = %s, want %s", got, tt.want) + } + }) + } +} diff --git a/pkg/controller/garbagecollector/garbagecollector.go b/pkg/controller/garbagecollector/garbagecollector.go index ab2b7fc616f..9028accb14c 100644 --- a/pkg/controller/garbagecollector/garbagecollector.go +++ b/pkg/controller/garbagecollector/garbagecollector.go @@ -45,6 +45,7 @@ import ( "k8s.io/client-go/util/workqueue" "k8s.io/controller-manager/controller" "k8s.io/controller-manager/pkg/informerfactory" + c "k8s.io/kubernetes/pkg/controller" "k8s.io/kubernetes/pkg/controller/apis/config/scheme" // import known versions @@ -532,8 +533,11 @@ func (gc *GarbageCollector) attemptToDeleteItem(ctx context.Context, item *node) // ownerReferences, otherwise the referenced objects will be stuck with // the FinalizerDeletingDependents and never get deleted. ownerUIDs := append(ownerRefsToUIDs(dangling), ownerRefsToUIDs(waitingForDependentsDeletion)...) - patch := deleteOwnerRefStrategicMergePatch(item.identity.UID, ownerUIDs...) - _, err = gc.patch(item, patch, func(n *node) ([]byte, error) { + p, err := c.GenerateDeleteOwnerRefStrategicMergeBytes(item.identity.UID, ownerUIDs) + if err != nil { + return err + } + _, err = gc.patch(item, p, func(n *node) ([]byte, error) { return gc.deleteOwnerRefJSONMergePatch(n, ownerUIDs...) }) return err @@ -612,8 +616,12 @@ func (gc *GarbageCollector) orphanDependents(owner objectReference, dependents [ go func(dependent *node) { defer wg.Done() // the dependent.identity.UID is used as precondition - patch := deleteOwnerRefStrategicMergePatch(dependent.identity.UID, owner.UID) - _, err := gc.patch(dependent, patch, func(n *node) ([]byte, error) { + p, err := c.GenerateDeleteOwnerRefStrategicMergeBytes(dependent.identity.UID, []types.UID{owner.UID}) + if err != nil { + errCh <- fmt.Errorf("orphaning %s failed, %v", dependent.identity, err) + return + } + _, err = gc.patch(dependent, p, func(n *node) ([]byte, error) { return gc.deleteOwnerRefJSONMergePatch(n, owner.UID) }) // note that if the target ownerReference doesn't exist in the diff --git a/pkg/controller/garbagecollector/garbagecollector_test.go b/pkg/controller/garbagecollector/garbagecollector_test.go index 61de8497afe..e239c51a4a8 100644 --- a/pkg/controller/garbagecollector/garbagecollector_test.go +++ b/pkg/controller/garbagecollector/garbagecollector_test.go @@ -60,6 +60,7 @@ import ( "k8s.io/client-go/util/workqueue" "k8s.io/controller-manager/pkg/informerfactory" "k8s.io/kubernetes/pkg/api/legacyscheme" + c "k8s.io/kubernetes/pkg/controller" ) type testRESTMapper struct { @@ -594,8 +595,11 @@ func TestDeleteOwnerRefPatch(t *testing.T) { }, }, } - patch := deleteOwnerRefStrategicMergePatch("100", "2", "3") - patched, err := strategicpatch.StrategicMergePatch(originalData, patch, v1.Pod{}) + p, err := c.GenerateDeleteOwnerRefStrategicMergeBytes("100", []types.UID{"2", "3"}) + if err != nil { + t.Fatal(err) + } + patched, err := strategicpatch.StrategicMergePatch(originalData, p, v1.Pod{}) if err != nil { t.Fatal(err) } diff --git a/pkg/controller/garbagecollector/patch.go b/pkg/controller/garbagecollector/patch.go index a58a6e40604..25d8b208513 100644 --- a/pkg/controller/garbagecollector/patch.go +++ b/pkg/controller/garbagecollector/patch.go @@ -29,33 +29,6 @@ import ( "k8s.io/kubernetes/pkg/controller/garbagecollector/metaonly" ) -type objectForDeleteOwnerRefStrategicMergePatch struct { - Metadata objectMetaForMergePatch `json:"metadata"` -} - -type objectMetaForMergePatch struct { - UID types.UID `json:"uid"` - OwnerReferences []map[string]string `json:"ownerReferences"` -} - -func deleteOwnerRefStrategicMergePatch(dependentUID types.UID, ownerUIDs ...types.UID) []byte { - var pieces []map[string]string - for _, ownerUID := range ownerUIDs { - pieces = append(pieces, map[string]string{"$patch": "delete", "uid": string(ownerUID)}) - } - patch := objectForDeleteOwnerRefStrategicMergePatch{ - Metadata: objectMetaForMergePatch{ - UID: dependentUID, - OwnerReferences: pieces, - }, - } - patchBytes, err := json.Marshal(&patch) - if err != nil { - return []byte{} - } - return patchBytes -} - // getMetadata tries getting object metadata from local cache, and sends GET request to apiserver when // local cache is not available or not latest. func (gc *GarbageCollector) getMetadata(apiVersion, kind, namespace, name string) (metav1.Object, error) { diff --git a/pkg/controller/history/controller_history.go b/pkg/controller/history/controller_history.go index dabcd399d4d..19ae0999af5 100644 --- a/pkg/controller/history/controller_history.go +++ b/pkg/controller/history/controller_history.go @@ -29,6 +29,7 @@ import ( appsinformers "k8s.io/client-go/informers/apps/v1" clientset "k8s.io/client-go/kubernetes" appslisters "k8s.io/client-go/listers/apps/v1" + "k8s.io/kubernetes/pkg/controller" hashutil "k8s.io/kubernetes/pkg/util/hash" apiequality "k8s.io/apimachinery/pkg/api/equality" @@ -332,10 +333,14 @@ func (rh *realHistory) AdoptControllerRevision(parent metav1.Object, parentKind } func (rh *realHistory) ReleaseControllerRevision(parent metav1.Object, revision *apps.ControllerRevision) (*apps.ControllerRevision, error) { + dataBytes, err := controller.GenerateDeleteOwnerRefStrategicMergeBytes(revision.UID, []types.UID{parent.GetUID()}) + if err != nil { + return nil, err + } + // Use strategic merge patch to add an owner reference indicating a controller ref released, err := rh.client.AppsV1().ControllerRevisions(revision.GetNamespace()).Patch(context.TODO(), revision.GetName(), - types.StrategicMergePatchType, - []byte(fmt.Sprintf(`{"metadata":{"ownerReferences":[{"$patch":"delete","uid":"%s"}],"uid":"%s"}}`, parent.GetUID(), revision.UID)), metav1.PatchOptions{}) + types.StrategicMergePatchType, dataBytes, metav1.PatchOptions{}) if err != nil { if errors.IsNotFound(err) { diff --git a/pkg/controller/history/controller_history_test.go b/pkg/controller/history/controller_history_test.go index 8b95aac5c57..56c7edd55ed 100644 --- a/pkg/controller/history/controller_history_test.go +++ b/pkg/controller/history/controller_history_test.go @@ -23,12 +23,14 @@ import ( "fmt" "reflect" "testing" + "time" apps "k8s.io/api/apps/v1" "k8s.io/api/core/v1" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" clientscheme "k8s.io/client-go/kubernetes/scheme" + core "k8s.io/client-go/testing" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/controller" @@ -39,9 +41,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/strategicpatch" - - core "k8s.io/client-go/testing" - "time" ) func TestRealHistory_ListControllerRevisions(t *testing.T) {