Clarify service and check error messages (use ID)

Error messages related to service and check operations previously included
the following substrings:
- service %q
- check %q

From this error message, it isn't clear that the expected field is the ID for
the entity, not the name. For example, if the user has a service named test,
the error message would read 'Unknown service "test"'. This is misleading -
a service with that *name* does exist, but not with that *ID*.

The substrings above have been modified to make it clear that ID is needed,
not name:
- service with ID %q
- check with ID %q

.changelog/10894.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+api: Improve error message if service or health check not found by stating that the entity must be referred to by ID, not name
+```

agent/acl.go

@@ -84,7 +84,13 @@ func (a *Agent) vetServiceUpdateWithAuthorizer(authz acl.Authorizer, serviceID s
 				structs.ServiceIDString(existing.Service, &existing.EnterpriseMeta))
 		}
 	} else {
-		return NotFoundError{Reason: fmt.Sprintf("Unknown service %q", serviceID)}
+		// Take care if modifying this error message.
+		// agent/local/state.go's deleteService assumes the Catalog.Deregister RPC call
+		// will include "Unknown service"in the error if deregistration fails due to a
+		// service with that ID not existing.
+		return NotFoundError{Reason: fmt.Sprintf(
+			"Unknown service ID %q. Ensure that the service ID is passed, not the service name.",
+			serviceID)}
 	}
 
 	return nil
@@ -143,7 +149,7 @@ func (a *Agent) vetCheckUpdateWithAuthorizer(authz acl.Authorizer, checkID struc
 			}
 		}
 	} else {
-		return fmt.Errorf("Unknown check %q", checkID.String())
+		return fmt.Errorf("Unknown check ID %q. Ensure that the check ID is passed, not the check name.", checkID.String())
 	}
 
 	return nil

agent/agent_endpoint.go

@@ -424,7 +424,9 @@ func (s *HTTPHandlers) AgentService(resp http.ResponseWriter, req *http.Request)
 			svcState := s.agent.State.ServiceState(sid)
 			if svcState == nil {
 				resp.WriteHeader(http.StatusNotFound)
-				fmt.Fprintf(resp, "unknown service ID: %s", sid.String())
+				fmt.Fprintf(resp,
+					"Unknown service ID %q. Ensure that the service ID is passed, not the service name.",
+					sid.String())
 				return "", nil, nil
 			}
 

agent/agent_endpoint_test.go

@@ -4751,7 +4751,7 @@ func TestAgent_ServiceMaintenance_BadRequest(t *testing.T) {
 		resp := httptest.NewRecorder()
 		a.srv.h.ServeHTTP(resp, req)
 		require.Equal(t, 404, resp.Code)
-		require.Contains(t, resp.Body.String(), `Unknown service "_nope_"`)
+		require.Contains(t, resp.Body.String(), `Unknown service ID "_nope_"`)
 	})
 }
 

agent/consul/catalog_endpoint.go

@@ -309,12 +309,12 @@ func vetRegisterWithACL(
 		// Service-level check for some other service. Make sure they've
 		// got write permissions for that service.
 		if ns == nil {
-			return fmt.Errorf("Unknown service '%s' for check '%s'", check.ServiceID, check.CheckID)
+			return fmt.Errorf("Unknown service ID '%s' for check ID '%s'", check.ServiceID, check.CheckID)
 		}
 
 		other, ok := ns.Services[check.ServiceID]
 		if !ok {
-			return fmt.Errorf("Unknown service '%s' for check '%s'", check.ServiceID, check.CheckID)
+			return fmt.Errorf("Unknown service ID '%s' for check ID '%s'", check.ServiceID, check.CheckID)
 		}
 
 		// We are only adding a check here, so we don't add the scope,
@@ -417,7 +417,7 @@ func vetDeregisterWithACL(
 	// ignore them from an ACL perspective.
 	if subj.ServiceID != "" {
 		if ns == nil {
-			return fmt.Errorf("Unknown service '%s'", subj.ServiceID)
+			return fmt.Errorf("Unknown service ID '%s'", subj.ServiceID)
 		}
 
 		ns.FillAuthzContext(&authzContext)
@@ -427,7 +427,7 @@ func vetDeregisterWithACL(
 		}
 	} else if subj.CheckID != "" {
 		if nc == nil {
-			return fmt.Errorf("Unknown check '%s'", subj.CheckID)
+			return fmt.Errorf("Unknown check ID '%s'", subj.CheckID)
 		}
 
 		nc.FillAuthzContext(&authzContext)

agent/local/state.go

@@ -272,7 +272,7 @@ func (l *State) addServiceLocked(service *structs.NodeService, token string) err
 	}
 
 	if l.agentEnterpriseMeta.PartitionOrDefault() != service.PartitionOrDefault() {
-		return fmt.Errorf("cannot add service %q to node in partition %q", service.CompoundServiceID(), l.config.Partition)
+		return fmt.Errorf("cannot add service ID %q to node in partition %q", service.CompoundServiceID(), l.config.Partition)
 	}
 
 	l.setServiceStateLocked(&ServiceState{
@@ -328,7 +328,14 @@ func (l *State) RemoveServiceWithChecks(serviceID structs.ServiceID, checkIDs []
 func (l *State) removeServiceLocked(id structs.ServiceID) error {
 	s := l.services[id]
 	if s == nil || s.Deleted {
-		return fmt.Errorf("Service %q does not exist", id)
+		// Take care if modifying this error message.
+		// deleteService assumes the Catalog.Deregister RPC call will include "Unknown service"
+		// in the error if deregistration fails due to a service with that ID not existing.
+
+		// When the service register endpoint is called, this error message is also typically
+		// shadowed by vetServiceUpdateWithAuthorizer, which checks for the existence of the
+		// service and, if none is found, returns an error before this function is ever called.
+		return fmt.Errorf("Unknown service ID %q. Ensure that the service ID is passed, not the service name.", id)
 	}
 
 	// To remove the service on the server we need the token.
@@ -539,7 +546,7 @@ func (l *State) addCheckLocked(check *structs.HealthCheck, token string) error {
 	// if there is a serviceID associated with the check, make sure it exists before adding it
 	// NOTE - This logic may be moved to be handled within the Agent's Addcheck method after a refactor
 	if _, ok := l.services[check.CompoundServiceID()]; check.ServiceID != "" && !ok {
-		return fmt.Errorf("Check %q refers to non-existent service %q", check.CheckID, check.ServiceID)
+		return fmt.Errorf("Check ID %q refers to non-existent service ID %q", check.CheckID, check.ServiceID)
 	}
 
 	l.setCheckStateLocked(&CheckState{
@@ -561,7 +568,7 @@ func (l *State) AddAliasCheck(checkID structs.CheckID, srcServiceID structs.Serv
 	defer l.Unlock()
 
 	if l.agentEnterpriseMeta.PartitionOrDefault() != checkID.PartitionOrDefault() {
-		return fmt.Errorf("cannot add alias check %q to node in partition %q", checkID.String(), l.config.Partition)
+		return fmt.Errorf("cannot add alias check ID %q to node in partition %q", checkID.String(), l.config.Partition)
 	}
 	if l.agentEnterpriseMeta.PartitionOrDefault() != srcServiceID.PartitionOrDefault() {
 		return fmt.Errorf("cannot add alias check for %q to node in partition %q", srcServiceID.String(), l.config.Partition)
@@ -612,7 +619,7 @@ func (l *State) RemoveCheck(id structs.CheckID) error {
 func (l *State) removeCheckLocked(id structs.CheckID) error {
 	c := l.checks[id]
 	if c == nil || c.Deleted {
-		return fmt.Errorf("Check %q does not exist", id)
+		return fmt.Errorf("Check ID %q does not exist", id)
 	}
 
 	// If this is a check for an aliased service, then notify the waiters.

agent/local/state_test.go

@@ -1952,7 +1952,7 @@ func TestAgent_AddCheckFailure(t *testing.T) {
 		ServiceID: "redis",
 		Status:    api.HealthPassing,
 	}
-	wantErr := errors.New(`Check "redis:1" refers to non-existent service "redis"`)
+	wantErr := errors.New(`Check ID "redis:1" refers to non-existent service ID "redis"`)
 
 	got := l.AddCheck(chk, "")
 	require.Equal(t, wantErr, got)
@@ -2114,7 +2114,7 @@ func servicesInSync(state *local.State, wantServices int, entMeta *structs.Enter
 	}
 	for id, s := range services {
 		if !s.InSync {
-			return fmt.Errorf("service %q should be in sync %+v", id.String(), s)
+			return fmt.Errorf("service ID %q should be in sync %+v", id.String(), s)
 		}
 	}
 	return nil
@@ -2133,6 +2133,34 @@ func checksInSync(state *local.State, wantChecks int, entMeta *structs.Enterpris
 	return nil
 }
 
+func TestState_RemoveServiceErrorMessages(t *testing.T) {
+	state := local.NewState(local.Config{}, hclog.New(nil), &token.Store{})
+
+	// Stub state syncing
+	state.TriggerSyncChanges = func() {}
+
+	require := require.New(t)
+
+	// Add 1 service
+	err := state.AddService(&structs.NodeService{
+		ID:      "web-id",
+		Service: "web-name",
+	}, "")
+	require.NoError(err)
+
+	// Attempt to remove service that doesn't exist
+	err = state.RemoveService(structs.NewServiceID("db", nil))
+	require.Contains(err.Error(), `Unknown service ID "db"`)
+
+	// Attempt to remove service by name (which isn't valid)
+	err = state.RemoveService(structs.NewServiceID("web-name", nil))
+	require.Contains(err.Error(), `Unknown service ID "web-name"`)
+
+	// Attempt to remove service by id (valid)
+	err = state.RemoveService(structs.NewServiceID("web-id", nil))
+	require.NoError(err)
+}
+
 func TestState_Notify(t *testing.T) {
 	t.Parallel()
 	logger := hclog.New(&hclog.LoggerOptions{