BUG/MEDIUM: configuration: stats refresh delay defaults to seconds

The 'stats refresh' keyword takes a delay that HAProxy reads as seconds
when no suffix is given, but it was parsed with ParseTimeout, which
assumes milliseconds. The model therefore held a value a thousand times
smaller than the one in the configuration file, and serializing wrote
that millisecond count back as a bare number, which HAProxy then read as
seconds again.

The raw text happened to survive a parse and serialize cycle, so this
stayed invisible when only rewriting a file, but the value exposed
through the model was wrong in both directions. Reading 'stats refresh
30' reported 30 ms for what HAProxy applies as 30 s, and asking for
30000 ms through the model produced 'stats refresh 30000', which HAProxy
applies as 30000 s.

Parse the delay with ParseTimeoutDefaultSeconds and serialize it with an
explicit 'ms' suffix, matching what is already done for the other
keywords using that parser, namely clitcpka-idle, clitcpka-intvl,
srvtcpka-idle and srvtcpka-intvl. The two changes belong together: fixing
the parser alone would leave the serializer scaling the delay by a
thousand on every reload, and would break callers that verify a submitted
configuration against its regenerated form.

The same fix is already in v6, where the delay is described as a duration
defaulting to seconds and serialized through misc.SerializeTime.

Author: aiharos

configuration/configuration.go

@@ -1076,7 +1076,8 @@ func (s *SectionParser) statsOptions() interface{} { //nolint:gocognit
 			opt.StatsMaxconn = mc.Value
 		case *stats.Refresh:
 			if v.Delay != "" {
-				opt.StatsRefreshDelay = misc.ParseTimeout(v.Delay)
+				// the keyword defaults to seconds, not milliseconds
+				opt.StatsRefreshDelay = misc.ParseTimeoutDefaultSeconds(v.Delay)
 			}
 		case *stats.ShowNode:
 			opt.StatsShowNodeName = misc.StringP(v.Name)
@@ -2605,7 +2606,9 @@ func (s *SectionObject) statsOptions(field reflect.Value) error {
 	}
 	if opt.StatsRefreshDelay != nil {
 		s := &stats.Refresh{
-			Delay: strconv.FormatInt(*opt.StatsRefreshDelay, 10),
+			// the model holds milliseconds while the keyword defaults to
+			// seconds, so the suffix must be explicit
+			Delay: fmt.Sprintf("%dms", *opt.StatsRefreshDelay),
 		}
 		ss = append(ss, s)
 	}

configuration/stats_refresh_test.go

@@ -0,0 +1,74 @@
+package configuration
+
+import (
+	"strings"
+	"testing"
+
+	parser "github.com/haproxytech/client-native/v5/config-parser"
+	"github.com/haproxytech/client-native/v5/config-parser/options"
+	"github.com/haproxytech/client-native/v5/models"
+
+	"github.com/stretchr/testify/require"
+)
+
+// statsRefreshRoundTrip parses a frontend holding a single 'stats refresh'
+// keyword and serializes it back, returning the regenerated keyword and the
+// milliseconds the model held in between.
+func statsRefreshRoundTrip(t *testing.T, delay string) (string, *int64) {
+	t.Helper()
+
+	pIn, err := parser.New(options.String("frontend stats\n  stats refresh " + delay + "\n"))
+	require.NoError(t, err)
+
+	fe := &models.Frontend{Name: "stats"}
+	require.NoError(t, ParseSection(fe, parser.Frontends, "stats", pIn))
+	require.NotNil(t, fe.StatsOptions)
+
+	pOut, err := parser.New(options.String("frontend stats\n"))
+	require.NoError(t, err)
+	require.NoError(t, CreateEditSection(fe, parser.Frontends, "stats", pOut))
+
+	var got string
+	for _, line := range strings.Split(pOut.String(), "\n") {
+		if line = strings.TrimSpace(line); strings.HasPrefix(line, "stats refresh ") {
+			got = strings.TrimPrefix(line, "stats refresh ")
+		}
+	}
+	return got, fe.StatsOptions.StatsRefreshDelay
+}
+
+// TestStatsRefreshDelayDefaultsToSeconds checks that a 'stats refresh' value
+// written without a suffix is read as seconds, as the keyword documents, and
+// that the delay is always serialized with an explicit unit. The model stores
+// milliseconds, so writing the bare number would scale the value by 1000 on
+// reload and make the round trip non-idempotent.
+func TestStatsRefreshDelayDefaultsToSeconds(t *testing.T) {
+	tests := []struct {
+		name     string
+		delay    string
+		expected string
+		ms       int64
+	}{
+		{name: "no suffix means seconds", delay: "30", expected: "30000ms", ms: 30000},
+		{name: "large value with no suffix", delay: "30000", expected: "30000000ms", ms: 30000000},
+		{name: "seconds", delay: "30s", expected: "30000ms", ms: 30000},
+		{name: "milliseconds", delay: "5000ms", expected: "5000ms", ms: 5000},
+		{name: "minutes", delay: "2m", expected: "120000ms", ms: 120000},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, ms := statsRefreshRoundTrip(t, tt.delay)
+			require.Equal(t, tt.expected, got)
+			require.NotNil(t, ms)
+			require.Equal(t, tt.ms, *ms)
+
+			// serializing the regenerated value again must be a no-op,
+			// otherwise every reload scales the delay by 1000
+			stable, msAgain := statsRefreshRoundTrip(t, got)
+			require.Equal(t, tt.expected, stable)
+			require.NotNil(t, msAgain)
+			require.Equal(t, tt.ms, *msAgain)
+		})
+	}
+}