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.

Sign up for GitHub

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

Jump to bottom

Give better error message for PUTs with no resource version #5690

Merged
merged 1 commit into from
Mar 27, 2015
Merged

Give better error message for PUTs with no resource version #5690

merged 1 commit into from
Mar 27, 2015

Conversation

satnam6502
Copy link
Contributor

Addresses issue #3085
Before if a PUT was made without a resource version an unhelpful error like this was returned:

{
  "kind": "Status",
  "creationTimestamp": null,
  "apiVersion": "v1beta1",
  "status": "Failure",
  "message": "service \"redis-master\" cannot be updated: 105: Key already exists (/registry/services/specs/default/redis-master) [28168]",
  "reason": "Conflict",
  "details": {
    "id": "redis-master",
    "kind": "service"
  },
  "code": 409
}

Now a more specific error is returned:

{
  "kind": "Status",
  "creationTimestamp": null,
  "apiVersion": "v1beta1",
  "status": "Failure",
  "message": "Service redis-master cannot be updated: no resource version supplied in PUT request",
  "code": 500
}

I ignore the error value from typer.ObjectVersionAndKind(obj) since I think it is more important to return the error message about the version.
Likewise, I ignore the error message from a.Name(obj).

@satnam6502 satnam6502 added this to the High Pri Bug fix-it milestone Mar 20, 2015
@bgrant0607 bgrant0607 mentioned this pull request Mar 20, 2015
Closed
bgrant0607
bgrant0607 reviewed Mar 20, 2015
View reviewed changes
pkg/apiserver/resthandler.go
@@ -294,6 +295,22 @@ func UpdateResource(r RESTUpdater, ctxFn ContextFunc, namer ScopeNamer, codec ru
return
}

// Check to see if a resource version has been supplied.
// If there is no resource version then fail this update request.
// ALTERNATIVE: Update at the most recent resource version.
Copy link
Member

Choose a reason for hiding this comment

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

We should not coerce to the most recent version by default. That would make it too easy to clobber the desired state.

Seems like this should be done in ValidateObjectMetaUpdate:
https://github.com/GoogleCloudPlatform/kubernetes/blob/9bc2b0c2dbb92e25a48c5aa21e3d3e2ef7d18687/pkg/api/validation/validation.go#L216

It can't be done here, because we allow creation via PUT as well, in which case there is no resourceVersion -- see line 327 below.

@satnam6502
Copy link
Contributor Author

@bgrant0607 : thanks, I will move the check. I will try to return the "details" as before and perhaps a different status code (bad request).

@satnam6502
Copy link
Contributor Author

Changed. Now when a PUT is made without a resource version the following error like this is returned:

{
  "kind": "Status",
  "creationTimestamp": null,
  "apiVersion": "v1beta1",
  "status": "Failure",
  "message": "service \"redis-master\" is invalid: metadata.resourceVersion: invalid value '': resourceVersion must be specified for an update",
  "reason": "Invalid",
  "details": {
    "id": "redis-master",
    "kind": "service",
    "causes": [
      {
        "reason": "FieldValueInvalid",
        "message": "metadata.resourceVersion: invalid value '': resourceVersion must be specified for an update",
        "field": "metadata.resourceVersion"
      }
    ]
  },
  "code": 422
}

PTAL.
Running e2e tests and I will report status when they are done.

@bgrant0607
Copy link
Member

Did you push your change?

@satnam6502 satnam6502 force-pushed the failputnover branch 3 times, most recently from edd7fb4 to b0e8262 Compare March 20, 2015 22:39
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot
Copy link

CLAs look good, thanks!

@satnam6502
Copy link
Contributor Author

I have now. Lots of Git confusion on my part.

@satnam6502
Copy link
Contributor Author

One e2e failure.

Summarizing 1 Failure:

[Fail] kubectl guestbook [It] should create and stop a working application 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/kubectl.go:122

@satnam6502
Copy link
Contributor Author

Running a second time made the guestbook test pass.

• [SLOW TEST:89.359 seconds]
kubectl
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/kubectl.go:112
  guestbook
  /go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/kubectl.go:110
    should create and stop a working application
    /go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/kubectl.go:109
------------------------------
SSSSSSSS
Ran 4 of 31 Specs in 206.457 seconds
SUCCESS! -- 4 Passed | 0 Failed | 0 Pending | 27 Skipped I0320 16:06:11.579530   29123 driver.go:89] All tests pass

@bgrant0607
Copy link
Member

LGTM. Rerunning tests.

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2015
@bgrant0607 bgrant0607 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2015
@bgrant0607
Copy link
Member

Well, the check does something:

--- FAIL: TestServiceRegistryUpdate (0.00s)
    rest_test.go:142: Expected no error: service "foo" is invalid: metadata.resourceVersion: invalid value '': resourceVersion must be specified for an update
--- FAIL: TestServiceRegistryUpdateExternalService (0.00s)
    rest_test.go:338: Unexpected call(s): []string(nil)
    rest_test.go:349: Unexpected call(s): []string(nil)
--- FAIL: TestServiceRegistryIPUpdate (0.00s)
panic: interface conversion: interface is nil, not *api.Service [recovered]
    panic: interface conversion: interface is nil, not *api.Service

@bgrant0607
Copy link
Member

@smarterclayton What's your opinion on whether we should require resourceVersion for non-creating PUTs? It feels like there should be a way to circumvent the concurrency check, but doing that by just omitting resourceVersion feels dangerous. I'd prefer the default behavior be safe.

@satnam6502
Copy link
Contributor Author

Updated the unit tests. PTAL.

@bgrant0607
Copy link
Member

LGTM

bgrant0607 added a commit that referenced this pull request Mar 27, 2015
@bgrant0607
Merge pull request #5690 from satnam6502/failputnover
a4bf91c 
Give better error message for PUTs with no resource version
@bgrant0607 bgrant0607 merged commit a4bf91c into kubernetes:master Mar 27, 2015
@manusa manusa mentioned this pull request May 11, 2022
Merged
@parul5sahoo parul5sahoo mentioned this pull request Jul 22, 2022
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bgrant0607 bgrant0607

Labels
None yet
Projects
None yet
Milestone
High Pri Bug fix-it
Development

Successfully merging this pull request may close these issues.
4 participants
@satnam6502 @bgrant0607 @googlebot @nikhiljindal