Skip MDM Declaration Validation logic with Configuration. - #38212
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38212 +/- ##
==========================================
- Coverage 66.28% 66.25% -0.04%
==========================================
Files 2467 2467
Lines 197447 197273 -174
Branches 8732 8732
==========================================
- Hits 130881 130704 -177
- Misses 54724 54732 +8
+ Partials 11842 11837 -5
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:
|
|
Thanks for the PR! Since this is a product change (opposed to a bug), we pass it through to our product team to review. |
|
Thanks @weswhet!
@mostlikelee's right! Tim, just a reminder to assign the relevant Product Designer (@marko-lisica in this case) and notify them in Slack: https://fleetdm.com/handbook/engineering#review-a-community-pull-request:~:text=Assign%20the%20PR,compliance%20Slack%20channel. |
|
Hey @marko-lisica I fixed most of the failing checks minus the current CodeQL one which isn't related to my changes. Let me know if you need anything else from me. |
|
Thanks @weswhet - we'll be in touch shortly once we get @marko-lisica to weigh in on this. |
|
Hey @marko-lisica - just curious but when do you anticipate being able to merge this PR? I spent time with Thanks in advance! |
|
Hey @Patagonia121, we are planning to ship this change in 4.83. It is currently on the drafting board, and we follow our regular drafting process. Next sprint engineering team will pick it up, review/QA, and make sure we can ship it. Later, we plan to officially release this so that all Fleet customers can use all DDM features after we test it internally. |
|
hey @marko-lisica just want to confirm that we're still good to go on 4.83 for this PR merge? thx |
There was a problem hiding this comment.
Hi @weswhet Looking at this now.
Could I get you to update all code references for skip_declaration_validation to allow_all_declarations like seen in the updated contributor docs?
Co-authored-by: Marko Lisica <83164494+marko-lisica@users.noreply.github.com>
Co-authored-by: Marko Lisica <83164494+marko-lisica@users.noreply.github.com>
Co-authored-by: Marko Lisica <83164494+marko-lisica@users.noreply.github.com>
08aeeea to
8bcbaa2
Compare
|
@weswhet I fixed the comments myself, will do a QA pass and then merge the PR. Thanks for the contribution 🎉 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis change introduces an experimental configuration flag 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/config/config.go (1)
1591-1591: Clarify the flag description to match actual behavior.The current description says it allows declaration types, but this flag bypasses broader declaration validation checks. Updating the help text would reduce operator confusion.
Proposed wording update
- man.addConfigBool("mdm.allow_all_declarations", false, "Experimental feature: Allows all MDM declaration types to be sent") + man.addConfigBool("mdm.allow_all_declarations", false, "Experimental feature: bypasses Apple declaration validation checks")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/config/config.go` at line 1591, Update the help text passed to man.addConfigBool for the "mdm.allow_all_declarations" flag so it accurately reflects behavior: instead of saying it "Allows all MDM declaration types to be sent", change the description to indicate it bypasses broader declaration validation checks (e.g., "Experimental: Bypass MDM declaration validation checks; may permit unsupported or invalid declaration types"). Locate the call to man.addConfigBool("mdm.allow_all_declarations", ...) and replace the existing description string with the clarified wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/Contributing/reference/configuration-for-contributors.md`:
- Line 251: Replace the malformed bold label `**Note: **` with `**Note:**` in
the markdown snippet so the emphasis has no inner trailing space and conforms to
markdownlint MD037; update the line containing the Asset declaration note (the
bold "Note" label) to use `**Note:**` exactly.
---
Nitpick comments:
In `@server/config/config.go`:
- Line 1591: Update the help text passed to man.addConfigBool for the
"mdm.allow_all_declarations" flag so it accurately reflects behavior: instead of
saying it "Allows all MDM declaration types to be sent", change the description
to indicate it bypasses broader declaration validation checks (e.g.,
"Experimental: Bypass MDM declaration validation checks; may permit unsupported
or invalid declaration types"). Locate the call to
man.addConfigBool("mdm.allow_all_declarations", ...) and replace the existing
description string with the clarified wording.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
changes/38366-contributor-flag-for-allowing-all-declarationsdocs/Contributing/reference/configuration-for-contributors.mdserver/config/config.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/mdm.go
|
@rachaelshaw Can you approve this PR as the contributor docs DRI? |
|
|
||
| ### mdm.allow_all_declarations | ||
|
|
||
| > Experimental feature. Use with caution. |
There was a problem hiding this comment.
Let's use the default experimental message:
| > Experimental feature. Use with caution. | |
| > **Experimental feature.** This feature is undergoing rapid improvement, which may result in breaking changes to the API or configuration surface. It is not recommended for use in automated workflows. |
There was a problem hiding this comment.
actually, never mind, I'll make this change after this is merged so I don't undo other approvals to this PR
There was a problem hiding this comment.
@rachaelshaw Merged the PR feel free to do the follow up doc change
Related issue: Resolves #
This PR adds the ability to skip any Fleet logic that validates on whether or not an MDM Declaration is usable with Fleet by setting a configuration of
FLEET_MDM_SKIP_DECLARATION_VALIDATIONtoTrue. We would like to leverage declarations such as config files as well as other none configuration declarations that are restricted like Activations, and any declaration that requires AssetData. We understand that these are not usable by most Fleet customers, especially those that use Fleets cloud hosting option.Since we run Fleet on our own infrastructure we are able to leverage our proxy to support all additional data components needed for declarations above.
Testing
I've built and run Fleet locally and validated that with the
FLEET_MDM_SKIP_DECLARATION_VALIDATIONI am able to add any declarations I please. Without the config I am unable to add the requested declaration types above.Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
mdm.allow_all_declarationsconfiguration option to permit all Apple MDM declaration types when enabled, overriding default validation checks.Documentation