From b18e8c428e6f270f366ae97e28460512c7209feb Mon Sep 17 00:00:00 2001
From: Florian Apolloner <florian@apolloner.eu>
Date: Thu, 29 Jul 2021 10:46:11 +0200
Subject: [PATCH] Fixed plan diffing to handle non-unique service names.

---
 .changelog/10965.txt       |  3 ++
 nomad/structs/diff.go      | 60 ++++++++++++++++++++++++++++-----
 nomad/structs/diff_test.go | 68 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+), 9 deletions(-)
 create mode 100644 .changelog/10965.txt

diff --git a/.changelog/10965.txt b/.changelog/10965.txt
new file mode 100644
index 00000000000..7778c5e0779
--- /dev/null
+++ b/.changelog/10965.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+plan: Handle multiple services with the same name.
+```
diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go
index 24d1f242e14..7912ca029e6 100644
--- a/nomad/structs/diff.go
+++ b/nomad/structs/diff.go
@@ -640,35 +640,77 @@ func serviceDiff(old, new *Service, contextual bool) *ObjectDiff {
 	return diff
 }
 
-// serviceDiffs diffs a set of services. If contextual diff is enabled, unchanged
-// fields within objects nested in the tasks will be returned.
-func serviceDiffs(old, new []*Service, contextual bool) []*ObjectDiff {
+// serviceDiffsInternal diffs a set of services with the same name.
+func serviceDiffsInternal(old, new []*Service, contextual bool) []*ObjectDiff {
 	oldMap := make(map[string]*Service, len(old))
 	newMap := make(map[string]*Service, len(new))
 	for _, o := range old {
-		oldMap[o.Name] = o
+		oldMap[o.PortLabel] = o
 	}
 	for _, n := range new {
-		newMap[n.Name] = n
+		newMap[n.PortLabel] = n
 	}
 
 	var diffs []*ObjectDiff
-	for name, oldService := range oldMap {
+	for portLabel, oldService := range oldMap {
 		// Diff the same, deleted and edited
-		if diff := serviceDiff(oldService, newMap[name], contextual); diff != nil {
+		if diff := serviceDiff(oldService, newMap[portLabel], contextual); diff != nil {
 			diffs = append(diffs, diff)
 		}
 	}
 
-	for name, newService := range newMap {
+	for portLabel, newService := range newMap {
 		// Diff the added
-		if old, ok := oldMap[name]; !ok {
+		if old, ok := oldMap[portLabel]; !ok {
 			if diff := serviceDiff(old, newService, contextual); diff != nil {
 				diffs = append(diffs, diff)
 			}
 		}
 	}
 
+	return diffs
+}
+
+// serviceDiffs diffs a set of services. If contextual diff is enabled, unchanged
+// fields within objects nested in the tasks will be returned.
+func serviceDiffs(old, new []*Service, contextual bool) []*ObjectDiff {
+	oldMap := make(map[string][]*Service, len(old))
+	newMap := make(map[string][]*Service, len(new))
+	for _, o := range old {
+		oldMap[o.Name] = append(oldMap[o.Name], o)
+	}
+	for _, n := range new {
+		newMap[n.Name] = append(newMap[n.Name], n)
+	}
+
+	var diffs []*ObjectDiff
+	for name, oldServices := range oldMap {
+		// services were deleted
+		if newServices, ok := newMap[name]; !ok {
+			for _, oldService := range oldServices {
+				diffs = append(diffs, serviceDiff(oldService, nil, contextual))
+			}
+		} else { // services where changed
+			// Handle the case of a "simple" change
+			if len(newServices) == 1 && len(oldServices) == 1 {
+				if diff := serviceDiff(oldServices[0], newServices[0], contextual); diff != nil {
+					diffs = append(diffs, diff)
+				}
+			} else { // More complex changes
+				diffs = append(diffs, serviceDiffsInternal(oldServices, newServices, contextual)...)
+			}
+		}
+	}
+
+	for name, newServices := range newMap {
+		// services where added
+		if _, ok := oldMap[name]; !ok {
+			for _, newService := range newServices {
+				diffs = append(diffs, serviceDiff(nil, newService, contextual))
+			}
+		}
+	}
+
 	sort.Sort(ObjectDiffs(diffs))
 	return diffs
 }
diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go
index 6eabcdc8b00..83132d8bc1e 100644
--- a/nomad/structs/diff_test.go
+++ b/nomad/structs/diff_test.go
@@ -3885,6 +3885,74 @@ func TestTaskGroupDiff(t *testing.T) {
 				},
 			},
 		},
+
+		{
+			TestCase:   "TaskGroup service added with same name",
+			Contextual: true,
+			Old: &TaskGroup{
+				Services: []*Service{
+					{
+						Name:      "foo",
+						PortLabel: "label1",
+					},
+				},
+			},
+
+			New: &TaskGroup{
+				Services: []*Service{
+					{
+						Name:      "foo",
+						PortLabel: "label1",
+					},
+					{
+						Name:      "foo",
+						PortLabel: "label2",
+					},
+				},
+			},
+			Expected: &TaskGroupDiff{
+				Type: DiffTypeEdited,
+				Objects: []*ObjectDiff{
+					{
+						Type: DiffTypeAdded,
+						Name: "Service",
+						Fields: []*FieldDiff{
+							{
+								Type: DiffTypeNone,
+								Name: "AddressMode",
+							},
+							{
+								Type: DiffTypeAdded,
+								Name: "EnableTagOverride",
+								New:  "false",
+							},
+							{
+								Type: DiffTypeAdded,
+								Name: "Name",
+								New:  "foo",
+							},
+							{
+								Type: DiffTypeNone,
+								Name: "Namespace",
+							},
+							{
+								Type: DiffTypeNone,
+								Name: "OnUpdate",
+							},
+							{
+								Type: DiffTypeAdded,
+								Name: "PortLabel",
+								New:  "label2",
+							},
+							{
+								Type: DiffTypeNone,
+								Name: "TaskName",
+							},
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for i, c := range cases {