Repository: jkroepke/openvpn-auth-oauth2
Pull request #109 (2024-01-03 21:17:10)

try fix tests
#### What this PR does / why we need it

#### Which issue this PR fixes

*(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

- fixes #

#### Special notes for your reviewer

#### Particularly user-facing changes

#### Checklist

Complete these before marking the PR as `ready to review`:

<!-- [Place an '[x]' (no spaces) in all applicable fields.] -->

- [ ] [DCO](https://github.com/prometheus-community/helm-charts/blob/main/CONTRIBUTING.md#sign-off-your-work) signed
- [ ] The PR title has a summary of the changes
- [ ] The PR body has a summary to reflect any significant (and particularly user-facing) changes introduced by this PR


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/jkroepke/openvpn-auth-oauth2/pull/109?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jan-Otto+Kr%C3%B6pke) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`72e8dcf`)](https://app.codecov.io/gh/jkroepke/openvpn-auth-oauth2/commit/72e8dcf26d94a7c46017e84a874b29f6e417f417?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jan-Otto+Kr%C3%B6pke) 75.62% compared to head [(`aff5dfa`)](https://app.codecov.io/gh/jkroepke/openvpn-auth-oauth2/pull/109?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jan-Otto+Kr%C3%B6pke) 75.76%.
> Report is 1 commits behind head on main.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
+ Coverage   75.62%   75.76%   +0.14%     
==========================================
  Files          39       39              
  Lines        2084     2084              
==========================================
+ Hits         1576     1579       +3     
+ Misses        433      431       -2     
+ Partials       75       74       -1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/jkroepke/openvpn-auth-oauth2/pull/109?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jan-Otto+Kr%C3%B6pke).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jan-Otto+Kr%C3%B6pke).


Patch:
diff --git a/internal/openvpn/handler.go b/internal/openvpn/handler.go
index 1be1b80..10ea861 100644
--- a/internal/openvpn/handler.go
+++ b/internal/openvpn/handler.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"io"
 	"strings"
 
 	"github.com/jkroepke/openvpn-auth-oauth2/internal/openvpn/connection"
@@ -74,7 +75,20 @@ func (c *Client) handleMessages() {
 
 	for {
 		if err = c.readMessage(&buf); err != nil {
-			c.errCh <- fmt.Errorf("readMessage: %w", err)
+			if errors.Is(err, io.EOF) {
+				c.logger.Warn("OpenVPN management interface connection terminated")
+				c.Shutdown()
+
+				return
+			}
+
+			c.shutdownMu.Lock()
+
+			if !c.closed {
+				c.errCh <- fmt.Errorf("error reading bytes: %w", err)
+			}
+
+			c.shutdownMu.Unlock()
 
 			return
 		}
diff --git a/internal/openvpn/main.go b/internal/openvpn/main.go
index 2c4092b..d641631 100644
--- a/internal/openvpn/main.go
+++ b/internal/openvpn/main.go
@@ -30,8 +30,9 @@ func NewClient(logger *slog.Logger, conf config.Config, oauth2Client *oauth2.Pro
 		logger: logger,
 		oauth2: oauth2Client,
 
-		closed: false,
-		mu:     sync.Mutex{},
+		closed:     false,
+		connMu:     sync.Mutex{},
+		shutdownMu: sync.Mutex{},
 
 		commandsBuffer: commandsBuffer,
 
@@ -39,7 +40,6 @@ func NewClient(logger *slog.Logger, conf config.Config, oauth2Client *oauth2.Pro
 		clientsCh:         make(chan connection.Client, 10),
 		commandResponseCh: make(chan string, 10),
 		commandsCh:        make(chan string, 10),
-		shutdownCh:        make(chan struct{}, 1),
 	}
 }
 
@@ -78,29 +78,21 @@ func (c *Client) Connect() error {
 		return err
 	}
 
-	for {
-		select {
-		case err := <-c.errCh:
-			if errors.Is(err, io.EOF) {
-				return ErrConnectionTerminated
-			}
+	err = <-c.errCh
 
-			c.close()
+	c.Shutdown()
 
-			if err != nil {
-				return fmt.Errorf("OpenVPN management error: %w", err)
-			}
-
-			return nil
-		case <-c.shutdownCh:
-			c.close()
-
-			return nil
-		}
+	if err != nil {
+		return fmt.Errorf("OpenVPN management error: %w", err)
 	}
+
+	return nil
 }
 
 func (c *Client) setupConnection() error {
+	c.connMu.Lock()
+	defer c.connMu.Unlock()
+
 	var err error
 
 	switch c.conf.OpenVpn.Addr.Scheme {
@@ -174,13 +166,25 @@ func (c *Client) checkClientSsoCapabilities(logger *slog.Logger, client connecti
 
 // Shutdown shutdowns the client connection.
 func (c *Client) Shutdown() {
-	c.mu.Lock()
-	defer c.mu.Unlock()
+	c.shutdownMu.Lock()
+	defer c.shutdownMu.Unlock()
 
-	if !c.closed {
-		c.logger.Info("shutdown OpenVPN management connection")
-		c.shutdownCh <- struct{}{}
+	c.connMu.Lock()
+	defer c.connMu.Unlock()
+
+	if c.closed {
+		return
 	}
+
+	c.closed = true
+	c.logger.Info("shutdown OpenVPN management connection")
+
+	if c.conn != nil {
+		_ = c.conn.Close()
+	}
+
+	close(c.commandsCh)
+	close(c.errCh)
 }
 
 // SendCommand passes command to a given connection (adds logging and EOL character) and returns the response.
@@ -266,15 +270,3 @@ func (c *Client) isMessageLineEOF(line []byte) bool {
 		bytes.HasPrefix(line, []byte(">INFO:")) ||
 		bytes.HasPrefix(line, []byte(">NOTIFY:"))
 }
-
-func (c *Client) close() {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-
-	if !c.closed {
-		c.closed = true
-
-		_ = c.conn.Close()
-		close(c.commandsCh)
-	}
-}
diff --git a/internal/openvpn/types.go b/internal/openvpn/types.go
index 589d18a..e3f6921 100644
--- a/internal/openvpn/types.go
+++ b/internal/openvpn/types.go
@@ -19,8 +19,9 @@ type Client struct {
 	logger  *slog.Logger
 	oauth2  *oauth2.Provider
 
-	mu     sync.Mutex
-	closed bool
+	shutdownMu sync.Mutex
+	connMu     sync.Mutex
+	closed     bool
 
 	commandsBuffer bytes.Buffer
 
@@ -28,5 +29,4 @@ type Client struct {
 	commandResponseCh chan string
 	commandsCh        chan string
 	errCh             chan error
-	shutdownCh        chan struct{}
 }


Test patch:
diff --git a/cmd/root_test.go b/cmd/root_test.go
index 5684b88..be59caa 100644
--- a/cmd/root_test.go
+++ b/cmd/root_test.go
@@ -20,8 +20,6 @@ func TestExecuteVersion(t *testing.T) {
 	t.Parallel()
 
 	var buf bytes.Buffer
-
-	buf.Grow(16 << 20)
 	_ = io.Writer(&buf)
 
 	returnCode := cmd.Execute([]string{"", "--version"}, &buf, "version", "commit", "date")
@@ -89,8 +87,6 @@ func TestExecuteConfigInvalid(t *testing.T) {
 
 			returnCode := cmd.Execute(tt.args, &buf, "version", "commit", "date")
 
-			time.Sleep(100 * time.Millisecond)
-
 			assert.Equal(t, 1, returnCode, buf.String())
 			assert.Contains(t, buf.String(), tt.err)
 		})
@@ -111,7 +107,6 @@ func TestExecuteConfigFileFound(t *testing.T) { //nolint: paralleltest
 		conn, err := managementInterface.Accept()
 		require.NoError(t, err)
 
-		defer conn.Close()
 		reader := bufio.NewReader(conn)
 
 		testutils.SendLine(t, conn, ">INFO:OpenVPN Management Interface Version 5 -- type 'help' for more info\r\n")
@@ -145,13 +140,9 @@ func TestExecuteConfigFileFound(t *testing.T) { //nolint: paralleltest
 	}
 
 	var buf bytes.Buffer
-
-	buf.Grow(16 << 20)
 	_ = io.Writer(&buf)
 
 	returnCode := cmd.Execute(args, &buf, "version", "commit", "date")
 
-	time.Sleep(100 * time.Millisecond)
-
 	assert.Equal(t, 0, returnCode, buf.String())
 }
diff --git a/internal/oauth2/handler_test.go b/internal/oauth2/handler_test.go
index 09f3b78..f07cf54 100644
--- a/internal/oauth2/handler_test.go
+++ b/internal/oauth2/handler_test.go
@@ -292,7 +292,6 @@ func TestHandler(t *testing.T) {
 			wg.Add(2)
 			go func() {
 				defer wg.Done()
-				defer client.Shutdown()
 
 				conn, err := managementInterface.Accept()
 				require.NoError(t, err)
diff --git a/internal/openvpn/main_test.go b/internal/openvpn/main_test.go
index f3f8624..7e7d0bd 100644
--- a/internal/openvpn/main_test.go
+++ b/internal/openvpn/main_test.go
@@ -207,7 +207,6 @@ func TestClientFull(t *testing.T) {
 				require.NoError(t, err)
 
 				defer conn.Close()
-				defer client.Shutdown()
 
 				reader := bufio.NewReader(conn)
 
@@ -263,14 +262,14 @@ func TestClientFull(t *testing.T) {
 			if tt.err != nil {
 				require.Error(t, err)
 				assert.Equal(t, tt.err.Error(), err.Error())
-
-				client.Shutdown()
 			} else {
 				wg.Wait()
-				if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, openvpn.ErrConnectionTerminated) {
+				if err != nil && !errors.Is(err, io.EOF) {
 					require.NoError(t, err)
 				}
 			}
+
+			client.Shutdown()
 		})
 	}
 }