BUG/MAJOR: reload runtime client if the configuration is changed with global endpoint

vgramer · mjuraga · commit 8900180d10b8 · 2023-07-27T11:54:26.000+02:00
Signed-off-by: Vincent Gramer &lt;vgramer@haproxy.com&gt;
diff --git a/client-native/cn.go b/client-native/cn.go
@@ -5,6 +5,10 @@ import (
 	"fmt"
 	"strconv"
 	"sync"
+	"time"
+
+	clientnative "github.com/haproxytech/client-native/v5"
+	"github.com/haproxytech/client-native/v5/models"
 
 	"github.com/haproxytech/client-native/v5/configuration"
 	configuration_options "github.com/haproxytech/client-native/v5/configuration/options"
@@ -155,3 +159,66 @@ func ConfigureRuntimeClient(ctx context.Context, confClient configuration.Config
 	log.Warning("Cannot read runtime API configuration, not using it")
 	return runtimeClient
 }
+
+// ReconfigureRuntime check if runtime client need be reconfigured by comparing the current configuration with the old
+// one (i.e. runtimeAPIsOld) and returns a callback that ReloadAgent can use to reconfigure the runtime client.
+func ReconfigureRuntime(client clientnative.HAProxyClient, runtimeAPIsOld []*models.RuntimeAPI) (callbackNeeded bool, callback func(), err error) {
+	cfg, err := client.Configuration()
+	if err != nil {
+		return false, nil, err
+	}
+	_, globalConf, err := cfg.GetGlobalConfiguration("")
+	if err != nil {
+		return false, nil, err
+	}
+	runtimeAPIsNew := globalConf.RuntimeAPIs
+	reconfigureRuntime := false
+	if len(runtimeAPIsOld) != len(runtimeAPIsNew) {
+		reconfigureRuntime = true
+	} else {
+		for _, runtimeOld := range runtimeAPIsOld {
+			if runtimeOld.Address == nil {
+				continue
+			}
+			found := false
+			for _, runtimeNew := range runtimeAPIsNew {
+				if runtimeNew.Address == nil {
+					continue
+				}
+				if *runtimeNew.Address == *runtimeOld.Address {
+					found = true
+					break
+				}
+			}
+			if !found {
+				reconfigureRuntime = true
+				break
+			}
+		}
+	}
+
+	if reconfigureRuntime {
+		dpapiCfg := dataplaneapi_config.Get()
+		haproxyOptions := dpapiCfg.HAProxy
+		return true, func() {
+			var rnt runtime_api.Runtime
+			i := 1
+			for i < 10 {
+				rnt = ConfigureRuntimeClient(context.Background(), cfg, haproxyOptions)
+				if rnt != nil {
+					break
+				}
+				time.Sleep(time.Duration(i) * time.Second)
+				i += i // exponential backoof
+			}
+			client.ReplaceRuntime(rnt)
+			if rnt == nil {
+				log.Debugf("reload callback completed, no runtime API")
+			} else {
+				log.Debugf("reload callback completed, runtime API reconfigured")
+			}
+		}, nil
+	}
+
+	return false, nil, nil
+}
diff --git a/e2e/tests/global/data/put_socket.json b/e2e/tests/global/data/put_socket.json
@@ -0,0 +1,12 @@
+{
+    "runtime_apis": [
+      {
+        "address": "/var/lib/haproxy/stats-new",
+        "level": "admin"
+      }
+    ],
+    "daemon": "enabled",
+    "master-worker": true,
+    "maxconn": 5000,
+    "pidfile": "/var/run/haproxy.pid"
+  }
diff --git a/e2e/tests/global/replace.bats b/e2e/tests/global/replace.bats
@@ -35,3 +35,53 @@ load 'utils/_helpers'
 	assert_equal "$(get_json_path "$BODY" '.data.runtime_apis[0].address')" "/var/lib/haproxy/stats"
 	assert_equal "$(get_json_path "$BODY" '.data.runtime_apis[0].level')" "admin"
 }
+
+
+@test "global: Replace a global configuration with socket path changed" {
+    resource_put "$_GLOBAL_BASE_PATH" "data/put_socket.json" ""
+	assert_equal "$SC" 202
+
+	resource_get "$_GLOBAL_BASE_PATH" ""
+	assert_equal "$SC" 200
+	assert_equal "$(get_json_path "$BODY" '.data.maxconn')" "5000"
+	assert_equal "$(get_json_path "$BODY" '.data.daemon')" "enabled"
+	assert_equal "$(get_json_path "$BODY" '.data.pidfile')" "/var/run/haproxy.pid"
+	assert_equal "$(get_json_path "$BODY" '.data.runtime_apis[0].address')" "/var/lib/haproxy/stats-new"
+	assert_equal "$(get_json_path "$BODY" '.data.runtime_apis[0].level')" "admin"
+
+	# check that runtime client has been reconfigured with the new socket
+	sleep 5
+	resource_get "$_RUNTIME_MAP_FILES_BASE_PATH" ""
+    assert_equal "$SC" 200
+}
+
+@test "global: Replace a global configuration with socket path changed (using transaction)" {
+    # create transaction
+    resource_post "$_TRANSACTIONS_BASE_PATH" ""
+    assert_equal "$SC" 201
+    local transaction_id; transaction_id=$(get_json_path "${BODY}" ".id")
+
+    # PUT new configuration
+    run dpa_curl PUT "${_GLOBAL_BASE_PATH}?transaction_id=${transaction_id}" "data/put_socket.json"
+	assert_success
+	dpa_curl_status_body '$output'
+	assert_equal "$SC" 202
+
+	# commit transaction
+    resource_put "$_TRANSACTIONS_BASE_PATH/$transaction_id" ""
+    assert_equal "$SC" 202
+
+    # check configuration has been applied
+	resource_get "$_GLOBAL_BASE_PATH" ""
+	assert_equal "$SC" 200
+	assert_equal "$(get_json_path "$BODY" '.data.maxconn')" "5000"
+	assert_equal "$(get_json_path "$BODY" '.data.daemon')" "enabled"
+	assert_equal "$(get_json_path "$BODY" '.data.pidfile')" "/var/run/haproxy.pid"
+	assert_equal "$(get_json_path "$BODY" '.data.runtime_apis[0].address')" "/var/lib/haproxy/stats-new"
+	assert_equal "$(get_json_path "$BODY" '.data.runtime_apis[0].level')" "admin"
+
+	# check that runtime client has been reconfigured with the new socket
+	sleep 5
+	resource_get "$_RUNTIME_MAP_FILES_BASE_PATH" ""
+    assert_equal "$SC" 200
+}
diff --git a/e2e/tests/global/utils/_helpers.bash b/e2e/tests/global/utils/_helpers.bash
@@ -16,3 +16,6 @@
 #
 
 _GLOBAL_BASE_PATH="/services/haproxy/configuration/global"
+_RUNTIME_MAP_FILES_BASE_PATH="/services/haproxy/runtime/maps"
+_TRANSACTIONS_BASE_PATH="/services/haproxy/transactions"
+
diff --git a/e2e/tests/raw/data/haproxy_socket.cfg.json b/e2e/tests/raw/data/haproxy_socket.cfg.json
@@ -0,0 +1 @@
+"global\n  chroot /var/lib/haproxy\n  user haproxy\n  group haproxy\n  maxconn 4000\n  pidfile /var/run/haproxy.pid\n  stats socket /var/lib/haproxy/stats-new level admin\n  log 127.0.0.1 local2\n\ndefaults\n  mode http\n  maxconn 3000\n  log global\n  option httplog\n  option redispatch\n  option dontlognull\n  option http-server-close\n  option forwardfor except 127.0.0.0/8\n  timeout http-request 10s\n  timeout check 10s\n  timeout connect 10s\n  timeout client 1m\n  timeout queue 1m\n  timeout server 1m\n  timeout http-keep-alive 10s\n  retries 3\n\nbackend test_backend\n  mode http\n  balance roundrobin\n  option forwardfor\n  server server_01 10.1.1.1:8080 check weight 80\n  server server_02 10.1.1.2:8080 check weight 80\n  server server_03 10.1.1.2:8080 check weight 80\n"
diff --git a/e2e/tests/raw/post.bats b/e2e/tests/raw/post.bats
@@ -34,3 +34,26 @@ load 'utils/_helpers'
   assert_equal "$SC" 400
   assert_equal "$(get_json_path "$BODY" '.message')" "invalid configuration: no newline character found"
 }
+
+
+@test "raw: Post new configuration with socket path changed" {
+    resource_post "$_RAW_BASE_PATH" 'data/haproxy_socket.cfg.json'
+	assert_equal "$SC" 202
+
+	resource_get "$_RAW_BASE_PATH" ""
+	assert_equal "$SC" 200
+
+    local socket; socket='stats socket /var/lib/haproxy/stats-new'
+    if [[ "$BODY" != *"$socket"* ]]; then
+       batslib_print_kv_single_or_multi 8 \
+          'configuration' "$BODY" \
+          'expected socket'   "$socket" \
+          | batslib_decorate 'configuration does not contains the new socket' \
+          | fail
+    fi
+
+	# check that runtime client has been reconfigured with the new socket
+	sleep 5
+	resource_get "$_RUNTIME_MAP_FILES_BASE_PATH" ""
+    assert_equal "$SC" 200
+}
diff --git a/handlers/global.go b/handlers/global.go
@@ -19,7 +19,7 @@ import (
 	"github.com/go-openapi/runtime/middleware"
 	client_native "github.com/haproxytech/client-native/v5"
 	"github.com/haproxytech/client-native/v5/models"
-
+	cn "github.com/haproxytech/dataplaneapi/client-native"
 	"github.com/haproxytech/dataplaneapi/haproxy"
 	"github.com/haproxytech/dataplaneapi/misc"
 	"github.com/haproxytech/dataplaneapi/operations/global"
@@ -84,22 +84,48 @@ func (h *ReplaceGlobalHandlerImpl) Handle(params global.ReplaceGlobalParams, pri
 		return global.NewReplaceGlobalDefault(int(*e.Code)).WithPayload(e)
 	}
 
+	// save old runtime configuration to know if the runtime client must be configured after the new configuration is
+	// reloaded by HAProxy. Logic is done by cn.ReconfigureRuntime() function.
+	_, globalConf, err := configuration.GetGlobalConfiguration("")
+	if err != nil {
+		e := misc.HandleError(err)
+		return global.NewReplaceGlobalDefault(int(*e.Code)).WithPayload(e)
+	}
+	runtimeAPIsOld := globalConf.RuntimeAPIs
+
 	err = configuration.PushGlobalConfiguration(params.Data, t, v)
 	if err != nil {
 		e := misc.HandleError(err)
 		return global.NewReplaceGlobalDefault(int(*e.Code)).WithPayload(e)
 	}
 
 	if params.TransactionID == nil {
+		callbackNeeded, reconfigureFunc, err := cn.ReconfigureRuntime(h.Client, runtimeAPIsOld)
+		if err != nil {
+			e := misc.HandleError(err)
+			return global.NewReplaceGlobalDefault(int(*e.Code)).WithPayload(e)
+		}
+
 		if *params.ForceReload {
-			err := h.ReloadAgent.ForceReload()
+			if callbackNeeded {
+				err = h.ReloadAgent.ForceReloadWithCallback(reconfigureFunc)
+			} else {
+				err = h.ReloadAgent.ForceReload()
+			}
+
 			if err != nil {
 				e := misc.HandleError(err)
 				return global.NewReplaceGlobalDefault(int(*e.Code)).WithPayload(e)
 			}
 			return global.NewReplaceGlobalOK().WithPayload(params.Data)
 		}
-		rID := h.ReloadAgent.Reload()
+
+		var rID string
+		if callbackNeeded {
+			rID = h.ReloadAgent.ReloadWithCallback(reconfigureFunc)
+		} else {
+			rID = h.ReloadAgent.Reload()
+		}
 		return global.NewReplaceGlobalAccepted().WithReloadID(rID).WithPayload(params.Data)
 	}
 	return global.NewReplaceGlobalAccepted().WithPayload(params.Data)
diff --git a/handlers/raw.go b/handlers/raw.go
@@ -16,21 +16,15 @@
 package handlers
 
 import (
-	"context"
 	"fmt"
 	"strconv"
 	"strings"
-	"time"
 
 	"github.com/go-openapi/runtime/middleware"
 	client_native "github.com/haproxytech/client-native/v5"
 	"github.com/haproxytech/client-native/v5/models"
-	"github.com/haproxytech/client-native/v5/runtime"
-
 	cn "github.com/haproxytech/dataplaneapi/client-native"
-	dataplaneapi_config "github.com/haproxytech/dataplaneapi/configuration"
 	"github.com/haproxytech/dataplaneapi/haproxy"
-	"github.com/haproxytech/dataplaneapi/log"
 	"github.com/haproxytech/dataplaneapi/misc"
 	"github.com/haproxytech/dataplaneapi/operations/configuration"
 )
@@ -137,7 +131,7 @@ func (h *PostRawConfigurationHandlerImpl) Handle(params configuration.PostHAProx
 	if forceReload {
 		var callbackNeeded bool
 		var reconfigureFunc func()
-		callbackNeeded, reconfigureFunc, err = h.reconfigureRuntime(runtimeAPIsOld)
+		callbackNeeded, reconfigureFunc, err = cn.ReconfigureRuntime(h.Client, runtimeAPIsOld)
 		if err != nil {
 			e := misc.HandleError(err)
 			return configuration.NewPostHAProxyConfigurationDefault(int(*e.Code)).WithPayload(e)
@@ -153,7 +147,7 @@ func (h *PostRawConfigurationHandlerImpl) Handle(params configuration.PostHAProx
 		}
 		return configuration.NewPostHAProxyConfigurationCreated().WithPayload(params.Data)
 	}
-	callbackNeeded, reconfigureFunc, err := h.reconfigureRuntime(runtimeAPIsOld)
+	callbackNeeded, reconfigureFunc, err := cn.ReconfigureRuntime(h.Client, runtimeAPIsOld)
 	if err != nil {
 		e := misc.HandleError(err)
 		return configuration.NewPostHAProxyConfigurationDefault(int(*e.Code)).WithPayload(e)
@@ -169,67 +163,6 @@ func (h *PostRawConfigurationHandlerImpl) Handle(params configuration.PostHAProx
 	return configuration.NewPostHAProxyConfigurationAccepted().WithReloadID(rID).WithPayload(params.Data)
 }
 
-func (h *PostRawConfigurationHandlerImpl) reconfigureRuntime(runtimeAPIsOld []*models.RuntimeAPI) (callbackNeeded bool, callback func(), err error) {
-	cfg, err := h.Client.Configuration()
-	if err != nil {
-		return false, nil, err
-	}
-	_, globalConf, err := cfg.GetGlobalConfiguration("")
-	if err != nil {
-		return false, nil, err
-	}
-	runtimeAPIsNew := globalConf.RuntimeAPIs
-	reconfigureRuntime := false
-	if len(runtimeAPIsOld) != len(runtimeAPIsNew) {
-		reconfigureRuntime = true
-	} else {
-		for _, runtimeOld := range runtimeAPIsOld {
-			if runtimeOld.Address == nil {
-				continue
-			}
-			found := false
-			for _, runtimeNew := range runtimeAPIsNew {
-				if runtimeNew.Address == nil {
-					continue
-				}
-				if *runtimeNew.Address == *runtimeOld.Address {
-					found = true
-					break
-				}
-			}
-			if !found {
-				reconfigureRuntime = true
-				break
-			}
-		}
-	}
-
-	if reconfigureRuntime {
-		dpapiCfg := dataplaneapi_config.Get()
-		haproxyOptions := dpapiCfg.HAProxy
-		return true, func() {
-			var rnt runtime.Runtime
-			i := 1
-			for i < 10 {
-				rnt = cn.ConfigureRuntimeClient(context.Background(), cfg, haproxyOptions)
-				if rnt != nil {
-					break
-				}
-				time.Sleep(time.Duration(i) * time.Second)
-				i += i // exponential backoof
-			}
-			h.Client.ReplaceRuntime(rnt)
-			if rnt == nil {
-				log.Debugf("reload callback completed, no runtime API")
-			} else {
-				log.Debugf("reload callback completed, runtime API reconfigured")
-			}
-		}, nil
-	}
-
-	return false, nil, nil
-}
-
 func executeRuntimeActions(actionsStr string, client client_native.HAProxyClient) error {
 	runtime, err := client.Runtime()
 	if err != nil {
diff --git a/handlers/transaction.go b/handlers/transaction.go

Original file line number	Diff line number	Diff line change
`@@ -16,3 +16,6 @@`
`16`	`16`	`#`
`17`	`17`
`18`	`18`	`_GLOBAL_BASE_PATH="/services/haproxy/configuration/global"`
	`19`	`+_RUNTIME_MAP_FILES_BASE_PATH="/services/haproxy/runtime/maps"`
	`20`	`+_TRANSACTIONS_BASE_PATH="/services/haproxy/transactions"`
	`21`	`+`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+"global\n chroot /var/lib/haproxy\n user haproxy\n group haproxy\n maxconn 4000\n pidfile /var/run/haproxy.pid\n stats socket /var/lib/haproxy/stats-new level admin\n log 127.0.0.1 local2\n\ndefaults\n mode http\n maxconn 3000\n log global\n option httplog\n option redispatch\n option dontlognull\n option http-server-close\n option forwardfor except 127.0.0.0/8\n timeout http-request 10s\n timeout check 10s\n timeout connect 10s\n timeout client 1m\n timeout queue 1m\n timeout server 1m\n timeout http-keep-alive 10s\n retries 3\n\nbackend test_backend\n mode http\n balance roundrobin\n option forwardfor\n server server_01 10.1.1.1:8080 check weight 80\n server server_02 10.1.1.2:8080 check weight 80\n server server_03 10.1.1.2:8080 check weight 80\n"