[Inference API] Get _services skips EIS authorization call if CCM is not configured

[jonathan-buttner]

When GET _inference/_services is made, the services implementation makes an EIS authorization request to determine what task types the cluster is authorized for.

If the environment supports CCM but the user hasn't configured an api key yet the the request would produce a few warning logs. This PR adds logic to skip trying to make the authorization request if it is a CCM supported environment but the user hasn't configured the api key.

If it is not a CCM supported environment, or if the user has supplied the API key, then the authorization request will be made.

[elasticsearchmachine]

Pinging @elastic/search-inference-team (Team:Search - Inference)

[DonalEvans]

Nitpick, but this method name could be a little confusing, since it might imply that it returns an "authorization helper" rather than being a helper method. Maybe calling it "getOrSkipAuthorization()" would be better? Or just use "getAuthorization()" ?

[DonalEvans]

There's some inconsistency between using "configured" and "enabled" when talking about CCM, with some places preferring "configured" (getAuthorizationSkippingIfCcmNotConfigured(), skipIfCcmNotConfigured, logger.debug("CCM is not configured...), but others using "enabled" (isCcmEnabledListener, ccmService.isEnabled(isCcmEnabledListener)). Is there a meaningful difference between "configured" and "enabled" here, or would it be possible to simplify things by using "enabled" throughout?

[jonathan-buttner]

I'll refactor to stick to enabled and remove configured.

For the method name getAuthorizationSkippingIfCcmNotConfigured, this is a little more tricky because I'm trying to say that this method will retrieve the authorization information if ccm is enabled and we're in a supported environment. I'm going to switch it to getAuthorizationIfPermittedEnvironment. Let me know if you have a better idea though.

[DonalEvans]

That sounds good to me

[DonalEvans]

Just personal preference, but I find it easier to parse the boolean logic here if it gets refactored to:

if (skipIfCcmNotConfigured && ccmFeature.isCcmSupportedEnvironment()) {
    var isCcmEnabledListener = ActionListener.<Boolean>wrap(response -> {
        if (response == null || response == false) {
            logger.debug("CCM is not configured, skipping authorization request to Elastic Inference Service.");
            countdownListener.onResponse(ElasticInferenceServiceAuthorizationModel.unauthorized());
        } else {
            retrieveAuthorizationInformation(countdownListener, sender);
        }
    }, e -> {
        logger.atDebug().withThrowable(e).log("Failed to determine if CCM is configured, returning unauthorized.");
        countdownListener.onResponse(ElasticInferenceServiceAuthorizationModel.unauthorized());
    });

    ccmService.isEnabled(isCcmEnabledListener);
} else {
    retrieveAuthorizationInformation(countdownListener, sender);
}

[DonalEvans]

Nitpick, but for better readability, consider renaming response to enabled.

[DonalEvans]

Rather than using a hard-coded value for the timeout here, it would be better to use the ESTestCase.TEST_REQUEST_TIMEOUT constant (which is also 30 seconds). There is also a TIMEOUT constant defined in this class which can be replaced with the ESTestCase constant.

[DonalEvans]

This comment is a little misleading, since we would expect no requests made to EIS even if CCM was configured, because we're throwing an exception before we can send any requests. With that in mind, it might be good to have a test where CCM is enabled and we throw an exception, and then confirm that we don't send any requests in that scenario either.

Also, rather than asserting on the size being 0, asserting that the list is empty will provide a much more informative error message if the assertion fails, allowing us to see what unexpected objects were present instead of just knowing that the list's size was non-zero:

assertThat(webServer.requests(), empty());

[DonalEvans]

The cause/effect is flipped here compared to how we'd normally want to use verify(), since it's not the case that we expect to call isCcmSupportedEnvironment() when we encounter an exception, but rather that calling isCcmSupportedEnvironment() is what causes the exception to be thrown in the first place. If we don't call isCcmSupportedEnvironment() then the test will fail because no exception will be thrown, so we don't need this verify check here.

[DonalEvans]

Everything past this line is duplicated in testGetAuthorizationSkipIfCcmNotConfigured_ReturnsAValidResponse_WhenCcmSupportedEnvironmentAndConfigured() and can probably be extracted to a common method if you pass in a boolean for whether you expect CCMService.isEnabled() to be called.

[DonalEvans]

We don't need to enqueue anything here, because we never send a request that would require a response.

[DonalEvans]

We can get some additional (possibly redundant) coverage by using createMockCcmService(randomBoolean()) here, since we expect the behaviour to be the same regardless of whether CCM is enabled, if the environment doesn't support it. I don't know if it's possible to actually configure things so that the environment doesn't support CMM but CMM is enabled, but it wouldn't hurt to have a test that confirms the behaviour just in case.

[DonalEvans]

Should this be logged at a higher level than debug?

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	9.3