From a5df442be788b84c0c437d6f8c123a2a26039f64 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 29 Jun 2023 22:28:07 +0200 Subject: [PATCH] flowcontrol: copy object before passing it to client-go Create to avoid data race This is similar to 5e1c6cd0d4ffb, except that here generics are involved: because client-go Create under the hood mutates its input value temporarily, callers must make a copy if the object is read from some other goroutine. The race as reported by "go test -race" for test/integration/examples.TestAggregatedAPIServerRejectRedirectResponse is: WARNING: DATA RACE Read at 0x00c000556010 by goroutine 16128: reflect.Value.String() /usr/local/go/src/reflect/value.go:2565 +0x216 encoding/json.stringEncoder() /usr/local/go/src/encoding/json/encode.go:645 +0x223 encoding/json.structEncoder.encode() /usr/local/go/src/encoding/json/encode.go:759 +0x2ba encoding/json.structEncoder.encode-fm() :1 +0xdb encoding/json.ptrEncoder.encode() /usr/local/go/src/encoding/json/encode.go:943 +0x382 encoding/json.ptrEncoder.encode-fm() :1 +0x90 encoding/json.(*encodeState).reflectValue() /usr/local/go/src/encoding/json/encode.go:358 +0x88 encoding/json.(*encodeState).marshal() /usr/local/go/src/encoding/json/encode.go:330 +0x224 encoding/json.Marshal() /usr/local/go/src/encoding/json/encode.go:161 +0xf9 k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/util/flowcontrol/format.ToJSON() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/util/flowcontrol/format/formatting.go:81 +0x44 k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/util/flowcontrol/format.Stringer.String() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/util/flowcontrol/format/formatting.go:68 +0x5a4 k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/util/flowcontrol/format.(*Stringer).String() :1 +0x4b fmt.(*pp).handleMethods() /usr/local/go/src/fmt/print.go:673 +0x4db fmt.(*pp).printArg() /usr/local/go/src/fmt/print.go:756 +0xce4 fmt.(*pp).doPrintf() /usr/local/go/src/fmt/print.go:1077 +0x599 fmt.Fprintf() /usr/local/go/src/fmt/print.go:224 +0x7e k8s.io/kubernetes/vendor/k8s.io/klog/v2.(*loggingT).printfDepth() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:733 +0x216 k8s.io/kubernetes/vendor/k8s.io/klog/v2.(*loggingT).printf() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:718 +0xcc k8s.io/kubernetes/vendor/k8s.io/klog/v2.Verbose.Infof() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:1418 +0x64 k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/util/flowcontrol.(*cfgMeal).digestFlowSchemasLocked() ... Previous write at 0x00c000556010 by goroutine 15271: k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/apis/meta/v1.(*TypeMeta).SetGroupVersionKind() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/meta.go:121 +0x193 k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/runtime.WithVersionEncoder.Encode() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/runtime/helper.go:239 +0x325 k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/runtime.(*WithVersionEncoder).Encode() :1 +0xfb k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/runtime.Encode() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/runtime/codec.go:49 +0xb3 k8s.io/kubernetes/vendor/k8s.io/client-go/rest.(*Request).Body() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/client-go/rest/request.go:470 +0x884 k8s.io/kubernetes/vendor/k8s.io/client-go/kubernetes/typed/flowcontrol/v1beta3.(*flowSchemas).Create() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/client-go/kubernetes/typed/flowcontrol/v1beta3/flowschema.go:118 +0x23c k8s.io/kubernetes/pkg/registry/flowcontrol/ensurer.(*objectOps[...]).Create() :1 +0x10f k8s.io/kubernetes/pkg/registry/flowcontrol/ensurer.EnsureConfiguration[...]() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/registry/flowcontrol/ensurer/strategy.go:272 +0x1b5 k8s.io/kubernetes/pkg/registry/flowcontrol/ensurer.EnsureConfigurations[...]() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/registry/flowcontrol/ensurer/strategy.go:247 +0xf3 k8s.io/kubernetes/pkg/registry/flowcontrol/rest.ensureMandatoryConfiguration() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/registry/flowcontrol/rest/storage_flowcontrol.go:230 +0x59e k8s.io/kubernetes/pkg/registry/flowcontrol/rest.ensure() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/registry/flowcontrol/rest/storage_flowcontrol.go:202 +0x1b2 k8s.io/kubernetes/pkg/registry/flowcontrol/rest.(*bootstrapConfigurationEnsurer).ensureAPFBootstrapConfiguration.func1.1() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/registry/flowcontrol/rest/storage_flowcontrol.go:159 +0xe4 k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.runConditionWithCrashProtectionWithContext() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:154 +0x7b --- pkg/registry/flowcontrol/ensurer/strategy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/registry/flowcontrol/ensurer/strategy.go b/pkg/registry/flowcontrol/ensurer/strategy.go index 680c68e1661..ff0b9c2717f 100644 --- a/pkg/registry/flowcontrol/ensurer/strategy.go +++ b/pkg/registry/flowcontrol/ensurer/strategy.go @@ -269,7 +269,7 @@ func EnsureConfiguration[ObjectType configurationObjectType](ctx context.Context } // we always re-create a missing configuration object - if _, err = ops.Create(ctx, bootstrap, metav1.CreateOptions{FieldManager: fieldManager}); err == nil { + if _, err = ops.Create(ctx, ops.DeepCopy(bootstrap), metav1.CreateOptions{FieldManager: fieldManager}); err == nil { klog.V(2).InfoS(fmt.Sprintf("Successfully created %s", bootstrap.GetObjectKind().GroupVersionKind().Kind), "type", configurationType, "name", name) return nil }