Merge pull request #136802 from pohly/fix-data-race-refs-populaterefs

Fix data race in PopulateRefs by copying Items and AdditionalProperties

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

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 {

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)
+	}
+}