From affe97e22d46263352fcf046560f6b884c1d4549 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Thu, 23 Dec 2021 16:30:36 -0500
Subject: [PATCH] config: correctly capture all errors.

Some calls to multierror.Append were not using the existing b.err, which meant we
were losing all previous errors.
---
 agent/config/builder.go      | 10 +++++-----
 agent/config/builder_test.go | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/agent/config/builder.go b/agent/config/builder.go
index 7f3ab5e4e..987a04fbb 100644
--- a/agent/config/builder.go
+++ b/agent/config/builder.go
@@ -1631,7 +1631,7 @@ func (b *builder) serviceVal(v *ServiceDefinition) *structs.ServiceDefinition {
 
 	meta := make(map[string]string)
 	if err := structs.ValidateServiceMetadata(kind, v.Meta, false); err != nil {
-		b.err = multierror.Append(fmt.Errorf("invalid meta for service %s: %v", stringVal(v.Name), err))
+		b.err = multierror.Append(b.err, fmt.Errorf("invalid meta for service %s: %v", stringVal(v.Name), err))
 	} else {
 		meta = v.Meta
 	}
@@ -1646,13 +1646,13 @@ func (b *builder) serviceVal(v *ServiceDefinition) *structs.ServiceDefinition {
 	}
 
 	if err := structs.ValidateWeights(serviceWeights); err != nil {
-		b.err = multierror.Append(fmt.Errorf("Invalid weight definition for service %s: %s", stringVal(v.Name), err))
+		b.err = multierror.Append(b.err, fmt.Errorf("Invalid weight definition for service %s: %s", stringVal(v.Name), err))
 	}
 
 	if (v.Port != nil || v.Address != nil) && (v.SocketPath != nil) {
-		b.err = multierror.Append(
+		b.err = multierror.Append(b.err,
 			fmt.Errorf("service %s cannot have both socket path %s and address/port",
-				stringVal(v.Name), stringVal(v.SocketPath)), b.err)
+				stringVal(v.Name), stringVal(v.SocketPath)))
 	}
 
 	return &structs.ServiceDefinition{
@@ -1890,7 +1890,7 @@ func (b *builder) durationValWithDefault(name string, v *string, defaultVal time
 	}
 	d, err := time.ParseDuration(*v)
 	if err != nil {
-		b.err = multierror.Append(fmt.Errorf("%s: invalid duration: %q: %s", name, *v, err))
+		b.err = multierror.Append(b.err, fmt.Errorf("%s: invalid duration: %q: %s", name, *v, err))
 	}
 	return d
 }
diff --git a/agent/config/builder_test.go b/agent/config/builder_test.go
index 5901431a0..80ad7b368 100644
--- a/agent/config/builder_test.go
+++ b/agent/config/builder_test.go
@@ -291,3 +291,39 @@ func TestLoad_EmptyClientAddr(t *testing.T) {
 		})
 	}
 }
+
+func TestBuilder_DurationVal_InvalidDuration(t *testing.T) {
+	b := builder{}
+	badDuration1 := "not-a-duration"
+	badDuration2 := "also-not"
+	b.durationVal("field1", &badDuration1)
+	b.durationVal("field1", &badDuration2)
+
+	require.Error(t, b.err)
+	require.Contains(t, b.err.Error(), "2 errors")
+	require.Contains(t, b.err.Error(), badDuration1)
+	require.Contains(t, b.err.Error(), badDuration2)
+}
+
+func TestBuilder_ServiceVal_MultiError(t *testing.T) {
+	b := builder{}
+	b.serviceVal(&ServiceDefinition{
+		Meta:       map[string]string{"": "empty-key"},
+		Port:       intPtr(12345),
+		SocketPath: strPtr("/var/run/socket.sock"),
+		Checks: []CheckDefinition{
+			{Interval: strPtr("bad-interval")},
+		},
+		Weights: &ServiceWeights{Passing: intPtr(-1)},
+	})
+	require.Error(t, b.err)
+	require.Contains(t, b.err.Error(), "4 errors")
+	require.Contains(t, b.err.Error(), "bad-interval")
+	require.Contains(t, b.err.Error(), "Key cannot be blank")
+	require.Contains(t, b.err.Error(), "Invalid weight")
+	require.Contains(t, b.err.Error(), "cannot have both socket path")
+}
+
+func intPtr(v int) *int {
+	return &v
+}