Enforce GitOps exceptions - #42191
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #42191 +/- ##
==========================================
+ Coverage 66.64% 68.49% +1.85%
==========================================
Files 2532 1506 -1026
Lines 202882 175149 -27733
Branches 9180 0 -9180
==========================================
- Hits 135218 119977 -15241
+ Misses 55430 43014 -12416
+ Partials 12234 12158 -76
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:
|
1dad91e to
9a34f73
Compare
b385f59 to
21935f8
Compare
There was a problem hiding this comment.
same as the non-deprecated: switching test to expect that top-level key omission means 🔪
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.
Tip: disable this comment in your organization's Code Review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Implements enforcement of GitOps “exceptions” for labels, secrets, and software so that excepted entities cannot be managed via GitOps (error if key is present), and non-excepted entities treat missing keys as “delete all”.
Changes:
- Track YAML key presence for
labels,secrets, andsoftwareduring GitOps parsing. - Enforce exception rules during
fleetctl gitopsapply (error on present keys; delete-all semantics when not excepted and key omitted). - Update/extend unit + integration tests to cover presence tracking and exception behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
server/service/client.go |
Enforces GitOps exceptions and adjusts how secrets/labels/software are applied/cleared. |
pkg/spec/gitops.go |
Adds presence tracking fields and relaxes software requirement for team files; adds HasChanges() helper. |
pkg/spec/gitops_test.go |
Adds tests validating presence tracking behavior. |
cmd/fleetctl/fleetctl/gitops.go |
Adjusts label-change computation to treat omitted labels as delete-all when not excepted. |
cmd/fleetctl/fleetctl/gitops_test.go |
Adds/updates tests for exception enforcement and omitted-key semantics. |
cmd/fleetctl/integrationtest/gitops/*.go |
Updates integration tests to align with new exception + omitted-key behavior. |
cmd/fleetctl/fleetctl/testing_utils/testing_utils.go |
Extends label mocks to support new label-apply flow. |
cmd/fleetctl/fleetctl/testdata/gitops/*.yml |
Updates GitOps fixtures to include labels where needed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughAdds server-side handling and presence tracking for GitOps top-level keys (labels, secrets, software): parsing now records whether each key was present, software parsing can inject server-generated synthetic software per team, and label-change computation gains a 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cmd/fleetctl/fleetctl/testing_utils/testing_utils.go (1)
826-828: Avoid unconditional success inLabelByNameFuncmock.Returning a label for any name can mask “missing label” paths in tests that rely on
AddLabelMocks. Prefer matching the constrained behavior already used inLabelsByNameFunc.Suggested mock tightening
ds.LabelByNameFunc = func(ctx context.Context, name string, filter fleet.TeamFilter) (*fleet.Label, error) { - return &fleet.Label{ID: 1, Name: name}, nil + validLabels := map[string]*fleet.Label{ + "a": {ID: 1, Name: "a"}, + "b": {ID: 2, Name: "b"}, + } + lbl, ok := validLabels[name] + if !ok { + return nil, ¬FoundError{} + } + return lbl, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/testing_utils/testing_utils.go` around lines 826 - 828, The mock for LabelByNameFunc currently returns a label for any name which hides missing-label behavior; update LabelByNameFunc to mirror the constrained behavior used in LabelsByNameFunc (and AddLabelMocks) by only returning a non-nil *fleet.Label when the requested name exists in the test's expected label set (or matches the same lookup/filters used by LabelsByNameFunc), otherwise return (nil, sql.ErrNoRows) or a not-found error so tests exercising missing-label paths behave correctly.cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go (1)
135-137: Avoid leaking GitOps exception state across tests.
SetupSuitenow disables exceptions on the shared server, and the comment explicitly expects some tests to re-enable them. Without a reset inTearDownTestor a helper witht.Cleanup, any exception-on case will leak config into later tests and make the suite order-dependent.♻️ One simple reset point
func (s *enterpriseIntegrationGitopsTestSuite) TearDownTest() { t := s.T() ctx := context.Background() + + appConf, err := s.DS.AppConfig(ctx) + require.NoError(t, err) + appConf.GitOpsConfig.Exceptions = fleet.GitOpsExceptions{} + require.NoError(t, s.DS.SaveAppConfig(ctx, appConf)) mysql.ExecAdhocSQL(t, s.DS, func(q sqlx.ExtContext) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go` around lines 135 - 137, SetupSuite currently sets appConf.GitOpsConfig.Exceptions = fleet.GitOpsExceptions{} which can be mutated by tests and leak into others; add a reset to restore a known default after each test — either modify TearDownTest to set appConf.GitOpsConfig.Exceptions = fleet.GitOpsExceptions{} (or the original saved value) or ensure tests that change exceptions call t.Cleanup to restore the original value; reference the SetupSuite mutation and the TearDownTest helper to implement the per-test reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/fleetctl/fleetctl/gitops_test.go`:
- Around line 790-845: The tests currently only check populated `labels:` and
`secrets:` blocks; add additional bare-key YAML cases mirroring the `software:`
test so presence-only (null) keys are rejected too. In gitops_test.go, create
temp files like tmpFile and tmpFile2 but write YAML that contains only `labels:`
(no items) and only `secrets:` respectively, then call
RunAppNoChecks([]string{"gitops", "-f", tmpFileX.Name()}) and assert an error
contains `"labels" is excepted from GitOps management` and `"secrets" is
excepted from GitOps management` to match the existing `software` bare-key
check.
- Around line 913-931: The test currently only inspects deletedLabels and
appliedSecrets which can be misleading (nil can mean "never called" or "called
with nil"); change the mocks to track explicit invocations/counters and
parameter captures for ApplyLabelSpecsWithAuthorFunc, SetAsideLabelsFunc,
DeleteLabelFunc, LabelsByNameFunc, and ApplyEnrollSecretsFunc (e.g., boolean
flags or call counters and saved args) and assert those flags/counters remain
zero/false and saved args unchanged after the operation so the test verifies
that no write-path functions were called or mutated; also apply the same
explicit invocation assertions for the corresponding mocks referenced around
lines 1029-1032.
In `@pkg/spec/gitops.go`:
- Around line 1735-1747: The code currently returns early when the "software"
key is absent which clears result.Software and prevents downstream resolution
from inheriting server-side software; instead, when softwareRaw is not present
(ok == false) you should not return early—set result.SoftwarePresent = false but
leave result.Software untouched (or mark it to mean "inherit server state") and
continue, so downstream logic that resolves titles/IDs and exception-mode
behavior will treat omitted software as "preserve existing" rather than "delete
all"; adjust the branch around result.global(), the ok checks, and the return of
multiError accordingly (use the existing symbols softwareRaw,
result.SoftwarePresent, result.global(), multiError) so only explicit "software:
null" or non-exception cases cause deletion or errors.
In `@server/service/client.go`:
- Around line 2141-2147: The code currently skips populating team["software"]
only for regular teams but still calls doGitOpsNoTeamSetupAndSoftware (via
gitopsCommand) for the no-team/unassigned path, allowing an empty software
payload to wipe Unassigned when exceptions.Software is true; update the logic so
that when exceptions.Software is true you do not call or synthesize the no-team
software path: add a guard around the call to doGitOpsNoTeamSetupAndSoftware (or
inside gitopsCommand) that checks exceptions.Software and skips creating/sending
an empty software payload for Unassigned, or alternatively have
doGitOpsNoTeamSetupAndSoftware return early if exceptions.Software is true to
avoid touching team["software"] for the Unassigned/no-team case.
---
Nitpick comments:
In `@cmd/fleetctl/fleetctl/testing_utils/testing_utils.go`:
- Around line 826-828: The mock for LabelByNameFunc currently returns a label
for any name which hides missing-label behavior; update LabelByNameFunc to
mirror the constrained behavior used in LabelsByNameFunc (and AddLabelMocks) by
only returning a non-nil *fleet.Label when the requested name exists in the
test's expected label set (or matches the same lookup/filters used by
LabelsByNameFunc), otherwise return (nil, sql.ErrNoRows) or a not-found error so
tests exercising missing-label paths behave correctly.
In `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go`:
- Around line 135-137: SetupSuite currently sets appConf.GitOpsConfig.Exceptions
= fleet.GitOpsExceptions{} which can be mutated by tests and leak into others;
add a reset to restore a known default after each test — either modify
TearDownTest to set appConf.GitOpsConfig.Exceptions = fleet.GitOpsExceptions{}
(or the original saved value) or ensure tests that change exceptions call
t.Cleanup to restore the original value; reference the SetupSuite mutation and
the TearDownTest helper to implement the per-test reset.
🪄 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: 2eb4f006-1b3a-4654-b552-89185c1bdf89
📒 Files selected for processing (11)
cmd/fleetctl/fleetctl/gitops.gocmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testdata/gitops/global_macos_windows_custom_settings_valid.ymlcmd/fleetctl/fleetctl/testing_utils/testing_utils.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_deprecated_test.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.gocmd/fleetctl/integrationtest/gitops/gitops_integration_test.gocmd/fleetctl/integrationtest/gitops/software_test.gopkg/spec/gitops.gopkg/spec/gitops_test.goserver/service/client.go
|
@iansltx some valid-seeming pickups from coderabbit, looking now |
489afd5 to
2e87777
Compare
ee98099 to
2721898
Compare
| } | ||
| // When labels are excepted and the key is omitted, preserve | ||
| // existing labels (no-op). Otherwise delete/update as normal. | ||
| preserveLabels := appConfig.GitOpsConfig.Exceptions.Labels && !config.LabelsPresent |
There was a problem hiding this comment.
Seems like the config.LabelsPresent check here doesn't really do anything for us, because if the exception is set then labels being present will error in doGitops. We do get different label ops out of this, but GitOps will fail anyway.
Can probably leave this as-is but I had to do an LLM-assisted trace through to figure out why we'd care about the one case where config.LabelsPresent would matter for preserveLabels calculation.
iansltx
left a comment
There was a problem hiding this comment.
Before merging this, please make sure the Fleet Free scenario I mentioned is good to go. Also, might be worth editing the comment for clarity on the preserveLabels bit; if you do that, happy to re-approve after that's done.
2721898 to
5082ce2
Compare
Omitting the labels key deletes existing labels unless the labels exception is enabled (see computeLabelChanges). The previous text said omission preserved them, which stopped being true in #42191.
…ons (#50552) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #50551 Two published pages still describe pre-4.84 behavior, telling users that GitOps mode doesn't restrict label editing in the UI. Since 4.84 that's only true when the labels exception is enabled. This PR corrects both pages and documents the exceptions framework. **`docs/Configuration/yaml-files.md`** - `labels` section: rewrote the note around the two exception states. Beyond the sentence the issue flagged, the premise it rested on was also stale: the note said omitting the `labels` key leaves existing labels intact. Since #42191, `computeLabelChanges` (`cmd/fleetctl/fleetctl/gitops.go:947`) branches on `len(specifiedLabels) == 0`, so omitting the key deletes every custom label in that scope unless the labels exception is enabled. Its own tests name this behavior ("labels omitted removes all regular labels when not excepted"). The note now spells out both states and fixes a `label` / `labels` typo. - `gitops` section: added a note that exceptions can't be set in YAML. `Client.DoGitOps` strips the `exceptions` key defensively (`server/service/client.go:726`), so this was worth stating explicitly. **`articles/gitops-mode.md`** - Added an "Exceptions" section covering the three exception types, what an exception does to both the UI and `fleetctl gitops`, and the enroll secrets default. Upgrade behavior is left to the release notes. It notes that exceptions affect `fleetctl gitops` whether or not GitOps mode is on, since neither the apply-path check nor `computeLabelChanges` reads `gitops_mode_enabled`. - "Still available" no longer lists "Add and edit labels" unconditionally. It now points at the exceptions section for labels, software, and enroll secrets. Behavior the docs now match: - UI gating is `GitOpsModeTooltipWrapper` with `entityType="labels"` (`frontend/pages/labels/components/LabelForm/LabelForm.tsx:172`, `NewLabelPage.tsx:676`, `HostsFilterBlock.tsx:223`). `useGitOpsMode` treats an enabled exception as GitOps mode being off for that entity. - Apply-path enforcement is in `server/service/client.go:2219-2242` (premium only). - Defaults: `server/fleet/app.go:1216` for new installs, migration `20260323144117_AddGitOpsExceptionsToAppConfig.go` for upgrades. The backend is unchanged and was already correct. `ModifyLabel` applies no GitOps check, and the per-host label endpoints stay available regardless of GitOps mode or exception state, so this PR is docs-only. # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. Not applicable: documentation-only change, no product behavior change. ## Testing - [x] QA'd all new/changed functionality manually Verified the described behavior against the UI gating, the `fleetctl gitops` apply path, and the exception defaults in code (references above).
Related issue: Resolves #42180
Summary by CodeRabbit
New Features
Behavior Changes
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
labels:key from default.yml clears all global labelslabels:key from a fleet .yml clears all labels for that fleetlabels:key from default.yml clears all global labelslabels:key from a fleet .yml clears all labels for that fleetlabels:key from default .yml leaves existing global labels as-islabels:key from a fleet .yml leaves existing labels as-islabels:key on default .yml generates an errorlabels:key on a fleet .yml generates an errorlabels_include_anyreferencing an existing label succeeds withoutlabels:keylabels_include_anyreferencing an existing label succeeds withoutlabels:keylabels_include_anyreferencing an existing label succeeds withoutlabels:keylabels_include_anyreferencing an existing label succeeds withoutlabels:key (requires software exceptions off)labels_include_anyreferencing an existing label succeeds withoutlabels:key (requires software exceptions off)labels_include_anyreferencing an existing label succeeds withoutlabels:key (requires software exceptions off)secrets:key from default.yml clears all global secretssecrets:key from a fleet .yml clears all secrets for that fleetsecrets:key from default .yml leaves existing global secrets as-issecrets:key from a fleet .yml leaves existing secrets as-issecrets:key on default .yml generates an errorsecrets:key on a fleet .yml generates an errorsoftware:key from no-team.yml/unassigned.yml clears all software for "no team"software:key from a fleet .yml clears all software for that fleetsoftware:key on a fleet .yml clears all software for that fleetsoftware:key on no-team.yml/unassigned.yml clears all software for "no teamsoftware:key from a fleet .yml leaves existing software as-issoftware:key on a fleet .yml generates an errorsoftware:key from no-team.yml/unassigned.yml leaves existing software as-is for "no team"software:key on no-team.yml/unassigned.yml generates an errorinstall_software.hash_sha256referencing an existing package succeeds withoutsoftware:keyinstall_software.app_store_idreferencing an existing VPP app succeeds withoutsoftware:keyfleet_maintained_app_slugreferencing an existing FMA succeeds withoutsoftware:keysoftware:from no-team.yml/unassigned.yml preserves existing no-team softwareinstall_software.hash_sha256referencing existing no-team software succeeds withoutsoftware:keyFor unreleased bug fixes in a release candidate, one of:
Confirmed that the fix is not expected to adversely impact load test results
I don't think so. There is a bit of overhead when this feature is used since we have to fetch software from the server, but it would be done in a specific test, so even if there is an impact it should affect existing load testing, only new, specific tests.