From 18c27b77dc03d6deafcb36286de45bd025614440 Mon Sep 17 00:00:00 2001 From: yue9944882 <291271447@qq.com> Date: Fri, 12 Jul 2019 12:14:57 +0800 Subject: [PATCH] publish path parameter refactor: make route param inserting less fragile --- .../pkg/controller/openapi/BUILD | 3 + .../pkg/controller/openapi/builder.go | 71 ++++++------ .../pkg/controller/openapi/builder_test.go | 104 ++++++++++++++++++ 3 files changed, 147 insertions(+), 31 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD index ef711c98ec8..efdc22ef250 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD @@ -54,11 +54,14 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/json:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/endpoints:go_default_library", "//vendor/github.com/go-openapi/spec:go_default_library", "//vendor/github.com/google/go-cmp/cmp:go_default_library", "//vendor/github.com/google/gofuzz:go_default_library", "//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library", "//vendor/github.com/googleapis/gnostic/compiler:go_default_library", + "//vendor/github.com/stretchr/testify/assert:go_default_library", + "//vendor/github.com/stretchr/testify/require:go_default_library", "//vendor/gopkg.in/yaml.v2:go_default_library", "//vendor/k8s.io/kube-openapi/pkg/util/proto:go_default_library", ], diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go index ba7722a73ec..56ab468c009 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go @@ -52,6 +52,9 @@ const ( var ( swaggerPartialObjectMetadataDescriptions = metav1beta1.PartialObjectMetadata{}.SwaggerDoc() + + nameToken = "{name}" + namespaceToken = "{namespace}" ) var definitions map[string]common.OpenAPIDefinition @@ -96,33 +99,33 @@ func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string) ( routes := make([]*restful.RouteBuilder, 0) root := fmt.Sprintf("/apis/%s/%s/%s", b.group, b.version, b.plural) + if b.namespaced { - routes = append(routes, b.buildRoute(root, "", "GET", "list", sampleList). - Operation("list"+b.kind+"ForAllNamespaces")) + routes = append(routes, b.buildRoute(root, "", "GET", "list", "list", sampleList).Operation("list"+b.kind+"ForAllNamespaces")) root = fmt.Sprintf("/apis/%s/%s/namespaces/{namespace}/%s", b.group, b.version, b.plural) } - routes = append(routes, b.buildRoute(root, "", "GET", "list", sampleList)) - routes = append(routes, b.buildRoute(root, "", "POST", "create", sample).Reads(sample)) - routes = append(routes, b.buildRoute(root, "", "DELETE", "deletecollection", status)) + routes = append(routes, b.buildRoute(root, "", "GET", "list", "list", sampleList)) + routes = append(routes, b.buildRoute(root, "", "POST", "post", "create", sample).Reads(sample)) + routes = append(routes, b.buildRoute(root, "", "DELETE", "deletecollection", "deletecollection", status)) - routes = append(routes, b.buildRoute(root, "/{name}", "GET", "read", sample)) - routes = append(routes, b.buildRoute(root, "/{name}", "PUT", "replace", sample).Reads(sample)) - routes = append(routes, b.buildRoute(root, "/{name}", "DELETE", "delete", status)) - routes = append(routes, b.buildRoute(root, "/{name}", "PATCH", "patch", sample).Reads(patch)) + routes = append(routes, b.buildRoute(root, "/{name}", "GET", "get", "read", sample)) + routes = append(routes, b.buildRoute(root, "/{name}", "PUT", "put", "replace", sample).Reads(sample)) + routes = append(routes, b.buildRoute(root, "/{name}", "DELETE", "delete", "delete", status)) + routes = append(routes, b.buildRoute(root, "/{name}", "PATCH", "patch", "patch", sample).Reads(patch)) subresources, err := apiextensions.GetSubresourcesForVersion(crd, version) if err != nil { return nil, err } if subresources != nil && subresources.Status != nil { - routes = append(routes, b.buildRoute(root, "/{name}/status", "GET", "read", sample)) - routes = append(routes, b.buildRoute(root, "/{name}/status", "PUT", "replace", sample).Reads(sample)) - routes = append(routes, b.buildRoute(root, "/{name}/status", "PATCH", "patch", sample).Reads(patch)) + routes = append(routes, b.buildRoute(root, "/{name}/status", "GET", "get", "read", sample)) + routes = append(routes, b.buildRoute(root, "/{name}/status", "PUT", "put", "replace", sample).Reads(sample)) + routes = append(routes, b.buildRoute(root, "/{name}/status", "PATCH", "patch", "patch", sample).Reads(patch)) } if subresources != nil && subresources.Scale != nil { - routes = append(routes, b.buildRoute(root, "/{name}/scale", "GET", "read", scale)) - routes = append(routes, b.buildRoute(root, "/{name}/scale", "PUT", "replace", scale).Reads(scale)) - routes = append(routes, b.buildRoute(root, "/{name}/scale", "PATCH", "patch", scale).Reads(patch)) + routes = append(routes, b.buildRoute(root, "/{name}/scale", "GET", "get", "read", scale)) + routes = append(routes, b.buildRoute(root, "/{name}/scale", "PUT", "put", "replace", scale).Reads(scale)) + routes = append(routes, b.buildRoute(root, "/{name}/scale", "PATCH", "patch", "patch", scale).Reads(patch)) } for _, route := range routes { @@ -189,9 +192,9 @@ func subresource(path string) string { panic("failed to parse subresource; invalid path") } -func (b *builder) descriptionFor(path, verb string) string { +func (b *builder) descriptionFor(path, operationVerb string) string { var article string - switch verb { + switch operationVerb { case "list": article = " objects of kind " case "read", "replace": @@ -209,7 +212,7 @@ func (b *builder) descriptionFor(path, verb string) string { if len(sub) > 0 { sub = " " + sub + " of" } - switch verb { + switch operationVerb { case "patch": description = "partially update" + sub + article + b.kind case "deletecollection": @@ -219,7 +222,7 @@ func (b *builder) descriptionFor(path, verb string) string { } description = "delete collection of" + sub + " " + b.kind default: - description = verb + sub + article + b.kind + description = operationVerb + sub + article + b.kind } return description @@ -229,29 +232,35 @@ func (b *builder) descriptionFor(path, verb string) string { // action can be one of: GET, PUT, PATCH, POST, DELETE; // verb can be one of: list, read, replace, patch, create, delete, deletecollection; // sample is the sample Go type for response type. -func (b *builder) buildRoute(root, path, action, verb string, sample interface{}) *restful.RouteBuilder { +func (b *builder) buildRoute(root, path, httpMethod, actionVerb, operationVerb string, sample interface{}) *restful.RouteBuilder { var namespaced string if b.namespaced { namespaced = "Namespaced" } - route := b.ws.Method(action). + route := b.ws.Method(httpMethod). Path(root+path). To(func(req *restful.Request, res *restful.Response) {}). - Doc(b.descriptionFor(path, verb)). + Doc(b.descriptionFor(path, operationVerb)). Param(b.ws.QueryParameter("pretty", "If 'true', then the output is pretty printed.")). - Operation(verb+namespaced+b.kind+strings.Title(subresource(path))). + Operation(operationVerb+namespaced+b.kind+strings.Title(subresource(path))). Metadata(endpoints.ROUTE_META_GVK, metav1.GroupVersionKind{ Group: b.group, Version: b.version, Kind: b.kind, }). - Metadata(endpoints.ROUTE_META_ACTION, strings.ToLower(action)). + Metadata(endpoints.ROUTE_META_ACTION, actionVerb). Produces("application/json", "application/yaml"). Returns(http.StatusOK, "OK", sample). Writes(sample) + if strings.Contains(root, namespaceToken) || strings.Contains(path, namespaceToken) { + route.Param(b.ws.PathParameter("namespace", "object name and auth scope, such as for teams and projects").DataType("string")) + } + if strings.Contains(root, nameToken) || strings.Contains(path, nameToken) { + route.Param(b.ws.PathParameter("name", "name of the "+b.kind).DataType("string")) + } // Build consume media types - if action == "PATCH" { + if httpMethod == "PATCH" { route.Consumes("application/json-patch+json", "application/merge-patch+json", "application/strategic-merge-patch+json") @@ -260,16 +269,16 @@ func (b *builder) buildRoute(root, path, action, verb string, sample interface{} } // Build option parameters - switch verb { + switch actionVerb { case "get": // TODO: CRD support for export is still under consideration endpoints.AddObjectParams(b.ws, route, &metav1.GetOptions{}) case "list", "deletecollection": endpoints.AddObjectParams(b.ws, route, &metav1.ListOptions{}) - case "replace", "patch": + case "put", "patch": // TODO: PatchOption added in feature branch but not in master yet endpoints.AddObjectParams(b.ws, route, &metav1.UpdateOptions{}) - case "create": + case "post": endpoints.AddObjectParams(b.ws, route, &metav1.CreateOptions{}) case "delete": endpoints.AddObjectParams(b.ws, route, &metav1.DeleteOptions{}) @@ -277,13 +286,13 @@ func (b *builder) buildRoute(root, path, action, verb string, sample interface{} } // Build responses - switch verb { - case "create": + switch actionVerb { + case "post": route.Returns(http.StatusAccepted, "Accepted", sample) route.Returns(http.StatusCreated, "Created", sample) case "delete": route.Returns(http.StatusAccepted, "Accepted", sample) - case "replace": + case "put": route.Returns(http.StatusCreated, "Created", sample) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go index 89e919cf283..dc22e6e9299 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go @@ -21,6 +21,8 @@ import ( "testing" "github.com/go-openapi/spec" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" @@ -28,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apiserver/pkg/endpoints" ) func TestNewBuilder(t *testing.T) { @@ -417,6 +420,107 @@ func TestNewBuilder(t *testing.T) { } } +func TestCRDRouteParameterBuilder(t *testing.T) { + testCRDKind := "Foo" + testCRDGroup := "foo-group" + testCRDVersion := "foo-version" + testCRDResourceName := "foos" + + testCases := []struct { + scope apiextensions.ResourceScope + paths map[string]struct { + expectNamespaceParam bool + expectNameParam bool + expectedActions sets.String + } + }{ + { + scope: apiextensions.NamespaceScoped, + paths: map[string]struct { + expectNamespaceParam bool + expectNameParam bool + expectedActions sets.String + }{ + "/apis/foo-group/foo-version/foos": {expectNamespaceParam: false, expectNameParam: false, expectedActions: sets.NewString("list")}, + "/apis/foo-group/foo-version/namespaces/{namespace}/foos": {expectNamespaceParam: true, expectNameParam: false, expectedActions: sets.NewString("post", "list", "deletecollection")}, + "/apis/foo-group/foo-version/namespaces/{namespace}/foos/{name}": {expectNamespaceParam: true, expectNameParam: true, expectedActions: sets.NewString("get", "put", "patch", "delete")}, + "/apis/foo-group/foo-version/namespaces/{namespace}/foos/{name}/scale": {expectNamespaceParam: true, expectNameParam: true, expectedActions: sets.NewString("get", "patch", "put")}, + "/apis/foo-group/foo-version/namespaces/{namespace}/foos/{name}/status": {expectNamespaceParam: true, expectNameParam: true, expectedActions: sets.NewString("get", "patch", "put")}, + }, + }, + { + scope: apiextensions.ClusterScoped, + paths: map[string]struct { + expectNamespaceParam bool + expectNameParam bool + expectedActions sets.String + }{ + "/apis/foo-group/foo-version/foos": {expectNamespaceParam: false, expectNameParam: false, expectedActions: sets.NewString("post", "list", "deletecollection")}, + "/apis/foo-group/foo-version/foos/{name}": {expectNamespaceParam: false, expectNameParam: true, expectedActions: sets.NewString("get", "put", "patch", "delete")}, + "/apis/foo-group/foo-version/foos/{name}/scale": {expectNamespaceParam: false, expectNameParam: true, expectedActions: sets.NewString("get", "patch", "put")}, + "/apis/foo-group/foo-version/foos/{name}/status": {expectNamespaceParam: false, expectNameParam: true, expectedActions: sets.NewString("get", "patch", "put")}, + }, + }, + } + + for _, testCase := range testCases { + testNamespacedCRD := &apiextensions.CustomResourceDefinition{ + Spec: apiextensions.CustomResourceDefinitionSpec{ + Scope: testCase.scope, + Group: testCRDGroup, + Names: apiextensions.CustomResourceDefinitionNames{ + Kind: testCRDKind, + Plural: testCRDResourceName, + }, + Versions: []apiextensions.CustomResourceDefinitionVersion{ + { + Name: testCRDVersion, + }, + }, + Subresources: &apiextensions.CustomResourceSubresources{ + Status: &apiextensions.CustomResourceSubresourceStatus{}, + Scale: &apiextensions.CustomResourceSubresourceScale{}, + }, + }, + } + swagger, err := BuildSwagger(testNamespacedCRD, testCRDVersion) + require.NoError(t, err) + require.Equal(t, len(testCase.paths), len(swagger.Paths.Paths), testCase.scope) + for path, expected := range testCase.paths { + t.Run(path, func(t *testing.T) { + path, ok := swagger.Paths.Paths[path] + if !ok { + t.Errorf("unexpected path %v", path) + } + + hasNamespaceParam := false + hasNameParam := false + for _, param := range path.Parameters { + if param.In == "path" && param.Name == "namespace" { + hasNamespaceParam = true + } + if param.In == "path" && param.Name == "name" { + hasNameParam = true + } + } + assert.Equal(t, expected.expectNamespaceParam, hasNamespaceParam) + assert.Equal(t, expected.expectNameParam, hasNameParam) + + actions := sets.NewString() + for _, operation := range []*spec.Operation{path.Get, path.Post, path.Put, path.Patch, path.Delete} { + if operation != nil { + action, ok := operation.VendorExtensible.Extensions.GetString(endpoints.ROUTE_META_ACTION) + if ok { + actions.Insert(action) + } + } + } + assert.Equal(t, expected.expectedActions, actions) + }) + } + } +} + func properties(p map[string]spec.Schema) sets.String { ret := sets.NewString() for k := range p {