Validate Accept and Content-Type header for compatible API #54103

pgomulka · 2020-03-24T15:32:32Z

Adding validation of Accept And Content-Type headers. The idea is to verify combinations of presence and values of both headers depending if a request has a content, headers are versioned (type/vnd.elasticsearch+subtype;compatible-with) .
It also changes the way media type is parsed (previously always assuming application as a type i.e application/json) It should expect different types like text - used in SQL
Not adding a compatible headers for _cat api. These in order to return a default text do not expect Accept header (Content-type is not needed as content is not present)

See #53228 (comment) for more context

1226 tests, 227 failures, 16ignored 23 / 2 failled in index/* failing CompatRestIT. test {yaml=index/70_mix_typeless_typeful/Index call that introduces new field mappings} CompatRestIT. test {yaml=index/70_mix_typeless_typeful/Index with typeless API on an index that has types}

…ype-index-get

Introduce module for compat code

…failing

code review follow up

This reverts commit ae1799f, reversing changes made to f00f438.

This reverts commit a0fce89.

When a request to a compatible api contains a body it will require a content-type header to also contain a compatible media-type - in a form application/vnd.elasticsearch+json;compatible-with=7 When there is no body - it will not be required. Accept header is always required for compatible API

jakelandis

I think the compatibleWithVersion can be simplified to a return a boolean and only care about version (not media types). Also, the exception for where it is thrown is resulting in a closed connection without a proper response code.

jakelandis · 2020-04-13T16:30:50Z

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

@@ -80,6 +80,7 @@
    public static final Version V_7_8_0 = new Version(7080099, org.apache.lucene.util.Version.LUCENE_8_5_0);
    public static final Version V_8_0_0 = new Version(8000099, org.apache.lucene.util.Version.LUCENE_8_5_0);
    public static final Version CURRENT = V_8_0_0;
+    public static final Version PREVIOUS = V_7_0_0;


This is a bit ambiguous, and probably not a necessary as a static constant.

Can we add in a method like minimumRestCompatibilityVersion ?

In doing so, can you please make the method static? It should only apply to the CURRENT version. The fact the other compatibility methods are on Version instances makes testing and modifying the rules difficult, even though in practice the CURRENT version's compatibility is all we need, and the others are just artificial for testing.

agree, the method should be static

jakelandis · 2020-04-13T18:30:45Z

server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java

+                badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
+                //todo // tempting to just rethrow. removing content type is just making this less obvious
+                throw e;
+//                innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause);


can you clean up the this todo abit ? Not sure I follow.

just throwing an exception was helping checking the logs, but indeed was breaking the channel creation and there was no response sent back..
will fix

jakelandis · 2020-04-13T19:00:59Z

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

+                    return Version.CURRENT;
+                }
+                // both headers are versioned but set to incorrect version
+                throw new CompatibleApiHeadersCombinationException(


Throwing the exception here results in an abrupt termination of the connection (no error code or proper response). Not sure the fix, but we should add a REST test to ensure the error code.

I think that we allow RestRequest to be in an incorrect state. When creating a request and it fails due to Content-Type validation https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/RestRequest.java#L93 or parameter validation https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/RestRequest.java#L145
we try create again without either content-type or without parameters.

This makes it harder to create a validation of headers.. we would need to allow to have an incorrect state that would be used for channel creation in order to response back..

jakelandis · 2020-04-13T19:58:23Z

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

-    private boolean isRequestCompatible() {
-        return isHeaderCompatible(header(CompatibleConstants.COMPATIBLE_HEADER));
-    }
+    private Version compatibleWithVersion() {


I think this can be greatly simplified and read easier if this returns a boolean and only concerns itself with the version (not the supported media types (that can happen later if need be)).

I think something like this below the same level of validation.

private void addCompatibleParameter() { if (isRequestingCompatibility()) { params().put(CompatibleConstants.COMPATIBLE_PARAMS_KEY, String.valueOf(Version.CURRENT.major - 1)); param(CompatibleConstants.COMPATIBLE_PARAMS_KEY); } } private boolean isRequestingCompatibility() { String acceptHeader = header(CompatibleConstants.COMPATIBLE_ACCEPT_HEADER); String aVersion = XContentType.parseVersion(acceptHeader); byte acceptVersion = aVersion == null ? Version.CURRENT.major : Integer.valueOf(aVersion).byteValue(); String contentTypeHeader = header(CompatibleConstants.COMPATIBLE_CONTENT_TYPE_HEADER); String cVersion = XContentType.parseVersion(contentTypeHeader); byte contentTypeVersion = cVersion == null ? Version.CURRENT.major : Integer.valueOf(cVersion).byteValue(); if(Version.CURRENT.major < acceptVersion || Version.CURRENT.major - acceptVersion > 1 ){ throw new IllegalStateException("something about only 1 major version support"); } if (hasContent()) { if(Version.CURRENT.major < contentTypeVersion || Version.CURRENT.major - contentTypeVersion > 1 ){ throw new IllegalStateException("something about only 1 major version support"); } if (contentTypeVersion != acceptVersion) { throw new IllegalStateException("something about needing to match"); } } return contentTypeVersion < Version.CURRENT.major || acceptVersion < Version.CURRENT.major; }

pgomulka · 2020-04-16T08:19:45Z

modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4BadRequestIT.java

+        final ObjectPath objectPath = ObjectPath.createFromResponse(response);
+        final Map<String, Object> map = objectPath.evaluate("error");
+        assertThat(map.get("type"), equalTo("compatible_api_headers_combination_exception"));
+        assertThat(map.get("reason"), equalTo("Content-Type and Accept headers have to match when content is present. " +


how much details do we want on this exception?
at the moment it helped debugging tests when seeing which failed validation

I think this is a good level of detail.

pgomulka · 2020-04-16T08:55:32Z

qa/rest-compat-tests/src/main/java/org/elasticsearch/rest/compat/AbstractCompatRestTest.java

-                         ));
-        });
+            // for cat apis accept headers would break tests which expect txt response
+            if (doSection.getApiCallSection().getApi().startsWith("cat") == false) {


we have tests that need Accept header to be empty - expecting text response
maybe we should extend XContentType to text
but then again, we don't expect content-type to be text, and adding text there would allow this

I think we can punt on the cat api's for now, and this is sufficient for now. this code is temporary and i can address this later.

pgomulka · 2020-04-16T09:03:07Z

server/src/test/java/org/elasticsearch/common/xcontent/XContentTypeTests.java

@@ -121,4 +121,21 @@ public void testVersionParsing() {
        assertThat(XContentType.parseVersion("APPLICATION/JSON"),
            nullValue());
    }
+
+    public void testVersionParsingOnText() {


as per my comment https://github.com/elastic/elasticsearch/pull/54103/files#r409392721
maybe we should also extend parsing media types that are text? Should we unify this with SQL?
see RestSqlQueryAction at the moment sql has to use format parameter
cc @astefan

Can we postpone that decision for now and work and in future work ensure that text based APIs are good ?

jakelandis

The changes here represent some really good progress and the code LGMT. Lets look to getting this merged into the feature branch.

pgomulka and others added 29 commits March 5, 2020 12:44

rest index action - 283 nofix

a4f80ca

minor tests

c9c0e55

compile fixees

258542e

Merge branch 'compat_rest_api' into compat/type-index-get

548fbd9

disable testing conventions

0dbea8a

assertions and todo for header fix

4cf6bc1

more tests and cleanup

3ac22b1

Merge remote-tracking branch 'upstream/compat_rest_api' into compat/t…

38721fe

…ype-index-get

introduce a module to house the REST code

f2db19f

fix preCommit

b83b4ce

Merge pull request #18 from jakelandis/introduce_module

cf1d340

Introduce module for compat code

move restindex compatible handlers to rest-compatibility module. 228 …

5c4a02d

…failing

moving test classes and compat related code to separate v7 module

0ef7e18

code review follow up

test class rename and return 400 when compatible header not present

835ce56

clean up deprecation warnings and remove use of consumers

f440231

v7 compatible actions warnings tests

84f1dde

rename tests and enable them

d106d1b

rename isV7Compatible method

cf61bbd

checkstyle

f00f438

Merge branch 'master' into compat/type-index-get

ae1799f

Revert "Merge branch 'master' into compat/type-index-get"

a0fce89

This reverts commit ae1799f, reversing changes made to f00f438.

Merge branch 'compat_rest_api' into compat/type-index-get

7e55744

Revert "Revert "Merge branch 'master' into compat/type-index-get""

00fe62d

This reverts commit a0fce89.

use locale with toLowerCase

a6f0b9a

spotless

4eff534

imports and disable integ tests as there are none

2d4161c

fix tests

6830f97

pgomulka added the :Core/Infra/Core Core issues without another label label Mar 24, 2020

pgomulka added 11 commits April 6, 2020 14:00

test cases

09ef281

remove unused testcase

bf09243

precommit

30f1c40

headers validation

a013a03

273 failing

6c28434

*.*

37d6426

precommit

1d57fe7

cleanup

0662ef3

cleanup

86f8bc0

Merge branch 'compat_rest_api' into compat/headers

b292080

fix docs

a4f472d

pgomulka changed the title ~~WIP compatible api headers~~ Validate Accept and Content-Type header for compatible API Apr 9, 2020

pgomulka marked this pull request as ready for review April 9, 2020 13:01

pgomulka added 5 commits April 9, 2020 17:47

fix different mimetypes

bb827bc

support text formats

e9a4fbd

do not validate media type correctness

f876d46

do not allow empty accept header

47fd5cb

allow empty accept header

bcb11fd

jakelandis requested changes Apr 13, 2020

View reviewed changes

pgomulka added 4 commits April 14, 2020 16:32

exception propagation to user

225a00b

check which tests fail

ebaa7c9

fix netty test

c96bd86

refactor test to be using hamcrest style

10e8d1b

pgomulka commented Apr 16, 2020

View reviewed changes

pgomulka added 2 commits April 16, 2020 11:44

cleanup

3181daf

Merge branch 'compat_rest_api' into compat/headers

9bd396a

jakelandis approved these changes Apr 17, 2020

View reviewed changes

pgomulka merged commit 3b3da9d into elastic:compat_rest_api Apr 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Accept and Content-Type header for compatible API #54103

Validate Accept and Content-Type header for compatible API #54103

pgomulka commented Mar 24, 2020 •

edited

Loading

jakelandis left a comment

jakelandis Apr 13, 2020

rjernst Apr 15, 2020

pgomulka Apr 16, 2020

jakelandis Apr 13, 2020

pgomulka Apr 14, 2020

jakelandis Apr 13, 2020

pgomulka Apr 14, 2020

jakelandis Apr 13, 2020

pgomulka Apr 16, 2020

jakelandis Apr 17, 2020

pgomulka Apr 16, 2020

jakelandis Apr 17, 2020

pgomulka Apr 16, 2020

jakelandis Apr 17, 2020

jakelandis left a comment

Validate Accept and Content-Type header for compatible API #54103

Validate Accept and Content-Type header for compatible API #54103

Conversation

pgomulka commented Mar 24, 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

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 left a comment

Choose a reason for hiding this comment

pgomulka commented Mar 24, 2020 •

edited

Loading