pkg/server/api: Add /healthz for load-balancer health checks #239

wking · 2018-12-16T07:25:30Z

The server currently 404s the root path. From a master:

[core@ip-10-0-26-134 ~]$ curl -ik https://wking-api.devcluster.openshift.com:49500/
HTTP/1.1 404 Not Found
Content-Type: text/plain; charset=utf-8
X-Content-Type-Options: nosniff
Date: Sun, 16 Dec 2018 06:30:14 GMT
Content-Length: 19

404 page not found

but we need a reliable response on the range 200-399 to satisfy our network load balancer health checks, which do not support configurable response status codes (see here and here, which are Terraform links, but they discuss an AWS restriction that is not Terraform-specific). This pull-request adds a /healthz endpoint which always 204s (when the server is alive to handle it).

Required for openshift/installer#924.

openshift-ci-robot · 2018-12-16T07:25:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wking · 2018-12-16T08:48:02Z

/hold

Using a muxer for this server seems overly complicated. Lets table this change until I have time to drop the muxer, unless other folks can think of a reason to keep it.

abhinavdahiya · 2018-12-16T16:30:19Z

/hold

Using a muxer for this server seems overly complicated. Lets table this change until I have time to drop the muxer, unless other folks can think of a reason to keep it.

Do you mind expanding on why to think it's overcomplicated.

wking · 2018-12-16T16:42:26Z

Do you mind expanding on why to think it's overcomplicated.

Each sub-handler will be stwitching on Path anyway, and we don't have so many paths for the muxer's extra level of path switching to help much. And it's nice to only have to put things like #240's method guard in one spot (vs. repeating in each of the muxer's sub-handlers).

wking · 2018-12-17T05:06:37Z

Rebased onto #242 with 47439f8 -> 9fc7b2b. This will collide with #240, but rebasing either will be easy, so I don't think it matters which lands first.

cgwalters · 2018-12-17T14:53:00Z

LGTM
/assign abhinavdahiya

ashcrow

LGTM as well.

wking · 2018-12-17T19:17:34Z

With #242 closed, I've gone back to a separate sub-handler for /healthz and rebased onto #240 to get unit tests with 9fc7b2b -> 628e654.

abhinavdahiya · 2018-12-17T19:31:05Z

pkg/server/api.go

+ return
+ }
+
+ if r.URL.Path == "" {


what is this check capturing.. ?

also the path is going to be set to /healthz based on setup here 628e654#diff-4bbf1ce644e1b41f1ed61ab858def528R46

Sorry, I'm trying to wrap my head around how muxers work :p. It looks like they pass the full URL through, and aren't stripping off the portion handled by the muxer's switch. Anyhow, don't bother looking at this PR until #240 has landed ;).

i did some local testing and this sheds some light on Path reaginrding how the muxer works.
#242 (comment)

wking · 2018-12-17T21:51:45Z

Ok, I've rebased this onto the new #246 with 628e654 -> 6964b05. #246 gets api_test.go running over the muxer instead of just the API/config handler.

The previous API required consumers to construct a handler and pass that handler back into a separate function to create a server based on the handler. With this commit, I've rerolled the API to return the handler directly, and the new public API is just NewBootstrapServer and NewClusterServer. To serve both HTTP and HTTPS from the returned server, I'm making shallow copies and altering Addr to set the per-protocol ports in those copies. I've also made some "server" -> "config" renames in the types that have getConfig methods. And APIHandler is now configHandler. That helps consolidate the /config/* handling under a consistent name, so we have: * An http.Server that is the server (e.g. as returned by NewClusterServer). * That server's handler is a muxer, and one of the muxer handlers is the configHandler for /config/*. * The configHandler structure holds a getConfig function, and we have bootstrapConfig and clusterConfig which can supply getConfig. Using a function type for this avoids the need for an interface and mock getConfig type. The defaultHandler gives us our usual empty 404s for requests that don't match our other handlers. Without it, we'd have returned Go's default: 404 page not found body, and had unit-test failures like: --- FAIL: TestHandler (0.00s) --- FAIL: TestHandler/GET_/does-not-exist (0.00s) api_test.go:93: response body length 19 does not match Content-Length -1

The server currently 404s the root path. From a master: [core@ip-10-0-26-134 ~]$ curl -ik https://wking-api.devcluster.openshift.com:49500/ HTTP/1.1 404 Not Found Content-Type: text/plain; charset=utf-8 X-Content-Type-Options: nosniff Date: Sun, 16 Dec 2018 06:30:14 GMT Content-Length: 19 404 page not found but we need a reliable response on the range 200-399 to satisfy our network load balancer health checks, which do not support configurable response status codes [1,2] (these are Terraform links, but they discuss an AWS restriction that is not Terraform-specific). This commit adds a /healthz endpoint which always 204s (when the server is alive to handle it). [1]: hashicorp/terraform-provider-aws#2708 (comment) [2]: https://github.com/terraform-providers/terraform-provider-aws/pull/2906/files#diff-375aea487c27a6ada86edfd817ba2401R612

cgwalters · 2018-12-21T20:09:07Z

/hold cancel

abhinavdahiya · 2018-12-21T20:20:55Z

The PR includes commit 6cfa51c

/hold

crawford · 2019-01-04T23:08:49Z

@abhinavdahiya @wking What still needs to happen with this PR?

wking · 2019-01-04T23:23:47Z

What still needs to happen with this PR?

We need to sort out the underlying #246, or figure out another way to get unit tests on the full muxed handler.

abhinavdahiya · 2019-01-05T00:22:19Z

@abhinavdahiya @wking What still needs to happen with this PR?

this is what i would have expected it to look like without #246

wking · 2019-01-05T00:58:00Z

this is what i would have expected it to look like without #246

That looks more complicated to me:

$ git show --oneline --stat 50f1439777
50f1439 pkg/server/api: Add /healthz for load-balancer health checks
 pkg/server/api.go      | 21 +++++++++++++
 pkg/server/api_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
$ git show --oneline --stat d0a7ae21b0
d0a7ae2 add healthz
 pkg/server/api.go      |  47 ++++++++--
 pkg/server/api_test.go | 243 ++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 269 insertions(+), 21 deletions(-)

But, shrug, you've put more work into this repo than me. Want to file a PR?

abhinavdahiya · 2019-01-05T01:09:15Z

this is what i would have expected it to look like without #246

That looks more complicated to me:

Hmm, based on what the lines changed?

$ git show --oneline --stat 50f1439777
50f1439 pkg/server/api: Add /healthz for load-balancer health checks
 pkg/server/api.go      | 21 +++++++++++++
 pkg/server/api_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
$ git show --oneline --stat d0a7ae21b0
d0a7ae2 add healthz
 pkg/server/api.go      |  47 ++++++++--

this is because 50f1439 carries over defaultHandler from 6cfa51c

pkg/server/api_test.go | 243 ++++++++++++++++++++++++++++++++++++++++++++++---

this is because this has 2 types of tests

test for each handler api, default, healthz
test for server ie the muxer that @wking asked here pkg/server/api: Add /healthz for load-balancer health checks #239 (comment)

2 files changed, 269 insertions(+), 21 deletions(-)
But, _shrug_, you've put more work into this repo than me. Want to file a PR?

#267

wking · 2019-01-05T01:43:20Z

Hmm, based on what the lines changed?

Yeah, and also the package API.

this is because 50f1439 carries over defaultHandler from 6cfa51c

But 6cfa51c as a whole is a net reduction of 40 lines.

this is because this has 2 types of tests

With comprehensive muxer tests, I don't think we need sub-handler tests.

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 16, 2018

openshift-ci-robot requested review from ashcrow and crawford December 16, 2018 07:25

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2018

wking force-pushed the server-healthz branch from fa4657b to c0e5a4f Compare December 16, 2018 08:19

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 16, 2018

wking force-pushed the server-healthz branch from c0e5a4f to 47439f8 Compare December 16, 2018 08:29

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2018

wking mentioned this pull request Dec 16, 2018

pkg/server/api: Return 405s for non-GET requests #240

Merged

wking force-pushed the server-healthz branch from 47439f8 to 9fc7b2b Compare December 17, 2018 05:05

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 17, 2018

openshift-ci-robot assigned abhinavdahiya Dec 17, 2018

ashcrow approved these changes Dec 17, 2018

View reviewed changes

wking force-pushed the server-healthz branch from 9fc7b2b to 628e654 Compare December 17, 2018 19:16

abhinavdahiya reviewed Dec 17, 2018

View reviewed changes

wking force-pushed the server-healthz branch from 628e654 to 6964b05 Compare December 17, 2018 21:49

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 17, 2018

wking force-pushed the server-healthz branch 2 times, most recently from 4dc0a08 to 144b903 Compare December 17, 2018 23:21

wking force-pushed the server-healthz branch 2 times, most recently from 0f57e46 to d3fa0ad Compare December 18, 2018 05:37

wking added 2 commits December 18, 2018 23:19

wking force-pushed the server-healthz branch from d3fa0ad to 50f1439 Compare December 19, 2018 07:26

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 21, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 21, 2018

wking mentioned this pull request Jan 4, 2019

data/aws/vpc: Use HTTPS for load balancer health checks openshift/installer#924

Merged

abhinavdahiya mentioned this pull request Jan 5, 2019

server: Add /healthz #267

Merged

openshift-merge-robot closed this in #267 Jan 8, 2019

wking mentioned this pull request Jan 16, 2019

pkg/server: Trim public API to return *http.Server #246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/server/api: Add /healthz for load-balancer health checks #239

pkg/server/api: Add /healthz for load-balancer health checks #239

wking commented Dec 16, 2018

openshift-ci-robot commented Dec 16, 2018

wking commented Dec 16, 2018

abhinavdahiya commented Dec 16, 2018

wking commented Dec 16, 2018

wking commented Dec 17, 2018

cgwalters commented Dec 17, 2018

ashcrow left a comment

wking commented Dec 17, 2018

abhinavdahiya Dec 17, 2018

abhinavdahiya Dec 17, 2018

wking Dec 17, 2018

abhinavdahiya Dec 17, 2018

wking commented Dec 17, 2018

cgwalters commented Dec 21, 2018

abhinavdahiya commented Dec 21, 2018

crawford commented Jan 4, 2019

wking commented Jan 4, 2019

abhinavdahiya commented Jan 5, 2019 •

edited

Loading

wking commented Jan 5, 2019

abhinavdahiya commented Jan 5, 2019

wking commented Jan 5, 2019

pkg/server/api: Add /healthz for load-balancer health checks #239

pkg/server/api: Add /healthz for load-balancer health checks #239

Conversation

wking commented Dec 16, 2018

openshift-ci-robot commented Dec 16, 2018

wking commented Dec 16, 2018

abhinavdahiya commented Dec 16, 2018

wking commented Dec 16, 2018

wking commented Dec 17, 2018

cgwalters commented Dec 17, 2018

ashcrow left a comment

Choose a reason for hiding this comment

wking commented Dec 17, 2018

abhinavdahiya Dec 17, 2018

Choose a reason for hiding this comment

abhinavdahiya Dec 17, 2018

Choose a reason for hiding this comment

wking Dec 17, 2018

Choose a reason for hiding this comment

abhinavdahiya Dec 17, 2018

Choose a reason for hiding this comment

wking commented Dec 17, 2018

cgwalters commented Dec 21, 2018

abhinavdahiya commented Dec 21, 2018

crawford commented Jan 4, 2019

wking commented Jan 4, 2019

abhinavdahiya commented Jan 5, 2019 • edited Loading

wking commented Jan 5, 2019

abhinavdahiya commented Jan 5, 2019

wking commented Jan 5, 2019

abhinavdahiya commented Jan 5, 2019 •

edited

Loading