*: fix server panic on invalid Election Proclaim/Resign HTTP requests #9379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gyuho
force-pushed
the
fix-election
branch
2 times, most recently
from
370b386
to
b928829
Compare
gyuho
changed the title
xiang90
reviewed
Mar 1, 2018
|
|
||
| "github.com/coreos/etcd/clientv3" | ||
| "github.com/coreos/etcd/clientv3/concurrency" | ||
| epb "github.com/coreos/etcd/etcdserver/api/v3election/v3electionpb" | ||
| ) | ||
|
|
||
| // ErrMissingLeaderField is returned when election API is | ||
| // missing "leader" field. | ||
| var ErrMissingLeaderField = errors.New(`missing "leader" field`) |
There was a problem hiding this comment.
ErrMissingLeaderKey
LeaderKey is the user facing API. Check the actual proto in v3electionpb to verify?
There was a problem hiding this comment.
@xiang90 Right, we define as LeaderKey in proto. Just fixed.
Thanks!
|
lgtm in general |
xiang90
reviewed
Mar 1, 2018
|
|
||
| "github.com/coreos/etcd/clientv3" | ||
| "github.com/coreos/etcd/clientv3/concurrency" | ||
| epb "github.com/coreos/etcd/etcdserver/api/v3election/v3electionpb" | ||
| ) | ||
|
|
||
| // ErrMissingLeaderKey is returned when election API is | ||
| // missing "leader" field. | ||
| var ErrMissingLeaderKey = errors.New(`missing "leader" field`) |
There was a problem hiding this comment.
the error message also needs to be fixed.
There was a problem hiding this comment.
Oh I meant,
type ProclaimRequest struct {
// leader is the leadership hold on the election.
Leader *LeaderKey `protobuf:"bytes,1,opt,name=leader" json:"leader,omitempty"`
// value is an update meant to overwrite the leader's current value.
Value []byte `protobuf:"bytes,2,opt,name=value,proto3" json:"value,omitempty"`
}So, the JSON field is still "leader"
There was a problem hiding this comment.
the error message can be more descriptive like leader field must be provided
|
lgtm |
fanminshi
reviewed
Mar 2, 2018
|
|
||
| "github.com/coreos/etcd/clientv3" | ||
| "github.com/coreos/etcd/clientv3/concurrency" | ||
| epb "github.com/coreos/etcd/etcdserver/api/v3election/v3electionpb" | ||
| ) | ||
|
|
||
| // ErrMissingLeaderKey is returned when election API request | ||
| // is missing "leader" field. |
There was a problem hiding this comment.
missing "leader" field. -> missing the "leader" field.?
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
|
lgtm |
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
gyuho
added a commit
that referenced
this pull request
Mar 8, 2018
…rigin-release-3.2 Automated cherry pick of #9379
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Server should never panic from wrong-formatted client requests. Without this patch,
/v3/election/proclaimrequest withleaderfield missing can panic etcd servers.@jpbetz This needs 3.2 backport as well.