From f7c3fce6a51ddbf84ff82e8eff21e188a4389d11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Tue, 10 Jan 2023 12:40:15 +0100 Subject: [PATCH] fix(kyaml): add lock for schema related globals --- kyaml/Makefile | 2 +- kyaml/openapi/openapi.go | 40 +++++++++++++++---- kyaml/openapi/openapi_test.go | 74 +++++++++++++++++++++++++++++++++-- 3 files changed, 105 insertions(+), 11 deletions(-) diff --git a/kyaml/Makefile b/kyaml/Makefile index 3fa93c088..d9619cbd3 100644 --- a/kyaml/Makefile +++ b/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 ./... diff --git a/kyaml/openapi/openapi.go b/kyaml/openapi/openapi.go index 1caea811d..37874f144 100644 --- a/kyaml/openapi/openapi.go +++ b/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 globalSchema openapiData +var ( + // 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 -var kubernetesOpenAPIVersion string + // kubernetesOpenAPIVersion specifies which builtin kubernetes schema to use. + kubernetesOpenAPIVersion string //nolint:gochecknoglobals -// customSchemaFile stores the custom OpenApi schema if it is provided -var customSchema []byte + // globalSchema contains global state information about the openapi + globalSchema openapiData //nolint:gochecknoglobals + + // customSchemaFile stores the custom OpenApi schema if it is provided + 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 } diff --git a/kyaml/openapi/openapi_test.go b/kyaml/openapi/openapi_test.go index abb4bb25f..801d2b5be 100644 --- a/kyaml/openapi/openapi_test.go +++ b/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() + }) +}