Close on already registered vici errors (#30)

Currently if strong-duckling fails to read an event request the event handler is
not unregistered as the methods return the request error. This can lead to a
failure state where following stat collections try to register an event handler
but fails as it is already registered as it was not unregistered before.
Strong-duckling is unable to recover from this state.

This change moves unregistration of event handlers into a defered function to
ensure that if registration succeeds we will always attempt to unregister.

RegisterEvent guarantees that it does not keep the handler registered in memory
if any part of the vici registration process fails, which gives us the guarantee
that we will never get into the mentioned bad state again.

Signed-off-by: Bjørn Sørensen <[email protected]>

internal/vici/initiate.go

@@ -15,6 +15,13 @@ func (c *ClientConn) Initiate(child string, ike string, logger func(fields map[s
 		request["ike"] = ike
 	}
 
+	err = c.RegisterEvent("control-log", func(response map[string]interface{}) {
+		logger(response)
+	})
+	if err != nil {
+		return fmt.Errorf("error registering control-log event: %w", err)
+	}
+
 	defer func() {
 		unregisterErr := c.UnregisterEvent("control-log")
 		if unregisterErr != nil {
@@ -26,13 +33,6 @@ func (c *ClientConn) Initiate(child string, ike string, logger func(fields map[s
 		}
 	}()
 
-	err = c.RegisterEvent("control-log", func(response map[string]interface{}) {
-		logger(response)
-	})
-	if err != nil {
-		return fmt.Errorf("error registering control-log event: %w", err)
-	}
-
 	msg, err := c.Request("initiate", request)
 	if msg["success"] != "yes" {
 		return fmt.Errorf("initiate unsuccessful: %v", msg["errmsg"])

internal/vici/listConns.go

@@ -6,10 +6,9 @@ import (
 	"strings"
 )
 
-func (c *ClientConn) ListConns(ike string) (map[string]IKEConf, error) {
-	conns := map[string]IKEConf{}
+func (c *ClientConn) ListConns(ike string) (conns map[string]IKEConf, err error) {
+	conns = map[string]IKEConf{}
 	var eventErr error
-	var err error
 
 	err = c.RegisterEvent("list-conn", func(response map[string]interface{}) {
 		connsMap, err := mapConnections(response)
@@ -24,6 +23,18 @@ func (c *ClientConn) ListConns(ike string) (map[string]IKEConf, error) {
 	if err != nil {
 		return nil, fmt.Errorf("error registering list-conn event: %w", err)
 	}
+
+	defer func() {
+		unregisterErr := c.UnregisterEvent("list-conn")
+		if unregisterErr != nil {
+			if err == nil {
+				err = unregisterErr
+				return
+			}
+			err = fmt.Errorf("unregister list-conn failed: %v: %w", unregisterErr, err)
+		}
+	}()
+
 	if eventErr != nil {
 		return nil, eventErr
 	}
@@ -38,11 +49,6 @@ func (c *ClientConn) ListConns(ike string) (map[string]IKEConf, error) {
 		return nil, fmt.Errorf("error requesting list-conns: %w", err)
 	}
 
-	err = c.UnregisterEvent("list-conn")
-	if err != nil {
-		return nil, fmt.Errorf("error unregistering list-conns event: %w", err)
-	}
-
 	return conns, nil
 }
 

internal/vici/listSas.go

@@ -1,6 +1,7 @@
 package vici
 
 import (
+	"fmt"
 	"strconv"
 )
 
@@ -133,11 +134,11 @@ func (s *ChildSA) GetBytesOut() uint64 {
 // To be simple, list all clients that are connecting to this server .
 // A client is a sa.
 // Lists currently active IKE_SAs
-func (c *ClientConn) ListSas(ike string, ike_id string) (map[string]IkeSa, error) {
-	sas := map[string]IkeSa{}
+func (c *ClientConn) ListSas(ike string, ike_id string) (sas map[string]IkeSa, err error) {
+	sas = map[string]IkeSa{}
 	var eventErr error
 	//register event
-	err := c.RegisterEvent("list-sa", func(response map[string]interface{}) {
+	err = c.RegisterEvent("list-sa", func(response map[string]interface{}) {
 		sa := map[string]IkeSa{}
 		err := convertFromGeneral(response, &sa)
 		if err != nil {
@@ -151,6 +152,18 @@ func (c *ClientConn) ListSas(ike string, ike_id string) (map[string]IkeSa, error
 	if err != nil {
 		return nil, err
 	}
+
+	defer func() {
+		unregisterErr := c.UnregisterEvent("list-sa")
+		if unregisterErr != nil {
+			if err == nil {
+				err = unregisterErr
+				return
+			}
+			err = fmt.Errorf("unregister list-sa failed: %v: %w", unregisterErr, err)
+		}
+	}()
+
 	if eventErr != nil {
 		return nil, eventErr
 	}
@@ -166,9 +179,5 @@ func (c *ClientConn) ListSas(ike string, ike_id string) (map[string]IkeSa, error
 	if err != nil {
 		return nil, err
 	}
-	err = c.UnregisterEvent("list-sa")
-	if err != nil {
-		return nil, err
-	}
 	return sas, nil
 }