Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serve OpenAPI spec with single /openapi/v2 endpoint #59293

Merged
merged 3 commits into from
Feb 27, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions Godeps/Godeps.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ func ClusterRoles() []rbac.ClusterRole {
// do not expand this pattern for openapi discovery docs
// move to a single openapi endpoint that takes accept/accept-encoding headers
"/swagger.json", "/swagger-2.0.0.pb-v1",
"/openapi", "/openapi/*",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saving the place for /openapi/v3 in future. Also, we will add /openapi (which returns list of supported versions) as a followup.

"/api", "/api/*",
"/apis", "/apis/*",
).RuleOrDie(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,8 @@ items:
- /apis
- /apis/*
- /healthz
- /openapi
- /openapi/*
- /swagger-2.0.0.pb-v1
- /swagger.json
- /swaggerapi
Expand Down
10 changes: 5 additions & 5 deletions staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion staging/src/k8s.io/apimachinery/Godeps/Godeps.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions staging/src/k8s.io/apiserver/Godeps/Godeps.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 8 additions & 1 deletion staging/src/k8s.io/apiserver/pkg/server/routes/openapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ limitations under the License.
package routes

import (
"github.com/emicklei/go-restful"
restful "github.com/emicklei/go-restful"
"github.com/golang/glog"

"k8s.io/apiserver/pkg/server/mux"
Expand All @@ -32,8 +32,15 @@ type OpenAPI struct {

// Install adds the SwaggerUI webservice to the given mux.
func (oa OpenAPI) Install(c *restful.Container, mux *mux.PathRecorderMux) {
// NOTE: [DEPRECATION] We will announce deprecation for format-separated endpoints for OpenAPI spec,
// and switch to a single /openapi/v2 endpoint in Kubernetes 1.10. The design doc and deprecation process
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.10 or 1.11?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deprecation plan in design doc is to announce deprecation in 1.10, redirect old endpoint to new endpoint in 1.11, and remove old endpoint (and probably return a deprecation message) in 1.12

// are tracked at: https://docs.google.com/document/d/19lEqE9lc4yHJ3WJAJxS_G7TcORIJXGHyq3wpwcH28nU.
_, err := handler.BuildAndRegisterOpenAPIService("/swagger.json", c.RegisteredWebServices(), oa.Config, mux)
if err != nil {
glog.Fatalf("Failed to register open api spec for root: %v", err)
}
_, err = handler.BuildAndRegisterOpenAPIVersionedService("/openapi/v2", c.RegisteredWebServices(), oa.Config, mux)
if err != nil {
glog.Fatalf("Failed to register versioned open api spec for root: %v", err)
}
}
2 changes: 1 addition & 1 deletion staging/src/k8s.io/client-go/Godeps/Godeps.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 17 additions & 4 deletions staging/src/k8s.io/client-go/discovery/discovery_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,12 @@ import (
restclient "k8s.io/client-go/rest"
)

// defaultRetries is the number of times a resource discovery is repeated if an api group disappears on the fly (e.g. ThirdPartyResources).
const defaultRetries = 2
const (
// defaultRetries is the number of times a resource discovery is repeated if an api group disappears on the fly (e.g. ThirdPartyResources).
defaultRetries = 2
// protobuf mime type
mimePb = "application/com.github.proto-openapi.spec.v2@v1.0+protobuf"
)

// DiscoveryInterface holds the methods that discover server-supported API groups,
// versions and resources.
Expand Down Expand Up @@ -329,9 +333,18 @@ func (d *DiscoveryClient) ServerVersion() (*version.Info, error) {

// 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()
data, err := d.restClient.Get().AbsPath("/openapi/v2").SetHeader("Accept", mimePb).Do().Raw()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot change this until 1.11 to maintain one version skew compatibility in kubectl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can do that or we can try new endpoint first and then fallback on the old one. Pro: we have new endpoints in one version and if there is any problem, we would figure it out before completely remove the old endpoint. Con: new clients will have a little more latency to talk to old clusters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was working on the fallback logic after I sent out the PR. Thanks for pointing that out :) Updated.

if err != nil {
return nil, err
if errors.IsForbidden(err) || errors.IsNotFound(err) {
Copy link
Member

@liggitt liggitt Feb 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the apiserver in 1.9 (and in master) returns the same answer from /openapi/v2 as it does from /

that means a superuser (or request against the unsecured port) won't get an HTTP error, it'll get a json doc that isn't the openapi document. this code needs to detect that and we should fix that bug in the apiserver so that an unhandled path returns a 404, and doesn't fall back to /

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt the apiserver seems to return a 404 with the / json doc as body, which follows HTTPbis spec

s.Handler.NonGoRestfulMux.NotFoundHandler(routes.IndexLister{
StatusCode: http.StatusNotFound,
PathProvider: s.listedPathProvider,
})

< HTTP/1.1 404 Not Found
< Content-Length: 524
< Content-Type: application/json
< Date: Fri, 23 Feb 2018 20:04:32 GMT
<
{
"paths": [
"/apis",
"/apis/",
"/apis/apiextensions.k8s.io",
"/apis/apiextensions.k8s.io/v1beta1",
"/healthz",
"/healthz/etcd",
"/healthz/ping",
"/healthz/poststarthook/generic-apiserver-start-informers",
"/healthz/poststarthook/start-apiextensions-controllers",
"/healthz/poststarthook/start-apiextensions-informers",
"/metrics",
"/swagger-2.0.0.json",
"/swagger-2.0.0.pb-v1",
"/swagger-2.0.0.pb-v1.gz",
"/swagger.json",
"/swaggerapi",
"/version"
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't notice the http status was 404. That's good

// single endpoint not found/registered in old server, try to fetch old endpoint
// TODO(roycaihw): remove this in 1.11
data, err = d.restClient.Get().AbsPath("/swagger-2.0.0.pb-v1").Do().Raw()
if err != nil {
return nil, err
}
} else {
return nil, err
}
}
document := &openapi_v2.Document{}
err = proto.Unmarshal(data, document)
Expand Down
51 changes: 50 additions & 1 deletion staging/src/k8s.io/client-go/discovery/discovery_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -326,9 +326,14 @@ var returnedOpenAPI = openapi_v2.Document{
},
}

func openapiSchemaFakeServer() (*httptest.Server, error) {
func openapiSchemaDeprecatedFakeServer() (*httptest.Server, error) {
var sErr error
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
// old server returns 403 on new endpoint request
if req.URL.Path == "/openapi/v2" {
w.WriteHeader(http.StatusForbidden)
return
}
if req.URL.Path != "/swagger-2.0.0.pb-v1" {
sErr = fmt.Errorf("Unexpected url %v", req.URL)
}
Expand All @@ -349,6 +354,33 @@ func openapiSchemaFakeServer() (*httptest.Server, error) {
return server, sErr
}

func openapiSchemaFakeServer() (*httptest.Server, error) {
var sErr error
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
if req.URL.Path != "/openapi/v2" {
sErr = fmt.Errorf("Unexpected url %v", req.URL)
}
if req.Method != "GET" {
sErr = fmt.Errorf("Unexpected method %v", req.Method)
}
decipherableFormat := req.Header.Get("Accept")
if decipherableFormat != "application/com.github.proto-openapi.spec.v2@v1.0+protobuf" {
sErr = fmt.Errorf("Unexpected accept mime type %v", decipherableFormat)
}

mime.AddExtensionType(".pb-v1", "application/com.github.googleapis.gnostic.OpenAPIv2@68f4ded+protobuf")

output, err := proto.Marshal(&returnedOpenAPI)
if err != nil {
sErr = err
return
}
w.WriteHeader(http.StatusOK)
w.Write(output)
}))
return server, sErr
}

func TestGetOpenAPISchema(t *testing.T) {
server, err := openapiSchemaFakeServer()
if err != nil {
Expand All @@ -366,6 +398,23 @@ func TestGetOpenAPISchema(t *testing.T) {
}
}

func TestGetOpenAPISchemaFallback(t *testing.T) {
server, err := openapiSchemaDeprecatedFakeServer()
if err != nil {
t.Errorf("unexpected error starting fake server: %v", err)
}
defer server.Close()

client := NewDiscoveryClientForConfigOrDie(&restclient.Config{Host: server.URL})
got, err := client.OpenAPISchema()
if err != nil {
t.Fatalf("unexpected error getting openapi: %v", err)
}
if e, a := returnedOpenAPI, *got; !reflect.DeepEqual(e, a) {
t.Errorf("expected %v, got %v", e, a)
}
}

func TestServerPreferredResources(t *testing.T) {
stable := metav1.APIResourceList{
GroupVersion: "v1",
Expand Down
4 changes: 2 additions & 2 deletions staging/src/k8s.io/code-generator/Godeps/Godeps.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions staging/src/k8s.io/kube-aggregator/Godeps/Godeps.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"sync"
"time"

"github.com/emicklei/go-restful"
restful "github.com/emicklei/go-restful"
"github.com/go-openapi/spec"

"k8s.io/apiserver/pkg/server"
Expand Down Expand Up @@ -51,7 +51,8 @@ type specAggregator struct {
openAPISpecs map[string]*openAPISpecInfo

// provided for dynamic OpenAPI spec
openAPIService *handler.OpenAPIService
openAPIService *handler.OpenAPIService
openAPIVersionedService *handler.OpenAPIService
}

var _ AggregationManager = &specAggregator{}
Expand Down Expand Up @@ -109,11 +110,19 @@ func BuildAndRegisterAggregator(downloader *Downloader, delegationTarget server.
}

// Install handler
// NOTE: [DEPRECATION] We will announce deprecation for format-separated endpoints for OpenAPI spec,
// and switch to a single /openapi/v2 endpoint in Kubernetes 1.10. The design doc and deprecation process
// are tracked at: https://docs.google.com/document/d/19lEqE9lc4yHJ3WJAJxS_G7TcORIJXGHyq3wpwcH28nU.
s.openAPIService, err = handler.RegisterOpenAPIService(
specToServe, "/swagger.json", pathHandler)
if err != nil {
return nil, err
}
s.openAPIVersionedService, err = handler.RegisterOpenAPIVersionedService(
specToServe, "/openapi/v2", pathHandler)
if err != nil {
return nil, err
}

return s, nil
}
Expand Down Expand Up @@ -207,14 +216,25 @@ func (s *specAggregator) buildOpenAPISpec() (specToReturn *spec.Swagger, err err

// updateOpenAPISpec aggregates all OpenAPI specs. It is not thread-safe. The caller is responsible to hold proper locks.
func (s *specAggregator) updateOpenAPISpec() error {
if s.openAPIService == nil {
if s.openAPIService == nil || s.openAPIVersionedService == nil {
// openAPIVersionedService and deprecated openAPIService should be initialized together
if !(s.openAPIService == nil && s.openAPIVersionedService == nil) {
return fmt.Errorf("unexpected openapi service initialization error")
}
return nil
}
specToServe, err := s.buildOpenAPISpec()
if err != nil {
return err
}
return s.openAPIService.UpdateSpec(specToServe)
// openAPIService.UpdateSpec and openAPIVersionedService.UpdateSpec read the same swagger spec
// serially and update their local caches separately. Both endpoints will have same spec in
// their caches if the caller is holding proper locks.
err = s.openAPIService.UpdateSpec(specToServe)
if err != nil {
return err
}
return s.openAPIVersionedService.UpdateSpec(specToServe)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we know that versionedService's updateSpec will also update the spec fo the old endpoint? We should verify that and add a comment here to document we confirmed that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added

}

// tryUpdatingServiceSpecs tries updating openAPISpecs map with specified specInfo, and keeps the map intact
Expand Down