Merge pull request #113062 from alexzielenski/client-go-json-output

client-go: raw data options for fetching openapiv3

Kubernetes-commit: bfb78f3e74188b19e627c1126cafaa1be36901d4

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,38 +170,52 @@ 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)
-	i := 0
-	for k, v := range paths {
-		i++
 
-		_, err = v.Schema()
+	contentTypes := []string{
-		assert.NoError(t, err)
+		runtime.ContentTypeJSON, openapi.ContentTypeOpenAPIV3PB,
-
-		path := "/openapi/v3/" + strings.TrimPrefix(k, "/")
-		assert.Equal(t, 1, fakeServer.RequestCounters[path])
-
-		// Ensure schema call is served from memory
-		_, err = v.Schema()
-		assert.NoError(t, err)
-		assert.Equal(t, 1, fakeServer.RequestCounters[path])
-
-		client.Invalidate()
-
-		// Refetch the schema from a new openapi client to try to force a new
-		// http request
-		newPaths, err := client.OpenAPIV3().Paths()
-		if !assert.NoError(t, err) {
-			continue
-		}
-
-		// Ensure schema call is still served from disk
-		_, err = newPaths[k].Schema()
-		assert.NoError(t, err)
-		assert.Equal(t, 1+i, fakeServer.RequestCounters["/openapi/v3"])
-		assert.Equal(t, 1, fakeServer.RequestCounters[path])
 	}
+
+	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.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.Schema(contentType)
+				assert.NoError(t, err)
+				assert.Equal(t, 1, fakeServer.RequestCounters[path])
+
+				client.Invalidate()
+
+				// Refetch the schema from a new openapi client to try to force a new
+				// http request
+				newPaths, err := client.OpenAPIV3().Paths()
+				if !assert.NoError(t, err) {
+					continue
+				}
+
+				// Ensure schema call is still served from disk
+				_, err = newPaths[k].Schema(contentType)
+				assert.NoError(t, err)
+				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,40 +417,48 @@ func TestOpenAPIMemCache(t *testing.T) {
 	paths, err := openapiClient.Paths()
 	require.NoError(t, err)
 
-	for k, v := range paths {
+	contentTypes := []string{
-		original, err := v.Schema()
+		runtime.ContentTypeJSON, openapi.ContentTypeOpenAPIV3PB,
-		if !assert.NoError(t, err) {
+	}
-			continue
-		}
 
-		pathsAgain, err := openapiClient.Paths()
+	for _, contentType := range contentTypes {
-		if !assert.NoError(t, err) {
+		t.Run(contentType, func(t *testing.T) {
-			continue
+			for k, v := range paths {
-		}
+				original, err := v.Schema(contentType)
+				if !assert.NoError(t, err) {
+					continue
+				}
 
-		schemaAgain, err := pathsAgain[k].Schema()
+				pathsAgain, err := openapiClient.Paths()
-		if !assert.NoError(t, err) {
+				if !assert.NoError(t, err) {
-			continue
+					continue
-		}
+				}
 
-		assert.True(t, reflect.ValueOf(paths).Pointer() == reflect.ValueOf(pathsAgain).Pointer())
+				schemaAgain, err := pathsAgain[k].Schema(contentType)
-		assert.True(t, reflect.ValueOf(original).Pointer() == reflect.ValueOf(schemaAgain).Pointer())
+				if !assert.NoError(t, err) {
+					continue
+				}
 
-		// Invalidate and try again. This time pointers should not be equal
+				assert.True(t, reflect.ValueOf(paths).Pointer() == reflect.ValueOf(pathsAgain).Pointer())
-		client.Invalidate()
+				assert.True(t, reflect.ValueOf(original).Pointer() == reflect.ValueOf(schemaAgain).Pointer())
 
-		pathsAgain, err = client.OpenAPIV3().Paths()
+				// Invalidate and try again. This time pointers should not be equal
-		if !assert.NoError(t, err) {
+				client.Invalidate()
-			continue
-		}
 
-		schemaAgain, err = pathsAgain[k].Schema()
+				pathsAgain, err = client.OpenAPIV3().Paths()
-		if !assert.NoError(t, err) {
+				if !assert.NoError(t, err) {
-			continue
+					continue
-		}
+				}
 
-		assert.True(t, reflect.ValueOf(paths).Pointer() != reflect.ValueOf(pathsAgain).Pointer())
+				schemaAgain, err = pathsAgain[k].Schema(contentType)
-		assert.True(t, reflect.ValueOf(original).Pointer() != reflect.ValueOf(schemaAgain).Pointer())
+				if !assert.NoError(t, err) {
-		assert.Equal(t, original, schemaAgain)
+					continue
+				}
+
+				assert.True(t, reflect.ValueOf(paths).Pointer() != reflect.ValueOf(pathsAgain).Pointer())
+				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,28 +581,68 @@ func TestGetOpenAPISchemaV3(t *testing.T) {
 		t.Fatalf("unexpected error getting openapi: %v", err)
 	}
 
-	for k, v := range paths {
+	contentTypes := []string{
-		actual, err := v.Schema()
+		runtime.ContentTypeJSON, openapi.ContentTypeOpenAPIV3PB,
-		if err != nil {
+	}
-			t.Fatal(err)
-		}
 
-		expected := testV3Specs[k]
+	for _, contentType := range contentTypes {
-		if !golangproto.Equal(expected, actual) {
+		t.Run(contentType, func(t *testing.T) {
-			t.Fatalf("expected \n%v\n\ngot:\n%v", expected, actual)
+			for k, v := range paths {
-		}
+				actual, err := v.Schema(contentType)
+				if err != nil {
+					t.Fatal(err)
+				}
 
-		// Ensure that fetching schema once again does not return same instance
+				expected := testV3Specs[k]
-		actualAgain, err := v.Schema()
+				switch contentType {
-		if err != nil {
-			t.Fatal(err)
-		}
 
-		if reflect.ValueOf(actual).Pointer() == reflect.ValueOf(actualAgain).Pointer() {
+				case runtime.ContentTypeJSON:
-			t.Fatal("expected schema not to be cached")
+					var actualSpec spec3.OpenAPI
-		} else if !golangproto.Equal(actual, actualAgain) {
+
-			t.Fatal("expected schema values to be equal")
+					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)
+				case openapi.ContentTypeOpenAPIV3PB:
+					// 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 equal values: %v", cmp.Diff(expectedPB, actual))
+					}
+				default:
+					panic(fmt.Errorf("unrecognized content type: %v", contentType))
+				}
+
+				// Ensure that fetching schema once again does not return same instance
+				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")
+				}
+			}
+
+		})
 	}
 }
 

openapi/cached/groupversion.go

@@ -19,15 +19,19 @@ package cached
 import (
 	"sync"
 
-	openapi_v3 "github.com/google/gnostic/openapiv3"
 	"k8s.io/client-go/openapi"
 )
 
 type groupversion struct {
 	delegate openapi.GroupVersion
-	once     sync.Once
+
-	doc      *openapi_v3.Document
+	lock sync.Mutex
-	err      error
+	docs map[string]docInfo
+}
+
+type docInfo struct {
+	data []byte
+	err  error
 }
 
 func newGroupVersion(delegate openapi.GroupVersion) *groupversion {
@@ -36,10 +40,19 @@ func newGroupVersion(delegate openapi.GroupVersion) *groupversion {
 	}
 }
 
-func (g *groupversion) Schema() (*openapi_v3.Document, error) {
+func (g *groupversion) Schema(contentType string) ([]byte, error) {
-	g.once.Do(func() {
+	g.lock.Lock()
-		g.doc, g.err = g.delegate.Schema()
+	defer g.lock.Unlock()
-	})
 
-	return g.doc, g.err
+	cachedInfo, ok := g.docs[contentType]
+	if !ok {
+		if g.docs == nil {
+			g.docs = make(map[string]docInfo)
+		}
+
+		cachedInfo.data, cachedInfo.err = g.delegate.Schema(contentType)
+		g.docs[contentType] = cachedInfo
+	}
+
+	return cachedInfo.data, cachedInfo.err
 }

openapi/groupversion.go

@@ -19,15 +19,13 @@ package openapi
 import (
 	"context"
 
-	openapi_v3 "github.com/google/gnostic/openapiv3"
-	"google.golang.org/protobuf/proto"
 	"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"
 
 type GroupVersion interface {
-	Schema() (*openapi_v3.Document, error)
+	Schema(contentType string) ([]byte, error)
 }
 
 type groupversion struct {
@@ -39,21 +37,10 @@ func newGroupVersion(client *client, item handler3.OpenAPIV3DiscoveryGroupVersio
 	return &groupversion{client: client, item: item}
 }
 
-func (g *groupversion) Schema() (*openapi_v3.Document, error) {
+func (g *groupversion) Schema(contentType string) ([]byte, error) {
-	data, err := g.client.restClient.Get().
+	return g.client.restClient.Get().
 		RequestURI(g.item.ServerRelativeURL).
-		SetHeader("Accept", openAPIV3mimePb).
+		SetHeader("Accept", contentType).
 		Do(context.TODO()).
 		Raw()
-
-	if err != nil {
-		return nil, err
-	}
-
-	document := &openapi_v3.Document{}
-	if err := proto.Unmarshal(data, document); err != nil {
-		return nil, err
-	}
-
-	return document, nil
 }

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