elastic / elasticsearch

Free and Open, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
68.57k stars 24.36k forks source link

[Rest Api Compatibility] Framework support for EQL and SQL endpoints #92710

Open pgomulka opened 1 year ago

pgomulka commented 1 year ago

Description

Rest api compatibility was not covering the support for eql and sql plugins (not supporting any plugins) at the time when we were implementing it.

SQL and EQL have a custom logic on creating XContentBuilder. Those two places (and possibly more) should be revisited to make sure the right parameters are passed in when creating XContentBuilder https://github.com/elastic/elasticsearch/blob/237fd28dc73a449a93d19c6e6f066383c398f2da/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java#L56 https://github.com/elastic/elasticsearch/blob/237fd28dc73a449a93d19c6e6f066383c398f2da/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/RestEqlSearchAction.java#L71

I recon at some point we can consider extending the rest api compatibility coverage to plugins and fix this for EQL and SQL. The rest api compatibility framework should support additional media types used in SQL (just headers formatting, not request/respone parsing)

steps to reproduce currently not working behaviour

curl --request PUT \
  --url http://localhost:9200/xxx \
  --header 'Accept: application/vnd.elasticsearch+json;compatible-with=8' \
  --header 'Authorization: Basic ZWxhc3RpYzpwYXNzd29yZA==' \
  --header 'Content-Type: application/vnd.elasticsearch+json;compatible-with=8'

curl --request GET \
  --url http://localhost:9200/xxx/_eql/search \
  --header 'Accept: application/vnd.elasticsearch+json;compatible-with=8' \
  --header 'Authorization: Basic ZWxhc3RpYzpwYXNzd29yZA==' \
  --header 'Content-Type: application/vnd.elasticsearch+json;compatible-with=8' \
  --data '{"keep_on_completion":true,"query":"any where true","timestamp_field":"timestamp","wait_for_completion_timeout":"1nanos"}
' -v
*   Trying 127.0.0.1:9200...
* Connected to localhost (127.0.0.1) port 9200 (#0)
> GET /xxx/_eql/search HTTP/1.1
> Host: localhost:9200
> User-Agent: curl/7.79.1
> Accept: application/vnd.elasticsearch+json;compatible-with=8
> Authorization: Basic ZWxhc3RpYzpwYXNzd29yZA==
> Content-Type: application/vnd.elasticsearch+json;compatible-with=8
> Content-Length: 122
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< X-elastic-product: Elasticsearch
< content-type: application/json;compatible-with=8
< content-length: 150
< 
* Connection #0 to host localhost left intact
{"id":"Fnc2czQ2Vk1KVHRteWo2MUQxdlVtblEaWU4zYVNxOTlTWS1hNzctckN5U3hrZzo1ODA=","is_partial":true,"is_running":true,"took":3,"timed_out":false,"hits":{}}%    
elasticsearchmachine commented 1 year ago

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

elasticsearchmachine commented 1 year ago

Pinging @elastic/es-ql (Team:QL)

stevejgordon commented 1 year ago

Thanks for raising this, @pgomulka. We have a linked issue in the .NET client which may be caused by potential differences in the response. I'll look into that more deeply on our end to check what seems to have changed. It would be helpful to ensure clients running in compatibility mode can use EQL and SQL. Right now, it's the only way for .NET consumers to work with a v8 server in .NET, so consumption may be broken without it.

sethmlarson commented 1 year ago

+1 on having compatibility mode preserve the structure of responses in 7.x for EQL and SQL, going forward all API endpoints should respect compatibility mode negotiation.

bpintea commented 1 year ago

Those two places (and possibly more) should be revisited to make sure the right parameters are passed in when creating XContentBuilder

Linking https://github.com/elastic/elasticsearch/issues/56030 to potentially also revisit and https://github.com/elastic/elasticsearch/issues/52587 just for behavior awareness.

elasticsearchmachine commented 6 months ago

Pinging @elastic/es-analytics-geo (Team:Analytics)