Skip to content

http: add downstream remote reset response flag#32623

Merged
wbpcode merged 10 commits intoenvoyproxy:mainfrom
botengyao:downstream_remote_end
Mar 8, 2024
Merged

http: add downstream remote reset response flag#32623
wbpcode merged 10 commits intoenvoyproxy:mainfrom
botengyao:downstream_remote_end

Conversation

@botengyao
Copy link
Copy Markdown
Member

@botengyao botengyao commented Feb 28, 2024

Add response flag DownstreamRemoteReset when the http2 details are http2.remote_reset or http2.remote_refuse here

This is similar as what router does for upstream

case Http::StreamResetReason::RemoteReset:
case Http::StreamResetReason::RemoteRefusedStreamReset:
case Http::StreamResetReason::ConnectError:
return StreamInfo::CoreResponseFlag::UpstreamRemoteReset;

Commit Message:
Additional Description:
Risk Level: low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #32623 was opened by botengyao.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #32623 was opened by botengyao.

see: more, trace.

Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
@botengyao botengyao marked this pull request as ready for review February 28, 2024 20:43
@botengyao botengyao requested a review from wbpcode as a code owner February 28, 2024 20:43
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
@KBaichoo
Copy link
Copy Markdown
Contributor

KBaichoo commented Mar 5, 2024

PTAL @wbpcode

Copy link
Copy Markdown
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!
Left a minor comment.
Please add a release note.

response_encoder_.stream_.resetStream(StreamResetReason::ConnectError);
}

TEST_F(HttpConnectionManagerImplTest, DownstreamRemoteResetRemoteReset) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: modify test-name (double RemoteReset) and add a one-line comment for each test

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done, thanks!

response_encoder_.stream_.resetStream(StreamResetReason::RemoteReset);
}

TEST_F(HttpConnectionManagerImplTest, DownstreamRemoteResetRemoteRefusedStreamReset) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: same as above.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM except the comment from @adisuissa

@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Mar 6, 2024

Could you also check the CI failure?

@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Mar 6, 2024

/wait

Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
@botengyao
Copy link
Copy Markdown
Member Author

/retest

@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Mar 7, 2024

/lgtm api

@repokitteh-read-only repokitteh-read-only Bot removed the api label Mar 7, 2024
@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Mar 7, 2024

/wait ci and merge main

@botengyao
Copy link
Copy Markdown
Member Author

/retest

@botengyao
Copy link
Copy Markdown
Member Author

/retest

Copy link
Copy Markdown
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@wbpcode wbpcode merged commit 0cb0b01 into envoyproxy:main Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants