Extend support for all built-in - #49354
Conversation
Relates to #38806 Extend host name templates beyond the three host-identity variables to also accept the IdP end-user variables, and re-enqueue the rename when a referenced variable's value changes. Re-enqueue on value change: - An IdP data change (SCIM user/group create/update/delete) re-queues only the affected hosts whose template uses the changed IdP variable — IdP values are per host, so the scope is the specific hosts mapped to that user/group. - A custom (secret) value change re-queues every eligible host in each team / "No team" whose template references the changed secret — secret values are global, so the scope is the whole team/No-team. Built-in variables intentionally NOT supported: - Certificate/CA variables — $FLEET_VAR_NDES_SCEP_CHALLENGE, _NDES_SCEP_PROXY_URL, _CUSTOM_SCEP_CHALLENGE_*, _CUSTOM_SCEP_PROXY_URL_*, _SMALLSTEP_SCEP_CHALLENGE_*, _SMALLSTEP_SCEP_PROXY_URL_*, _DIGICERT_DATA_*, _DIGICERT_PASSWORD_*, _SCEP_WINDOWS_CERTIFICATE_ID, _CERTIFICATE_RENEWAL_ID (and legacy _SCEP_RENEWAL_ID), _PSSO_DEVICE_REGISTRATION_TOKEN. These resolve to one-time SCEP challenges, proxy URLs, base64 PKCS12 cert data, or Fleet-minted tokens — meaningless as a device name, and resolving them has side effects (issuing certificates, consuming one-time challenges) and would leak secrets into a name that's broadcast on-device, in osquery, and in the UI. - Legacy $FLEET_VAR_HOST_END_USER_EMAIL_IDP — deprecated ("avoid in new replacements") and not a documented built-in variable, so it's excluded in favor of the supported IDP_USERNAME variables.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49354 +/- ##
========================================
Coverage 67.97% 67.97%
========================================
Files 3823 3823
Lines 241031 241232 +201
Branches 12702 12702
========================================
+ Hits 163831 163979 +148
- Misses 62325 62360 +35
- Partials 14875 14893 +18
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:
|
5171525 to
fbbda2f
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughHost-name templates now distinguish host-identity and IdP end-user variables. Apple device-name reconciliation resolves IdP values after host fields, while missing data produces delivery failures and datastore errors remain retryable. Secret and SCIM variable changes re-queue affected enforcement records. App-config, team, and GitOps validation paths classify errors and skip validation for unchanged templates. Tests cover resolution, re-enqueueing, global templates, and GitOps behavior. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/apple_device_names.go (1)
261-268: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPer-host
GetEndUserslookup when the template uses IdP variables.Unlike the secret expansion above (memoized once per distinct template), this issues one
fleet.GetEndUsersdatastore call per host whenever the team's template contains an IdP variable — a per-host DB round trip inside the batch loop. Given the batch is capped byreconcileHostDeviceNamesBatchSizeand mirrors how profile-side IdP resolution already works per host, this is likely an acceptable tradeoff, but worth keeping in mind if the cron's batch size or IdP-template adoption grows.🤖 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/service/apple_device_names.go` around lines 261 - 268, No code change is required: retain the per-host GetEndUsers lookup in the device-name template flow, as it matches existing profile-side IdP resolution and the bounded batch size. Avoid introducing memoization or broader refactoring unless future performance requirements change.
🤖 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/secret_variables.go`:
- Around line 99-108: The secret writes and resendDeviceNamesForSecretChange
call must execute atomically. Update the surrounding secret-update flow and
resendDeviceNamesForSecretChange to share one transaction-aware helper or
durable retry/outbox mechanism, ensuring a failed requeue cannot commit the
secret changes without preserving the pending device-name requeue for retries.
---
Nitpick comments:
In `@server/service/apple_device_names.go`:
- Around line 261-268: No code change is required: retain the per-host
GetEndUsers lookup in the device-name template flow, as it matches existing
profile-side IdP resolution and the bounded batch size. Avoid introducing
memoization or broader refactoring unless future performance requirements
change.
🪄 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
Run ID: 8159a4ab-9ed5-4b59-b286-07c3a2deac43
📒 Files selected for processing (13)
ee/server/service/teams.goserver/datastore/mysql/apple_device_names.goserver/datastore/mysql/scim.goserver/datastore/mysql/secret_variables.goserver/datastore/mysql/secret_variables_test.goserver/fleet/name_template.goserver/fleet/name_template_test.goserver/service/appconfig.goserver/service/appconfig_test.goserver/service/apple_device_names.goserver/service/apple_device_names_test.goserver/service/apple_mdm_test.goserver/service/integration_mdm_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/apple_device_names.go`:
- Around line 325-330: The SQL regex patterns in
server/datastore/mysql/apple_device_names.go lines 325-330 and
server/datastore/mysql/scim.go lines 1594-1610 must match only actual template
tokens: update the patterns used by the team query and SCIM query to require the
`$` prefix and support both `$FLEET_SECRET_NAME`/`${FLEET_SECRET_NAME}` and
`$FLEET_VAR_NAME`/`${FLEET_VAR_NAME}` forms, respectively, so unrelated literal
text is excluded.
In `@server/service/apple_device_names.go`:
- Around line 270-275: Update the substitution flow around
resolveHostNameIDPValue so all IdP variable values are resolved first, then
apply replacements against the original name template in a single pass; do not
feed each replacement’s output into subsequent substitutions. Preserve existing
error details and return behavior, and add a regression test covering an IdP
value containing syntax for a later variable so that syntax remains literal.
🪄 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
Run ID: 415948a5-f41a-4608-af49-19ff927a25cf
📒 Files selected for processing (13)
ee/server/service/teams.goserver/datastore/mysql/apple_device_names.goserver/datastore/mysql/scim.goserver/datastore/mysql/secret_variables.goserver/datastore/mysql/secret_variables_test.goserver/fleet/name_template.goserver/fleet/name_template_test.goserver/service/appconfig.goserver/service/appconfig_test.goserver/service/apple_device_names.goserver/service/apple_device_names_test.goserver/service/apple_mdm_test.goserver/service/integration_mdm_test.go
| var user *fleet.HostEndUser | ||
| if len(users) > 0 && users[0].IdpUserName != "" { | ||
| user = &users[0] | ||
| } |
There was a problem hiding this comment.
is there a real-world case where a host has more than one end user but this would still always pick the first one?
| } | ||
| } | ||
|
|
||
| // TestModifyAppConfigHostNameTemplateSecretErrors verifies that when the "No team" |
There was a problem hiding this comment.
is this really "No team"? I might be missing something but I don't see how this test is specific to "No team" 😬
2 participants
Relates to #38806
Extend host name templates beyond the three host-identity variables to also accept the IdP end-user variables, and re-enqueue the rename when a referenced variable's value changes.
Re-enqueue on value change:
Built-in variables intentionally NOT supported:
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Testing
Summary by CodeRabbit
name_templatebetter supports secret placeholders and preserves them for server-side processing.