add HostIDs to label API (#29185)

for #27701

# Checklist for submitter

- [X] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files)
for more information.
- [X] Manual QA for all new/changed functionality

## Details

This PR fixes an issue where adding or removing a single host on a
manual label in the UI had unexpected results when that host's serial
number was not unique. When adding the host, all other hosts with the
same serial # would be added. When removing the host, _no_ hosts would
be removed unless _all_ the hosts with the matching serial # were
removed. The fix here is to introduce a new API param `host_ids` which
allows sending explicit Fleet host IDs to the add/update label APIs.
These are guaranteed to be unique.

## Testing

* Added new automated tests for the `NewLabel` and `ModifyLabel`
services
* Manually tested adding and modifying labels using hosts with duplicate
serials (I manually updated serials in my local db to get duplicates)

## Notes

* The existing `hosts` param is preserved (and tested) since API-only
users may rely on it.
* A separate API docs PR will be opened.

changes/27701-fix-manual-label-with-duplicate-serials

@@ -0,0 +1 @@
+- Fixed an issue where adding/updating a manual label had inconsistent results when multiple hosts shared a serial number

frontend/services/entities/labels.ts

@@ -31,10 +31,6 @@ const isManualLabelFormData = (
   return "targetedHosts" in formData;
 };
 
-const getUniqueHostIdentifier = (host: IHost) => {
-  return host.hardware_serial || host.uuid || host.hostname;
-};
-
 const generateCreateLabelBody = (
   formData: IDynamicLabelFormData | IManualLabelFormData
 ) => {
@@ -43,9 +39,7 @@ const generateCreateLabelBody = (
     return {
       name: formData.name,
       description: formData.description,
-      hosts: formData.targetedHosts.map((host) =>
-        getUniqueHostIdentifier(host)
-      ),
+      host_ids: formData.targetedHosts.map((host) => host.id),
     };
   }
   return formData;

server/fleet/labels.go

@@ -14,7 +14,8 @@ type ModifyLabelPayload struct {
 	// valid for manual labels. If it is nil (not just len() == 0, but == nil),
 	// then the list of hosts is not modified. If it is not nil and len == 0,
 	// then all members are removed.
-	Hosts []string `json:"hosts"`
+	Hosts   []string `json:"hosts"`
+	HostIDs []uint   `json:"host_ids"`
 }
 
 type LabelPayload struct {
@@ -30,7 +31,8 @@ type LabelPayload struct {
 	// supported by HostByIdentifier) that are part of the label. This defines a
 	// manual label. Can be empty for a manual label that doesn't target any
 	// host. Must be empty for a dynamic label.
-	Hosts []string `json:"hosts"`
+	Hosts   []string `json:"hosts"`
+	HostIDs []uint   `json:"host_ids"`
 }
 
 // LabelType is used to catagorize the kind of label

server/service/integration_core_test.go

@@ -4218,7 +4218,7 @@ func (s *integrationTestSuite) TestLabels() {
 	// create a label with both a query and hosts, error
 	res := s.Do("POST", "/api/latest/fleet/labels", &fleet.LabelPayload{Name: t.Name(), Query: "select 1", Hosts: []string{manualHosts[0].UUID}}, http.StatusUnprocessableEntity)
 	errMsg := extractServerErrorText(res.Body)
-	require.Contains(t, errMsg, `Only one of either "query" or "hosts" can be included in the request.`)
+	require.Contains(t, errMsg, `Only one of either "query" or "hosts/host_ids" can be included in the request.`)
 
 	// create invalid label, conflicts with builtin name
 	for n := range builtinsMap {
@@ -4351,6 +4351,19 @@ func (s *integrationTestSuite) TestLabels() {
 	assert.Equal(t, newName, modResp.Label.Name)
 	manualLbl2.Name = newName
 
+	// modify manual label 2 adding some hosts by ID
+	modResp = modifyLabelResponse{}
+	newName = "modified_manual_label2"
+	s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/labels/%d", manualLbl2.ID),
+		&fleet.ModifyLabelPayload{Name: &newName, HostIDs: []uint{manualHosts[1].ID, manualHosts[2].ID}}, http.StatusOK, &modResp)
+	assert.Equal(t, manualLbl2.ID, modResp.Label.ID)
+	assert.Equal(t, fleet.LabelTypeRegular, modResp.Label.LabelType)
+	assert.Equal(t, fleet.LabelMembershipTypeManual, modResp.Label.LabelMembershipType)
+	assert.ElementsMatch(t, []uint{manualHosts[1].ID, manualHosts[2].ID}, modResp.Label.HostIDs)
+	assert.EqualValues(t, 2, modResp.Label.HostCount)
+	assert.Equal(t, newName, modResp.Label.Name)
+	manualLbl2.Name = newName
+
 	// modify manual label 2 clearing its hosts
 	modResp = modifyLabelResponse{}
 	s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/labels/%d", manualLbl2.ID), &fleet.ModifyLabelPayload{Hosts: []string{}, Description: ptr.String("desc")}, http.StatusOK, &modResp)

server/service/labels.go

@@ -53,6 +53,11 @@ func (svc *Service) NewLabel(ctx context.Context, p fleet.LabelPayload) (*fleet.
 	if !ok {
 		return nil, nil, fleet.ErrNoContext
 	}
+
+	if len(p.Hosts) > 0 && len(p.HostIDs) > 0 {
+		return nil, nil, fleet.NewInvalidArgumentError("hosts", `Only one of either "hosts" or "host_ids" can be included in the request.`)
+	}
+
 	filter := fleet.TeamFilter{User: vc.User, IncludeObserver: true}
 
 	label := &fleet.Label{
@@ -66,8 +71,8 @@ func (svc *Service) NewLabel(ctx context.Context, p fleet.LabelPayload) (*fleet.
 	}
 	label.Name = p.Name
 
-	if p.Query != "" && len(p.Hosts) > 0 {
-		return nil, nil, fleet.NewInvalidArgumentError("query", `Only one of either "query" or "hosts" can be included in the request.`)
+	if p.Query != "" && (len(p.Hosts) > 0 || len(p.HostIDs) > 0) {
+		return nil, nil, fleet.NewInvalidArgumentError("query", `Only one of either "query" or "hosts/host_ids" can be included in the request.`)
 	}
 	label.Query = p.Query
 	if p.Query == "" {
@@ -90,11 +95,13 @@ func (svc *Service) NewLabel(ctx context.Context, p fleet.LabelPayload) (*fleet.
 		return nil, nil, err
 	}
 
-	var hostIDs []uint
 	if label.LabelMembershipType == fleet.LabelMembershipTypeManual {
-		hostIDs, err = svc.ds.HostIDsByIdentifier(ctx, filter, p.Hosts)
-		if err != nil {
-			return nil, nil, err
+		hostIDs := p.HostIDs
+		if len(p.Hosts) > 0 {
+			hostIDs, err = svc.ds.HostIDsByIdentifier(ctx, filter, p.Hosts)
+			if err != nil {
+				return nil, nil, err
+			}
 		}
 		return svc.ds.UpdateLabelMembershipByHostIDs(ctx, label.ID, hostIDs, filter)
 	}
@@ -140,6 +147,11 @@ func (svc *Service) ModifyLabel(ctx context.Context, id uint, payload fleet.Modi
 	if !ok {
 		return nil, nil, fleet.ErrNoContext
 	}
+
+	if len(payload.Hosts) > 0 && len(payload.HostIDs) > 0 {
+		return nil, nil, fleet.NewInvalidArgumentError("hosts", `Only one of either "hosts" or "host_ids" can be included in the request.`)
+	}
+
 	filter := fleet.TeamFilter{User: vc.User, IncludeObserver: true}
 
 	label, _, err := svc.ds.Label(ctx, id, filter)
@@ -161,25 +173,30 @@ func (svc *Service) ModifyLabel(ctx context.Context, id uint, payload fleet.Modi
 	if payload.Description != nil {
 		label.Description = *payload.Description
 	}
-	if len(payload.Hosts) > 0 && label.LabelMembershipType != fleet.LabelMembershipTypeManual {
-		return nil, nil, fleet.NewInvalidArgumentError("hosts", "cannot provide a list of hosts for a dynamic label")
-	}
 
-	// use SaveLabel to update label info, and UpdateLabelMembershipByHostIDs to update membership. Approach using label
-	// names and ApplyLabelSpecs doesn't work for multiple hosts with the same name.
-
-	if payload.Hosts != nil {
-		// get host ids for valid hosts. since this endpoint will contain hosts identified by serial
-		// number, there should be no duplicates
-
-		hostIds, err := svc.ds.HostIDsByIdentifier(ctx, filter, payload.Hosts)
+	hostIDs := payload.HostIDs
+	if len(payload.Hosts) > 0 {
+		// If hosts were provided, convert them to IDs.
+		hostIDs, err = svc.ds.HostIDsByIdentifier(ctx, filter, payload.Hosts)
 		if err != nil {
 			return nil, nil, err
 		}
-		if _, _, err := svc.ds.UpdateLabelMembershipByHostIDs(ctx, label.ID, hostIds, filter); err != nil {
+	} else if payload.Hosts != nil {
+		// If an empry list was provided, create an empty list of IDs
+		// so that we can remove all hosts from the label.
+		hostIDs = make([]uint, 0)
+	}
+
+	if len(hostIDs) > 0 && label.LabelMembershipType != fleet.LabelMembershipTypeManual {
+		return nil, nil, fleet.NewInvalidArgumentError("hosts", "cannot provide a list of hosts for a dynamic label")
+	}
+
+	if hostIDs != nil {
+		if _, _, err := svc.ds.UpdateLabelMembershipByHostIDs(ctx, label.ID, hostIDs, filter); err != nil {
 			return nil, nil, err
 		}
 	}
+
 	return svc.ds.SaveLabel(ctx, label, filter)
 }
 

server/service/labels_test.go

@@ -431,3 +431,87 @@ func TestBatchValidateLabels(t *testing.T) {
 		})
 	}
 }
+
+func TestNewManualLabel(t *testing.T) {
+	ds := new(mock.Store)
+	svc, ctx := newTestService(t, ds, nil, nil)
+	ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}})
+
+	ds.NewLabelFunc = func(ctx context.Context, lbl *fleet.Label, opts ...fleet.OptionalArg) (*fleet.Label, error) {
+		lbl.ID = 1
+		lbl.LabelMembershipType = fleet.LabelMembershipTypeManual
+		return lbl, nil
+	}
+	ds.HostIDsByIdentifierFunc = func(ctx context.Context, filter fleet.TeamFilter, hostnames []string) ([]uint, error) {
+		return []uint{99, 100}, nil
+	}
+
+	t.Run("using hostnames", func(t *testing.T) {
+		ds.UpdateLabelMembershipByHostIDsFunc = func(ctx context.Context, labelID uint, hostIds []uint, teamFilter fleet.TeamFilter) (*fleet.Label, []uint, error) {
+			require.Equal(t, uint(1), labelID)
+			require.Equal(t, []uint{99, 100}, hostIds)
+			return nil, nil, nil
+		}
+		_, _, err := svc.NewLabel(ctx, fleet.LabelPayload{
+			Name:  "foo",
+			Hosts: []string{"host1", "host2"},
+		})
+		require.NoError(t, err)
+	})
+
+	t.Run("using IDs", func(t *testing.T) {
+		ds.UpdateLabelMembershipByHostIDsFunc = func(ctx context.Context, labelID uint, hostIds []uint, teamFilter fleet.TeamFilter) (*fleet.Label, []uint, error) {
+			require.Equal(t, uint(1), labelID)
+			require.Equal(t, []uint{1, 2}, hostIds)
+			return nil, nil, nil
+		}
+		_, _, err := svc.NewLabel(ctx, fleet.LabelPayload{
+			Name:    "foo",
+			HostIDs: []uint{1, 2},
+		})
+		require.NoError(t, err)
+	})
+}
+
+func TestModifyManualLabel(t *testing.T) {
+	ds := new(mock.Store)
+	svc, ctx := newTestService(t, ds, nil, nil)
+	ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}})
+
+	ds.LabelFunc = func(ctx context.Context, lid uint, teamFilter fleet.TeamFilter) (*fleet.Label, []uint, error) {
+		return &fleet.Label{
+			ID:                  lid,
+			LabelMembershipType: fleet.LabelMembershipTypeManual,
+		}, nil, nil
+	}
+	ds.HostIDsByIdentifierFunc = func(ctx context.Context, filter fleet.TeamFilter, hostnames []string) ([]uint, error) {
+		return []uint{99, 100}, nil
+	}
+	ds.SaveLabelFunc = func(ctx context.Context, lbl *fleet.Label, filter fleet.TeamFilter) (*fleet.Label, []uint, error) {
+		return nil, nil, nil
+	}
+
+	t.Run("using hostnames", func(t *testing.T) {
+		ds.UpdateLabelMembershipByHostIDsFunc = func(ctx context.Context, labelID uint, hostIds []uint, teamFilter fleet.TeamFilter) (*fleet.Label, []uint, error) {
+			require.Equal(t, uint(1), labelID)
+			require.Equal(t, []uint{99, 100}, hostIds)
+			return nil, nil, nil
+		}
+		_, _, err := svc.ModifyLabel(ctx, 1, fleet.ModifyLabelPayload{
+			Hosts: []string{"host1", "host2"},
+		})
+		require.NoError(t, err)
+	})
+
+	t.Run("using IDs", func(t *testing.T) {
+		ds.UpdateLabelMembershipByHostIDsFunc = func(ctx context.Context, labelID uint, hostIds []uint, teamFilter fleet.TeamFilter) (*fleet.Label, []uint, error) {
+			require.Equal(t, uint(1), labelID)
+			require.Equal(t, []uint{1, 2}, hostIds)
+			return nil, nil, nil
+		}
+		_, _, err := svc.ModifyLabel(ctx, 1, fleet.ModifyLabelPayload{
+			HostIDs: []uint{1, 2},
+		})
+		require.NoError(t, err)
+	})
+}