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

Add a proper / minimal health check endpoint #1933

Closed
colearendt opened this issue Sep 3, 2021 · 12 comments · Fixed by #2092
Closed

Add a proper / minimal health check endpoint #1933

colearendt opened this issue Sep 3, 2021 · 12 comments · Fixed by #2092
Labels
enhancement a feature, ready for implementation

Comments

@colearendt
Copy link
Contributor

Environment

  • PostgreSQL version: v8.0.0
  • PostgREST version: v8.0.0
  • Operating system:

Description of issue

Related to #1565

When running Postgrest in Kubernetes (I have been toying around with a helm chart), there is not the option to use a HEAD request as suggested #1565 . Further, my own personal /rpc/health_check endpoint requires a round-trip to the database. It would be nice if postgrest had its own configurable health endpoint (if the path is configurable, then you do not have to worry about conflicts with user-defined tables/functions).

This is particularly useful when the schema generated by postgrest (and thus the response at /) is large, and we want health checks to be minimal as far as overhead / responsiveness is concerned. This is the case for readiness probes in Kubernetes.

My current health check implementation, which uses an /rpc to return a simple {} and HTTP 200.

BEGIN;

--
-- Name: health_check(); Type: FUNCTION; Schema: public; Owner: dba
--

CREATE OR REPLACE FUNCTION "public"."health_check"() RETURNS jsonb
    LANGUAGE plpgsql
    AS $$
BEGIN

RETURN '{}'::jsonb;

END;
$$;


ALTER FUNCTION "public"."health_check"() OWNER TO dba;
GRANT ALL ON FUNCTION "public"."health_check"() TO dba;
GRANT EXECUTE ON FUNCTION "public"."health_check"() TO PUBLIC;

COMMIT;
@steve-chavez
Copy link
Member

steve-chavez commented Sep 3, 2021

Hm, so Kubernetes doesn't do HEAD requests kubernetes/kubernetes#49937 (don't see any strong objections in the issue, maybe they could add the capability)

if the path is configurable, then you do not have to worry about conflicts with user-defined tables/functions).

Can you add headers for GET requests on Kubernetes? Another option might be supporting a special header at the root / endpoint.

(Related: #1891)

@colearendt
Copy link
Contributor Author

colearendt commented Sep 3, 2021

Thanks for the quick response!! Ahh nice - I had missed the httpHead issue. It would be nice if Kubernetes add that 😞 It's a pity that the issue rotted.

Yes, a header would be possible as well (I am already using this since my health check is not in my default schema 😉 ) - headers are configurable with something like this:

readinessProbe:
  httpGet:
    httpHeaders:
      - name: Accept-Profile
        value: myschema

@steve-chavez
Copy link
Member

Cool. Then we'd need to find a suitable header for this. It could a custom media type like the one we use for a single json object - Accept: application/vnd.pgrst.object+json.

We could also take advantage of this header to do #1526.

@wolfgangwalther
Copy link
Member

Then we'd need to find a suitable header for this. It could a custom media type like the one we use for a single json object - Accept: application/vnd.pgrst.object+json.

What about using Prefer: return=minimal on the root endpoint for that purpose? That sounds more like what we're trying to do here - which is kind of a poor-mans-HEAD-request, right?

@steve-chavez
Copy link
Member

What about using Prefer: return=minimal on the root endpoint for that purpose?

Fully agree. That would solve the issue and also be consistent with what we have now.

@steve-chavez steve-chavez added enhancement a feature, ready for implementation difficulty: beginner Pure Haskell task labels Sep 6, 2021
@steve-chavez
Copy link
Member

Another thing about using HEAD at the root endpoint is that it executes a couple of complex queries to do OpenAPI.

So this minimal root endpoint should execute a simpler query like select 1 to check the connection. Since we have the listener worker running, another option would be to check on AppState to see if we have a healthy connection.

@steve-chavez
Copy link
Member

There was an idea on #1526 (comment) about using a second port.

One alternative would be to use a secondary port to host endpoints for liveness/metrics etc. This would avoid creating a breaking change where we reserve e.g. /internal or a similar base path, and trivially allow users to not expose these endpoints externally.

So we could have a management-port=3001 config that enables an internal management api. The health checks could be reachable at

GET localhost:3001/healthy

This would also let us have a consistent interface for metrics(#1526)

@wolfgangwalther @colearendt WDYT?

@wolfgangwalther
Copy link
Member

So we could have a management-port=3001 config that enables an internal management api. The health checks could be reachable at

How would that work with unix sockets?

Maybe we can just make it a management-endpoint='internal' or similiar config to enable the /internal endpoint, which could then serve all kinds of stuff (/internal/healthy, /internal/metrics, ...)?

Disabled by default, thereby no breaking change and we don't hardcode any endpoint that will then be blocked for regular use.

@steve-chavez
Copy link
Member

How would that work with unix sockets?

Hm, I guess the same - one extra unix socket for the management api, i.e. a config like management-unix-socket.

Maybe we can just make it a management-endpoint='internal

This seems simpler though.

which could then serve all kinds of stuff (/internal/healthy, /internal/metrics, ...)?

Instead of nested routes, maybe they could use query strings, like /internal?select=healthy or /internal?select=metrics. This would be somewhat consistent to our design and perhaps we can reuse some functions.

One downside of the management-endpoint in comparison to the second port/socket is that it's exposed to the public by default. Perhaps we should require a JWT to consume this endpoint?

@steve-chavez
Copy link
Member

How would that work with unix sockets?

Maybe this doesn't matter in this case though, if you can hit the admin unix socket you might as well do a systemctl status postgrest.service. The use case is having an admin interface for other http services like kubernetes/prometheus.

(Down the road, we might get requests to implement these sort of endpoints - reload, quit, ..)

@colearendt
Copy link
Contributor Author

I do like the second port approach, and find that to be consistent with what other services offer, especially with prometheus metrics, etc. Most health-check libraries (Kubernetes, Prometheus, etc.) do not have great support for authentication (or run in a non-interactive context where authentication is challenging), so having the endpoint be unauthenticated is ideal.

Ensuring that the health check is actually tied to the health of the other service / database connection is key too. I have seen applications where the "main" service goes down, but the health check stays up / happy 🙈 😢

@steve-chavez
Copy link
Member

There's now proper health check endpoints on the latest pre-release: https://github.com/PostgREST/postgrest/releases/tag/v9.0.0.20220107

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a feature, ready for implementation
Development

Successfully merging a pull request may close this issue.

3 participants