OpenAPI downloads protobuf rather than Json #46803

apelisse · 2017-06-01T21:03:43Z

What this PR does / why we need it:
The current implementation of the OpenAPI getter fetches the swagger in a Json format from the apiserver. The Json file is big (~1.7mb), which means that it takes a long time to download, and then a long time to parse. Because that is going to be needed on each kubectl run later, we want this to be as fast as possible.

The apiserver has been modified to be able to return a protobuf version of the swagger, which this patch intends to use.

Note that there is currently no piece of code that exists that allows us to go from the protobuf version of the file, back into Json and/or spec.Swagger. Because the protobuf is not very different (but significantly different enough that it can't be translated), I've updated the code to use openapi_v2.Document (the protobuf type) everywhere rather than spec.Swagger. The behavior should be identical though.

There are more changes that are coming in follow-up pull-requests: using the gzip version (also provided by the new apiserver) to even further reduce the size of the downloaded content, and use the HTTP Etag cache mechanism to completely get rid of recurrent fetch requests. I'm currently working on these two features.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): partly #38637

Special notes for your reviewer:

Release note:

NONE

mengqiy · 2017-06-02T03:38:09Z

pkg/kubectl/cmd/util/openapi/openapi.go

@@ -98,7 +100,7 @@ type Kind struct {
 	PrimitiveType string

 	// Extensions are openapi extensions for the object definition.


Update the comment.

Maybe not. They are the same the type under the cover.

mengqiy · 2017-06-02T03:43:58Z

pkg/kubectl/cmd/util/openapi/openapi.go

+}
+
+func vendorExtensionToMap(e []*openapi_v2.NamedAny) map[string]interface{} {
+	var values map[string]interface{}


You can values := map[string]interface{}{}
and then remove

if values == nil { values = make(map[string]interface{}) }

Yeah, that's exactly what I had done initially. Problem is that it broke many tests. Many tests build an expected Kind/Type with the default Extension value nil. Problem is, this function will always initialize Extension with an empty Extension, which doesn't compare as equal to nil in reflect.DeepEqual.

IOW, the goal of this if is to return a nil map rather than an empty map, so that the tests don't have to worry about initializing an empty map ...

mengqiy · 2017-06-02T04:00:16Z

pkg/kubectl/cmd/util/openapi/openapi.go

@@ -202,13 +228,13 @@ func (o *Resources) parseDefinition(name string, s spec.Schema) Kind {
 	if o.isPrimitive(s) {
 		value.PrimitiveType = o.getTypeNameForField(s)
 	}
-	for fieldname, property := range s.Properties {
-		value.Fields[fieldname] = o.parseField(property)
+	for _, ns := range s.Properties.GetAdditionalProperties() {


Is it safer to use s.GetProperties().GetAdditionalProperties()?

Safer, no. More consistent with how I've done it throughout the file, yes.

I discussed that with Phillip and we pretty much agreed that null-checking everything was probably overkill

mengqiy · 2017-06-02T04:06:43Z

pkg/kubectl/cmd/util/openapi/openapi.go

 	// Get the reference for complex types
 	if o.isDefinitionReference(s) {
 		return o.nameForDefinitionField(s)
 	}
 	// Recurse if type is array
 	if o.isArray(s) {
-		return fmt.Sprintf("%s array", o.getTypeNameForField(*s.Items.Schema))
+		return fmt.Sprintf("%s array", o.getTypeNameForField(s.GetItems().GetSchema()[0]))


Is it guaranteed to have at least one item?

Good point, but I think it falls under the same logic as above: "we assume we have tested this code against the swagger file before".

mengqiy · 2017-06-02T04:13:48Z

pkg/kubectl/cmd/util/openapi/openapi.go

 	}
-	return *s.Items.Schema, nil
+	return s.GetItems().GetSchema()[0], nil


Same question here. Should we check the length of the schema slice?

mengqiy · 2017-06-02T04:17:42Z

pkg/kubectl/cmd/util/openapi/openapi.go

 	empty := schema.GroupVersionKind{}

 	// Get the extensions
-	extList, f := s.Extensions[groupVersionKindExtensionKey]
+	extList, f := vendorExtensionToMap(s.GetVendorExtension())[groupVersionKindExtensionKey]


Maybe save the result of vendorExtensionToMap(s.GetVendorExtension()) to avoid process this again below.

mengqiy · 2017-06-02T04:19:50Z

pkg/kubectl/cmd/util/openapi/openapi.go

@@ -366,9 +391,9 @@ func (o *Resources) getGroupVersionKind(s spec.Schema) (schema.GroupVersionKind,
 	gvk := extListCasted[0]

 	// Expect a empty of a map with 3 entries
-	gvkMap, ok := gvk.(map[string]interface{})
+	gvkMap, ok := gvk.(map[interface{}]interface{})


Interesting. It is not a map[string]interface{} any more?

Yeah, seems like the new type uses a different format.

mengqiy · 2017-06-02T04:23:33Z

pkg/kubectl/cmd/util/openapi/openapi.go

@@ -20,8 +20,10 @@ import (
 	"fmt"
 	"strings"

-	"github.com/go-openapi/spec"
+	yaml "gopkg.in/yaml.v2"


Not sure if we should use this or github.com/ghodss/yaml?

Yeah, probably doesn't matter, but it looks like yours is used slightly more

@fabianofranz Do you know what yaml pkg should we use?

Actually we don't have a choice. I use a feature from gopkg.in/yaml.v2

mengqiy · 2017-06-02T04:33:52Z

staging/src/k8s.io/client-go/discovery/discovery_client.go

-func (d *DiscoveryClient) OpenAPISchema() (*spec.Swagger, error) {
-	data, err := d.restClient.Get().AbsPath("/swagger.json").Do().Raw()
+func (d *DiscoveryClient) OpenAPISchema() (*openapi_v2.Document, error) {
+	data, err := d.restClient.Get().AbsPath("/swagger-2.0.0.pb-v1").Do().Raw()


Are you sure about the name.
I found /swagger.pb in https://docs.google.com/document/d/1EcnvLRqP9-5mqQ-kghfnB3GtB8eIrkVQjn1B16NRrr0/edit#heading=h.xgjl2srtytjt

After code-review, it's been changed to that: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go#L110

droot

looks good to me, a few minor comments.

droot · 2017-06-02T18:16:27Z

pkg/kubectl/cmd/util/cached_discovery.go

 	"github.com/golang/glog"
+	openapi_v2 "github.com/googleapis/gnostic/OpenAPIv2"


wondering if we have any convention for package import names ? using "_" is a standard thing (I see metav1 below) ?

This is mostly because my goimports was broken. I've fixed it and I'm using the default name now (which happens to also be openapi_v2 ;-)

droot · 2017-06-02T18:27:37Z

pkg/kubectl/cmd/util/openapi/openapi.go

-		definition := o.parseDefinition(name, d)
-		o.NameToDefinition[name] = definition
+	for _, ns := range doc.GetDefinitions().GetAdditionalProperties() {
+		definition := o.parseDefinition(ns.Name, ns.Value)


does ns have Getters for name and value. If yes, then we should use those to be consistent.

Yes, good catch!

apelisse · 2017-06-02T21:18:48Z

Addressed! Please take a look!

mengqiy · 2017-06-02T21:47:10Z

@k8s-bot pull-kubernetes-unit test this
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

mengqiy · 2017-06-02T21:48:27Z

/lgtm

mengqiy · 2017-06-02T22:59:03Z

@k8s-bot pull-kubernetes-unit test this

mbohlool · 2017-06-03T09:09:26Z

apiserver. The Json file is big (~330mb), which means that it takes a long time to download, and then

330mb? it is 1.7MB. right?

But the point is valid that it takes much longer to unmarshal. Some numbers here: https://docs.google.com/document/d/1EcnvLRqP9-5mqQ-kghfnB3GtB8eIrkVQjn1B16NRrr0

apelisse · 2017-06-03T16:57:11Z

330mb? it is 1.7MB. right?

I have no idea how I made up that number :D. I'll update the description.

apelisse · 2017-06-05T15:53:01Z

@pwittrock Please take a look!

pwittrock · 2017-06-05T18:16:53Z

/approve

apelisse · 2017-06-12T03:47:14Z

I've added a new commit to download the gzipped protobuf.

pwittrock · 2017-06-12T18:59:52Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

This is much faster.

pwittrock · 2017-06-26T16:40:07Z

/lgtm

pwittrock · 2017-06-26T16:40:12Z

/approve

deads2k · 2017-06-26T17:23:14Z

staging/src/k8s.io/client-go/discovery/discovery_client.go

-	data, err := d.restClient.Get().AbsPath("/swagger.json").Do().Raw()
+// OpenAPISchema fetches the open api schema using a rest client and parses the proto.
+func (d *DiscoveryClient) OpenAPISchema() (*openapi_v2.Document, error) {
+	stream, err := d.restClient.Get().AbsPath("/swagger-2.0.0.pb-v1.gz").Stream()


This looks off to me. Why wouldn't you request a gzipped encoding of the normal protobuf struct?

Because the server doesn't support it

Because the server doesn't support it

I'm pretty sure it merged recently here: #46966

This code is targeting this end-point, is it related?

deads2k · 2017-06-26T20:29:43Z

This code is targeting this end-point, is it related?

The handler was supposed to be written as a "normal" http handler wrapper. Can you try wrapping it? I'd rather not enshrine a temporary condition in the API.

apelisse · 2017-06-26T20:32:28Z

Yeah I'll look at it

apelisse · 2017-06-27T21:10:00Z

OK I've just removed that part. Go will take care of that part if the server supports the gzip encoding.

apelisse · 2017-06-27T21:10:14Z

Can you PTAL @deads2k

mbohlool · 2017-06-28T02:14:30Z

If it is not too much trouble, can you separate generated/godep commits from others?

deads2k · 2017-06-28T12:57:14Z

staging/src/k8s.io/client-go/discovery/discovery_client.go

-	data, err := d.restClient.Get().AbsPath("/swagger.json").Do().Raw()
+// OpenAPISchema fetches the open api schema using a rest client and parses the proto.
+func (d *DiscoveryClient) OpenAPISchema() (*openapi_v2.Document, error) {
+	data, err := d.restClient.Get().AbsPath("/swagger-2.0.0.pb-v1").Do().Raw()


Ok, this now looks good apiserver-wise, but client-side it seems like this will break when run against old servers. Don't you need a fallback to the old swagger.json?

It shouldn't. 1.7 server already supports that end-point, and this code is meant for 1.8 client.

It shouldn't. 1.7 server already supports that end-point, and this code is meant for 1.8 client.

I rather not have this be the thing that breaks generic commands (like create) from working. If it returns an error, will kubectl still work? If not, at least guarding it sufficiently to keep that working seems reasonable even if client side validation doesn't work.

If --validate=false, we shouldn't block on getting the openapi. If --validate=true, we should fail if we can't get the openapi to validate.

IOW, This is fine for now as this code is not even used for validation. The only way to actually exercise this code today is to use --experimental-use-openapi-print-columns.

In other words, I don't think kubectl of any version should ever use openapi with a pre-1.7 server version.

Given code to go from spec.Schema -> openapi_v2.Document, it seems like you could hit the old endpoint, read into spec.Schema (slow as it is), then convert that to the openapi_v2.Document to return.

I'm fine telling people they shouldn't use the feature against older servers. I'm less fine straight up breaking them. With --validate=true being the default, that's what would happen with a 1.8 kubectl and 1.6 server (which a client may not be able to update), right? Openapi for validation is a goal.

We could keep the code as it is, have it fail on validating with the openapi, and then automatically fallback on swagger. That would allow us even longer backward compatibility.

We could keep the code as it is, have it fail on validating with the openapi, and then automatically fallback on swagger. That would allow us even longer backward compatibility.

Sold. Is it practical do in this pull?

Nothing really uses this code so far, and I'm working on adding new features but it's too substantial to be added to this pr. I'll definitely add you as a reviewer/approver on pull-requests to come.

Nothing really uses this code so far, and I'm working on adding new features but it's too substantial to be added to this pr. I'll definitely add you as a reviewer/approver on pull-requests to come.

Ok. Please open an issue for yourself about maintaining compatibility.

deads2k · 2017-06-30T16:19:50Z

I'm ok with an issue that you'll fix in 1.8 to maintain compatibility.

/approve

k8s-github-robot · 2017-06-30T16:20:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, deads2k, mengqiy, pwittrock

Associated issue: 38637

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubectl/OWNERS~~ [deads2k,pwittrock]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

apelisse · 2017-06-30T17:57:14Z

/retest

k8s-github-robot · 2017-06-30T23:28:47Z

Automatic merge from submit-queue (batch tested with PRs 43558, 48261, 42376, 46803, 47058)

apelisse assigned droot, mengqiy and pwittrock Jun 1, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 1, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 1, 2017

mengqiy reviewed Jun 2, 2017

View reviewed changes

apelisse force-pushed the new-download-openapi branch 2 times, most recently from 85f9230 to d7ccf24 Compare June 2, 2017 18:03

droot reviewed Jun 2, 2017

View reviewed changes

apelisse force-pushed the new-download-openapi branch from d7ccf24 to 8c98bb4 Compare June 2, 2017 20:36

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2017

apelisse force-pushed the new-download-openapi branch from 8c98bb4 to 8c2ce5f Compare June 12, 2017 03:45

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 12, 2017

apelisse force-pushed the new-download-openapi branch from 8c2ce5f to b8f4faa Compare June 12, 2017 15:45

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2017

openapi: Fetch protobuf rather than Json

224dba9

This is much faster.

apelisse force-pushed the new-download-openapi branch from b8f4faa to 16a76d2 Compare June 23, 2017 20:52

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2017

apelisse assigned deads2k and unassigned droot, mengqiy and pwittrock Jun 26, 2017

deads2k reviewed Jun 26, 2017

View reviewed changes

apelisse force-pushed the new-download-openapi branch from 16a76d2 to 224dba9 Compare June 27, 2017 21:08

deads2k reviewed Jun 28, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 30, 2017

k8s-github-robot merged commit 9c74026 into kubernetes:master Jun 30, 2017

liggitt mentioned this pull request Oct 3, 2017

in-cluster kubectl can't get nonResource URL in local-cluster-up #53349

Closed

		@@ -98,7 +100,7 @@ type Kind struct {
		PrimitiveType string

		// Extensions are openapi extensions for the object definition.

		"github.com/golang/glog"
		openapi_v2 "github.com/googleapis/gnostic/OpenAPIv2"

OpenAPI downloads protobuf rather than Json #46803

OpenAPI downloads protobuf rather than Json #46803

Conversation

apelisse commented Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Jun 2, 2017

mengqiy commented Jun 2, 2017

mengqiy commented Jun 2, 2017

mengqiy commented Jun 2, 2017

mbohlool commented Jun 3, 2017

apelisse commented Jun 3, 2017

apelisse commented Jun 5, 2017

pwittrock commented Jun 5, 2017

apelisse commented Jun 12, 2017

pwittrock commented Jun 12, 2017

pwittrock commented Jun 26, 2017

pwittrock commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 26, 2017

apelisse commented Jun 26, 2017

apelisse commented Jun 27, 2017

apelisse commented Jun 27, 2017

mbohlool commented Jun 28, 2017

Choose a reason for hiding this comment

apelisse Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 30, 2017

k8s-github-robot commented Jun 30, 2017

apelisse commented Jun 30, 2017

k8s-github-robot commented Jun 30, 2017

apelisse commented Jun 1, 2017 •

edited

Loading

apelisse Jun 2, 2017 •

edited

Loading

apelisse Jun 28, 2017 •

edited

Loading