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

Shouldnt be returning etcd errors #10064

Closed
nikhiljindal opened this issue Jun 18, 2015 · 10 comments
Closed

Shouldnt be returning etcd errors #10064

nikhiljindal opened this issue Jun 18, 2015 · 10 comments
Assignees
@nikhiljindal
Labels
area/apiserver area/security area/usability kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@nikhiljindal
Copy link
Contributor

As discussed with @bgrant0607, we return etcd error as is if update fails: https://github.com/GoogleCloudPlatform/kubernetes/blob/5520386b180d3ddc4fa7b7dfe6f52642cc0c25f3/pkg/api/errors/etcd/etcd.go#L48. All other functions except InterpretUpdateError() are fine.

Also, we shouldnt be checking resourceVersion while updating in the apiServer: https://github.com/GoogleCloudPlatform/kubernetes/blob/4fdcbc3096432692ee27c8d3f7e85580b29f780c/pkg/registry/generic/etcd/etcd.go#L302. We should let etcd do the conflict check.
@nikhiljindal nikhiljindal self-assigned this Jun 18, 2015
@nikhiljindal nikhiljindal added priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jun 18, 2015
@bgrant0607
Copy link
Member

See also #10031

@bgrant0607
Copy link
Member

And #1990

@bgrant0607
Copy link
Member

We should log unexpected etcd errors (at a suitably high verbosity level).

@bgrant0607 bgrant0607 added this to the v1.0 milestone Jun 19, 2015
@davidopp davidopp mentioned this issue Jun 20, 2015
Closed
@bgrant0607
Copy link
Member

Also the case from #10031 reported by @smarterclayton :
The events REST storage object is not using generic.Etcd, and it's not wrapping the returned errors from etcd with the appropriate pkg/api/errors/etcd handles. As a result, we return raw etcd errors to clients which leaks server information (security) and is also ugly.

Spawned from openshift/origin#3196

@bgrant0607
Copy link
Member

The general issue is #1990

@bgrant0607
Copy link
Member

The etcd errors were previously noted there: #1990 (comment)

@nikhiljindal nikhiljindal mentioned this issue Jun 23, 2015
Merged
@bgrant0607
Copy link
Member

Can this be closed?

@bgrant0607 bgrant0607 added the kind/bug Categorizes issue or PR as related to a bug. label Jul 1, 2015
@smarterclayton
Copy link
Contributor

I had a comment about the linked change - I would have preferred not to change etcd/error like this because it changes error filtering cases.

@nikhiljindal
Copy link
Contributor Author

Fixed by #10246

@bgrant0607 bgrant0607 reopened this Jul 1, 2015
@bgrant0607 bgrant0607 modified the milestones: v1.0-post, v1.0 Jul 6, 2015
@nikhiljindal nikhiljindal mentioned this issue Jul 10, 2015
Closed
@bgrant0607 bgrant0607 removed this from the v1.0-post milestone Jul 24, 2015
@nikhiljindal
Copy link
Contributor Author

Verified that all rest handlers call writeNegotiated() which calls errToAPIStatus:

kubernetes/pkg/apiserver/apiserver.go

Line 423 in 21455ab

status := errToAPIStatus(err)
, so we never return raw etcd errors outside the apiserver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nikhiljindal nikhiljindal

Labels
area/apiserver area/security area/usability kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@smarterclayton @bgrant0607 @nikhiljindal @bgrant0607-nocc