Don't throw gitops-exceptions-related errors on Free tier - #44118
Conversation
![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.
|
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 as they are similar to previous changes (1)
WalkthroughGitOps exception enforcement for YAML-present keys ( 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/client.go (1)
1906-1925:⚠️ Potential issue | 🟠 MajorGating the entire exceptions block behind
IsPremium()silently wipes enroll secrets on Free tier and creates inconsistent exception handling.The system's migration and default initialization (from
server/fleet/app.go:1091and the migration at20260323144117_AddGitOpsExceptionsToAppConfig.go:18) explicitly setExceptions.Secrets = truefor all tiers and instances, intending to preserve existing secrets when a GitOps file omits thesecrets:key. With this change, the entire exceptions block is gated behindIsPremium(), so on Free tier:
exceptionsremains zero-valued locally inDoGitOps- Lines 1938 and 2179 evaluate
!exceptions.Secretsastrue- If incoming YAML omits
secrets:, both conditions are met, andincoming.OrgSettings["secrets"]is overwritten with an empty slice- All enroll secrets are deleted at both org and team scope
This contradicts the migration's stated intent to "preserve current implicit behavior" and creates a silent regression for Free-tier users who relied on omitting
secrets:to keep existing secrets.Additionally, line 407 in
ApplyGroupreadsappconfig.GitOpsConfig.Exceptions.Softwarewithout a premium check, creating inconsistency: if exceptions are truly tier-gated, this read should also be gated.The fix is to load exceptions unconditionally and gate only the error-raising on premium:
Suggested fix
var exceptions fleet.GitOpsExceptions - if appConfig != nil && appConfig.License.IsPremium() { + if appConfig != nil { exceptions = appConfig.GitOpsConfig.Exceptions - if exceptions.Labels && incoming.LabelsPresent { - return nil, errors.New( - `"labels" is excepted from GitOps management. Remove the "labels:" key from your GitOps file or disable the exception in Fleet settings.`) - } - if exceptions.Secrets && incoming.SecretsPresent { - return nil, errors.New( - `"secrets" is excepted from GitOps management. Remove the "secrets:" key from your GitOps file or disable the exception in Fleet settings.`) - } - if exceptions.Software && incoming.SoftwarePresent && incoming.TeamName != nil { - return nil, errors.New( - `"software" is excepted from GitOps management. Remove the "software:" key from your GitOps file or disable the exception in Fleet settings.`) + // Exception enforcement (raising errors) is premium-only, but the "preserve on + // absence" semantics are honored on all tiers per the migration's intent to + // prevent data loss when Exceptions.Secrets=true on Free tier. + if appConfig.License.IsPremium() { + if exceptions.Labels && incoming.LabelsPresent { + return nil, errors.New( + `"labels" is excepted from GitOps management. Remove the "labels:" key from your GitOps file or disable the exception in Fleet settings.`) + } + if exceptions.Secrets && incoming.SecretsPresent { + return nil, errors.New( + `"secrets" is excepted from GitOps management. Remove the "secrets:" key from your GitOps file or disable the exception in Fleet settings.`) + } + if exceptions.Software && incoming.SoftwarePresent && incoming.TeamName != nil { + return nil, errors.New( + `"software" is excepted from GitOps management. Remove the "software:" key from your GitOps file or disable the exception in Fleet settings.`) + } } }This preserves the tier-gated error feature while maintaining consistent exception-reading logic between
DoGitOpsandApplyGroup, and honors the migration's intent to prevent Free-tier secrets loss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/client.go` around lines 1906 - 1925, Do not gate loading of GitOps exceptions behind IsPremium() in DoGitOps; always set exceptions = appConfig.GitOpsConfig.Exceptions (if appConfig != nil) so zero-valued migration defaults like Exceptions.Secrets are respected and secrets are not implicitly wiped, but only perform the premium-only error checks (the errors that return when incoming.SecretsPresent/incoming.LabelsPresent/incoming.SoftwarePresent) when appConfig.License.IsPremium() is true; update logic in DoGitOps to read exceptions unconditionally and only wrap the error-return branches with IsPremium() checks (mirroring ApplyGroup which already reads appconfig.GitOpsConfig.Exceptions without a premium gate) and ensure when exceptions.Secrets is true you do not overwrite incoming.OrgSettings["secrets"] or incoming.TeamSettings["secrets"] with empty values.
🧹 Nitpick comments (1)
server/service/client.go (1)
941-949: Consistency nit:softwareExceptedhere still reads the exception flag without a license check.Now that
DoGitOpsgates all exception handling behindappConfig.License.IsPremium(),ApplyGroupis the lone remaining reader that trustsappconfig.GitOpsConfig.Exceptions.Softwareon any tier. For a tenant that previously hadSoftware=truepersisted (e.g., Premium → Free downgrade), this path will still skip validation inApplyGroupwhileDoGitOpswill process software normally — a subtle split-brain.Consider aligning the check so both call sites have the same notion of when exceptions apply:
♻️ Suggested alignment
- softwareExcepted := viaGitOps && appconfig != nil && appconfig.GitOpsConfig.Exceptions.Software + softwareExcepted := viaGitOps && appconfig != nil && appconfig.License.IsPremium() && appconfig.GitOpsConfig.Exceptions.Software🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/client.go` around lines 941 - 949, The code uses softwareExcepted := viaGitOps && appconfig != nil && appconfig.GitOpsConfig.Exceptions.Software but does not check the license; update softwareExcepted to the same gated definition used by DoGitOps (e.g., include appconfig.License.IsPremium() in the boolean) so exception logic is consistent with validateTeamOrNoTeamMacOSSetupSoftware and avoid split-brain with ApplyGroup/DoGitOps; locate the variable construction (softwareExcepted) and adjust the condition accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/service/client.go`:
- Around line 1906-1925: Do not gate loading of GitOps exceptions behind
IsPremium() in DoGitOps; always set exceptions =
appConfig.GitOpsConfig.Exceptions (if appConfig != nil) so zero-valued migration
defaults like Exceptions.Secrets are respected and secrets are not implicitly
wiped, but only perform the premium-only error checks (the errors that return
when incoming.SecretsPresent/incoming.LabelsPresent/incoming.SoftwarePresent)
when appConfig.License.IsPremium() is true; update logic in DoGitOps to read
exceptions unconditionally and only wrap the error-return branches with
IsPremium() checks (mirroring ApplyGroup which already reads
appconfig.GitOpsConfig.Exceptions without a premium gate) and ensure when
exceptions.Secrets is true you do not overwrite incoming.OrgSettings["secrets"]
or incoming.TeamSettings["secrets"] with empty values.
---
Nitpick comments:
In `@server/service/client.go`:
- Around line 941-949: The code uses softwareExcepted := viaGitOps && appconfig
!= nil && appconfig.GitOpsConfig.Exceptions.Software but does not check the
license; update softwareExcepted to the same gated definition used by DoGitOps
(e.g., include appconfig.License.IsPremium() in the boolean) so exception logic
is consistent with validateTeamOrNoTeamMacOSSetupSoftware and avoid split-brain
with ApplyGroup/DoGitOps; locate the variable construction (softwareExcepted)
and adjust the condition accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8136417a-0cfc-40f8-8981-3cd5ff9d3887
📒 Files selected for processing (1)
server/service/client.go
There was a problem hiding this comment.
Pull request overview
This PR adjusts fleetctl gitops client-side GitOps exception enforcement so that “excepted from GitOps management” validation errors are only triggered for Premium-tier licenses, aligning behavior with Free-tier expectations.
Changes:
- Gate GitOps exception enforcement behind
appConfig.License.IsPremium(). - Update the inline comment to indicate the enforcement is premium-only.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44118 +/- ##
==========================================
+ Coverage 63.40% 66.77% +3.37%
==========================================
Files 2628 2628
Lines 211127 211158 +31
Branches 9536 9536
==========================================
+ Hits 133856 141002 +7146
+ Misses 64922 57334 -7588
- Partials 12349 12822 +473
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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleetctl/fleetctl/gitops_test.go (1)
632-717: LGTM — test correctly validates free-tier bypass of exception enforcement.The test appropriately mirrors
TestGitOpsExceptionEnforcementwith inverted assertions for free tier, and the handling of the software/team-file case is pragmatic given free-tier team limitations.One small nit: the software case at lines 712–716 only verifies the specific exception-enforcement error isn't present, but otherwise accepts any error silently. If the team-file path on free tier ever starts failing for an unexpected new reason, this branch will still pass. Consider documenting the allowed failure modes or capturing the expected error to tighten the contract:
Optional: tighten the software case assertion
_, err = RunAppNoChecks([]string{"gitops", "-f", tmpFile3.Name()}) - // Free tier may reject team files for other reasons, but it must NOT be the exception error. + // Free tier rejects team files (teams are premium), but the failure must NOT be the + // exception-enforcement error — any other error is acceptable. if err != nil { assert.NotContains(t, err.Error(), `"software" is excepted from GitOps management`) + // Sanity check: confirm the failure is the expected premium-gated-teams error, + // so regressions in free-tier behavior don't silently pass this test. + assert.ErrorContains(t, err, "Teams are available in Fleet Premium") }(Adjust the expected substring to whatever free-tier currently emits for team files.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/gitops_test.go` around lines 632 - 717, The software/team-file branch in TestGitOpsExceptionEnforcementFreeTier currently only checks that the specific exception message is not present and otherwise ignores any error; update the assertion around the RunAppNoChecks call for tmpFile3 to more tightly specify allowed outcomes: either require.NoError(t, err) if free-tier should accept team files, or require.ErrorContains(t, err, "<expected free-tier team-file error>") combined with assert.NotContains(t, err.Error(), `"software" is excepted from GitOps management`) to ensure we still fail if the exception-enforcement message appears; modify the block that references tmpFile3/RunAppNoChecks accordingly (replace the current if err != nil { assert.NotContains... } with the chosen stricter assertion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/fleetctl/fleetctl/gitops_test.go`:
- Around line 632-717: The software/team-file branch in
TestGitOpsExceptionEnforcementFreeTier currently only checks that the specific
exception message is not present and otherwise ignores any error; update the
assertion around the RunAppNoChecks call for tmpFile3 to more tightly specify
allowed outcomes: either require.NoError(t, err) if free-tier should accept team
files, or require.ErrorContains(t, err, "<expected free-tier team-file error>")
combined with assert.NotContains(t, err.Error(), `"software" is excepted from
GitOps management`) to ensure we still fail if the exception-enforcement message
appears; modify the block that references tmpFile3/RunAppNoChecks accordingly
(replace the current if err != nil { assert.NotContains... } with the chosen
stricter assertion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f89f8cbf-9ed8-4ac7-acb9-01e6f998960d
📒 Files selected for processing (1)
cmd/fleetctl/fleetctl/gitops_test.go
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #44118 # Details On free tier, ignore exceptions and always apply enroll secrets when present. # 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/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. n/a, unreleased ## Testing - [X] Added/updated automated tests - [X] QA'd all new/changed functionality manually @AndreyKizimenko QA'd manually For unreleased bug fixes in a release candidate, one of: - [X] Confirmed that the fix is not expected to adversely impact load test results
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #44118 # Details On free tier, ignore exceptions and always apply enroll secrets when present. # 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/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. n/a, unreleased ## Testing - [X] Added/updated automated tests - [X] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [X] QA'd all new/changed functionality manually @AndreyKizimenko QA'd manually For unreleased bug fixes in a release candidate, one of: - [X] Confirmed that the fix is not expected to adversely impact load test results <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed GitOps to correctly apply enrollment secrets and labels on free tier licenses, even when exception flags are configured. * **Tests** * Added tests validating that GitOps properly applies secrets and labels for free tier customers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Related issue: Resolves #44098
Details
We set the "secrets" exception on for all new instances (and the label exception for existing instances), but you can't turn them off in the free tier. That means GitOps runs (including the one we use to initialize new instances) would fail with the "you can't use this key because the exception is on" error. This PR fixes the issue by not enforcing that rule for free tier instances.
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.
n/a, unreleased
Testing
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit