Skip to content

extensions: support disabling the grpc http1 reverse bridge filter per-route#8645

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
sp-manuel-jurado:grpc_http1_reverse_bridge
Oct 21, 2019
Merged

extensions: support disabling the grpc http1 reverse bridge filter per-route#8645
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
sp-manuel-jurado:grpc_http1_reverse_bridge

Conversation

@sp-manuel-jurado
Copy link
Copy Markdown
Contributor

@sp-manuel-jurado sp-manuel-jurado commented Oct 17, 2019 •
edited by zuercher
Loading

Support disabling the filter per route in the grpc http1 reverse bridge filter.

Risk Level: low (adds per-filter config)
Testing: unit tests added
Docs: inline
Release Notes: updated
Fixes: #8052

Signed-off-by: Manuel Jurado manuel.jurado@socialpoint.es

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Oct 17, 2019

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8645 was opened by sp-manuel-jurado.

see: more, trace.

@sp-manuel-jurado sp-manuel-jurado mentioned this pull request Oct 17, 2019
Closed
@sp-manuel-jurado sp-manuel-jurado force-pushed the grpc_http1_reverse_bridge branch from 1d5244d to 1b33e94 Compare October 17, 2019 16:55
@zuercher zuercher self-assigned this Oct 17, 2019
zuercher
zuercher reviewed Oct 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks! This looks pretty close to me. Mostly just some grammar and formatting nits.

@sp-manuel-jurado sp-manuel-jurado changed the title Allow disable grpc_http1_reverse_bridge per route. Support disabling the filter per route in the grpc http1 reverse bridge filter. Oct 18, 2019
sp-manuel-jurado
sp-manuel-jurado commented Oct 18, 2019
View reviewed changes
source/common/config/api_type_db.generated.pb_text Outdated
Copy link
Copy Markdown
Contributor Author

@sp-manuel-jurado sp-manuel-jurado Oct 18, 2019

Choose a reason for hiding this comment

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

Due to master branch rebase.

Copy link
Copy Markdown
Member

@zuercher zuercher Oct 18, 2019

Choose a reason for hiding this comment

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

In the future, please just merge master if you need/want to pick up changes. Github does a better job detecting and hiding this kind of change in that case and keeps reviews simpler. Because we eventually squash commits onto master when we merge, PRs always end up as a single commit on master so the history will stay clean.

sp-manuel-jurado
sp-manuel-jurado commented Oct 18, 2019
View reviewed changes
@sp-manuel-jurado
Copy link
Copy Markdown
Contributor Author

sp-manuel-jurado commented Oct 18, 2019

@zuercher code review fixed.
Thank you very much for your review.

@sp-manuel-jurado sp-manuel-jurado mentioned this pull request Oct 18, 2019
Closed
zuercher
zuercher previously approved these changes Oct 18, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks again. This looks good. I'll assign folks from the api-shepherds group to get their approval. I'm also going to go ahead and update the PR comment/title to match our template.

@zuercher zuercher changed the title Support disabling the filter per route in the grpc http1 reverse bridge filter. extensions: support disabling the grpc http1 reverse bridge filter per-route Oct 18, 2019
@zuercher
Copy link
Copy Markdown
Member

zuercher commented Oct 18, 2019

@htuch and/or @mattklein123 this is ready for API review

mattklein123
mattklein123 previously approved these changes Oct 18, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

API LGTM, thank you!

htuch
htuch previously requested changes Oct 18, 2019
View reviewed changes
api/envoy/config/filter/http/grpc_http1_reverse_bridge/v2alpha1/config.proto Outdated
Copy link
Copy Markdown
Member

@htuch htuch Oct 18, 2019

Choose a reason for hiding this comment

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

@mattklein123 @rshriram @envoyproxy/api-shepherds where did we end up with just having a generic disable per filter? It seems kind of crazy we need to add docs, tests, etc. for every single filter we want to have a disable/pass-thru on at the per-route level.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Oct 18, 2019

Choose a reason for hiding this comment

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

I agree we should do this, but I'm not sure we should block this PR on that.

Copy link
Copy Markdown
Member

@htuch htuch Oct 18, 2019

Choose a reason for hiding this comment

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

Sure, let's merge, I've opened #8669 to track.

@htuch htuch mentioned this pull request Oct 18, 2019
Closed
@zuercher zuercher dismissed htuch’s stale review October 18, 2019 23:42

Per comment, ok to merge.

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Oct 19, 2019

Please merge master and fix format to pick up the next free field annotations.

/wait

@sp-manuel-jurado
FilterConfigPerRoute support in gRPC http1 reverse bridge.
072f072 
Signed-off-by: Manuel Jurado <manuel.jurado@socialpoint.es>
@sp-manuel-jurado
Code review fixed.
e0cdb0d 
Signed-off-by: Manuel Jurado <manuel.jurado@socialpoint.es>
@sp-manuel-jurado sp-manuel-jurado force-pushed the grpc_http1_reverse_bridge branch from 0edac04 to e0cdb0d Compare October 21, 2019 08:44
@sp-manuel-jurado sp-manuel-jurado force-pushed the grpc_http1_reverse_bridge branch from 6616b34 to 9b9164e Compare October 21, 2019 09:45
@sp-manuel-jurado
Build fixed.
23d8619 
Signed-off-by: Manuel Jurado <manuel.jurado@socialpoint.es>
@sp-manuel-jurado sp-manuel-jurado force-pushed the grpc_http1_reverse_bridge branch from 9b9164e to 23d8619 Compare October 21, 2019 10:09
@sp-manuel-jurado
Copy link
Copy Markdown
Contributor Author

sp-manuel-jurado commented Oct 21, 2019

Please merge master and fix format to pick up the next free field annotations.

@mattklein123 done /cc @htuch @zuercher @snowp
thx!

mattklein123
mattklein123 approved these changes Oct 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

@sp-manuel-jurado in the future please do not ever force push, just merge master and add commits. It messes up the GH review UI. Thank you!

@mattklein123 mattklein123 merged commit e0e94c5 into envoyproxy:master Oct 21, 2019
@sp-manuel-jurado
Copy link
Copy Markdown
Contributor Author

sp-manuel-jurado commented Oct 22, 2019

Thank you @mattklein123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuercher zuercher zuercher approved these changes

@mattklein123 mattklein123 mattklein123 approved these changes

@snowp snowp Awaiting requested review from snowp

+1 more reviewer

@htuch htuch htuch left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@zuercher zuercher

@snowp snowp

@mattklein123 mattklein123

@htuch htuch

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable http_filters.envoy.filters.http.grpc_http1_reverse_bridge only for one service

5 participants

@sp-manuel-jurado @zuercher @mattklein123 @htuch @snowp