Skip to content
This repository was archived by the owner on Apr 29, 2020. It is now read-only.

Commit 5825242

Browse files
robertabbottrobertabbott
committed
Merge pull request #217 from robertabbott/abbott/fixPanicsInHealth
find*Result in health package handles empty slice inputs
2 parents fd92d5b + d7afb62 commit 5825242

File tree

4 files changed

+169
-58
lines changed

4 files changed

+169
-58
lines changed

‎pkg/health/health.go‎

Lines changed: 96 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package health
22

33
import (
4+
"fmt"
45
"time"
56

67
"github.com/hashicorp/consul/api"
@@ -37,6 +38,14 @@ type Result struct {
3738
Output string
3839
}
3940

41+
type HealthEmpty struct {
42+
Res string
43+
}
44+
45+
func (h *HealthEmpty) Error() string {
46+
return h.Res
47+
}
48+
4049
func (h ConsulHealthChecker) WatchNodeService(nodename string, serviceID string, resultCh chan<- Result, errCh chan<- error, quitCh <-chan struct{}) {
4150
defer close(resultCh)
4251

@@ -53,24 +62,47 @@ func (h ConsulHealthChecker) WatchNodeService(nodename string, serviceID string,
5362
if err != nil {
5463
errCh <- err
5564
}
56-
kvCheck, err := h.consulStore.GetHealth(nodename, serviceID)
57-
if err != nil {
58-
errCh <- err
59-
} else {
60-
catalogResults := make([]Result, 0)
61-
for _, check := range checks {
62-
outResult := consulCheckToResult(*check)
63-
// only retain checks if they're for this service, or for the
64-
// entire node
65-
if outResult.Service != serviceID && outResult.Service != "" {
66-
continue
67-
}
68-
catalogResults = append(catalogResults, outResult)
65+
catalogResults := make([]Result, 0)
66+
for _, check := range checks {
67+
outResult := consulCheckToResult(*check)
68+
// only retain checks if they're for this service, or for the
69+
// entire node
70+
if outResult.Service != serviceID && outResult.Service != "" {
71+
continue
6972
}
70-
kvCheckResult := consulWatchToResult(kvCheck)
71-
catalogCheckResult := findWorstResult(catalogResults)
72-
resultCh <- findBestResult([]Result{kvCheckResult, catalogCheckResult})
73+
catalogResults = append(catalogResults, outResult)
7374
}
75+
// GetHealth will fail if there are no kv results
76+
kvCheck, err := h.consulStore.GetHealth(nodename, serviceID)
77+
pickHealthResult(catalogResults, kvCheck, err, resultCh, errCh)
78+
}
79+
}
80+
}
81+
82+
// if there are no health results available will place error in errCh then return
83+
func pickHealthResult(catalogResults []Result, kvCheck kp.WatchResult, kvCheckError error, resultCh chan<- Result, errCh chan<- error) {
84+
if kvCheckError != nil {
85+
// if there are neither kv nor catalog results
86+
if len(catalogResults) == 0 {
87+
fmt.Println(kvCheckError)
88+
errCh <- kvCheckError
89+
return
90+
} else {
91+
// there are no kv results but there are catalog results
92+
catalogCheckResult, _ := findWorstResult(catalogResults)
93+
resultCh <- catalogCheckResult
94+
}
95+
} else {
96+
// there are kv checks
97+
kvCheckResult := consulWatchToResult(kvCheck)
98+
if len(catalogResults) == 0 {
99+
// there are kv results but not catalog results
100+
resultCh <- kvCheckResult
101+
} else {
102+
// there are both kv and catalog results
103+
catalogCheckResult, _ := findWorstResult(catalogResults)
104+
best, _ := findBestResult([]Result{kvCheckResult, catalogCheckResult})
105+
resultCh <- best
74106
}
75107
}
76108
}
@@ -103,30 +135,37 @@ func (h ConsulHealthChecker) Service(serviceID string) (map[string]Result, error
103135
return nil, util.Errorf("/health/service failed for %q: %s", serviceID, err)
104136
}
105137

106-
return selectResult(catalogEntries, kvEntries)
138+
return selectResult(catalogEntries, kvEntries), nil
107139
}
108140

109-
func selectResult(catalogEntries []*api.ServiceEntry, kvEntries map[string]kp.WatchResult) (map[string]Result, error) {
141+
func selectResult(catalogEntries []*api.ServiceEntry, kvEntries map[string]kp.WatchResult) map[string]Result {
110142
ret := make(map[string]Result)
143+
111144
for _, entry := range catalogEntries {
112145
res := make([]Result, 0, len(entry.Checks))
113146
for _, check := range entry.Checks {
114147
res = append(res, consulCheckToResult(*check))
115148
}
116-
ret[entry.Node.Node] = findWorstResult(res)
149+
val, HEErr := findWorstResult(res)
150+
if HEErr == nil {
151+
ret[entry.Node.Node] = val
152+
}
117153
}
118154

119155
for _, kvEntry := range kvEntries {
120156
res := consulWatchToResult(kvEntry)
121157
// if kvEntry already exists for this service take the best of kv store and catalog
122158
if _, ok := ret[kvEntry.Node]; ok {
123-
ret[kvEntry.Node] = findBestResult([]Result{res, ret[kvEntry.Node]})
159+
val, HEErr := findBestResult([]Result{res, ret[kvEntry.Node]})
160+
if HEErr == nil {
161+
ret[kvEntry.Node] = val
162+
}
124163
} else {
125164
ret[kvEntry.Node] = res
126165
}
127166
}
128167

129-
return ret, nil
168+
return ret
130169
}
131170

132171
func consulCheckToResult(c api.HealthCheck) Result {
@@ -151,17 +190,38 @@ func consulWatchToResult(w kp.WatchResult) Result {
151190

152191
// Returns the poorest status of all checks in the given list, plus the check
153192
// ID of one of those checks.
154-
func FindWorst(results []Result) (string, HealthState) {
155-
worst := findWorstResult(results)
156-
return worst.ID, worst.Status
193+
func FindWorst(results []Result) (string, HealthState, *HealthEmpty) {
194+
if len(results) == 0 {
195+
return "", Critical, &HealthEmpty{
196+
Res: "no results were passed to FindWorst",
197+
}
198+
}
199+
worst, HEErr := findWorstResult(results)
200+
if HEErr != nil {
201+
return "", Critical, HEErr
202+
}
203+
return worst.ID, worst.Status, HEErr
157204
}
158205

159-
func FindBest(results []Result) (string, HealthState) {
160-
best := findBestResult(results)
161-
return best.ID, best.Status
206+
func FindBest(results []Result) (string, HealthState, *HealthEmpty) {
207+
if len(results) == 0 {
208+
return "", Critical, &HealthEmpty{
209+
Res: "no results were passed to FindBest",
210+
}
211+
}
212+
best, HEErr := findBestResult(results)
213+
if HEErr != nil {
214+
return "", Critical, HEErr
215+
}
216+
return best.ID, best.Status, HEErr
162217
}
163218

164-
func findWorstResult(results []Result) Result {
219+
func findWorstResult(results []Result) (Result, *HealthEmpty) {
220+
if len(results) == 0 {
221+
return Result{Status: Critical}, &HealthEmpty{
222+
Res: "no results were passed to findWorstResult",
223+
}
224+
}
165225
ret := Passing
166226
retVal := results[0]
167227
for _, res := range results {
@@ -170,10 +230,15 @@ func findWorstResult(results []Result) Result {
170230
retVal = res
171231
}
172232
}
173-
return retVal
233+
return retVal, nil
174234
}
175235

176-
func findBestResult(results []Result) Result {
236+
func findBestResult(results []Result) (Result, *HealthEmpty) {
237+
if len(results) == 0 {
238+
return Result{Status: Critical}, &HealthEmpty{
239+
Res: "no results were passed to findBestResult",
240+
}
241+
}
177242
ret := Critical
178243
retVal := results[0]
179244
for _, res := range results {
@@ -182,5 +247,5 @@ func findBestResult(results []Result) Result {
182247
retVal = res
183248
}
184249
}
185-
return retVal
250+
return retVal, nil
186251
}

‎pkg/health/health_test.go‎

Lines changed: 58 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,45 @@ import (
99
"github.com/square/p2/pkg/kp"
1010
)
1111

12+
func TestPickHealthResult(t *testing.T) {
13+
catalogResults := []Result{Result{Status: Passing}}
14+
kvCheck := kp.WatchResult{Status: string(Critical)}
15+
errCh := make(chan error)
16+
resultCh := make(chan Result)
17+
18+
go pickHealthResult([]Result{}, kp.WatchResult{}, fmt.Errorf("no kvCheck"), resultCh, errCh)
19+
select {
20+
case _ = <-resultCh:
21+
t.Fatal("pickhealthresult was passed no results but did not return an error")
22+
case err := <-errCh:
23+
Assert(t).AreNotEqual(err, nil, "err should not have been nil")
24+
}
25+
26+
go pickHealthResult([]Result{}, kvCheck, nil, resultCh, errCh)
27+
select {
28+
case res := <-resultCh:
29+
Assert(t).AreEqual(res, Result{Status: Critical}, "pick health result did not return the correct kv result")
30+
case err := <-errCh:
31+
t.Fatal(fmt.Sprintf("pickhealthresult returned an error: %s, but was passed a valid kvCheck", err))
32+
}
33+
34+
go pickHealthResult(catalogResults, kp.WatchResult{}, fmt.Errorf("no kvCheck"), resultCh, errCh)
35+
select {
36+
case res := <-resultCh:
37+
Assert(t).AreEqual(res, Result{Status: Passing}, "pick health result did not return the correct kv result")
38+
case err := <-errCh:
39+
t.Fatal(fmt.Sprintf("pickhealthresult returned an error: %s, but was passed a valid catalogue check", err))
40+
}
41+
42+
go pickHealthResult(catalogResults, kvCheck, nil, resultCh, errCh)
43+
select {
44+
case res := <-resultCh:
45+
Assert(t).AreEqual(res, Result{Status: Passing}, "pick health result did not return the correct kv result")
46+
case err := <-errCh:
47+
t.Fatal(fmt.Sprintf("pickhealthresult returned an error: %s, but was passed a valid checks from both sources", err))
48+
}
49+
}
50+
1251
func TestFindWorst(t *testing.T) {
1352
a := Result{
1453
ID: "testcrit",
@@ -23,14 +62,17 @@ func TestFindWorst(t *testing.T) {
2362
Status: Passing,
2463
}
2564

26-
id, _ := FindWorst([]Result{a, b})
65+
id, _, _ := FindWorst([]Result{a, b})
2766
Assert(t).AreEqual(id, a.ID, "FindWorst between critical and warning should have returned testcrit")
2867

29-
id, _ = FindWorst([]Result{b, c})
68+
id, _, _ = FindWorst([]Result{b, c})
3069
Assert(t).AreEqual(id, b.ID, "FindWorst between warning and passing should have returned testwarn")
3170

32-
id, _ = FindWorst([]Result{c, c})
71+
id, _, _ = FindWorst([]Result{c, c})
3372
Assert(t).AreEqual(id, c.ID, "FindWorst between two passing results should have returned testpass")
73+
74+
id, _, err := FindWorst([]Result{})
75+
Assert(t).AreNotEqual(err, nil, "FindWorst did not return error for empty result slice")
3476
}
3577

3678
func TestPickServiceResult(t *testing.T) {
@@ -39,26 +81,24 @@ func TestPickServiceResult(t *testing.T) {
3981
t3 := mockServiceEntry("3", Passing)
4082

4183
// Catalog is healthy but KV is not present
42-
testMap := getResult([]*api.ServiceEntry{t1, t2, t3})
84+
testMap, err := getResult([]*api.ServiceEntry{t1, t2, t3})
85+
Assert(t).AreEqual(err, nil, "getResult failed")
4386
catalog := []*api.ServiceEntry{t1, t2, t3}
44-
res, err := selectResult(catalog, nil)
45-
Assert(t).AreEqual(err, nil, "catalog is healthy, kv not present and selectResult returned err")
87+
res := selectResult(catalog, nil)
4688
for key, value := range testMap {
4789
Assert(t).AreEqual(res[key], value, "catalog is healthy, kv not present, selectResult did not match what was expected")
4890
}
4991

5092
// Catalog is healthy but KV is not
5193
watchRes := mockWatchResult(catalog, Critical)
52-
res, err = selectResult(catalog, watchRes)
53-
Assert(t).AreEqual(err, nil, "catalog is healthy but kv is not and selectResult returned err")
94+
res = selectResult(catalog, watchRes)
5495
for key, value := range testMap {
5596
Assert(t).AreEqual(res[key], value, "catalog is healthy, kv is not, selectResult did not match what was expected")
5697
}
5798

5899
// KV is healthy but catalog is not present
59100
kv := mockWatchResult(catalog, Passing)
60-
res, err = selectResult(nil, kv)
61-
Assert(t).AreEqual(err, nil, "kv is healthy, catalog not present and selectResult returned err")
101+
res = selectResult(nil, kv)
62102
for key, value := range testMap {
63103
Assert(t).AreEqual(consulWatchToResult(kv[key]), value, "kv is healthy, catalog not present, selectResult did not match what was expected")
64104
}
@@ -69,8 +109,7 @@ func TestPickServiceResult(t *testing.T) {
69109
catalog = []*api.ServiceEntry{t1, t2, t3}
70110

71111
// KV is healthy but catalog is not
72-
res, err = selectResult(catalog, kv)
73-
Assert(t).AreEqual(err, nil, "kv is healthy, catalog is not and selectResult returned err")
112+
res = selectResult(catalog, kv)
74113
for key, value := range testMap {
75114
Assert(t).AreEqual(consulWatchToResult(kv[key]), value, "kv is healthy, catalog is not and selectResult did not match what was expected")
76115
}
@@ -125,17 +164,21 @@ func mockWatchResult(entries []*api.ServiceEntry, st HealthState) map[string]kp.
125164
return ret
126165
}
127166

128-
func getResult(entries []*api.ServiceEntry) map[string]Result {
167+
func getResult(entries []*api.ServiceEntry) (map[string]Result, error) {
168+
var HEErr *HealthEmpty
129169
res := make(map[string]Result)
130170
for _, entry := range entries {
131171
val := make([]Result, 0, len(entry.Checks))
132172
for _, check := range entry.Checks {
133173
val = append(val, consulCheckToResult(*check))
134174
}
135-
res[entry.Node.Node] = findWorstResult(val)
175+
res[entry.Node.Node], HEErr = findWorstResult(val)
176+
if HEErr != nil {
177+
return res, HEErr
178+
}
136179
}
137180

138-
return res
181+
return res, nil
139182
}
140183

141184
func mockServiceEntry(s string, st HealthState) *api.ServiceEntry {

‎pkg/kp/kv.go‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ import (
1515
"github.com/square/p2/pkg/util"
1616
)
1717

18+
// Healthcheck TTL
19+
const TTL = 60 * time.Second
20+
1821
var showConsulErrors = kingpin.Flag(
1922
"show-consul-errors-unsafe",
2023
"Show detailed error messages from Consul (use only when debugging)",
@@ -46,7 +49,7 @@ type WatchResult struct {
4649
Service string
4750
Status string
4851
Output string
49-
Time string
52+
Time time.Time
5053
}
5154

5255
type consulStore struct {
@@ -111,7 +114,10 @@ func (c consulStore) GetHealth(service, node string) (WatchResult, error) {
111114
if err != nil {
112115
return WatchResult{}, KVError{Op: "get", Key: key, UnsafeError: err}
113116
}
114-
117+
stale := isStale(*healthRes)
118+
if stale {
119+
return *healthRes, KVError{Op: "get", Key: key, UnsafeError: fmt.Errorf("stale health entry")}
120+
}
115121
return *healthRes, nil
116122
}
117123

@@ -260,12 +266,8 @@ func (c consulStore) Ping() error {
260266
}
261267

262268
func addTimeStamp(value WatchResult) (time.Time, WatchResult) {
263-
currentTime := time.Now()
264-
if value.Time == "" {
265-
value.Time = currentTime.String()
266-
}
267-
268-
return currentTime, value
269+
value.Time = time.Now()
270+
return value.Time, value
269271
}
270272

271273
func HealthPath(service, node string) string {
@@ -274,3 +276,7 @@ func HealthPath(service, node string) string {
274276
}
275277
return fmt.Sprintf("%s/%s/%s", "health", service, node)
276278
}
279+
280+
func isStale(res WatchResult) bool {
281+
return time.Since(res.Time) > TTL
282+
}

0 commit comments

Comments
 (0)