cmd: Add health check commands #319

tleef · 2019-12-18T04:05:59Z

Related issue

Proposed changes

Add health check commands to the oathkeeper CLI.

motivation:
Support for Docker HEALTHCHECK

Docker's HEALTHCHECK instruction executes a command e.g. CMD curl -f http://localhost/ || exit 1 in the container to determine the health status of the container. The exit code of the command determines the health status. The possible exit codes are:

0: success - the container is healthy and ready for use
1: unhealthy - the container is not working correctly
2: reserved - do not use this exit code

Rather than using curl (which is not available in the official oathkeeper image) to query the oathkeeper api, we can query the api using oathkeeper's CLI. This has a few advantages:

No additional dependencies e.g. curl
The health check is portable to other environments
The health check can be more rigorous than would otherwise be possible with a simple http tool

See this blog post for more details.

example usage:
In keeping with the conventions already in place by the rules command, the new health command has the following usage

oathkeeper health -e <endpoint> alive for checking liveness
oathkeeper health -e <endpoint> ready for checking readiness

$ oathkeeper health -e http://localhost:4456 ready
{
    "status": "ok"
}

Checklist

I have read the contributing guidelines
I have read the security policy
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation within the code base (if appropriate)
I have documented my changes in the
developer guide (if appropriate)

Further comments

claassistantio · 2019-12-18T04:06:31Z

All committers have signed the CLA.

aeneasr

Awesome! Just some documentation things. Also, I think these command should exit with a status code of 1 if the check fails!

cmd/health.go

cmd/health_alive.go

aeneasr · 2019-12-18T10:57:53Z

cmd/health_alive.go

+var healthAliveCmd = &cobra.Command{
+	Use:   "alive",
+	Short: "Command for checking alive status",
+	Run: func(cmd *cobra.Command, args []string) {


Please add a long description here, with at least one example, showcasing how to use this command.

Also, you should add a warning:

If the target endpoint runs behind a Load Balancer, this command will effectively test the health of the Load Balancer, not the actual ORY Oathkeeper deployment.

I added the Load Balancer warning to the parent health command since that is where the endpoint flag is declared.

Most devs do not read the "top level" command docs, only the specific docs. It makes sense to have the note in all three!

aeneasr · 2019-12-18T10:58:39Z

cmd/health_alive.go

+
+		r, err := client.API.IsInstanceAlive(api.NewIsInstanceAliveParams())
+		cmdx.Must(err, "%s", err)
+		fmt.Println(cmdx.FormatResponse(r.Payload))


I think this command should exit with an error code of 1 if the instance is not yet alive. Any response from the server that has a status code of != 200 means that this instance is not alive yet.

I added explicit exit codes in all cases now. cmdx.Must(err, "%s", err) will exit 1 if there was an error e.g. a 404 response. If no error, it will exit 1 if r.Payload.Status != "ok". Otherwise, exit 0

Ah right, I forgot that the API will return an error if the code is not 200. We should still print out the received status code, which will make it easier to debug :)

The error status code is printed in the error message. For example:

$ oathkeeper health -e http://localhost:4456/bad ready > unknown error (status 404): {resp:0xc0028aee10}

I can't find any way to get a reference to the StatusCode based on the what the internal API client returns.

You can type assert the error with runtime.APIError and then you have access to the Response, code, and so on, so e.g.

if apiErr, ok := err.(*runtime.APIError); ok { // ... } else { // ... }

however, I think it's ok as it is at the moment!

aeneasr · 2019-12-18T11:00:04Z

cmd/health_ready.go

@@ -0,0 +1,26 @@
+package cmd


Please apply the comments from health_alive here as well!

Co-Authored-By: hackerman <[email protected]>

aeneasr

Thank you for the changes, a few last things and then we're good to go! :)

aeneasr · 2019-12-18T13:11:55Z

cmd/health_alive.go

+var healthAliveCmd = &cobra.Command{
+	Use:   "alive",
+	Short: "Command for checking alive status",
+	Run: func(cmd *cobra.Command, args []string) {


Most devs do not read the "top level" command docs, only the specific docs. It makes sense to have the note in all three!

aeneasr · 2019-12-18T13:12:27Z

cmd/health_ready.go

+		if r.Payload.Status != "ok" {
+			os.Exit(1)
+		}
+		os.Exit(0)


No need for os.Exit(0), this is always the case!

I removed the explicit exit 0 for both

aeneasr · 2019-12-18T13:13:40Z

cmd/health_ready.go

+		fmt.Println(cmdx.FormatResponse(r.Payload))
+		// When healthy, ORY Oathkeeper always returns a status of "ok"
+		if r.Payload.Status != "ok" {
+			os.Exit(1)


There should be an error message here. Something like:

fmt.Printf("The endpoint does not appear to be healthy, received HTTP Status Code %d and status %s\n", r.StatusCode, r.Payload.Status)

(I think the status code is r.StatusCode).

The HTTP StatusCode doesn't appear to be available. The internal API client returns the following struct

type IsInstanceReadyOK struct { Payload *models.SwaggerHealthStatus }

If the HTTP status code isn't a 200, it will be printed as an error and the process will exit 1 which is handled by cmdx.Must(err, "%s", err). Otherwise, the Payload is always printed so you can see the payload status

Yeah, you're right!

aeneasr · 2019-12-18T14:06:12Z

Thank you!

tleef · 2019-12-18T14:16:04Z

Thanks for all your help reviewing! Does this mean I get to chat in the #contibutors channel now? ;)

aeneasr · 2019-12-18T14:20:02Z

Credit where credit is due ;) You're now able to write in the contributors channel!

Co-Authored-By: hackerman <[email protected]>

cmd: Add health check commands

a9a870a

tleef added 2 commits December 18, 2019 01:16

Fix lint errors

551549e

Add tests

2651962

aeneasr reviewed Dec 18, 2019

View reviewed changes

tleef and others added 3 commits December 18, 2019 09:06

Update cmd/health.go

5dffcf4

Co-Authored-By: hackerman <[email protected]>

Update cmd/health_alive.go

ec2b2e8

Co-Authored-By: hackerman <[email protected]>

Add usage examples and explicitly control exit codes

1d2f821

aeneasr reviewed Dec 18, 2019

View reviewed changes

Add note to alive/ready subcommands and remove explicity exit 0

84aa65a

aeneasr merged commit 0dd3fe3 into ory:master Dec 18, 2019

tleef deleted the cmd_healthcheck branch December 18, 2019 14:45

pike1212 pushed a commit to pike1212/oathkeeper that referenced this pull request Dec 18, 2019

cmd: Add health check commands (ory#319)

0419d4a

Co-Authored-By: hackerman <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd: Add health check commands #319

cmd: Add health check commands #319

tleef commented Dec 18, 2019 •

edited

Loading

claassistantio commented Dec 18, 2019 •

edited

Loading

aeneasr left a comment

aeneasr Dec 18, 2019

aeneasr Dec 18, 2019

tleef Dec 18, 2019

aeneasr Dec 18, 2019

tleef Dec 18, 2019

aeneasr Dec 18, 2019

tleef Dec 18, 2019

aeneasr Dec 18, 2019

tleef Dec 18, 2019

aeneasr Dec 18, 2019

aeneasr Dec 18, 2019

aeneasr Dec 18, 2019

tleef Dec 18, 2019

aeneasr left a comment

aeneasr Dec 18, 2019

aeneasr Dec 18, 2019

tleef Dec 18, 2019

aeneasr Dec 18, 2019

tleef Dec 18, 2019 •

edited

Loading

aeneasr Dec 18, 2019

aeneasr commented Dec 18, 2019

tleef commented Dec 18, 2019

aeneasr commented Dec 18, 2019

cmd: Add health check commands #319

cmd: Add health check commands #319

Conversation

tleef commented Dec 18, 2019 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

claassistantio commented Dec 18, 2019 • edited Loading

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tleef Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Dec 18, 2019

tleef commented Dec 18, 2019

aeneasr commented Dec 18, 2019

tleef commented Dec 18, 2019 •

edited

Loading

claassistantio commented Dec 18, 2019 •

edited

Loading

tleef Dec 18, 2019 •

edited

Loading