Merge pull request #104064 from manugupt1/unsafe-apf-controller

fix unsafe json construction for digestConfigObjects.

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go

@@ -29,6 +29,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -416,15 +417,14 @@ func (cfgCtlr *configController) digestConfigObjects(newPLs []*flowcontrol.Prior
 
 		// if we are going to issue an update, be sure we track every name we update so we know if we update it too often.
 		currResult.updatedItems.Insert(fsu.flowSchema.Name)
-
-		enc, err := json.Marshal(fsu.condition)
+		patchBytes, err := makeFlowSchemaConditionPatch(fsu.condition)
 		if err != nil {
 			// should never happen because these conditions are created here and well formed
 			panic(fmt.Sprintf("Failed to json.Marshall(%#+v): %s", fsu.condition, err.Error()))
 		}
-		klog.V(4).Infof("%s writing Condition %s to FlowSchema %s, which had ResourceVersion=%s, because its previous value was %s", cfgCtlr.name, string(enc), fsu.flowSchema.Name, fsu.flowSchema.ResourceVersion, fcfmt.Fmt(fsu.oldValue))
+		klog.V(4).Infof("%s writing Condition %s to FlowSchema %s, which had ResourceVersion=%s, because its previous value was %s, diff: %s",
+			cfgCtlr.name, fsu.condition, fsu.flowSchema.Name, fsu.flowSchema.ResourceVersion, fcfmt.Fmt(fsu.oldValue), cmp.Diff(fsu.oldValue, fsu.condition))
 		fsIfc := cfgCtlr.flowcontrolClient.FlowSchemas()
-		patchBytes := []byte(fmt.Sprintf(`{"status": {"conditions": [ %s ] } }`, string(enc)))
 		patchOptions := metav1.PatchOptions{FieldManager: cfgCtlr.asFieldManager}
 		_, err = fsIfc.Patch(context.TODO(), fsu.flowSchema.Name, apitypes.StrategicMergePatchType, patchBytes, patchOptions, "status")
 		if err != nil {
@@ -442,6 +442,20 @@ func (cfgCtlr *configController) digestConfigObjects(newPLs []*flowcontrol.Prior
 	return suggestedDelay, utilerrors.NewAggregate(errs)
 }
 
+// makeFlowSchemaConditionPatch takes in a condition and returns the patch status as a json.
+func makeFlowSchemaConditionPatch(condition flowcontrol.FlowSchemaCondition) ([]byte, error) {
+	o := struct {
+		Status flowcontrol.FlowSchemaStatus `json:"status"`
+	}{
+		Status: flowcontrol.FlowSchemaStatus{
+			Conditions: []flowcontrol.FlowSchemaCondition{
+				condition,
+			},
+		},
+	}
+	return json.Marshal(o)
+}
+
 // shouldDelayUpdate checks to see if a flowschema has been updated too often and returns true if a delay is needed.
 // Only invoke this in the one and only worker goroutine
 func (cfgCtlr *configController) shouldDelayUpdate(flowSchemaName string) bool {

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/patch_test.go

@@ -0,0 +1,88 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package flowcontrol
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+	"time"
+
+	"github.com/google/go-cmp/cmp"
+	flowcontrol "k8s.io/api/flowcontrol/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func Test_configController_generatePatchBytes(t *testing.T) {
+	now := time.Now().UTC()
+	tests := []struct {
+		name      string
+		condition flowcontrol.FlowSchemaCondition
+		want      []byte
+	}{
+		{
+			name: "check if only condition is parsed",
+			condition: flowcontrol.FlowSchemaCondition{
+				Type:               flowcontrol.FlowSchemaConditionDangling,
+				Status:             flowcontrol.ConditionTrue,
+				Reason:             "test reason",
+				Message:            "test none",
+				LastTransitionTime: metav1.NewTime(now),
+			},
+			want: []byte(fmt.Sprintf(`{"status":{"conditions":[{"type":"Dangling","status":"True","lastTransitionTime":"%s","reason":"test reason","message":"test none"}]}}`, now.Format(time.RFC3339))),
+		},
+		{
+			name: "check when message has double quotes",
+			condition: flowcontrol.FlowSchemaCondition{
+				Type:               flowcontrol.FlowSchemaConditionDangling,
+				Status:             flowcontrol.ConditionTrue,
+				Reason:             "test reason",
+				Message:            `test ""none`,
+				LastTransitionTime: metav1.NewTime(now),
+			},
+			want: []byte(fmt.Sprintf(`{"status":{"conditions":[{"type":"Dangling","status":"True","lastTransitionTime":"%s","reason":"test reason","message":"test \"\"none"}]}}`, now.Format(time.RFC3339))),
+		},
+		{
+			name: "check when message has a whitespace character that can be escaped",
+			condition: flowcontrol.FlowSchemaCondition{
+				Type:   flowcontrol.FlowSchemaConditionDangling,
+				Status: flowcontrol.ConditionTrue,
+				Reason: "test reason",
+				Message: `test 		none`,
+				LastTransitionTime: metav1.NewTime(now),
+			},
+			want: []byte(fmt.Sprintf(`{"status":{"conditions":[{"type":"Dangling","status":"True","lastTransitionTime":"%s","reason":"test reason","message":"test \t\tnone"}]}}`, now.Format(time.RFC3339))),
+		},
+		{
+			name: "check when a few fields (message & lastTransitionTime) are missing",
+			condition: flowcontrol.FlowSchemaCondition{
+				Type:   flowcontrol.FlowSchemaConditionDangling,
+				Status: flowcontrol.ConditionTrue,
+				Reason: "test reason",
+			},
+			want: []byte(`{"status":{"conditions":[{"type":"Dangling","status":"True","lastTransitionTime":null,"reason":"test reason"}]}}`),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, _ := makeFlowSchemaConditionPatch(tt.condition)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("makeFlowSchemaConditionPatch() got = %s, want %s; diff is %s", got, tt.want, cmp.Diff(tt.want, got))
+			}
+		})
+	}
+}