Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #37116 +/- ##
==========================================
- Coverage 65.92% 65.92% -0.01%
==========================================
Files 2329 2329
Lines 184953 185260 +307
Branches 7707 7707
==========================================
+ Hits 121925 122124 +199
- Misses 51883 51969 +86
- Partials 11145 11167 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR implements backend support for team-scoped labels where label membership respects team context. Changes include adding team_id to label creation, introducing transactional label deletion with related membership cleanup, filtering label queries by team, and cascading label cleanup when teams are deleted. Label memberships are cleaned up when hosts transfer between teams. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
server/datastore/mysql/policies.go (1)
1461-1474: Label membership cleanup on team change is correct; optional empty-slice guardThe helper correctly mirrors
cleanupPolicyMembershipOnTeamChangeby deletinglabel_membershiprows for team-scoped labels (labels.team_id IS NOT NULL) for the given hosts, which is what you want when hosts move between teams.You may optionally add a fast-path guard:
if len(hostIDs) == 0 { return nil }to avoid
sqlx.Inbeing called with an empty slice if a future caller ever passeshostIDs == nil/len==0. Not strictly required given current patterns, but it makes the API more robust against misuse.server/datastore/mysql/teams.go (1)
139-149: teamLabelsRefs list is reasonable; keep it aligned with schemaThe new
teamLabelsRefsslice cleanly centralizes the set of tables that must be purged before deleting team labels, mirroring thehostRefspattern. This looks good; just keep this list in sync with any future FK additions/changes for label‑related tables.server/service/integration_mdm_test.go (1)
18891-18891: Unused test fixture:ubuntuHostNoTeamis created but never asserted upon.The host
ubuntuHostNoTeamis created and has mock live queries set up, but there are no assertions verifying its label memberships remain unaffected after team t1 deletion. Consider either:
- Adding an assertion to verify "No team" hosts are unaffected, or
- Removing this unused fixture to keep the test focused.
Also applies to: 18896-18896
server/datastore/mysql/labels.go (1)
461-487: Deletion order should delete child rows before parent rows.The function deletes from
labelstable first (line 466), then fromlabel_membership(line 474). This order is inverted—child rows inlabel_membershipshould be deleted before the parent rows inlabelsto avoid potential foreign key violations if constraints are added in the future.func deleteLabelsInTx(ctx context.Context, tx sqlx.ExtContext, labelIDs []uint) error { + // Delete child rows first + query, args, err := sqlx.In(`DELETE FROM label_membership WHERE label_id IN (?)`, labelIDs) + if err != nil { + return ctxerr.Wrap(ctx, err, "build sqlx.In statement for label_membership") + } + if _, err = tx.ExecContext(ctx, query, args...); err != nil { + return ctxerr.Wrap(ctx, err, "delete label_membership") + } + + query, args, err = sqlx.In(`DELETE FROM pack_targets WHERE type = ? AND target_id IN (?)`, fleet.TargetLabel, labelIDs) + if err != nil { + return ctxerr.Wrap(ctx, err, "build sqlx.In statement for pack_targets") + } + if _, err = tx.ExecContext(ctx, query, args...); err != nil { + return ctxerr.Wrap(ctx, err, "deleting pack_targets") + } + + // Delete parent rows last query, args, err := sqlx.In(`DELETE FROM labels WHERE id IN (?)`, labelIDs) if err != nil { return ctxerr.Wrap(ctx, err, "build sqlx.In statement for labels") } if _, err = tx.ExecContext(ctx, query, args...); err != nil { return ctxerr.Wrap(ctx, err, "delete label") } - - query, args, err = sqlx.In(`DELETE FROM label_membership WHERE label_id IN (?)`, labelIDs) - if err != nil { - return ctxerr.Wrap(ctx, err, "build sqlx.In statement for label_membership") - } - if _, err = tx.ExecContext(ctx, query, args...); err != nil { - return ctxerr.Wrap(ctx, err, "delete label_membership") - } - - query, args, err = sqlx.In(`DELETE FROM pack_targets WHERE type = ? AND target_id IN (?)`, fleet.TargetLabel, labelIDs) - if err != nil { - return ctxerr.Wrap(ctx, err, "build sqlx.In statement for pack_targets") - } - if _, err = tx.ExecContext(ctx, query, args...); err != nil { - return ctxerr.Wrap(ctx, err, "deleting pack_targets") - } return nil }server/service/integration_enterprise_test.go (1)
22651-22876: Strong, end‑to‑end coverage of team‑scoped label behaviorThis test does a good job stitching together the whole flow: team/global label creation, distributed reads filtered by team and built‑ins, distributed writes (including cross‑team label IDs) feeding back into host label membership, manual labels, host transfer clearing team labels, and team deletion cascading labels and membership while preserving labels on other teams. The use of a global admin
UserwithTeamFilterkeeps the authorization story consistent with how the datastore behaves.One optional improvement for future hardening would be to add a small distributed write scenario for the no‑team host, similar to the macOST1 cross‑team case, to explicitly assert that results for team‑scoped labels are ignored when the host has no team (and that only global labels can be applied). That would mirror the current cross‑team protection and close the last gap in the matrix of (team1, team2, no‑team) × (read, write).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
changes/36781-team-labels-backend(1 hunks)server/datastore/mysql/hosts.go(1 hunks)server/datastore/mysql/labels.go(4 hunks)server/datastore/mysql/labels_test.go(3 hunks)server/datastore/mysql/migrations/tables/20251207050413_TeamLabels_test.go(1 hunks)server/datastore/mysql/policies.go(1 hunks)server/datastore/mysql/teams.go(2 hunks)server/datastore/mysql/teams_test.go(6 hunks)server/service/integration_enterprise_test.go(1 hunks)server/service/integration_mdm_test.go(1 hunks)server/service/osquery.go(1 hunks)server/service/osquery_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/service/integration_mdm_test.goserver/service/osquery_test.goserver/service/osquery.goserver/datastore/mysql/teams.goserver/datastore/mysql/hosts.goserver/datastore/mysql/migrations/tables/20251207050413_TeamLabels_test.goserver/datastore/mysql/teams_test.goserver/service/integration_enterprise_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/labels.goserver/datastore/mysql/policies.go
🧠 Learnings (4)
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
Applied to files:
server/service/integration_mdm_test.goserver/service/integration_enterprise_test.goserver/datastore/mysql/labels_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/datastore/mysql/migrations/tables/20251207050413_TeamLabels_test.goserver/datastore/mysql/teams_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/labels.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/datastore/mysql/migrations/tables/20251207050413_TeamLabels_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/labels.go
📚 Learning: 2025-08-13T18:20:42.136Z
Learnt from: titanous
Repo: fleetdm/fleet PR: 31075
File: tools/redis-tests/elasticache/iam_auth.go:4-10
Timestamp: 2025-08-13T18:20:42.136Z
Learning: For test harnesses and CLI tools in the Fleet codebase, resource cleanup on error paths (like closing connections before log.Fatalf) may not be necessary since the OS handles cleanup when the process exits. These tools prioritize simplicity over defensive programming patterns used in production code.
Applied to files:
server/datastore/mysql/teams_test.go
🧬 Code graph analysis (9)
server/service/osquery_test.go (1)
server/mock/datastore_mock.go (1)
LabelQueriesForHostFunc(168-168)
server/service/osquery.go (3)
server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)tools/github-manage/pkg/ghapi/models.go (1)
Sprint(41-46)server/ptr/ptr.go (1)
Bool(25-27)
server/datastore/mysql/teams.go (1)
server/contexts/ctxerr/ctxerr.go (2)
Wrapf(211-214)Wrap(199-202)
server/datastore/mysql/hosts.go (1)
server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
server/datastore/mysql/teams_test.go (4)
server/ptr/ptr.go (2)
String(10-12)Bool(25-27)server/fleet/software_installer.go (1)
NewTempFileReader(950-970)server/fleet/labels.go (4)
LabelIdentsWithScope(302-305)LabelScope(288-288)LabelScopeIncludeAny(296-296)LabelIdent(281-284)server/datastore/mysql/testing_utils.go (1)
ExecAdhocSQL(420-424)
server/service/integration_enterprise_test.go (4)
server/fleet/hosts.go (1)
Host(276-405)server/fleet/teams.go (2)
Team(69-95)TeamFilter(577-586)server/fleet/labels.go (4)
LabelTypeRegular(65-65)LabelMembershipTypeDynamic(100-100)LabelMembershipTypeManual(102-102)LabelTypeBuiltIn(68-68)server/fleet/datastore.go (1)
IsNotFound(2758-2764)
server/datastore/mysql/labels_test.go (1)
server/fleet/labels.go (3)
Label(142-155)LabelType(61-61)LabelMembershipType(95-95)
server/datastore/mysql/labels.go (2)
server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)server/fleet/labels.go (1)
LabelMembershipTypeDynamic(100-100)
server/datastore/mysql/policies.go (1)
server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
🔇 Additional comments (24)
server/datastore/mysql/migrations/tables/20251207050413_TeamLabels_test.go (1)
51-54: Nolint G115 annotations are appropriate for test-only ID castsCasting auto-increment IDs to
uintin tests is safe here, and the explicit//nolint:goseccomments keep gosec noise down without affecting behavior.changes/36781-team-labels-backend (1)
1-1: Changelog entry matches PR scopeThe brief note cleanly reflects the backend-only addition of team label support and is sufficient for the changeset.
server/service/osquery.go (1)
1118-1135: Stale team-label result clearing is well targeted; confirm key format assumptionThe new logic correctly protects against the timing race you describe: by re-fetching the host’s current label queries and forcing
falsefor labels not present in that set, stale results from a previous team can’t reintroduce membership whenRecordLabelQueryExecutionsruns.This relies on
LabelQueriesForHostreturning a map keyed by stringified numeric label IDs (e.g."42"), which matches howingestMembershipQueryparses IDs fromfleet_label_query_<id>. Please confirm that invariant holds across the datastore implementation and tests so this filter never silently misses labels.server/datastore/mysql/hosts.go (1)
3292-3294: Label membership cleanup on team change is wired correctlyHooking
cleanupLabelMembershipOnTeamChangeintoAddHostsToTeamalongside the existing policy/query/conditional-access cleanup keeps team‑scoped label memberships consistent when hosts move between teams; no issues spotted with ordering or scoping here.server/datastore/mysql/teams.go (3)
160-167: Existing teamRefs cleanup remains correctly scoped by team_idThe loop over
teamRefsstill issuesDELETE FROM <table> WHERE team_id = ?, so team deletion remains properly constrained to the target team and uses only static table names viafmt.Sprintf, which is safe here.
199-201: Pack target cleanup for deleted team remains correctly filteredThe
DELETE FROM pack_targets WHERE type = ? AND target_id = ?call continues to scope cleanup toTargetTeamrows for this team only; formatting change doesn’t alter behavior.
169-192: Verify that other label deletion paths handle FK-constrained tablesThe team label cleanup sequence at these lines is correct: labels are properly removed from
in_house_app_labels,software_installer_labels, andvpp_app_team_labels(usingsqlx.Into avoid FK RESTRICT failures) before callingdeleteLabelsInTx.However, the generic
DeleteLabelfunction (inserver/datastore/mysql/labels.go) callsdeleteLabelsInTxdirectly without pre-cleaning these three FK-restricted tables. Since all three tables have explicitON DELETE RESTRICTconstraints onlabel_id(as confirmed in the migrations), attempting to delete a label that is referenced in any of these tables will fail at runtime with a foreign key constraint violation.Ensure that the
DeleteLabelfunction (or any other generic label deletion path) either:
- Pre-deletes from these three tables before calling
deleteLabelsInTx, or- Extends
deleteLabelsInTxto handle cleanup of all FK-restricted label tables.server/service/osquery_test.go (2)
1476-1481: Align label query keys with numeric label IDsUsing
"1","2","3"as keys matches how label results are reported (hostLabelQueryPrefix + "<id>") and keeps TestLabelQueries consistent with the team/ID-based label semantics exercised later in the test. Looks good.
3041-3043: Mocking a label query here correctly drives the error-logging pathProviding a non-empty
LabelQueriesForHostFuncwith key"1"ensures thehostLabelQueryPrefix + "1"results in this test actually invokeRecordLabelQueryExecutionsFunc, so the combined error logging being asserted is meaningfully exercised. No changes needed.server/service/integration_mdm_test.go (4)
18898-18918: LGTM!Good test setup with labels scoped to different teams (t1, t2) and a global label, providing proper coverage for verifying cascading deletion only affects the targeted team's labels.
18920-18932: LGTM!Proper simulation of label membership via the distributed query API, ensuring both team-scoped and global labels are assigned to respective hosts.
18934-19013: LGTM!Comprehensive setup creating various entities (software installer, VPP app, profiles, policy, query) that reference the team label. This ensures the deletion cascade handles all dependent relationships properly.
19015-19043: LGTM!The assertions thoroughly validate:
- Team label (l1t1) is deleted with the team
- Other team's label (l2t2) remains unaffected
- Label membership cleanup for hosts in the deleted team
- Label membership preservation for hosts in other teams
The sorting and ID-based ordering assumption is valid given l2t2 is created before globalLabel.
server/datastore/mysql/labels_test.go (3)
1817-1895: Test coverage for host transfer label cleanup looks good.The test correctly validates that when hosts are transferred to "No team", their team-scoped label memberships are removed while global label memberships are preserved. This aligns with the PR objective of ensuring label membership respects team context.
2378-2486: Team labels test coverage is comprehensive.The test validates:
LabelQueriesForHostreturns both global and team-scoped labels for team hosts- Platform filtering works correctly with team labels
- Manual team labels can be added to hosts via
AddLabelsToHost- Global hosts only receive global label queries
This provides solid coverage for the new team-scoped label functionality.
2211-2376: Test forUpdateLabelMembershipByHostIDsis well-structured.The test thoroughly exercises adding, removing, and replacing hosts in manual labels, including edge cases like:
- Adding/removing multiple hosts
- Clearing all hosts from a label
- Hosts with duplicate hostnames
- Verifying
GetLabelSpecreturns host IDs correctlyserver/datastore/mysql/teams_test.go (4)
56-64: Good setup for team label deletion tests.Creating the admin user and global VPP token provides the necessary context for the software installer and VPP app creation that follows. This ensures the test can validate cascading deletions across all related entities.
128-277: Comprehensive test data setup for team label associations.The test creates team labels with associations across multiple tables:
in_house_app_labels- in-house apps scoped to the team labelsoftware_installer_labels- software installers scoped to the team labelvpp_app_labels- VPP apps scoped to the team labellabel_membership- host membership in the team labelThis ensures the team deletion path properly cleans up all label-related foreign key references.
329-337: Good validation of team label cleanup after deletion.The assertions verify that:
- The team label is no longer retrievable by name
- The host's label memberships are empty after the team (and its labels) are deleted
This confirms the cascading deletion works correctly.
298-309:teamLabelsRefsis correctly defined with all required tables.The variable includes all three tables that reference
label_idwithON DELETE RESTRICTconstraints:in_house_app_labels,software_installer_labels, andvpp_app_team_labels. The definition correctly excludes tables usingON DELETE CASCADEorON DELETE SET NULL, which don't require explicit deletion before label removal.server/datastore/mysql/labels.go (3)
397-412: Team ID properly added to label creation.The INSERT now correctly includes
team_idas the 9th parameter, enabling team-scoped labels. The placeholder count and parameter list are consistent.
570-574: Team-scoped label query filter is correctly implemented.The WHERE clause
(team_id IS NULL OR team_id = ?)correctly returns:
- Global labels (team_id IS NULL) for all hosts
- Team-specific labels only for hosts belonging to that team
This aligns with the PR objective of scoping label membership by team_id. Based on the coding guidelines, this SQL query has appropriate filtering criteria for the intended entity scope.
441-459: DeleteLabel properly wraps the transaction and handles errors.The refactored DeleteLabel correctly:
- Fetches the label ID first to return a proper NotFoundError if missing
- Uses the new
deleteLabelsInTxhelper for transactional cleanup- Preserves foreign key error handling for labels referenced by other entities
server/service/integration_enterprise_test.go (1)
22621-22648: Helper correctly models distributed label result statesThis helper cleanly captures the three label result cases (failed execution, match, no match) and builds a realistic distributed write payload keyed by
hostLabelQueryPrefix + labelID. Given hosts in this test are always created with a non-nilNodeKey, the implementation looks solid and is a nice reusable building block for other label-related tests.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Apologies for the delay on review (tests took a little longer). A few questions + minor feedback items, otherwise looks good.
There was also the typo CodeRabbit caught (that I didn't), so you'll want to fix that too.
Co-authored-by: Ian Littman <iansltx@gmail.com>
2 participants
Resolves #36781.
changes/,orbit/changes/oree/fleetd-chrome/changes.SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.