Skip to content

extensions/filters: ext_authz filter to use StopAllIterationAndWatermark status#6565

Merged
lizan merged 1 commit intoenvoyproxy:masterfrom
soya3129:authz
Apr 15, 2019
Merged

extensions/filters: ext_authz filter to use StopAllIterationAndWatermark status#6565
lizan merged 1 commit intoenvoyproxy:masterfrom
soya3129:authz

Conversation

@soya3129
Copy link
Contributor

Signed-off-by: Yang Song yasong@google.com

Description: Use StopAllIteration status for ext_authz filter.
Risk Level: high.
Testing: unit testing.
Docs Changes: No behavior change expected.
Release Notes: n/a

Signed-off-by: Yang Song <yasong@google.com>
@soya3129 soya3129 changed the title extensions: ext_authz filter to use StopAllIterationAndWatermark status extensions/filters: ext_authz filter to use StopAllIterationAndWatermark status Apr 12, 2019
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

LGTM, but would like @gsagula to double check too.

Copy link
Member

@gsagula gsagula left a comment

Choose a reason for hiding this comment

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

@soya3129 It looks good overall. Just one small question.

return filter_return_ == FilterReturn::StopDecoding ? Http::FilterHeadersStatus::StopIteration
: Http::FilterHeadersStatus::Continue;
return filter_return_ == FilterReturn::StopDecoding
? Http::FilterHeadersStatus::StopAllIterationAndWatermark
Copy link
Member

Choose a reason for hiding this comment

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

If I understood correctly, StopAllIterationAndWatermark will hold headers, data and trailers. It will also return an HTTP 413 if the buffer limit is exceeded. If that's correct, isn't a change of behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! StopAllIterationAndWatermark behaves the same as if StopIterationAndWater is returned by decodeData() (the original behavior), instead of returning 413, reading will be disabled until buffered data are processed. Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

No problem! Sorry, I was looking at the description for StopAllIterationAndBuffer. Thanks!

Copy link
Member

@gsagula gsagula left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@lizan lizan merged commit a801993 into envoyproxy:master Apr 15, 2019
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.

3 participants