You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "dmunozv04 (via GitHub)" <gi...@apache.org> on 2025/09/30 17:07:09 UTC
[PR] Guacamole 2086: Add Tomcat HealthCheckValve [guacamole-client]
dmunozv04 opened a new pull request, #1119:
URL: https://github.com/apache/guacamole-client/pull/1119
This PR allows to set Tomcat's HealthCheckValve with environment variables. The implementation is taken from the RemoteIpValve script.
The second commit uses these new variables to configure docker's HEALTHCHECK. This could be removed if it shouldn't be enabled by default.
I'm happy to make any changes necessary. Thanks for reviewing this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "dmunozv04 (via GitHub)" <gi...@apache.org>.
dmunozv04 commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2401853889
##########
Dockerfile:
##########
@@ -99,8 +99,11 @@ USER guacamole
# Environment variable defaults
ENV BAN_ENABLED=true \
ENABLE_FILE_ENVIRONMENT_PROPERTIES=true \
- GUACAMOLE_HOME=/etc/guacamole
+ GUACAMOLE_HOME=/etc/guacamole \
+ HEALTH_CHECK_VALVE_ENABLED=true
# Start Guacamole under Tomcat, listening on 0.0.0.0:8080
EXPOSE 8080
+HEALTHCHECK --interval=60s --timeout=10s --start-period=30s \
+ CMD curl --fail --silent --show-error http://localhost:8080/health || exit 1
Review Comment:
Docker will show the container as `Up About a minute (health: starting)` and after the start-period it'll show it as `Up 3 minutes (unhealthy)`
This health status is just a visual indication, but will not cause the container to fail.
However, it looks like docker swarm will attempt to restart an unhealthy container to try to get it running ([source](https://statusq.org/archives/2022/02/01/10830/)), and [Podman](https://www.redhat.com/en/blog/podman-edge-healthcheck) can be configured to restart, stop or kill an unhealthy container.
Kubernetes just ignores this HEALTHCHECK and relies on its own [probes](https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/)
Considering this, and the low impact of having an http route that just replies with `200 OK`, maybe it could be better to always force the health check to enabled or not have it set on the Dockerfile like it's been until now. I don't know what option is better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "dmunozv04 (via GitHub)" <gi...@apache.org>.
dmunozv04 commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2422981924
##########
guacamole-docker/bin/healthcheck.sh:
##########
@@ -0,0 +1,40 @@
+#!/bin/bash -e
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+##
+## @fn healthcheck.sh
+##
+## Performs a health check for the Guacamole container. If HEALTH_CHECK_VALVE_ENABLED
+## is set to "true", this script will check the actual health endpoint. Otherwise,
+## it will simply return success and log that health checks are disabled.
+##
+
+
+# Check if health check valve is enabled
+if [ "$HEALTH_CHECK_VALVE_ENABLED" = "true" ]; then
+ # Default health check path (/health)
+ HEALTH_CHECK_PATH="${HEALTH_CHECK_VALVE_PATH:-/health}"
+ # Perform actual health check via curl
+ curl --fail --silent --show-error "http://localhost:8080${HEALTH_CHECK_PATH}" || exit 1
Review Comment:
I assumed an absolute path would be provided since it looks like tomcat only supports absolute paths:
https://github.com/apache/tomcat/blob/eaea7503163e44ba7260c9c7b3af3f4ba387d072/java/org/apache/catalina/valves/HealthCheckValve.java#L82
If the user provides a relative path, the health check won't be reachable and curl will complain: `"curl: (3) URL rejected: Port number was not a decimal number between 0 and 65535\n"`
Maybe a check can be added when loading the variables to warn the user if the path is not absolute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2402569249
##########
Dockerfile:
##########
@@ -99,8 +99,11 @@ USER guacamole
# Environment variable defaults
ENV BAN_ENABLED=true \
ENABLE_FILE_ENVIRONMENT_PROPERTIES=true \
- GUACAMOLE_HOME=/etc/guacamole
+ GUACAMOLE_HOME=/etc/guacamole \
+ HEALTH_CHECK_VALVE_ENABLED=true
# Start Guacamole under Tomcat, listening on 0.0.0.0:8080
EXPOSE 8080
+HEALTHCHECK --interval=60s --timeout=10s --start-period=30s \
+ CMD curl --fail --silent --show-error http://localhost:8080/health || exit 1
Review Comment:
Hmmm....I'm not sure about adding something like this that is known to cause problems. I also did a quick look to see if it's possible to do a conditional `HEALTHCHECK`, and that didn't turn up anything obvious. Maybe in the `|| exit 1` portion a conditional statement could be put to check for the status of the the variable that enables or disables the health check?
Alternatively, I think maybe the way to go is just not to make it configurable and simply enable the Tomcat `HealthCheckValve` always, as you suggest.
@mike-jumper Thoughts or preferences?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2404976531
##########
Dockerfile:
##########
@@ -99,8 +99,11 @@ USER guacamole
# Environment variable defaults
ENV BAN_ENABLED=true \
ENABLE_FILE_ENVIRONMENT_PROPERTIES=true \
- GUACAMOLE_HOME=/etc/guacamole
+ GUACAMOLE_HOME=/etc/guacamole \
+ HEALTH_CHECK_VALVE_ENABLED=true
# Start Guacamole under Tomcat, listening on 0.0.0.0:8080
EXPOSE 8080
+HEALTHCHECK --interval=60s --timeout=10s --start-period=30s \
+ CMD curl --fail --silent --show-error http://localhost:8080/health || exit 1
Review Comment:
We could move the healthcheck implementation into a script and run _that_ instead of directly running `curl`:
```Dockerfile
HEALTHCHECK --interval=60s --timeout=10s --start-period=30s \
CMD [ "/opt/guacamole/bin/healthcheck.sh" ]
```
That would allow the script to deal with pinging `http://localhost:8080/health` only if `HEALTH_CHECK_VALVE_ENABLED` is `true`, simply succeeding if not. It would also avoid invoking `curl` unnecessarily, and would give a future point of expansion if we implement additional checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "dmunozv04 (via GitHub)" <gi...@apache.org>.
dmunozv04 commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2422787205
##########
Dockerfile:
##########
@@ -99,8 +99,11 @@ USER guacamole
# Environment variable defaults
ENV BAN_ENABLED=true \
ENABLE_FILE_ENVIRONMENT_PROPERTIES=true \
- GUACAMOLE_HOME=/etc/guacamole
+ GUACAMOLE_HOME=/etc/guacamole \
+ HEALTH_CHECK_VALVE_ENABLED=true
# Start Guacamole under Tomcat, listening on 0.0.0.0:8080
EXPOSE 8080
+HEALTHCHECK --interval=60s --timeout=10s --start-period=30s \
+ CMD curl --fail --silent --show-error http://localhost:8080/health || exit 1
Review Comment:
Sounds good to me, I'll implement it soon.
This way the health check will also work if a user changes the `HEALTH_CHECK_VALVE_PATH` variable to something different than `/health`
Thanks for taking your time to review this PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "dmunozv04 (via GitHub)" <gi...@apache.org>.
dmunozv04 commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2422828365
##########
Dockerfile:
##########
@@ -99,8 +99,11 @@ USER guacamole
# Environment variable defaults
ENV BAN_ENABLED=true \
ENABLE_FILE_ENVIRONMENT_PROPERTIES=true \
- GUACAMOLE_HOME=/etc/guacamole
+ GUACAMOLE_HOME=/etc/guacamole \
+ HEALTH_CHECK_VALVE_ENABLED=true
# Start Guacamole under Tomcat, listening on 0.0.0.0:8080
EXPOSE 8080
+HEALTHCHECK --interval=60s --timeout=10s --start-period=30s \
+ CMD curl --fail --silent --show-error http://localhost:8080/health || exit 1
Review Comment:
I added the script, let me know if you want any other changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2422968073
##########
guacamole-docker/bin/healthcheck.sh:
##########
@@ -0,0 +1,40 @@
+#!/bin/bash -e
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+##
+## @fn healthcheck.sh
+##
+## Performs a health check for the Guacamole container. If HEALTH_CHECK_VALVE_ENABLED
+## is set to "true", this script will check the actual health endpoint. Otherwise,
+## it will simply return success and log that health checks are disabled.
+##
+
+
+# Check if health check valve is enabled
+if [ "$HEALTH_CHECK_VALVE_ENABLED" = "true" ]; then
+ # Default health check path (/health)
+ HEALTH_CHECK_PATH="${HEALTH_CHECK_VALVE_PATH:-/health}"
+ # Perform actual health check via curl
+ curl --fail --silent --show-error "http://localhost:8080${HEALTH_CHECK_PATH}" || exit 1
Review Comment:
A couple of concerns about the `HEALTH_CHECK_VALVE_PATH` variable:
* It looks like it assumes that an absolute path will be provided?
* What happens if the user provides a relative path (no leading /), both here and in the server.xml file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "dmunozv04 (via GitHub)" <gi...@apache.org>.
dmunozv04 commented on PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#issuecomment-3353251319
Fixed, sorry for that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2398667673
##########
Dockerfile:
##########
@@ -99,8 +99,11 @@ USER guacamole
# Environment variable defaults
ENV BAN_ENABLED=true \
ENABLE_FILE_ENVIRONMENT_PROPERTIES=true \
- GUACAMOLE_HOME=/etc/guacamole
+ GUACAMOLE_HOME=/etc/guacamole \
+ HEALTH_CHECK_VALVE_ENABLED=true
# Start Guacamole under Tomcat, listening on 0.0.0.0:8080
EXPOSE 8080
+HEALTHCHECK --interval=60s --timeout=10s --start-period=30s \
+ CMD curl --fail --silent --show-error http://localhost:8080/health || exit 1
Review Comment:
What happens if the user sets the `HEALTH_CHECK_VALVE_ENABLED` property to `false`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2086: Add Tomcat HealthCheckValve [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #1119:
URL: https://github.com/apache/guacamole-client/pull/1119#discussion_r2426206761
##########
guacamole-docker/bin/healthcheck.sh:
##########
@@ -0,0 +1,40 @@
+#!/bin/bash -e
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+##
+## @fn healthcheck.sh
+##
+## Performs a health check for the Guacamole container. If HEALTH_CHECK_VALVE_ENABLED
+## is set to "true", this script will check the actual health endpoint. Otherwise,
+## it will simply return success and log that health checks are disabled.
+##
+
+
+# Check if health check valve is enabled
+if [ "$HEALTH_CHECK_VALVE_ENABLED" = "true" ]; then
+ # Default health check path (/health)
+ HEALTH_CHECK_PATH="${HEALTH_CHECK_VALVE_PATH:-/health}"
+ # Perform actual health check via curl
+ curl --fail --silent --show-error "http://localhost:8080${HEALTH_CHECK_PATH}" || exit 1
Review Comment:
Yeah, I would think checking the variable would be the right way to go...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org