Add HTTP health server (closes #115)

[keeganwitt]

fixes #115

[keeganwitt]

I'm not sure if I did this the right way. We also might want more checks than I've implemented.

[faisal-memon]

Thanks @keeganwitt for submitting this.

[keeganwitt]

Do you think we should wrap the FileWriteStatus in a Health struct so that the top level JSON is an object/map that can contain future health check info?

[faisal-memon]

Do you think we should wrap the FileWriteStatus in a Health struct so that the top level JSON is an object/map that can contain future health check info?

Not mandatory but I think that would make the output look a little nicer.

[keeganwitt]

Do you think we should wrap the FileWriteStatus in a Health struct so that the top level JSON is an object/map that can contain future health check info?

I guess it already is an object, just doesn't have a top level key. So currently looks like

{
    "x509_write_success": true,
    "jwt_write_successes": {
        "a.jwt": true,
        "b.jwt": false
    }
}

But we might want something like this, so then we could add future keys, like a hypothetical socket_connection_failed

{
    "write_successes": {
        "x509_write_success": true,
        "jwt_write_successes": {
            "a.jwt": true,
            "b.jwt": false
        }
    },
    "socket_connection_failed": false
}

[keeganwitt]

Do you think we should wrap the FileWriteStatus in a Health struct so that the top level JSON is an object/map that can contain future health check info?

I guess it already is an object, just doesn't have a top level key. So currently looks like

{
    "x509_write_success": true,
    "jwt_write_successes": {
        "a.jwt": true,
        "b.jwt": false
    }
}

But we might want something like this, so then we could add future keys, like a hypothetical socket_connection_failed

{
    "write_successes": {
        "x509_write_success": true,
        "jwt_write_successes": {
            "a.jwt": true,
            "b.jwt": false
        }
    },
    "socket_connection_failed": false
}

Optionally, we can also wrap it at the top level:

{
    "health": {
        "write_successes": {
            "x509_write_success": true,
            "jwt_write_successes": {
                "a.jwt": true,
                "b.jwt": false
            }
        },
        "socket_connection_failed": false
    }
}

[keeganwitt]

I'm wondering now if the health check should distinguish between "failed to write" and "not yet written". That distinction would be important for a readiness probe.

[keeganwitt]

Here's what the output results look like when it first starts

// live
{
  "status": "OK",
  "health": {
    "file_write_statuses": {
      "x509_write_status": "unwritten",
      "jwt_write_status": {
        "certs": "unwritten"
      }
    }
  }
}

// ready
{
  "status": "Service Unavailable",
  "health": {
    "file_write_statuses": {
      "x509_write_status": "unwritten",
      "jwt_write_status": {
        "certs": "unwritten"
      }
    }
  }
}

[keeganwitt]

Here's what the output results look like when it first starts

// live
{
  "status": "OK",
  "health": {
    "file_write_statuses": {
      "x509_write_status": "unwritten",
      "jwt_write_status": {
        "certs": "unwritten"
      }
    }
  }
}

// ready
{
  "status": "Service Unavailable",
  "health": {
    "file_write_statuses": {
      "x509_write_status": "unwritten",
      "jwt_write_status": {
        "certs": "unwritten"
      }
    }
  }
}

Maybe the status should be "ready/not ready" and "live/not live" instead of an HTTP status. Does that read easier?

[kfox1111]

Hmm....

We may be crossing purposes too much?

Most of the time the check I've seen is pretty simple. http return code and not much more.

For details, they need to be readable as metrics so they can be collected over time, graphed, and alerted on. The formatting here looks more useful to humans then for machine processing.

Not saying that is a bad thing, but maybe we're too focused on it where others skip it?

[faisal-memon]

These two lines can go into startSidecar()

[faisal-memon]

Suggested change

	err = startSidecar(hclConfig, log, spiffeSidecar)
	if err != nil {
	if err = startSidecar(hclConfig, log, spiffeSidecar); err != nil {

[faisal-memon]

@keeganwitt Just two minor comments and then we're good to go. @kfox1111 Valid point. Lets merge this and see how it goes. Can always change the output to be more machine friendly.

[keeganwitt]

Hmm....

We may be crossing purposes too much?

Most of the time the check I've seen is pretty simple. http return code and not much more.

For details, they need to be readable as metrics so they can be collected over time, graphed, and alerted on. The formatting here looks more useful to humans then for machine processing.

Not saying that is a bad thing, but maybe we're too focused on it where others skip it?

I was actually thinking of a human looking at this endpoint. Like if one of the probes fails, you can hit it with your browser and see why. I agree this doesn't replace the need for metrics.

[kfox1111]

I mean, the current output is not directly machine understandable as, while its in a common format, the machine has no way of understanding what keys mean what. So, as is, its only directly understandable by a human.

Someone could code something to make a monitoring system fully parse it and report on it, but that could be done also by just having proper metrics.