uptime-kuma icon indicating copy to clipboard operation
uptime-kuma copied to clipboard

Bugfix: Fix healthcheck port value in healthcheck.js for Kubernetes support

Open DevKyleS opened this issue 1 year ago • 7 comments

Description

Fixes Health Check ERROR failure when running node extra/healthcheck.js on Kubernetes.

  • This corrects healthcheck usage to the variable with the correct value.

This was originally added in v1.18.0 in https://github.com/louislam/uptime-kuma/commit/60e12f4bfa55f10b0f5d90e3a6f87bf94e2cdb96#diff-ee9923109e2377a6084a61753be03a75dd172a53849b6bc8d83a8a55ae76d948R28 and usage on https://github.com/louislam/uptime-kuma/blob/60e12f4bfa55f10b0f5d90e3a6f87bf94e2cdb96/extra/healthcheck.js#L32 where this expects strictly the port number and nothing more.

Impact

  • The issue impacts k8s pods/service named uptime-kuma specifically which ends up overwriting the UPTIME_KUMA_PORT environment value variable

Risk

  • Risk is low since this should not cause breaking changes for other methods to run the service while address a specific scenario by k8s users

Example values

These values came from running export|sort from pod

declare -x UPTIME_KUMA_PORT="tcp://10.43.57.32:3001"
declare -x UPTIME_KUMA_SERVICE_PORT="3001"

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • [x] My code follows the style guidelines of this project
  • [x] I ran ESLint and other linters for modified files
  • [x] I have performed a self-review of my own code and tested it
  • [x] I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • [x] My changes generate no new warnings
  • [ ] My code needed automated testing. I have added them (this is optional task)

DevKyleS avatar Sep 14 '22 03:09 DevKyleS

Might also need to correct hostname to use UPTIME_KUMA_SERVICE_HOST instead of UPTIME_KUMA_HOST as it's undefined for me (to which defaulting to 127.0.0.1 will work)

Env variables from kubernetes pod

export|sort

...
declare -x HOSTNAME="uptime-kuma-666bb7894-m2z8w"
declare -x UPTIME_KUMA_PORT="tcp://10.43.57.32:3001"
declare -x UPTIME_KUMA_PORT_3001_TCP="tcp://10.43.57.32:3001"
declare -x UPTIME_KUMA_PORT_3001_TCP_ADDR="10.43.57.32"
declare -x UPTIME_KUMA_PORT_3001_TCP_PORT="3001"
declare -x UPTIME_KUMA_PORT_3001_TCP_PROTO="tcp"
declare -x UPTIME_KUMA_SERVICE_HOST="10.43.57.32"
declare -x UPTIME_KUMA_SERVICE_PORT="3001"
declare -x UPTIME_KUMA_SERVICE_PORT_HTTP="3001"
...

DevKyleS avatar Sep 14 '22 04:09 DevKyleS

Please noted that k8s is not officially supported by Uptime Kuma. And this pull request is a breaking change.

These environment variables is already using, it cannot be changed. https://github.com/louislam/uptime-kuma/wiki/Environment-Variables

I think it is related to your issue: https://github.com/louislam/uptime-kuma/issues/741

louislam avatar Sep 14 '22 06:09 louislam

Thank you @louislam, I tried the changes recommended in https://github.com/louislam/uptime-kuma/issues/741#issuecomment-945854426 but this didn't solve the issue.

Apologies for the distraction due to breaking change....

To address the breaking changes I re-added the expected variables as documented. This fixes the defect for Kubernetes deployments of the healthcheck port value and hostname.

I don't think we can address the same variables used during server startup are accessible by healthcheck.js. https://github.com/louislam/uptime-kuma/blob/b22176d218afd1337387bfeef9fd3d3c98358bb1/server/server.js#L91-L99

Is this sufficient to add to the next release? What else can be addressed?

Validation

Below I tested the changes in https://github.com/louislam/uptime-kuma/pull/2083/commits/db4b2cd984e9b7dd82e0af5995e7235eb2f67517 and the health check is passing.

# export|sort
...
declare -x UPTIME_KUMA_PORT="tcp://10.43.57.32:3001"
declare -x UPTIME_KUMA_PORT_3001_TCP="tcp://10.43.57.32:3001"
declare -x UPTIME_KUMA_PORT_3001_TCP_ADDR="10.43.57.32"
declare -x UPTIME_KUMA_PORT_3001_TCP_PORT="3001"
declare -x UPTIME_KUMA_PORT_3001_TCP_PROTO="tcp"
declare -x UPTIME_KUMA_SERVICE_HOST="10.43.57.32"
declare -x UPTIME_KUMA_SERVICE_PORT="3001"
declare -x UPTIME_KUMA_SERVICE_PORT_HTTP="3001"

root@uptime-kuma-web-666bb7894-64jnx:/app# node extra/healthcheck.js 
Health Check OK [Res Code: 302]
...

Finally to confirm values I updated the log message to output hostname and port values

console.log(`Health Check OK [Res Code: ${res.statusCode}]` + "\nhostname: " + hostname + "\nport: " + port);

... which resulted in correct behavior where port is a single int value instead of a string like tcp://10.43.57.32:3001 before these changes.

root@uptime-kuma-web-666bb7894-64jnx:/app# node extra/healthcheck.js 
Health Check OK [Res Code: 302]
hostname: 10.43.57.32
port: 3001

I do want to better understand how UPTIME_KUMA_PORT is getting this string-type value by Kubernetes so I'm reading up on https://kubernetes.io/docs/concepts/services-networking/service/#environment-variables

DevKyleS avatar Sep 14 '22 20:09 DevKyleS

interesting, for me the healthcheck.js works just fine in OpenShift 4.10.x (aka Kubernetes 1.23.x).

Currently deployed as 1.18.0 (with the repackaged rootless container found in the same repo):

https://github.com/k3rnelpan1c-dev/uptime-kuma-helm/blob/main/uptime-kuma/templates/statefulSet.yaml#L42-L46

EDIT: Not 1to1 comparable due to the repackaged image and different K8s environment, yet still interesting.

k3rnelpan1c-dev avatar Sep 18 '22 16:09 k3rnelpan1c-dev

I did some search that UPTIME_KUMA_SERVICE_PORT is a dynamic variable <your_service_name>_SERVICE_PORT. Which means if you change you service name, it will be not working.

Also even though I mentioned that K8S is not officially supported by Uptime Kuma, a lot of K8S users is still using it and working properly as far as I know.

And logically, the port number must be 3001 if UPTIME_KUMA_PORT and PORT is not set. You should check what set UPTIME_KUMA_PORT.

I am still thinking your issue is related to #741. Did you read @ikaruswill's reply, because there are two solutions: https://github.com/louislam/uptime-kuma/issues/741#issuecomment-945854426

either rename the service to something else like uptime-kuma-web

disable service links for uptime-kuma by setting enableServiceLinks: false

louislam avatar Oct 02 '22 10:10 louislam

@louislam Thank you for the response, yes that workaround did resolve my issue. I'd like to enhance the handling for this edge case in the code to remediate the problem for all.

And logically, the port number must be 3001 if UPTIME_KUMA_PORT and PORT is not set. You should check what set UPTIME_KUMA_PORT.

Does this PR not correct the code to this logic for deriving the service port number? Or shall I reverse the order or add additional restrictions?

https://github.com/louislam/uptime-kuma/pull/2083/files#diff-ee9923109e2377a6084a61753be03a75dd172a53849b6bc8d83a8a55ae76d948R28

DevKyleS avatar Oct 07 '22 02:10 DevKyleS

I just re-visit this pull request, I just realized that I actually didn't fully understand your issue previously.

I will probably accept your pr, but I recently changed this script into golang for better performance: https://github.com/louislam/uptime-kuma/blob/master/extra/healthcheck.go

And can the script identify the current environment is in K8S? I want it to check in K8s only.

louislam avatar Dec 31 '22 19:12 louislam

Why not use Primary base URL for healthcheck? That would mean more robust, end-to-end check. For example my kuma is hosted behind apache which handles the TLS.

trogper avatar Feb 26 '23 18:02 trogper