feat: add readiness health check for external services

[zvigrinberg]

Implements Feature depicted in JIRA-2228
a new readiness check was added to endpoint :9000/q/health/ready, External Providers Readiness Check, and off course, it affects the readiness status ( up or down) - if there is no even one provider that returns valid http code, it will be DOWN:

{
    "status": "UP",
    "checks": [
        {
            "name": "camel-routes",
            "status": "UP"
        },
        {
            "name": "External Providers Readiness Check",
            "status": "UP",
            "data": {
                "check.kind": "READINESS"
            }
        },
        {
            "name": "context",
            "status": "UP",
            "data": {
                "context.phase": "5",
                "context.name": "camel-2",
                "context.version": "4.1.0",
                "context.status": "Started",
                "check.kind": "ALL"
            }
        },
        {
            "name": "camel-consumers",
            "status": "UP"
        }
    ]
}

Tested successfully when deployed exhort with 2 replicas in DEV enironment Openshift cluster.
Deployed 2 exhort instances ( deployment + service) in the same project, each deployment with 2 pods replicas, and demonstrated the difference after "sabotaging on purpose" exactly one of the pods of each of the deployments.
Procedure in the following Repo:

https://github.com/zvigrinberg/exhort-service-readiness-experiment/blob/main/README.md#simulating-by-blocking-external-access-from-1-pod-to-real-snyk-address-using-firewall

[ruromero]

The probe is missing some details to give more information about the integration with all the providers.
The approach is good.

One thing that is missing is to check if the provider is enabled or not. In case it is disabled, it should also be shown in the health check.

Don't forget to add the check for OssIndex even though it is currently disabled.

[ruromero]

Maybe you can create a POJO to hold the providerName, the status and message to show it in the check details.

 "data": {
                "check.kind": "READINESS",
                "osv-nvd": "500 - Unable to validate osv-nvd Token: onguard: nodename nor servname provided, or not known",
                "snyk": "200 - OK"
            },

[zvigrinberg]

@ruromero i can , but then i'll have to add @RegisterForReflection annotation for this new POJO...
in the meantime, i'll add all the missing details using a map ( like i did in previous implementation), and then maybe we'll discuss it so i'll incorporate such a POJO with required fields as members...
WDYT?

[zvigrinberg]

Using my approach, now it looks like this:

{
    "status": "UP",
    "checks": [
        {
            "name": "camel-routes",
            "status": "UP"
        },
        {
            "name": "External Providers Readiness Check",
            "status": "UP",
            "data": {
                "oss-index": "{responseBody=Invalid token provided. Unauthorized, isEnabled=false, StatusCode=401}",
                "osv-nvd": "{responseBody=Exception occurred during execution on the exchange: Exchange[A36E02E84B42677-0000000000000002], isEnabled=true, StatusCode=500}",
                "snyk": "{responseBody=Token validated successfully, isEnabled=true, StatusCode=200}",
                "check.kind": "READINESS"
            }
        },
        {
            "name": "context",
            "status": "UP",
            "data": {
                "context.phase": "5",
                "context.name": "camel-4",
                "context.version": "4.1.0",
                "context.status": "Started",
                "check.kind": "ALL"
            }
        },
        {
            "name": "camel-consumers",
            "status": "UP"
        }
    ]
}

[ruromero]

I don't see why creating a POJO and adding the annotation is an impediment?
The data shown now is much more descriptive now, the only downside is that you should do some transformation so that information is clearer. I don't think showing the Object reference of the Exchange is helpful.

I would suggest to show a proper error message and if the body is empty just show the Exchange.HTTP_RESPONSE_TEXT. Maybe look at how the ProviderResponseHandler#processResponseError retrieves the error from the Exchange.
Finally, when disabled, only show disabled and not even make the request.

[zvigrinberg]

if you're insisting, i can use a POJO and an annotation.
I Think using Exchange.HTTP_RESPONSE_TEXT as raw http response body is enough, using what you did in ProviderResponseHandler#processResponseError is an overkill :)

[ruromero]

I pointed out this method as reference. Sometimes the response_text is empty and you must retrieve the details from the exception and sometimes the exception is a wrapper of an httpclientException.

[zvigrinberg]

The probe is missing some details to give more information about the integration with all the providers. The approach is good.

One thing that is missing is to check if the provider is enabled or not. In case it is disabled, it should also be shown in the health check.

Don't forget to add the check for OssIndex even though it is currently disabled.

@ruromero I Added the check for ossIndex, but currently i put only empty token (" ") for the sake of the readiness check, so it will fail on 401, once we'll activate/enable oss-index, we'll define properties for user and token/password for ossindex, and will use them in ossindex credentials validations as default credentials ( if user didn't pass these headers in the request), both for request and for credentials validation routes.

[ruromero]

@zvigrinberg thanks for the changes but I consider the api.<provider>.disabled check a requirement for this feature. When a provider is disabled, it must be shown in the check as you already do but it should not perform a request.

[ruromero]

I don't see why creating a POJO and adding the annotation is an impediment?
The data shown now is much more descriptive now, the only downside is that you should do some transformation so that information is clearer. I don't think showing the Object reference of the Exchange is helpful.

I would suggest to show a proper error message and if the body is empty just show the Exchange.HTTP_RESPONSE_TEXT. Maybe look at how the ProviderResponseHandler#processResponseError retrieves the error from the Exchange.
Finally, when disabled, only show disabled and not even make the request.

[zvigrinberg]

@zvigrinberg thanks for the changes but I consider the api.<provider>.disabled check a requirement for this feature. When a provider is disabled, it must be shown in the check as you already do but it should not perform a request.

@ruromero OK , So only perform the call if the provider is enabled right?, and show nothing in httpstatus in this case?

[ruromero]

@ruromero OK , So only perform the call if the provider is enabled right?, and show nothing in httpstatus in this case?

It is also interesting to show disabled in the status but nobody will see it as the management api is internal

[zvigrinberg]

@ruromero OK , So only perform the call if the provider is enabled right?, and show nothing in httpstatus in this case?

It is also interesting to show disabled in the status but nobody will see it as the management api is internal

Yeah it's only for debugging purposes when there are problems with readiness of pods, you invoking that endpoint on the management interface, and can learn from the response body about the providers' status and why it's not ready, this is the only purpose and usage, there is no applicative usage indeed.

[zvigrinberg]

@zvigrinberg thanks for the changes but I consider the api.<provider>.disabled check a requirement for this feature. When a provider is disabled, it must be shown in the check as you already do but it should not perform a request.

@ruromero OK , So only perform the call if the provider is enabled right?, and show nothing in httpstatus in this case?

@ruromero maybe i can just re-use com.redhat.exhort.api.v4.ProviderStatus instead of creating a new POJO in vain. and just to give up on the isEnabled field

[zvigrinberg]

@zvigrinberg thanks for the changes but I consider the api.<provider>.disabled check a requirement for this feature. When a provider is disabled, it must be shown in the check as you already do but it should not perform a request.

@ruromero OK , So only perform the call if the provider is enabled right?, and show nothing in httpstatus in this case?

@ruromero maybe i can just re-use com.redhat.exhort.api.v4.ProviderStatus instead of creating a new POJO in vain. and just to give up on the isEnabled field

@ruromero Using this POJO (ProviderStatus) ,it's working good, and with a bit of string manipulation and skipping its generated toString implementation, this is what eventually returned

{
    "status": "UP",
    "checks": [
        {
            "name": "camel-routes",
            "status": "UP"
        },
        {
            "name": "External Providers Readiness Check",
            "status": "UP",
            "data": {
                "oss-index": "providerName=oss-index, isEnabled=false, message=Provider oss-index is disabled",
                "osv-nvd": "providerName=osv-nvd, isEnabled=true, statusCode=503, message=Service Unavailable",
                "snyk": "providerName=snyk, isEnabled=true, statusCode=200, message=OK",
                "check.kind": "READINESS"
            }
        },
        {
            "name": "context",
            "status": "UP",
            "data": {
                "context.phase": "5",
                "context.name": "camel-15",
                "context.version": "4.1.0",
                "context.status": "Started",
                "check.kind": "ALL"
            }
        },
        {
            "name": "camel-consumers",
            "status": "UP"
        }
    ]
}

[ruromero]

Thanks @zvigrinberg
I agree with your comments and the solution. I have added a few comments to simplify the code.

[zvigrinberg]

Thanks @zvigrinberg I agree with your comments and the solution. I have added a few comments to simplify the code.

Hi @ruromero ,
applied all suggestions.

Thanks.

[ruromero]

LGTM

Thanks for all the work. Just fix the commits to match conventional semantic commits and when the build is green we're ready to merge.