Skip to content

Commit

Permalink
http2: merges identical implementations of ConnectionImpl::trackInbou…
Browse files Browse the repository at this point in the history 
…ndFrames() (#32078)

Hoists (Client|Server)ConnectionImpl::trackInboundFrames() to the base class, since they are identical.

Commit Message: http2: merges identical implementations of ConnectionImpl::trackInboundFrames()
Additional Description:
Risk Level: none
Testing: ran unit and integration tests locally
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Biren Roy <birenroy@google.com>
  • Loading branch information
@birenroy
birenroy authored Jan 27, 2024
1 parent 885a6e0 commit c9d11a0
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 52 deletions.
69 changes: 23 additions & 46 deletions source/common/http/http2/codec_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1390,6 +1390,29 @@ void ConnectionImpl::addOutboundFrameFragment(Buffer::OwnedImpl& output, const u
output.addDrainTracker(releasor);
}

Status ConnectionImpl::trackInboundFrames(int32_t stream_id, size_t length, uint8_t type,
uint8_t flags, uint32_t padding_length) {
Status result;
ENVOY_CONN_LOG(trace, "track inbound frame type={} flags={} length={} padding_length={}",
connection_, static_cast<uint64_t>(type), static_cast<uint64_t>(flags),
static_cast<uint64_t>(length), padding_length);

result = protocol_constraints_.trackInboundFrames(length, type, flags, padding_length);
if (!result.ok()) {
ENVOY_CONN_LOG(trace, "error reading frame: {} received in this HTTP/2 session.", connection_,
result.message());
if (isInboundFramesWithEmptyPayloadError(result)) {
ConnectionImpl::StreamImpl* stream = getStreamUnchecked(stream_id);
if (stream) {
stream->setDetails(Http2ResponseCodeDetails::get().inbound_empty_frame_flood);
}
// Above if is defensive, because the stream has just been created and therefore always
// exists.
}
}
return result;
}

ssize_t ConnectionImpl::onSend(const uint8_t* data, size_t length) {
ENVOY_CONN_LOG(trace, "send data: bytes={}", connection_, length);
Buffer::OwnedImpl buffer;
Expand Down Expand Up @@ -2069,30 +2092,6 @@ int ClientConnectionImpl::onHeader(int32_t stream_id, HeaderString&& name, Heade
return saveHeader(stream_id, std::move(name), std::move(value));
}

// TODO(yanavlasov): move to the base class once the runtime flag is removed.
Status ClientConnectionImpl::trackInboundFrames(int32_t stream_id, size_t length, uint8_t type,
uint8_t flags, uint32_t padding_length) {
Status result;
ENVOY_CONN_LOG(trace, "track inbound frame type={} flags={} length={} padding_length={}",
connection_, static_cast<uint64_t>(type), static_cast<uint64_t>(flags),
static_cast<uint64_t>(length), padding_length);

result = protocol_constraints_.trackInboundFrames(length, type, flags, padding_length);
if (!result.ok()) {
ENVOY_CONN_LOG(trace, "error reading frame: {} received in this HTTP/2 session.", connection_,
result.message());
if (isInboundFramesWithEmptyPayloadError(result)) {
ConnectionImpl::StreamImpl* stream = getStreamUnchecked(stream_id);
if (stream) {
stream->setDetails(Http2ResponseCodeDetails::get().inbound_empty_frame_flood);
}
// Above if is defensive, because the stream has just been created and therefore always
// exists.
}
}
return result;
}

StreamResetReason ClientConnectionImpl::getMessagingErrorResetReason() const {
connection_.streamInfo().setResponseFlag(StreamInfo::ResponseFlag::UpstreamProtocolError);

Expand Down Expand Up @@ -2171,28 +2170,6 @@ int ServerConnectionImpl::onHeader(int32_t stream_id, HeaderString&& name, Heade
return saveHeader(stream_id, std::move(name), std::move(value));
}

Status ServerConnectionImpl::trackInboundFrames(int32_t stream_id, size_t length, uint8_t type,
uint8_t flags, uint32_t padding_length) {
ENVOY_CONN_LOG(trace, "track inbound frame type={} flags={} length={} padding_length={}",
connection_, static_cast<uint64_t>(type), static_cast<uint64_t>(flags),
static_cast<uint64_t>(length), padding_length);

auto result = protocol_constraints_.trackInboundFrames(length, type, flags, padding_length);
if (!result.ok()) {
ENVOY_CONN_LOG(trace, "error reading frame: {} received in this HTTP/2 session.", connection_,
result.message());
if (isInboundFramesWithEmptyPayloadError(result)) {
ConnectionImpl::StreamImpl* stream = getStreamUnchecked(stream_id);
if (stream) {
stream->setDetails(Http2ResponseCodeDetails::get().inbound_empty_frame_flood);
}
// Above if is defensive, because the stream has just been created and therefore always
// exists.
}
}
return result;
}

Http::Status ServerConnectionImpl::dispatch(Buffer::Instance& data) {
// Make sure downstream outbound queue was not flooded by the upstream frames.
RETURN_IF_ERROR(protocol_constraints_.checkOutboundFrameLimits());
Expand Down
8 changes: 2 additions & 6 deletions source/common/http/http2/codec_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -728,8 +728,8 @@ class ConnectionImpl : public virtual Connection,

// Adds buffer fragment for a new outbound frame to the supplied Buffer::OwnedImpl.
void addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data, size_t length);
virtual Status trackInboundFrames(int32_t stream_id, size_t length, uint8_t type, uint8_t flags,
uint32_t padding_length) PURE;
Status trackInboundFrames(int32_t stream_id, size_t length, uint8_t type, uint8_t flags,
uint32_t padding_length);
void onKeepaliveResponse();
void onKeepaliveResponseTimeout();
bool slowContainsStreamId(int32_t stream_id) const;
Expand Down Expand Up @@ -775,8 +775,6 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
ConnectionCallbacks& callbacks() override { return callbacks_; }
Status onBeginHeaders(int32_t stream_id) override;
int onHeader(int32_t stream_id, HeaderString&& name, HeaderString&& value) override;
Status trackInboundFrames(int32_t stream_id, size_t length, uint8_t type, uint8_t flags,
uint32_t) override;
void dumpStreams(std::ostream& os, int indent_level) const override;
StreamResetReason getMessagingErrorResetReason() const override;
Http::ConnectionCallbacks& callbacks_;
Expand All @@ -802,8 +800,6 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
ConnectionCallbacks& callbacks() override { return callbacks_; }
Status onBeginHeaders(int32_t stream_id) override;
int onHeader(int32_t stream_id, HeaderString&& name, HeaderString&& value) override;
Status trackInboundFrames(int32_t stream_id, size_t length, uint8_t type, uint8_t flags,
uint32_t padding_length) override;
absl::optional<int> checkHeaderNameForUnderscores(absl::string_view header_name) override;
StreamResetReason getMessagingErrorResetReason() const override {
return StreamResetReason::LocalReset;
Expand Down

0 comments on commit c9d11a0

Please sign in to comment.