Add protobuf binary version of OpenAPI spec #45836

mbohlool · 2017-05-15T20:05:54Z

Fixes #45833
Partially fixes #42841

OpenAPI spec is now available in protobuf binary and gzip format (with ETag support)

pwittrock · 2017-05-16T01:04:00Z

@mehdy looks like some bazel issue

pwittrock · 2017-05-16T15:49:55Z

@apelisse @mengqiy

apelisse · 2017-05-16T16:25:21Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

+	files := []fileInfo{
+		{".json", o.swaggerBytes, MIME_JSON},
+		{".pb", o.swaggerPb, MIME_PB},
+		{".pb.gz", o.swaggerPbGz, MIME_PB_GZ},


Maybe we can remove this handler and use https://github.com/NYTimes/gziphandler?

We would get the gzipped json for free too

While I love reusing code, adding dependencies to gogep is painful and this is only 3-4 line more code here. I would like to keep it if it is not that bad.

apelisse · 2017-05-16T16:28:08Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

+		{".json", o.swaggerBytes, MIME_JSON},
+		{".pb", o.swaggerPb, MIME_PB},
+		{".pb.gz", o.swaggerPbGz, MIME_PB_GZ},
+		{".json.sha512", o.swaggerHash[:], MIME_BINARY},


I assume we expose the sha for caching. Could we use a http ETag instead? As this would make the implementation more transparent on both side? I can help with that if needed.

http://www.sanarias.com/blog/115LearningHTTPcachinginGo

Etag look like a good idea.

ETag implemented. PTAL.

apelisse · 2017-05-16T16:33:23Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

+		return fmt.Errorf("Serving path must ends with \".json\".")
+	}
+
+	servePathBase := servePath[:len(servePath)-5]


What is 5? Is it len(".json")?

Yes, I've added a constant for ".json" and change this to len(...)

pwittrock · 2017-05-16T16:37:50Z

FYI:

hack/make-rules/../../hack/verify-federation-openapi-spec.sh

apelisse · 2017-05-16T16:36:37Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

 }

 // RegisterOpenAPIService registers a handler to provides standard OpenAPI specification.
+// Note: servePath is the path to the filename with no extension. Supported extensions


Am I mis-reading or is that the opposite of what it actually is. servePath is the path to the filename WITH the .json extension.

You are right, I forgot to remove this comment after some iteration to the code. Done.

apelisse · 2017-05-16T16:36:56Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

 func RegisterOpenAPIService(servePath string, webServices []*restful.WebService, config *openapi.Config, mux *genericmux.PathRecorderMux) (err error) {
+
+	if !strings.HasSuffix(servePath, ".json") {
+		return fmt.Errorf("Serving path must ends with \".json\".")


Because OpenAPI spec is in json format. We can relax this limitation if we had a reason for it but right now it helps me remove the .json and add extensions for other formats.

I think go vet will ask for this to become errors.New instead of fmt.Errorf.

I tried go vet it didn't complain about fmt.Errorf and I guess we are doing the same in other places, I don't have strong opinion on either.

pwittrock · 2017-05-16T19:45:37Z

@alexandercampbell

alexandercampbell · 2017-05-16T19:57:36Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

 func RegisterOpenAPIService(servePath string, webServices []*restful.WebService, config *openapi.Config, mux *genericmux.PathRecorderMux) (err error) {
+
+	if !strings.HasSuffix(servePath, ".json") {
+		return fmt.Errorf("Serving path must ends with \".json\".")


I think go vet will ask for this to become errors.New instead of fmt.Errorf.

alexandercampbell · 2017-05-16T19:59:46Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

 func RegisterOpenAPIService(servePath string, webServices []*restful.WebService, config *openapi.Config, mux *genericmux.PathRecorderMux) (err error) {
+
+	if !strings.HasSuffix(servePath, ".json") {


I don't like this because it has the potential to break piping. What if my JSON information is fed in from /dev/stdin?

I am not sure if I understand this. The caller should specify an http serving path for the file and I am enforcing the path ends with json. It cannot be any pipe like stdin.

alexandercampbell · 2017-05-16T20:00:34Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

+	MIME_JSON   = "application/json"
+	MIME_PB     = "application/com.github.googleapis.gnostic.OpenAPIv2+protobuf"
+	MIME_PB_GZ  = "application/x-gzip"
+	MIME_BINARY = "application/octet-stream"


Good constants to have.

Unfortunately (or fortunately) we don't need this anymore after etag implementation.

alexandercampbell · 2017-05-16T20:01:35Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

+		mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+			resp := restful.NewResponse(w)
+			if r.URL.Path != path {
+				resp.WriteErrorString(http.StatusNotFound, "Path not found!")


Doesn't this need to return early?

I think it will and I don't think this check serve anything here, just looked at other HandleFuncs in our code and they all checked this, I will look into this more and remove this if statement if it doesn't help in any way.

Oh, I didn't understand your comment earlier, yes, this needs an early return for sure. fixed.

alexandercampbell · 2017-05-16T20:03:51Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

+	return nil
+}
+
+func (o *openAPI) makeAdditionalFormats() error {


Docstring on this would be nice. I can't figure out the purpose of this function.

Good point. I like methods to be self explanatory so instead of adding comments to this, I restructure it to two toProtoBinary and toGzip method that is clear what they do.

alexandercampbell · 2017-05-16T20:04:09Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi_test.go

@@ -187,6 +187,7 @@ func getConfig(fullMethods bool) (*openapi.Config, *restful.Container) {
 			InfoProps: spec.InfoProps{
 				Title:       "TestAPI",
 				Description: "Test API",
+				Version: "unversioned",


This doesn't look gofmt'd. (maybe a Github rendering issue?)

mbohlool · 2017-05-16T20:06:30Z

Comments addressed. Thanks to @apelisse @sttts for ETag suggestion. PTAL.

alexandercampbell · 2017-05-16T20:10:31Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

 	protocolList []string
 	servePath    string
 	definitions  map[string]openapi.OpenAPIDefinition
 }

+func computeEtag(data []byte) string {
+	sha := sha512.Sum512(data)
+	return fmt.Sprintf("\"%s\"",


This looks like a good situation to either use %q or a backtick string literal.

Good point. fixed.

apelisse

Thank you @mbohlool ! That's awesome

apelisse · 2017-05-16T20:13:10Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

+		mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Path != path {
+				w.WriteHeader(http.StatusNotFound)
+				w.Write([]byte("Path not found!"))


it is indeed missing the early return

apelisse · 2017-05-16T20:18:44Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

 	protocolList []string
 	servePath    string
 	definitions  map[string]openapi.OpenAPIDefinition
 }

+func computeEtag(data []byte) string {
+	sha := sha512.Sum512(data)
+	return fmt.Sprintf("\"%s\"",


You could have used fmt.Sprintf("\"%x\"", sha512.Sum512(data)) here.

I like that. Done.

apelisse · 2017-05-16T20:21:28Z

staging/src/k8s.io/apiserver/pkg/server/openapi/openapi.go

+	}
+
+	files := []fileInfo{
+		{".json", o.swaggerBytes, computeEtag(o.swaggerBytes)},


Could you compute etag in the loop? That would remove the extra field in the struct.

apelisse · 2017-05-16T20:39:55Z

Client side cache can be implemented using https://github.com/gregjones/httpcache/ and https://github.com/gregjones/httpcache/diskcache

mbohlool · 2017-05-16T20:49:35Z

@apelisse @alexandercampbell Addressed your comments. PTAL.

alexandercampbell · 2017-05-16T20:57:46Z

I don't see any tests for this code. Are you planning on adding some?

mbohlool · 2017-05-16T22:31:32Z

@alexandercampbell To test this properly, I need some functionality from gnostic (converting proto back to json). It should be there, we can either wait for that and keep this PR open or merge this and when that is available, we can add the test. what do you think?

alexandercampbell · 2017-05-17T20:15:50Z

@mbohlool any way to get some unit tests on some of the smaller functions in this PR?

pwittrock · 2017-05-18T20:45:44Z

If unit testing is the only remaining issue, since @mbohlool is going to be out for a week, and we have 3 weeks of stabilization after feature freeze, how about we merge this and introduce the unit tests next week after he returns?

alexandercampbell · 2017-05-18T21:01:31Z

Sure. LGTM.

pwittrock · 2017-05-18T21:20:56Z

/lgtm

lavalamp · 2017-05-18T22:45:09Z

Thanks for the quick changes!

/lgtm
/approve

lavalamp · 2017-05-18T22:49:46Z

/lgtm

k8s-github-robot · 2017-05-18T22:50:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, mbohlool, pwittrock

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

~~OWNERS~~ [lavalamp]

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

mbohlool · 2017-05-19T08:10:45Z

reapplied LGTM. test was failing because an extra file got into one of the commits (a file that I planned for next PR that is adding tests for this). I only removed that file with no other changes.

mbohlool · 2017-05-19T22:42:27Z

@k8s-bot verify test this (it is not failing locally for me)

mbohlool · 2017-05-19T22:44:57Z

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

mbohlool · 2017-05-20T00:38:40Z

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

mbohlool · 2017-05-20T00:38:49Z

@k8s-bot kops aws e2e test this

k8s-ci-robot · 2017-05-20T17:37:31Z

@mbohlool: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`601570a`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-05-20T18:01:04Z

Automatic merge from submit-queue

pwittrock · 2017-05-20T18:39:17Z

Woot

…

On May 20, 2017 11:01 AM, "Kubernetes Submit Queue" < ***@***.***> wrote: Merged #45836 <#45836>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45836 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHhIAiuf7UEqkZhoUzCjfFYe3GYOzQI7ks5r7yp2gaJpZM4Nbn6G> .

k8s-github-robot assigned dchen1107 May 15, 2017

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

k8s-github-robot assigned brendandburns May 15, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 15, 2017

mbohlool requested a review from pwittrock May 15, 2017 20:06

mbohlool force-pushed the openapi_pb branch from 8657193 to 13db7fb Compare May 16, 2017 01:04

apelisse reviewed May 16, 2017

View reviewed changes

alexandercampbell reviewed May 16, 2017

View reviewed changes

mbohlool force-pushed the openapi_pb branch from 113cacb to 6a8d392 Compare May 16, 2017 20:22

apelisse reviewed May 16, 2017

View reviewed changes

mbohlool force-pushed the openapi_pb branch from 9a90785 to 8c8285a Compare May 16, 2017 20:48

mbohlool force-pushed the openapi_pb branch from 8c8285a to 8699f77 Compare May 16, 2017 21:27

k8s-ci-robot assigned pwittrock May 18, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 18, 2017

mbohlool force-pushed the openapi_pb branch from f087238 to a55d81c Compare May 18, 2017 22:48

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

mbohlool force-pushed the openapi_pb branch from a55d81c to 8b874eb Compare May 19, 2017 08:09

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2017

mbohlool added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2017

mbohlool force-pushed the openapi_pb branch from 8b874eb to 3e833ff Compare May 19, 2017 17:51

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2017

mbohlool force-pushed the openapi_pb branch from 3e833ff to ac1b372 Compare May 19, 2017 21:56

mbohlool added 5 commits May 19, 2017 15:39

Add gnostic to Godep

6702504

Add protobuf binary version of openapi spec

161b480

bugfix: form parameters should have type in OpenAPI spec

4b0fbfe

Update bazel

4d4abf3

Fix hack/update-federation-openapi-spec.sh flakyness

601570a

mbohlool force-pushed the openapi_pb branch from ac1b372 to 601570a Compare May 19, 2017 23:40

mbohlool mentioned this pull request May 20, 2017

Bugfix: Force Godep check #46153

Closed

mbohlool added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2017

k8s-github-robot merged commit a8bff0e into kubernetes:master May 20, 2017

		func RegisterOpenAPIService(servePath string, webServices []restful.WebService, config openapi.Config, mux *genericmux.PathRecorderMux) (err error) {

		if !strings.HasSuffix(servePath, ".json") {

Add protobuf binary version of OpenAPI spec #45836

Add protobuf binary version of OpenAPI spec #45836

Conversation

mbohlool commented May 15, 2017 • edited Loading

pwittrock commented May 16, 2017

pwittrock commented May 16, 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

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

pwittrock commented May 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented May 16, 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

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

mbohlool commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse left a 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

apelisse commented May 16, 2017 • edited Loading

mbohlool commented May 16, 2017

alexandercampbell commented May 16, 2017

mbohlool commented May 16, 2017

alexandercampbell commented May 17, 2017

pwittrock commented May 18, 2017

alexandercampbell commented May 18, 2017

pwittrock commented May 18, 2017

lavalamp commented May 18, 2017

lavalamp commented May 18, 2017

k8s-github-robot commented May 18, 2017

mbohlool commented May 19, 2017

mbohlool commented May 19, 2017

mbohlool commented May 19, 2017

mbohlool commented May 20, 2017

mbohlool commented May 20, 2017

k8s-ci-robot commented May 20, 2017 • edited Loading

k8s-github-robot commented May 20, 2017

pwittrock commented May 20, 2017 via email

mbohlool commented May 15, 2017 •

edited

Loading

apelisse commented May 16, 2017 •

edited

Loading

k8s-ci-robot commented May 20, 2017 •

edited

Loading