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.

Sign up for GitHub

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

server.max-http-request-header-size doesn't affect Netty server with http2 enabled #36766

Closed
wants to merge 2 commits into from
Closed

server.max-http-request-header-size doesn't affect Netty server with http2 enabled #36766

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71d4076
Fix issue when server.max-http-request-header-size doesn't have effec…
NersesAM Aug 5, 2023
e3a9cda
Revert unrelated .whenNotNull() call removals.
NersesAM Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions .../org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ public void customize(NettyReactiveWebServerFactory factory) {
map.from(nettyProperties::getIdleTimeout).to((idleTimeout) -> customizeIdleTimeout(factory, idleTimeout));
map.from(nettyProperties::getMaxKeepAliveRequests)
.to((maxKeepAliveRequests) -> customizeMaxKeepAliveRequests(factory, maxKeepAliveRequests));
map.from(this.serverProperties.getMaxHttpRequestHeaderSize())
.to((maxHttpRequestHeaderSize) -> customizeHttp2MaxHeaderSize(factory, maxHttpRequestHeaderSize.toBytes()));
customizeRequestDecoder(factory, map);
}

Expand All @@ -83,25 +85,19 @@ private void customizeConnectionTimeout(NettyReactiveWebServerFactory factory, D
private void customizeRequestDecoder(NettyReactiveWebServerFactory factory, PropertyMapper propertyMapper) {
factory.addServerCustomizers((httpServer) -> httpServer.httpRequestDecoder((httpRequestDecoderSpec) -> {
propertyMapper.from(this.serverProperties.getMaxHttpRequestHeaderSize())
.whenNonNull()
Copy link
Contributor Author

@NersesAM NersesAM Aug 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Removed these as well in this method because PropertyMapper is already initialised with .alwaysApplyingWhenNonNull() so these are redundant

Copy link
Member

Choose a reason for hiding this comment

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

Thanks but please revert those as they are unrelated.

.to((maxHttpRequestHeader) -> httpRequestDecoderSpec
.maxHeaderSize((int) maxHttpRequestHeader.toBytes()));
ServerProperties.Netty nettyProperties = this.serverProperties.getNetty();
maxChunkSize(propertyMapper, httpRequestDecoderSpec, nettyProperties);
propertyMapper.from(nettyProperties.getMaxInitialLineLength())
.whenNonNull()
.to((maxInitialLineLength) -> httpRequestDecoderSpec
.maxInitialLineLength((int) maxInitialLineLength.toBytes()));
propertyMapper.from(nettyProperties.getH2cMaxContentLength())
.whenNonNull()
.to((h2cMaxContentLength) -> httpRequestDecoderSpec
.h2cMaxContentLength((int) h2cMaxContentLength.toBytes()));
propertyMapper.from(nettyProperties.getInitialBufferSize())
.whenNonNull()
.to((initialBufferSize) -> httpRequestDecoderSpec.initialBufferSize((int) initialBufferSize.toBytes()));
propertyMapper.from(nettyProperties.isValidateHeaders())
.whenNonNull()
.to(httpRequestDecoderSpec::validateHeaders);
propertyMapper.from(nettyProperties.isValidateHeaders()).to(httpRequestDecoderSpec::validateHeaders);
return httpRequestDecoderSpec;
}));
}
Expand All @@ -110,7 +106,6 @@ private void customizeRequestDecoder(NettyReactiveWebServerFactory factory, Prop
private void maxChunkSize(PropertyMapper propertyMapper, HttpRequestDecoderSpec httpRequestDecoderSpec,
ServerProperties.Netty nettyProperties) {
propertyMapper.from(nettyProperties.getMaxChunkSize())
.whenNonNull()
.to((maxChunkSize) -> httpRequestDecoderSpec.maxChunkSize((int) maxChunkSize.toBytes()));
}

Expand All @@ -122,4 +117,9 @@ private void customizeMaxKeepAliveRequests(NettyReactiveWebServerFactory factory
factory.addServerCustomizers((httpServer) -> httpServer.maxKeepAliveRequests(maxKeepAliveRequests));
}

private void customizeHttp2MaxHeaderSize(NettyReactiveWebServerFactory factory, long maxHttpRequestHeaderSize) {
factory.addServerCustomizers(((httpServer) -> httpServer.http2Settings(
Copy link
Member

Choose a reason for hiding this comment

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

This looks odd to me. This is the first HTTP2-specific setting that I can see. And it being applied regardless of http2 being enabled doesn't look right.

I wonder why using HTTP2 requires to set this setting twice as we're already taking care of that in configuring HttpRequestDecoderSpec.

Copy link
Contributor Author

@NersesAM NersesAM Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

http1.1 uses different netty codec. Compare this stacktrace with the one above and their settings are separate

io.netty.handler.codec.http.TooLongHttpHeaderException: HTTP header is larger than 8192 bytes.
	at io.netty.handler.codec.http.HttpObjectDecoder$HeaderParser.newException(HttpObjectDecoder.java:1091)
	at io.netty.handler.codec.http.HttpObjectDecoder$HeaderParser.parse(HttpObjectDecoder.java:1058)
	at io.netty.handler.codec.http.HttpObjectDecoder.readHeaders(HttpObjectDecoder.java:656)
	at io.netty.handler.codec.http.HttpObjectDecoder.decode(HttpObjectDecoder.java:285)
	at io.netty.handler.codec.http.HttpServerCodec$HttpServerRequestDecoder.decode(HttpServerCodec.java:140)
	at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:529)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:468)
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:290)
	at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:251)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:166)
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:788)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:724)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:650)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:562)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:833)

here is where in reactor-netty-http sets the http2 settings into netty
https://github.com/reactor/reactor-netty/blob/99800186555bf874c4a4fdf6abf3e4b2da4fe0ee/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerConfig.java#L430C7-L430C7

(http2SettingsSpecBuilder) -> http2SettingsSpecBuilder.maxHeaderListSize(maxHttpRequestHeaderSize))));
}

}
30 changes: 25 additions & 5 deletions ...springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;
import org.mockito.junit.jupiter.MockitoExtension;
import reactor.netty.http.Http2SettingsSpec;
import reactor.netty.http.server.HttpRequestDecoderSpec;
import reactor.netty.http.server.HttpServer;

Expand Down Expand Up @@ -126,21 +127,32 @@ void setMaxKeepAliveRequests() {
verifyMaxKeepAliveRequests(factory, 100);
}

@Test
void setHttp2MaxRequestHeaderSize() {
DataSize headerSize = DataSize.ofKilobytes(24);
this.serverProperties.setMaxHttpRequestHeaderSize(headerSize);
NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class);
this.customizer.customize(factory);
verifyHttp2MaxHeaderSize(factory, headerSize.toBytes());
}

@Test
void configureHttpRequestDecoder() {
ServerProperties.Netty nettyProperties = this.serverProperties.getNetty();
this.serverProperties.setMaxHttpRequestHeaderSize(DataSize.ofKilobytes(24));
nettyProperties.setValidateHeaders(false);
nettyProperties.setInitialBufferSize(DataSize.ofBytes(512));
nettyProperties.setH2cMaxContentLength(DataSize.ofKilobytes(1));
setMaxChunkSize(nettyProperties);
nettyProperties.setMaxInitialLineLength(DataSize.ofKilobytes(32));
NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class);
this.customizer.customize(factory);
then(factory).should().addServerCustomizers(this.customizerCaptor.capture());
NettyServerCustomizer serverCustomizer = this.customizerCaptor.getValue();
then(factory).should(times(2)).addServerCustomizers(this.customizerCaptor.capture());
NettyServerCustomizer serverCustomizer = this.customizerCaptor.getAllValues().get(1);
HttpServer httpServer = serverCustomizer.apply(HttpServer.create());
HttpRequestDecoderSpec decoder = httpServer.configuration().decoder();
assertThat(decoder.validateHeaders()).isFalse();
assertThat(decoder.maxHeaderSize()).isEqualTo(this.serverProperties.getMaxHttpRequestHeaderSize().toBytes());
assertThat(decoder.initialBufferSize()).isEqualTo(nettyProperties.getInitialBufferSize().toBytes());
assertThat(decoder.h2cMaxContentLength()).isEqualTo(nettyProperties.getH2cMaxContentLength().toBytes());
assertMaxChunkSize(nettyProperties, decoder);
Expand All @@ -162,7 +174,7 @@ private void verifyConnectionTimeout(NettyReactiveWebServerFactory factory, Inte
then(factory).should(never()).addServerCustomizers(any(NettyServerCustomizer.class));
return;
}
then(factory).should(times(2)).addServerCustomizers(this.customizerCaptor.capture());
then(factory).should(times(3)).addServerCustomizers(this.customizerCaptor.capture());
NettyServerCustomizer serverCustomizer = this.customizerCaptor.getAllValues().get(0);
HttpServer httpServer = serverCustomizer.apply(HttpServer.create());
Map<ChannelOption<?>, ?> options = httpServer.configuration().options();
Expand All @@ -174,19 +186,27 @@ private void verifyIdleTimeout(NettyReactiveWebServerFactory factory, Duration e
then(factory).should(never()).addServerCustomizers(any(NettyServerCustomizer.class));
return;
}
then(factory).should(times(2)).addServerCustomizers(this.customizerCaptor.capture());
then(factory).should(times(3)).addServerCustomizers(this.customizerCaptor.capture());
NettyServerCustomizer serverCustomizer = this.customizerCaptor.getAllValues().get(0);
HttpServer httpServer = serverCustomizer.apply(HttpServer.create());
Duration idleTimeout = httpServer.configuration().idleTimeout();
assertThat(idleTimeout).isEqualTo(expected);
}

private void verifyMaxKeepAliveRequests(NettyReactiveWebServerFactory factory, int expected) {
then(factory).should(times(2)).addServerCustomizers(this.customizerCaptor.capture());
then(factory).should(times(3)).addServerCustomizers(this.customizerCaptor.capture());
NettyServerCustomizer serverCustomizer = this.customizerCaptor.getAllValues().get(0);
HttpServer httpServer = serverCustomizer.apply(HttpServer.create());
int maxKeepAliveRequests = httpServer.configuration().maxKeepAliveRequests();
assertThat(maxKeepAliveRequests).isEqualTo(expected);
}

private void verifyHttp2MaxHeaderSize(NettyReactiveWebServerFactory factory, long expected) {
then(factory).should(times(2)).addServerCustomizers(this.customizerCaptor.capture());
NettyServerCustomizer serverCustomizer = this.customizerCaptor.getAllValues().get(0);
HttpServer httpServer = serverCustomizer.apply(HttpServer.create());
Http2SettingsSpec decoder = httpServer.configuration().http2SettingsSpec();
assertThat(decoder.maxHeaderListSize()).isEqualTo(expected);
}

}