Enable add/remove routes after server starts by yangchenyun · Pull Request #15 · inetaf/tcpproxy

yangchenyun · 2018-04-07T07:33:10Z

Use a map instead of slice to hold routes for each configuration
Issue a serial number for the route added in order to remove them later
Ensure Add/RemoveRoute is thread-safe by using a lock to guard write to the routes, because each write to router has its own serial number, racing condition only exists during registration. The lock should have minimal affect during serving.

Also added two test helpers to simplify and complete existing tests:

func testRouteToBackend(t *testing.T, front net.Listener, back net.Listener, msg string) 
func testNotRouteToBackend(t *testing.T, front net.Listener, back net.Listener, msg string) <-chan bool

- Use a map instead of slice to hold routes for each configuration - Use a lock to guard read / write to the routes - Issue a serial number for the route added in order to remove them later - Add/RemoveRoute is thread-safe

- returns null routeId to indicate a failed registration

googlebot · 2018-04-07T07:33:15Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

yangchenyun · 2018-04-07T07:36:27Z

I signed it!

googlebot · 2018-04-07T07:36:30Z

CLAs look good, thanks!

- Add the test case for existing backends - Add the test case for add/remove route after server starts

bradfitz · 2018-04-10T19:56:10Z

All the renamings in this PR obscure the actual change you're trying to make.

Can you split your changes into two parts, or refrain from renames for now?

bradfitz · 2018-04-10T19:58:12Z

tcpproxy.go

+// not found, this is an no-op.
+//
+// Both AddRoute and RemoveRoute is go-routine safe.
+func (p *Proxy) RemoveRoute(ipPort string, routeId int) (err error) {


un-name this, per https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters

Done. Thanks.

bradfitz · 2018-04-10T19:58:56Z

tcpproxy.go

 // The ipPort is any valid net.Listen TCP address.
-func (p *Proxy) AddRoute(ipPort string, dest Target) {
-	p.addRoute(ipPort, fixedTarget{dest})
+func (p *Proxy) AddRoute(ipPort string, dest Target) (routeId int) {


rebase to fix the case of these originally, rather than fixing it in a later commit in the series.

bradfitz · 2018-04-10T19:59:26Z

tcpproxy.go

 	stopACME    bool     // if true, AddSNIRoute doesn't add targets to acmeTargets.
 }

+func NewConfig() (cfg *config) {


needs docs.

and unname result parameter per https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters

but actually I'd just delete this and let the zero value of Config be useful. You can initialize the routes map as needed.

Removed the constructor.

bradfitz · 2018-04-10T20:00:21Z

tcpproxy.go

 // config contains the proxying state for one listener.
 type config struct {
-	routes      []route
+	sync.Mutex  // protect r/w of routes


comment should be like:

mu sync.Mutex // guards following x int y int

and blank line before stuff that is no longer guarded by it.

yangchenyun · 2018-04-11T01:41:24Z

I separated the test refactoring in a #16, and I would do rebase and squash after that is check in.

georgmu · 2018-09-06T09:56:55Z

Adding routes only works for an already registered ipPort. The stuff in done in tcpproxy.Start() - looping over configs and calling p.netListen() is not done again.

For removal of a route, it would be nice to cancel onging connections for this route. This would require a map or list of ongoing connections for a route

Inspired by inetaf#15

yangchenyun added 4 commits April 6, 2018 22:17

Implemented Proxy.RemoveRoute

f89ab2b

- Use a map instead of slice to hold routes for each configuration - Use a lock to guard read / write to the routes - Issue a serial number for the route added in order to remove them later - Add/RemoveRoute is thread-safe

Propagate the new addRoute API to SNI and HTTP host

7c3486f

Extract a test helper to correct front -> back msg passing

26f8927

Handle the case when adding route after server starts

cfba6a1

- returns null routeId to indicate a failed registration

Added the test helper to assert no message reached backend

cfffc79

- Add the test case for existing backends - Add the test case for add/remove route after server starts

yangchenyun force-pushed the master branch from 3a757bb to cfffc79 Compare April 7, 2018 08:58

Correct linter warnings

a64c39c

bradfitz requested changes Apr 10, 2018

View reviewed changes

Review fixes

08e1ca7

yangchenyun force-pushed the master branch from a21b1c1 to 08e1ca7 Compare May 24, 2018 03:42

yangchenyun added 3 commits May 24, 2018 20:21

use sync.Map to save routes

d41a005

do not create the config during route deletion

9a40e9d

added support for default handler

2041ee5

patdowney pushed a commit to patdowney/tcpproxy that referenced this pull request Mar 1, 2020

Add support allow routes to be added after the tcpproxy is running.

335dbdc

Inspired by inetaf#15

Conversation

yangchenyun commented Apr 7, 2018

Uh oh!

googlebot commented Apr 7, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

yangchenyun commented Apr 7, 2018

Uh oh!

googlebot commented Apr 7, 2018

Uh oh!

bradfitz commented Apr 10, 2018

Uh oh!

bradfitz Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

yangchenyun Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

bradfitz Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

bradfitz Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

yangchenyun Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

bradfitz Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

yangchenyun Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

yangchenyun commented Apr 11, 2018

Uh oh!

georgmu commented Sep 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants