Skip to content

netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods#9004

Merged
ejona86 merged 5 commits intogrpc:masterfrom
ejona86:netty-more-header-methods
Mar 22, 2022
Merged

netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods#9004
ejona86 merged 5 commits intogrpc:masterfrom
ejona86:netty-more-header-methods

Conversation

@ejona86
Copy link
Copy Markdown
Member

@ejona86 ejona86 commented Mar 21, 2022

The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes #8981

CC @njhill
Alternative to #9001

@ejona86
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
1e59f1a 
The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes grpc#8981
@ejona86 ejona86 requested a review from sergiitk March 21, 2022 22:13
ejona86
ejona86 commented Mar 21, 2022
View reviewed changes
Comment thread netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
AsciiString value = requireAsciiString(csValue);
if (isPseudoHeader(name)) {
addPseudoHeader(name, value);
AsciiString previous = getPseudoHeader(name);
Copy link
Copy Markdown
Member Author

@ejona86 ejona86 Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be possible to implement this without a get-then-set if setPseudoHeader() returned the old value. We'd have to re-set the old value if there was one already present, but that's not the worst. Returning-old-valuesetPseudoHeader() would seem to most benefit remove(), but it isn't safe for remove because it would throw for remove(":status'). Having the returning-old-value setPseudoHeader() made it really easy to make mistakes, so I went with the KISS approach. These code paths don't matter for performance.

sergiitk
sergiitk approved these changes Mar 21, 2022
View reviewed changes
Comment thread netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
Comment thread netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java Outdated
Comment thread netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java
ejona86 added 3 commits March 21, 2022 15:36
@ejona86
Update GrpcHttp2HeadersUtilsTest.java
3afddae
@ejona86
Use getall() instead of get()
ce8fcf5 
I had these changes locally, but failed to use "-a" for my "git commit"
to add them...
@ejona86
Tests for exceptions
658d4ce
njhill
njhill reviewed Mar 21, 2022
View reviewed changes
Comment thread netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java
Comment thread netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
Comment thread netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
@ejona86 ejona86 merged commit 4a0fe99 into grpc:master Mar 22, 2022
@ejona86 ejona86 deleted the netty-more-header-methods branch March 22, 2022 14:39
This was referenced Mar 22, 2022
Merged
Merged
@hezhangjian hezhangjian mentioned this pull request Mar 31, 2022
Merged
@hezhangjian hezhangjian mentioned this pull request Apr 19, 2022
Merged
lhotari added a commit to lhotari/pulsar that referenced this pull request Apr 19, 2022
@lhotari
Upgrade grpc to 1.45.1 and protobuf to 3.19.2
f92ab73 
- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004
lhotari added a commit to lhotari/pulsar that referenced this pull request Apr 19, 2022
@lhotari
Upgrade grpc to 1.45.1 and protobuf to 3.19.2
d690028 
- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004
lhotari added a commit to lhotari/pulsar that referenced this pull request Apr 19, 2022
@lhotari
Upgrade grpc to 1.45.1 and protobuf to 3.19.2
c282f1f 
- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004
lhotari added a commit to apache/pulsar that referenced this pull request Apr 19, 2022
@lhotari
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (#15212
332a3c7 
)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes #14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@lhotari
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (apa…
622b485 
…che#15212)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes apache#14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004
lhotari added a commit to datastax/pulsar that referenced this pull request Apr 20, 2022
@lhotari
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (apa…
4afb0a6 
…che#15212)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes apache#14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004

(cherry picked from commit 332a3c7)
gaoran10 pushed a commit to apache/pulsar that referenced this pull request Apr 21, 2022
@lhotari @gaoran10
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (#15212
a547163 
)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes #14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004

(cherry picked from commit 332a3c7)
gaoran10 pushed a commit to apache/pulsar that referenced this pull request Apr 21, 2022
@lhotari @gaoran10
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (#15212
4fb5180 
)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes #14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004

(cherry picked from commit 332a3c7)
lhotari added a commit to datastax/pulsar that referenced this pull request Apr 22, 2022
@lhotari
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (apa…
65c2a59 
…che#15212)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes apache#14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004

(cherry picked from commit 332a3c7)
lhotari added a commit to datastax/pulsar that referenced this pull request Apr 22, 2022
@lhotari
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (apa…
4079eb9 
…che#15212)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes apache#14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004

(cherry picked from commit 332a3c7)
codelipenghui pushed a commit to apache/pulsar that referenced this pull request Apr 28, 2022
@lhotari @codelipenghui
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (#15212
d39c655 
)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes #14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004

(cherry picked from commit 332a3c7)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Apr 28, 2022
@lhotari @nicoloboschi
Upgrade Netty to 4.1.76.Final, Netty Tcnative, grpc and protobuf (apa…
395ac6b 
…che#15212)

* Upgrade Netty to 4.1.76.Final and Netty Tcnative to 2.0.51.Final

Fixes apache#14015
- release notes https://netty.io/news/2022/04/12/4-1-76-Final.html
  - contains fix for netty/netty#11695

* Upgrade grpc to 1.45.1 and protobuf to 3.19.2

- grpc < 1.45.1 is not compatible with Netty > 4.1.74.Final
  - grpc/grpc-java#9004

(cherry picked from commit 332a3c7)
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sergiitk sergiitk sergiitk approved these changes

+1 more reviewer

@njhill njhill njhill left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Netty 4.1.75.Final HTTP/2 connection closures

3 participants

@ejona86 @sergiitk @njhill