Remove pseudo headers when converting to Spring headers #4293

minwoox · 2022-06-08T13:13:32Z

Motivation:
We store HTTP/2 pseudo headers to the map in HttpHeaders with other headers.
Then, the entries of the map are copied to Spring HttpHeaders in Spring integration.
However, it can be a problem when the copied Spring HttpHeaders is converted to Netty Headers later:
https://github.com/spring-cloud/spring-cloud-gateway/blob/v3.1.3/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/NettyRoutingFilter.java#L125-L128
The validation is failed when the pseudo headers are set.

Modifications:

Remove HTTP/2 pseudo headers when creating Spring HttpHeaders from Armeria HttpHeaders.
- Convert :authority header to HOST header.
Override ServerHttpRequest.getLocalAddress() which is added since Spring 5.2.x.

Result:

You no longer see IllegalArgumentException indicating the prohibited character of a header name
in an environment where Spring integration module and Netty client are used together.(e.g. Spring Cloud Gateway)

Motivation: We store HTTP/2 pseudo headers to the map in `HttpHeaders` with other headers. Then, the entries of the map are copied to Spring `HttpHeaders` in Spring integration. However, it can be a problem when the copied Spring `HttpHeaders` is converted to Netty Headers later: https://github.com/spring-cloud/spring-cloud-gateway/blob/v3.1.3/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/NettyRoutingFilter.java#L125-L128 The validation is failed when the pseudo headers are set. Modifications: - Remove HTTP/2 pseudo headers when creating Spring `HttpHeaders` from Armeria `HttpHeaders`. - Convert `:authority` header to `HOST` header. Result: - You no longer see `IllegalArgumentException` indicating prohibited character of a header name in an environment that Spring integration module and Netty client are used togeter.(e.g. Spring Cloud Gateway)

trustin · 2022-06-08T13:33:00Z

...configure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaHttpHeadersUtil.java

+
+final class ArmeriaHttpHeadersUtil {
+
+    static HttpHeaders fromArmeriaHttpHeaders(com.linecorp.armeria.common.HttpHeaders armeriaHeaders) {


Can't we use ArmeriaHttpUtil.toNettyHttp1ClientHeaders()?

We can't because this converts to Spring HttpHeaders(org.springframework.http.HttpHeaders) but the method converts to Netty HttpHeaders(io.netty.handler.codec.http). 😄

I am concerned that some disallowed HTTP/2 headers unfiltered by this method could be added to springHeaders.
How about adding a variant of ArmeriaHttpUtil.toNettyHttp1ClientHeaders() like ArmeriaHttpUtil.toHttp1ClientHeaders()?

void toHttp1ClientHeaders( HttpHeaders inputHeaders, BiConsumer<CharSequence, String> outputHeadersWriter) { ... } var springHeaders = ...; toHttp1ClientHeaders(armeriaHeaders, (key, value) -> springHeaders.add(key, value));

Let me just run a quick benchmark to see if there's any degradation using BiConsumer. Anyway, I guess it's time to gather the same logic:
https://github.com/line/armeria/blob/master/tomcat9/src/main/java/com/linecorp/armeria/server/tomcat/TomcatService.java#L527-L551
https://github.com/line/armeria/blob/master/jetty9/src/main/java/com/linecorp/armeria/server/jetty/JettyService.java#L378-L390

I am concerned that some disallowed HTTP/2 headers unfiltered by this method could be added to springHeaders.

Could you tell me which one you are concerned?

Had a chat with the team and decided to create a method in ArmeriaHttpUtil to dedup the logic. 😉

Could you tell me which one you are concerned?

If an HTTP/2 request is downgraded to HTTP/1, we may need to remove disallowed headers from HTTP/2 headers as we did in toNettyHttp1ClientHeaders.

Got it. 😄
Because this will be also used to convert Armeria HTTP/1.1 headers, which has pseudo headers, to other framework headers, let's keep it as it is. 😉

trustin · 2022-06-08T13:34:22Z

...nfigure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaServerHttpRequest.java

@@ -15,6 +15,7 @@
 */
 package com.linecorp.armeria.spring.web.reactive;

+import static com.linecorp.armeria.spring.web.reactive.ArmeriaHttpHeadersUtil.fromArmeriaHttpHeaders;


.. and ArmeriaHttpUtil.toNettyHttp1ServerHeaders()?

codecov · 2022-06-08T14:29:24Z

Codecov Report

Merging #4293 (0b2e5e2) into master (f899257) will decrease coverage by 0.01%.
The diff coverage is 90.00%.

❗ Current head 0b2e5e2 differs from pull request most recent head 128b420. Consider uploading reports for the commit 128b420 to get more accurate results

@@             Coverage Diff              @@
##             master    #4293      +/-   ##
============================================
- Coverage     73.37%   73.36%   -0.02%     
- Complexity    16974    16985      +11     
============================================
  Files          1450     1450              
  Lines         64069    64107      +38     
  Branches       8045     8062      +17     
============================================
+ Hits          47009    47030      +21     
- Misses        12970    12986      +16     
- Partials       4090     4091       +1

Impacted Files	Coverage Δ
.../spring/web/reactive/ArmeriaServerHttpRequest.java	`68.57% <80.00%> (-2.02%)`	⬇️
...ecorp/armeria/internal/common/ArmeriaHttpUtil.java	`88.72% <88.88%> (+<0.01%)`	⬆️
...om/linecorp/armeria/server/jetty/JettyService.java	`79.81% <100.00%> (+0.61%)`	⬆️
...spring/web/reactive/ArmeriaClientHttpResponse.java	`81.08% <100.00%> (-0.50%)`	⬇️
.../linecorp/armeria/server/tomcat/TomcatService.java	`57.62% <100.00%> (-0.85%)`	⬇️
...armeria/internal/common/stream/SubscriberUtil.java	`72.22% <0.00%> (-11.12%)`	⬇️
...rnal/common/GracefulConnectionShutdownHandler.java	`68.08% <0.00%> (-6.39%)`	⬇️
...rmeria/internal/common/kotlin/ArmeriaKotlinUtil.kt	`35.29% <0.00%> (-6.38%)`	⬇️
...eria/internal/common/stream/StreamMessageUtil.java	`61.11% <0.00%> (-5.56%)`	⬇️
...m/linecorp/armeria/client/DecodedHttpResponse.java	`80.00% <0.00%> (-5.00%)`	⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f899257...128b420. Read the comment docs.

ikhoon · 2022-06-09T06:12:01Z

...configure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaHttpHeadersUtil.java

+            final AsciiString k = e.getKey();
+            final String v = e.getValue();
+
+            if (k.isEmpty()) {


Is it possible to create a header with an empty key?
I'm not strong here but an assert may show our logic clearer.

assert !k.isEmpty();

I don't think so. Just copied it without much thinking 😆
https://github.com/line/armeria/blob/master/jetty9/src/main/java/com/linecorp/armeria/server/jetty/JettyService.java#L380

ikhoon · 2022-06-09T06:23:45Z

...configure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaHttpHeadersUtil.java

+
+final class ArmeriaHttpHeadersUtil {
+
+    static HttpHeaders fromArmeriaHttpHeaders(com.linecorp.armeria.common.HttpHeaders armeriaHeaders) {


I am concerned that some disallowed HTTP/2 headers unfiltered by this method could be added to springHeaders.
How about adding a variant of ArmeriaHttpUtil.toNettyHttp1ClientHeaders() like ArmeriaHttpUtil.toHttp1ClientHeaders()?

void toHttp1ClientHeaders( HttpHeaders inputHeaders, BiConsumer<CharSequence, String> outputHeadersWriter) { ... } var springHeaders = ...; toHttp1ClientHeaders(armeriaHeaders, (key, value) -> springHeaders.add(key, value));

jrhee17 · 2022-06-09T09:16:40Z

Functionally looks good to me 👍 I think it would be nice to dedup/group header conversion logic somehow, but looks good to me apart from this 🙇

jrhee17

Thanks @minwoox 🙇 👍 🙇

jrhee17 · 2022-06-10T03:40:16Z

core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java

+     * Copies header value pairs of the specified {@linkplain HttpHeaders Armeria headers} to the
+     * {@link BiConsumer} excluding HTTP/2 pseudo headers that starts with ':'. This also converts
+     * {@link HttpHeaderNames#AUTHORITY} header to {@link HttpHeaderNames#HOST} header if
+     * the {@linkplain HttpHeaders Armeria headers} does not have one.


I think you meant the opposite

Suggested change

* the {@linkplain HttpHeaders Armeria headers} does not have one.

* the {@linkplain HttpHeaders Armeria headers} has one.

I actually meant it. 😉 Let me change the word one to {@link HttpHeaderNames#HOST} because the original sentence is unclear. 😅

Ah I see what you meant 😅

ikhoon · 2022-06-10T05:36:41Z

...nfigure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaServerHttpRequest.java

@@ -67,6 +69,12 @@ final class ArmeriaServerHttpRequest extends AbstractServerHttpRequest {
                   .publishOn(Schedulers.fromExecutor(ctx.eventLoop()));
    }

+    private static HttpHeaders springHeaders(RequestHeaders headers) {
+        final HttpHeaders springHeaders = new HttpHeaders();
+        toHttp1Headers(headers, (key, value) -> springHeaders.add(key.toString(), value));


How about adding a new functional interface that takes three arguments and injecting the springHeaders into the method so that the lambda does not capture the object outside of the local scope.

Suggested change

toHttp1Headers(headers, (key, value) -> springHeaders.add(key.toString(), value));

toHttp1Headers(headers, springHeaders, (output, key, value) -> output.add(key.toString(), value));

That's brilliant. 👍

ikhoon

Thanks, @minwoox! 🙇‍♂️

...figure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaClientHttpResponse.java

trustin

Thanks, @minwoox !

minwoox added the defect label Jun 8, 2022

minwoox added this to the 1.17.0 milestone Jun 8, 2022

minwoox requested review from ikhoon, jrhee17 and trustin as code owners June 8, 2022 13:13

trustin reviewed Jun 8, 2022

View reviewed changes

ikhoon reviewed Jun 9, 2022

View reviewed changes

Dedup converting header logic

f1df421

jrhee17 approved these changes Jun 10, 2022

View reviewed changes

ikhoon reviewed Jun 10, 2022

View reviewed changes

Address the comment by @ikhoon

0b2e5e2

ikhoon approved these changes Jun 11, 2022

View reviewed changes

...figure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaClientHttpResponse.java Outdated Show resolved Hide resolved

Rename to key

128b420

trustin approved these changes Jul 1, 2022

View reviewed changes

trustin merged commit adbf596 into line:master Jul 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pseudo headers when converting to Spring headers #4293

Remove pseudo headers when converting to Spring headers #4293

minwoox commented Jun 8, 2022

trustin Jun 8, 2022

minwoox Jun 9, 2022

ikhoon Jun 9, 2022 •

edited

Loading

minwoox Jun 9, 2022 •

edited

Loading

minwoox Jun 9, 2022

ikhoon Jun 10, 2022

minwoox Jun 10, 2022 •

edited

Loading

trustin Jun 8, 2022

codecov bot commented Jun 8, 2022 •

edited

Loading

ikhoon Jun 9, 2022

minwoox Jun 9, 2022

ikhoon Jun 9, 2022 •

edited

Loading

jrhee17 commented Jun 9, 2022

jrhee17 left a comment

jrhee17 Jun 10, 2022

minwoox Jun 10, 2022

jrhee17 Jun 10, 2022

ikhoon Jun 10, 2022

minwoox Jun 10, 2022

ikhoon left a comment

trustin left a comment


		final class ArmeriaHttpHeadersUtil {

		static HttpHeaders fromArmeriaHttpHeaders(com.linecorp.armeria.common.HttpHeaders armeriaHeaders) {

	* the {@linkplain HttpHeaders Armeria headers} does not have one.
	* the {@linkplain HttpHeaders Armeria headers} has one.

	toHttp1Headers(headers, (key, value) -> springHeaders.add(key.toString(), value));
	toHttp1Headers(headers, springHeaders, (output, key, value) -> output.add(key.toString(), value));

Remove pseudo headers when converting to Spring headers #4293

Remove pseudo headers when converting to Spring headers #4293

Conversation

minwoox commented Jun 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

minwoox Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

jrhee17 commented Jun 9, 2022

jrhee17 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

ikhoon left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon Jun 9, 2022 •

edited

Loading

minwoox Jun 9, 2022 •

edited

Loading

minwoox Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 8, 2022 •

edited

Loading

ikhoon Jun 9, 2022 •

edited

Loading