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

fix: change hasura health check url to make auth server work in gcp #565

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

itsmurugappan
Copy link
Contributor

@itsmurugappan itsmurugappan commented Sep 22, 2024

Google cloud doesnt support "z" urls - https://stackoverflow.com/questions/43380939/where-does-the-convention-of-using-healthz-for-application-health-checks-come-f

And hasura provides an alternate end point for this https://hasura.io/docs/2.0/api-reference/health/ .

Checklist

  • No breaking changes
  • Tests pass
  • New features have new tests
  • Documentation is updated

@dbarrosop
Copy link
Member

dbarrosop commented Sep 23, 2024

Hi, thanks for the PR.

Google cloud doesnt support "z" urls - https://stackoverflow.com/questions/43380939/where-does-the-convention-of-using-healthz-for-application-health-checks-come-f

Where does it say that? I couldn't find any reference to it. Do you have any documentation we can reference?

Regarding the change, unfortunately, it isn't good (and doesn't work) for two main reasons:

  1. It is done on the javascript side, which isn't used (we are transitioning to go) so this change basically has no impact.
  2. It is a breaking change as changing the healthz path will break every single deployment

To properly perform this change you will have to:

  1. Add a second healthz endpoint here. Do not change the existing one as any PR modififying it will not be accepted. Adding a second one should be fine.
  2. After you have edited the openapi.yaml you can run go generate ./... to generate some stubs and boilerplate.
  3. You will have to implement the new method added to the server interface (which can just be an alias to this.

Also, don't call the endpoint /hasura/healthz. If the issue is that the endpoint is called healthz we can just add a second one called /health (without the z)

README.md Outdated Show resolved Hide resolved
@xmlking
Copy link
Contributor

xmlking commented Sep 23, 2024

Where does it say that? I couldn't find any reference to it. Do you have any documentation we can reference?

Workloads in Google Cloud cannot have URL paths with /healthz . here is are some references from stackoverflow

here is the proof (hasrua hosted in google Cloud Run)

image

When hasura-auth container starts, it use this code to check if hasura is UP and healthy. without this check pass, hasura-auth in GCP will not start at all.

I think there is some misunderstanding here:
@itsmurugappan is asking to use Hasura's second health endpoint /hasura/healthz instead of /healthz

Important

Hasura include both health-check endpoints in all deployment environments. we don't have to do anything for hasura.

I think this change will not have any side-effect

@xmlking
Copy link
Contributor

xmlking commented Sep 23, 2024

hasura docs
image

@dbarrosop
Copy link
Member

yes, you are completely right. I totally misinterpreted the change. Thanks for the clarification.

However, if you need this change because you are hosting hasura in GCP, wouldn't you also need an alternative /health endpoint for hasura-auth?

@itsmurugappan
Copy link
Contributor Author

since hasura auth health check end point is not called through the loadbalancer by any client we dint need it.

@xmlking
Copy link
Contributor

xmlking commented Sep 24, 2024

yes, you are completely right. I totally misinterpreted the change. Thanks for the clarification.

However, if you need this change because you are hosting hasura in GCP, wouldn't you also need an alternative /health endpoint for hasura-auth?

Would be nice to have alternative health-check endpoints for all nhost services that won't conflict with GCP.
But as minimum viable solution, this PR is good enough making it work in GCP.

@dbarrosop dbarrosop merged commit 91bb815 into nhost:main Sep 25, 2024
11 checks passed
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 this pull request may close these issues.

3 participants