Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SNI-based dynamic forward proxy filter #10448

Merged
merged 22 commits into from
Apr 16, 2020
Merged

SNI-based dynamic forward proxy filter #10448

merged 22 commits into from
Apr 16, 2020

Conversation

lizan
Copy link
Member

@lizan lizan commented Mar 19, 2020

Signed-off-by: Lizan Zhou [email protected]

Description:
Implement a network filter to do DNS resolution based on SNI. So a TCP connection can be routed to a dynamic_forward_proxy cluster based on that.

Risk Level: Low (extension only)
Testing: manual, unit test, integration test.
Docs Changes: protodoc
Release Notes: Added
Fixes #9916

@repokitteh-read-only
Copy link

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

🐱

Caused by: #10448 was opened by lizan.

see: more, trace.

@lizan lizan requested review from rshriram and Shikugawa March 19, 2020 00:35
@lizan
fix docs
201a4cd 
Signed-off-by: Lizan Zhou <[email protected]>
htuch
htuch reviewed Mar 19, 2020
View reviewed changes
Copy link
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.

API review..

lizan added 3 commits March 20, 2020 00:03
@junr03
Copy link
Member

junr03 commented Mar 20, 2020

Thanks for the API review @htuch. @lizan I have been in meetings all day today, I'll get a first review up by monday :)

@lizan lizan mentioned this pull request Mar 23, 2020
Closed
junr03
junr03 suggested changes Mar 24, 2020
View reviewed changes
Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

First pass of comments!

@junr03
Copy link
Member

junr03 commented Mar 26, 2020

/wait

lizan added 4 commits March 27, 2020 23:36
@lizan
add doc
278f62a 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
fix
7f3ffd0 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
review
6431bba 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
review
eae66c7 
Signed-off-by: Lizan Zhou <[email protected]>
@htuch
Copy link
Member

htuch commented Mar 30, 2020

/lgtm api

junr03
junr03 reviewed Mar 31, 2020
View reviewed changes
Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

mostly lgtm assigning to @mattklein123 so he can take a pass.

@lizan
add todo
3d6fbb2 
Signed-off-by: Lizan Zhou <[email protected]>
mattklein123
mattklein123 requested changes Apr 1, 2020
View reviewed changes
Copy link
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.

Thanks this is super cool. Just a few small comments.

/wait

...nvoy/config/filter/network/sni_dynamic_forward_proxy/v2alpha/sni_dynamic_forward_proxy.proto Outdated
[(validate.rules).message = {required: true}];

oneof port_specifier {
// The port number to connect to the upstream.
Copy link
Member

Choose a reason for hiding this comment

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

Is this flexible enough? Might we need different ports for different hosts at some point? Is that why you did the oneof?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's the reason we have oneof here as pointed out by @htuch. SNI itself dosn't contain port number but we can support original dst port etc.

@htuch
Copy link
Member

htuch commented Apr 7, 2020

Please merge master to pick up #10672. We no longer accept changes to v2 (without explicit exception), so any API modifications should happen in v3. If this PR is adding a new proto, please follow the updated instructions in https://github.com/envoyproxy/envoy/blob/master/api/STYLE.md#adding-an-extension-configuration-to-the-api.

@stale
Copy link

stale bot commented Apr 14, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 14, 2020
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 15, 2020
@lizan
fix docs
8bfec4a 
Signed-off-by: Lizan Zhou <[email protected]>
mattklein123
mattklein123 reviewed Apr 15, 2020
View reviewed changes
Copy link
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.

Thanks LGTM with 1 question. Can you also merge master?

/wait

source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc
return Network::FilterStatus::Continue;
}

// TODO(lizan): implement circuit breaker in SNI dynamic forward proxy like it is in HTTP:
Copy link
Member

Choose a reason for hiding this comment

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

Is there an reason not to do this now since it's pretty easy? Or can this be done as an immediate follow up? This is going to get used and this is a pretty big potential issue I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unlike HTTP in network filters we don't have immediate access to cluster info through filter callbacks. There are plumbings need to happen there.

Copy link
Member

Choose a reason for hiding this comment

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

OK sounds good.

@mattklein123 mattklein123 merged commit bc4ea0a into envoyproxy:master Apr 16, 2020
@lizan lizan deleted the sni_dynamic_forward_proxy branch April 16, 2020 17:10
@stevesloka stevesloka mentioned this pull request May 14, 2020
Closed
@haitch haitch mentioned this pull request Dec 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junr03 junr03 junr03 requested changes

@htuch htuch htuch left review comments

@Shikugawa Shikugawa Shikugawa left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@rshriram rshriram Awaiting requested review from rshriram

Assignees

@junr03 junr03

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dynamic forward proxy support for TLS traffic
5 participants
@lizan @junr03 @htuch @mattklein123 @Shikugawa