Merge pull request #99 from telekom-mms/feature/avoid-network-device-…

…errors-when-disconnecting Avoid network device errors when disconnecting from VPN
telekom-mms · May 31, 2024 · 542a96b · 542a96b
2 parents eeedb7a + 62024b1
commit 542a96b
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 66 deletions.
diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go
@@ -289,6 +289,10 @@ func (d *Daemon) updateVPNConfigUp(config *vpnconfig.Config) {
 	}
 	d.setStatusIP(ip)
 	d.setStatusDevice(config.Device.Name)
+
+	d.setStatusConnectionState(vpnstatus.ConnectionStateConnected)
+	d.setStatusConnectedAt(time.Now().Unix())
+	log.Info("Daemon configured VPN connection")
 }
 
 // updateVPNConfigDown updates the VPN config for VPN disconnect.
@@ -323,6 +327,8 @@ func (d *Daemon) updateVPNConfigDown() {
 	d.setStatusConnectedAt(0)
 	d.setStatusIP("")
 	d.setStatusDevice("")
+
+	log.Info("Daemon unconfigured VPN connection")
 }
 
 // updateVPNConfig updates the VPN config with config update in client request.
@@ -447,18 +453,6 @@ func (d *Daemon) handleRunnerEvent(e *ocrunner.ConnectEvent) {
 	d.handleRunnerDisconnect()
 }
 
-// handleVPNSetupEvent handles a VPN setup event.
-func (d *Daemon) handleVPNSetupEvent(event *vpnsetup.Event) {
-	switch event.Type {
-	case vpnsetup.EventSetupOK:
-		d.setStatusConnectionState(vpnstatus.ConnectionStateConnected)
-		d.setStatusConnectedAt(time.Now().Unix())
-		log.Info("Daemon configured VPN connection")
-	case vpnsetup.EventTeardownOK:
-		log.Info("Daemon unconfigured VPN connection")
-	}
-}
-
 // handleSleepMonEvent handles a suspend/resume event from SleepMon.
 func (d *Daemon) handleSleepMonEvent(sleep bool) {
 	log.WithField("sleep", sleep).Debug("Daemon handling SleepMon event")
@@ -673,8 +667,8 @@ func (d *Daemon) start() {
 	defer d.stopTND()
 	defer d.vpnsetup.Stop()
 	defer d.server.Stop()
-	defer d.handleRunnerDisconnect() // clean up vpn config
 	defer d.runner.Stop()
+	defer d.handleRunnerDisconnect() // clean up vpn config
 	defer d.dbus.Stop()
 	defer d.server.Shutdown()
 
@@ -697,9 +691,6 @@ func (d *Daemon) start() {
 		case e := <-d.runner.Events():
 			d.handleRunnerEvent(e)
 
-		case e := <-d.vpnsetup.Events():
-			d.handleVPNSetupEvent(e)
-
 		case e := <-d.sleepmon.Events():
 			d.handleSleepMonEvent(e)
 

diff --git a/internal/ocrunner/connect.go b/internal/ocrunner/connect.go
@@ -9,6 +9,7 @@ import (
 	"os/user"
 	"strconv"
 	"strings"
+	"syscall"
 
 	log "github.com/sirupsen/logrus"
 	"github.com/telekom-mms/oc-daemon/pkg/logininfo"
@@ -187,6 +188,10 @@ func (c *Connect) handleConnect(e *ConnectEvent) {
 	parameters = append(parameters, c.config.ExtraArgs...)
 	c.command = execCommand(c.config.OpenConnect, parameters...)
 
+	// run command in own process group so it is not canceled by interrupt
+	// signal sent to daemon
+	c.command.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+
 	// run command, pass login info to stdin
 	b := bytes.NewBufferString(e.login.Cookie)
 	c.command.Stdin = b

diff --git a/internal/vpnsetup/vpnsetup.go b/internal/vpnsetup/vpnsetup.go
@@ -25,17 +25,7 @@ const (
 type command struct {
 	cmd     uint8
 	vpnconf *vpnconfig.Config
-}
-
-// Event types.
-const (
-	EventSetupOK uint8 = iota
-	EventTeardownOK
-)
-
-// Event is a VPNSetup event.
-type Event struct {
-	Type uint8
+	done    chan struct{}
 }
 
 // VPNSetup sets up the configuration of the vpn tunnel that belongs to the
@@ -51,19 +41,10 @@ type VPNSetup struct {
 	ensureClosed chan struct{}
 
 	cmds   chan *command
-	events chan *Event
 	done   chan struct{}
 	closed chan struct{}
 }
 
-// sendEvents sends the event.
-func (v *VPNSetup) sendEvent(event *Event) {
-	select {
-	case v.events <- event:
-	case <-v.done:
-	}
-}
-
 // setupVPNDevice sets up the vpn device with config.
 func setupVPNDevice(ctx context.Context, c *vpnconfig.Config) {
 	// set mtu on device
@@ -437,9 +418,6 @@ func (v *VPNSetup) setup(ctx context.Context, vpnconf *vpnconfig.Config) {
 
 	// ensure VPN config
 	v.startEnsure(ctx, vpnconf)
-
-	// signal setup complete
-	v.sendEvent(&Event{EventSetupOK})
 }
 
 // teardown tears down the vpn configuration.
@@ -451,13 +429,12 @@ func (v *VPNSetup) teardown(ctx context.Context, vpnconf *vpnconfig.Config) {
 	teardownVPNDevice(ctx, vpnconf)
 	v.teardownRouting()
 	v.teardownDNS(ctx, vpnconf)
-
-	// signal teardown complete
-	v.sendEvent(&Event{EventTeardownOK})
 }
 
 // handleCommand handles a command.
 func (v *VPNSetup) handleCommand(ctx context.Context, c *command) {
+	defer close(c.done)
+
 	switch c.cmd {
 	case commandSetup:
 		v.setup(ctx, c.vpnconf)
@@ -486,7 +463,6 @@ func (v *VPNSetup) handleDNSReport(r *dnsproxy.Report) {
 // start starts the VPN setup.
 func (v *VPNSetup) start() {
 	defer close(v.closed)
-	defer close(v.events)
 
 	// create context
 	ctx := context.Background()
@@ -520,23 +496,24 @@ func (v *VPNSetup) Stop() {
 
 // Setup sets the VPN config up.
 func (v *VPNSetup) Setup(vpnconfig *vpnconfig.Config) {
-	v.cmds <- &command{
+	c := &command{
 		cmd:     commandSetup,
 		vpnconf: vpnconfig,
+		done:    make(chan struct{}),
 	}
+	v.cmds <- c
+	<-c.done
 }
 
 // Teardown tears the VPN config down.
 func (v *VPNSetup) Teardown(vpnconfig *vpnconfig.Config) {
-	v.cmds <- &command{
+	c := &command{
 		cmd:     commandTeardown,
 		vpnconf: vpnconfig,
+		done:    make(chan struct{}),
 	}
-}
-
-// Events returns the events channel.
-func (v *VPNSetup) Events() chan *Event {
-	return v.events
+	v.cmds <- c
+	<-c.done
 }
 
 // NewVPNSetup returns a new VPNSetup.
@@ -550,7 +527,6 @@ func NewVPNSetup(
 		splitrtConf:  splitrtConfig,
 
 		cmds:   make(chan *command),
-		events: make(chan *Event),
 		done:   make(chan struct{}),
 		closed: make(chan struct{}),
 	}

diff --git a/internal/vpnsetup/vpnsetup_test.go b/internal/vpnsetup/vpnsetup_test.go
@@ -357,9 +357,8 @@ func TestVPNSetupSetupTeardown(_ *testing.T) {
 	v.Start()
 	vpnconf := vpnconfig.New()
 
-	// setup config and wait for setup event
+	// setup config
 	v.Setup(vpnconf)
-	<-v.Events()
 
 	// send dns report while config is active
 	report := dnsproxy.NewReport("example.com", nil, 300)
@@ -368,9 +367,8 @@ func TestVPNSetupSetupTeardown(_ *testing.T) {
 	// wait long enough for ensure timer
 	time.Sleep(time.Second * 2)
 
-	// teardown config, wait for teardown event
+	// teardown config
 	v.Teardown(vpnconf)
-	<-v.Events()
 
 	// send dns report while config is not active
 	v.dnsProxy.Reports() <- dnsproxy.NewReport("example.com", nil, 300)
@@ -379,16 +377,6 @@ func TestVPNSetupSetupTeardown(_ *testing.T) {
 	v.Stop()
 }
 
-// TestVPNSetupEvents tests Events of VPNSetup.
-func TestVPNSetupEvents(t *testing.T) {
-	v := NewVPNSetup(dnsproxy.NewConfig(), splitrt.NewConfig())
-	want := v.events
-	got := v.Events()
-	if got != want {
-		t.Errorf("got %p, want %p", got, want)
-	}
-}
-
 // TestNewVPNSetup tests NewVPNSetup.
 func TestNewVPNSetup(t *testing.T) {
 	dnsConfig := dnsproxy.NewConfig()
@@ -399,7 +387,6 @@ func TestNewVPNSetup(t *testing.T) {
 		v.dnsProxyConf != dnsConfig ||
 		v.splitrtConf != splitrtConfig ||
 		v.cmds == nil ||
-		v.events == nil ||
 		v.done == nil ||
 		v.closed == nil {
 		t.Errorf("invalid vpn setup")