From 370e18fdd102542beeb6d01a58666bd8a198f3f1 Mon Sep 17 00:00:00 2001
From: Alexander Zielenski <zielenski@google.com>
Date: Wed, 15 Dec 2021 15:15:22 -0800
Subject: [PATCH] fix bug which causes openapi builder to lookup cached
 possibly incorrect schema

---
 .../pkg/controller/openapi/builder/builder.go | 36 +++++++++++--------
 .../openapi/builder/builder_test.go           | 28 +++++++++++++++
 2 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go
index 3788b770150..d935eb2d919 100644
--- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go
+++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go
@@ -80,6 +80,7 @@ func refForOpenAPIVersion(schemaRef string, v2 bool) string {
 }
 
 var definitions map[string]common.OpenAPIDefinition
+var definitionsV3 map[string]common.OpenAPIDefinition
 var buildDefinitions sync.Once
 var namer *openapi.DefinitionNamer
 
@@ -460,26 +461,31 @@ func addEmbeddedProperties(s *spec.Schema, opts Options) {
 // getDefinition gets definition for given Kubernetes type. This function is extracted from
 // kube-openapi builder logic
 func getDefinition(name string, v2 bool) spec.Schema {
-	buildDefinitions.Do(generateBuildDefinitionsFunc(v2))
-	return definitions[name].Schema
+	buildDefinitions.Do(generateBuildDefinitionsFunc)
+
+	if v2 {
+		return definitions[name].Schema
+	}
+	return definitionsV3[name].Schema
 }
 
 func withDescription(s spec.Schema, desc string) spec.Schema {
 	return *s.WithDescription(desc)
 }
 
-func generateBuildDefinitionsFunc(v2 bool) func() {
-	return func() {
-		namer = openapi.NewDefinitionNamer(runtime.NewScheme())
-		definitions = utilopenapi.GetOpenAPIDefinitionsWithoutDisabledFeatures(generatedopenapi.GetOpenAPIDefinitions)(func(name string) spec.Ref {
-			defName, _ := namer.GetDefinitionName(name)
-			prefix := v3DefinitionPrefix
-			if v2 {
-				prefix = definitionPrefix
-			}
-			return spec.MustCreateRef(prefix + common.EscapeJsonPointer(defName))
-		})
-	}
+func generateBuildDefinitionsFunc() {
+	namer = openapi.NewDefinitionNamer(runtime.NewScheme())
+	definitionsV3 = utilopenapi.GetOpenAPIDefinitionsWithoutDisabledFeatures(generatedopenapi.GetOpenAPIDefinitions)(func(name string) spec.Ref {
+		defName, _ := namer.GetDefinitionName(name)
+		prefix := v3DefinitionPrefix
+		return spec.MustCreateRef(prefix + common.EscapeJsonPointer(defName))
+	})
+
+	definitions = utilopenapi.GetOpenAPIDefinitionsWithoutDisabledFeatures(generatedopenapi.GetOpenAPIDefinitions)(func(name string) spec.Ref {
+		defName, _ := namer.GetDefinitionName(name)
+		prefix := definitionPrefix
+		return spec.MustCreateRef(prefix + common.EscapeJsonPointer(defName))
+	})
 }
 
 // addTypeMetaProperties adds Kubernetes-specific type meta properties to input schema:
@@ -528,7 +534,7 @@ func (b *builder) getOpenAPIConfig(v2 bool) *common.Config {
 		},
 		GetOperationIDAndTags: openapi.GetOperationIDAndTags,
 		GetDefinitionName: func(name string) (string, spec.Extensions) {
-			buildDefinitions.Do(generateBuildDefinitionsFunc(v2))
+			buildDefinitions.Do(generateBuildDefinitionsFunc)
 			return namer.GetDefinitionName(name)
 		},
 		GetDefinitions: func(ref common.ReferenceCallback) map[string]common.OpenAPIDefinition {
diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go
index d923a4f391c..ebef7e224d6 100644
--- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go
+++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go
@@ -18,6 +18,7 @@ package builder
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -802,3 +803,30 @@ func TestBuildOpenAPIV3(t *testing.T) {
 		})
 	}
 }
+
+// Tests that getDefinition's ref building function respects the v2 flag for v2
+// vs v3 operations
+// This bug did not surface since we only so far look up types which do not make
+// use of refs
+func TestGetDefinitionRefPrefix(t *testing.T) {
+	// A bug was triggered by generating the cached definition map for one version,
+	// but then performing a looking on another. The map is generated upon
+	// the first call to getDefinition
+
+	// ManagedFieldsEntry's Time field is known to use arefs
+	managedFieldsTypePath := "k8s.io/apimachinery/pkg/apis/meta/v1.ManagedFieldsEntry"
+
+	v2Ref := getDefinition(managedFieldsTypePath, true).SchemaProps.Properties["time"].SchemaProps.Ref
+	v3Ref := getDefinition(managedFieldsTypePath, false).SchemaProps.Properties["time"].SchemaProps.Ref
+
+	v2String := v2Ref.String()
+	v3String := v3Ref.String()
+
+	if !strings.HasPrefix(v3String, v3DefinitionPrefix) {
+		t.Errorf("v3 ref (%v) does not have the correct prefix (%v)", v3String, v3DefinitionPrefix)
+	}
+
+	if !strings.HasPrefix(v2String, definitionPrefix) {
+		t.Errorf("v2 ref (%v) does not have the correct prefix (%v)", v2String, definitionPrefix)
+	}
+}