From b393c90ce77545db6274b68ef3b3395370f1dd06 Mon Sep 17 00:00:00 2001 From: Jared Kirschner Date: Fri, 20 Aug 2021 22:03:24 -0400 Subject: [PATCH] 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 | 3 +++ agent/acl.go | 10 ++++++++-- agent/agent_endpoint.go | 4 +++- agent/agent_endpoint_test.go | 2 +- agent/consul/catalog_endpoint.go | 8 ++++---- agent/local/state.go | 17 ++++++++++++----- agent/local/state_test.go | 32 ++++++++++++++++++++++++++++++-- 7 files changed, 61 insertions(+), 15 deletions(-) create mode 100644 .changelog/10894.txt diff --git a/.changelog/10894.txt b/.changelog/10894.txt new file mode 100644 index 0000000000..2799a4537a --- /dev/null +++ b/.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 +``` diff --git a/agent/acl.go b/agent/acl.go index 5502992160..ae7efdde13 100644 --- a/agent/acl.go +++ b/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 diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 623897ac80..297f96cbfe 100644 --- a/agent/agent_endpoint.go +++ b/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 } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 3d4a80beb4..a240aebeda 100644 --- a/agent/agent_endpoint_test.go +++ b/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_"`) }) } diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 4de46558a0..eb107193ab 100644 --- a/agent/consul/catalog_endpoint.go +++ b/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) diff --git a/agent/local/state.go b/agent/local/state.go index a729bf06c8..03db0badb7 100644 --- a/agent/local/state.go +++ b/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. diff --git a/agent/local/state_test.go b/agent/local/state_test.go index af89443099..62ebd40400 100644 --- a/agent/local/state_test.go +++ b/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{