Compatible API header parsing plugin #60516

pgomulka · 2020-07-31T13:45:37Z

x-pack RestCompatibilityPlugin defines a function that takes information about RestRequest (Accept, Content-Type and hasContent) and returns a Version object indicating what compatible api version was requested.
Function also validates the values provided and throws ElasticsearchStatusException when incorrect combination of header values was used.

In oss distribution a simplistic version of a function, always returning Version.CURRENT is used.

That function is abstracted as CompatibleVersion functional interface.It is being used in RestController - for routing and in AbstractRestChannel for response shape serialisation. To make it available on AbstractRestChannel , it had to be added to ResourceHandlingHttpChannel

Some revisions to the idea of using a function

this plugin is used in both RestController as well as AbstractRestChannel the best way to make getCompatibleVersion function to be usable in both places is to inject function into a RestRequest This means that it has to be available in AbstractHttpServerTransport and all its subclasses (8 of them)

jakelandis

A few comments around naming and what is getting passed around.

jakelandis · 2020-08-24T19:30:18Z

x-pack/plugin/compat-rest-request/build.gradle

@@ -0,0 +1,17 @@
+evaluationDependsOn(xpackModule('core'))


the above line should not be necessary.

x-pack/plugin/compat-rest-request/build.gradle

jakelandis · 2020-08-24T19:37:24Z

server/src/main/java/org/elasticsearch/Version.java

@@ -277,6 +277,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        return builder.value(toString());
    }

+    public Version previousMajor() {
+        return Version.fromString(this.major - 1 + ".0.0");


nit: can you cache this is a class variable ? (i.e. only calculate it once with a null check)

hm that would lead to a recursive creation. Maybe we could cache the string to build a previous version major-1+."0.0" or a byte (previousMajor-1)*1000000
then using Version.from* returns a cached instance

server/src/main/java/org/elasticsearch/rest/RestRequest.java

jakelandis · 2020-08-24T21:30:52Z

server/src/main/java/org/elasticsearch/node/Node.java

+            throw new IllegalStateException("Only one rest compatibility plugin is allowed");
+        } else if (restCompatibilityPlugins.size() == 1){
+            restCompatibleFunction =
+                restCompatibilityPlugins.get(0);


This really isn't a function (ie method reference) it is an instance of the plugin which satisfies the functional interface. I don't think we should pass the plugin instance around.

I think you can just change this to

restCompatibleFunction = (a,b,c) -> restCompatibilityPlugins.get(0).getCompatibleVersion(a,b,c);

so that you are passing around the method reference. However, it is a bit confusing to return a method reference that satifies the same interface as the object. Maybe two interfaces (one functional like you have, and another a marker interface to filter the plugins) would read better ? Maybe a functional interface CompatibleVersion (instead of RestCompatibility) , and marker interface RestCompatibilityPlugin ?

I gave it a go, let me know if this looks any better.
There is a CompatibleVersion field on rest request, but there is a getCompatibleVersionFunction to return it in order to implement a copying constructor.
There is also a getCompatibleVersion function on the RestRequest object itself which uses the CompatibleVersion field.

It might be a bit confusing, WDYT?

also I am not sure about the marker interface. We still have to define a function on it in order to use it, right?

server/src/main/java/org/elasticsearch/plugins/NetworkPlugin.java

jakelandis · 2020-08-24T21:37:43Z

server/src/main/java/org/elasticsearch/plugins/RestCompatibility.java

+import org.elasticsearch.common.Nullable;
+
+@FunctionalInterface
+public interface RestCompatibility {


See other comments about maybe breaking this up into two interfaces and simplifying the method name to just get

jakelandis · 2020-08-24T21:54:51Z

We need to pass that method reference through quite a bit of code. Since this is really only used in a very limited context I wonder if the consuming code should use some form of SPI to load class with the method reference ? We might end up with a couple instances of the class, or have to do some custom class loading ... @pgomulka - what are your thoughts...is this bleeding too much into other areas of the codebase for the sole purpose of passing it through to where it is used ?

pgomulka · 2020-09-01T09:35:08Z

@jakelandis I don't think we can use the SPI classloader. In order to load a class with ServiceLoader it would have to be available on a classpath within the server classloader. Xpack modules also have their own classloaders, so ServiceLoader won't be able to load classes from them (unless we pass the plugin instance inteself to the place where ServiceLoader is used - but that requires the same effort as the solution in this PR)
we discussed this here #60220

elasticmachine · 2020-09-08T08:59:55Z

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

pgomulka · 2020-09-08T11:13:18Z

@elasticmachine run elasticsearch-ci/2

jaymode · 2020-09-08T15:34:49Z

server/src/main/java/org/elasticsearch/node/Node.java

+    /**
+     * @return A function that can be used to determine the requested REST compatible version
+     */
+    private CompatibleVersion getRestCompatibleFunction() {


can we add tests for this? Maybe in NodeTests

server/src/main/java/org/elasticsearch/plugins/RestCompatibilityPlugin.java

server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java

server/src/main/java/org/elasticsearch/rest/RestHandler.java

server/src/test/java/org/elasticsearch/VersionTests.java

Co-authored-by: Jay Modi <jaymode@users.noreply.github.com>

jaymode

LGTM. Thanks for the iterations.

server/src/main/java/org/elasticsearch/node/Node.java

Co-authored-by: Jay Modi <jaymode@users.noreply.github.com>

...lugin/rest-compatibility/src/main/java/org/elasticsearch/compat/CompatibleVersionPlugin.java

pgomulka · 2020-09-18T15:12:26Z

server/src/main/java/org/elasticsearch/rest/RestRequest.java

@@ -79,11 +79,18 @@ public boolean isContentConsumed() {
        return contentConsumed;
    }

+    // for testing


also if we make mediatypeparser even more strict, we could remove the RestRequest.parseContentType (line 504) and line 60
WDYT?

…ka/elasticsearch into compat_plugin_inside_rest_request

pgomulka · 2020-10-06T12:45:49Z

...lugin/rest-compatibility/src/main/java/org/elasticsearch/compat/CompatibleVersionPlugin.java

+        Byte aVersion = XContentType.parseVersion(acceptHeader);
+        byte acceptVersion = aVersion == null ? Version.CURRENT.major : Integer.valueOf(aVersion).byteValue();
+        Byte cVersion = XContentType.parseVersion(contentTypeHeader);
+        byte contentTypeVersion = cVersion == null ? Version.CURRENT.major : Integer.valueOf(cVersion).byteValue();


this only supports versioned media types defined in XContentType so json, yaml, cbor, smile

What if someone sends a request like:

curl --request POST \ --url http://localhost:9200/_sql \ --header 'accept: application/vnd.elasticsearch+csv;compatible-with=7' \ --header 'authorization: Basic ZWxhc3RpYzpwYXNzd29yZA==' \ --header 'content-type: application/vnd.elasticsearch+json;compatible-with=7' \ --header 'x-opaque-id: 123' \ --data '{ "query": "SELECT * FROM library WHERE release_date < \u00272000-01-01\u0027" } '

the current logic is not able to parse accept: application/vnd.elasticsearch+csv;compatible-with=7 so sets acceptVersion = 8 however it can parse content-type: application/vnd.elasticsearch+json;compatible-with=7' so contentTypeVersion = 7 that leads to an exception https://github.com/elastic/elasticsearch/pull/60516/files#diff-793fe1b13ffa171bfc74dab865948b08R47 Content-Type and Accept version requests have to match.

How will clients send these headers? will client be aware to which API should they send versioned media types?
@elastic/es-clients any thoughts?

To make it work now the request would have to be sent without versions.

curl --request POST \ --url http://localhost:9200/_sql \ --header 'accept: application/csv' \ --header 'content-type: application/json' \ --header 'x-opaque-id: 123' \ --data '{ "query": "SELECT * FROM library WHERE release_date < \u00272000-01-01\u0027" } '

The clients are running with the assumption that we can send the vendor mimetype to all API's.

That way a client running 7.11 can start sending application/vnd.elasticsearch+json;compatible-with=7 to API's even if that API does not have any special backward compatibility handling yet.

If the user upgrades the server first to 7.11 and the API now does have backward compatibility baked in nothing really changes when the client is updated afterwards.

If the user updates to 8.x before the API gets bwc handling the client will still send and receive v7 mime types.

-> accept: application/vnd.elasticsearch+json;compatible-with=7 -> content-type: application/vnd.elasticsearch+json;compatible-with=7

If the client upgrades to 8.x:

-> accept: application/vnd.elasticsearch+json;compatible-with=8 -> content-type: application/vnd.elasticsearch+json;compatible-with=8

If nothing changes in the server 10.x and the API still has no backward compatibly layer. (and the client is still on 8.x)

-> accept: application/vnd.elasticsearch+json;compatible-with=8 -> content-type: application/vnd.elasticsearch+json;compatible-with=8

Will fail, even if nothing actually changed in between these different versions. Adhering to supported backward compatibility guides.

the current logic is not able to parse accept: application/vnd.elasticsearch+csv;compatible-with=7 so sets acceptVersion = 8 however it can parse content-type: application/vnd.elasticsearch+json;compatible-with=7' so contentTypeVersion = 7

What are the cases where it can happen that json will have a compatible response but csv does not? Don't they flow through the same xcontent builders/parsers?

The behavior of setting acceptVersion to something different then what the user/client explicitly provided should not happen. The resulting version mismatch error will be very confusing.

pgomulka · 2020-11-16T13:35:01Z

closing as the change was implemented in 3 PRs
#64406 Introduce per rest endpoint media types
#64423 Allow registering compatible handlers
#64481 Introduce Compatible Version plugin

pgomulka and others added 13 commits July 27, 2020 15:27

draft

c6aeb38

compatible version from a plugin

e85833e

moving compatibility function to a plugin

7f8b84a

remove unused interface

88598dc

suggested changes

c009235

cleanup

dce47ac

more cleanup

3470081

Merge pull request #21 from jakelandis/jake-xpack-spi-request

72d7b13

Some revisions to the idea of using a function

interface instead of function

6b06d7f

testcase for combinations

5ec8038

add hascontent argumetn and pass tests

a45171e

precommit

4ccf457

pgomulka added the WIP label Jul 31, 2020

pgomulka self-assigned this Jul 31, 2020

pgomulka added 6 commits August 20, 2020 17:56

Merge branch 'master' into compat_plugin_inside_rest_request

0c6a5f7

precommit

8e834b6

header validation test

29fcc81

precommit

8a0952b

compatibleWithVersion on RestHandler

b3c626e

import

8f42c1d

pgomulka mentioned this pull request Aug 21, 2020

Allow parsing Content-Type and Accept headers with version #61427

Merged

fake request with a default to current

924a35e

jakelandis requested changes Aug 24, 2020

View reviewed changes

jakelandis requested a review from jaymode August 31, 2020 16:31

pgomulka added 3 commits September 1, 2020 16:57

split plugin interface

0e0e590

rename file

51376bb

remove evaluaiton depends on

03e9a63

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Sep 8, 2020

pgomulka removed Team:Core/Infra Meta label for core/infra team WIP labels Sep 8, 2020

This was referenced Sep 8, 2020

WIP Creating Compatible API rest requests in xpack #59993

Closed

Compat/xpack spi #59981

Closed

Compat/rest controller xpack #59703

Closed

remove exception and empty lines

541cf6d

jaymode reviewed Sep 8, 2020

View reviewed changes

pgomulka and others added 3 commits September 14, 2020 15:11

Apply suggestions from code review

fd34207

Co-authored-by: Jay Modi <jaymode@users.noreply.github.com>

add testcase to node init

b3e7654

unused import

1d5b099

jaymode approved these changes Sep 16, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/node/Node.java Outdated Show resolved Hide resolved

Update server/src/main/java/org/elasticsearch/node/Node.java

1bb983b

Co-authored-by: Jay Modi <jaymode@users.noreply.github.com>

pgomulka commented Sep 17, 2020

View reviewed changes

...lugin/rest-compatibility/src/main/java/org/elasticsearch/compat/CompatibleVersionPlugin.java Outdated Show resolved Hide resolved

pgomulka commented Sep 18, 2020

View reviewed changes

pgomulka added 4 commits October 5, 2020 15:26

Merge branch 'master' into compat_plugin_inside_rest_request

448a8e7

Merge branch 'compat_plugin_inside_rest_request' of github.com:pgomul…

91011cc

…ka/elasticsearch into compat_plugin_inside_rest_request

use media type parser

89d50f5

spotless

97baf91

pgomulka commented Oct 6, 2020

View reviewed changes

Merge branch 'master' into compat_plugin_inside_rest_request

324dd57

pgomulka mentioned this pull request Oct 8, 2020

WIP media type registry with singleton instance pgomulka/elasticsearch#24

Draft

jaymode mentioned this pull request Nov 6, 2020

Allow registering compatible handlers #64423

Merged

pgomulka closed this Nov 16, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatible API header parsing plugin #60516

Compatible API header parsing plugin #60516

pgomulka commented Jul 31, 2020 •

edited

Loading

jakelandis left a comment

jakelandis Aug 24, 2020

jakelandis Aug 24, 2020

pgomulka Sep 1, 2020

jakelandis Aug 24, 2020

pgomulka Sep 1, 2020

pgomulka Sep 1, 2020

jakelandis Aug 24, 2020

jakelandis commented Aug 24, 2020

pgomulka commented Sep 1, 2020 •

edited

Loading

elasticmachine commented Sep 8, 2020

pgomulka commented Sep 8, 2020

jaymode Sep 8, 2020

jaymode left a comment

pgomulka Sep 18, 2020

pgomulka Oct 6, 2020 •

edited

Loading

Mpdreamz Oct 6, 2020

pgomulka commented Nov 16, 2020

Compatible API header parsing plugin #60516

Compatible API header parsing plugin #60516

Conversation

pgomulka commented Jul 31, 2020 • edited Loading

jakelandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis commented Aug 24, 2020

pgomulka commented Sep 1, 2020 • edited Loading

elasticmachine commented Sep 8, 2020

pgomulka commented Sep 8, 2020

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka commented Nov 16, 2020

pgomulka commented Jul 31, 2020 •

edited

Loading

pgomulka commented Sep 1, 2020 •

edited

Loading

pgomulka Oct 6, 2020 •

edited

Loading