fix unsafe JSON construction

Signed-off-by: Zou Nengren <zouyee1989@gmail.com>

pkg/controller/controller_ref_manager.go

@@ -17,15 +17,17 @@ limitations under the License.
 package controller
 
 import (
+	"encoding/json"
 	"fmt"
 	"sync"
 
 	apps "k8s.io/api/apps/v1"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog"
 )
@@ -213,11 +215,12 @@ func (m *PodControllerRefManager) AdoptPod(pod *v1.Pod) error {
 	}
 	// Note that ValidateOwnerReferences() will reject this patch if another
 	// OwnerReference exists with controller=true.
-	addControllerPatch := fmt.Sprintf(
+
-		`{"metadata":{"ownerReferences":[{"apiVersion":"%s","kind":"%s","name":"%s","uid":"%s","controller":true,"blockOwnerDeletion":true}],"uid":"%s"}}`,
+	patchBytes, err := ownerRefControllerPatch(m.Controller, m.controllerKind, pod.UID)
-		m.controllerKind.GroupVersion(), m.controllerKind.Kind,
+	if err != nil {
-		m.Controller.GetName(), m.Controller.GetUID(), pod.UID)
+		return err
-	return m.podControl.PatchPod(pod.Namespace, pod.Name, []byte(addControllerPatch))
+	}
+	return m.podControl.PatchPod(pod.Namespace, pod.Name, patchBytes)
 }
 
 // ReleasePod sends a patch to free the pod from the control of the controller.
@@ -225,8 +228,11 @@ func (m *PodControllerRefManager) AdoptPod(pod *v1.Pod) error {
 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())
-	deleteOwnerRefPatch := fmt.Sprintf(`{"metadata":{"ownerReferences":[{"$patch":"delete","uid":"%s"}],"uid":"%s"}}`, m.Controller.GetUID(), pod.UID)
+	patchBytes, err := deleteOwnerRefStrategicMergePatch(pod.UID, m.Controller.GetUID())
-	err := m.podControl.PatchPod(pod.Namespace, pod.Name, []byte(deleteOwnerRefPatch))
+	if err != nil {
+		return err
+	}
+	err = m.podControl.PatchPod(pod.Namespace, pod.Name, patchBytes)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			// If the pod no longer exists, ignore it.
@@ -335,11 +341,11 @@ func (m *ReplicaSetControllerRefManager) AdoptReplicaSet(rs *apps.ReplicaSet) er
 	}
 	// Note that ValidateOwnerReferences() will reject this patch if another
 	// OwnerReference exists with controller=true.
-	addControllerPatch := fmt.Sprintf(
+	patchBytes, err := ownerRefControllerPatch(m.Controller, m.controllerKind, rs.UID)
-		`{"metadata":{"ownerReferences":[{"apiVersion":"%s","kind":"%s","name":"%s","uid":"%s","controller":true,"blockOwnerDeletion":true}],"uid":"%s"}}`,
+	if err != nil {
-		m.controllerKind.GroupVersion(), m.controllerKind.Kind,
+		return err
-		m.Controller.GetName(), m.Controller.GetUID(), rs.UID)
+	}
-	return m.rsControl.PatchReplicaSet(rs.Namespace, rs.Name, []byte(addControllerPatch))
+	return m.rsControl.PatchReplicaSet(rs.Namespace, rs.Name, patchBytes)
 }
 
 // ReleaseReplicaSet sends a patch to free the ReplicaSet from the control of the Deployment controller.
@@ -347,8 +353,11 @@ func (m *ReplicaSetControllerRefManager) AdoptReplicaSet(rs *apps.ReplicaSet) er
 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())
-	deleteOwnerRefPatch := fmt.Sprintf(`{"metadata":{"ownerReferences":[{"$patch":"delete","uid":"%s"}],"uid":"%s"}}`, m.Controller.GetUID(), replicaSet.UID)
+	patchBytes, err := deleteOwnerRefStrategicMergePatch(replicaSet.UID, m.Controller.GetUID())
-	err := m.rsControl.PatchReplicaSet(replicaSet.Namespace, replicaSet.Name, []byte(deleteOwnerRefPatch))
+	if err != nil {
+		return err
+	}
+	err = m.rsControl.PatchReplicaSet(replicaSet.Namespace, replicaSet.Name, patchBytes)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			// If the ReplicaSet no longer exists, ignore it.
@@ -470,11 +479,11 @@ func (m *ControllerRevisionControllerRefManager) AdoptControllerRevision(history
 	}
 	// Note that ValidateOwnerReferences() will reject this patch if another
 	// OwnerReference exists with controller=true.
-	addControllerPatch := fmt.Sprintf(
+	patchBytes, err := ownerRefControllerPatch(m.Controller, m.controllerKind, history.UID)
-		`{"metadata":{"ownerReferences":[{"apiVersion":"%s","kind":"%s","name":"%s","uid":"%s","controller":true,"blockOwnerDeletion":true}],"uid":"%s"}}`,
+	if err != nil {
-		m.controllerKind.GroupVersion(), m.controllerKind.Kind,
+		return err
-		m.Controller.GetName(), m.Controller.GetUID(), history.UID)
+	}
-	return m.crControl.PatchControllerRevision(history.Namespace, history.Name, []byte(addControllerPatch))
+	return m.crControl.PatchControllerRevision(history.Namespace, history.Name, patchBytes)
 }
 
 // ReleaseControllerRevision sends a patch to free the ControllerRevision from the control of its controller.
@@ -482,8 +491,12 @@ func (m *ControllerRevisionControllerRefManager) AdoptControllerRevision(history
 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())
-	deleteOwnerRefPatch := fmt.Sprintf(`{"metadata":{"ownerReferences":[{"$patch":"delete","uid":"%s"}],"uid":"%s"}}`, m.Controller.GetUID(), history.UID)
+	patchBytes, err := deleteOwnerRefStrategicMergePatch(history.UID, m.Controller.GetUID())
-	err := m.crControl.PatchControllerRevision(history.Namespace, history.Name, []byte(deleteOwnerRefPatch))
+	if err != nil {
+		return err
+	}
+
+	err = m.crControl.PatchControllerRevision(history.Namespace, history.Name, patchBytes)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			// If the ControllerRevision no longer exists, ignore it.
@@ -499,3 +512,64 @@ 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"`
+}
+
+func deleteOwnerRefStrategicMergePatch(dependentUID types.UID, ownerUIDs ...types.UID) ([]byte, error) {
+	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 nil, err
+	}
+	return patchBytes, nil
+}
+
+type objectForAddOwnerRefPatch struct {
+	Metadata objectMetaForPatch `json:"metadata"`
+}
+
+type objectMetaForPatch struct {
+	OwnerReferences []metav1.OwnerReference `json:"ownerReferences"`
+	UID             types.UID               `json:"uid"`
+}
+
+func ownerRefControllerPatch(controller metav1.Object, controllerKind schema.GroupVersionKind, uid types.UID) ([]byte, error) {
+	blockOwnerDeletion := true
+	isController := true
+	addControllerPatch := objectForAddOwnerRefPatch{
+		Metadata: objectMetaForPatch{
+			UID: uid,
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion:         controllerKind.GroupVersion().String(),
+					Kind:               controllerKind.Kind,
+					Name:               controller.GetName(),
+					UID:                controller.GetUID(),
+					Controller:         &isController,
+					BlockOwnerDeletion: &blockOwnerDeletion,
+				},
+			},
+		},
+	}
+	patchBytes, err := json.Marshal(&addControllerPatch)
+	if err != nil {
+		return nil, err
+	}
+	return patchBytes, nil
+}

pkg/controller/garbagecollector/patch.go

@@ -19,7 +19,6 @@ package garbagecollector
 import (
 	"encoding/json"
 	"fmt"
-	"strings"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -29,13 +28,31 @@ 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 []string
+	var pieces []map[string]string
 	for _, ownerUID := range ownerUIDs {
-		pieces = append(pieces, fmt.Sprintf(`{"$patch":"delete","uid":"%s"}`, ownerUID))
+		pieces = append(pieces, map[string]string{"$patch": "delete", "uid": string(ownerUID)})
 	}
-	patch := fmt.Sprintf(`{"metadata":{"ownerReferences":[%s],"uid":"%s"}}`, strings.Join(pieces, ","), dependentUID)
+	patch := objectForDeleteOwnerRefStrategicMergePatch{
-	return []byte(patch)
+		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

pkg/controller/history/controller_history.go

@@ -18,6 +18,7 @@ package history
 
 import (
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"hash/fnv"
 	"sort"
@@ -290,17 +291,43 @@ func (rh *realHistory) DeleteControllerRevision(revision *apps.ControllerRevisio
 	return rh.client.AppsV1().ControllerRevisions(revision.Namespace).Delete(revision.Name, nil)
 }
 
+type objectForPatch struct {
+	Metadata objectMetaForPatch `json:"metadata"`
+}
+
+// objectMetaForPatch define object meta struct for patch operation
+type objectMetaForPatch struct {
+	OwnerReferences []metav1.OwnerReference `json:"ownerReferences"`
+	UID             types.UID               `json:"uid"`
+}
+
 func (rh *realHistory) AdoptControllerRevision(parent metav1.Object, parentKind schema.GroupVersionKind, revision *apps.ControllerRevision) (*apps.ControllerRevision, error) {
+	blockOwnerDeletion := true
+	isController := true
 	// Return an error if the parent does not own the revision
 	if owner := metav1.GetControllerOfNoCopy(revision); owner != nil {
 		return nil, fmt.Errorf("attempt to adopt revision owned by %v", owner)
 	}
+	addControllerPatch := objectForPatch{
+		Metadata: objectMetaForPatch{
+			UID: revision.UID,
+			OwnerReferences: []metav1.OwnerReference{{
+				APIVersion:         parentKind.GroupVersion().String(),
+				Kind:               parentKind.Kind,
+				Name:               parent.GetName(),
+				UID:                parent.GetUID(),
+				Controller:         &isController,
+				BlockOwnerDeletion: &blockOwnerDeletion,
+			}},
+		},
+	}
+	patchBytes, err := json.Marshal(&addControllerPatch)
+	if err != nil {
+		return nil, err
+	}
 	// Use strategic merge patch to add an owner reference indicating a controller ref
 	return rh.client.AppsV1().ControllerRevisions(parent.GetNamespace()).Patch(revision.GetName(),
-		types.StrategicMergePatchType, []byte(fmt.Sprintf(
+		types.StrategicMergePatchType, patchBytes)
-			`{"metadata":{"ownerReferences":[{"apiVersion":"%s","kind":"%s","name":"%s","uid":"%s","controller":true,"blockOwnerDeletion":true}],"uid":"%s"}}`,
-			parentKind.GroupVersion().String(), parentKind.Kind,
-			parent.GetName(), parent.GetUID(), revision.UID)))
 }
 
 func (rh *realHistory) ReleaseControllerRevision(parent metav1.Object, revision *apps.ControllerRevision) (*apps.ControllerRevision, error) {

pkg/util/node/node.go

@@ -26,7 +26,7 @@ import (
 
 	"k8s.io/klog"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -158,14 +158,29 @@ func GetZoneKey(node *v1.Node) string {
 	return region + ":\x00:" + failureDomain
 }
 
+type nodeForConditionPatch struct {
+	Status nodeStatusForPatch `json:"status"`
+}
+
+type nodeStatusForPatch struct {
+	Conditions []v1.NodeCondition `json:"conditions"`
+}
+
 // SetNodeCondition updates specific node condition with patch operation.
 func SetNodeCondition(c clientset.Interface, node types.NodeName, condition v1.NodeCondition) error {
 	generatePatch := func(condition v1.NodeCondition) ([]byte, error) {
-		raw, err := json.Marshal(&[]v1.NodeCondition{condition})
+		patch := nodeForConditionPatch{
+			Status: nodeStatusForPatch{
+				Conditions: []v1.NodeCondition{
+					condition,
+				},
+			},
+		}
+		patchBytes, err := json.Marshal(&patch)
 		if err != nil {
 			return nil, err
 		}
-		return []byte(fmt.Sprintf(`{"status":{"conditions":%s}}`, raw)), nil
+		return patchBytes, nil
 	}
 	condition.LastHeartbeatTime = metav1.NewTime(time.Now())
 	patch, err := generatePatch(condition)
@@ -176,15 +191,27 @@ func SetNodeCondition(c clientset.Interface, node types.NodeName, condition v1.N
 	return err
 }
 
+type nodeForCIDRMergePatch struct {
+	Spec nodeSpecForMergePatch `json:"spec"`
+}
+
+type nodeSpecForMergePatch struct {
+	PodCIDR  string   `json:"podCIDR"`
+	PodCIDRs []string `json:"podCIDRs,omitempty"`
+}
+
 // PatchNodeCIDR patches the specified node's CIDR to the given value.
 func PatchNodeCIDR(c clientset.Interface, node types.NodeName, cidr string) error {
-	raw, err := json.Marshal(cidr)
+	patch := nodeForCIDRMergePatch{
+		Spec: nodeSpecForMergePatch{
+			PodCIDR: cidr,
+		},
+	}
+	patchBytes, err := json.Marshal(&patch)
 	if err != nil {
 		return fmt.Errorf("failed to json.Marshal CIDR: %v", err)
 	}
 
-	patchBytes := []byte(fmt.Sprintf(`{"spec":{"podCIDR":%s}}`, raw))
-
 	if _, err := c.CoreV1().Nodes().Patch(string(node), types.StrategicMergePatchType, patchBytes); err != nil {
 		return fmt.Errorf("failed to patch node CIDR: %v", err)
 	}
@@ -193,18 +220,18 @@ func PatchNodeCIDR(c clientset.Interface, node types.NodeName, cidr string) erro
 
 // PatchNodeCIDRs patches the specified node.CIDR=cidrs[0] and node.CIDRs to the given value.
 func PatchNodeCIDRs(c clientset.Interface, node types.NodeName, cidrs []string) error {
-	rawCidrs, err := json.Marshal(cidrs)
+	// set the pod cidrs list and set the old pod cidr field
-	if err != nil {
+	patch := nodeForCIDRMergePatch{
-		return fmt.Errorf("failed to json.Marshal CIDRs: %v", err)
+		Spec: nodeSpecForMergePatch{
+			PodCIDR:  cidrs[0],
+			PodCIDRs: cidrs,
+		},
 	}
 
-	rawCidr, err := json.Marshal(cidrs[0])
+	patchBytes, err := json.Marshal(&patch)
 	if err != nil {
 		return fmt.Errorf("failed to json.Marshal CIDR: %v", err)
 	}
-
-	// set the pod cidrs list and set the old pod cidr field
-	patchBytes := []byte(fmt.Sprintf(`{"spec":{"podCIDR":%s , "podCIDRs":%s}}`, rawCidr, rawCidrs))
 	klog.V(4).Infof("cidrs patch bytes are:%s", string(patchBytes))
 	if _, err := c.CoreV1().Nodes().Patch(string(node), types.StrategicMergePatchType, patchBytes); err != nil {
 		return fmt.Errorf("failed to patch node CIDR: %v", err)