diff --git a/discovery/cached/disk/cached_discovery_test.go b/discovery/cached/disk/cached_discovery_test.go index c6326480..ac30604a 100644 --- a/discovery/cached/disk/cached_discovery_test.go +++ b/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.SchemaJSON() - 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() - 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].SchemaJSON() - assert.NoError(t, err) - assert.Equal(t, 1+i, fakeServer.RequestCounters["/openapi/v3"]) - assert.Equal(t, 1, fakeServer.RequestCounters[path]) + 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.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 { diff --git a/discovery/cached/memory/memcache_test.go b/discovery/cached/memory/memcache_test.go index 9d57f400..a3a29bf7 100644 --- a/discovery/cached/memory/memcache_test.go +++ b/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 { - original, err := v.SchemaPB() - if !assert.NoError(t, err) { - continue - } + contentTypes := []string{ + runtime.ContentTypeJSON, openapi.ContentTypeOpenAPIV3PB, + } - pathsAgain, err := openapiClient.Paths() - if !assert.NoError(t, err) { - continue - } + for _, contentType := range contentTypes { + t.Run(contentType, func(t *testing.T) { + for k, v := range paths { + original, err := v.Schema(contentType) + if !assert.NoError(t, err) { + continue + } - schemaAgain, err := pathsAgain[k].SchemaPB() - if !assert.NoError(t, err) { - continue - } + pathsAgain, err := openapiClient.Paths() + if !assert.NoError(t, err) { + continue + } - assert.True(t, reflect.ValueOf(paths).Pointer() == reflect.ValueOf(pathsAgain).Pointer()) - assert.True(t, reflect.ValueOf(original).Pointer() == reflect.ValueOf(schemaAgain).Pointer()) + schemaAgain, err := pathsAgain[k].Schema(contentType) + if !assert.NoError(t, err) { + continue + } - // Invalidate and try again. This time pointers should not be equal - client.Invalidate() + assert.True(t, reflect.ValueOf(paths).Pointer() == reflect.ValueOf(pathsAgain).Pointer()) + assert.True(t, reflect.ValueOf(original).Pointer() == reflect.ValueOf(schemaAgain).Pointer()) - pathsAgain, err = client.OpenAPIV3().Paths() - if !assert.NoError(t, err) { - continue - } + // Invalidate and try again. This time pointers should not be equal + client.Invalidate() - schemaAgain, err = pathsAgain[k].SchemaPB() - if !assert.NoError(t, err) { - continue - } + pathsAgain, err = client.OpenAPIV3().Paths() + if !assert.NoError(t, err) { + 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) + schemaAgain, err = pathsAgain[k].Schema(contentType) + if !assert.NoError(t, err) { + 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) + } + }) } } diff --git a/discovery/discovery_client_test.go b/discovery/discovery_client_test.go index 4d8a2754..4dc4a84c 100644 --- a/discovery/discovery_client_test.go +++ b/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,32 +581,64 @@ func TestGetOpenAPISchemaV3(t *testing.T) { t.Fatalf("unexpected error getting openapi: %v", err) } - for k, v := range paths { - actual, err := v.SchemaPB() - if err != nil { - t.Fatal(err) - } + contentTypes := []string{ + runtime.ContentTypeJSON, openapi.ContentTypeOpenAPIV3PB, + } - expected := testV3Specs[k] - expectedPB, err := golangproto.Marshal(expected) - if err != nil { - t.Fatal(err) - } - if !reflect.DeepEqual(expectedPB, actual) { - t.Fatalf("expected \n%v\n\ngot:\n%v", expected, actual) - } + for _, contentType := range contentTypes { + t.Run(contentType, func(t *testing.T) { + 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 - actualAgain, err := v.SchemaPB() - if err != nil { - t.Fatal(err) - } + expected := testV3Specs[k] + if contentType == runtime.ContentTypeJSON { + var actualSpec spec3.OpenAPI - 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") - } + 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 equal values: %v", cmp.Diff(expectedPB, actual)) + } + } + + // 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") + } + } + + }) } } diff --git a/openapi/cached/groupversion.go b/openapi/cached/groupversion.go index 079deda5..65a4189f 100644 --- a/openapi/cached/groupversion.go +++ b/openapi/cached/groupversion.go @@ -25,13 +25,13 @@ import ( type groupversion struct { delegate openapi.GroupVersion - jsonOnce sync.Once - jsonBytes []byte - jsonErr error + lock sync.Mutex + docs map[string]docInfo +} - pbOnce sync.Once - pbBytes []byte - pbErr error +type docInfo struct { + data []byte + err error } func newGroupVersion(delegate openapi.GroupVersion) *groupversion { @@ -40,18 +40,19 @@ func newGroupVersion(delegate openapi.GroupVersion) *groupversion { } } -func (g *groupversion) SchemaPB() ([]byte, error) { - g.pbOnce.Do(func() { - g.pbBytes, g.pbErr = g.delegate.SchemaPB() - }) +func (g *groupversion) Schema(contentType string) ([]byte, error) { + g.lock.Lock() + defer g.lock.Unlock() - return g.pbBytes, g.pbErr -} - -func (g *groupversion) SchemaJSON() ([]byte, error) { - g.jsonOnce.Do(func() { - g.jsonBytes, g.jsonErr = g.delegate.SchemaJSON() - }) - - return g.jsonBytes, g.jsonErr + 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 } diff --git a/openapi/groupversion.go b/openapi/groupversion.go index 1041c42e..6822b047 100644 --- a/openapi/groupversion.go +++ b/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 jsonMime = "application/json" +const ContentTypeOpenAPIV3PB = "application/com.github.proto-openapi.spec.v3@v1.0+protobuf" type GroupVersion interface { - // Raw data types - SchemaJSON() ([]byte, error) - SchemaPB() ([]byte, error) + Schema(contentType string) ([]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). - 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). + SetHeader("Accept", contentType). Do(context.TODO()). Raw() diff --git a/util/testing/fake_openapi_handler.go b/util/testing/fake_openapi_handler.go index e9a07acf..b6e4d668 100644 --- a/util/testing/fake_openapi_handler.go +++ b/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 { - return nil, err - } - testV3Specs[gv] = gnosticSpec + + testV3Specs[gv] = spec openAPIVersionedService.UpdateGroupVersion(gv, spec) }