From ab2e6e87b8b520ab75b786bac6f2c1d7cf741b8f Mon Sep 17 00:00:00 2001
From: Seth Hoenig <seth.a.hoenig@gmail.com>
Date: Wed, 20 Jul 2022 12:20:11 -0500
Subject: [PATCH] jobspec: ensure service uniqueness in job validation

---
 .changelog/13869.txt     |  3 +++
 nomad/structs/structs.go | 47 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100644 .changelog/13869.txt

diff --git a/.changelog/13869.txt b/.changelog/13869.txt
new file mode 100644
index 000000000000..a29c7568bb85
--- /dev/null
+++ b/.changelog/13869.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+servicedisco: Fixed a bug where non-unique services would escape job validation
+```
diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go
index 3dff17cf30bc..ae83d73bc97e 100644
--- a/nomad/structs/structs.go
+++ b/nomad/structs/structs.go
@@ -6628,6 +6628,19 @@ func (tg *TaskGroup) validateServices() error {
 	// Accumulate task names in this group
 	taskSet := set.New[string](len(tg.Tasks))
 
+	// each service in a group must be unique (i.e. used in MakeAllocServiceID)
+	type unique struct {
+		name string
+		task string
+		port string
+	}
+
+	// Accumulate service IDs in this group
+	idSet := set.New[unique](0)
+
+	// Accumulate IDs that are duplicates
+	idDuplicateSet := set.New[unique](0)
+
 	// Accumulate the providers used for this task group. Currently, Nomad only
 	// allows the use of a single service provider within a task group.
 	providerSet := set.New[string](1)
@@ -6642,6 +6655,7 @@ func (tg *TaskGroup) validateServices() error {
 		}
 
 		for _, service := range task.Services {
+
 			// Ensure no task-level checks specify a task
 			for _, check := range service.Checks {
 				if check.TaskName != "" {
@@ -6649,6 +6663,13 @@ func (tg *TaskGroup) validateServices() error {
 				}
 			}
 
+			// Track that we have seen this service id
+			id := unique{service.Name, task.Name, service.PortLabel}
+			if !idSet.Insert(id) {
+				// accumulate duplicates for a single error later on
+				idDuplicateSet.Insert(id)
+			}
+
 			// Track that we have seen this service provider
 			providerSet.Insert(service.Provider)
 		}
@@ -6656,6 +6677,13 @@ func (tg *TaskGroup) validateServices() error {
 
 	for i, service := range tg.Services {
 
+		// Track that we have seen this service id
+		id := unique{service.Name, "group", service.PortLabel}
+		if !idSet.Insert(id) {
+			// accumulate duplicates for a single error later on
+			idDuplicateSet.Insert(id)
+		}
+
 		// Track that we have seen this service provider
 		providerSet.Insert(service.Provider)
 
@@ -6688,6 +6716,25 @@ func (tg *TaskGroup) validateServices() error {
 		}
 	}
 
+	// Produce an error of any services which are not unique enough in the group
+	// i.e. have same <task, name, port>
+	if idDuplicateSet.Size() > 0 {
+		mErr.Errors = append(mErr.Errors,
+			fmt.Errorf(
+				"Services are not unique: %s",
+				idDuplicateSet.String(
+					func(u unique) string {
+						s := u.task + "->" + u.name
+						if u.port != "" {
+							s += ":" + u.port
+						}
+						return s
+					},
+				),
+			),
+		)
+	}
+
 	// The initial feature release of native service discovery only allows for
 	// a single service provider to be used across all services in a task
 	// group.