Posting a rollback returns a deploymentstatus

[nhumrich]

Posting a rollback doesnt return a rollback object, it instead returns a deployment status.

What this PR does / why we need it:
Fixes the swagger spec. Its needed because the automatically generated clients (i,e. python) break when calling this api because they cant correctly deserialize the object

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nhumrich
We suggest the following additional approver: liggitt

Assign the PR to them by writing /assign @liggitt in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

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

• api/OWNERS

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

[nhumrich]

/release-note-none

[nhumrich]

/assign @liggitt

[liggitt]

This file is generated and can not be manually modified.

[liggitt]

@mbohlool suggestions for ensuring the correct return type is displayed?

[nhumrich]

This file is generated and can not be manually modified.

I have no idea where to even go to change this then. Can you point me in the right direction?

[dims]

@nhumrich you are probably on the right track with this change, next step would be to run hack/update-all.sh to see what other files get changed/updated with this change in the spec file.

@liggitt i believe the swagger.json is the one that gets auto generated. but the experiment above update-all should confirm if the changes hold up. for sure, just editing the json file is not enough. let me kick off the tests to confirm.

/ok-to-test

[k8s-ci-robot]

@nhumrich: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	a0145b6	link	/test pull-kubernetes-verify

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.

[nhumrich]

I would really like to help/contribute. But honestly, im stuck. I have no idea where the change needs to be made:

This file is generated and can not be manually modified.

I tried the suggested:

next step would be to run hack/update-all.sh to see what other files get changed/updated with this change in the spec file.

but just get an error, cant even run the file.

+++ [1206 21:14:52] Restoring kubernetes godeps
!!! Error in ./hack/update-all.sh:56
  Error in ./hack/update-all.sh:56. '"${KUBE_ROOT}/hack/godep-restore.sh" ${V}' exited with status 1
Call stack:
  1: ./hack/update-all.sh:56 main(...)
Exiting with status 1

I have no idea where to go from here (yes I ran make). Any suggestions?

[saad-ali]

@caesarxuchao can you help

[roycaihw]

/assign

I think I figured out what's going on here. The openapi-spec & swagger-spec are auto-generated in two steps:

1. staging/src/k8s.io/code-generator/cmd/openapi-gen/ generates all the openapi schema based on the Go types.

2. In order to get the routes & response schema, we actually bring up a local cluster and register all the API groups (try hack/update-openapi-spec.sh. you can find the apiserver log in /tmp/openapi-api-server.log)

The problem is in the second step. When we are installing extensions.v1beta1, the apiserver by defaults registered extensions.v1beta1.DeploymentRollback as producedObject for
POST /apis/extensions/v1beta1/namespaces/{namespace}/deployments/{name}/rollback. (The TODO is related to our OpenAPI v3 improvements )

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

Lines 703 to 707 in da44038

	Returns(http.StatusOK, "OK", producedObject).
	// TODO: in some cases, the API may return a v1.Status instead of the versioned object
	// but currently go-restful can't handle multiple different objects being returned.
	Returns(http.StatusCreated, "Created", producedObject).
	Returns(http.StatusAccepted, "Accepted", producedObject).

Note that you can specified the producedObject other than default, if the rest storage implements the rest.StorageMetadata interface:

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

Lines 236 to 239 in da44038

	storageMeta, isMetadata := storage.(rest.StorageMetadata)
	if !isMetadata {
	storageMeta = defaultStorageMetadata{}
	}

The rest.Storage implementation for extensions.deployment lives in pkg/registry/extensions/rest/storage_extensions.go, which refers to apps.deployment

kubernetes/pkg/registry/extensions/rest/storage_extensions.go

Line 69 in da44038

storage["deployments/rollback"] = deploymentStorage.Rollback

I tested with adding the following lines into pkg/registry/apps/deployment/storage/storage.go

// ProducesMIMETypes returns a list of the MIME types the specified HTTP verb (GET, POST, DELETE,
// PATCH) can respond with.
func (r *RollbackREST) ProducesMIMETypes(verb string) []string {
       return []string{}
}

// ProducesObject returns an object the specified HTTP verb respond with. It will overwrite storage object if
// it is not nil. Only the type of the return object matters, the value will be ignored.
func (r *RollbackREST) ProducesObject(verb string) interface{} {
       return &metav1.Status{}
}

var _ = rest.StorageMetadata(&RollbackREST{})

and ran hack/update-openapi-spec.sh. The openapi-spec gets updated:

@@ -29197,19 +29197,19 @@
       "200": {
        "description": "OK",
        "schema": {
-        "$ref": "#/definitions/io.k8s.api.apps.v1beta1.DeploymentRollback"
+        "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Status"
...
... (201 and 401 as well)
@@ -47551,19 +47551,19 @@
       "200": {
        "description": "OK",
        "schema": {
-        "$ref": "#/definitions/io.k8s.api.extensions.v1beta1.DeploymentRollback"
+        "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Status"
...
... (201 and 401 as well)

Note that this changes both extensions.deployment and apps.deployment. Please also update the swagger-spec. It probably worth a release note even for fixing a (deprecated) wrong API schema.

[roffe]

Stale issue?

[nhumrich]

I hope this issue isn't stale. I could really benefit from a fix. I don't have the knowledge to know how to push this issue further by myself.

[roycaihw]

@nhumrich If #56591 (comment) wasn't clear, I can apply the fix if you want.

[nhumrich]

Yes please

[greggilbert]

Hey - sorry, I didn't quite follow all of the above. Does #63837 being merged in supersede this PR, or does this need to go in as well in order for the issue to be resolved?

[roycaihw]

@greggilbert #63837 contains the change in this PR. The openapi/swagger specs has been fixed. We could close this PR now

/close