Repository navigation
fix(rpc): apply RST_STREAM frame limit by default - #7015
waynercheung wants to merge 1 commit into
Conversation
The node.rpc.maxRstStream and secondsPerWindow options defaulted to 0, which left the HTTP/2 RST_STREAM frame limit unset, and the limit was configured only when both values were positive. Give them sensible defaults, validate them at load time, and apply the limit unconditionally so the configuration behaves consistently. - NodeConfig: default maxRstStream/secondsPerWindow to 1000/5. In postProcess, reject negative values for either option and reject Integer.MAX_VALUE specifically for maxRstStream, which grpc-java treats as disabling the limit. Independently replace each zero with its default and log a warning. - RpcService: always configure maxRstFramesPerWindow from the config values; validation stays solely in NodeConfig, consistent with the neighboring maxConcurrentCallsPerConnection option. - reference.conf, docs/configuration.md and config/README.md: document the new defaults and the 0 fallback for compatibility. - Tests: pin the Java and reference defaults, preserve positive overrides, and verify PING at the RST limit followed by GOAWAY after one additional reset using a prebuilt HTTP/2 frame burst. Note: an explicit 0 now falls back to the default instead of leaving the limit unset, and maxRstStream = 2147483647 is rejected at startup.
| parameter.getRpcMaxRstStream(), parameter.getRpcSecondsPerWindow()); | ||
| } | ||
| .maxHeaderListSize(parameter.getMaxHeaderListSize()) | ||
| .maxRstFramesPerWindow( |
There was a problem hiding this comment.
[DISCUSS] This bounds RST_STREAM per connection, but not per client. grpc-netty creates a new RstStreamCounter for each connection (NettyServerTransport#L144 → NettyServerHandler#L268-L269) and, on overflow, closes only that connection with ENHANCE_YOUR_CALM "too_many_rststreams" (#L590-L594). The gRPC ports have no per-IP connection cap: there is no ServerTransportFilter and no TLS, maxConnectionsWithSameIp is P2P-only, and RateLimiterInterceptor runs per call, after the stream already exists. A client that reconnects after each GOAWAY therefore keeps much of its stream-creation rate; in a loopback test, reconnecting under 1000/5 still kept more than half of the unlimited rate. The javadoc itself says the setting "can reduce the impact … when combined with TLS and maxConcurrentCallsPerConnection".
This is still a clear improvement and needs no code change here. Could the upgrade note say that a reconnecting client is not bounded, and recommend connection-rate or per-IP limits at the LB/firewall for public gRPC ports? A per-IP gRPC connection cap could go under "Follow up".
What does this PR do?
Turns
node.rpc.maxRstStreamandnode.rpc.secondsPerWindowinto properly defaulted and validated options, and applies the gRPC HTTP/2 RST_STREAM frame limit unconditionally.common(NodeConfig): default both options to1000 / 5. InpostProcess, reject negative values for either option and rejectInteger.MAX_VALUEspecifically formaxRstStream(which grpc-java treats as disabling the limit), and independently fall back each0to its default with a warning.framework(RpcService): always callmaxRstFramesPerWindow(...)with the configured values; validation stays solely inNodeConfig, consistent with the neighboringmaxConcurrentCallsPerConnectionoption.reference.conf,docs/configuration.md,config/README.md: document the new defaults and the0fallback kept for backward compatibility.Why are these changes required?
Both options previously defaulted to
0, andRpcServiceconfiguredmaxRstFramesPerWindowonly when both values were positive. As a result the RST_STREAM frame limit was left unset in a default deployment, and misconfigured values (negative for either option, ormaxRstStream == Integer.MAX_VALUE) were accepted silently. This brings the two options in line with how the neighboringmaxConcurrentCallsPerConnectionoption already behaves (a secure default plus validation), so the setting is consistent across the config, cannot be silently disabled, and rejects invalid values at startup.This PR has been tested by:
NodeConfigTest: default resolves to1000/5; each0independently falls back to its default; negative values andmaxRstStream == Integer.MAX_VALUEare rejected withPARAMETER_INIT; explicit positive values (includingInteger.MAX_VALUE - 1and anInteger.MAX_VALUEwindow) are preserved.RpcServiceHttp2SecurityTest: the limit is enforced at the connection layer under the default, under the legacy0/0, and under an explicit small limit; a connection at the limit still gets its PING acknowledged, and one more RST_STREAM past the limit is answered withGOAWAY(ENHANCE_YOUR_CALM).reference.confvalidation.Follow up
None.
Extra details
0for either option now falls back to its default (with a startup warning) instead of leaving the limit unset;maxRstStream = 2147483647(Integer.MAX_VALUE) is rejected at startup. Upgrade notes are included indocs/configuration.mdandcommon/src/main/java/org/tron/core/config/README.md.