Add a capabilities API to check node and cluster capabilities #106820
Conversation
thecoop
force-pushed
the
node-capabilities
branch
from
92e11ec to
1bc5b0e
Compare
server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Already looking quite good, Simon! 🎉
In yaml / bwc test we have to stick with historical features, i suppose. Considering we have to start annotating a decent amount of existing endpoints for the current version, we certainly don't want to do that multiple times for past versions as well (I hope 😁 ).
.../java/org/elasticsearch/action/admin/cluster/node/capabilities/NodesCapabilitiesRequest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/RestController.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java
Outdated
Show resolved
Hide resolved
|
Currently all capability checks of invalid APIs actually return supported, due to wildcards matching more than it should. Created #107425 to discuss this |
.../java/org/elasticsearch/action/admin/cluster/node/capabilities/NodesCapabilitiesRequest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java
Outdated
Show resolved
Hide resolved
...g/elasticsearch/action/admin/cluster/node/capabilities/TransportNodesCapabilitiesAction.java
Show resolved
Hide resolved
Add test for random invalid api
|
LGTM except for the open naming part, but I think we've got some decent options to pick from above 👍 |
|
Reducing the scope of rest handler wildcards is proving extremely problematic - we may need to merge this without changing wildcards. This will mean that checking for endpoint capabilities (does this node support |
That seems to defeat the purpose of the api, effectively making this a one way test "Is this API definitely available"? but not "Is this api not available?" |
|
But it will enable this to be used to check for endpoint features, which is the main use case people are asking for at the moment |
.../java/org/elasticsearch/action/admin/cluster/node/capabilities/NodesCapabilitiesRequest.java
Outdated
Show resolved
Hide resolved
| @@ -77,6 +78,13 @@ public final long getUsageCount() { | |||
|
|
|||
| @Override | |||
| public final void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { | |||
| // check if the query has any parameters that are not in the supported set (if declared) | |||
| Set<String> supported = supportedQueryParameters(); | |||
| if (supported != null && supported.containsAll(request.params().keySet()) == false) { | |||
There was a problem hiding this comment.
This check seems like it removes the need for the unconsumed parameters infra below, since it more directly checks whether a parameter is supported, rather the current "anything unused must be unsupported"? This can be a followup, but it seems like that could be removed after this change (or at least once all apis have supportedQueryParams setup?)
There was a problem hiding this comment.
They've got slightly different use cases - see #106820 (comment)
There was a problem hiding this comment.
I don't think they are different use cases. The reason we check if params are "consumed" was as a way to check if params are valid without explicitly defining which params are valid. But we can discuss if/how to remove param consumption check after merging this one.
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @thecoop, I've created a changelog YAML for you. |
Add a
_capabilitiesREST api and corresponding transport actions to support checking for node capabilities across a cluster.This adds
RestControllerto the injector. There's now a circular dependency between the capabilities REST handler, the transport actions, and the RestController, but that's unavoidable due to what the transport action actually has to do.