Skip to content

Implementation of bidirection kill for kill filter.#15240

Merged
htuch merged 3 commits intoenvoyproxy:mainfrom
KBaichoo:kill-bidi
Mar 3, 2021
Merged

Implementation of bidirection kill for kill filter.#15240
htuch merged 3 commits intoenvoyproxy:mainfrom
KBaichoo:kill-bidi

Conversation

@KBaichoo
Copy link
Copy Markdown
Contributor

@KBaichoo KBaichoo commented Mar 1, 2021

Signed-off-by: Kevin Baichoo [email protected]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: Modify the kill filter to support directionality.
Additional Description: This allows us to more easily test response crashes.
Risk Level: low (this is a test-only filter AFAIK)
Testing: unit test, integration test
Docs Changes: Included
Release Notes: Included
Platform Specific Features: N/A
Related Issue #14249

/assign @qqustc

Since you're one of the maintainers of the filter. PTAL.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15240 was opened by KBaichoo.

see: more, trace.

@KBaichoo
Copy link
Copy Markdown
Contributor Author

KBaichoo commented Mar 1, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15240 (comment) was created by @KBaichoo.

see: more, trace.

@KBaichoo
Copy link
Copy Markdown
Contributor Author

KBaichoo commented Mar 1, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15240 (comment) was created by @KBaichoo.

see: more, trace.

qqustc
qqustc previously approved these changes Mar 2, 2021
@KBaichoo
Copy link
Copy Markdown
Contributor Author

KBaichoo commented Mar 2, 2021

@alyssawilk since you're the maintainer on-call, can you assign maintainer to review these changes? @qqustc is an extension owner and has already signed off. Thanks!

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo minor comments.
/wait

// Default to `INBOUND` if unspecified.
enum Direction {
INBOUND = 0;
OUTBOUND = 1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Question on whether to call this REQUEST/RESPONSE or DECODE/ENCODE?

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.

decode and encode happen in each direction. Vote for REQUEST/RESPONSE

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Went with REQUEST/RESPONSE

@KBaichoo
Copy link
Copy Markdown
Contributor Author

KBaichoo commented Mar 3, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15240 (comment) was created by @KBaichoo.

see: more, trace.

Copy link
Copy Markdown
Member

@htuch htuch 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!

@repokitteh-read-only repokitteh-read-only bot removed the api label Mar 3, 2021
@htuch htuch merged commit 53dc7af into envoyproxy:main Mar 3, 2021
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.

5 participants