SpringBootHealthCheckEnricher should auto detect configured liveness and readiness probes

[rohanKanojia]

Component

JKube Kit

Is your enhancement related to a problem? Please describe

Description

Originally posted by @adriannowak in Gitter thread

jkube-healthcheck-spring-boot cannot distinguish between readiness and liveness probe, it sets the same '/actuator/health' endpoint for those two probes. shoudn't it be /actuator/health/readiness and /actuator/health/liveness

When user configures Spring Boot to enable liveness and readiness probes via application.properties:

management.health.probes.enabled=true

It enables /actuator/health/liveness and /actuator/health/readiness endpoints. However SpringBootHealthCheckEnricher still generates liveness and readiness probes with /actuator/health value:

$ mvn k8s:resource


$ cat target/classes/META-INF/jkube/kubernetes.yml | grep -A4 Probe
          livenessProbe:
            failureThreshold: 3
            httpGet:
              path: /actuator/health
              port: 8080
--
          readinessProbe:
            failureThreshold: 3
            httpGet:
              path: /actuator/health
              port: 8080

Describe the solution you'd like

SpringBootHealthCheckEnricher detects explicit liveness and readiness probes are enabled via checking project configuration.

Describe alternatives you've considered

No response

Additional context

No response

[l3002]

@manusa / @rohanKanojia : I am willing to pick this up if it is available for contributors. Kindly assign this to me.

[rohanKanojia]

@l3002 : We need to figure out what property gets set whenever probes are enabled, also is the behavior the same when Spring boot configuration is provided in the form of application.yml instead of application.properties.

Are you okay with investigating this part first and then sharing your findings?

[l3002]

yeah, not a problem from my end. I'll share my investigation findings before implementing anything.

[l3002]

@manusa / @rohanKanojia, I'm sorry I haven't been able to get to this for a while, I've been cramped up with work from my day job. I'll try to propose a solution for this by the end of next week. I hope that's fine.

[l3002]

Hi @manusa / @rohanKanojia: After doing a bit research on the topic, I'm unable to understand the point of changing the endpoints with the explicit actuator endpoint. Correct me if I'm wrong that we require LivenessStateHealthIndicator and ReadinessStateHealthIndicator from the endpoint to check the liveness and readiness of the application while it is deployed on kubernetes. If that's the case then, I don't think we require explicit endpoints for retrieving this data from actuators as per below:

Both the global endpoint /actuator/health and explicit endpoints /acutator/health/liveness & /acutator/health/readiness can be used to retrieve the data. So, I'm not sure if this is required.

But If this still holds some importance then I guess we just need to set the value for PATH in config with regards to the project configuration in SpringBootHealthCheckEnricher$Config enum:

[manusa]

Both the global endpoint /actuator/health and explicit endpoints /acutator/health/liveness & /acutator/health/readiness can be used to retrieve the data. So, I'm not sure if this is required.

Path to documentation https://docs.spring.io/spring-boot/docs/current/reference/html/actuator.html#actuator.endpoints.kubernetes-probes

We don't know if the user has customized the health checks with specific checks for readiness and liveness (which are different concepts, Kubernetes acts differently upon failure of the probe -liveness failure triggers restart, readiness failure isolates the pod for a period until healthy.).

It's important that we respect this and that we define the probe as per the specific endpoints instead of the global endpoint.

[l3002]

Okay, I understand it now. So then, I guess it would be better to completely replace the global endpoint with two separate specific endpoints for liveness and readiness. Would that be fine?

[rohanKanojia]

So then, I guess it would be better to completely replace the global endpoint with two separate specific endpoints for liveness and readiness.

We should give precedence to specific endpoints when we detect user has explicitly configured them. Otherwise, we can stick to current behavior.

[l3002]

Hi @manusa / @rohanKanojia, I'm really sorry, I haven't been able to give this issue a considerable amount of time. Though, I do have a plan to implement this through by adding two additional constants to Config enum for liveness probe path & readiness probe path and replacing any instances of PATH with a conditional statement. I still need some time to test this idea and make it as simple and readable as possible.

[rohanKanojia]

@l3002 : polite ping, Did you get any time to revisit this issue? If not, shall I unassign you from this issue?

[l3002]

@rohanKanojia: I'm still working on this, I'm trying to figure out, All the places where the changes are needed, after changing the Config Enum. Though, I haven't been able to give this issue much time since past couple of weeks, I'm still working on it and possibly will be able to close this by 2nd week of the next month.

As this is a part of enhancement, I'm trying to be as cautious as possible. Just don't want to accidently break anything, this is a pretty big issue for me.

[rohanKanojia]

I'm trying to figure out, All the places where the changes are needed, after changing the Config Enum.

I think my original motivation behind creating this issue was to auto detect this configuration. I didn't mean to add a new configuration option for this. Let me try to explain step by step what needs to be done:

1. Add a boolean field in SpringBootConfiguration class named managementHealthProbesEnabled. This would be storing value of detected management.health.probes.enabled property. You would need to add an equivalent builder method call for this new field here:
   

   jkube/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/SpringBootConfiguration.java

   Lines 55 to 58 in a621561

   	configBuilder
   	.managementPort(Optional.ofNullable(properties.getProperty("management.port")).map(Integer::parseInt).orElse(null))
   	.serverPort(Integer.parseInt(properties.getProperty("server.port", DEFAULT_SERVER_PORT)))
   	.serverKeystore(properties.getProperty("server.ssl.key-store"))

2. SpringBootHealthCheckEnricher would read the value of springBootConfiguration.isManagementHealthProbesEnabled() to decide whether to go with generic health check path /actuator/health or specifics /actuator/health/liveness / /actuator/health/readiness
   

   jkube/jkube-kit/jkube-kit-spring-boot/src/main/java/org/eclipse/jkube/springboot/enricher/SpringBootHealthCheckEnricher.java

   Line 120 in a621561

   String actuatorBasePath = springBootConfiguration.getActuatorDefaultBasePath();

3. Right now buildProbe has no information about what kind of probe it's building, maybe we can add another String field called actuatorPathSuffix that will base passed as /readiness and /liveness from getReadinessProbe() and getLivenessProbe() respectively. When springBootConfiguration.isManagementHealthProbesEnabled() would evaluate to true, we will append these suffixes to actuatorBasePath if there is no explicit Config.PATH provided.
4. Add tests verifying above mentioned changes work as expected

[l3002]

@rohanKanojia : Oh, okay. I thought of creating the two separate Config fields for LIVENESS_PROBE_PATH & READINESS_PROBE_PATH in addition to the PATH already present and then add a new field to SpringBootConfiguration class for management.health.probes.enabled, but it would indeed be much easier to add a suffix based on the value of springBootConfiguration.managementHealthProbesEnabled as the base path already matches both of the specific parts.

Thanks for your insight. I'll try to implement this in code and test the result this week and update.

[l3002]

@manusa / @rohanKanojia : Wanted to give both of you an update, I've implemented the changes and now, I'm working on Unit Tests for the same.