fix(kyaml): add lock for schema related globals

kyaml/Makefile

@@ -33,7 +33,7 @@ lint: $(MYGOBIN)/golangci-lint
 	  --skip-dirs internal/forked
 
 test:
-	go test -v -cover ./...
+	go test -race -v -cover ./...
 
 fix:
 	go fix ./...

kyaml/openapi/openapi.go

@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"strings"
+	"sync"
 
 	openapi_v2 "github.com/google/gnostic/openapiv2"
 	"google.golang.org/protobuf/proto"
@@ -21,14 +22,27 @@ import (
 	k8syaml "sigs.k8s.io/yaml"
 )
 
-// globalSchema contains global state information about the openapi
+var (
-var globalSchema openapiData
+	// schemaLock is the lock for schema related globals.
+	//
+	// NOTE: This lock helps with preventing panics that might occur due to the data
+	// race that concurrent access on this variable might cause but it doesn't
+	// fully fix the issue described in https://github.com/kubernetes-sigs/kustomize/issues/4824.
+	// For instance concurrently running goroutines where each of them calls SetSchema()
+	// and/or GetSchemaVersion might end up received nil errors (success) whereas the
+	// seconds one would overwrite the global variable that has been written by the
+	// first one.
+	schemaLock sync.RWMutex //nolint:gochecknoglobals
 
-// kubernetesOpenAPIVersion specifies which builtin kubernetes schema to use
+	// kubernetesOpenAPIVersion specifies which builtin kubernetes schema to use.
-var kubernetesOpenAPIVersion string
+	kubernetesOpenAPIVersion string //nolint:gochecknoglobals
+
+	// globalSchema contains global state information about the openapi
+	globalSchema openapiData //nolint:gochecknoglobals
 
 	// customSchemaFile stores the custom OpenApi schema if it is provided
-var customSchema []byte
+	customSchema []byte //nolint:gochecknoglobals
+)
 
 // openapiData contains the parsed openapi state.  this is in a struct rather than
 // a list of vars so that it can be reset from tests.
@@ -278,9 +292,12 @@ func AddSchema(s []byte) error {
 
 // ResetOpenAPI resets the openapi data to empty
 func ResetOpenAPI() {
+	schemaLock.Lock()
+	defer schemaLock.Unlock()
+
 	globalSchema = openapiData{}
-	kubernetesOpenAPIVersion = ""
 	customSchema = nil
+	kubernetesOpenAPIVersion = ""
 }
 
 // AddDefinitions adds the definitions to the global schema.
@@ -551,6 +568,9 @@ const (
 
 // SetSchema sets the kubernetes OpenAPI schema version to use
 func SetSchema(openAPIField map[string]string, schema []byte, reset bool) error {
+	schemaLock.Lock()
+	defer schemaLock.Unlock()
+
 	// this should only be set once
 	schemaIsSet := (kubernetesOpenAPIVersion != "") || customSchema != nil
 	if schemaIsSet && !reset {
@@ -588,6 +608,9 @@ func SetSchema(openAPIField map[string]string, schema []byte, reset bool) error
 
 // GetSchemaVersion returns what kubernetes OpenAPI version is being used
 func GetSchemaVersion() string {
+	schemaLock.RLock()
+	defer schemaLock.RUnlock()
+
 	switch {
 	case kubernetesOpenAPIVersion == "" && customSchema == nil:
 		return kubernetesOpenAPIDefaultVersion
@@ -600,6 +623,9 @@ func GetSchemaVersion() string {
 
 // initSchema parses the json schema
 func initSchema() {
+	schemaLock.Lock()
+	defer schemaLock.Unlock()
+
 	if globalSchema.schemaInit {
 		return
 	}

kyaml/openapi/openapi_test.go

@@ -6,10 +6,12 @@ package openapi
 import (
 	"fmt"
 	"os"
+	"sync"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
@@ -120,7 +122,7 @@ openAPI:
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
-	if !assert.NoError(t, os.WriteFile(f.Name(), []byte(inputyaml), 0600)) {
+	if !assert.NoError(t, os.WriteFile(f.Name(), []byte(inputyaml), 0o600)) {
 		t.FailNow()
 	}
 
@@ -172,7 +174,7 @@ openAPI:
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
-	if !assert.NoError(t, os.WriteFile(f.Name(), []byte(inputyaml), 0600)) {
+	if !assert.NoError(t, os.WriteFile(f.Name(), []byte(inputyaml), 0o600)) {
 		t.FailNow()
 	}
 
@@ -207,7 +209,7 @@ kind: Example
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
-	if !assert.NoError(t, os.WriteFile(f.Name(), []byte(inputyaml), 0600)) {
+	if !assert.NoError(t, os.WriteFile(f.Name(), []byte(inputyaml), 0o600)) {
 		t.FailNow()
 	}
 
@@ -325,3 +327,69 @@ func TestIsNamespaceScoped_custom(t *testing.T) {
 	assert.True(t, isFound)
 	assert.True(t, isNamespaceable)
 }
+
+func TestCanSetAndResetSchemaConcurrently(t *testing.T) {
+	t.Run("SetSchema doesn't cause a data race when called concurrently", func(t *testing.T) {
+		set := func(wg *sync.WaitGroup) {
+			defer wg.Done()
+			err := SetSchema(
+				map[string]string{
+					"/apis/custom.io/v1": "true",
+				},
+				[]byte(`
+				{
+				  "definitions": {},
+				  "paths": {
+					"/apis/custom.io/v1/namespaces/{namespace}/customs/{name}": {
+					  "get": {
+						"x-kubernetes-action": "get",
+						"x-kubernetes-group-version-kind": {
+						  "group": "custom.io",
+						  "kind": "Custom",
+						  "version": "v1"
+						}
+					  }
+					},
+					"/apis/custom.io/v1/clustercustoms": {
+					  "get": {
+						"x-kubernetes-action": "get",
+						"x-kubernetes-group-version-kind": {
+						  "group": "custom.io",
+						  "kind": "ClusterCustom",
+						  "version": "v1"
+						}
+					  }
+					}
+				  }
+				}
+			`),
+				true,
+			)
+			require.NoError(t, err)
+		}
+
+		var wg sync.WaitGroup
+		require.NotPanics(t, func() {
+			for i := 0; i < 100; i++ {
+				wg.Add(1)
+				go set(&wg)
+			}
+		})
+		wg.Wait()
+	})
+
+	t.Run("ResetOpenAPI doesn't cause a data race when called concurrently", func(t *testing.T) {
+		reset := func(wg *sync.WaitGroup) {
+			defer wg.Done()
+			ResetOpenAPI()
+		}
+		var wg sync.WaitGroup
+		require.NotPanics(t, func() {
+			for i := 0; i < 100; i++ {
+				wg.Add(1)
+				go reset(&wg)
+			}
+		})
+		wg.Wait()
+	})
+}