From 4803f18fae1e39d200d98f09e445a97ccd6f5526 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Wed, 4 Aug 2021 15:56:59 +0900 Subject: [PATCH 1/4] Revert "port/builtin: RemovePort() block until conn is closed" This reverts commit 72fdd244d9088102ed28f6294f3677ab216f8b1d. `routineStopCh <- struct{}{}` in `parent.go` can be delivered to the `copyConnToChild(c, socketPath, spec, stopCh)` routine in `tcp.go`, so this does not work as expected and has to be reverted. Signed-off-by: Akihiro Suda --- pkg/port/builtin/parent/parent.go | 10 ++-------- pkg/port/builtin/parent/tcp/tcp.go | 7 ++----- pkg/port/builtin/parent/udp/udp.go | 3 +-- 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/pkg/port/builtin/parent/parent.go b/pkg/port/builtin/parent/parent.go index abd2c5e2..2895a8f0 100644 --- a/pkg/port/builtin/parent/parent.go +++ b/pkg/port/builtin/parent/parent.go @@ -12,7 +12,6 @@ import ( "strings" "sync" "syscall" - "time" "github.com/pkg/errors" @@ -141,13 +140,8 @@ func (d *driver) AddPort(ctx context.Context, spec port.Spec) (*port.Status, err } routineStopCh := make(chan struct{}) routineStop := func() error { - routineStopCh <- struct{}{} - select { - case <-routineStopCh: - case <-time.After(5 * time.Second): - return errors.New("stop timeout after 5 seconds") - } - return nil + close(routineStopCh) + return nil // FIXME } switch spec.Proto { case "tcp", "tcp4", "tcp6": diff --git a/pkg/port/builtin/parent/tcp/tcp.go b/pkg/port/builtin/parent/tcp/tcp.go index dcc1068f..7a7a167f 100644 --- a/pkg/port/builtin/parent/tcp/tcp.go +++ b/pkg/port/builtin/parent/tcp/tcp.go @@ -12,7 +12,7 @@ import ( "github.com/rootless-containers/rootlesskit/pkg/port/builtin/msg" ) -func Run(socketPath string, spec port.Spec, stopCh chan struct{}, logWriter io.Writer) error { +func Run(socketPath string, spec port.Spec, stopCh <-chan struct{}, logWriter io.Writer) error { ln, err := net.Listen(spec.Proto, net.JoinHostPort(spec.ParentIP, strconv.Itoa(spec.ParentPort))) if err != nil { fmt.Fprintf(logWriter, "listen: %v\n", err) @@ -31,10 +31,7 @@ func Run(socketPath string, spec port.Spec, stopCh chan struct{}, logWriter io.W } }() go func() { - defer func() { - ln.Close() - close(stopCh) - }() + defer ln.Close() for { select { case c, ok := <-newConns: diff --git a/pkg/port/builtin/parent/udp/udp.go b/pkg/port/builtin/parent/udp/udp.go index f20721bc..0080dd22 100644 --- a/pkg/port/builtin/parent/udp/udp.go +++ b/pkg/port/builtin/parent/udp/udp.go @@ -13,7 +13,7 @@ import ( "github.com/rootless-containers/rootlesskit/pkg/port/builtin/parent/udp/udpproxy" ) -func Run(socketPath string, spec port.Spec, stopCh chan struct{}, logWriter io.Writer) error { +func Run(socketPath string, spec port.Spec, stopCh <-chan struct{}, logWriter io.Writer) error { addr, err := net.ResolveUDPAddr(spec.Proto, net.JoinHostPort(spec.ParentIP, strconv.Itoa(spec.ParentPort))) if err != nil { return err @@ -51,7 +51,6 @@ func Run(socketPath string, spec port.Spec, stopCh chan struct{}, logWriter io.W case <-stopCh: // udpp.Close closes ln as well udpp.Close() - close(stopCh) return } } From 1f444f0c4ee2300c8c2bd6576b19eac08a4055e5 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Wed, 4 Aug 2021 16:15:34 +0900 Subject: [PATCH 2/4] port/builtin: RemovePort() block until conn is closed, take 2 Alternative to reverted PR 262 Signed-off-by: Akihiro Suda --- pkg/port/builtin/parent/parent.go | 31 +++++++++++++++++++++++------- pkg/port/builtin/parent/tcp/tcp.go | 7 +++++-- pkg/port/builtin/parent/udp/udp.go | 4 +++- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/pkg/port/builtin/parent/parent.go b/pkg/port/builtin/parent/parent.go index 2895a8f0..c6eecc82 100644 --- a/pkg/port/builtin/parent/parent.go +++ b/pkg/port/builtin/parent/parent.go @@ -12,6 +12,7 @@ import ( "strings" "sync" "syscall" + "time" "github.com/pkg/errors" @@ -41,7 +42,7 @@ func NewDriver(logWriter io.Writer, stateDir string) (port.ParentDriver, error) socketPath: socketPath, childReadyPipePath: childReadyPipePath, ports: make(map[int]*port.Status, 0), - stoppers: make(map[int]func() error, 0), + stoppers: make(map[int]func(context.Context) error, 0), nextID: 1, } return &d, nil @@ -53,7 +54,7 @@ type driver struct { childReadyPipePath string mu sync.Mutex ports map[int]*port.Status - stoppers map[int]func() error + stoppers map[int]func(context.Context) error nextID int } @@ -138,16 +139,27 @@ func (d *driver) AddPort(ctx context.Context, spec port.Spec) (*port.Status, err if err != nil { return nil, err } + // NOTE: routineStopCh is close-only channel. Do not send any data. + // See commit 4803f18fae1e39d200d98f09e445a97ccd6f5526 `Revert "port/builtin: RemovePort() block until conn is closed"` routineStopCh := make(chan struct{}) - routineStop := func() error { + routineStoppedCh := make(chan error) + routineStop := func(ctx context.Context) error { close(routineStopCh) - return nil // FIXME + select { + case stoppedResult, stoppedResultOk := <-routineStoppedCh: + if stoppedResultOk { + return stoppedResult + } + return errors.New("routineStoppedCh was closed without sending data?") + case <-ctx.Done(): + return errors.Wrap(err, "timed out while waiting for routineStoppedCh after closing routineStopCh") + } } switch spec.Proto { case "tcp", "tcp4", "tcp6": - err = tcp.Run(d.socketPath, spec, routineStopCh, d.logWriter) + err = tcp.Run(d.socketPath, spec, routineStopCh, routineStoppedCh, d.logWriter) case "udp", "udp4", "udp6": - err = udp.Run(d.socketPath, spec, routineStopCh, d.logWriter) + err = udp.Run(d.socketPath, spec, routineStopCh, routineStoppedCh, d.logWriter) default: // NOTREACHED return nil, errors.New("spec was not validated?") @@ -188,7 +200,12 @@ func (d *driver) RemovePort(ctx context.Context, id int) error { if !ok { return errors.Errorf("unknown id: %d", id) } - err := stop() + if _, ok := ctx.Deadline(); !ok { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, 5*time.Second) + defer cancel() + } + err := stop(ctx) delete(d.stoppers, id) delete(d.ports, id) return err diff --git a/pkg/port/builtin/parent/tcp/tcp.go b/pkg/port/builtin/parent/tcp/tcp.go index 7a7a167f..32c71446 100644 --- a/pkg/port/builtin/parent/tcp/tcp.go +++ b/pkg/port/builtin/parent/tcp/tcp.go @@ -12,7 +12,7 @@ import ( "github.com/rootless-containers/rootlesskit/pkg/port/builtin/msg" ) -func Run(socketPath string, spec port.Spec, stopCh <-chan struct{}, logWriter io.Writer) error { +func Run(socketPath string, spec port.Spec, stopCh <-chan struct{}, stoppedCh chan error, logWriter io.Writer) error { ln, err := net.Listen(spec.Proto, net.JoinHostPort(spec.ParentIP, strconv.Itoa(spec.ParentPort))) if err != nil { fmt.Fprintf(logWriter, "listen: %v\n", err) @@ -31,7 +31,10 @@ func Run(socketPath string, spec port.Spec, stopCh <-chan struct{}, logWriter io } }() go func() { - defer ln.Close() + defer func() { + stoppedCh <- ln.Close() + close(stoppedCh) + }() for { select { case c, ok := <-newConns: diff --git a/pkg/port/builtin/parent/udp/udp.go b/pkg/port/builtin/parent/udp/udp.go index 0080dd22..67062117 100644 --- a/pkg/port/builtin/parent/udp/udp.go +++ b/pkg/port/builtin/parent/udp/udp.go @@ -13,7 +13,7 @@ import ( "github.com/rootless-containers/rootlesskit/pkg/port/builtin/parent/udp/udpproxy" ) -func Run(socketPath string, spec port.Spec, stopCh <-chan struct{}, logWriter io.Writer) error { +func Run(socketPath string, spec port.Spec, stopCh <-chan struct{}, stoppedCh chan error, logWriter io.Writer) error { addr, err := net.ResolveUDPAddr(spec.Proto, net.JoinHostPort(spec.ParentIP, strconv.Itoa(spec.ParentPort))) if err != nil { return err @@ -51,6 +51,8 @@ func Run(socketPath string, spec port.Spec, stopCh <-chan struct{}, logWriter io case <-stopCh: // udpp.Close closes ln as well udpp.Close() + stoppedCh <- nil + close(stoppedCh) return } } From 87d443683ac1e8aba4110b8081f15aaae432aaa2 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Wed, 4 Aug 2021 16:16:19 +0900 Subject: [PATCH 3/4] v0.14.4 Signed-off-by: Akihiro Suda --- pkg/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/version/version.go b/pkg/version/version.go index bc4bbe9e..4fe558f1 100644 --- a/pkg/version/version.go +++ b/pkg/version/version.go @@ -1,3 +1,3 @@ package version -const Version = "0.14.3+dev" +const Version = "0.14.4" From dc055ac5826136e5142a79c2c5b124b07ca8ea0e Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Wed, 4 Aug 2021 16:16:26 +0900 Subject: [PATCH 4/4] v0.14.4+dev Signed-off-by: Akihiro Suda --- pkg/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/version/version.go b/pkg/version/version.go index 4fe558f1..b7fc0cd4 100644 --- a/pkg/version/version.go +++ b/pkg/version/version.go @@ -1,3 +1,3 @@ package version -const Version = "0.14.4" +const Version = "0.14.4+dev"