Clean up Windows profile preprocess parameter passing in advance of adding additional profile variables - #35804
Clean up Windows profile preprocess parameter passing in advance of adding additional profile variables#35804iansltx wants to merge 9 commits into
Conversation
Built initial versions of the structs first, then unleashed Zed + Sonnet 4.5 with the following prompts: - Use the new preprocessDependencies and preprocessParams structs to clean up parameters for preprocessWindowsProfileContents. - Move appConfig into preprocessDependencies - Extract profileContents back out of preprocessParams into a dedicated parameter Then did deterministic refactors of params order on the function signature and hostCmdUUID -> hostUUID since we're talking about hostUUID here.
More Zed + Sonnet 4.5 prompting: - Extract the logic of L327-L331 of profile_verifier.go into a receiver for *fleet.GroupedCertificateAuthorities, then use that in the caller so PreprocessWindowsProfileContentsForDeployment is passed a map[string]*fleet.CustomSCEPProxyCA instead of a *fleet.GroupedCertificateAuthorities - Use PreprocessDependencies and PreprocessParams structs in the params for PreprocessWindowsProfileContentsForVerification.
TODO: split structs so it's clearer what we're using when, at which point we can type-match against which struct we're handling
… confirm verification based on type Next up: 1. Inlining struct creation at call sites 2. Doing the same thing for dependencies
… on how it's called
… assertion to prove it)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR refactors Windows profile preprocessing from direct parameter passing to a dependency injection pattern. It introduces interfaces and structs for dependencies (context, logger, datastore, host cache, app config, custom SCEP CAs) and parameters (host UUID, profile UUID, managed certificates). A utility method converts CustomSCEPProxyCA slices into maps for efficient lookup. Changes
Sequence DiagramsequenceDiagram
participant Old as Previous Approach
participant Service as Service Layer
participant New as New Approach
participant Verifier as Profile Verifier
Note over Old: Direct Parameter Passing
Old->>Service: ctx, logger, ds, appConfig, hostUUID,<br/>profUUID, groupedCAs, content...
Service->>Verifier: Preprocess(many params)
Note over New: Dependency Injection Pattern
Service->>Service: Build ProfilePreprocessDeps<br/>(ctx, logger, ds, appConfig, CAs)
Service->>Service: Build ProfilePreprocessParams<br/>(hostUUID, profUUID, certs)
Service->>Verifier: Preprocess(deps, params, content)
Verifier->>Verifier: deps.GetContext()<br/>deps.GetLogger()<br/>deps.GetDS()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly Related PRs
Suggested Reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/mdm/microsoft/profile_verifier.go (1)
423-439: Fix IDP replacement error handling and clarify deploy-only castingIn
preprocessWindowsProfileContents, two things are worth adjusting:
- IDP branch: returning a nil error on failed replacement
replacedContents, replacedVariable, err := profiles.ReplaceHostEndUserIDPVariables(...) if err != nil { return profileContents, err } if !replacedVariable { return profileContents, ctxerr.Wrap(deps.GetContext(), err, "host end user IDP variable replacement failed for variable") }At the
!replacedVariablebranch,erris guaranteed to benil(you already returned when it was non-nil), soctxerr.Wrapwill likely returnnil, causing the function to report success while leaving the variable unreplaced. That defeats the intention of treating “not replaced” as a profile-level processing error.A safer implementation that integrates with the existing
MicrosoftProfileProcessingErrorhandling:- replacedContents, replacedVariable, err := profiles.ReplaceHostEndUserIDPVariables(deps.GetContext(), deps.GetDS(), fleetVar, result, params.GetHostUUID(), deps.GetHostIdForUUIDCache(), func(errMsg string) error { - return &MicrosoftProfileProcessingError{message: errMsg} - }) + replacedContents, replacedVariable, err := profiles.ReplaceHostEndUserIDPVariables( + deps.GetContext(), + deps.GetDS(), + fleetVar, + result, + params.GetHostUUID(), + deps.GetHostIdForUUIDCache(), + func(errMsg string) error { + return &MicrosoftProfileProcessingError{message: errMsg} + }, + }) if err != nil { return profileContents, err } if !replacedVariable { - return profileContents, ctxerr.Wrap(deps.GetContext(), err, "host end user IDP variable replacement failed for variable") + return profileContents, &MicrosoftProfileProcessingError{ + message: fmt.Sprintf("host end user IDP variable replacement failed for variable %s", fleetVar), + } } result = replacedContentsThis ensures callers (e.g.,
ReconcileWindowsProfiles) see a concreteMicrosoftProfileProcessingErrorand can handle it per-profile.
- Shadowing
depsandparamswhen asserting deploy-only behaviorparams, isForDeploy := params.(ProfilePreprocessParamsForDeploy) if !isForDeploy { continue } deps, hasDepsForDeploy := deps.(ProfilePreprocessDependenciesForDeploy) if !hasDepsForDeploy { return profileContents, errors.New("missing configuration to deploy profiles") }This works (the short declarations introduce new, concrete-typed
params/depsin the loop scope), but the shadowing is subtle and easy to misread.For readability, consider renaming the asserted values:
- params, isForDeploy := params.(ProfilePreprocessParamsForDeploy) + deployParams, isForDeploy := params.(ProfilePreprocessParamsForDeploy) if !isForDeploy { continue } - deps, hasDepsForDeploy := deps.(ProfilePreprocessDependenciesForDeploy) + deployDeps, hasDepsForDeploy := deps.(ProfilePreprocessDependenciesForDeploy) if !hasDepsForDeploy { return profileContents, errors.New("missing configuration to deploy profiles") }and then use
deployParams/deployDepsin the SCEP/custom-SCEP cases:- case fleetVar == string(fleet.FleetVarSCEPWindowsCertificateID): - result = profiles.ReplaceFleetVariableInXML(fleet.FleetVarSCEPWindowsCertificateIDRegexp, result, params.ProfileUUID) + case fleetVar == string(fleet.FleetVarSCEPWindowsCertificateID): + result = profiles.ReplaceFleetVariableInXML( + fleet.FleetVarSCEPWindowsCertificateIDRegexp, + result, + deployParams.ProfileUUID, + ) ... - err := profiles.IsCustomSCEPConfigured(deps.Context, deps.CustomSCEPCAs, caName, fleetVar, ...) + err := profiles.IsCustomSCEPConfigured(deployDeps.Context, deployDeps.CustomSCEPCAs, caName, fleetVar, ...) ... - replacedContents, managedCertificate, replacedVariable, err := profiles.ReplaceCustomSCEPProxyURLVariable(deps.Context, deps.Logger, deps.DataStore, deps.AppConfig, fleetVar, deps.CustomSCEPCAs, result, params.HostUUID, params.ProfileUUID) + replacedContents, managedCertificate, replacedVariable, err := profiles.ReplaceCustomSCEPProxyURLVariable( + deployDeps.Context, + deployDeps.Logger, + deployDeps.DataStore, + deployDeps.AppConfig, + fleetVar, + deployDeps.CustomSCEPCAs, + result, + deployParams.HostUUID, + deployParams.ProfileUUID, + ) ... - *params.ManagedCertificatePayloads = append(*params.ManagedCertificatePayloads, managedCertificate) + *deployParams.ManagedCertificatePayloads = append(*deployParams.ManagedCertificatePayloads, managedCertificate)This keeps verify vs deploy behavior unchanged but makes the code much easier to follow.
Also applies to: 441-495
🧹 Nitpick comments (2)
server/mdm/microsoft/profile_verifier_test.go (1)
1152-1221: Deployment preprocessing wiring is sound; consider strengthening multi-CA coverageThe deployment tests correctly:
- Share a
hostIDForUUIDCachemap across cases and assert it is populated, exercising the cache path.- Build
customSCEPCAsonce viagroupedCAs.ToCustomSCEPProxyCAMap()and pass it throughProfilePreprocessDependenciesForDeploy.- Reuse a shared
managedCertificatesslice pointer to accumulate managed cert payloads across calls.Functionally this matches the production usage in
ReconcileWindowsProfiles.One thing you might consider (optionally) is adding a test with multiple Custom SCEP CAs to ensure the map helper and downstream lookups behave correctly when more than one CA is configured. That would have caught issues like taking the address of the range variable in
ToCustomSCEPProxyCAMap.server/mdm/microsoft/profile_verifier.go (1)
303-322: New preprocessing wrappers and dependency/param structs are well-structuredThe introduction of:
ProfilePreprocessDependenciesForVerify/ProfilePreprocessDependenciesForDeployProfilePreprocessParamsForVerify/ProfilePreprocessParamsForDeployMicrosoftProfileProcessingErrorcleanly separates shared vs deploy-only concerns and gives callers explicit containers to pass context, datastore, app config, CA config, and managed-cert slices.
Two minor points you may want to tighten:
- The comment on
PreprocessWindowsProfileContentsForVerificationstill says it doesn’t require a datastore/logger, but the underlying generic preprocessing now does use them for IDP variables; updating the comment would avoid confusion.- The interfaces (
ProfilePreprocessDependencies,ProfilePreprocessParams) plus concrete structs are slightly over-abstracted for the current usage; if the only implementations are the concrete structs, you could simplify by havingpreprocessWindowsProfileContentsaccept the deploy/verify structs directly. Not urgent.Also applies to: 334-392
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/fleet/certificate_authorities.go(1 hunks)server/mdm/microsoft/profile_verifier.go(7 hunks)server/mdm/microsoft/profile_verifier_test.go(4 hunks)server/service/microsoft_mdm.go(2 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/mdm/microsoft/profile_verifier_test.goserver/service/microsoft_mdm.goserver/mdm/microsoft/profile_verifier.goserver/fleet/certificate_authorities.go
🧠 Learnings (1)
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/mdm/microsoft/profile_verifier_test.goserver/service/microsoft_mdm.goserver/mdm/microsoft/profile_verifier.go
🔇 Additional comments (3)
server/mdm/microsoft/profile_verifier_test.go (1)
931-947: Verification preprocessing deps/params wiring looks correctThe
ProfilePreprocessDependenciesForVerifyconstruction and updated call toPreprocessWindowsProfileContentsForVerificationmatch the new API and keep the tests focused on host UUID substitution. No functional issues here.server/service/microsoft_mdm.go (1)
2318-2328: New deployment preprocessing dependency wiring is correct
ReconcileWindowsProfilesnow builds a singleProfilePreprocessDependenciesForDeploywith sharedHostIDForUUIDCache,AppConfig, andCustomSCEPCAs, and passes per-host/per-profile params plus a sharedmanagedCertificatePayloadsslice. This matches the intended dependency/params pattern and integrates cleanly with the later bulk upsert and per-profile error handling.Once the
ToCustomSCEPProxyCAMaprange-variable bug is fixed, this should behave correctly even with multiple Custom SCEP CAs.Also applies to: 2358-2369
server/mdm/microsoft/profile_verifier.go (1)
45-50: LoopOverExpectedHostProfiles refactor to use preprocessing deps looks goodCreating a single
ProfilePreprocessDependenciesForVerify(with context, logger, datastore, and a host ID cache) and routing expected profiles throughPreprocessWindowsProfileContentsForVerificationkeeps verification consistent with deployment preprocessing and centralizes variable handling. The per-host cache is correctly scoped to the loop and won’t leak across hosts.Also applies to: 58-64
| // ToCustomSCEPProxyCAMap converts the CustomScepProxy slice to a map keyed by CA name | ||
| func (g *GroupedCertificateAuthorities) ToCustomSCEPProxyCAMap() map[string]*CustomSCEPProxyCA { | ||
| customSCEPCAs := make(map[string]*CustomSCEPProxyCA, len(g.CustomScepProxy)) | ||
| for _, ca := range g.CustomScepProxy { | ||
| customSCEPCAs[ca.Name] = &ca | ||
| } | ||
| return customSCEPCAs | ||
| } |
There was a problem hiding this comment.
Fix range-variable pointer bug in ToCustomSCEPProxyCAMap
Using &ca takes the address of the range variable, so every map entry will point at the last element when len(g.CustomScepProxy) > 1. This will break lookups once multiple custom SCEP CAs are configured.
Suggested fix:
-// ToCustomSCEPProxyCAMap converts the CustomScepProxy slice to a map keyed by CA name
-func (g *GroupedCertificateAuthorities) ToCustomSCEPProxyCAMap() map[string]*CustomSCEPProxyCA {
- customSCEPCAs := make(map[string]*CustomSCEPProxyCA, len(g.CustomScepProxy))
- for _, ca := range g.CustomScepProxy {
- customSCEPCAs[ca.Name] = &ca
- }
- return customSCEPCAs
-}
+// ToCustomSCEPProxyCAMap converts the CustomScepProxy slice to a map keyed by CA name.
+func (g *GroupedCertificateAuthorities) ToCustomSCEPProxyCAMap() map[string]*CustomSCEPProxyCA {
+ customSCEPCAs := make(map[string]*CustomSCEPProxyCA, len(g.CustomScepProxy))
+ for i := range g.CustomScepProxy {
+ ca := &g.CustomScepProxy[i]
+ customSCEPCAs[ca.Name] = ca
+ }
+ return customSCEPCAs
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ToCustomSCEPProxyCAMap converts the CustomScepProxy slice to a map keyed by CA name | |
| func (g *GroupedCertificateAuthorities) ToCustomSCEPProxyCAMap() map[string]*CustomSCEPProxyCA { | |
| customSCEPCAs := make(map[string]*CustomSCEPProxyCA, len(g.CustomScepProxy)) | |
| for _, ca := range g.CustomScepProxy { | |
| customSCEPCAs[ca.Name] = &ca | |
| } | |
| return customSCEPCAs | |
| } | |
| // ToCustomSCEPProxyCAMap converts the CustomScepProxy slice to a map keyed by CA name. | |
| func (g *GroupedCertificateAuthorities) ToCustomSCEPProxyCAMap() map[string]*CustomSCEPProxyCA { | |
| customSCEPCAs := make(map[string]*CustomSCEPProxyCA, len(g.CustomScepProxy)) | |
| for i := range g.CustomScepProxy { | |
| ca := &g.CustomScepProxy[i] | |
| customSCEPCAs[ca.Name] = ca | |
| } | |
| return customSCEPCAs | |
| } |
🤖 Prompt for AI Agents
In server/fleet/certificate_authorities.go around lines 501 to 508, the function
takes the address of the range loop variable (using &ca) which causes every map
entry to point to the same address (the last element); fix this by taking the
address of the actual slice element (e.g., use the index: for i := range
g.CustomScepProxy { customSCEPCAs[g.CustomScepProxy[i].Name] =
&g.CustomScepProxy[i] }) or by creating a new local variable inside the loop and
taking its address, so each map entry stores a distinct pointer to the
corresponding slice element.
There was a problem hiding this comment.
@coderabbitai This is incorrect. We're using Go 1.25 here; the loop variable scope issue flagged hasn't been a thing for multiple minor releases of Go.
There was a problem hiding this comment.
@iansltx, you're absolutely correct, and I apologize for the incorrect review comment. In Go 1.22, the language changed for loops to make variables have per-iteration scope instead of per-loop scope, with the new semantics applying to packages in modules that declare go 1.22 or later in their go.mod files.
Since you're using Go 1.25, each iteration of the loop creates new variables, which means taking the address of ca with &ca is perfectly safe. The code in ToCustomSCEPProxyCAMap() is correct as written.
Thank you for the correction!
🧠 Learnings used
Learnt from: getvictor
Repo: fleetdm/fleet PR: 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.
Learnt from: lucasmrod
Repo: fleetdm/fleet PR: 30550
File: server/datastore/mysql/policies_test.go:6368-6378
Timestamp: 2025-07-03T15:13:36.494Z
Learning: Go 1.22 introduced a significant change to loop variable semantics where each iteration of a for loop creates a new variable, automatically fixing the classic closure capture bug that existed in earlier Go versions. Code that captures loop variables in closures no longer needs explicit variable copying in Go 1.22+. This fix is only enabled if the module's go.mod file specifies go 1.22 or higher.
Learnt from: lucasmrod
Repo: fleetdm/fleet PR: 30550
File: server/datastore/mysql/policies_test.go:6368-6378
Timestamp: 2025-07-03T15:13:36.494Z
Learning: Go 1.22 introduced a significant change to loop variable semantics where each iteration of a for loop creates a new variable, automatically fixing the classic closure capture bug that existed in earlier Go versions. Code that captures loop variables in closures no longer needs explicit variable copying in Go 1.22+.
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31579
File: ee/orbit/pkg/hostidentity/host_identity.go:15-15
Timestamp: 2025-08-04T18:28:51.825Z
Learning: The math/rand/v2 package is available in Go 1.22 and later releases, not just a proposal as previously thought.
Learnt from: getvictor
Repo: fleetdm/fleet PR: 33218
File: orbit/pkg/table/santa/ringbuffer_test.go:48-50
Timestamp: 2025-10-07T19:42:55.988Z
Learning: Go 1.22 introduced the range-over-int feature, allowing `for i := range N` syntax to iterate from 0 to N-1. This is valid and idiomatic Go code in projects using Go 1.22 or later.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35804 +/- ##
========================================
Coverage 66.23% 66.23%
========================================
Files 2108 2109 +1
Lines 179054 179274 +220
Branches 7473 7473
========================================
+ Hits 118593 118750 +157
- Misses 49586 49640 +54
- Partials 10875 10884 +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:
|
TODO: Update test coverage
|
Created a PR from the cron branch. Closing this one, opening the other. |
2 participants
Should be no functional changes, though in a few cases I was able to pull some dep declarations outside of loops. Setting this as a separate PR so refactors can be evaluated without worrying about functional changes, and vice versa. Functional changes on #34364 will be stacked on top of this.
Summary by CodeRabbit