OpenAPI aggregation for kube-aggregator #46734

mbohlool · 2017-05-31T21:15:15Z

This PR implements OpenAPI aggregation layer for kube-aggregator. On each API registration, it tries to download swagger.spec of the user api server. On failure it will try again next time (either on another add or get /swagger.* on aggregator server) up to five times. To merge specs, it first remove all unrelated paths from the downloaded spec (anything other than group/version of the API service) and then remove all unused definitions. Adding paths are straightforward as they won't have any conflicts, but definitions will most probably have conflicts. To resolve that, we would reused any definition that is not changed (documentation changes are fine) and rename the definition otherwise.

To use this PR, kube aggregator should have nonResourceURLs (for get verb) to user apiserver.

Support OpenAPI spec aggregation for kube-aggregator

fixes: #43717

mbohlool · 2017-06-01T05:34:37Z

@k8s-bot test this

lavalamp · 2017-06-01T17:21:32Z

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

+func RegisterOpenAPIService(openapiSpec *spec.Swagger, servePath string, mux *genericmux.PathRecorderMux) (*OpenAPIService, error) {
+
+	if !strings.HasSuffix(servePath, JSON_EXT) {
+		return nil, fmt.Errorf("Serving path must ends with \"%s\".", JSON_EXT)


Does this block actually calling the .gz or .pb-v1 paths?

The caller to RegisterOpenAPIService will call it with json extension, but the function will reuse the name and serve other formats too. The caller do not need to worry about other formats, only json.

lavalamp · 2017-06-01T17:48:34Z

/approve

I didn't do a super detailed code review yet - anything I should look at in particular?

sttts · 2017-06-06T12:02:24Z

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

@@ -28,6 +28,7 @@ import (
 	"github.com/emicklei/go-restful-swagger12"
 	"github.com/golang/glog"

+	"github.com/go-openapi/spec"


empty line below, none above.

deads2k · 2017-06-06T12:04:21Z

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

@@ -0,0 +1,482 @@
+/*


This file doesn't belong in the genericapiserver. No "normal" apiserver has a reason to aggregate openapi specs, only the aggregator does that. We should contain the logic where it is needed and keep the generic stuff separate.

I plan to move OpenAPI stuff out as a library, so where this code exists doesn't matter to me, I will move it to api aggregator for now.

deads2k · 2017-06-06T12:07:56Z

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

+				w.Write([]byte("Path not found!"))
+				return
+			}
+			o.update(r)


This isn't what I expect in the genericapiserver implementation. We never have to update that one since it won't ever change.

The aggregator should get its own openapi endpoint handler.

I think the ability to update spec is required specially if we want to support CRDs. A server that has static spec can ignore OpenAPIService and just install the handler. This will make more sense when we just extract openapi code in its own repo. I will continue this discussion in the other PR (that I will send).

sttts · 2017-06-06T12:08:23Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

+
+	// List of the specs that needs to be loaded. When a spec is successfully loaded
+	// it will be removed from this list and added to apiServiceSpecs.
+	// Map values are retry counts. After a preset retries, it will stop


Ugly. Put this into the variable name at the very least: toLoadAPISpecRetries. Though, if a retry mechnism is really necessary, this ought to be in some separate controller instead of "globally" in the aggregator.

sttts · 2017-06-06T12:09:50Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go


 	// TODO unregister group level discovery when there are no more versions for the group
 	// We don't need this right away because the handler properly delegates when no versions are present
 }
+
+func (_ *APIAggregator) loadOpenAPISpec(p *proxyHandler, r *http.Request) (*spec.Swagger, error) {


Will fix. (do we force godoc for private methods? not that I think having godocs is bad)

no, we don't. Only in not obvious cases, a godoc helps to read the code.

deads2k · 2017-06-06T12:10:41Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -118,6 +133,24 @@ type APIAggregator struct {
 	// handledGroups are the groups that already have routes
 	handledGroups sets.String

+	// Swagger spec for each api service
+	apiServiceSpecs map[string]*spec.Swagger


These should all be abstracted into an interface that accepts Add/Remove apiServices and the logic should be contained close to the handler you're coding.

sttts · 2017-06-06T12:12:07Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -37,6 +40,14 @@ import (
 	listersv1 "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/pkg/version"

+	"bytes"
+	"fmt"


sort your imports please. this is a mess.

sttts · 2017-06-06T12:12:51Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -54,6 +65,10 @@ var (
 	Codecs               = serializer.NewCodecFactory(Scheme)
 )

+const (
+	LOAD_OPENAPI_SPEC_MAX_RETRIES = 10


godoc. Make it CamelCase. We don't use capital case anywhere else.

sttts · 2017-06-06T12:13:42Z

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

@@ -0,0 +1,482 @@
+/*


why is something about aggregation in the generic apiserver?

Discussed in a previous comment. Will fix.

sttts · 2017-06-06T12:14:27Z

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

+
+// Clone OpenAPI spec
+func CloneSpec(source *spec.Swagger) (*spec.Swagger, error) {
+	if ret, err := cloner.DeepCopy(source); err != nil {


this cloner is empty. We use the reflection code path without any custom deepcopy funcs. Intentional?

Yes. As far as I can tell, we can't generate DeepCopy for spec.Swagger as it is not in our codebase. Please let me know if I am wrong (I want to be in this case).

Good point! Just leave that for the moment. I will take care of it, when phase 2 of #48544 goes in.

deads2k · 2017-06-06T12:15:10Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -244,6 +280,20 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg
 		return nil
 	})

+	s.GenericAPIServer.PrepareOpenAPIService()
+
+	if s.GenericAPIServer.OpenAPIService != nil {


This should fall out more cleanly once you refactor for the above comments. I think aggregator should always server openapi, so once it's all contained in the correct repos and packages I suspect you'll end up with a cleaner end product. This reads like coding by coincidence.

sttts · 2017-06-06T12:16:51Z

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

@@ -180,6 +188,9 @@ func (s *GenericAPIServer) HealthzChecks() []healthz.HealthzChecker {
 func (s *GenericAPIServer) ListedPaths() []string {
 	return s.listedPathProvider.ListedPaths()
 }
+func (s *GenericAPIServer) OpenAPISpec() *spec.Swagger {
+	return s.OpenAPIService.GetSpec()


Why do we need this helper? Isn't s.OpenAPIService.GetSpec enough? No added value.

taking this back, it's the interface impl.

deads2k · 2017-06-06T12:18:03Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -274,6 +324,9 @@ func (s *APIAggregator) AddAPIService(apiService *apiregistration.APIService) {
 	}
 	proxyHandler.updateAPIService(apiService)
 	s.proxyHandlers[apiService.Name] = proxyHandler
+
+	s.deferLoadAPISpec(apiService.Name)


Ah ha. This is what broke kops. No reason for you to run before the proxy path handling completes.

And is there anything that triggers an update when resources of a user apiserver change? An APIService only changes when the group is modified (which doesn't know anything about the served resources). The whole design of the aggregator is around this very idea that knowing the group is enough and everything more detailed is proxied to the actual server.

Do you mean the spec for the Resource can change and hence the OpenAPI spec can change dynamically? For that reason, we need to periodically check with API servers or have a mechanism to listen on OpenAPI changes. With etag support, periodically checking doesn't look bad. On a general note, I think in previous comments we assumed ApiServer does serve static OpenAPI spec. We may need to agree on that point before some of these changes.

Yes, we need another channel for resource changes. The APIService informer is not enough.

Static in which sense? Resources can come and go. With CRD validation, we will also get JSON Schema snippets that can change any moment and should be part of the OpenAPI spec served by the aggregator.

sttts · 2017-06-06T12:18:22Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -244,6 +280,20 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg
 		return nil
 	})

+	s.GenericAPIServer.PrepareOpenAPIService()


why is this call needed? Don't we have the PrepareRun call for this?

deads2k · 2017-06-06T12:19:00Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go


 	// TODO unregister group level discovery when there are no more versions for the group
 	// We don't need this right away because the handler properly delegates when no versions are present
 }
+
+func (_ *APIAggregator) loadOpenAPISpec(p *proxyHandler, r *http.Request) (*spec.Swagger, error) {


This doesn't belong here, move to where your new handler lives.

This entire methods reads like duplication of function elsewhere. How about refactoring for the common bits that you need instead of copy/paste?

I tried refactoring for this one, it didn't make sense at the time. Will try again. Will move it to the handler anyway.

deads2k · 2017-06-06T12:22:18Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

+			s.apiServiceSpecs[name] = spec
+			loaded = true
+		}
+		s.toLoadAPISpec = newList


golang even allows this? It looks like a concurrent modification exception.

Why not structure it as a channel you requeue to with a single consumer and counts maintained separately. Simple for producers, simple for consumers, no locks, no mutating a collection you're iterating through.

Wow, this is kind of weird. So I think you're counting on having the pointer resolution for the loop get pinned, then you're mutating that pointer with a series of incorrect data until you've made it through the entire map and finally reassigned it to be correct? This construct should be removed and replaced.

sttts · 2017-06-06T12:30:25Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

+	loaded := false
+	newList := map[string]int{}
+	for name, retries := range s.toLoadAPISpec {
+		if retries >= LOAD_OPENAPI_SPEC_MAX_RETRIES {


shouldn't this be >? Otherwise, the first failure will set it to 1 and this if-clause ignores it.

sttts · 2017-06-06T12:35:17Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

+	}
+	openapi.FilterSpecByPaths(sp, []string{"/apis/apiregistration.k8s.io/"})
+	if _, found := sp.Paths.Paths["/version/"]; found {
+		return fmt.Errorf("Cleanup didn't work")


lower-case errors

sttts · 2017-06-06T12:48:32Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -302,6 +355,19 @@ func (s *APIAggregator) AddAPIService(apiService *apiregistration.APIService) {
 	s.handledGroups.Insert(apiService.Spec.Group)
 }

+func (s *APIAggregator) deferLoadAPISpec(name string) {


Do I get this right that this only puts the name onto the radar for the next update call which is triggered through a OpenAPI download request at https://github.com/kubernetes/kubernetes/pull/46734/files#diff-50c8cb84572c056e12063729607ca9edR149 ? If so, if we want such a interaction complexity at all, this deserves a decent comment. Where is this tested that we don't break that link?

This logic will be moved to a new (or existing) controller.

deads2k · 2017-06-06T13:03:43Z

Given the structural nature of the problems facing this pull and taking into account its importance in 1.7, I think the best path forward is to leave this in place for 1.7 and revert it in 1.8 so that @mbohlool can re-integrate it into the aggregator and apiserver in a maintainable way.

@lavalamp @sttts @liggitt @kubernetes/sig-api-machinery-misc

liggitt · 2017-06-06T14:14:36Z

Given the structural nature of the problems facing this pull and taking into account its importance in 1.7, I think the best path forward is to leave this in place for 1.7 and revert it in 1.8 so that @mbohlool can re-integrate it into the aggregator and apiserver in a maintainable way.

I agree

mbohlool · 2017-06-06T19:04:48Z

@liggitt @deads2k Thank you for the review. I know the PR has shortcomings and it was due to time constraint (only 4 days which 2 of them spent on setting up a test cluster with aggregation). I read all of your comments and instead of replying to them individually I am going to summarize them here and send a follow up PR to address them:

Functionality:

Add a new controller or integrate OpenAPI fetch process into current aggregator controller.

Refactoring and cleanup:

Remove all unnecessary changes in generic api server.
Add OpenAPI handler directly in kube-aggregator
Refactor the rest of the code into kube-aggregator
Small cleanups (import sorting, godoc, CamelCase for constants, etc.)

I can discuss your specific concerns in the follow up PR. I am fine with doing it in 1.7 but I am not sure I can make a strong case of exception for the code freeze, so most probably it will go into 1.8.

sttts · 2017-07-06T13:22:55Z

Bump ^^

deads2k · 2017-07-06T13:34:16Z

Yeah, let's go ahead with the revert and @mbohlool can have a second shot. Plenty of time in 1.8.

mbohlool · 2017-07-07T21:02:02Z

@liggitt @sttts @deads2k I went through your comments once and replied to them. Want to understand your comments and reach an agreement on them before start changing. My understanding is I only need to revert one of these commits ("Aggregate OpenAPI specs") in order to cleanly re-integrate them into the code. Let me know if you think otherwise.

Automatic merge from submit-queue (batch tested with PRs 49992, 48861, 49267, 49356, 49886) Reintegrate aggregation support for OpenAPI Reintegrating changes of #46734 Changes summary: - Extracted all OpenAPI specs to new repo `kube-openapi` - Make OpenAPI spec aggregator to copy and rename any non-requal model (even with documentation change only). - Load specs when adding APIServices and retry on failure until successful spec retrieval or a 404. - Assumes all Specs except aggregator's Spec are static - A re-register of any APIService will result in updating the spec for that service (Suggestion for TPR: they should be registered to aggregator API Server, Open for discussion if any more changes needed for another PR.) fixes #48548

k8s-github-robot assigned sttts and yujuhong May 31, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 31, 2017

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

mbohlool force-pushed the aggr branch from 89d526c to 2de6f2c Compare May 31, 2017 21:16

mbohlool added this to the v1.7 milestone May 31, 2017

mbohlool assigned mbohlool and unassigned sttts and yujuhong May 31, 2017

mbohlool force-pushed the aggr branch from 42fc1d1 to 9396bb4 Compare June 1, 2017 05:11

mbohlool requested a review from lavalamp June 1, 2017 05:12

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 1, 2017

mbohlool removed the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 1, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 1, 2017

mbohlool force-pushed the aggr branch from 9396bb4 to 869b94c Compare June 1, 2017 05:29

mbohlool force-pushed the aggr branch 7 times, most recently from 445e285 to 3feb7b6 Compare June 1, 2017 10:15

mbohlool changed the title ~~[WIP] OpenAPI aggregation for kube-aggregator~~ OpenAPI aggregation for kube-aggregator Jun 1, 2017

lavalamp reviewed Jun 1, 2017

View reviewed changes

lavalamp assigned lavalamp and unassigned mbohlool Jun 1, 2017

sttts reviewed Jun 6, 2017

View reviewed changes

deads2k reviewed Jun 6, 2017

View reviewed changes

sttts reviewed Jun 6, 2017

View reviewed changes

deads2k reviewed Jun 6, 2017

View reviewed changes

sttts reviewed Jun 6, 2017

View reviewed changes

deads2k reviewed Jun 6, 2017

View reviewed changes

sttts reviewed Jun 6, 2017

View reviewed changes

deads2k reviewed Jun 6, 2017

View reviewed changes

sttts reviewed Jun 6, 2017

View reviewed changes

deads2k reviewed Jun 6, 2017

View reviewed changes

sttts reviewed Jun 6, 2017

View reviewed changes

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 6, 2017

This was referenced Jul 6, 2017

Revert #46734: OpenAPI aggregation for kube-aggregator #48547

Closed

Reintegrate aggregation support for OpenAPI #48548

Closed

mbohlool mentioned this pull request Jul 13, 2017

Reintegrate aggregation support for OpenAPI #48861

Merged

OpenAPI aggregation for kube-aggregator #46734

OpenAPI aggregation for kube-aggregator #46734

Conversation

mbohlool commented May 31, 2017 • edited Loading

mbohlool commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jun 1, 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

mbohlool Jul 7, 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

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 Jul 7, 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

deads2k commented Jun 6, 2017

liggitt commented Jun 6, 2017

mbohlool commented Jun 6, 2017

sttts commented Jul 6, 2017

deads2k commented Jul 6, 2017

mbohlool commented Jul 7, 2017

mbohlool commented May 31, 2017 •

edited

Loading

mbohlool Jul 7, 2017 •

edited

Loading

mbohlool Jul 7, 2017 •

edited

Loading