Add macOS up-to-date policies driven by Apple GDMF + grace days - #50383
Add macOS up-to-date policies driven by Apple GDMF + grace days#50383cacaosteve wants to merge 1 commit into
Conversation
|
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 Fleet-maintained macOS currency policies from Apple’s GDMF catalog. The change stores update assets, computes version floors with a 30-day grace period, updates Fleet-managed policy queries, and refreshes them hourly. It adds Fleet-managed policy ownership validation, database uniqueness, GitOps reconciliation, and context-aware GDMF retry cancellation. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ 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: 7
🧹 Nitpick comments (5)
server/mdm/apple/gdmf/sync_test.go (1)
54-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd error-path and empty-asset coverage.
The test covers only the happy path. Three uncovered branches carry real behavior:
getAssetMetadataFnreturns an error, so no datastore call must happen.UpdatePolicyQueriesByNamereturns an error, so the sync must stop and wrap the error.- The GDMF response contains no macOS assets, so
SyncMacOSCurrencyPoliciesmust returnnilwithout callingUpdatePolicyQueriesByName.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mdm/apple/gdmf/sync_test.go` around lines 54 - 69, Extend the tests for SyncMacOSCurrencyPolicies with cases where getAssetMetadataFn returns an error and verify no datastore update occurs, UpdatePolicyQueriesByName returns an error and the sync stops with a wrapped error, and the GDMF response has no macOS assets and returns nil without updating policy queries.server/datastore/mysql/apple_software_update_assets.go (1)
29-44: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider one batched multi-row
INSERTinstead of a per-asset round trip.The loop issues one
ExecContextper asset. The GDMF macOS list is small today, so the impact is limited. A single statement with repeatedVALUEStuples still reduces round trips and keeps the transaction short.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_software_update_assets.go` around lines 29 - 44, Replace the per-asset ExecContext calls in the asset insertion loop with one batched multi-row INSERT using repeated VALUES tuples and the existing asset fields, while preserving the empty/invalid SupportedDevices fallback and ctxerr.Wrap error handling.server/mdm/apple/gdmf/sync.go (2)
70-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
MacOSAssetsForCurrencyPoliciesfor the asset-set fallback.Lines 72-75 repeat the
AssetSetsthenPublicAssetSetsfallback thatMacOSAssetsForCurrencyPoliciesalready implements inserver/mdm/apple/gdmf/macos_versions.golines 154-162. Two copies of the same precedence rule can drift, and the persisted rows would then disagree with the rows used for the policy floors.♻️ Proposed deduplication
- // Prefer AssetSets (fuller history); fall back to PublicAssetSets. - src := meta.AssetSets.MacOS - if len(src) == 0 { - src = meta.PublicAssetSets.MacOS - } + // Prefer AssetSets (fuller history); fall back to PublicAssetSets. + src := MacOSAssetsForCurrencyPolicies(meta)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mdm/apple/gdmf/sync.go` around lines 70 - 75, Update replaceMacOSAssets to obtain its macOS asset source through MacOSAssetsForCurrencyPolicies instead of duplicating the AssetSets-then-PublicAssetSets fallback; preserve the existing precedence and use the shared helper’s result when persisting asset rows.
90-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the date helper and skip the marshal for the empty case.
Two small points:
parsePostingDatenow parsesExpirationDateas well. Rename it toparseGDMFDateso the name matches both call sites.json.Marshalruns before the emptiness check, so the"null"result it produces for a nil slice is computed and then discarded. Check the length first.♻️ Proposed reordering
- devices, err := json.Marshal(a.SupportedDevices) - if err != nil { - return ctxerr.Wrap(ctx, err, "marshal supported devices") - } - if len(a.SupportedDevices) == 0 { - devices = []byte("[]") - } - row.SupportedDevices = devices + devices := []byte("[]") + if len(a.SupportedDevices) > 0 { + devices, err = json.Marshal(a.SupportedDevices) + if err != nil { + return ctxerr.Wrap(ctx, err, "marshal supported devices") + } + } + row.SupportedDevices = devices🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mdm/apple/gdmf/sync.go` around lines 90 - 102, Rename the date helper parsePostingDate to parseGDMFDate and update both PostingDate and ExpirationDate call sites. In the supported-devices handling, check len(a.SupportedDevices) before calling json.Marshal, use [] for the empty case, and marshal only when the slice is non-empty.server/mdm/apple/gdmf/macos_versions_test.go (1)
25-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a two-digit minor version case to the query assertions.
This assertion pins the current query text, so the string-comparison defect flagged on
server/mdm/apple/gdmf/macos_versions.golines 135-149 stays invisible. After the predicate becomes version-aware, add a case with a floor such as26.10.0and assert that a host on26.10.0satisfies the generated predicate while26.9.0does not.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mdm/apple/gdmf/macos_versions_test.go` around lines 25 - 35, Add a two-digit minor-version test case in the “grace 0 requires latest per major” coverage around RequiredMacOSVersions and PolicyQuery, using a floor such as 26.10.0; assert the generated predicate accepts 26.10.0 but rejects 26.9.0, so version-aware comparison behavior is covered beyond exact query-text assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/dogfood-policy-updater-latest-macos.sh:
- Around line 109-113: Scope each floor generated by query() to its own
major-version track by combining a version LIKE '{major}.%' guard with the
existing version comparison. Regenerate or manually update the queries in
.github/scripts/dogfood-policy-updater-latest-macos.sh#L109-L113,
it-and-security/lib/macos/policies/latest-macos.yml#L2-L4,
it-and-security/lib/macos/policies/acceptable-macos.yml#L1-L4, and both relevant
queries in
docs/01-Using-Fleet/standard-query-library/standard-query-library.yml#L1265-L1279;
also apply the same scoped logic in server/mdm/apple/gdmf/macos_versions.go.
- Around line 32-122: Replace the eval-based handoff around the Python generator
with non-executable data transfer: have the Python block emit structured JSON or
plain NAME=value records, then assign UP_TO_DATE_QUERY, ACCEPTABLE_QUERY,
LATEST_FLOORS, and ACCEPTABLE_FLOORS using jq or shell read operations. Preserve
the existing floors and query generation logic while ensuring network-derived
output is never re-parsed as shell code.
- Around line 163-186: Update replace_policy_query in the embedded Python
updater to terminate with a non-zero status when pattern.subn returns anything
other than exactly one match, instead of returning the original text after only
warning. Preserve the existing successful replacement behavior and ensure either
unmatched policy name causes the Python process, and therefore the workflow, to
fail.
In `@server/datastore/mysql/apple_software_update_assets.go`:
- Around line 14-47: Replace the delete-and-reinsert flow in
ReplaceAppleSoftwareUpdateAssets with an upsert keyed by (class,
product_version, build), preserving first_seen_at and changing updated_at only
when persisted asset values differ. After upserting, delete only rows for the
class whose product_version/build pairs are absent from assets; when assets is
empty, delete all rows for the class. Use the migration’s existing unique key
and maintain the current JSON normalization and transaction retry behavior.
In `@server/datastore/mysql/policies.go`:
- Around line 400-411: Update UpdatePolicyQueriesByName to include a platform
predicate restricting matches to macOS policies, while preserving the existing
normalized name and query-difference conditions. Ensure the corresponding query
arguments use the platform value Fleet stores for macOS so
SyncMacOSCurrencyPolicies cannot modify unrelated global or team policies with
the same name.
In `@server/mdm/apple/gdmf/macos_versions.go`:
- Around line 135-149: Update PolicyQuery to generate numeric component
comparisons for each VersionFloor instead of comparing os_version.version as
text. Build each track predicate with the major equality and minor/patch
boundary logic described, then combine the per-floor groups with OR while
preserving the empty-floor and terminating-semicolon behavior.
In `@server/mdm/apple/gdmf/sync.go`:
- Around line 25-31: Update the GDMF metadata-fetch indirection around
getAssetMetadataFn so it accepts context.Context, and pass ctx from the sync
flow into the underlying GetAssetMetadata request. Preserve the existing error
wrapping and replaceMacOSAssets behavior while ensuring the outbound request
uses the cron context for cancellation and deadlines.
---
Nitpick comments:
In `@server/datastore/mysql/apple_software_update_assets.go`:
- Around line 29-44: Replace the per-asset ExecContext calls in the asset
insertion loop with one batched multi-row INSERT using repeated VALUES tuples
and the existing asset fields, while preserving the empty/invalid
SupportedDevices fallback and ctxerr.Wrap error handling.
In `@server/mdm/apple/gdmf/macos_versions_test.go`:
- Around line 25-35: Add a two-digit minor-version test case in the “grace 0
requires latest per major” coverage around RequiredMacOSVersions and
PolicyQuery, using a floor such as 26.10.0; assert the generated predicate
accepts 26.10.0 but rejects 26.9.0, so version-aware comparison behavior is
covered beyond exact query-text assertions.
In `@server/mdm/apple/gdmf/sync_test.go`:
- Around line 54-69: Extend the tests for SyncMacOSCurrencyPolicies with cases
where getAssetMetadataFn returns an error and verify no datastore update occurs,
UpdatePolicyQueriesByName returns an error and the sync stops with a wrapped
error, and the GDMF response has no macOS assets and returns nil without
updating policy queries.
In `@server/mdm/apple/gdmf/sync.go`:
- Around line 70-75: Update replaceMacOSAssets to obtain its macOS asset source
through MacOSAssetsForCurrencyPolicies instead of duplicating the
AssetSets-then-PublicAssetSets fallback; preserve the existing precedence and
use the shared helper’s result when persisting asset rows.
- Around line 90-102: Rename the date helper parsePostingDate to parseGDMFDate
and update both PostingDate and ExpirationDate call sites. In the
supported-devices handling, check len(a.SupportedDevices) before calling
json.Marshal, use [] for the empty case, and marshal only when the slice is
non-empty.
🪄 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 Plus
Run ID: 0842257d-cfd6-4bf7-9811-71bddfbab2a1
📒 Files selected for processing (18)
.github/scripts/dogfood-policy-updater-latest-macos.shchanges/3934-macos-up-to-date-gdmf-policiescmd/fleet/cron.gocmd/fleet/cron_registration.godocs/01-Using-Fleet/standard-query-library/standard-query-library.ymlit-and-security/fleets/workstations.ymlit-and-security/lib/macos/policies/acceptable-macos.ymlit-and-security/lib/macos/policies/latest-macos.ymlserver/datastore/mysql/apple_software_update_assets.goserver/datastore/mysql/policies.goserver/fleet/apple_software_update_assets.goserver/fleet/cron_schedules.goserver/fleet/datastore.goserver/mdm/apple/gdmf/macos_versions.goserver/mdm/apple/gdmf/macos_versions_test.goserver/mdm/apple/gdmf/sync.goserver/mdm/apple/gdmf/sync_test.goserver/mock/datastore_mock.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50383 +/- ##
==========================================
+ Coverage 68.24% 68.26% +0.01%
==========================================
Files 3949 3953 +4
Lines 252226 252731 +505
Branches 13328 13328
==========================================
+ Hits 172140 172524 +384
- Misses 64671 64743 +72
- Partials 15415 15464 +49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
server/fleet/datastore.go (1)
1451-1454: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the empty-set contract.
The MySQL implementation rejects an empty
assetsslice with an error (server/datastore/mysql/apple_software_update_assets.golines 16-18). The interface comment does not state this. Add it so callers do not treat an empty feed as a valid "delete all" request.📝 Proposed doc addition
// ReplaceAppleSoftwareUpdateAssets upserts the provided GDMF assets for the // class (preserving first_seen_at on existing rows) and deletes rows absent - // from the new set. Used by the apple_software_update_assets cron. + // from the new set. It returns an error if assets is empty, so an empty + // upstream feed cannot erase the cache. Used by the + // apple_software_update_assets cron.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/fleet/datastore.go` around lines 1451 - 1454, Update the ReplaceAppleSoftwareUpdateAssets interface comment to explicitly state that an empty assets slice is rejected and does not delete all existing rows, matching the MySQL implementation’s contract.server/fleet/policies.go (1)
793-804: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReference name constants instead of repeating the literals.
server/mdm/apple/gdmf/macos_versions.godeclaresPolicyNameUpToDate,DogfoodPolicyNameUpToDate,PolicyNameAcceptable, andDogfoodPolicyNameAcceptablewith these same four strings. The literals now exist in two packages, so a rename in one place silently breaks the mapping in the other. Declare the four names in this file next to the key constants and let thegdmfpackage alias them, as it already does for the keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/fleet/policies.go` around lines 793 - 804, Define shared constants for the four macOS policy names alongside the Fleet-managed key constants, then update FleetManagedKeyForPolicyName to switch on those constants instead of repeating string literals. Expose these constants for the gdmf package to alias, replacing its duplicate name declarations while preserving the existing key alias pattern and mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies.go`:
- Around line 34-60: The migration’s macOS backfills can assign the same
fleet-managed key to multiple policies in one team scope. In
20260801062925_AddFleetManagedKeyToPolicies.go, restrict each UPDATE to the
single lowest-id matching policy per COALESCE(team_id, 0) scope; in
20260801062925_AddFleetManagedKeyToPolicies_test.go, add both supported policy
names in one Darwin scope before applyNext and assert migration success with
exactly one row receiving each key.
In `@server/datastore/mysql/policies.go`:
- Around line 430-437: Move the cleanupPolicy calls out of the withRetryTxx
transaction that processes the GDMF query update. Commit the transaction after
resetPolicyAutomationAttempts succeeds for all IDs, then iterate over the
returned IDs and invoke cleanupPolicy with the same full-membership cleanup
arguments outside the transaction, preserving error wrapping and relying on
needs_full_membership_cleanup for interrupted cleanup recovery.
In `@server/fleet/policies.go`:
- Around line 667-672: Validate FleetManagedKey at the ApplyPolicySpecs
policy-spec validation boundary before persistence, accepting only known
Fleet-managed keys and rejecting or ignoring user-supplied unknown values;
ensure arbitrary policy types cannot receive Fleet-managed keys and
duplicate-key database errors are prevented. Anchor the change around
FleetManagedKey handling in ApplyPolicySpecs and preserve Fleet’s existing
policy-name-based derivation behavior.
---
Nitpick comments:
In `@server/fleet/datastore.go`:
- Around line 1451-1454: Update the ReplaceAppleSoftwareUpdateAssets interface
comment to explicitly state that an empty assets slice is rejected and does not
delete all existing rows, matching the MySQL implementation’s contract.
In `@server/fleet/policies.go`:
- Around line 793-804: Define shared constants for the four macOS policy names
alongside the Fleet-managed key constants, then update
FleetManagedKeyForPolicyName to switch on those constants instead of repeating
string literals. Expose these constants for the gdmf package to alias, replacing
its duplicate name declarations while preserving the existing key alias pattern
and mappings.
🪄 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 Plus
Run ID: 63bc38c9-f0ef-4f88-aa34-da5387352c88
📒 Files selected for processing (16)
.github/scripts/dogfood-policy-updater-latest-macos.shdocs/01-Using-Fleet/standard-query-library/standard-query-library.ymlit-and-security/lib/macos/policies/acceptable-macos.ymlit-and-security/lib/macos/policies/latest-macos.ymlserver/datastore/mysql/apple_software_update_assets.goserver/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies.goserver/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies_test.goserver/datastore/mysql/policies.goserver/fleet/apple_software_update_assets.goserver/fleet/datastore.goserver/fleet/policies.goserver/mdm/apple/gdmf/macos_versions.goserver/mdm/apple/gdmf/macos_versions_test.goserver/mdm/apple/gdmf/sync.goserver/mdm/apple/gdmf/sync_test.goserver/mock/datastore_mock.go
🚧 Files skipped from review as they are similar to previous changes (7)
- server/fleet/apple_software_update_assets.go
- server/mdm/apple/gdmf/sync.go
- server/mock/datastore_mock.go
- server/mdm/apple/gdmf/macos_versions.go
- docs/01-Using-Fleet/standard-query-library/standard-query-library.yml
- it-and-security/lib/macos/policies/latest-macos.yml
- it-and-security/lib/macos/policies/acceptable-macos.yml
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/datastore/mysql/policies.go (1)
1793-1794: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake ownership updates presence-aware.
Line 1823 converts both an omitted field and an explicit empty value to nil. Line 1794 then preserves the existing database value for nil. An existing Fleet-managed policy therefore cannot become user-owned through
ApplyPolicySpecs, althoughPolicySpecdocuments empty as user-owned. Use a presence-aware input and writeNULLfor an explicit clear while preserving the value only when the field is omitted.Also applies to: 1820-1826
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/policies.go` around lines 1793 - 1794, The policy ownership update in ApplyPolicySpecs must distinguish an omitted fleet-managed-key field from an explicitly empty value. Preserve the existing database value only when the field is absent, but write NULL when the caller explicitly clears it; update the input/presence handling around the policy spec conversion and the fleet_managed_key SQL assignment (including the corresponding logic near the referenced later lines).
♻️ Duplicate comments (2)
server/fleet/policies.go (1)
668-672: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
FleetManagedKeybefore persistence.
PolicySpec.FleetManagedKeyaccepts any non-empty string, andApplyPolicySpecswrites it topolicies.fleet_managed_key. A caller can mark an unrelated policy as a macOS Fleet-managed policy, so the GDMF refresh can rewrite its query. A duplicate scoped key instead fails at the database constraint. Validate the key against the allowlist and enforce the compatible policy scope before insertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/fleet/policies.go` around lines 668 - 672, Validate PolicySpec.FleetManagedKey in ApplyPolicySpecs before inserting or updating policies: accept only supported Fleet-managed keys and require each key’s compatible policy scope, rejecting unknown keys or mismatched scopes before persistence. Preserve the existing duplicate-key database constraint behavior and leave empty FleetManagedKey values treated as user-owned.server/datastore/mysql/policies.go (1)
400-445: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCommit policy changes before deleting memberships.
UpdateFleetManagedPolicyQueriesperforms full membership and statistics cleanup in the retryable transaction.cleanupPolicywith full cleanup enabled deletes all memberships and can hold those locks until commit. This can block host result writes during a large refresh. Commit the query update first, setneeds_full_membership_cleanup, then run cleanup after commit and clear the flag only after successful cleanup, asApplyPolicySpecsdoes at Lines 1964-2009.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/policies.go` around lines 400 - 445, Refactor UpdateFleetManagedPolicyQueries to commit the policy query/checksum update and mark each changed policy with needs_full_membership_cleanup before deleting memberships. Run cleanupPolicy after the transaction commits, and clear the flag only after each cleanup succeeds, following the ApplyPolicySpecs pattern; preserve retry behavior and return the updated policy IDs while ensuring failed cleanup leaves the flag set for retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/datastore/mysql/policies.go`:
- Around line 1793-1794: The policy ownership update in ApplyPolicySpecs must
distinguish an omitted fleet-managed-key field from an explicitly empty value.
Preserve the existing database value only when the field is absent, but write
NULL when the caller explicitly clears it; update the input/presence handling
around the policy spec conversion and the fleet_managed_key SQL assignment
(including the corresponding logic near the referenced later lines).
---
Duplicate comments:
In `@server/datastore/mysql/policies.go`:
- Around line 400-445: Refactor UpdateFleetManagedPolicyQueries to commit the
policy query/checksum update and mark each changed policy with
needs_full_membership_cleanup before deleting memberships. Run cleanupPolicy
after the transaction commits, and clear the flag only after each cleanup
succeeds, following the ApplyPolicySpecs pattern; preserve retry behavior and
return the updated policy IDs while ensuring failed cleanup leaves the flag set
for retry.
In `@server/fleet/policies.go`:
- Around line 668-672: Validate PolicySpec.FleetManagedKey in ApplyPolicySpecs
before inserting or updating policies: accept only supported Fleet-managed keys
and require each key’s compatible policy scope, rejecting unknown keys or
mismatched scopes before persistence. Preserve the existing duplicate-key
database constraint behavior and leave empty FleetManagedKey values treated as
user-owned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a5af2b66-62c7-488d-842d-dd1f00f084b8
📒 Files selected for processing (7)
server/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies.goserver/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/schema.sqlserver/fleet/policies.goserver/mdm/apple/gdmf/macos_versions.goserver/mdm/apple/gdmf/macos_versions_test.go
💤 Files with no reviewable changes (1)
- server/mdm/apple/gdmf/macos_versions_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies_test.go
- server/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies.go
|
@cacaosteve Thanks for the contribution. I am moving this to draft. There are a number of code review comments that need to be addressed. This PR is also modifying a number of files that it shouldn't and are unrelated to the feature request. For example, editing scripts that we use for managing our internal instance of Fleet. Please move this back to ready for review after the code review comments have been addressed, the scope of code changes has been addressed, and manual QA has been completed. Thanks! |
|
@allenhouchins Thanks for the feedback — keeping this in draft. Addressed so far:
Still open before ready-for-review:
Will move out of draft only after those are done. |
|
Follow-up on latest review items (still draft):
Still before ready-for-review: redeploy current HEAD and record manual QA (including the grace-period pass/fail scenario). |
520c949 to
54e6865
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/mdm/apple/gdmf/api_test.go (1)
302-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoosen the wall-clock bound to avoid CI flakes.
retryBackoffis 1 second, so any bound below that proves cancellation interrupted the wait. A 250 ms bound on a loaded CI runner can fail even when the code is correct. Raise the bound while keeping it well under the backoff interval.♻️ Proposed adjustment
select { case err := <-result: require.ErrorIs(t, err, context.Canceled) - require.Less(t, time.Since(canceledAt), 250*time.Millisecond) + require.Less(t, time.Since(canceledAt), 750*time.Millisecond) require.Equal(t, 1, attempts)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mdm/apple/gdmf/api_test.go` around lines 302 - 309, Increase the time.Since(canceledAt) threshold in the cancellation test while keeping it below the 1-second retryBackoff interval, so the test still verifies prompt interruption without relying on an overly tight CI-sensitive bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/mdm/apple/gdmf/api_test.go`:
- Around line 302-309: Increase the time.Since(canceledAt) threshold in the
cancellation test while keeping it below the 1-second retryBackoff interval, so
the test still verifies prompt interruption without relying on an overly tight
CI-sensitive bound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dff5fc06-533c-4f6c-8949-8e5e9cc0042a
⛔ Files ignored due to path filters (1)
docs/Configuration/yaml-files.mdis excluded by!**/*.md
📒 Files selected for processing (32)
changes/3934-macos-up-to-date-gdmf-policiescmd/fleet/cron.gocmd/fleet/cron_registration.gocmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.godocs/01-Using-Fleet/standard-query-library/standard-query-library.ymlserver/datastore/mysql/apple_software_update_assets.goserver/datastore/mysql/apple_software_update_assets_test.goserver/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies.goserver/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/datastore/mysql/schema.sqlserver/fleet/apple_software_update_assets.goserver/fleet/cron_schedules.goserver/fleet/datastore.goserver/fleet/policies.goserver/fleet/policies_test.goserver/mdm/apple/gdmf/api.goserver/mdm/apple/gdmf/api_test.goserver/mdm/apple/gdmf/macos_versions.goserver/mdm/apple/gdmf/macos_versions_test.goserver/mdm/apple/gdmf/sync.goserver/mdm/apple/gdmf/sync_test.goserver/mock/datastore_mock.goserver/service/client.goserver/service/client_test.goserver/service/global_policies.goserver/service/global_policies_test.goserver/service/team_policies.goserver/service/team_policies_test.gotools/gitops-auto-complete/generated-schema.json
🚧 Files skipped from review as they are similar to previous changes (12)
- server/fleet/cron_schedules.go
- changes/3934-macos-up-to-date-gdmf-policies
- server/fleet/apple_software_update_assets.go
- cmd/fleet/cron_registration.go
- cmd/fleet/cron.go
- docs/01-Using-Fleet/standard-query-library/standard-query-library.yml
- server/datastore/mysql/migrations/tables/20260801062925_AddFleetManagedKeyToPolicies.go
- server/datastore/mysql/schema.sql
- server/mdm/apple/gdmf/macos_versions_test.go
- server/mock/datastore_mock.go
- server/mdm/apple/gdmf/sync.go
- server/mdm/apple/gdmf/macos_versions.go
35891b1 to
641b77f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
641b77f to
4e556f3
Compare
2ed5ce3 to
ea5e004
Compare
ea5e004 to
4eae1f2
Compare
4eae1f2 to
ac82cf8
Compare
Introduce fleet_managed_key policies synced from Apple GDMF, with GitOps ownership, unique team-key locking, and nilaway-safe apply paths.
880e7d7 to
1f40849
Compare
|
Hi @cacaosteve, we've recently introduced a lot of changes around the GDMF and storing available versions plus software update device ID's in relation to keeping Apple hosts on latests for OS updates as part of #39085 Once that is fully in, I would expect this PR to change drastically. If you still want to keep the community PR, you can re-work it to fit the new approach and re-open it so we can get it prioritised and linked. Thank you for the contribution though, this is great work and would make a lot of IT admins happy. |
|
@MagnusHJensen Thanks — that makes sense, and I'd like to keep the community PR. #3934 is the compliance-reporting side (Fleet-managed policies that stay current without hardcoded version pins), not MDM enforcement. Happy to rework this as a consumer of the shared GDMF / Plan after the AULD backend is in:
I'll leave this in draft and re-open when the rework is ready. Please link #3934 when you prioritize. |
Just a heads up, we're already providing customers with an example of that here (no product changes required): https://github.com/fleetdm/fleet/blob/main/.github/scripts/dogfood-policy-updater-latest-macos.sh |
|
@allenhouchins Thanks — yes, that dogfood updater is a solid GitOps pattern (SOFA → rewrite the version pins → PR). It's a different shape than #3934 though:
Happy to treat the script as the supported workaround for now and keep this PR parked in draft until you all decide whether in-product |
5 participants
Related issue: Resolves #3934
Checklist for submitter
changes/Testing
server/mdm/apple/gdmf, migration test,PolicySpecvalidation, MySQL upsert/unclaim tests, GitOps client managed-key batching)Summary
Fleet keeps macOS OS-currency policies current from Apple's GDMF software update catalog (
https://gdmf.apple.com/v2/pmv), so admins don't hardcode version pins that go stale.fleet_managed_key: macos_os_up_to_date, grace_days = 0): latest ProductVersion per major track (current + previous major)fleet_managed_key: macos_os_acceptable, grace_days = 30): previous point release allowed until the newest release is 30 days oldapple_software_update_assets, and rewrites policy queries only wherefleet_managed_keyis set explicitlyfleet_managed_key(the up-to-date entry is no longer a user-editabletemplate; Fleet owns the query text)Out of scope: dogfood
it-and-security/and internal dogfood updater scripts.Test plan
go test ./server/mdm/apple/gdmf/go test ./server/fleet/+ ApplyPolicySpecs duplicate-key / modify-platform service testsUpdateFleetManagedPolicyQueries, unclaim,ReplaceAppleSoftwareUpdateAssetsupsert/prune, unique team-key lockinggo build ./cmd/fleetNotes for reviewers
main; design notes stay local/uncommitted.it-and-security/or dogfood updater script changes.