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

Revert default HTTP/2 settings sent by the client #2346

Merged
merged 1 commit into from
Sep 9, 2022
Merged

Revert default HTTP/2 settings sent by the client #2346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions ...-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.netty.handler.codec.http2.Http2FrameCodecBuilder;
import io.netty.handler.codec.http2.Http2MultiplexHandler;

import static io.netty.buffer.ByteBufUtil.writeAscii;
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
import static io.servicetalk.http.netty.H2ServerParentChannelInitializer.initFrameLogger;
import static io.servicetalk.http.netty.H2ServerParentChannelInitializer.toNettySettings;
Expand All @@ -37,8 +38,7 @@ final class H2ClientParentChannelInitializer implements ChannelInitializer {

H2ClientParentChannelInitializer(final H2ProtocolConfig config) {
this.config = config;
final io.servicetalk.http.api.Http2Settings h2Settings = config.initialSettings();
nettySettings = toNettySettings(h2Settings);
nettySettings = toNettySettings(config.initialSettings()).pushEnabled(false).maxConcurrentStreams(0L);
Copy link
Member

Choose a reason for hiding this comment

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

If we set pushEnabled to false we don't really need to set maxConcurrentStreams

Copy link
Member Author

Choose a reason for hiding this comment

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

Either way it doesn't hurt. This is what grpc-java does. Let's be consistent.

}

@Override
Expand Down Expand Up @@ -93,7 +93,8 @@ private H2PushStreamHandler() {
@Override
public void channelRegistered(final ChannelHandlerContext ctx) {
// See SETTINGS_ENABLE_PUSH in https://tools.ietf.org/html/rfc7540#section-6.5.2
ctx.writeAndFlush(new DefaultHttp2GoAwayFrame(PROTOCOL_ERROR));
ctx.writeAndFlush(new DefaultHttp2GoAwayFrame(PROTOCOL_ERROR,
writeAscii(ctx.alloc(), "Server Push is not supported")));
// Http2ConnectionHandler.processGoAwayWriteResult will close the connection after GO_AWAY is flushed
}
}
Expand Down
16 changes: 15 additions & 1 deletion servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpClientConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
*/
package io.servicetalk.http.netty;

import io.servicetalk.http.api.Http2Settings;
import io.servicetalk.tcp.netty.internal.TcpClientConfig;
import io.servicetalk.transport.api.ClientSslConfig;
import io.servicetalk.transport.api.DelegatingClientSslConfig;

import java.util.List;
import javax.annotation.Nullable;

import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_ENABLE_PUSH;
import static io.servicetalk.http.netty.HttpServerConfig.httpAlpnProtocols;
import static io.servicetalk.utils.internal.NetworkUtils.isValidIpV4Address;
import static io.servicetalk.utils.internal.NetworkUtils.isValidIpV6Address;
Expand All @@ -41,7 +43,19 @@ final class HttpClientConfig {

HttpClientConfig() {
tcpConfig = new TcpClientConfig();
protocolConfigs = new HttpConfig();
protocolConfigs = new HttpConfig(h2Config -> {
final Http2Settings settings = h2Config.initialSettings();
final Long pushEnabled = settings.settingValue(SETTINGS_ENABLE_PUSH);
if (pushEnabled != null && pushEnabled != 0) {
throw new IllegalArgumentException("Server Push is not supported by the client, " +
"expected SETTINGS_ENABLE_PUSH value is null or 0, settings=" + settings);
}
final Long maxConcurrentStreams = settings.maxConcurrentStreams();
if (maxConcurrentStreams != null && maxConcurrentStreams != 0) {
throw new IllegalArgumentException("Server Push is not supported by the client, " +
"expected MAX_CONCURRENT_STREAMS value is null or 0, settings=" + settings);
}
});
}

HttpClientConfig(final HttpClientConfig from) {
Expand Down
15 changes: 6 additions & 9 deletions servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,12 @@
*/
package io.servicetalk.http.netty;

import io.servicetalk.http.api.Http2Settings;
import io.servicetalk.http.api.HttpProtocolConfig;

import java.util.List;
import java.util.function.Consumer;
import javax.annotation.Nullable;

import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_ENABLE_PUSH;
import static io.servicetalk.http.netty.HttpProtocolConfigs.h1Default;
import static java.util.Arrays.asList;
import static java.util.Collections.emptyList;
Expand All @@ -30,20 +29,23 @@
import static java.util.Objects.requireNonNull;

final class HttpConfig {
private final Consumer<H2ProtocolConfig> h2ConfigValidator;
@Nullable
private H1ProtocolConfig h1Config;
@Nullable
private H2ProtocolConfig h2Config;
private List<String> supportedAlpnProtocols;
private boolean allowDropTrailers;

HttpConfig() {
HttpConfig(final Consumer<H2ProtocolConfig> h2ConfigValidator) {
this.h2ConfigValidator = requireNonNull(h2ConfigValidator);
h1Config = h1Default();
h2Config = null;
supportedAlpnProtocols = emptyList();
}

HttpConfig(final HttpConfig from) {
this.h2ConfigValidator = from.h2ConfigValidator;
this.h1Config = from.h1Config;
this.h2Config = from.h2Config;
this.supportedAlpnProtocols = from.supportedAlpnProtocols;
Expand Down Expand Up @@ -107,12 +109,7 @@ private void h2Config(final H2ProtocolConfig h2Config) {
if (this.h2Config != null) {
throw new IllegalArgumentException("Duplicated configuration for HTTP/2 was found");
}
final Http2Settings settings = h2Config.initialSettings();
final Long pushEnabled = settings.settingValue(SETTINGS_ENABLE_PUSH);
if (pushEnabled != null && pushEnabled != 0) {
throw new IllegalArgumentException("Http2Settings push is enabled but not supported. settings=" + settings);
}

h2ConfigValidator.accept(h2Config);
this.h2Config = h2Config;
supportedAlpnProtocols = h1Config == null ? singletonList(h2Config.alpnId()) :
unmodifiableList(asList(h1Config.alpnId(), h2Config.alpnId()));
Expand Down
11 changes: 10 additions & 1 deletion servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpServerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package io.servicetalk.http.netty;

import io.servicetalk.http.api.Http2Settings;
import io.servicetalk.http.api.HttpLifecycleObserver;
import io.servicetalk.tcp.netty.internal.TcpServerConfig;
import io.servicetalk.transport.api.DelegatingServerSslConfig;
Expand All @@ -26,6 +27,7 @@
import java.util.Map.Entry;
import javax.annotation.Nullable;

import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_ENABLE_PUSH;
import static java.util.Objects.requireNonNull;

final class HttpServerConfig {
Expand All @@ -37,7 +39,14 @@ final class HttpServerConfig {

HttpServerConfig() {
tcpConfig = new TcpServerConfig();
httpConfig = new HttpConfig();
httpConfig = new HttpConfig(h2Config -> {
final Http2Settings settings = h2Config.initialSettings();
final Long pushEnabled = settings.settingValue(SETTINGS_ENABLE_PUSH);
if (pushEnabled != null && pushEnabled != 0) {
throw new IllegalArgumentException(
"Server cannot set SETTINGS_ENABLE_PUSH value other than 0, settings=" + settings);
}
});
}

TcpServerConfig tcpConfig() {
Expand Down