Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chunked HTTP APIs send (final/empty) chunk in response to HEAD requests #92032

Closed
DaveCTurner opened this issue Nov 30, 2022 · 5 comments · Fixed by #92042
Closed

Chunked HTTP APIs send (final/empty) chunk in response to HEAD requests #92032

DaveCTurner opened this issue Nov 30, 2022 · 5 comments · Fixed by #92042
Assignees
Labels
>bug :Distributed Coordination/Network Http and internode communication implementations Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination.

Comments

@DaveCTurner
Copy link
Contributor

DaveCTurner commented Nov 30, 2022

Responses to HEAD requests should contain no body, but we send an empty body instead. That's indistinguishable for one-shot APIs, but with chunked encoding this means we send a single empty chunk as a terminator:

00000000  48 45 41 44 20 2f 5f 61  6c 6c 20 48 54 54 50 2f   HEAD /_a ll HTTP/
00000010  31 2e 31 0d 0a 48 6f 73  74 3a 20 6c 6f 63 61 6c   1.1..Hos t: local
00000020  68 6f 73 74 3a 39 32 30  30 0d 0a 41 63 63 65 70   host:920 0..Accep
00000030  74 2d 45 6e 63 6f 64 69  6e 67 3a 20 67 7a 69 70   t-Encodi ng: gzip
00000040  0d 0a 0d 0a                                        ....
    00000000  48 54 54 50 2f 31 2e 31  20 32 30 30 20 4f 4b 0d   HTTP/1.1  200 OK.
    00000010  0a 58 2d 65 6c 61 73 74  69 63 2d 70 72 6f 64 75   .X-elast ic-produ
    00000020  63 74 3a 20 45 6c 61 73  74 69 63 73 65 61 72 63   ct: Elas ticsearc
    00000030  68 0d 0a 63 6f 6e 74 65  6e 74 2d 74 79 70 65 3a   h..conte nt-type:
    00000040  20 61 70 70 6c 69 63 61  74 69 6f 6e 2f 6a 73 6f    applica tion/jso
    00000050  6e 0d 0a 54 72 61 6e 73  66 65 72 2d 45 6e 63 6f   n..Trans fer-Enco
    00000060  64 69 6e 67 3a 20 63 68  75 6e 6b 65 64 0d 0a 0d   ding: ch unked...
    00000070  0a 30 0d 0a 0d 0a                                  .0....
              ^^ should have stopped here!

This became particularly noticeable with #92016 because HEAD /<index> is the recognised way to determine if an index exists, but really this is a general problem with the infrastructure introduced in #88311.

@DaveCTurner DaveCTurner added >bug :Distributed Coordination/Network Http and internode communication implementations labels Nov 30, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@DaveCTurner
Copy link
Contributor Author

Is this in fact a Netty bug? It looks like the trailing 30 0d 0a 0d 0a comes from Netty's own HttpObjectEncoder.

@original-brownbear
Copy link
Member

@DaveCTurner yea looks suspicious, on it now.

@DaveCTurner
Copy link
Contributor Author

For the record the only other endpoint I can see that supports HEAD requests and chunked responses today is /{index}/_mapping/{type} and only from 8.5.0 onwards (#89906). This API only works in v7-compatibility mode, it's removed in the v8 API, and it kind of ceased to be useful way back when we removed the ability for indices to have multiple mapping types back in 6.0. So I expect that in practice this bug has no impact in any released versions.

@original-brownbear
Copy link
Member

So I expect that in practice this bug has no impact in any released versions.

++ also I hunted this down. Fix incoming, it's Netty indeed but there's an "official" fix for this.

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Dec 1, 2022
Response bodies must always be empty for HEAD requests.
Since the request encoder does not know that its dealing with a response
to a HEAD request we have to indicate this fact to it.
Also, needed to adjust the test http client to use the http-codec so it is able
to correlate what responses are meant for HEAD requests and will correctly read
responses for HEAD requests.
Without this change the added test reproduces the extra bytes and fails with an assert
about more than one response received.

closes elastic#92032
original-brownbear added a commit that referenced this issue Dec 1, 2022
Response bodies must always be empty for HEAD requests.
Since the request encoder does not know that its dealing with a response
to a HEAD request we have to indicate this fact to it.
Also, needed to adjust the test http client to use the http-codec so it is able
to correlate what responses are meant for HEAD requests and will correctly read
responses for HEAD requests.
Without this change the added test reproduces the extra bytes and fails with an assert
about more than one response received.

closes #92032
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Dec 1, 2022
…c#92042)

Response bodies must always be empty for HEAD requests.
Since the request encoder does not know that its dealing with a response
to a HEAD request we have to indicate this fact to it.
Also, needed to adjust the test http client to use the http-codec so it is able
to correlate what responses are meant for HEAD requests and will correctly read
responses for HEAD requests.
Without this change the added test reproduces the extra bytes and fails with an assert
about more than one response received.

closes elastic#92032
elasticsearchmachine pushed a commit that referenced this issue Jan 3, 2023
…92042) (#92049)

* Fix Chunked APIs sending incorrect responses to HEAD requests (#92042)

Response bodies must always be empty for HEAD requests.
Since the request encoder does not know that its dealing with a response
to a HEAD request we have to indicate this fact to it.
Also, needed to adjust the test http client to use the http-codec so it is able
to correlate what responses are meant for HEAD requests and will correctly read
responses for HEAD requests.
Without this change the added test reproduces the extra bytes and fails with an assert
about more than one response received.

closes #92032

* fix compile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Coordination/Network Http and internode communication implementations Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants