Skip to content
Snippets Groups Projects

v5 follow-up: Improve some logging, cleanup code

Merged v5 follow-up: Improve some logging, cleanup code
fixup-v5-mr into main
3 unresolved threads
Merged Pea Nut requested to merge fixup-v5-mr into main
3 unresolved threads

Hey

that's just the things you mentioned and I fixed, before I recognized that you already pushed. Does not neet to be merged quickly/before this release.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • jkito
  • jkito
    jkito @jkito started a thread on an outdated change in commit 42b7db91
    • pkg/vpn/openvpn.go
    497 501 if b.isFailed() {
    498 502 status = Failed
    503 reason = "launcher failed"
    499 504 } else {
    500 status, err := b.getOpenvpnState()
    505 status, err = b.getOpenvpnState()
    501 506 if err != nil {
    502 507 status = Off
    503 }
    504 if status == Off && b.launch.FirewallIsUp() {
    505 return Failed, nil
    508 reason = err.Error()
    509 } else if status == Off && b.launch.FirewallIsUp() {
    510 reason = "OpenVPN is off but firewall blocks traffic"
    511 status = Failed
    512
    • jkito
      jkito @jkito
      Maintainer

      this is changing the behavior of the function, without explaining why its being done, if the goal is to just log reason about the state, then this can be done without changing the behavior by simply adding the new reason variable:

      diff --git a/pkg/vpn/openvpn.go b/pkg/vpn/openvpn.go
index c8c60241..ec2d6b41 100644
--- a/pkg/vpn/openvpn.go
+++ b/pkg/vpn/openvpn.go
@@ -499,17 +499,26 @@ func (b *Bitmask) ReloadFirewall() error {
 // GetStatus returns the VPN status
 func (b *Bitmask) GetStatus() (string, error) {
        status := Off
+       reason := ""
        if b.isFailed() {
                status = Failed
+               reason = "launcher failed"
        } else {
                status, err := b.getOpenvpnState()
                if err != nil {
                        status = Off
+                       reason = err.Error()
                }
                if status == Off && b.launch.FirewallIsUp() {
-                       return Failed, nil
+                       status = Failed
+                       reason = "OpenVPN is off but firewall blocks traffic"
                }
        }
+       log.Trace().
+               Str("status", status).
+               Str("reason", reason).
+               Msg("Current OpenVPN state")
+
        return status, nil
 }

      moreover we don't get much benefit by adding this, as there are logs like the following, which gives the exact event from the management client:

      DBG Got event from OpenVPN process event="INFO: OpenVPN Management Interface Version 5 -- type 'help' for more info"
DBG Got event from OpenVPN process event=AUTH
DBG Got event from OpenVPN process event="ASSIGN_IP: 10.41.0.3"
DBG Got event from OpenVPN process event="CONNECTED: 135.181.37.103"
    • Pea Nut
      Pea Nut @peanut2
      Author Developer

      I like to have the logging here because it explains the internal state we have (independent of the OpenVPN management messages.

      I think the comment above the function explains what the function/state/commit does: // Return the current state of the OpenVPN, err is not nil if we can not get the current state // status is one of our constants: On, Off, Starting, Stopping, Failed

      I like it much better than the version before, which just always returned nil as error... I rephrased the commit message a bit.

    • jkito
      jkito @jkito
      Maintainer

      I like it much better than the version before, which just always returned nil as error... I rephrased the commit message a bit.

      have you verified that it doesn't introduce any regression due to this change in behavior?

    • Pea Nut changed this line in version 7 of the diff

      changed this line in version 7 of the diff

    • Pea Nut
      Pea Nut @peanut2
      Author Developer

      No. I dropped the commit. If we return an error back to Reconnect function, the state is broken. This is because we return in Reconnect() in case of an error. But then things like b.statusCh <- Stopping are skipped which has impact on the state.
      Also: Reconnect() is only called as a go routine. So the return value does not get checked/errors not logged.

      Edited by Pea Nut
    • Please register or sign in to reply
  • Pea Nut removed Review Ready label

    removed Review Ready label

  • Pea Nut resolved all threads

    resolved all threads

  • Pea Nut added 5 commits

    added 5 commits

    • 5bbf9ec0...a4374993 - 2 commits from branch main
    • 3b309d07 - Improve logging and add comments to OpenVPN management handling
    • 4013e181 - Log fail reason and change error handling in Bitmask.GetStatus
    • 16e852d2 - Fix comment in Bonafide.GetBestLocation

    Compare with previous version

    Toggle commit list
  • Pea Nut added 30m of time spent at 2024-06-30

    added 30m of time spent at 2024-06-30

  • Pea Nut added 3 commits

    added 3 commits

    • 3d9c839b - Improve logging and add comments to OpenVPN management handling
    • 282c778e - Log fail reason and change error handling in Bitmask.GetStatus
    • 48dcd4c6 - Fix comment in Bonafide.GetBestLocation

    Compare with previous version

  • Pea Nut added 4 commits

    added 4 commits

    • d246808f - 1 commit from branch main
    • 34b35281 - Improve logging and add comments to OpenVPN management handling
    • 2adb7d6d - Log fail reason and change error handling in Bitmask.GetStatus
    • 3306755c - Fix comment in Bonafide.GetBestLocation

    Compare with previous version

    Toggle commit list
  • Pea Nut added time estimate of 2h

    added time estimate of 2h

  • Pea Nut added 6 commits

    added 6 commits

    • 3306755c...4ee5fba6 - 3 commits from branch main
    • c2b3ab58 - Improve logging and add comments to OpenVPN management handling
    • 88cdd57a - Log fail reason and change error handling in Bitmask.GetStatus
    • ca98e23d - Fix comment in Bonafide.GetBestLocation

    Compare with previous version

    Toggle commit list
  • jkito
    jkito @jkito started a thread on an outdated change in commit 88cdd57a
    • pkg/vpn/openvpn.go
    498 err = nil
    499 reason := ""
    500
    497 501 if b.isFailed() {
    498 502 status = Failed
    503 reason = "launcher failed"
    499 504 } else {
    500 status, err := b.getOpenvpnState()
    505 status, err = b.getOpenvpnState()
    501 506 if err != nil {
    502 507 status = Off
    503 }
    504 if status == Off && b.launch.FirewallIsUp() {
    505 return Failed, nil
    508 reason = err.Error()
    509 } else if status == Off && b.launch.FirewallIsUp() {
    • jkito
      jkito @jkito
      Maintainer

      is else if better then if i don't understand why this change is introduced, it just increases the diff imo and nothing else

      Edited by jkito
    • Pea Nut
      Pea Nut @peanut2
      Author Developer

      I try to improve the ugly code here. GetStatus returns always nil for the error!? If b.getOpenvpnState returns an error, we just set status = Off without returning!? Now the error handling is more explicit. And it logs. There is also a comment above the function. I use else if here because I need to check that err==nil.

    • Pea Nut changed this line in version 8 of the diff

      changed this line in version 8 of the diff

    • Please register or sign in to reply
  • Pea Nut added 10 commits

    added 10 commits

    • ca98e23d...218c8beb - 7 commits from branch main
    • 61bc0b4b - Improve logging and add comments to OpenVPN management handling
    • 36d04dde - Log fail reason and change error handling in Bitmask.GetStatus
    • a792900a - Fix comment in Bonafide.GetBestLocation

    Compare with previous version

    Toggle commit list
  • Pea Nut added 1 commit

    added 1 commit

    • 51032c6e - Fix comment in Bonafide.GetBestLocation

    Compare with previous version

  • Pea Nut added 30m of time spent at 2024-07-10

    added 30m of time spent at 2024-07-10

  • jkito added 17 commits

    added 17 commits

    • 51032c6e...3b8f0604 - 15 commits from branch main
    • adaffcbf - Improve logging and add comments to OpenVPN management handling
    • 12a4c94f - Fix comment in Bonafide.GetBestLocation

    Compare with previous version

  • jkito
    jkito @jkito started a thread on the diff
    • pkg/vpn/status.go
    53 65 eventCh := make(chan management.Event, 10)
    54 log.Info().Msg("New connection into the management")
    66 log.Debug().
    67 Str("endpoint", listenAddr).
    68 Msg("OpenVPN process connected to our management backend")
    55 69 b.managementClient = conn.Open(eventCh)
    56 70 b.managementClient.SendPassword(b.launch.MngPass)
    57 71 b.managementClient.SetStateEvents(true)
    58 72 b.eventHandler(eventCh)
    59 73 }
    74
    60 75 err := management.ListenAndServe(
    61 fmt.Sprintf("%s:%s", openvpnManagementAddr, openvpnManagementPort),
    76 listenAddr,
    62 77 management.IncomingConnHandlerFunc(newConnection),
    63 78 )
  • jkito approved this merge request

    approved this merge request

  • jkito
    jkito @jkito
    Maintainer

    Finally got to testing this on all the platforms!

  • jkito merged

    merged

    • Please register or sign in to reply