Add sticky MDM enrollment Redis key - #33935
Conversation
GitHub Actions
Failure
MagnusHJensen
changed the title
MagnusHJensen
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33935 +/- ##
==========================================
+ Coverage 64.14% 64.20% +0.05%
==========================================
Files 2052 2054 +2
Lines 206252 206694 +442
Branches 6885 6885
==========================================
+ Hits 132297 132701 +404
- Misses 63558 63574 +16
- Partials 10397 10419 +22
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:
|
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
| RefetchMDMUnenrollCriticalQueryDuration = 3 * time.Minute | ||
|
|
||
| StickyMDMEnrollmentKeyPrefix = "sticky_mdm_enrollment_" // + host UUID | ||
| StickyMDMEnrollmentTTL = 30 * time.Minute |
There was a problem hiding this comment.
I think 30 minutes is a fair small value, that way we ensure it's only within a short, reasonable timeframe we make the mdm team enrollment sticky, and avoid it causing weird behaviours if a host was to re-enroll orbit into a new team long enough after, where it isn't a mistake.
WalkthroughIntroduces a Redis-backed key-value store to set/read a short-lived “sticky MDM enrollment” flag. Service constructors and tests are updated to inject this store. Orbit enrollment conditionally ignores team updates when the sticky flag is present. MDM Apple check-in sets the flag on enrollment. MySQL host enrollment honors an option to skip team updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Device
participant MDM as MDM Apple Check-in
participant Service
participant KV as KeyValueStore (Redis)
participant DS as Datastore
Note over MDM,Service: MDM enrollment check-in
Device->>MDM: Authenticate (enroll)
MDM->>Service: Record MDM enrolled activity
Service->>KV: SET sticky_mdm_enrollment_<hardware_uuid> = "1" (TTL=30m)
KV-->>Service: OK/Err (non-fatal)
Service-->>Device: 200
Note over Device,Service: Orbit enroll later
Device->>Service: Orbit Enroll (hardware_uuid, enroll secret)
Service->>KV: GET sticky_mdm_enrollment_<hardware_uuid>
KV-->>Service: Value/null
alt Sticky present
Service->>DS: EnrollOrbit(..., IgnoreTeamUpdate=true)
else No sticky
Service->>DS: EnrollOrbit(..., IgnoreTeamUpdate=false)
end
DS-->>Service: Updated host
Service-->>Device: Enroll OK
sequenceDiagram
autonumber
participant API as Hosts Transfer API
participant KV as KeyValueStore (Redis)
participant Orbit as Orbit Enroll
participant DS as Datastore
Note over API,KV: After transfer to Team B
API->>KV: (No direct write in this PR)
Note over Orbit,DS: Subsequent Orbit enroll
Orbit->>KV: GET sticky_mdm_enrollment_<hardware_uuid>
KV-->>Orbit: "1"
Orbit->>DS: EnrollOrbit IgnoreTeamUpdate=true
DS-->>Orbit: Host remains in Team B
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 4
🧹 Nitpick comments (7)
server/datastore/mysql/hosts.go (1)
2254-2286: Conditional team_id update looks correct; verify both branches are tested
- Placeholder/args alignment is sound in both branches; WHERE id = ? scopes the UPDATE properly. Good.
- Please confirm tests cover: ignore=true/false and teamID nil/non‑nil so we don’t regress enrollment behavior.
Optionally:
- Include team_id and reason in the skip log to aid debugging (e.g., sticky flag source/TTL).
server/fleet/orbit.go (1)
165-169: Add a short GoDoc for WithEnrollOrbitIgnoreTeamUpdateAdd a brief comment (e.g., “WithEnrollOrbitIgnoreTeamUpdate configures whether team_id is skipped during Orbit (re)enrollment updates.”) to keep exported API documented.
server/service/apple_mdm_test.go (1)
1180-1184: Good injection; consider a small helper and pool choice
- Injecting keyValueStore and logger is correct.
- Optional: extract a test helper to build MDMAppleCheckinAndCommandService to avoid duplication across tests.
- Optional: NopRedis is fine here; ensure tests that validate sticky behavior use a real pool (e.g., redistest.SetupRedis) as done in integration tests.
Also applies to: 1250-1254
server/fleet/mdm.go (1)
29-31: Centralize sticky enrollment key construction
- Multiple suffix values are used (HardwareUUID in Orbit, r.ID in Apple MDM, host.UUID in tests); wrap key construction in a helper (e.g. StickyMDMEnrollmentKey(id string)) to ensure consistency.
server/service/orbit.go (1)
181-185: Approve the fail-open error handling approach.The code correctly reads the sticky MDM enrollment flag from Redis and logs errors without failing enrollment. This is appropriate since sticky enrollment is a protective measure rather than core functionality. However, consider adding:
- A comment explaining what the sticky enrollment flag is and why it's checked (e.g., "Sticky enrollment prevents race conditions where a host transferred to a team gets re-enrolled into 'No Team' due to timing issues with MDM profile delivery").
- Metrics/monitoring for Redis retrieval failures to detect consistent issues.
Apply this diff to add documentation:
+ // Check for sticky MDM enrollment flag. When set (e.g., after a host transfer), + // this prevents enrollment-based team changes for a time window to avoid race conditions + // with MDM profile delivery. stickyEnrollment, err := svc.keyValueStore.Get(ctx, fleet.StickyMDMEnrollmentKeyPrefix+hostInfo.HardwareUUID) if err != nil { - // We do not want to fail here, just log the error to notify + // Log error but continue enrollment (fail-open approach). If Redis is unavailable, + // enrollment proceeds without sticky behavior rather than blocking. level.Error(svc.logger).Log("msg", "failed to get sticky enrollment", "err", err, "host_uuid", hostInfo.HardwareUUID) }server/service/apple_mdm.go (1)
3578-3584: Harden sticky-key set: add timeout and nil-guardAvoid blocking the check-in path and guard against a nil store. Suggested change:
- // Set sticky key for MDM enrollments to avoid updating team id on orbit enrollments - err = svc.keyValueStore.Set(r.Context, fleet.StickyMDMEnrollmentKeyPrefix+r.ID, "1", fleet.StickyMDMEnrollmentTTL) - if err != nil { - // We do not want to fail here, just log the error to notify - level.Error(svc.logger).Log("msg", "failed to set sticky mdm enrollment key", "err", err, "host_uuid", r.ID) - } + // Set sticky key for MDM enrollments to avoid updating team id on orbit enrollments. + if svc.keyValueStore != nil { + ctx, cancel := context.WithTimeout(r.Context, 500*time.Millisecond) + defer cancel() + if err := svc.keyValueStore.Set(ctx, fleet.StickyMDMEnrollmentKeyPrefix+r.ID, "1", fleet.StickyMDMEnrollmentTTL); err != nil { + // Non-fatal; log and continue. + level.Error(svc.logger).Log("msg", "failed to set sticky mdm enrollment key", "err", err, "host_uuid", r.ID) + } + } else { + level.Warn(svc.logger).Log("msg", "keyValueStore is nil; sticky mdm enrollment disabled", "host_uuid", r.ID) + }server/service/testing_utils.go (1)
426-435: Consider conditional Redis pool initialization for efficiency.The code unconditionally calls
redistest.SetupRedisat line 428, even whenopts[0].Poolwill overrideredisPoolat line 430. This creates an unused Redis pool when a custom pool is provided via options.Consider initializing the Redis pool conditionally:
memLimitStore, _ := memstore.New(0) var limitStore throttled.GCRAStore = memLimitStore - redisPool := redistest.SetupRedis(t, t.Name(), false, false, false) // We are good to initalize a redis pool here as it is only called by integration tests + var redisPool fleet.RedisPool if len(opts) > 0 && opts[0].Pool != nil { redisPool = opts[0].Pool limitStore = &redis.ThrottledStore{ Pool: opts[0].Pool, KeyPrefix: "ratelimit::", } + } else { + redisPool = redistest.SetupRedis(t, t.Name(), false, false, false) // We are good to initalize a redis pool here as it is only called by integration tests }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
changes/26879-add-sticky-enrollment-redis-key(1 hunks)cmd/fleet/serve.go(2 hunks)ee/server/integrationtest/hostidentity/suite.go(2 hunks)ee/server/service/mdm_external_test.go(1 hunks)server/datastore/mysql/hosts.go(1 hunks)server/fleet/mdm.go(1 hunks)server/fleet/orbit.go(2 hunks)server/service/apple_mdm.go(2 hunks)server/service/apple_mdm_test.go(4 hunks)server/service/integration_mdm_test.go(5 hunks)server/service/orbit.go(1 hunks)server/service/service.go(3 hunks)server/service/testing_utils.go(3 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/fleet/orbit.goserver/service/orbit.goserver/service/service.goserver/service/apple_mdm_test.goee/server/integrationtest/hostidentity/suite.gocmd/fleet/serve.goserver/service/testing_utils.goserver/service/integration_mdm_test.goee/server/service/mdm_external_test.goserver/datastore/mysql/hosts.goserver/service/apple_mdm.go
🔇 Additional comments (16)
changes/26879-add-sticky-enrollment-redis-key (1)
1-1: LGTM!The changelog entry clearly describes the bug fix from the user's perspective. The issue where host transfers would be reversed by orbit enroll is now documented.
ee/server/integrationtest/hostidentity/suite.go (2)
12-12: LGTM!The import of
redistestis necessary to wire Redis-backed storage into the test setup.
52-52: LGTM!The Redis pool is correctly wired into the test server setup via
redistest.SetupRedis. The threefalseparameters likely control Redis features (e.g., cluster mode, TLS, persistence), and usingfalsevalues is appropriate for integration tests that need a simple Redis instance.cmd/fleet/serve.go (2)
777-777: LGTM!The Redis-backed key-value store is correctly wired into the main Fleet service initialization. Using
redis_key_value.New(redisPool)creates a new store instance that shares the underlying connection pool, which is appropriate for this stateless wrapper.
1282-1282: LGTM!The Redis-backed key-value store is correctly wired into the MDM Apple checkin and command service. This enables the sticky enrollment feature for MDM flows by providing the necessary storage backend.
server/fleet/orbit.go (1)
120-126: New IgnoreTeamUpdate flag: API shape LGTMField name and behavior are clear and align with the enrollment flow usage.
server/service/apple_mdm_test.go (2)
37-37: LGTM: Redis test import added appropriatelyImporting redistest aligns with new key-value dependency for tests.
52-52: LGTM: Key-value store import wired for testsredis_key_value is correctly introduced for injecting the store.
server/service/orbit.go (1)
193-193: LGTM! Correct implementation of sticky enrollment logic.The nil check correctly determines whether to ignore team updates during enrollment:
- If the Redis key exists (
stickyEnrollment != nil), team updates are ignored- If the key doesn't exist or Redis errors occurred, team updates proceed normally (safe default)
This properly implements the race condition mitigation described in the PR objectives.
server/service/service.go (2)
69-71: LGTM: service gains a KeyValueStore dependencyUnexported field is fine; clean injection point for the sticky enrollment feature.
181-182: LGTM: dependency wiredField assignment is correct; no other behavior changes here.
server/service/apple_mdm.go (2)
3484-3484: LGTM: adds KeyValueStore to Apple check-in serviceField addition is straightforward and aligns with the sticky enrollment design.
3487-3496: Constructor updated — callers pass non-nil storeAll instantiations (serve.go and testing_utils.go) use
redis_key_value.New(redisPool), which returns a non-nilKeyValueStore. No further action needed.server/service/testing_utils.go (3)
30-30: LGTM!The import addition is necessary for the
redistest.SetupRediscall introduced later in the file.
442-442: Verify that creating multipleredis_key_value.Newinstances is intentional.The code creates separate
redis_key_value.Newinstances at line 122 (for the main service'skeyValueStore) and line 442 (forNewMDMAppleCheckinAndCommandService). Ifopts[0].Poolis provided, both instances use the same underlying Redis pool but are separate instances.Please confirm this is intentional (e.g., different services need isolated instances) rather than unintended duplication.
219-219: LGTM!The
keyValueStoreparameter is properly initialized earlier in the function (lines 98-100 or 118-123) before being passed toNewService.
GitHub Actions
Inactive
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
What about osquery enrollment? Do we also need to check there? I notice that it also looks at the secret and uses it to set team ID during enrollment:
// WithEnrollOsqueryTeamID sets the team ID for datastore Host enrollment
func WithEnrollOsqueryTeamID(teamID *uint) DatastoreEnrollOsqueryOption {
return func(c *DatastoreEnrollOsqueryConfig) {
c.TeamID = teamID
}
}
2 participants
Related issue: Resolves #26879
We decided to opt for a sticky enrollment approach, and I opted for using redis, so this PR also adds a redis key value store to the free service to use.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit