refactor to use Schema(contentType)

Kubernetes-commit: e6e6dd826d76a2b19515cd907062f1f74f9e52c2

discovery/cached/disk/cached_discovery_test.go

@@ -29,6 +29,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/version"
 	"k8s.io/client-go/discovery"
@@ -169,20 +170,31 @@ func TestOpenAPIDiskCache(t *testing.T) {
 	paths, err := openapiClient.Paths()
 	require.NoError(t, err)
 	assert.Equal(t, 1, fakeServer.RequestCounters["/openapi/v3"])
-
 	require.Greater(t, len(paths), 0)
+
+	contentTypes := []string{
+		runtime.ContentTypeJSON, openapi.ContentTypeOpenAPIV3PB,
+	}
+
+	for _, contentType := range contentTypes {
+		t.Run(contentType, func(t *testing.T) {
+			// Reset all counters (cant just reset to nil since reference is shared)
+			for k := range fakeServer.RequestCounters {
+				delete(fakeServer.RequestCounters, k)
+			}
+
 			i := 0
 			for k, v := range paths {
 				i++
 
-		_, err = v.SchemaJSON()
+				_, err = v.Schema(contentType)
 				assert.NoError(t, err)
 
 				path := "/openapi/v3/" + strings.TrimPrefix(k, "/")
 				assert.Equal(t, 1, fakeServer.RequestCounters[path])
 
 				// Ensure schema call is served from memory
-		_, err = v.SchemaJSON()
+				_, err = v.Schema(contentType)
 				assert.NoError(t, err)
 				assert.Equal(t, 1, fakeServer.RequestCounters[path])
 
@@ -196,11 +208,14 @@ func TestOpenAPIDiskCache(t *testing.T) {
 				}
 
 				// Ensure schema call is still served from disk
-		_, err = newPaths[k].SchemaJSON()
+				_, err = newPaths[k].Schema(contentType)
 				assert.NoError(t, err)
-		assert.Equal(t, 1+i, fakeServer.RequestCounters["/openapi/v3"])
+				assert.Equal(t, i, fakeServer.RequestCounters["/openapi/v3"])
 				assert.Equal(t, 1, fakeServer.RequestCounters[path])
 			}
+		})
+	}
+
 }
 
 type fakeDiscoveryClient struct {

discovery/cached/memory/memcache_test.go

@@ -27,8 +27,10 @@ import (
 	"github.com/stretchr/testify/require"
 	errorsutil "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/discovery/fake"
+	"k8s.io/client-go/openapi"
 	"k8s.io/client-go/rest"
 	testutil "k8s.io/client-go/util/testing"
 )
@@ -415,8 +417,14 @@ func TestOpenAPIMemCache(t *testing.T) {
 	paths, err := openapiClient.Paths()
 	require.NoError(t, err)
 
+	contentTypes := []string{
+		runtime.ContentTypeJSON, openapi.ContentTypeOpenAPIV3PB,
+	}
+
+	for _, contentType := range contentTypes {
+		t.Run(contentType, func(t *testing.T) {
 			for k, v := range paths {
-		original, err := v.SchemaPB()
+				original, err := v.Schema(contentType)
 				if !assert.NoError(t, err) {
 					continue
 				}
@@ -426,7 +434,7 @@ func TestOpenAPIMemCache(t *testing.T) {
 					continue
 				}
 
-		schemaAgain, err := pathsAgain[k].SchemaPB()
+				schemaAgain, err := pathsAgain[k].Schema(contentType)
 				if !assert.NoError(t, err) {
 					continue
 				}
@@ -442,7 +450,7 @@ func TestOpenAPIMemCache(t *testing.T) {
 					continue
 				}
 
-		schemaAgain, err = pathsAgain[k].SchemaPB()
+				schemaAgain, err = pathsAgain[k].Schema(contentType)
 				if !assert.NoError(t, err) {
 					continue
 				}
@@ -451,4 +459,6 @@ func TestOpenAPIMemCache(t *testing.T) {
 				assert.True(t, reflect.ValueOf(original).Pointer() != reflect.ValueOf(schemaAgain).Pointer())
 				assert.Equal(t, original, schemaAgain)
 			}
+		})
+	}
 }

discovery/discovery_client_test.go

@@ -29,15 +29,20 @@ import (
 	"github.com/gogo/protobuf/proto"
 	openapi_v2 "github.com/google/gnostic/openapiv2"
 	openapi_v3 "github.com/google/gnostic/openapiv3"
+	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	golangproto "google.golang.org/protobuf/proto"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/version"
+	"k8s.io/client-go/openapi"
 	restclient "k8s.io/client-go/rest"
 	testutil "k8s.io/client-go/util/testing"
+	"k8s.io/kube-openapi/pkg/spec3"
 )
 
 func TestGetServerVersion(t *testing.T) {
@@ -537,7 +542,7 @@ func openapiSchemaFakeServer(t *testing.T) (*httptest.Server, error) {
 	return server, nil
 }
 
-func openapiV3SchemaFakeServer(t *testing.T) (*httptest.Server, map[string]*openapi_v3.Document, error) {
+func openapiV3SchemaFakeServer(t *testing.T) (*httptest.Server, map[string]*spec3.OpenAPI, error) {
 	res, err := testutil.NewFakeOpenAPIV3Server("testdata")
 	if err != nil {
 		return nil, nil, err
@@ -576,33 +581,65 @@ func TestGetOpenAPISchemaV3(t *testing.T) {
 		t.Fatalf("unexpected error getting openapi: %v", err)
 	}
 
+	contentTypes := []string{
+		runtime.ContentTypeJSON, openapi.ContentTypeOpenAPIV3PB,
+	}
+
+	for _, contentType := range contentTypes {
+		t.Run(contentType, func(t *testing.T) {
 			for k, v := range paths {
-		actual, err := v.SchemaPB()
+				actual, err := v.Schema(contentType)
 				if err != nil {
 					t.Fatal(err)
 				}
 
 				expected := testV3Specs[k]
-		expectedPB, err := golangproto.Marshal(expected)
+				if contentType == runtime.ContentTypeJSON {
+					var actualSpec spec3.OpenAPI
+
+					if err := json.Unmarshal(actual, &actualSpec); err != nil {
+						t.Fatal(err)
+					}
+
+					// Cannot use DeepEqual directly due to differences in how
+					// default key is being handled in gnostic vs kube-openapi
+					// Our test server parses the files in directly as gnostic
+					// which retains empty maps/lists, etc.
+					require.EqualValues(t, expected, &actualSpec)
+				} else {
+					// Convert to JSON then to gnostic then to PB for comparison
+					expectedJSON, err := json.Marshal(expected)
+					if err != nil {
+						t.Fatal(err)
+					}
+
+					expectedGnostic, err := openapi_v3.ParseDocument(expectedJSON)
+					if err != nil {
+						t.Fatal(err)
+					}
+
+					expectedPB, err := golangproto.Marshal(expectedGnostic)
 					if err != nil {
 						t.Fatal(err)
 					}
 					if !reflect.DeepEqual(expectedPB, actual) {
-			t.Fatalf("expected \n%v\n\ngot:\n%v", expected, actual)
+						t.Fatalf("expected equal values: %v", cmp.Diff(expectedPB, actual))
+					}
 				}
 
 				// Ensure that fetching schema once again does not return same instance
-		actualAgain, err := v.SchemaPB()
+				actualAgain, err := v.Schema(contentType)
 				if err != nil {
 					t.Fatal(err)
 				}
 
 				if reflect.ValueOf(actual).Pointer() == reflect.ValueOf(actualAgain).Pointer() {
 					t.Fatal("expected schema not to be cached")
-		} else if !reflect.DeepEqual(expectedPB, actualAgain) {
-			t.Fatal("expected schema values to be equal")
 				}
 			}
+
+		})
+	}
 }
 
 func TestGetOpenAPISchemaForbiddenFallback(t *testing.T) {

openapi/cached/groupversion.go

@@ -25,13 +25,13 @@ import (
 type groupversion struct {
 	delegate openapi.GroupVersion
 
-	jsonOnce  sync.Once
+	lock sync.Mutex
-	jsonBytes []byte
+	docs map[string]docInfo
-	jsonErr   error
+}
 
-	pbOnce  sync.Once
+type docInfo struct {
-	pbBytes []byte
+	data []byte
-	pbErr   error
+	err  error
 }
 
 func newGroupVersion(delegate openapi.GroupVersion) *groupversion {
@@ -40,18 +40,19 @@ func newGroupVersion(delegate openapi.GroupVersion) *groupversion {
 	}
 }
 
-func (g *groupversion) SchemaPB() ([]byte, error) {
+func (g *groupversion) Schema(contentType string) ([]byte, error) {
-	g.pbOnce.Do(func() {
+	g.lock.Lock()
-		g.pbBytes, g.pbErr = g.delegate.SchemaPB()
+	defer g.lock.Unlock()
-	})
 
-	return g.pbBytes, g.pbErr
+	cachedInfo, ok := g.docs[contentType]
+	if !ok {
+		if g.docs == nil {
+			g.docs = make(map[string]docInfo)
 		}
 
-func (g *groupversion) SchemaJSON() ([]byte, error) {
+		cachedInfo.data, cachedInfo.err = g.delegate.Schema(contentType)
-	g.jsonOnce.Do(func() {
+		g.docs[contentType] = cachedInfo
-		g.jsonBytes, g.jsonErr = g.delegate.SchemaJSON()
+	}
-	})
+
-
+	return cachedInfo.data, cachedInfo.err
-	return g.jsonBytes, g.jsonErr
 }

openapi/groupversion.go

@@ -22,13 +22,10 @@ import (
 	"k8s.io/kube-openapi/pkg/handler3"
 )
 
-const openAPIV3mimePb = "application/com.github.proto-openapi.spec.v3@v1.0+protobuf"
+const ContentTypeOpenAPIV3PB = "application/com.github.proto-openapi.spec.v3@v1.0+protobuf"
-const jsonMime = "application/json"
 
 type GroupVersion interface {
-	// Raw data types
+	Schema(contentType string) ([]byte, error)
-	SchemaJSON() ([]byte, error)
-	SchemaPB() ([]byte, error)
 }
 
 type groupversion struct {
@@ -40,23 +37,10 @@ func newGroupVersion(client *client, item handler3.OpenAPIV3DiscoveryGroupVersio
 	return &groupversion{client: client, item: item}
 }
 
-func (g *groupversion) SchemaPB() ([]byte, error) {
+func (g *groupversion) Schema(contentType string) ([]byte, error) {
 	data, err := g.client.restClient.Get().
 		RequestURI(g.item.ServerRelativeURL).
-		SetHeader("Accept", openAPIV3mimePb).
+		SetHeader("Accept", contentType).
-		Do(context.TODO()).
-		Raw()
-
-	if err != nil {
-		return nil, err
-	}
-	return data, nil
-}
-
-func (g *groupversion) SchemaJSON() ([]byte, error) {
-	data, err := g.client.restClient.Get().
-		RequestURI(g.item.ServerRelativeURL).
-		SetHeader("Accept", jsonMime).
 		Do(context.TODO()).
 		Raw()
 

util/testing/fake_openapi_handler.go

@@ -27,14 +27,13 @@ import (
 	"strings"
 	"sync"
 
-	openapi_v3 "github.com/google/gnostic/openapiv3"
 	"k8s.io/kube-openapi/pkg/handler3"
 	"k8s.io/kube-openapi/pkg/spec3"
 )
 
 type FakeOpenAPIServer struct {
 	HttpServer      *httptest.Server
-	ServedDocuments map[string]*openapi_v3.Document
+	ServedDocuments map[string]*spec3.OpenAPI
 	RequestCounters map[string]int
 }
 
@@ -62,7 +61,7 @@ func NewFakeOpenAPIV3Server(specsPath string) (*FakeOpenAPIServer, error) {
 	}
 
 	grouped := make(map[string][]byte)
-	var testV3Specs = make(map[string]*openapi_v3.Document)
+	var testV3Specs = make(map[string]*spec3.OpenAPI)
 
 	addSpec := func(path string) {
 		file, err := os.Open(path)
@@ -97,11 +96,8 @@ func NewFakeOpenAPIV3Server(specsPath string) (*FakeOpenAPIServer, error) {
 		if err != nil {
 			return nil, err
 		}
-		gnosticSpec, err := openapi_v3.ParseDocument(jsonSpec)
+
-		if err != nil {
+		testV3Specs[gv] = spec
-			return nil, err
-		}
-		testV3Specs[gv] = gnosticSpec
 		openAPIVersionedService.UpdateGroupVersion(gv, spec)
 	}