Added support of $FLEET_VAR_HOST_UUID in Windows MDM configuration profiles - #31695
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #31695 +/- ##
=======================================
Coverage 63.77% 63.77%
=======================================
Files 1963 1964 +1
Lines 192101 192175 +74
Branches 6327 6289 -38
=======================================
+ Hits 122505 122566 +61
- Misses 60027 60031 +4
- Partials 9569 9578 +9
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change introduces support for the Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as IT Admin
participant Fleet as Fleet Server
participant Host as Windows Host
Admin->>Fleet: Uploads Windows MDM profile with $FLEET_VAR_HOST_UUID
Fleet->>Fleet: Validate profile variables (only allow HOST_UUID)
alt Valid
Fleet->>Host: Delivers profile (with $FLEET_VAR_HOST_UUID)
Host->>Fleet: Requests profile
Fleet->>Fleet: Substitute $FLEET_VAR_HOST_UUID with host UUID
Fleet->>Host: Sends customized profile
else Invalid variable
Fleet-->>Admin: Rejects profile upload with error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
server/service/microsoft_mdm.go (1)
2232-2235: Consider simplifying the XML escaping approachThe current buffer allocation and escaping can be simplified. Since
xml.EscapeTextmay need to grow the buffer anyway when special characters are present, the initial capacity doesn't provide much benefit.- // Use XML escaping for the replacement value to be safe and prevent XML injection - b := make([]byte, 0, len(hostUUID)) - buf := bytes.NewBuffer(b) - _ = xml.EscapeText(buf, []byte(hostUUID)) - escapedUUID := buf.String() + // Use XML escaping for the replacement value to be safe and prevent XML injection + var buf bytes.Buffer + _ = xml.EscapeText(&buf, []byte(hostUUID)) + escapedUUID := buf.String()server/service/integration_mdm_profiles_test.go (2)
7010-7016: Consider adding cleanup for the created team.While the test suite may handle cleanup automatically, it's good practice to ensure the team is cleaned up after the test completes to avoid potential test pollution.
Add a cleanup defer statement after team creation:
func (s *integrationMDMTestSuite) TestWindowsProfilesWithFleetVariables() { t := s.T() ctx := t.Context() // Create a team for team-scoped tests tm, err := s.ds.NewTeam(ctx, &fleet.Team{Name: "test_windows_fleet_vars"}) require.NoError(t, err) + t.Cleanup(func() { + _ = s.ds.DeleteTeam(ctx, tm.ID) + })
7167-7173: Add cleanup for the created team.Similar to the previous test, consider adding cleanup for the team to prevent test pollution.
func (s *integrationMDMTestSuite) TestWindowsProfilesFleetVariableSubstitution() { t := s.T() ctx := context.Background() // Create a team tm, err := s.ds.NewTeam(ctx, &fleet.Team{Name: t.Name() + "team"}) require.NoError(t, err) + t.Cleanup(func() { + _ = s.ds.DeleteTeam(ctx, tm.ID) + })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
changes/30879-host-uuid-for-windows-profiles(1 hunks)server/fleet/mdm.go(1 hunks)server/service/apple_mdm.go(2 hunks)server/service/integration_mdm_profiles_test.go(1 hunks)server/service/mdm.go(1 hunks)server/service/mdm_test.go(1 hunks)server/service/microsoft_mdm.go(4 hunks)server/service/microsoft_mdm_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit Configuration File
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/service/microsoft_mdm.goserver/service/mdm.goserver/fleet/mdm.goserver/service/microsoft_mdm_test.goserver/service/apple_mdm.goserver/service/mdm_test.goserver/service/integration_mdm_profiles_test.go
🧠 Learnings (2)
📚 Learning: in the host_identity_scep_certificates table schema, the varbinary(100) size for public_key_raw, the...
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.748Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
Applied to files:
server/fleet/mdm.go
📚 Learning: in ee/server/service/hostidentity/depot/depot.go, the scep depot interface methods like put() do not...
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Applied to files:
server/service/microsoft_mdm_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: publish
- GitHub Check: test-packaging (ubuntu-latest, remote)
- GitHub Check: test-packaging (ubuntu-latest, local)
- GitHub Check: build-binaries
- GitHub Check: test-preview (ubuntu-latest)
- GitHub Check: test-db-changes
- GitHub Check: Analyze (javascript)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: Analyze (go)
- GitHub Check: lint (windows-latest)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: lint (macos-latest)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: build-and-check
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fast, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: check-doc-gen
🔇 Additional comments (10)
changes/30879-host-uuid-for-windows-profiles (1)
1-1: LGTM! Clear and accurate changelog entry.The changelog entry concisely describes the new feature and correctly references the
$FLEET_VAR_HOST_UUIDvariable format that users will interact with.server/service/apple_mdm.go (1)
82-86: Rename looks good – no further actionLocal variable was renamed for clarity and the only in-file reference was updated. No stale occurrences detected.
Also applies to: 494-495
server/service/mdm_test.go (1)
2251-2360: LGTM! Comprehensive test coverage for Windows profile Fleet variable validation.This test function provides excellent coverage for the
validateWindowsProfileFleetVariablesfunction with well-structured test cases covering:
- Valid scenarios: no variables, supported
$FLEET_VAR_HOST_UUIDvariable (with and without braces), multiple occurrences- Invalid scenarios: unsupported variables, mixed supported/unsupported variables, unknown variables
- Proper error message validation to ensure specific unsupported variables are identified
The test implementation follows Go testing best practices and aligns perfectly with the PR objective to add
$FLEET_VAR_HOST_UUIDsupport for Windows MDM profiles.server/service/mdm.go (5)
1519-1523: Well-structured variable declaration for supported Fleet variables.The explicit allowlist approach using a slice is clean and makes it easy to extend support for additional variables in the future. The variable name clearly indicates its purpose and scope.
1525-1538: Improved validation logic with better error handling.The refactored validation function is a significant improvement:
- Early return when no variables are found avoids unnecessary processing
- Iterates through found variables to check against the allowlist
- Provides specific error messages indicating which variable is unsupported
- Uses
slices.Containsfor clean membership checkingThe logic correctly handles the case where multiple unsupported variables exist by returning on the first unsupported one found, which is appropriate for validation scenarios.
1534-1534: Excellent error message formatting.The error message is clear, specific, and actionable. It follows the established pattern of Fleet error messages and includes the exact variable name that caused the validation failure.
1522-1522: Confirmedfleet.FleetVarHostUUIDdefinitionThe constant is defined in
server/fleet/mdm.go:// server/fleet/mdm.go const ( FleetVarHostUUID = "HOST_UUID" // … )No changes are required—this reference is valid and accessible.
1526-1526:findFleetVariablesis already defined in this packageThe call in
server/service/mdm.gois valid—findFleetVariables(contents)is implemented inserver/service/apple_mdm.go:5867(and no import is necessary for unexported functions in the same package). You can safely remove the import-check suggestion.Likely an incorrect or invalid review comment.
server/service/integration_mdm_profiles_test.go (2)
7018-7140: Excellent test coverage for Fleet variable validation!The test cases provide comprehensive coverage of supported and unsupported Fleet variables, including edge cases like variables with/without braces, mixed variable scenarios, and proper error message validation.
7208-7273: Well-structured end-to-end test with thorough verification!The test effectively validates:
- UUID substitution in profile content for both global and team hosts
- Proper removal of Fleet variable patterns (both
$FLEET_VAR_HOST_UUIDand${FLEET_VAR_HOST_UUID})- Correct profile status updates in the database
- Proper MDM session handling with status responses
The helper function
verifyProfileSubstitutionis particularly well-designed for reusability across different host scenarios.
…d-for-windows-profiles
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (27)
changes/30879-host-uuid-for-windows-profiles (1)
1-2: Clarify scope in changelog (Windows-only, Premium).Recommend explicitly stating platform and edition to reduce ambiguity in release notes.
-Added support of $FLEET_VAR_HOST_UUID in Windows MDM configuration profiles. +Added support for $FLEET_VAR_HOST_UUID in Windows MDM configuration profiles. +Note: Windows-only. Available in Fleet Premium.server/service/apple_mdm.go (1)
494-549: Optional: use a precomputed set for O(1) lookups instead of slices.ContainsNot required, but you can avoid repeated linear scans by computing a set once and checking membership in O(1). Example diffs:
- Build a set alongside the slice:
var ( @@ - fleetVarsSupportedInAppleConfigProfiles = []string{ + fleetVarsSupportedInAppleConfigProfiles = []string{ fleet.FleetVarNDESSCEPChallenge, fleet.FleetVarNDESSCEPProxyURL, fleet.FleetVarHostEndUserEmailIDP, fleet.FleetVarHostHardwareSerial, fleet.FleetVarHostEndUserIDPUsername, fleet.FleetVarHostEndUserIDPUsernameLocalPart, fleet.FleetVarHostEndUserIDPGroups, fleet.FleetVarHostEndUserIDPDepartment, fleet.FleetVarSCEPRenewalID, } + // Precomputed set for O(1) membership checks in validation paths. + fleetVarsSupportedInAppleConfigProfilesSet = func() map[string]struct{} { + m := make(map[string]struct{}, len(fleetVarsSupportedInAppleConfigProfiles)) + for _, v := range fleetVarsSupportedInAppleConfigProfiles { + m[v] = struct{}{} + } + return m + }() )
- Use the set in validation:
- if !slices.Contains(fleetVarsSupportedInAppleConfigProfiles, k) { + if _, ok := fleetVarsSupportedInAppleConfigProfilesSet[k]; !ok { found := false switch {server/datastore/mysql/mdm_test.go (3)
675-676: Signature update LGTM; consider explicit empty slice over nilPassing nil works for []string, but using []string{} makes “no Fleet vars used” explicit and avoids surprises if downstream code distinguishes nil vs empty (e.g., JSON/null vs [] or conditional len checks).
- nil, + []string{},(Apply to both occurrences in this hunk.)
Also applies to: 681-682
766-767: Confirm nil implies “no variables recorded” semantics (and consider asserting it)If nil is intended to mean “no Fleet vars used,” ensure the datastore treats it identically to an empty slice (i.e., no rows inserted into mdm_configuration_profile_variables). If relevant to this test’s scope, consider asserting that zero vars are recorded for the created profile.
I can draft a focused assertion or a small companion test that checks the variables table is empty when usesFleetVars is nil.
6473-6474: Consistent use of nil for usesFleetVars; ensure no unintended DB artifactsFor top-level profiles, passing nil should not create any rows in mdm_configuration_profile_variables. If the implementation differentiates nil vs empty, prefer []string{} for clarity.
- nil, + []string{},server/fleet/datastore.go (1)
1729-1729: Document usesFleetVars semantics and nil vs empty behaviorPlease add a short comment explaining allowed values, whether order matters, and how nil vs empty should be treated (both meaning “no variables” is typical). This avoids ambiguity for implementers and tests.
For example (non-diff snippet):
// NewMDMWindowsConfigProfile creates and returns a new configuration profile. // usesFleetVars is the set of Fleet variable names referenced by the profile payload. // Valid values are constrained/validated upstream; both nil and empty slice mean "no variables". NewMDMWindowsConfigProfile(ctx context.Context, cp MDMWindowsConfigProfile, usesFleetVars []string) (*MDMWindowsConfigProfile, error)server/datastore/mysql/apple_mdm_test.go (1)
146-146: Team-scoped call updated correctly; consider adding non-nil variable coverage.This change aligns with the new signature. As a follow-up, add or confirm a Windows-specific test that:
- Creates a profile with variables containing $FLEET_VAR_HOST_UUID.
- Verifies mdm_configuration_profile_variables is populated accordingly.
- Asserts substitution occurs (and that updates to the host UUID trigger resend as planned).
I can draft a subtest skeleton if helpful.
server/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.go (3)
16-21: Simplify binding and drop sqlx dependency (optional)You only bind a single value. Switching to positional binds removes the sqlx dependency and reduces complexity.
- insStmt := ` - INSERT INTO fleet_variables ( - name, is_prefix, created_at - ) VALUES - ('FLEET_VAR_HOST_UUID', 0, :created_at) - ` - // use a constant time so that the generated schema is deterministic - createdAt := time.Date(2025, 8, 8, 0, 0, 0, 0, time.UTC) - stmt, args, err := sqlx.Named(insStmt, map[string]any{"created_at": createdAt}) - if err != nil { - return fmt.Errorf("Failed to prepare insert for FLEET_VAR_HOST_UUID: %w", err) - } - _, err = tx.Exec(stmt, args...) + insStmt := ` + INSERT INTO fleet_variables ( + name, is_prefix, created_at + ) VALUES + ('FLEET_VAR_HOST_UUID', 0, ?) + ` + // use a constant time so that the generated schema is deterministic + createdAt := time.Date(2025, 8, 8, 0, 0, 0, 0, time.UTC) + _, err := tx.Exec(insStmt, createdAt) if err != nil { - return fmt.Errorf("Failed to insert FLEET_VAR_HOST_UUID into fleet_variables: %w", err) + return fmt.Errorf("failed to insert FLEET_VAR_HOST_UUID into fleet_variables: %w", err) }If you apply this, also remove the sqlx import.
Also applies to: 24-31
3-9: Remove unused dependency if you adopt positional bindsIf you take the optional simplification, drop the sqlx import:
import ( "database/sql" "fmt" "time" - - "github.com/jmoiron/sqlx" )
26-30: Nit: error message casingGo errors should start lowercase. Also, after simplifying binds, only one error path remains.
- return fmt.Errorf("Failed to prepare insert for FLEET_VAR_HOST_UUID: %w", err) + return fmt.Errorf("failed to prepare insert for FLEET_VAR_HOST_UUID: %w", err)- return fmt.Errorf("Failed to insert FLEET_VAR_HOST_UUID into fleet_variables: %w", err) + return fmt.Errorf("failed to insert FLEET_VAR_HOST_UUID into fleet_variables: %w", err)server/mock/datastore_mock.go (1)
7439-7444: Add nil-guard and copy function under lock to avoid race/panic in tests.Current code dereferences
s.NewMDMWindowsConfigProfileFuncwithout checking for nil and reads it outside the lock. Suggest capturing it under the mutex and panicking with a clear message if unset.func (s *DataStore) NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error) { - s.mu.Lock() - s.NewMDMWindowsConfigProfileFuncInvoked = true - s.mu.Unlock() - return s.NewMDMWindowsConfigProfileFunc(ctx, cp, usesFleetVars) + s.mu.Lock() + s.NewMDMWindowsConfigProfileFuncInvoked = true + fn := s.NewMDMWindowsConfigProfileFunc + s.mu.Unlock() + if fn == nil { + panic("datastore mock: NewMDMWindowsConfigProfileFunc not set") + } + return fn(ctx, cp, usesFleetVars) }server/datastore/mysql/mdm.go (1)
328-330: Comment is accurate; clarify cross-platform variable keying to avoid confusion.Since Apple uses identifier and Windows uses name, consider adding a brief doc comment (in the type that carries these variables) clarifying what “identifier” represents per platform to prevent misuse.
server/datastore/mysql/microsoft_mdm_test.go (2)
2242-2246: Consider adding a variables-bearing profile in this labels testThese calls are fine. To guard against regressions, add one Windows profile here with a non-nil variables slice to ensure:
- variable associations are persisted alongside include/exclude label logic, and
- ListMDMWindowsProfilesToInstall behavior remains unaffected by mere presence of variables.
If helpful, I can sketch an assertion similar to checkProfileVariables used later in this file to confirm associations for Windows.
2423-2431: Exercise batchSetMDMWindowsProfilesDB with variables to validate Windows associationsAdd a subtest in
server/datastore/mysql/microsoft_mdm_test.go(after the existingapplyAndExpectblock) that:
- Calls
ds.batchSetMDMWindowsProfilesDBwith a non-nil variables slice (e.g. onefleet.MDMProfileUUIDFleetVariablesentry).- Queries
mdm_configuration_profile_variablesjoined withmdm_windows_configuration_profilesandfleet_variablesto assert the expectedwindows_profile_uuid → fleet_variable_idmapping.Example sketch:
t.Run("batch set persists Windows profile variables", func(t *testing.T) { tmID := ptr.Uint(1) newSet := []*fleet.MDMWindowsConfigProfile{ windowsConfigProfileForTest(t, "N-vars", "loc") } err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { updated, err := ds.batchSetMDMWindowsProfilesDB(ctx, tx, tmID, newSet, []fleet.MDMProfileUUIDFleetVariables{ { ProfileUUID: newSet[0].ProfileUUID, FleetVariables: []string{fleet.FleetVarHostUUID}, }, }) require.NoError(t, err) require.True(t, updated) return nil }) require.NoError(t, err) var names []string require.NoError(t, sqlx.SelectContext(ctx, ds.DB(), &names, ` SELECT fv.name FROM mdm_windows_configuration_profiles mwcp JOIN mdm_configuration_profile_variables mcpv ON mwcp.profile_uuid = mcpv.windows_profile_uuid JOIN fleet_variables fv ON mcpv.fleet_variable_id = fv.id WHERE mwcp.name = ? AND mwcp.team_id = ?`, "N-vars", *tmID)) require.Contains(t, names, string(fleet.FleetVarHostUUID)) })server/service/microsoft_mdm.go (3)
2241-2255: Consider astrings.Replacerfor cleaner, single-pass substitution
strings.ReplaceAllis invoked twice for every variable occurrence, which means two full scans of the string per host.
Building astrings.Replaceronce (e.g.strings.NewReplacer("$FLEET_VAR_HOST_UUID", escapedUUID, "${FLEET_VAR_HOST_UUID}", escapedUUID)) performs all replacements in a single traversal and keeps the intent obvious.Not critical, but worth it if large profiles are processed for thousands of hosts.
2247-2250: Capture the (unlikely)xml.EscapeTexterror
_ = xml.EscapeText(...)discards the error; although failure is rare, it still returnsio.ErrShortWrite/ErrWriteAfterClose.
Either handle it or drop the assignment completely and add a short comment so the reader knows it’s intentionally ignored.
2294-2297: Map key risks with sentinel delimiter
hostProfilesMapuses the keyhostUUID + "|" + profileUUID.
While UUIDs won’t normally contain|, string concatenation with a sentinel is brittle and easy to forget when the key shape changes. Consider a small struct key orfmt.Sprintf("%s:%s", …)to avoid accidental collisions.server/service/microsoft_mdm_test.go (2)
280-284: Brittle expectation on XML-escaped quotesThe test expects
"for a double-quote. If Go’sencoding/xmlever switches to", the test will fail even though the code is correct. Matchingstrings.Contains(result, """) || strings.Contains(result, """)(or simply confirming</>/&are escaped) would make the test less fragile.
528-536:receivedCommandoverwrites on multiple inserts
receivedCommandcaptures only the last call toMDMWindowsInsertCommandForHostsFunc.
If more than one host were processed, earlier (possibly failing) commands would be lost, making the assertion silently pass.
Collect the commands in a slice or assert the call count to keep the test accurate when additional hosts are added.server/service/integration_mdm_profiles_test.go (2)
3555-3558: Avoid “mystery nil” – pass a clearly-named empty map instead
nilis accepted here but gives no hint what the extra parameter represents (profile variables).
Usingmap[string]string{}(or a helper likefleet.NoProfileVars) keeps the call site self-documenting and avoids accidental panics if the callee ever dereferences the map.
7010-7140: Close HTTP response bodies in the table-driven loop
resp.Bodyis read but never closed; on macOS/Linux this leaks an FD per test-case.
Add adefer resp.Body.Close()immediately after each call tos.Do(...).resp := s.Do(...) +defer resp.Body.Close()server/service/mdm_test.go (2)
1208-1214: AssertusesFleetVarspropagation in the mock for stronger guaranteesAdd assertions to verify that when
$FLEET_VAR_HOST_UUIDappears in valid profiles, the service extracts and passes it to the datastore; otherwise, the slice is empty. This tightens the contract for the save flow.Apply this diff:
ds.NewMDMWindowsConfigProfileFunc = func(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error) { - if bytes.Contains(cp.SyncML, []byte("duplicate")) { - return nil, &alreadyExistsError{} - } - cp.ProfileUUID = uuid.New().String() - return &cp, nil + if bytes.Contains(cp.SyncML, []byte("duplicate")) { + return nil, &alreadyExistsError{} + } + + // Verify extracted Fleet vars are propagated correctly. + hasUUIDVar := bytes.Contains(cp.SyncML, []byte("$FLEET_VAR_HOST_UUID")) || + bytes.Contains(cp.SyncML, []byte("${FLEET_VAR_HOST_UUID}")) + if hasUUIDVar { + require.ElementsMatch(t, + []string{string(fleet.FleetVarHostUUID)}, + usesFleetVars, + ) + } else { + require.Len(t, usesFleetVars, 0) + } + + cp.ProfileUUID = uuid.New().String() + return &cp, nil }
2251-2360: Optionally add a couple more cases for robustnessConsider adding:
- Mixed forms in a single value (e.g.,
$FLEET_VAR_HOST_UUID and ${FLEET_VAR_HOST_UUID}).- Multiple items using the allowed var to ensure scanning across the document.
Apply this diff to extend the tests table:
@@ tests := []struct { @@ }{ + { + name: "HOST_UUID mixed forms in same value", + profileXML: `<Replace> + <Item> + <Target><LocURI>./Device/Vendor/MSFT/Policy/Config/System/AllowLocation</LocURI></Target> + <Data>$FLEET_VAR_HOST_UUID--${FLEET_VAR_HOST_UUID}</Data> + </Item> + </Replace>`, + wantErr: false, + }, + { + name: "multiple items with HOST_UUID", + profileXML: `<Replace> + <Item> + <Target><LocURI>./Device/Vendor/MSFT/Policy/Config/System/AllowLocation</LocURI></Target> + <Data>$FLEET_VAR_HOST_UUID</Data> + </Item> + <Item> + <Target><LocURI>./Device/Vendor/MSFT/Policy/Config/System/AllowTelemetry</LocURI></Target> + <Data>${FLEET_VAR_HOST_UUID}</Data> + </Item> + </Replace>`, + wantErr: false, + }, }server/datastore/mysql/microsoft_mdm.go (1)
1766-1847: Persisting Fleet variables on profile creation looks correct; consider dedupe/sort for stabilityThe transactional insert and follow-up association via
batchSetProfileVariableAssociationsDB(..., "windows")is solid. Minor improvement: dedupe and sortusesFleetVarsto avoid duplicate rows and ensure deterministic writes.Apply this diff within the function before building
profilesVarsToUpsert:@@ - // Save Fleet variables associated with this Windows profile + // Save Fleet variables associated with this Windows profile + // (dedupe/sort for deterministic associations) if len(usesFleetVars) > 0 { + if len(usesFleetVars) > 1 { + seen := make(map[string]struct{}, len(usesFleetVars)) + uniq := make([]string, 0, len(usesFleetVars)) + for _, v := range usesFleetVars { + if _, ok := seen[v]; !ok { + seen[v] = struct{}{} + uniq = append(uniq, v) + } + } + // optional: keep order stable for repeatable writes + sort.Strings(uniq) + usesFleetVars = uniq + } profilesVarsToUpsert := []fleet.MDMProfileUUIDFleetVariables{ { ProfileUUID: profileUUID, FleetVariables: usesFleetVars, }, } if err := batchSetProfileVariableAssociationsDB(ctx, tx, profilesVarsToUpsert, "windows"); err != nil { return ctxerr.Wrap(ctx, err, "inserting windows profile variable associations") } }Add the import if you sort:
import "sort"server/service/mdm.go (3)
1485-1492: Make variable collection deterministic (and reuse the already-built set).Order from ranging over a map is random; sort to avoid flakiness and improve reproducibility.
- // Collect Fleet variables used in the profile - foundVars := findFleetVariables(string(cp.SyncML)) - var usesFleetVars []string - for varName := range foundVars { - usesFleetVars = append(usesFleetVars, varName) - } + // Collect Fleet variables used in the profile (deterministic order) + foundVars := findFleetVariables(string(cp.SyncML)) + usesFleetVars := maps.Keys(foundVars) + slices.Sort(usesFleetVars)
1526-1531: Use a set for supported variables to simplify membership checks and future growth.A map[string]struct{} avoids linear scans and simplifies the validator.
-// fleetVarsSupportedInWindowsProfiles lists the Fleet variables that are -// supported in Windows configuration profiles. -var fleetVarsSupportedInWindowsProfiles = []string{ - fleet.FleetVarHostUUID, -} +// fleetVarsSupportedInWindowsProfiles lists the Fleet variables that are +// supported in Windows configuration profiles. +// Use a set for O(1) membership checks. +var fleetVarsSupportedInWindowsProfiles = map[string]struct{}{ + fleet.FleetVarHostUUID: {}, +}
1533-1542: Tighten validation by using set membership; keep error message unchanged.Switch to O(1) membership with the set suggested above. Logic stays the same.
- foundVars := findFleetVariables(contents) - if len(foundVars) == 0 { - return nil - } - - // Check if all found variables are supported - for varName := range foundVars { - if !slices.Contains(fleetVarsSupportedInWindowsProfiles, varName) { - return fleet.NewInvalidArgumentError("profile", fmt.Sprintf("Fleet variable $FLEET_VAR_%s is not supported in Windows profiles.", varName)) - } - } + foundVars := findFleetVariables(contents) + if len(foundVars) == 0 { + return nil + } + // Check if all found variables are supported + for varName := range foundVars { + if _, ok := fleetVarsSupportedInWindowsProfiles[varName]; !ok { + return fleet.NewInvalidArgumentError("profile", fmt.Sprintf("Fleet variable $FLEET_VAR_%s is not supported in Windows profiles.", varName)) + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
changes/30879-host-uuid-for-windows-profiles(1 hunks)server/datastore/mysql/apple_mdm_test.go(3 hunks)server/datastore/mysql/hosts_test.go(1 hunks)server/datastore/mysql/mdm.go(1 hunks)server/datastore/mysql/mdm_test.go(6 hunks)server/datastore/mysql/microsoft_mdm.go(5 hunks)server/datastore/mysql/microsoft_mdm_test.go(9 hunks)server/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.go(1 hunks)server/datastore/mysql/schema.sql(2 hunks)server/datastore/mysql/teams_test.go(1 hunks)server/fleet/datastore.go(1 hunks)server/fleet/mdm.go(1 hunks)server/mock/datastore_mock.go(2 hunks)server/service/apple_mdm.go(2 hunks)server/service/integration_mdm_profiles_test.go(4 hunks)server/service/mdm.go(3 hunks)server/service/mdm_test.go(3 hunks)server/service/microsoft_mdm.go(4 hunks)server/service/microsoft_mdm_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit Configuration File
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/fleet/mdm.goserver/datastore/mysql/teams_test.goserver/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/datastore/mysql/mdm.goserver/datastore/mysql/apple_mdm_test.goserver/service/microsoft_mdm_test.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/hosts_test.goserver/service/mdm.goserver/datastore/mysql/microsoft_mdm.goserver/service/microsoft_mdm.goserver/service/mdm_test.goserver/service/apple_mdm.goserver/service/integration_mdm_profiles_test.go
🧠 Learnings (9)
📚 Learning: 2025-07-08T16:13:39.114Z
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go:53-55
Timestamp: 2025-07-08T16:13:39.114Z
Learning: In the Fleet codebase, Down migration functions are intentionally left empty/no-op. The team does not implement rollback functionality for database migrations, so empty Down_* functions in migration files are correct and should not be flagged as issues.
Applied to files:
server/fleet/mdm.goserver/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.goserver/datastore/mysql/schema.sql
📚 Learning: 2025-07-07T22:21:15.748Z
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.748Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
Applied to files:
server/fleet/mdm.goserver/datastore/mysql/schema.sql
📚 Learning: 2025-08-08T07:40:05.274Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.274Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/datastore/mysql/teams_test.goserver/mock/datastore_mock.goserver/datastore/mysql/apple_mdm_test.goserver/service/microsoft_mdm_test.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/hosts_test.goserver/service/mdm_test.go
📚 Learning: 2025-08-08T07:40:05.274Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.274Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/datastore/mysql/teams_test.goserver/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.goserver/mock/datastore_mock.goserver/service/microsoft_mdm_test.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm_test.goserver/service/mdm_test.go
📚 Learning: 2025-07-08T16:11:49.555Z
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:115-115
Timestamp: 2025-07-08T16:11:49.555Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the error from result.RowsAffected() is intentionally ignored because the information is only used for logging purposes, not for critical program logic.
Applied to files:
server/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.goserver/service/microsoft_mdm.go
📚 Learning: 2025-08-01T15:08:16.858Z
Learnt from: sgress454
PR: fleetdm/fleet#31508
File: server/datastore/mysql/schema.sql:102-116
Timestamp: 2025-08-01T15:08:16.858Z
Learning: 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.
Applied to files:
server/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.goserver/datastore/mysql/schema.sql
📚 Learning: 2025-08-08T07:40:05.274Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.274Z
Learning: In fleetdm/fleet, tests may validly use testing.T.Context() when the module/toolchain targets Go 1.24+. Do not flag t.Context() usage in this codebase if go.mod/toolchain indicates Go >= 1.24.
Applied to files:
server/mock/datastore_mock.goserver/service/microsoft_mdm_test.goserver/datastore/mysql/microsoft_mdm_test.goserver/service/mdm_test.go
📚 Learning: 2025-07-08T16:12:48.797Z
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Applied to files:
server/datastore/mysql/mdm.goserver/service/microsoft_mdm_test.go
📚 Learning: 2025-07-08T16:06:54.576Z
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Applied to files:
server/service/microsoft_mdm.go
🔇 Additional comments (26)
server/fleet/mdm.go (1)
44-44: LGTM—HOST_UUID migration and Windows wiring confirmedConfirmed that:
- The migration
server/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.goinsertsFLEET_VAR_HOST_UUIDintofleet_variables.fleetVarsSupportedInWindowsProfilesinserver/service/mdm.goincludesfleet.FleetVarHostUUID, and the substitution logic inserver/service/microsoft_mdm.gocorrectly replaces$FLEET_VAR_HOST_UUID.- The
NewMDMWindowsConfigProfilemethod inserver/service/mdm.goaccepts and passes through the detected fleet variables.No further changes required.
server/datastore/mysql/teams_test.go (1)
93-98: LGTM: test updated to new signature.Adding the extra nil argument matches the updated NewMDMWindowsConfigProfile signature that accepts Fleet variables.
server/datastore/mysql/hosts_test.go (1)
3834-3834: Signature update handled correctlyPassing nil as the third arg to NewMDMWindowsConfigProfile is appropriate to indicate no Fleet variables used. LGTM.
server/service/apple_mdm.go (2)
82-86: Rename clarifies Apple-only scope — LGTMRenaming to fleetVarsSupportedInAppleConfigProfiles improves intent and avoids conflating with Windows profile handling. No behavior change.
82-86: All references updated tofleetVarsSupportedInAppleConfigProfiles
I ran a search for both the old and new symbols and found only the newfleetVarsSupportedInAppleConfigProfilesin:
- server/service/apple_mdm.go:82–86 (declaration)
- server/service/apple_mdm.go:494 (usage check)
No stale references to
fleetVarsSupportedInConfigProfilesremain.server/datastore/mysql/mdm_test.go (3)
780-781: LGTM; consistent with prior callsSame note as above: nil should be handled as an empty set of vars by the datastore layer. No changes requested here.
6483-6484: LGTMNo issues; matches the updated function signature.
703-704: All NewMDMWindowsConfigProfile call sites updated with usesFleetVars argument
No 2-arg invocations remain; every occurrence includes the new parameter.server/fleet/datastore.go (1)
1729-1729: Signature change aligns Windows with Apple — good consistencyAdding usesFleetVars []string to NewMDMWindowsConfigProfile matches the Apple counterpart and enables persisting used Fleet variables for Windows profiles.
server/datastore/mysql/apple_mdm_test.go (1)
168-174: Duplicate-name negative cases unaffected by new arg—good.The new nil arg shouldn't affect name uniqueness logic. Ensure the uniqueness constraints remain platform/team-scoped as intended (Apple vs Windows, team vs no-team).
If not already covered elsewhere, add an assertion that the variables association count is zero for these nil-variable creations to guard against regressions.
server/datastore/mysql/schema.sql (2)
1442-1444: Migration status bump to 409 with version_id 20250808000000 — OK; confirm version matches the migration filename
- Entry for 20250808000000 with is_applied=1 aligns with the described change.
- Please verify the migration filename and version_id are consistent with this row (the script in the previous comment covers this).
323-325: Migration and wiring for FLEET_VAR_HOST_UUID validated — schema.sql is up-to-date
The new migration (20250808000000_AddHostUUIDFleetVariable.go) insertsFLEET_VAR_HOST_UUIDwith the deterministic timestamp,schema.sqlreflects the AUTO_INCREMENT bump and new entry, and thefleet.FleetVarHostUUIDconstant plus replacement logic inserver/service(with comprehensive tests) are all in place.server/datastore/mysql/migrations/tables/20250808000000_AddHostUUIDFleetVariable.go (3)
22-24: Deterministic schema timestamp — goodUsing a fixed UTC timestamp keeps schema.sql generation stable. LGTM.
36-38: No-op Down migration — correct per Fleet practiceLeaving Down_* empty matches our established migrations approach. No changes needed.
16-21: Approve current INSERT for consistencyExisting migrations that seed
fleet_variables(e.g. AddSCEPRenewalIdFleetVar, SCIMAddDepartment) all use a plainINSERTand rely on the one-time application of migrations. IntroducingINSERT IGNOREorON DUPLICATE KEY UPDATEhere would diverge from those patterns. No change required.server/datastore/mysql/mdm.go (1)
323-326: batchSetMDMWindowsProfilesDB handles variable associations correctlyI’ve confirmed that:
- Empty or nil
profilesVariablesByIdentifieris a no-op (len == 0returns early).- The platform switch only allows
"windows"(non-Windows inputs error out).- Stale associations are deleted via the
DELETE FROM mdm_configuration_profile_variables WHERE windows_profile_uuid IN (…)step.- New variables are upserted using
INSERT … ON DUPLICATE KEY UPDATE.- All call sites for the updated signature are in sync (including tests).
server/datastore/mysql/microsoft_mdm_test.go (6)
1966-1969: LGTM: Windows team-scoped profile call updatedThe extra nil arg matches the updated signature and maintains previous behavior for this test.
1979-1994: LGTM: Duplicate-name constraints still exercised with updated signatureThe nil variables arg preserves intent while aligning to the new function shape.
2004-2012: LGTM: Labels FK check remains valid with the new argumentThe added nil variables arg does not alter the foreign key validation behavior being tested.
2024-2033: LGTM: Labels happy-path case remains validRetains the original assertions; the trailing nil variables arg is correct for the updated signature.
2254-2259: LGTMNo behavior change beyond adapting to the new parameter.
2266-2270: LGTMSignature alignment only; test intent preserved.
server/service/integration_mdm_profiles_test.go (1)
7255-7261: Double-check expectation: team host may also receive the global profileThe test asserts that the team host only sees the team-scoped profile. In Fleet today, global Windows profiles are delivered to all hosts (team or not).
If that behaviour hasn’t changed, this assertion could pass by chance (first profile inspected) and mask an unintended duplicate delivery.Please verify the intended semantics and, if global-plus-team delivery is correct, extend the test to assert the presence of both profiles rather than exactly one.
server/service/mdm_test.go (2)
1130-1132: Mock signature update to capture used Fleet variables: LGTMThe updated mock (
usesFleetVars []string) aligns with the datastore interface change and is appropriate for this authz-focused test.
2251-2360: Windows Fleet variable validation tests: solid coverageCovers allowed
$FLEET_VAR_HOST_UUID(with/without braces, multiple) and rejects unsupported/unknown vars with clear messages. Nice.server/datastore/mysql/microsoft_mdm.go (1)
1916-1917: batchSetMDMWindowsProfilesDB invocations updatedAll call sites now include the new
profilesVariablesByIdentifierparameter:
- server/datastore/mysql/mdm.go:324 – service layer passes
profilesVariablesByIdentifier- server/datastore/mysql/microsoft_mdm_test.go:2427 – test stub now passes
nilfor the new sliceNo further changes required.
|
What about verification? Have we thought about how Verification on Windows functions in the presence of Fleet vars? For this test for instance: https://github.com/fleetdm/fleet/pull/31695/files#diff-fb609ca1ba21d8c37cd6a5fae0e36315c271b263053730f76c0c6620c740ef43R2085 I think we'd have to substitute in the variables during generation of the queries to verify profiles on the device since we have to generate Get SyncML commands for all LocURIs in a given profile during verification. Likewise some profile types do a byte or string compare of the contents to the expected portion(basically what we think we sent) On macOS this is not a problem since we do a timestamp based verification but Windows is different and that's not possible AFAIK. |
You're right. I missed it since I did not manually test it. Verification is a big enough task that it would go to a new PR that I will work on next week. Rough plan is:
|
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
It's always tough to fully review a PR this big but this looks good to me.
On the subject of verification and validation it may be worth considering limiting where in the profile Fleet Vars can occur to only the data sections. That would likely make validation easier and lessen the likelihood of users being able to do things that might cause weird problems like putting variables into the LocURI and probably doesn't limit what users can do with this very much.
…d-for-windows-profiles # Conflicts: # server/datastore/mysql/schema.sql
…ofiles (#31695) Fixes #30879 Demo video: https://www.youtube.com/watch?v=jVyh5x8EMnc I added a `FleetVarName` type, which should improve safety/maintainability, but that resulted in a lot of files touched. I also added the following. However, these are not strictly needed for this feature (only useful for debug right now). But we are following the pattern created by MDM team. 1. Add the migration to insert HOST_UUID into fleet_variables 2. Update the Windows profile save logic to populate mdm_configuration_profile_variables # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation] - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit * **New Features** * Added support for the `$FLEET_VAR_HOST_UUID` variable in Windows MDM configuration profiles, enabling per-host customization during profile deployment. * Enhanced profile delivery by substituting Fleet variables with actual host data in Windows profiles. * Introduced a database migration to register the new Fleet variable for host UUID. * **Bug Fixes** * Improved validation and error handling to reject unsupported Fleet variables in Windows MDM profiles with detailed messages. * Ensured robust handling of errors during profile command insertion without aborting the entire reconciliation process. * **Tests** * Added extensive tests covering validation, substitution, error handling, and reconciliation workflows for Windows MDM profiles using Fleet variables. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Fixes #30879
Demo video: https://www.youtube.com/watch?v=jVyh5x8EMnc
I added a
FleetVarNametype, which should improve safety/maintainability, but that resulted in a lot of files touched.I also added the following. However, these are not strictly needed for this feature (only useful for debug right now). But we are following the pattern created by MDM team.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Summary by CodeRabbit
New Features
$FLEET_VAR_HOST_UUIDvariable in Windows MDM configuration profiles, enabling per-host customization during profile deployment.Bug Fixes
Tests