Add include_all label scope to policies and reports#44305
Merged
juan-fdz-hawa merged 7 commits intomainfrom Apr 30, 2026
Merged
Add include_all label scope to policies and reports#44305juan-fdz-hawa merged 7 commits intomainfrom
juan-fdz-hawa merged 7 commits intomainfrom
Conversation
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44305 +/- ##
==========================================
+ Coverage 66.76% 66.78% +0.01%
==========================================
Files 2636 2637 +1
Lines 211834 211957 +123
Branches 9425 9425
==========================================
+ Hits 141437 141547 +110
- Misses 57551 57556 +5
- Partials 12846 12854 +8
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:
|
juan-fdz-hawa
force-pushed
the
41564-policyreport-labels-backend
branch
from
b56c638 to
99a5ee9
Compare
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
juan-fdz-hawa
force-pushed
the
41564-policyreport-labels-backend
branch
from
99a5ee9 to
28c5771
Compare
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
juan-fdz-hawa
force-pushed
the
41564-policyreport-labels-backend
branch
2 times, most recently
from
493291c to
20ac8ca
Compare
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Error
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis pull request introduces a new Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
server/service/queries.go (1)
714-718:⚠️ Potential issue | 🟠 MajorQuerySpec does not support
LabelsIncludeAll, creating an asymmetry with the Query model.The
Querymodel supports bothLabelsIncludeAnyandLabelsIncludeAll, butQuerySpeconly definesLabelsIncludeAny. This means:
- Queries created via
NewQuery/ModifyQuerycan have labels scoped to "include all"- Spec import/export via
ApplyQuerySpecs,queryFromSpec, andspecFromQuerysilently dropsLabelsIncludeAll- Premium gating in
ApplyQuerySpecsonly checksLabelsIncludeAny, so extendingQuerySpecwithLabelsIncludeAllwould require gating updatesExtend
QuerySpecto includeLabelsIncludeAlland wire it through the spec flow (premium check,queryFromSpec,specFromQuery).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/queries.go` around lines 714 - 718, QuerySpec currently lacks a LabelsIncludeAll field causing label-all semantics to be dropped; add LabelsIncludeAll []string to the QuerySpec definition and then propagate it through the spec import/export and validation paths: update specFromQuery to populate LabelsIncludeAll from Query.LabelsIncludeAll, update queryFromSpec (svc.queryFromSpec) to set Query.LabelsIncludeAll from spec.LabelsIncludeAll, and update ApplyQuerySpecs premium gating (the check that currently inspects spec.LabelsIncludeAny and calls license.IsPremium(ctx) / setAuthCheckedOnPreAuthErr(ctx)) to also check spec.LabelsIncludeAll so both include-any and include-all are license-gated consistently; also ensure NewQuery/ModifyQuery behavior remains unchanged by testing that created/modified Queries round-trip through spec conversion without dropping LabelsIncludeAll.server/datastore/mysql/policies.go (1)
152-227:⚠️ Potential issue | 🟠 MajorFilter policy label inserts by the policy’s scope.
INSERT ... SELECT ... FROM labels WHERE name IN (?)is still unscoped. If a global label and a team label share the same name, this statement can select the wronglabel_idset for the policy or return extra rows and fail theRowsAffected()check withinvalid label.require_allnow carries that bug intolabels_include_alltoo. This needs a team/global filter tied to the owning policy.As per coding guidelines,
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied... Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/policies.go` around lines 152 - 227, The INSERT ... SELECT in updatePolicyLabelsTx (insertLabelStmt) lacks a team/global scope filter and can match labels with the same name across scopes; change the SELECT's WHERE clause to also filter by label scope using the policy's owning team (e.g., add "AND (team_id IS NULL OR team_id = ?)" or the appropriate team equality depending on your schema) and update the sqlx.In call to pass the policy's team id (or nil/NULL equivalent) as an extra argument so the parameter list and placeholders align; ensure the rowsAffected check still compares to len(labelNames) after the scoped query.server/fleet/policies.go (1)
470-548:⚠️ Potential issue | 🟠 Major
PolicySpecstill can’t expresslabels_include_all.The API/data models were updated for the new scope, but the spec model still only has
LabelsIncludeAny/LabelsExcludeAny, andVerify()hardcodesnilfor the include-all slot. That means the GitOps/apply-policy-specs path can’t create or preservelabels_include_alleven though the rest of the backend now supports it.Suggested direction
type PolicySpec struct { // Name is the name of the policy. Name string `json:"name"` @@ ScriptID *uint `json:"script_id"` LabelsIncludeAny []string `json:"labels_include_any,omitempty"` + LabelsIncludeAll []string `json:"labels_include_all,omitempty"` LabelsExcludeAny []string `json:"labels_exclude_any,omitempty"` @@ - return verifyLabelScopeMutualExclusion(p.LabelsIncludeAny, nil, p.LabelsExcludeAny) + return verifyLabelScopeMutualExclusion(p.LabelsIncludeAny, p.LabelsIncludeAll, p.LabelsExcludeAny) }
ApplyPolicySpecsinserver/datastore/mysql/policies.gowill also need to forward the new field when it builds the temporaryfleet.Policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/fleet/policies.go` around lines 470 - 548, PolicySpec is missing a LabelsIncludeAll field and Verify() currently hardcodes nil for the include-all slot so ApplyPolicySpecs can't create/preserve labels_include_all; add a new field LabelsIncludeAll []string `json:"labels_include_all,omitempty"` to PolicySpec and change Verify() to call verifyLabelScopeMutualExclusion(p.LabelsIncludeAny, p.LabelsIncludeAll, p.LabelsExcludeAny); then update ApplyPolicySpecs in server/datastore/mysql/policies.go to forward PolicySpec.LabelsIncludeAll into the temporary fleet.Policy it constructs so the include-all scope is persisted.server/datastore/mysql/queries.go (1)
359-376:⚠️ Potential issue | 🟠 MajorResolve query labels by scope, not just by name.
This lookup collapses everything into a single
label_name -> label_idmap, so same-named labels from different teams/global scopes overwrite each other at Line 392. In a batched apply, a team-scoped report can end up persisted against another scope’s label ID, which makes bothlabels_include_anyandlabels_include_alltarget the wrong hosts. Please key resolution by query scope as well as name, or resolve labels per query/team instead of using one global name map.As per coding guidelines,
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied... Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results.Also applies to: 385-392, 405-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/queries.go` around lines 359 - 376, The code currently collapses labels by name into lblNamesMap and queries the labels table without scope, causing same-named labels from different scopes/teams to overwrite each other; change resolution to include scope (e.g., team_id or scope field) by building a compound key (scope + label name) from each Query's LabelsIncludeAny/LabelsIncludeAll, collect those scoped keys per scope or per team, and either (A) run sqlx.In queries per scope/team with WHERE name IN (...) AND team_id = ? (or scope = ?), or (B) construct a single query that selects id, name, team_id for a list of (name,team_id) tuples and map results back by the compound key; update the sqlx.In call and any downstream lookups that use lblNamesMap so they index by the compound key instead of just label name (affecting the SELECT at sqlx.In and all uses of lblNamesMap and subsequent label-ID lookups).
🧹 Nitpick comments (3)
server/datastore/mysql/schema.sql (1)
2375-2384: Consider enforcing valid scope states inpolicy_labels.Line 2378 adds
require_all, but(exclude=1, require_all=1)is an undefined scope for the 3-scope model. A DB check would prevent ambiguous rows from any non-API write path.🔧 Suggested migration-level constraint
CREATE TABLE `policy_labels` ( `id` int unsigned NOT NULL AUTO_INCREMENT, `policy_id` int unsigned NOT NULL, `label_id` int unsigned NOT NULL, `exclude` tinyint(1) NOT NULL DEFAULT '0', `created_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, `require_all` tinyint(1) NOT NULL DEFAULT '0', + CONSTRAINT `policy_labels_scope_chk` CHECK (NOT (`exclude` = 1 AND `require_all` = 1)), PRIMARY KEY (`id`), ... )Based on learnings: "The schema.sql file in server/datastore/mysql/ is auto-generated from migrations for use with tests, so it cannot be manually edited. Any changes must be made through migrations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/schema.sql` around lines 2375 - 2384, Add a migration (do not edit schema.sql) that enforces valid scope states on the policy_labels table by adding a CHECK constraint that forbids the ambiguous combination exclude=1 AND require_all=1; implement this by creating a migration which runs ALTER TABLE policy_labels ADD CONSTRAINT chk_policy_labels_scope CHECK (NOT (exclude = 1 AND require_all = 1)) (and include the matching DROP CONSTRAINT in the down migration) so the constraint references the policy_labels table and the exclude and require_all columns.server/datastore/mysql/queries_test.go (1)
1375-1379: Tighten the mutual-exclusion assertion to avoid false positives.
require.Error(t, err)will pass for unrelated failures too. Consider asserting error content/type so this test only passes for the expected validation failure.💡 Suggested assertion hardening
err = ds.SaveQuery(ctx, query1, true, true) require.Error(t, err) + require.ErrorContains(t, err, "mutually exclusive")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/queries_test.go` around lines 1375 - 1379, The test currently uses require.Error(t, err) after calling ds.SaveQuery(ctx, query1, true, true) which can hide unrelated failures; change the assertion to verify the exact validation failure by checking the error type or message (e.g., use require.ErrorIs(t, err, <expected sentinel error>) or require.ErrorContains(t, err, "mutual exclusion") or require.EqualError(t, err, "<expected error string>") so the test only passes for the intended mutual-exclusion validation; refer to the SaveQuery call and the query1 setup in queries_test.go when adding the stronger assertion and use the project’s existing sentinel error (or canonical message) for query mutual-exclusion validation.server/service/integration_enterprise_test.go (1)
29323-29337: Consider extracting shared host factory helper.
mkHostlogic is duplicated across both tests. A small helper in the suite would reduce repetition and keep future host-shape updates in one place.Also applies to: 29480-29488
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_enterprise_test.go` around lines 29323 - 29337, Extract the duplicated mkHost closure into a shared helper method on the test suite (e.g., makeHost or MkHost on the suite struct) and have both tests call that helper instead of declaring identical closures; move the NewHost call and all host field initialization (DetailUpdatedAt, LabelUpdatedAt, PolicyUpdatedAt, SeenTime, OsqueryHostID, NodeKey, UUID, Hostname, Platform) into that single helper (which should accept the suffix string and use t.Name()/suite.T().Name() as before), and replace the duplicated mkHost usages in both locations with calls to the new suite helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/global_policies.go`:
- Line 498: The label existence check in ApplyPolicySpecs misses
LabelsIncludeAll—update the concatenation to include policy.LabelsIncludeAll
(e.g., use slices.Concat(policy.LabelsIncludeAll, policy.LabelsIncludeAny,
policy.LabelsExcludeAny)) so all label lists are validated; also fix
PolicySpec.Verify() to pass p.LabelsIncludeAll (not nil) into the
mutual-exclusion check so conflicts between include_all and other label types
are detected.
In `@server/service/integration_enterprise_test.go`:
- Around line 29447-29454: The test currently only asserts the HTTP status for
the POST to "/api/latest/fleet/queries" (via s.DoJSON with fleet.QueryPayload
and fleet.CreateQueryResponse) which can mask unrelated validation errors;
change the assertion to unmarshal and inspect the response body and assert the
specific mutex error (e.g. check CreateQueryResponse contains the expected mutex
error field/message or that an error string contains the mutex identifier) for
both occurrences where s.DoJSON is called (the "q-rej-..." case and the second
similar case), ensuring the test fails unless the returned payload explicitly
indicates the query mutex rejection.
- Around line 29356-29370: The tests currently only assert a 400 response for
conflicting label fields; update the two POST assertions (the s.DoJSON calls
sending fleet.GlobalPolicyRequest with LabelsIncludeAll + LabelsIncludeAny and
LabelsIncludeAll + LabelsExcludeAny) to also unmarshal the response and assert
the validation error body contains the specific mutual-exclusion validation for
the label fields (e.g., an error entry referencing LabelsIncludeAll or a
message/substr like "mutually exclusive" or "cannot be set together"); locate
the calls that create "rej1-<test>" and "rej2-<test>" and add checks on the
response error field/message so the test fails only when the mutual-exclusion
validation is missing, not on any generic 400.
In `@server/service/queries_test.go`:
- Around line 233-237: The test currently uses require.NotErrorIs(t, err,
fleet.ErrMissingLicense) which will pass for unrelated errors; update the
assertion in the empty-slice ModifyQuery tests (the call to svc.ModifyQuery with
LabelsIncludeAny/All set to empty slices) to assert no error occurred (e.g.,
require.NoError/require.Nil) so the test fails on any unexpected error; apply
the same change to the other instance covering lines 285-292 that tests the
empty-label path.
---
Outside diff comments:
In `@server/datastore/mysql/policies.go`:
- Around line 152-227: The INSERT ... SELECT in updatePolicyLabelsTx
(insertLabelStmt) lacks a team/global scope filter and can match labels with the
same name across scopes; change the SELECT's WHERE clause to also filter by
label scope using the policy's owning team (e.g., add "AND (team_id IS NULL OR
team_id = ?)" or the appropriate team equality depending on your schema) and
update the sqlx.In call to pass the policy's team id (or nil/NULL equivalent) as
an extra argument so the parameter list and placeholders align; ensure the
rowsAffected check still compares to len(labelNames) after the scoped query.
In `@server/datastore/mysql/queries.go`:
- Around line 359-376: The code currently collapses labels by name into
lblNamesMap and queries the labels table without scope, causing same-named
labels from different scopes/teams to overwrite each other; change resolution to
include scope (e.g., team_id or scope field) by building a compound key (scope +
label name) from each Query's LabelsIncludeAny/LabelsIncludeAll, collect those
scoped keys per scope or per team, and either (A) run sqlx.In queries per
scope/team with WHERE name IN (...) AND team_id = ? (or scope = ?), or (B)
construct a single query that selects id, name, team_id for a list of
(name,team_id) tuples and map results back by the compound key; update the
sqlx.In call and any downstream lookups that use lblNamesMap so they index by
the compound key instead of just label name (affecting the SELECT at sqlx.In and
all uses of lblNamesMap and subsequent label-ID lookups).
In `@server/fleet/policies.go`:
- Around line 470-548: PolicySpec is missing a LabelsIncludeAll field and
Verify() currently hardcodes nil for the include-all slot so ApplyPolicySpecs
can't create/preserve labels_include_all; add a new field LabelsIncludeAll
[]string `json:"labels_include_all,omitempty"` to PolicySpec and change Verify()
to call verifyLabelScopeMutualExclusion(p.LabelsIncludeAny, p.LabelsIncludeAll,
p.LabelsExcludeAny); then update ApplyPolicySpecs in
server/datastore/mysql/policies.go to forward PolicySpec.LabelsIncludeAll into
the temporary fleet.Policy it constructs so the include-all scope is persisted.
In `@server/service/queries.go`:
- Around line 714-718: QuerySpec currently lacks a LabelsIncludeAll field
causing label-all semantics to be dropped; add LabelsIncludeAll []string to the
QuerySpec definition and then propagate it through the spec import/export and
validation paths: update specFromQuery to populate LabelsIncludeAll from
Query.LabelsIncludeAll, update queryFromSpec (svc.queryFromSpec) to set
Query.LabelsIncludeAll from spec.LabelsIncludeAll, and update ApplyQuerySpecs
premium gating (the check that currently inspects spec.LabelsIncludeAny and
calls license.IsPremium(ctx) / setAuthCheckedOnPreAuthErr(ctx)) to also check
spec.LabelsIncludeAll so both include-any and include-all are license-gated
consistently; also ensure NewQuery/ModifyQuery behavior remains unchanged by
testing that created/modified Queries round-trip through spec conversion without
dropping LabelsIncludeAll.
---
Nitpick comments:
In `@server/datastore/mysql/queries_test.go`:
- Around line 1375-1379: The test currently uses require.Error(t, err) after
calling ds.SaveQuery(ctx, query1, true, true) which can hide unrelated failures;
change the assertion to verify the exact validation failure by checking the
error type or message (e.g., use require.ErrorIs(t, err, <expected sentinel
error>) or require.ErrorContains(t, err, "mutual exclusion") or
require.EqualError(t, err, "<expected error string>") so the test only passes
for the intended mutual-exclusion validation; refer to the SaveQuery call and
the query1 setup in queries_test.go when adding the stronger assertion and use
the project’s existing sentinel error (or canonical message) for query
mutual-exclusion validation.
In `@server/datastore/mysql/schema.sql`:
- Around line 2375-2384: Add a migration (do not edit schema.sql) that enforces
valid scope states on the policy_labels table by adding a CHECK constraint that
forbids the ambiguous combination exclude=1 AND require_all=1; implement this by
creating a migration which runs ALTER TABLE policy_labels ADD CONSTRAINT
chk_policy_labels_scope CHECK (NOT (exclude = 1 AND require_all = 1)) (and
include the matching DROP CONSTRAINT in the down migration) so the constraint
references the policy_labels table and the exclude and require_all columns.
In `@server/service/integration_enterprise_test.go`:
- Around line 29323-29337: Extract the duplicated mkHost closure into a shared
helper method on the test suite (e.g., makeHost or MkHost on the suite struct)
and have both tests call that helper instead of declaring identical closures;
move the NewHost call and all host field initialization (DetailUpdatedAt,
LabelUpdatedAt, PolicyUpdatedAt, SeenTime, OsqueryHostID, NodeKey, UUID,
Hostname, Platform) into that single helper (which should accept the suffix
string and use t.Name()/suite.T().Name() as before), and replace the duplicated
mkHost usages in both locations with calls to the new suite helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 899ec747-915f-4fde-b9d7-f5496381d2b9
📒 Files selected for processing (19)
changes/415640-policy-reports-new-custom-optionserver/datastore/mysql/hosts.goserver/datastore/mysql/migrations/tables/20260429151737_AddRequireAllToPolicyAndQueryLabels.goserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/datastore/mysql/queries.goserver/datastore/mysql/queries_test.goserver/datastore/mysql/schema.sqlserver/fleet/api_policies.goserver/fleet/labels.goserver/fleet/policies.goserver/fleet/queries.goserver/service/global_policies.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/queries.goserver/service/queries_test.goserver/service/team_policies.gotools/cloner-check/generated_files/query.txt
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/service/integration_core_test.go (1)
16934-16936: Align the test comment with actual coverage (or addexclude_anycase).Line 16936 says free-tier
include/exclude_anystill work, but this test only exercisesLabelsIncludeAny. Either add aLabelsExcludeAnyassertion or narrow the comment to avoid over-claiming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_core_test.go` around lines 16934 - 16936, The test comment for TestLabelScopePremiumGate overstates coverage by claiming both free-tier include/exclude_any work while the test only exercises LabelsIncludeAny; either add a parallel assertion exercising LabelsExcludeAny (e.g., call the same helper/flow used for LabelsIncludeAny but asserting LabelsExcludeAny behavior) or change the comment to only mention LabelsIncludeAny; locate TestLabelScopePremiumGate and update the comment or add a LabelsExcludeAny assertion to match the claim.server/service/queries_test.go (1)
285-292: Consider explicitly testing emptyLabelsIncludeAllinApplyQuerySpecstoo.This test currently proves the empty
LabelsIncludeAnypath. Since this PR introducesinclude_all, addingLabelsIncludeAll: []string{}in the same payload (or an adjacent case) would better guard regressions for explicit-empty handling.Suggested test tweak
err := svc.ApplyQuerySpecs(viewerCtx, []*fleet.QuerySpec{ { Name: "test query", Query: "select 1", LabelsIncludeAny: []string{}, // explicit empty slice, not nil + LabelsIncludeAll: []string{}, // explicit empty slice, not nil }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/queries_test.go` around lines 285 - 292, Add an assertion path that explicitly tests an empty LabelsIncludeAll slice when calling svc.ApplyQuerySpecs: update the test case that currently sets LabelsIncludeAny: []string{} to also include LabelsIncludeAll: []string{} (or add an adjacent call) so ApplyQuerySpecs(viewerCtx, []*fleet.QuerySpec{ ... }) is exercised for both explicit-empty include_any and include_all; ensure the call still checks require.NoError(t, err) to verify explicit-empty handling for the new include_all field in the ApplyQuerySpecs code path.server/service/integration_enterprise_test.go (1)
29395-29402: Add policy PATCH coverage forlabels_include_all+labels_exclude_any.The PATCH mutex test currently covers only include_all/include_any. Add the include_all/exclude_any PATCH rejection as well to fully cover the mutual-exclusion contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_enterprise_test.go` around lines 29395 - 29402, The test currently exercises the PATCH mutex for LabelsIncludeAll + LabelsIncludeAny; add a parallel assertion that a PATCH to the same endpoint via s.DoJSON (the same call pattern used with fleet.ModifyGlobalPolicyRequest and inner fleet.ModifyPolicyPayload) that sets LabelsIncludeAll and LabelsExcludeAny together also returns http.StatusBadRequest; locate the existing PATCH call using s.DoJSON("/api/latest/fleet/policies/%d", ...) and duplicate it replacing LabelsIncludeAny with LabelsExcludeAny (keeping LabelsIncludeAll set to lblA.Name and LabelsExcludeAny set to lblB.Name) and assert the response type is &fleet.ModifyGlobalPolicyResponse{} and status is BadRequest to cover the include_all/exclude_any mutual-exclusion case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/integration_enterprise_test.go`:
- Around line 29395-29402: The test currently only asserts a 400 status for the
PATCH call; update the assertion to verify the specific mutex error by capturing
the response body into a fleet.ModifyGlobalPolicyResponse (or appropriate
response struct) from the s.DoJSON call and assert that the returned error
string equals fleet.ErrPolicyConflictingLabels.Error(); locate the PATCH
invocation used to modify the policy (the s.DoJSON("PATCH",
fmt.Sprintf("/api/latest/fleet/policies/%d", createResp.Policy.ID),
fleet.ModifyGlobalPolicyRequest{...}, http.StatusBadRequest,
&fleet.ModifyGlobalPolicyResponse{}) line) and replace the status-only check
with code that decodes into &fleet.ModifyGlobalPolicyResponse and asserts
resp.Err == fleet.ErrPolicyConflictingLabels.Error().
---
Nitpick comments:
In `@server/service/integration_core_test.go`:
- Around line 16934-16936: The test comment for TestLabelScopePremiumGate
overstates coverage by claiming both free-tier include/exclude_any work while
the test only exercises LabelsIncludeAny; either add a parallel assertion
exercising LabelsExcludeAny (e.g., call the same helper/flow used for
LabelsIncludeAny but asserting LabelsExcludeAny behavior) or change the comment
to only mention LabelsIncludeAny; locate TestLabelScopePremiumGate and update
the comment or add a LabelsExcludeAny assertion to match the claim.
In `@server/service/integration_enterprise_test.go`:
- Around line 29395-29402: The test currently exercises the PATCH mutex for
LabelsIncludeAll + LabelsIncludeAny; add a parallel assertion that a PATCH to
the same endpoint via s.DoJSON (the same call pattern used with
fleet.ModifyGlobalPolicyRequest and inner fleet.ModifyPolicyPayload) that sets
LabelsIncludeAll and LabelsExcludeAny together also returns
http.StatusBadRequest; locate the existing PATCH call using
s.DoJSON("/api/latest/fleet/policies/%d", ...) and duplicate it replacing
LabelsIncludeAny with LabelsExcludeAny (keeping LabelsIncludeAll set to
lblA.Name and LabelsExcludeAny set to lblB.Name) and assert the response type is
&fleet.ModifyGlobalPolicyResponse{} and status is BadRequest to cover the
include_all/exclude_any mutual-exclusion case.
In `@server/service/queries_test.go`:
- Around line 285-292: Add an assertion path that explicitly tests an empty
LabelsIncludeAll slice when calling svc.ApplyQuerySpecs: update the test case
that currently sets LabelsIncludeAny: []string{} to also include
LabelsIncludeAll: []string{} (or add an adjacent call) so
ApplyQuerySpecs(viewerCtx, []*fleet.QuerySpec{ ... }) is exercised for both
explicit-empty include_any and include_all; ensure the call still checks
require.NoError(t, err) to verify explicit-empty handling for the new
include_all field in the ApplyQuerySpecs code path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1700361-3c8c-4a6a-b17b-623392ca6911
📒 Files selected for processing (3)
server/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/queries_test.go
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
nulmete
reviewed
Apr 30, 2026
Member
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
Overall code LGTM.
I guess I'm just missing context or the feature's spec is not fully clear to me 😢
I'll do a 2nd pass once my questions are clarified 👍
| DiscardData bool `json:"discard_data" db:"discard_data"` | ||
| // LabelsIncludeAny is a list of labels that will be used to | ||
| // target a query | ||
| // LabelsIncludeAny scopes the query to hosts that are members of ANY of the listed labels. |
Member
There was a problem hiding this comment.
don't need to change, but just wondering if we can keep comments in a single place (right now it's the same comment in Query and QueryPayload structs, so in the long run we would also have to maintain them and keep them in sync 😢 )
| ErrQueryConflictingLabels = errors.New("report can include at most one of labels_include_any or labels_include_all") | ||
| ) | ||
|
|
||
| // verifyQueryLabelScopeMutualExclusion enforces that at most scope rule is set. |
Member
There was a problem hiding this comment.
Suggested change
| // verifyQueryLabelScopeMutualExclusion enforces that at most scope rule is set. | |
| // verifyQueryLabelScopeMutualExclusion enforces that at most one scope rule is set. |
| policy.LabelsExcludeAny = append(policy.LabelsExcludeAny, fleet.LabelIdent{LabelName: label}) | ||
| } | ||
| // If the client sent any of the three label scope fields, treat all three as authoritative | ||
| // for the policy's label state. The validator on ModifyPolicyPayload enforces that at most |
Member
There was a problem hiding this comment.
The validator refers to p.Verify(), right?
| }) | ||
| } | ||
|
|
||
| // TestLabelScopePremiumGate verifies that the new label scope fields are |
Member
There was a problem hiding this comment.
"new label scope fields" refer just to include_all, correct?
disregard if I'm wrong
Suggested change
| // TestLabelScopePremiumGate verifies that the new label scope fields are | |
| // TestLabelScopePremiumGate verifies that the include_all label scope is |
Fixes #41564. Extends policies and reports with a new scope, 'include_all', used for targetting hosts that are are member of every listed label.
juan-fdz-hawa
force-pushed
the
41564-policyreport-labels-backend
branch
from
207c4f6 to
7c740c5
Compare
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
This was referenced Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #41564
Extends policies and reports with a new scope, 'include_all', used for targetting hosts that are are member of every listed label.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit