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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(health): fix http status code for health endpoints #6648

Merged
merged 1 commit into from Oct 26, 2020
Merged

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Oct 25, 2020

Fixes #6647

Signed-off-by: Raymond Feng enjoyjava@gmail.com

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

* @param status - Health status
* @param response - Http response
*/
function handleStatus(status: HealthStatus, response: Response) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raymondfeng this might be a bit picky but in general it would make most sense to return a 500 for failing live checks and 503 for failing ready.

503 --> temporary state --> ready fail --> stop routing traffic (temporary)
500 --> something went wrong --> live fail --> restart service

that being said it also assumes that the consumer of the module properly implements the custom checks and for example does not check for dependencies in live checks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think k8s treats 500 and 503 differently for health checks. But regardless, I improved to code to return 503/500 respectively.

Fixes #6647

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 LGTM.
Probably add more test cases to pass the coverage check.

@raymondfeng raymondfeng merged commit fdca8c0 into master Oct 26, 2020
1 of 2 checks passed
@raymondfeng raymondfeng deleted the fix-6647 branch October 26, 2020 21:44
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.

health extension returns 200 even if custom live or ready check fails
3 participants