Merge pull request kubernetes#80074 from yue9944882/bugfix/publish-path-parameter

Fixes missing path parameter to CRD restful container

Author: k8s-ci-robot

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",
     ],

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,30 +269,30 @@ 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{})
 		route.Reads(&metav1.DeleteOptions{}).ParameterNamed("body").Required(false)
 	}
 
 	// 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)
 	}
 

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go

@@ -21,13 +21,16 @@ 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"
 	structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
 	"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 {