From 8fb33338635565f2f755a4557b94c26039c175d9 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Wed, 26 Jan 2022 13:54:26 -0500 Subject: [PATCH] Revert "Merge pull request #107456 from tkashem/apf-ssa" This reverts commit 6faa4f001008a5a29476f5722f66430c35f48229, reversing changes made to 33a2c50bce334467640e016f68cf19e9382ba1a7. --- .../pkg/util/flowcontrol/apf_controller.go | 39 ++++---- .../pkg/util/flowcontrol/patch_test.go | 88 +++++++++++++++++++ .../apiserver/apply/reset_fields_test.go | 6 +- 3 files changed, 114 insertions(+), 19 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/util/flowcontrol/patch_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go b/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go index 8ea4000eebc..78785d9a2dc 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go +++ b/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go @@ -20,6 +20,7 @@ import ( "context" "crypto/sha256" "encoding/binary" + "encoding/json" "errors" "fmt" "math" @@ -33,6 +34,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + apitypes "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" @@ -51,7 +53,6 @@ import ( "k8s.io/utils/clock" flowcontrol "k8s.io/api/flowcontrol/v1beta2" - flowcontrolapplyconfiguration "k8s.io/client-go/applyconfigurations/flowcontrol/v1beta2" flowcontrolclient "k8s.io/client-go/kubernetes/typed/flowcontrol/v1beta2" flowcontrollister "k8s.io/client-go/listers/flowcontrol/v1beta2" ) @@ -434,16 +435,18 @@ 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) + 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())) + } if klog.V(4).Enabled() { 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() - applyOptions := metav1.ApplyOptions{FieldManager: cfgCtlr.asFieldManager, Force: true} - - // the condition field in fsStatusUpdate holds the new condition we want to update. - // TODO: this will break when we have multiple conditions for a flowschema - _, err := fsIfc.ApplyStatus(context.TODO(), toFlowSchemaApplyConfiguration(fsu), applyOptions) + patchOptions := metav1.PatchOptions{FieldManager: cfgCtlr.asFieldManager} + _, err = fsIfc.Patch(context.TODO(), fsu.flowSchema.Name, apitypes.StrategicMergePatchType, patchBytes, patchOptions, "status") if err != nil { if apierrors.IsNotFound(err) { // This object has been deleted. A notification is coming @@ -459,18 +462,18 @@ func (cfgCtlr *configController) digestConfigObjects(newPLs []*flowcontrol.Prior return suggestedDelay, utilerrors.NewAggregate(errs) } -func toFlowSchemaApplyConfiguration(fsUpdate fsStatusUpdate) *flowcontrolapplyconfiguration.FlowSchemaApplyConfiguration { - condition := flowcontrolapplyconfiguration.FlowSchemaCondition(). - WithType(fsUpdate.condition.Type). - WithStatus(fsUpdate.condition.Status). - WithReason(fsUpdate.condition.Reason). - WithLastTransitionTime(fsUpdate.condition.LastTransitionTime). - WithMessage(fsUpdate.condition.Message) - - return flowcontrolapplyconfiguration.FlowSchema(fsUpdate.flowSchema.Name). - WithStatus(flowcontrolapplyconfiguration.FlowSchemaStatus(). - WithConditions(condition), - ) +// 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. diff --git a/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/patch_test.go b/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/patch_test.go new file mode 100644 index 00000000000..b468b6d3b0d --- /dev/null +++ b/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/v1beta2" + 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)) + } + }) + } +} diff --git a/test/integration/apiserver/apply/reset_fields_test.go b/test/integration/apiserver/apply/reset_fields_test.go index c77e7aec0a2..55afd075f01 100644 --- a/test/integration/apiserver/apply/reset_fields_test.go +++ b/test/integration/apiserver/apply/reset_fields_test.go @@ -68,7 +68,11 @@ var resetFieldsStatusData = map[schema.GroupVersionResource]string{ // resetFieldsStatusDefault conflicts with statusDefault const resetFieldsStatusDefault = `{"status": {"conditions": [{"type": "MyStatus", "status":"False"}]}}` -var resetFieldsSkippedResources = map[string]struct{}{} +var resetFieldsSkippedResources = map[string]struct{}{ + // TODO: flowschemas is flaking, + // possible bug in the flowschemas controller. + "flowschemas": {}, +} // noConflicts is the set of reources for which // a conflict cannot occur.