Allow overriding the port used for healthchecks #1567
Conversation
healthcheck/healthcheck.go
Outdated
| } | ||
| u.Path = u.Path + backend.Path | ||
|
|
||
| req := &http.Request{ |
There was a problem hiding this comment.
I'm inclined to say we should just call http.NewRequest to be on the safe side, especially if more fields are added over time that might be important for us but would likely be left behind.
WDYT?
There was a problem hiding this comment.
It seems inefficient to me to start with a URL, convert it to a string, and then have NewRequest parse it back into a URL. In this code path, it probably doesn't matter. I can change to NewRequest.
There was a problem hiding this comment.
(I saw you made the change.)
Yeah it seems a bit cumbersome; personally though, I'd like to err on the side of safety and stability.
healthcheck/healthcheck_test.go
Outdated
| Path: "/path", | ||
| Interval: healthCheckInterval, | ||
| LB: lb, | ||
| for _, withPort := range []bool{false, true} { |
There was a problem hiding this comment.
I don't think we need to test all permutations here. I suggest we write a test for the newly created newRequest method instead where we won't need to jump through as many hoops (as we do in this test).
There was a problem hiding this comment.
That makes sense. I'll just write a test for newRequest.
|
@timoreimann I've made the requested changes. PTAL when you have a chance. Thanks for the feedback! |
healthcheck/healthcheck.go
Outdated
| resp, err := client.Get(serverURL.String() + backend.Path) | ||
| req, err := backend.newRequest(serverURL) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Could you please add a log message here. I'm inclined to say it should be of level ERROR since we should apparently only run into this case if we have a logic error in our code.
healthcheck/healthcheck_test.go
Outdated
| } | ||
|
|
||
| for _, test := range tests { | ||
| backend := NewBackendHealthCheck( |
There was a problem hiding this comment.
Please use parallelized sub-tests here. See one of the existing tests for what needs to be changed for this (in a nutshell:
test := testt.Run(...)t.Parallel()
)
There was a problem hiding this comment.
Sure. I wasn't sure it was worth the effort, but I can make it match the others.
healthcheck/healthcheck_test.go
Outdated
| } | ||
|
|
||
| req, err := backend.newRequest(u) | ||
|
|
There was a problem hiding this comment.
Nit-pick: I'd prefer no blank line here.
healthcheck/healthcheck_test.go
Outdated
| req, err := backend.newRequest(u) | ||
|
|
||
| if err != nil { | ||
| t.Errorf("failed to create new backend request for %s", u.String()) |
There was a problem hiding this comment.
Once we use sub-tests, it's safe (and appropriate) to do t.Fatalf here.
healthcheck/healthcheck_test.go
Outdated
| } | ||
|
|
||
| actual := req.URL.String() | ||
| if test.expected != actual { |
There was a problem hiding this comment.
Nit-pick: Put actual on the left-hand and expected on the right-hand side to match the order in the error message on the following line.
healthcheck/healthcheck_test.go
Outdated
| req, err := backend.newRequest(u) | ||
|
|
||
| if err != nil { | ||
| t.Errorf("failed to create new backend request for %s", u.String()) |
There was a problem hiding this comment.
Please include all test case-identifying input parameters and the error message in the output, e.g.:
"URL=%s/%s port=%d: failed to create new backend request: %s", u.String(), test.path, test.port, err)
healthcheck/healthcheck_test.go
Outdated
| req, err := backend.newRequest(u) | ||
|
|
||
| if err != nil { | ||
| t.Errorf("failed to create new backend request for %s", u.String()) |
There was a problem hiding this comment.
Do we need the .String() part in u.String()? Is the formatter possibly "smart enough" to figure it out from u alone?
healthcheck/healthcheck_test.go
Outdated
|
|
||
| actual := req.URL.String() | ||
| if test.expected != actual { | ||
| t.Errorf("got %s for healthcheck URL, wanted %s", actual, test.expected) |
There was a problem hiding this comment.
Include test-case identifiers here too.
Since we need it at two spots, it seems reasonable to me to extract them into a variable.
healthcheck/healthcheck_test.go
Outdated
|
|
||
| req, err := backend.newRequest(u) | ||
| testCase := fmt.Sprintf("URL=%s/%s port=%d", u, test.path, test.port) |
There was a problem hiding this comment.
Now that you've added a description to each test case and injected that into the t.Run() call, we don't need the prefix anymore. :-)
Would you mind removing just that part again?
|
@timoreimann changes made. Let me know when it looks okay and I can squash and repush, if you want. |
|
@timoreimann it's all in one commit now. Not the normal way I'd squash things, but it's done :) |
|
@bakins I actually like my commits squashed to atomic scale. ;-) @emilevauge @vdemeester your review turn. |
docs/basics.md
Outdated
| @@ -278,7 +278,8 @@ as long as it keeps returning HTTP status codes other than 200 OK to HTTP GET | |||
| requests periodically carried out by Traefik. The check is defined by a path | |||
| appended to the backend URL and an interval (given in a format understood by [time.ParseDuration](https://golang.org/pkg/time/#ParseDuration)) specifying how | |||
| often the health check should be executed (the default being 30 seconds). Each | |||
| backend must respond to the health check within 5 seconds. | |||
| backend must respond to the health check within 5 seconds. By default, the port | |||
| of the backend sever is used, however, this may be overridden. | |||
Some services use a different port for auxiliary functions such as healthchecks and metrics. Traefik itself does this. This PR allows overriding the port used per backend.