Fix enterprise discovery chain tests; Fix multi-level split merging

agent/consul/discoverychain/compile.go

@@ -274,7 +274,9 @@ func (c *compiler) compile() (*structs.CompiledDiscoveryChain, error) {
 		return nil, err
 	}
 
-	c.flattenAdjacentSplitterNodes()
+	if err := c.flattenAdjacentSplitterNodes(); err != nil {
+		return nil, err
+	}
 
 	if err := c.removeUnusedNodes(); err != nil {
 		return nil, err
@@ -394,7 +396,7 @@ func (c *compiler) detectCircularReferences() error {
 	return nil
 }
 
-func (c *compiler) flattenAdjacentSplitterNodes() {
+func (c *compiler) flattenAdjacentSplitterNodes() error {
 	for {
 		anyChanged := false
 		for _, node := range c.nodes {
@@ -416,11 +418,14 @@ func (c *compiler) flattenAdjacentSplitterNodes() {
 				for _, innerSplit := range nextNode.Splits {
 					effectiveWeight := split.Weight * innerSplit.Weight / 100
 
+					// Copy the definition from the inner node but merge in the parent
+					// to preserve any config it needs to pass through.
+					newDef, err := innerSplit.Definition.MergeParent(split.Definition)
+					if err != nil {
+						return err
+					}
 					newDiscoverySplit := &structs.DiscoverySplit{
-						// Copy the definition from the inner node so any extra config (e.g.
+						Definition: newDef,
-						// header manipulation) will be applied to requests taking this
-						// path.
-						Definition: innerSplit.Definition,
 						Weight:     structs.NormalizeServiceSplitWeight(effectiveWeight),
 						NextNode:   innerSplit.NextNode,
 					}
@@ -436,7 +441,7 @@ func (c *compiler) flattenAdjacentSplitterNodes() {
 		}
 
 		if !anyChanged {
-			return
+			return nil
 		}
 	}
 }

agent/consul/discoverychain/compile_test.go

@@ -1775,7 +1775,22 @@ func testcase_AllBellsAndWhistles() compileTestCase {
 			Name: "svc-split-again",
 			Splits: []structs.ServiceSplit{
 				{Weight: 75, Service: "main", ServiceSubset: "v1"},
-				{Weight: 25, Service: "svc-split-one-more-time"},
+				{
+					Weight:  25,
+					Service: "svc-split-one-more-time",
+					RequestHeaders: &structs.HTTPHeaderModifiers{
+						Set: map[string]string{
+							"parent": "1",
+							"shared": "from-parent",
+						},
+					},
+					ResponseHeaders: &structs.HTTPHeaderModifiers{
+						Set: map[string]string{
+							"parent": "2",
+							"shared": "from-parent",
+						},
+					},
+				},
 			},
 		},
 		&structs.ServiceSplitterConfigEntry{
@@ -1783,7 +1798,23 @@ func testcase_AllBellsAndWhistles() compileTestCase {
 			Name: "svc-split-one-more-time",
 			Splits: []structs.ServiceSplit{
 				{Weight: 80, Service: "main", ServiceSubset: "v2"},
-				{Weight: 20, Service: "main", ServiceSubset: "v3"},
+				{
+					Weight:        20,
+					Service:       "main",
+					ServiceSubset: "v3",
+					RequestHeaders: &structs.HTTPHeaderModifiers{
+						Set: map[string]string{
+							"child":  "3",
+							"shared": "from-child",
+						},
+					},
+					ResponseHeaders: &structs.HTTPHeaderModifiers{
+						Set: map[string]string{
+							"child":  "4",
+							"shared": "from-parent",
+						},
+					},
+				},
 			},
 		},
 	)
@@ -1888,6 +1919,20 @@ func testcase_AllBellsAndWhistles() compileTestCase {
 							Weight:        80,
 							Service:       "main",
 							ServiceSubset: "v2",
+							// Should inherit these from parent verbatim as there was no
+							// child-split header manip.
+							RequestHeaders: &structs.HTTPHeaderModifiers{
+								Set: map[string]string{
+									"parent": "1",
+									"shared": "from-parent",
+								},
+							},
+							ResponseHeaders: &structs.HTTPHeaderModifiers{
+								Set: map[string]string{
+									"parent": "2",
+									"shared": "from-parent",
+								},
+							},
 						},
 						Weight:   8,
 						NextNode: "resolver:v2.main.default.default.dc1",
@@ -1897,6 +1942,21 @@ func testcase_AllBellsAndWhistles() compileTestCase {
 							Weight:        20,
 							Service:       "main",
 							ServiceSubset: "v3",
+							// Should get a merge of child and parent rules
+							RequestHeaders: &structs.HTTPHeaderModifiers{
+								Set: map[string]string{
+									"parent": "1",
+									"child":  "3",
+									"shared": "from-child",
+								},
+							},
+							ResponseHeaders: &structs.HTTPHeaderModifiers{
+								Set: map[string]string{
+									"parent": "2",
+									"child":  "4",
+									"shared": "from-parent",
+								},
+							},
 						},
 						Weight:   2,
 						NextNode: "resolver:v3.main.default.default.dc1",

agent/structs/config_entry_discoverychain.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/mitchellh/copystructure"
 	"github.com/mitchellh/hashstructure"
 
 	"github.com/hashicorp/consul/acl"
@@ -665,11 +666,82 @@ type ServiceSplit struct {
 
 	// NOTE: Partition is not represented here by design. Do not add it.
 
+	// NOTE: Any configuration added to Splits that needs to be passed to the
+	// proxy needs special handling MergeParent below.
+
 	// Allow HTTP header manipulation to be configured.
 	RequestHeaders  *HTTPHeaderModifiers `json:",omitempty" alias:"request_headers"`
 	ResponseHeaders *HTTPHeaderModifiers `json:",omitempty" alias:"response_headers"`
 }
 
+// MergeParent is called by the discovery chain compiler when a split directs to
+// another splitter. We refer to the first ServiceSplit as the parent and the
+// ServiceSplits of the second splitter as its children. The parent ends up
+// "flattened" by the compiler, i.e. replaced with it's children recursively
+// with the weights modified as necessary.
+//
+// Since the parent is never included in the output, any request processing
+// config attached to it (e.g. header manipulation) would be lost and not take
+// affect when splitters direct to other splitters. To avoid that, we define a
+// MergeParent operation which is called by the compiler on each child split
+// during flattening. It must merge any request processing configuration from
+// the passed parent into the child such that the end result is equivalent to a
+// request first passing through the parent and then the child. Response
+// handling must occur as if the request first passed through the through the
+// child to the parent.
+//
+// MergeDefaults leaves both s and parent unchanged and returns a deep copy to
+// avoid confusing issues where config changes after being compiled.
+func (s *ServiceSplit) MergeParent(parent *ServiceSplit) (*ServiceSplit, error) {
+	if s == nil && parent == nil {
+		return nil, nil
+	}
+
+	var err error
+	var copy ServiceSplit
+
+	if s == nil {
+		copy = *parent
+		copy.RequestHeaders, err = parent.RequestHeaders.Clone()
+		if err != nil {
+			return nil, err
+		}
+		copy.ResponseHeaders, err = parent.ResponseHeaders.Clone()
+		if err != nil {
+			return nil, err
+		}
+		return &copy, nil
+	} else {
+		copy = *s
+	}
+
+	var parentReq *HTTPHeaderModifiers
+	if parent != nil {
+		parentReq = parent.RequestHeaders
+	}
+
+	// Merge any request handling from parent _unless_ it's overridden by us.
+	copy.RequestHeaders, err = s.RequestHeaders.MergeDefaults(parentReq)
+	if err != nil {
+		return nil, err
+	}
+
+	var parentResp *HTTPHeaderModifiers
+	if parent != nil {
+		parentResp = parent.ResponseHeaders
+	}
+
+	// Merge any response handling. Note that we allow parent to override this
+	// time since responses flow the other way so the unflattened behavior would
+	// be that the parent processing happens _after_ ours potentially overriding
+	// it.
+	copy.ResponseHeaders, err = parentResp.MergeDefaults(s.ResponseHeaders)
+	if err != nil {
+		return nil, err
+	}
+	return &copy, nil
+}
+
 // ServiceResolverConfigEntry defines which instances of a service should
 // satisfy discovery requests for a given named service.
 //
@@ -1507,3 +1579,57 @@ func (m *HTTPHeaderModifiers) Validate(protocol string) error {
 	}
 	return nil
 }
+
+// MergeDefaults takes another HTTPHeaderModifiers and merges it's fields. The
+// fields from this object take precedence over the passed in defaults if there
+// is a collision. The resulting object is returned leaving both m and defaults
+// unchanged. The semantics in the case of `Add` are that our Add will override
+// the default if they affect the same key since we have no way to express
+// multiple adds to the same key. We could change that, but it makes the config
+// syntax more complex for a huge edgecase.
+func (m *HTTPHeaderModifiers) MergeDefaults(defaults *HTTPHeaderModifiers) (*HTTPHeaderModifiers, error) {
+	if defaults.IsZero() {
+		return m.Clone()
+	}
+	if m == nil {
+		return defaults.Clone()
+	}
+
+	res, err := defaults.Clone()
+	if err != nil {
+		return nil, err
+	}
+	for k, v := range m.Add {
+		res.Add[k] = v
+	}
+	for k, v := range m.Set {
+		res.Set[k] = v
+	}
+
+	// Deduplicate removes.
+	removed := make(map[string]struct{})
+	for _, k := range res.Remove {
+		removed[k] = struct{}{}
+	}
+	for _, k := range m.Remove {
+		if _, ok := removed[k]; !ok {
+			res.Remove = append(res.Remove, k)
+		}
+	}
+
+	return res, nil
+}
+
+// Clone returns a deep-copy of m unless m is nil
+func (m *HTTPHeaderModifiers) Clone() (*HTTPHeaderModifiers, error) {
+	if m == nil {
+		return nil, nil
+	}
+
+	cpy, err := copystructure.Copy(m)
+	if err != nil {
+		return nil, err
+	}
+	m = cpy.(*HTTPHeaderModifiers)
+	return m, nil
+}

agent/structs/config_entry_discoverychain_test.go

@@ -1325,6 +1325,165 @@ func TestServiceSplitterConfigEntry(t *testing.T) {
 	}
 }
 
+func TestServiceSplitMergeParent(t *testing.T) {
+
+	type testCase struct {
+		name                string
+		split, parent, want *ServiceSplit
+		wantErr             string
+	}
+
+	run := func(t *testing.T, tc testCase) {
+		got, err := tc.split.MergeParent(tc.parent)
+		if tc.wantErr != "" {
+			require.Error(t, err)
+			require.Contains(t, err.Error(), tc.wantErr)
+		} else {
+			require.NoError(t, err)
+			require.Equal(t, tc.want, got)
+		}
+	}
+
+	testCases := []testCase{
+		{
+			name: "all header manip fields set",
+			split: &ServiceSplit{
+				Weight:  50.0,
+				Service: "foo",
+				RequestHeaders: &HTTPHeaderModifiers{
+					Add: map[string]string{
+						"child-only":      "1",
+						"both-want-child": "2",
+					},
+					Set: map[string]string{
+						"child-only":      "3",
+						"both-want-child": "4",
+					},
+					Remove: []string{"child-only-req", "both-req"},
+				},
+				ResponseHeaders: &HTTPHeaderModifiers{
+					Add: map[string]string{
+						"child-only":       "5",
+						"both-want-parent": "6",
+					},
+					Set: map[string]string{
+						"child-only":       "7",
+						"both-want-parent": "8",
+					},
+					Remove: []string{"child-only-resp", "both-resp"},
+				},
+			},
+			parent: &ServiceSplit{
+				Weight:  25.0,
+				Service: "bar",
+				RequestHeaders: &HTTPHeaderModifiers{
+					Add: map[string]string{
+						"parent-only":     "9",
+						"both-want-child": "10",
+					},
+					Set: map[string]string{
+						"parent-only":     "11",
+						"both-want-child": "12",
+					},
+					Remove: []string{"parent-only-req", "both-req"},
+				},
+				ResponseHeaders: &HTTPHeaderModifiers{
+					Add: map[string]string{
+						"parent-only":      "13",
+						"both-want-parent": "14",
+					},
+					Set: map[string]string{
+						"parent-only":      "15",
+						"both-want-parent": "16",
+					},
+					Remove: []string{"parent-only-resp", "both-resp"},
+				},
+			},
+			want: &ServiceSplit{
+				Weight:  50.0,
+				Service: "foo",
+				RequestHeaders: &HTTPHeaderModifiers{
+					Add: map[string]string{
+						"child-only":      "1",
+						"both-want-child": "2",
+						"parent-only":     "9",
+					},
+					Set: map[string]string{
+						"child-only":      "3",
+						"both-want-child": "4",
+						"parent-only":     "11",
+					},
+					Remove: []string{"parent-only-req", "both-req", "child-only-req"},
+				},
+				ResponseHeaders: &HTTPHeaderModifiers{
+					Add: map[string]string{
+						"child-only":       "5",
+						"parent-only":      "13",
+						"both-want-parent": "14",
+					},
+					Set: map[string]string{
+						"child-only":       "7",
+						"parent-only":      "15",
+						"both-want-parent": "16",
+					},
+					Remove: []string{"child-only-resp", "both-resp", "parent-only-resp"},
+				},
+			},
+		},
+		{
+			name: "no header manip",
+			split: &ServiceSplit{
+				Weight:  50,
+				Service: "foo",
+			},
+			parent: &ServiceSplit{
+				Weight:  50,
+				Service: "bar",
+			},
+			want: &ServiceSplit{
+				Weight:  50,
+				Service: "foo",
+			},
+		},
+		{
+			name: "nil parent",
+			split: &ServiceSplit{
+				Weight:  50,
+				Service: "foo",
+			},
+			parent: nil,
+			want: &ServiceSplit{
+				Weight:  50,
+				Service: "foo",
+			},
+		},
+		{
+			name:  "nil child",
+			split: nil,
+			parent: &ServiceSplit{
+				Weight:  50,
+				Service: "foo",
+			},
+			want: &ServiceSplit{
+				Weight:  50,
+				Service: "foo",
+			},
+		},
+		{
+			name:   "both nil",
+			split:  nil,
+			parent: nil,
+			want:   nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			run(t, tc)
+		})
+	}
+}
+
 func TestServiceRouterConfigEntry(t *testing.T) {
 
 	httpMatch := func(http *ServiceRouteHTTPMatch) *ServiceRouteMatch {