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

JettyClient behavior when SETTINGS_HEADER_TABLE_SIZE is set to 0 in SETTINGS Frame. #4890

Closed
vidit1578756 opened this issue May 19, 2020 · 20 comments · Fixed by #4927
Closed
Assignees
Labels

Comments

@vidit1578756
Copy link

Jetty version: 9.4.27.v20200227

Java version: 14

Question:

It has been observed that when server sets SETTINGS_HEADER_TABLE_SIZE (0x1) value to 0 in settings frame, jetty-client still uses dynamic table for encoding headers.
Is it the expected behavior?

@sbordet
Copy link
Contributor

sbordet commented May 20, 2020

Do you have evidence in terms of DEBUG logs?

Looking at the code we don't use the dynamic table when the table size is 0.

@vidit1578756
Copy link
Author

Http2HeaderTableSize.zip
jetty-debug.txt

I have attached zip file which contains "Http2_Header_Table_Size_Zero.pcapng" and "Http2_Header_Table_Size_Not_Zero.pcapng".

Use filter: ((http2 ) && (tcp.dstport == 9091)) in wireshark.

Call is between jetty-client and undertow server running on port 9091.

"Http2_Header_Table_Size_Not_Zero.pcapng (header table size is non-zero)"

In this, jetty-client will represent Header fields with “Literal Header Field with Incremental Indexing — Indexed Name” or “Literal Header Field with Incremental Indexing — New Name” and in subsequent requests, it will use “Index Header Field Representation” to refer the entry in dynamic table. So, this is working as expected.

Http2_Header_Table_Size_Zero.pcapng (header table size is zero)

In this, jetty-client will represent Header fields with “Literal Header Field without Indexing — Indexed Name” or “Literal Header Field without Indexing — New Name” but for few header fields it also uses “Literal Header Field with Incremental Indexing — Indexed Name” or “Literal Header Field with Incremental Indexing — New Name” but the catch is it never uses “Index Header Field Representation” to refer entry in dynamic table in subsequent requests. However, it does not use dynamic table but it represents header fields with "Literal Header Field with Incremental Indexing" which tells the decoder to add entry in dynamic table.

I think client is misrepresenting few header fields.

I have also attached DEBUG logs but when DEBUG is enabled, it throws org.eclipse.jetty.http2.hpack.HpackException$SessionException and request fails. It happens only for DEBUG log level.

More details can be found in attached debug logs.

@vidit1578756
Copy link
Author

We are having customer issue related to this. It will be very helpful if some pointers can be provided.

Thanks,

@gregw
Copy link
Contributor

gregw commented Jun 1, 2020

@vidit1578756 I had missed your second comment above which I think better describes your issue. It is not so much that the dynamic table is used (which it is not, we checked), but that the header encoding chosen is index, even though it will never actually be added to the table. I'll have a look to see if we can take the table size into account when selecting which encoding to use.

Note that for open source source "customer issues" are not a big motivating factor. You're welcome to take out a commercial support contract if you want the jetty developers to be motivated by your commercial interests. However, in this case, I would be interested to know what those "issues" are, is perfectly legal to send indexed headers that do not end up in the dynamic table.

gregw added a commit that referenced this issue Jun 1, 2020
When choosing a strategy to encode a HTTP2 field, do not use indexed
if the field is larger than the dynamic table.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw linked a pull request Jun 1, 2020 that will close this issue
@gregw
Copy link
Contributor

gregw commented Jun 1, 2020

@vidit1578756 I have created PR #4927 that refines the strategy used to pick an encoding, so we now do not index if the field is larger than the dynamic table. We are probably building release later this week, so can you verify if my diagnosis above is correct and if this PR fixes your issue?

@vidit1578756
Copy link
Author

yes, I think your diagnosis is correct and this PR fixes this issue.

Thanks for the help

gregw added a commit that referenced this issue Jun 1, 2020
Fixed checkstyle in test

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Jun 1, 2020
Only index 0 content-length

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Jun 1, 2020
Fixed comments

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Jun 1, 2020
Don't parse int

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Jun 1, 2020
* Issue #4890 Large HTTP2 Fields encoded

When choosing a strategy to encode a HTTP2 field, do not use indexed
if the field is larger than the dynamic table.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4890 Large HTTP2 Fields encoded

Fixed checkstyle in test

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4890 Large HTTP2 Fields encoded

Only index 0 content-length

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4890 Large HTTP2 Fields encoded

Fixed comments

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4890 Large HTTP2 Fields encoded

Don't parse int

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@vidit1578756
Copy link
Author

Hi @gregw ,

Just one more thing, when DEBUG logs are enabled and SETTINGS_HEADER_TABLE_SIZE (dynamic table) is set to 0, org.eclipse.jetty.http2.hpack.HpackException$SessionException is thrown.

You can find attached logs at https://github.com/eclipse/jetty.project/files/4662118/jetty-debug.txt.

@sbordet
Copy link
Contributor

sbordet commented Jun 4, 2020

@vidit1578756 the logs show the jars in use are from 9.4.27, and the lines numbers don't match with the latest code.

We don't see any NPE in the latest code.

@vidit1578756
Copy link
Author

Hi @sbordet ,

jett-logs-hpack-debug.txt

I have update jetty client to 9.4.30.v20200611 but when DEBUG logs are enabled and SETTINGS_HEADER_TABLE_SIZE (dynamic table) is set to 0, org.eclipse.jetty.http2.hpack.HpackException$SessionException is still thrown with cause java.lang.NullPointerException.

I have attached logs for reference.

sbordet added a commit that referenced this issue Jul 6, 2020
… set to 0 in SETTINGS Frame.

Fixed NPE in debug logs.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Contributor

sbordet commented Jul 6, 2020

@vidit1578756 thanks for the logs, I fixed this in 43353d0.
Can you please try it out?

@joakime
Copy link
Contributor

joakime commented Jul 22, 2020

Merged PR #4927
No reply from OP.
Considering this fixed.
Will reopen if OP ever replies.

@joakime joakime closed this as completed Jul 22, 2020
@idikshit
Copy link

Hi,

We are facing a very similar issue as mentioned in this ticket

Jetty version: 9.4.38.v20210224

Java version: 14

When the SETTINGS_HEADER_TABLE_SIZE is set to 0, we see the indexing happening based on

  1. Literal Header Field with Incremental indexing - Indexed name
  2. Literal header Field with Incremental indexing - New Name

@sbordet
Copy link
Contributor

sbordet commented Apr 21, 2021

@idikshit you see this on the client or on the server?
Do you have DEBUG logs?

@idikshit
Copy link

@sbordet we are facing this issue on the client side. Please find the attached http2-hpack DEBUG logs

debug_logs.txt

@sbordet
Copy link
Contributor

sbordet commented Apr 21, 2021

@idikshit the logs looks fine to me, but there is too little information to tell - don't strip them as the context information is important to us.

Do you have a reproducer that we can run to reproduce the issue?
Or otherwise please detail with as much information as you can what the client sends, what the client receives, what do you expect and what happens instead.

@idikshit
Copy link

Hi @sbordet, please find additional details mentioned below

Scenario - Client (Egress-Gateway Jetty client) initiates connection to back-end server. Server sets SETTINGS_HEADER_TABLE_SIZE=0 in settings frame, but Egress-Gateway Jetty-client is still using HPACK compression with either of the two representations

  1. Literal Header Field with Incremental Indexing - Indexed Name
  2. Literal header Field with Incremental indexing - New Name

As a result the index table grows at back-end server, and server sends GO_AWAY frame

The following sample headers are sent in GET request from Egress-Gateway Jetty client to server for which server sends 200 OK response
a. 3gpp-sbi-target-apiroot
b. Content-Type

Since SETTINGS_HEADER_TABLE_SIZE is 0 (set by back-end server) we expect the headers not to be indexed, added in the dynamic table. They are expected to have the below representations

  1. Literal Header Field without Indexing – Indexed Name
  2. Literal Header Field without Indexing – New Name

Please find the attached DEBUG logs, Network Trace and captured snippets
You can apply filter as: "http2" or "(tcp.dstport == 5819)" in attached Network trace

Hpack.zip

Is there any setting / configuration that we can adapt to in Jetty Client in order to disable dynamic indexing (apart from SETTINGS_HEADER_TABLE_SIZE=0)

@joakime joakime reopened this Apr 21, 2021
@sbordet sbordet self-assigned this May 20, 2021
@github-actions
Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label May 21, 2022
@sbordet sbordet removed the Stale For auto-closed stale issues and pull requests label May 23, 2022
@github-actions
Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label May 24, 2023
@lachlan-roberts
Copy link
Contributor

@sbordet I think this would likely be fixed with #9749

@github-actions github-actions bot removed the Stale For auto-closed stale issues and pull requests label May 25, 2023
@sbordet
Copy link
Contributor

sbordet commented Dec 13, 2023

Closing, see also #10805.

@sbordet sbordet closed this as completed Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants