Return a 410 (Gone) status code for unavailable API endpoints

[swallez]

Updates RestController to return a 410 (Gone) status code for known REST handlers that are unavailable in the current environment (typically serverless). Also uses ElasticsearchException to return the detailed format by default, that contains more useful information for caller-side diagnostics and logs (in particular the "type" property).

Returning 410 instead of 404 allows a better handling of the differences brought by serverless in several areas:

• in client libraries, 404 is usually not seen, unless the version of the client is newer than that of the server, and the user calls an enpoint function that doesn't exist on the server. Having a 410 will allow to report better errors to users in versions of the client libraries meant to be used with both the statefull and serverless variants of Elasticsearch
• in telemetry and Cloud proxy logs, it clearly distinguishes calls to unavailable endpoints from regular 404. This can give better insights on the usage of serverless, and also help support in identifying bad usage of serverless.

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[rjernst]

The concept here is great, having a different response code for known APIs that do not exist in serverless will help avoid confusion from users. However, I think the implementation in this PR is not sufficient. This PR depends on all endpoints from non-serverless being registered. Yet serverless completely omits some bits of code (eg watcher), so the endpoints are never registered.

I think an alternative approach would be to build a (compile time) list of APIs that exist to reference. It would need to span across modules/plugins. And in the plugin case it would actually be an improvement since a user might not have a plugin installed, yet try to use an API from it. So mapping a source of the API (eg plugin name or elasticsearch) would be another good thing to have.

Again, I think the concept here is good, but that should ensure there is consistency.

[swallez]

Thanks for the review @rjernst. I actually thought about the problem you outline but first wanted to "test the waters" with this simple change.

For plugins that are not loaded, we can indeed consider having a resource file that lists the URL patterns of all controllers that do not have a ServerlessScope annotation. In serverless mode RestController.handleBadRequest will check this list to identify known URL patterns that aren't active and return 410 in that case.

Maintaining this resource file manually would be time consuming and error-prone. It can be generated by introspecting classes, looking for RestController implementations, checking the ServerlessScope and calling routes() if not present to register URL patterns in the resource file.

Since this generation has to call the code, it seems complex (if not impossible) to be part of the build system. We can use a similar approach to ElasticsearchExceptionTest and do it in a unit test.

That unit test will use an approach similar to snapshot testing:

• introspect classes to build the non-serverless URL pattern list (check annotations and call routes())
• compare it with the existing resource file
• if they're different, fail the test and write the correct resource file on disk to a temporary location so that the developer can check the difference and update the existing resource file.

This approach guarantees that the list of non-serverless patterns will be exhaustive and up to date.

This resource file also brings other benefits: it will be the source of truth to identify API endpoints that do not exist in serverless. It can then be leveraged to verify that @availability annotations in the API specification are correct. This will ensure there is no discrepancy in serverless client libaries with the actual capabilities of the server.

What do you think? And of course I'm willing to contribute this 😉

[rjernst]

Generated resources files are exactly what I was thinking. The generation is indeed a bit tricky. I'd like to get @mark-vieira 's thoughts on such generation. I don't think we should do it in a test, but instead have an explicit generator that the build calls. I think we would want to have this as a task in each module, and then collect these resource files in the distribution.

[mark-vieira]

I think we would want to have this as a task in each module, and then collect these resource files in the distribution.

Something like this would probably work, we'd have to work out the details on how we'd aggregate these and where. To grab these files the distribution would still have to depend on these artifacts, and we want to make sure we make that dynamic somehow.

@rjernst this would be something we'd only include in the serverless distribution, yes?

[rjernst]

this would be something we'd only include in the serverless distribution, yes?

It somewhat depends. Right now the endpoint check for endpoints missing is in server. We would need to plug this into serverless (not currently pluggable, though I may be making it pluggable very soon for other reasons).

One reason I think it might be nice to generate and collect these resources across the board is for plugins, so that we can distinguish when a plugin api is called but the plugin is not installed. It's not a hard blocker here, we can do just serverless for now, but it seems to me like the work is pretty much the same whether the collection happens in serverless or not.

[joshdover]

After discussion, we think this will be a good solution for us on the Beats side. We'd like to help make sure this moves along to unblock us. Thanks!

[tvernum]

My preference would be for us to merge a version of this PR, and then iterate on something that handles unavailable modules.
If we want clients to be able to rely on 410 responses, then we should get a version of that implemented now, and improve it over time.

[technige]

@tvernum Thank you, yes, I'd be in favour of getting something in place sooner rather than later, even if it's not perfect. We need to carry out regression testing of Stack clients against Serverless, and being able to distinguish endpoints in this way is closely linked to that process.

[swallez]

@tvernum agree. Can we assign a reviewer to get this merged?

As for the next steps, I feel comfortable contributing the approach using snapshot testing that I suggested. Implementing it in the build system is beyond my current knowledge of it.

[elasticsearchmachine]

Hi @swallez, I've created a changelog YAML for you.

[rjernst]

LGTM