Fix missing GitOps label validation for invalid field combinations#44410
Merged
sharon-fdm merged 19 commits intomainfrom May 5, 2026
Merged
Fix missing GitOps label validation for invalid field combinations#44410sharon-fdm merged 19 commits intomainfrom
sharon-fdm merged 19 commits intomainfrom
Conversation
GitOps silently accepted labels with invalid parameter combinations (e.g. manual labels with query/criteria/platform). Add per-type validation in both the GitOps parser and the service layer. Closes #34229
![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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds centralized validation for label membership fields via a new exported function 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/labels_test.go (1)
771-860: Add a missing negative test: dynamic label without query.This block covers many invalid combinations, but it doesn’t assert that a dynamic label with empty
Queryis rejected. Adding that case will lock in expected service behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/labels_test.go` around lines 771 - 860, The tests miss a negative case asserting that a dynamic label with an empty or missing Query is rejected; add a test call to svc.ApplyLabelSpecs passing a fleet.LabelSpec with Name like "dynamic_no_query", LabelMembershipType: fleet.LabelMembershipTypeDynamic and no Query (or empty string) and assert an error containing a message like "declared as dynamic but contains no query" to mirror the other validations; locate the test near the other invalid-combination checks in labels_test.go and use the same pattern (require.Error and require.ErrorContains) as the surrounding cases.
🤖 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/labels.go`:
- Around line 590-600: In the fleet.LabelMembershipTypeDynamic branch (the
switch case that checks fleet.LabelMembershipTypeDynamic), add a validation that
rejects empty or nil spec.Query in the same way you already reject
spec.HostVitalsCriteria and spec.Hosts: return
fleet.NewUserMessageError(ctxerr.Errorf(ctx, "label %s is declared as dynamic
but missing query", spec.Name), http.StatusUnprocessableEntity). Ensure this
check runs alongside the existing checks for spec.HostVitalsCriteria and
len(spec.Hosts) so dynamic labels must provide a non-empty Query.
---
Nitpick comments:
In `@server/service/labels_test.go`:
- Around line 771-860: The tests miss a negative case asserting that a dynamic
label with an empty or missing Query is rejected; add a test call to
svc.ApplyLabelSpecs passing a fleet.LabelSpec with Name like "dynamic_no_query",
LabelMembershipType: fleet.LabelMembershipTypeDynamic and no Query (or empty
string) and assert an error containing a message like "declared as dynamic but
contains no query" to mirror the other validations; locate the test near the
other invalid-combination checks in labels_test.go and use the same pattern
(require.Error and require.ErrorContains) as the surrounding cases.
🪄 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: 96c1a917-29e9-4ec0-83b6-51fc58ac552e
📒 Files selected for processing (5)
changes/34229-gitops-label-validationpkg/spec/gitops.gopkg/spec/gitops_test.goserver/service/labels.goserver/service/labels_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes fleetctl gitops label validation so invalid field combinations (e.g. manual labels specifying query/criteria/platform) are rejected with clear errors, addressing #34229.
Changes:
- Added per-label-type validation in the GitOps label parser (
pkg/spec/gitops.go) for mutually exclusive/required fields and dynamic platform value validation. - Added service-layer validation for invalid label field combinations in
ApplyLabelSpecs(server/service/labels.go). - Expanded automated test coverage for invalid/valid label specs in both GitOps parsing and service application.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
server/service/labels.go |
Adds service-layer validation of mutually exclusive fields per label membership type. |
server/service/labels_test.go |
Adds service tests ensuring invalid label spec combinations are rejected. |
pkg/spec/gitops.go |
Adds GitOps parser validation for per-type field rules and dynamic platform validation. |
pkg/spec/gitops_test.go |
Adds GitOps tests covering invalid field combinations and valid cases. |
changes/34229-gitops-label-validation |
User-visible changelog entry for the validation fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44410 +/- ##
=======================================
Coverage 66.68% 66.69%
=======================================
Files 2651 2651
Lines 213525 213609 +84
Branches 9768 9767 -1
=======================================
+ Hits 142392 142466 +74
- Misses 58167 58180 +13
+ Partials 12966 12963 -3
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:
|
Collaborator
Author
|
@claude review |
…ation into switch - Dynamic case now rejects empty query (matches GitOps parser) - Platform validation moved inside dynamic case only (manual/host_vitals already reject any platform, so the top-level check was misleading) - Added test for dynamic label missing query
# Conflicts: # server/service/labels_test.go
Legacy manual/host_vitals labels may have a non-empty platform in the DB. generate-gitops would include it in the YAML, breaking the round-trip since the new validation rejects platform on those types.
# Conflicts: # it-and-security/lib/all/labels/nudge-test-devices.yml
Address review: ValidateLabelMembershipFields now returns *InvalidArgumentError with per-field entries (platform, query, criteria, hosts) instead of generic errors. Also removes the leftover broad platform check in gitops.go and strips platform from non-dynamic labels in fleetctl get labels output to fix the get→apply round-trip for legacy data.
Use err.WithStatus(422) instead of wrapping in UserMessageError, so the encoder preserves field-specific entries matching NewLabel behavior.
Strip Query/HostVitalsCriteria/Platform/Hosts per membership type in fleetctl get labels output so legacy rows with type-mismatched fields survive the dump-and-reapply round-trip. Also add missing criteria column to ds.GetLabelSpec SELECT to match ds.GetLabelSpecs, fixing host_vitals labels fetched by name.
Collaborator
Author
Review Comments Addressed
All 17 review threads resolved. |
nulmete
approved these changes
May 5, 2026
Member
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
LGTM.
Tested a few scenarios below:
- gitops surfaces multiple errors when there's an invalid combination
- created a manual label with platform="windows" (AFAICS we could have some records in prod with this configuration) - then verified that getting the YAML for it doesn't include the platform
- applied gitops with a valid manual label and verified it shows up on the UI
NOTE: I see we're referencing "teams" instead of "fleets" so I'll create a separate issue for that.
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
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: Closes #34229
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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
fleetctl gitopssilently accepted labels with invalid parameter combinations (e.g. manual labels with query/criteria/platform). Added per-type field validation in a centralizedfleet.ValidateLabelMembershipFieldsfunction, called from the GitOps parser,ApplyLabelSpecs, andNewLabel.manualname,description,hostsquery,criteria,platformdynamicname,description,query,platformcriteria,hosts; validates platform valuehost_vitalsname,description,criteriaquery,platform,hostsAutomated tests
TestLabelInvalidFieldCombinationsinpkg/spec/gitops_test.go— 17 sub-tests covering every invalid combination per label type, plus 3 valid happy-path cases.TestNewLabelFieldValidationinserver/service/labels_test.go— 4 cases for NewLabel validation.TestApplyLabelSpecsManualLabelNilHosts— 10 sub-cases for ApplyLabelSpecs field validation.TestWhenCreatingNewLabelsPlatformIsValidated— platform validation across NewLabel and ApplyLabelSpecs.All existing
pkg/specandserver/servicelabel tests pass.Summary by CodeRabbit
Manual test results
Ran against a local Fleet server with the built binary.
API - NewLabel (POST /api/latest/fleet/labels)
platformplatformplatformqueryAPI - ApplyLabelSpecs (POST /api/latest/fleet/spec/labels)
queryhostsRound-trip: get labels --yaml then apply
GitOps parser (fleetctl gitops --dry-run)
Code walkthrough
server/fleet/labels.go— AddedValidateLabelMembershipFields(*LabelSpec) *InvalidArgumentError. This is the single source of truth for label field validation, returning field-specific errors (platform,query,criteria,hosts). Lives here because this package defines the label types both callers import. Also usesstrings.TrimSpaceto reject whitespace-only queries.server/service/labels.go— Three changes: (1) Removed the early blanket platform check fromNewLabelthat ran before the membership type was known. (2) AddedValidateLabelMembershipFieldscall inNewLabelafter type inference, so the API rejects invalid combos at creation time. (3) Replaced three incomplete inline checks inApplyLabelSpecswith a single call to the centralized function, usingerr.WithStatus(422)to preserve field-specific error shape in the API response.pkg/spec/gitops.go— Replaced the inline validation switch and a standaloneValidLabelPlatformVariantscheck with a call toValidateLabelMembershipFields. Unwraps the returned errors individually intomultiErrorso all validation problems are reported to the user at once.cmd/fleetctl/fleetctl/generate_gitops.go— Gated platform emission onLabelMembershipTypeDynamicso legacy manual/host_vitals labels with a stored platform don't produce YAML that fails re-import.cmd/fleetctl/fleetctl/get.go— AddedstripMismatchedLabelFieldswhich clears type-inappropriate fields (query, platform, criteria, hosts) per membership type before YAML output. Called in both code paths: listing all labels and fetching a single label by name. Ensures theget labels --yaml→applyround-trip works for legacy data.server/datastore/mysql/labels.go— Added missingl.criteriacolumn toGetLabelSpecSELECT, matchingGetLabelSpecs. Without it, host_vitals labels fetched by name lost their criteria in the YAML output, causing re-import to fail with the new validation.