Repository navigation
Conversation
| * `timeout` - the request gets terminated if not performed within a timeout. The implementation needs to take special care of releasing the thread pool (if necessary). Subsequent calls that take longer than expected should not exhaust the thread pools inside Wildfly. | ||
| * `fail-on` - a set of conditions under which the probe fails. The initial implementation should cover: | ||
| * `db` - failing on database failures | ||
| * `idp-<prefix>` - failing when an IdP is not accessible |
There was a problem hiding this comment.
Maybe we should have an option to fail on all idp's?
There was a problem hiding this comment.
<prefix> doesn't make sense here, should be <identity provider alias>
There was a problem hiding this comment.
Maybe we should have an option to fail on all idp's?
+1
But I'd keep the option to ping a specific IdP as well.
There was a problem hiding this comment.
What would the IdP check do? Just verify if the IdP is online? That doesn't tell us much, does it? Let's say an IdP changes API and login stops working for us. A simple online check won't discover this.
There was a problem hiding this comment.
There are two ways to do that:
- Active - we send a request to an external IDP and see if it's up. Not perfect as you mentioned but it's fairly easy to implement.
- Passive - one of the things mentioned in the Observability Design is passive tracking of the number of exceptions from a specific external IdP. We can leverage this mechanism here and implement a switch - if we observe 10 exceptions in a certain amount of time, we assume the external IdP has some problems. Otherwise, we assume it's fine.
I'm leaning towards the passive mode. I know it might be less accurate, but it's actually "free" for us.
There was a problem hiding this comment.
+1 for the passive mode, I like it! But we could perhaps combine both approaches.
There was a problem hiding this comment.
Perhaps... But I would need very specific examples here.
There was a problem hiding this comment.
After having a discussion with @stianst, we decided to remove both External IdPs and User Federation from the Healthcheck API.
Here's the most important argument for doing this:
Including both checks probably makes the end-user situation worse. If we include an External IdP check in the Health endpoint, Kubernetes will plug all the pods out of the Service (a load balancer) if that IdP is down. Once all pods are removed from the Service, an end-user will see an HTTP 503 Error page generated by the OpenShift router. If however, the External IdP is down and we don't include it in the Health endpoint, a user will probably face an error during log in. But the error page will be displayed by Keycloak and we control what message we send there. I consider this as a slightly better situation, although the end-result is the same - a user can not log in.
There was a problem hiding this comment.
Please do not resolve this conversation. It should stay here as a reference why we decided to leave the External IdPs and User Federation out.
|
Did you send this to dev mailing list? |
There was a problem hiding this comment.
@slaskawi I kind of agree here with Stian about the timeout. IMO that could be a default for now.
Regards the design of the endpoint, would be nice to cover the expected responses, errors and status codes. Also, use the query parameter fail-on to everything is a bit confusing.
@abstractj I added some clarification. I'm totally fine with setting a sensible timeout and use it as default. But I think the parameter should stay there because of saturated CPU use cases.
@abstractj That's a very good point. I'll do that in the next iteration. |
13c6436 to
5cffe2c
Compare
|
Thank you all for very good comments. I uploaded the next iteration of the draft. |
|
Was there any thought of bringing the concerns and problems with directly utilizing SmallRye Health back to the project itself: https://github.com/smallrye/smallrye-health? For one, we've had a goal for the last 6 months to provide pure Java implementations of SmallRye projects with CDI bits layered on top. If SmallRye Metrics is a project that hasn't got there yet, maybe the team could help it to get there to provide a more re-usable implementation? From an overall perspective, I'd be less concerned with what an MP spec might do and focus on aligning with what a SmallRye project does, especially if it's purely internal to Keycloak and not something end users are creating. Please bring concerns and problems with the SmallRye Health implementation to the project. Without that feedback it can't be improved and we end up replicating similar code over many projects. |
There was a problem hiding this comment.
Missing a clear list of what checks we should have
@kenfinnigan What concerns do you mean exactly? If you're referring to the lack of CDI support in Keycloak, that's probably not a SmallRye Health project's problem. Probably the only thing that could be improved is to create some sort of static access to the most critical classes. I created an issue for this here: smallrye/smallrye-health#172
@kenfinnigan I believe the goal has been achieved. SmallRye Health works great with CDI. But during initial design of Keycloak, CDI has been dropped. I've heard that performance was one of the deciding factors but let me defer to @stianst here.
@kenfinnigan I already did that with the ticket I created. Thanks for pointing this out! |
@stianst The list has been provided in lines 30-32. We currently have only 3 of them: If you'd like to extend this list, please suggest a comment inline. |
5cffe2c to
b9088e8
Compare
|
Dear all, I've just uploaded the third iteration of the Healthcheck proposal. Differences from the previous version:
Feedback is more than welcome! Thanks, |
There was a problem hiding this comment.
Kube wants a liveness and a readiness probe right? It seems we are only getting a single endpoint, so what is used for what? There's no mention of Quarkus in this design, only WF, so how does this apply to Quarkus (will we be able to do something that is uniform?).
|
Today morning, both @stianst and I had a discussion on the future plans of both Healthcheck API and the Observability designs. We agreed that our main focus will be Quarkus-based distribution. As for the time being, we'll use what we have for the Wildfly-based distribution (which translates to the embedded metrics, custom Aerogear Extension for additional metrics and the Healthcheck as they are now). Both Designs will be repurposed and updated in the following days. One of the crucial differences is that we agreed to verify if we can use CDI extension in Quarkus (just to provide additional context - the CDI has been removed from Wildfly-based distribution as it introduces additional latency and increased the bootstrap time). Both Microprofile Health as well as Micrometer implementation provide CDI extensions to easily wire everything together. //cc @stianst @ASzc @miquelsi @abstractj @vmuzikar @pedroigor |
f79f5e2 to
e01ab64
Compare
|
Dear all, After some discussion with Stian, I've created another iteration of the proposal. You may find it here: #55 Differences from the previous version:
In general, this version is very simplistic. I've gone quite a long journey from the initial proposal to what we have now. Feedback is more than welcome but I highly recommend to comment on the PR directly. Thanks, |
| There are 3 possible HTTP Status codes that might be returned from the endpoint: | ||
| * 200 - Healthcheck passes | ||
| * 500, 503 - Healthcheck fails |
There was a problem hiding this comment.
Looking at the link from above, the 200 status can also provide key-value detailed report.
Could you provide sample response for all of the three status codes? What kind of data datapoints would be mandatorily exposed for any health check, if any? Would e.g. cause be mandatory for any failed check in 500 / 503 response?
There was a problem hiding this comment.
That's a good point @hmlnarik !
I believe it doesn't make sense to expose anything for a successful check. Successful checks are not logged anywhere in Kubernetes (failed are - in Events).
HTTP 503 is a standard response for a failed probe. It should contain enough debugging information to help users investigating a failure. In our case - this should probably the time that the probe fails. It might also be a good idea to expose an exception message but we need to make sure, there's no secret information with it.
HTTP 500 indicates an exception thrown in the probe's code.
There was a problem hiding this comment.
I slightly updated the text. I hope it works for you.
e01ab64 to
9922828
Compare
|
I've just pushed a minor review to address @hmlnarik 's comment. After the implementation, I'll review this again and make sure all the bits match. After this step - it will be ready to be merged. |
9922828 to
58fe59c
Compare
| by the Agroal framework. However, when Keycloak is idle, the implementation needs to either ask Agroal to maintain at least | ||
| one, healthy connection or perform an active check. | ||
|
|
||
| #### Healthcheck API for Liveness and Readiness probes |
There was a problem hiding this comment.
It's not completely clear here, but I don't believe it is best practice for a readiness or liveness endpoint to check external dependencies. What is restarting keycloak pods going to do if the database is down? (Just make a situation worse, would be my guess)
Apologies if I'm misunderstanding the intent.
There was a problem hiding this comment.
The Healthcheck Endpoint only checks the database at the moment (in it's readiness probe). Perhaps, in the future we will include a remote Infinispan cluster. But this needs a bit of experimentation.
The Liveness check only checks if the Keycloak REST interface is up. If it's not - something terrible happened at the boot time. So there's a good argument to restart it.
58fe59c to
ca25063
Compare
|
@miquelsi @vmuzikar @ASzc @abstractj The last bits have just been added. @abstractj Could you please merge it? |
|
Will this still be merged? Would be great to have this health check. |
https://issues.redhat.com/browse/KEYCLOAK-12398