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

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

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

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...)
+		p, err := c.GenerateDeleteOwnerRefStrategicMergeBytes(item.identity.UID, ownerUIDs)
-		_, err = gc.patch(item, patch, func(n *node) ([]byte, error) {
+		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)
+			p, err := c.GenerateDeleteOwnerRefStrategicMergeBytes(dependent.identity.UID, []types.UID{owner.UID})
-			_, err := gc.patch(dependent, patch, func(n *node) ([]byte, error) {
+			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

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")
+	p, err := c.GenerateDeleteOwnerRefStrategicMergeBytes("100", []types.UID{"2", "3"})
-	patched, err := strategicpatch.StrategicMergePatch(originalData, patch, v1.Pod{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	patched, err := strategicpatch.StrategicMergePatch(originalData, p, v1.Pod{})
 	if err != nil {
 		t.Fatal(err)
 	}

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) {

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,
+		types.StrategicMergePatchType, dataBytes, metav1.PatchOptions{})
-		[]byte(fmt.Sprintf(`{"metadata":{"ownerReferences":[{"$patch":"delete","uid":"%s"}],"uid":"%s"}}`, parent.GetUID(), revision.UID)), metav1.PatchOptions{})
 
 	if err != nil {
 		if errors.IsNotFound(err) {

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) {