Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reintroduce health liveliness check #1861

Closed
abernix opened this issue Sep 21, 2022 · 1 comment · Fixed by #1859
Closed

reintroduce health liveliness check #1861

abernix opened this issue Sep 21, 2022 · 1 comment · Fixed by #1859
Assignees

Comments

@abernix
Copy link
Member

abernix commented Sep 21, 2022

We removed the previous health check as a requirement of #1765 and finished that work in #1766, with full-intention of reintroducing it when we had a bit more time to design its features, ports, etc. Some of this was caught up with socket address listener time-constraints in getting the 1.0.0-rc.0 out the door with a desire to limit breaking changes.

We're still going to need some time to design a more fully developed health check, but a couple things are immediately true about the suggestion to use a basic GET GraphQL request as the health check:

  • Not all utilities that send HTTP requests actually support sending HTTP headers, as we note is required in our health check documentation

    While not the immediately identified hurdle in that issue, this is very likely to become the next hurdle a user using AWS ALB health checks will encounter in New healthcheck path is not compatible with AWS ALB #1852 after addressing the initial challenge they identified (URL encoding). We fully believe this is going to be a challenge for other non-health-check tools, but as of today, health checks are a pressing need.

  • Providing these headers (or at least some headers) is required to satisfy the CSRF protection introduced in feat: CSRF plugin #1006 (which matches the implementation in Apollo Server), even on GET requests.

We'll need to reintroduce a simple health check for now and build out that functionality to enable users to integrate with existing tools without as much friction as they're currently encountering. In the future, we want to consider the impact of CSRF protections on GET requests, but we shouldn't make rapid moves here without consulting with our security focused partners.

For now, let's bring back a liveliness check

@glasser
Copy link
Member

glasser commented Sep 21, 2022

Just as a note in support of this: one reason that the Apollo Server non-GraphQL health check felt problematic was that it wasn't actually coordinated with the server's ability to server GraphQL results — for example, it was still healthy even if the server had not properly loaded a schema. I would expect that in the context of Router it would be easier to ensure that the health check checks for more than just "is the process running" and could be based on that as well (though I suspect Router probably doesn't start listening at all without a schema?)

BrynCooke pushed a commit that referenced this issue Sep 22, 2022
closes #1861
fixes #1852


### Reintroduce health liveliness check ([Issue
#1861](#1861))

Depending on their environments and cloud settings, users may or may not
be able to craft health probes that are able to make CSRF compatible
GraphQL queries.
This is one of the reasons why we reintroduced a health check in the
router.

The health liveness check endpoint is exposed on
`127.0.0.1:8088/health`, and its listen address can be changed in the
yaml configuration:

```yaml
health-check:
  listen: 127.0.0.1:8088 # default
  enabled: true # default
```

Co-authored-by: bryn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants