From 317c96a0627fdaa12ce1ed64df1d4014912046e3 Mon Sep 17 00:00:00 2001 From: tennisleng Date: Fri, 5 Dec 2025 20:01:59 -0500 Subject: [PATCH 1/2] Fix data race in PopulateRefs by copying Items and AdditionalProperties The populateRefs function was modifying result.Items.Schema and result.AdditionalProperties.Schema directly. Since result is a shallow copy of the input schema, these pointer fields still reference the original shared structures, causing a data race when multiple goroutines call PopulateRefs concurrently. Fix by creating copies of Items and AdditionalProperties before modifying their Schema field. --- .../src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go b/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go index 56e2a4bbd3a..808d6434156 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go @@ -86,7 +86,9 @@ func populateRefs(schemaOf func(ref string) (*spec.Schema, bool), visited sets.S } if populated != result.AdditionalProperties.Schema { changed = true - result.AdditionalProperties.Schema = populated + newProps := *result.AdditionalProperties + newProps.Schema = populated + result.AdditionalProperties = &newProps } } // schema is a list, populate its items @@ -97,7 +99,9 @@ func populateRefs(schemaOf func(ref string) (*spec.Schema, bool), visited sets.S } if populated != result.Items.Schema { changed = true - result.Items.Schema = populated + newItems := *result.Items + newItems.Schema = populated + result.Items = &newItems } } if changed { From ad7c5fda15224b35d92e0aeb6737e296a69fab91 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 6 Feb 2026 15:03:14 +0100 Subject: [PATCH 2/2] PopulateRefs unit test The test fails on master with: WARNING: DATA RACE Read at 0x00c00038ccf8 by goroutine 10: k8s.io/apiserver/pkg/cel/openapi/resolver.populateRefs() /nvme/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go:82 +0xb0a k8s.io/apiserver/pkg/cel/openapi/resolver.PopulateRefs() /nvme/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go:37 +0x325 k8s.io/apiserver/pkg/cel/openapi/resolver.TestPopulateRefs.func2() /nvme/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs_test.go:99 +0x4f sync.(*WaitGroup).Go.func1() /nvme/gopath/go-1.25.2/src/sync/waitgroup.go:239 +0x5d Previous write at 0x00c00038ccf8 by goroutine 11: k8s.io/apiserver/pkg/cel/openapi/resolver.populateRefs() /nvme/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go:89 +0xc1c k8s.io/apiserver/pkg/cel/openapi/resolver.PopulateRefs() /nvme/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs.go:37 +0x325 k8s.io/apiserver/pkg/cel/openapi/resolver.TestPopulateRefs.func3() /nvme/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs_test.go:106 +0x3a sync.(*WaitGroup).Go.func1() /nvme/gopath/go-1.25.2/src/sync/waitgroup.go:239 +0x5d ... --- FAIL: TestPopulateRefs (0.00s) refs_test.go:115: read-only input schema got modified (- original, + modification): &spec.Schema{ VendorExtensible: {}, SchemaProps: spec.SchemaProps{ ... // 22 identical fields MinProperties: nil, Required: nil, Items: &spec.SchemaOrArray{ Schema: &spec.Schema{ VendorExtensible: {}, SchemaProps: spec.SchemaProps{ - ID: "", + ID: "item", Ref: spec.Ref{ Ref: jsonreference.Ref{ ... // 2 ignored fields HasFullURL: false, - HasURLPathOnly: true, + HasURLPathOnly: false, HasFragmentOnly: false, HasFileScheme: false, HasFullFilePath: false, }, }, Schema: "", Description: "", ... // 31 identical fields }, SwaggerSchemaProps: {}, ExtraProps: nil, }, Schemas: nil, }, AllOf: nil, OneOf: nil, AnyOf: nil, Not: nil, Properties: {"a": {SchemaProps: {Ref: {Ref: {HasURLPathOnly: true, ...}}}}}, AdditionalProperties: &spec.SchemaOrBool{ Allows: false, Schema: &spec.Schema{ VendorExtensible: {}, SchemaProps: spec.SchemaProps{ - ID: "", + ID: "additional", Ref: spec.Ref{ Ref: jsonreference.Ref{ ... // 2 ignored fields HasFullURL: false, - HasURLPathOnly: true, + HasURLPathOnly: false, HasFragmentOnly: false, HasFileScheme: false, HasFullFilePath: false, }, }, Schema: "", Description: "", ... // 31 identical fields }, SwaggerSchemaProps: {}, ExtraProps: nil, }, }, PatternProperties: nil, Dependencies: nil, ... // 2 identical fields }, SwaggerSchemaProps: {}, ExtraProps: nil, } testing.go:1617: race detected during execution of test FAIL --- staging/src/k8s.io/apiserver/go.mod | 2 +- .../pkg/cel/openapi/resolver/refs_test.go | 124 ++++++++++++++++++ 2 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs_test.go diff --git a/staging/src/k8s.io/apiserver/go.mod b/staging/src/k8s.io/apiserver/go.mod index 6a4d0e23863..099e9a47f3c 100644 --- a/staging/src/k8s.io/apiserver/go.mod +++ b/staging/src/k8s.io/apiserver/go.mod @@ -13,6 +13,7 @@ require ( github.com/emicklei/go-restful/v3 v3.13.0 github.com/fsnotify/fsnotify v1.9.0 github.com/go-logr/logr v1.4.3 + github.com/go-openapi/jsonreference v0.20.2 github.com/google/cel-go v0.26.0 github.com/google/gnostic-models v0.7.0 github.com/google/go-cmp v0.7.0 @@ -76,7 +77,6 @@ require ( github.com/fxamacker/cbor/v2 v2.9.0 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-openapi/jsonpointer v0.21.0 // indirect - github.com/go-openapi/jsonreference v0.20.2 // indirect github.com/go-openapi/swag v0.23.0 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/golang-jwt/jwt/v5 v5.3.0 // indirect diff --git a/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs_test.go b/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs_test.go new file mode 100644 index 00000000000..c2f0fc5594f --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/cel/openapi/resolver/refs_test.go @@ -0,0 +1,124 @@ +/* +Copyright 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 resolver + +import ( + "sync" + "testing" + + "github.com/go-openapi/jsonreference" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "k8s.io/kube-openapi/pkg/validation/spec" +) + +func schemaRef(ref string) *spec.Schema { + return &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Ref: spec.MustCreateRef(ref), + }, + } +} + +func newSchema(ref string) *spec.Schema { + return &spec.Schema{ + SchemaProps: spec.SchemaProps{ + ID: ref, + }, + } +} + +// rootSchema returns a schema with references in all places where TestPopulateRefs +// resolves references. +func rootSchema() *spec.Schema { + return &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{ + "a": *schemaRef("prop"), + }, + AdditionalProperties: &spec.SchemaOrBool{ + Schema: schemaRef("additional"), + }, + Items: &spec.SchemaOrArray{ + Schema: schemaRef("item"), + }, + }, + } +} + +func resolvedRootSchema() *spec.Schema { + return &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{ + "a": *newSchema("prop"), + }, + AdditionalProperties: &spec.SchemaOrBool{ + Schema: newSchema("additional"), + }, + Items: &spec.SchemaOrArray{ + Schema: newSchema("item"), + }, + }, + } +} + +func TestPopulateRefs(t *testing.T) { + schemas := map[string]*spec.Schema{ + "root": rootSchema(), + } + + // Add one schema for each of the references above, with an ID that allows + // verifying that the right schema got resolved. + for _, ref := range []string{"prop", "additional", "item"} { + schemas[ref] = newSchema(ref) + } + + schemaOf := func(ref string) (*spec.Schema, bool) { + schema, ok := schemas[ref] + return schema, ok + } + + // Comparing the root schema below detects mutations where the data is semantically different, + // but it cannot detect undesired writes where the thing being written is the same (unlikely, + // but it could happen). Therefore we run two PopulateRefs calls and let the data race + // detector warn about concurrent, uncoordinated writes. + var wg sync.WaitGroup + var actualResolved *spec.Schema + wg.Go(func() { + schema, err := PopulateRefs(schemaOf, "root") + if err != nil { + t.Errorf("first PopulateRefs failed: %v", err) + } + actualResolved = schema + }) + wg.Go(func() { + _, err := PopulateRefs(schemaOf, "root") + if err != nil { + t.Errorf("second PopulateRefs failed: %v", err) + } + }) + wg.Wait() + + if diff := cmp.Diff(resolvedRootSchema(), actualResolved, cmpopts.IgnoreUnexported(jsonreference.Ref{})); diff != "" { + t.Errorf("unexpected resolved schema (- want, + got):\n%s", diff) + } + + if diff := cmp.Diff(rootSchema(), schemas["root"], cmpopts.IgnoreUnexported(jsonreference.Ref{})); diff != "" { + t.Errorf("read-only input schema got modified (- original, + modification):\n%s", diff) + } +}