Add user configurable fleetd base URL#48136
Closed
William-TecNQ wants to merge 11 commits into
Closed
Conversation
… related tests to pass in app configration
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #48136 +/- ##
==========================================
- Coverage 67.32% 67.23% -0.10%
==========================================
Files 3657 3657
Lines 231233 231319 +86
Branches 12062 12211 +149
==========================================
- Hits 155676 155521 -155
- Misses 61590 61864 +274
+ Partials 13967 13934 -33
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:
|
Contributor
|
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 (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughA new Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/fleetdbase/fleetd_base_test.go (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd precedence and trailing-slash coverage for the new base URL contract.
Current tests validate env-only and config-only cases, but not the critical
configURL + envprecedence or trailing-slash behavior.Suggested test additions
func TestGetBaseURL(t *testing.T) { + t.Run("configURL takes precedence over env variable", func(t *testing.T) { + dev_mode.SetOverride("FLEET_DEV_DOWNLOAD_FLEETDM_URL", "https://download-testing.fleetdm.com", t) + require.Equal(t, "https://download.test.com", getBaseURL("https://download.test.com")) + }) + t.Run("with env variable", func(t *testing.T) { dev_mode.SetOverride("FLEET_DEV_DOWNLOAD_FLEETDM_URL", "https://download-testing.fleetdm.com", t) require.Equal(t, "https://download-testing.fleetdm.com", getBaseURL("")) }) @@ func TestGetPKGManifestURL(t *testing.T) { + t.Run("configURL with trailing slash", func(t *testing.T) { + require.Equal(t, "https://download.test.com/stable/fleetd-base-manifest.plist", GetPKGManifestURL("https://download.test.com/")) + }) + t.Run("with env variable", func(t *testing.T) { dev_mode.SetOverride("FLEET_DEV_DOWNLOAD_FLEETDM_URL", "https://download-test.fleetdm.com", t) require.Equal(t, "https://download-test.fleetdm.com/stable/fleetd-base-manifest.plist", GetPKGManifestURL("")) })Also applies to: 88-100
🤖 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 `@pkg/fleetdbase/fleetd_base_test.go` around lines 13 - 25, The TestGetBaseURL test function lacks coverage for two critical scenarios: precedence when both configURL and the FLEET_DEV_DOWNLOAD_FLEETDM_URL environment variable are set, and handling of trailing slashes in URLs. Add a test case using t.Run that sets both the FLEET_DEV_DOWNLOAD_FLEETDM_URL environment variable via dev_mode.SetOverride and passes a non-empty configURL parameter to getBaseURL to verify which value takes precedence. Additionally, add separate test cases to verify that the getBaseURL function correctly handles trailing slashes in configURL values, testing both cases where configURL has a trailing slash and where it does not.
🤖 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 `@pkg/fleetdbase/fleetd_base.go`:
- Around line 22-35: The getBaseURL function returns the configBaseURL as-is
without normalizing it, which causes malformed URLs with double slashes when
admins provide a trailing slash (e.g., https://mirror.local/fleetd/ becomes
https://mirror.local/fleetd//stable/meta.json). In the getBaseURL function,
before returning configBaseURL on line 23 or any other returned URL value, trim
any trailing slashes to ensure URLs are properly normalized. This normalization
should also be applied to other URL construction locations referenced at lines
60-62 to maintain consistency across all path appending operations.
---
Nitpick comments:
In `@pkg/fleetdbase/fleetd_base_test.go`:
- Around line 13-25: The TestGetBaseURL test function lacks coverage for two
critical scenarios: precedence when both configURL and the
FLEET_DEV_DOWNLOAD_FLEETDM_URL environment variable are set, and handling of
trailing slashes in URLs. Add a test case using t.Run that sets both the
FLEET_DEV_DOWNLOAD_FLEETDM_URL environment variable via dev_mode.SetOverride and
passes a non-empty configURL parameter to getBaseURL to verify which value takes
precedence. Additionally, add separate test cases to verify that the getBaseURL
function correctly handles trailing slashes in configURL values, testing both
cases where configURL has a trailing slash and where it does not.
🪄 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: 2c0833df-0f39-4921-b47e-b0f403fc4d58
⛔ Files ignored due to path filters (1)
articles/self-host-fleetd-for-automatic-enrollment.mdis excluded by!**/*.md
📒 Files selected for processing (16)
changes/48060-self-host-fleetd-base-urlfrontend/__mocks__/configMock.tsfrontend/interfaces/config.tsfrontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsxfrontend/pages/admin/OrgSettingsPage/cards/Advanced/components/ServerAuthenticationSection/ServerAuthenticationSection.tsxpkg/fleetdbase/fleetd_base.gopkg/fleetdbase/fleetd_base_test.goserver/fleet/app.goserver/service/appconfig.goserver/service/integration_apple_vpp_config_test.goserver/service/integration_mdm_dep_test.goserver/service/integration_mdm_lifecycle_test.goserver/service/integration_mdm_test.goserver/service/integration_vpp_install_test.goserver/service/microsoft_mdm.goserver/worker/apple_mdm.go
Comment on lines
+22
to
35
| func getBaseURL(configBaseURL string) string { | ||
| if configBaseURL != "" { | ||
| return configBaseURL | ||
| } | ||
| devURL := dev_mode.Env("FLEET_DEV_DOWNLOAD_FLEETDM_URL") | ||
| if devURL != "" { | ||
| return devURL | ||
| } | ||
| return "https://download.fleetdm.com" | ||
| } | ||
|
|
||
| func GetMetadata() (*Metadata, error) { | ||
| baseURL := getBaseURL() | ||
| func GetMetadata(configBaseURL string) (*Metadata, error) { | ||
| baseURL := getBaseURL(configBaseURL) | ||
| rawURL := fmt.Sprintf("%s/stable/meta.json", baseURL) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize base URL before appending /stable/... paths.
Returning configBaseURL as-is on Line 23 can produce malformed URLs like https://mirror.local/fleetd//stable/meta.json and ...//stable/fleetd-base-manifest.plist when admins enter a trailing slash.
Suggested fix
import (
"encoding/json"
"fmt"
"net/http"
"net/url"
+ "strings"
"github.com/fleetdm/fleet/v4/server/dev_mode"
)
func getBaseURL(configBaseURL string) string {
if configBaseURL != "" {
- return configBaseURL
+ return strings.TrimRight(configBaseURL, "/")
}
devURL := dev_mode.Env("FLEET_DEV_DOWNLOAD_FLEETDM_URL")
if devURL != "" {
- return devURL
+ return strings.TrimRight(devURL, "/")
}
return "https://download.fleetdm.com"
}Also applies to: 60-62
🤖 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 `@pkg/fleetdbase/fleetd_base.go` around lines 22 - 35, The getBaseURL function
returns the configBaseURL as-is without normalizing it, which causes malformed
URLs with double slashes when admins provide a trailing slash (e.g.,
https://mirror.local/fleetd/ becomes
https://mirror.local/fleetd//stable/meta.json). In the getBaseURL function,
before returning configBaseURL on line 23 or any other returned URL value, trim
any trailing slashes to ensure URLs are properly normalized. This normalization
should also be applied to other URL construction locations referenced at lines
60-62 to maintain consistency across all path appending operations.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a user-configurable fleetd base URL (via org advanced settings) and threads it through Apple (manifest URL) and Windows (meta.json fetch) MDM enrollment flows so environments with restricted outbound access can self-host the fleetd artifacts.
Changes:
- Introduces
agent_settings.fleetd_base_urlin app config and returns it from/fleet/config. - Uses the configured base URL when generating the macOS manifest URL and when fetching Windows fleetd-base metadata.
- Updates integration/unit tests and adds a new guide article + changelog entry.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| server/worker/apple_mdm.go | Uses app config fleetd_base_url when generating the macOS fleetd manifest URL for MDM install. |
| server/service/microsoft_mdm.go | Uses app config fleetd_base_url when fetching fleetd-base metadata for Windows MDM install. |
| server/service/integration_vpp_install_test.go | Updates integration tests to validate manifest URL generation against configured base URL. |
| server/service/integration_mdm_test.go | Updates shared helper to assert manifest URL using configured base URL; updates call sites. |
| server/service/integration_mdm_lifecycle_test.go | Updates assertions to match configured fleetd base URL behavior. |
| server/service/integration_mdm_dep_test.go | Updates DEP-related assertions to match configured fleetd base URL behavior. |
| server/service/integration_apple_vpp_config_test.go | Updates assertions for fleetd install command to use configured base URL. |
| server/service/appconfig.go | Includes AgentSettings in the config response payload. |
| server/fleet/app.go | Adds AgentSettings and fleetd_base_url to the app config model. |
| pkg/fleetdbase/fleetd_base.go | Makes fleetd-base URL helpers accept a configured base URL. |
| pkg/fleetdbase/fleetd_base_test.go | Updates unit tests for new fleetdbase function signatures and config override behavior. |
| frontend/pages/admin/OrgSettingsPage/cards/Advanced/components/ServerAuthenticationSection/ServerAuthenticationSection.tsx | Adds “Fleet Agent base URL” UI field with tooltip/help link. |
| frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx | Adds form state, validation, and payload wiring for agent_settings.fleetd_base_url. |
| frontend/interfaces/config.ts | Extends IConfig with agent_settings.fleetd_base_url. |
| frontend/mocks/configMock.ts | Updates config mock to include agent_settings. |
| changes/48060-self-host-fleetd-base-url | Adds changelog entry for the new setting. |
| articles/self-host-fleetd-for-automatic-enrollment.md | Adds a guide describing self-hosting the fleetd artifacts and configuring the base URL. |
Comments suppressed due to low confidence (1)
pkg/fleetdbase/fleetd_base.go:38
rawURL := fmt.Sprintf("%s/stable/meta.json", baseURL)will produce a double slash whenfleetd_base_urlends with/(e.g.https://example.com/→https://example.com//stable/...), which can break self-hosted setups depending on the web server/proxy. Building the URL viaurl.JoinPathavoids that class of errors and correctly handles base URLs that include a subdirectory.
baseURL := getBaseURL(configBaseURL)
rawURL := fmt.Sprintf("%s/stable/meta.json", baseURL)
parsedURL, err := url.Parse(rawURL)
if err != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,4 @@ | |||
| // pacakge fleetdbase contains functions to interact with downloads.fleetdm.com | |||
| // package fleetdbase contains functions to interact with downloads.fleetdm.com | |||
Comment on lines
+60
to
63
| func GetPKGManifestURL(configBaseURL string) string { | ||
| baseURL := getBaseURL(configBaseURL) | ||
| return fmt.Sprintf("%s/stable/fleetd-base-manifest.plist", baseURL) | ||
| } |
Comment on lines
+33
to
37
| func GetMetadata(configBaseURL string) (*Metadata, error) { | ||
| baseURL := getBaseURL(configBaseURL) | ||
| rawURL := fmt.Sprintf("%s/stable/meta.json", baseURL) | ||
|
|
||
| parsedURL, err := url.Parse(rawURL) |
| fleetdBaseURL := appCfg.AgentSettings.FleetdBaseURL | ||
| fleetdMetadata, err := fleetdbase.GetMetadata(fleetdBaseURL) | ||
| if err != nil { | ||
| svc.logger.WarnContext(ctx, "unable to get fleetd-base metadata") |
Comment on lines
229
to
231
| ConditionalAccess: appConfig.ConditionalAccess, | ||
| AgentSettings: appConfig.AgentSettings, | ||
| }, |
Comment on lines
+565
to
+570
| func (a *AppleMDM) installFleetd(ctx context.Context, hostUUID string) (string, error) { | ||
| manifestURL := fleetdbase.GetPKGManifestURL() | ||
| cfg, err := a.Datastore.AppConfig(ctx) | ||
| if err != nil { | ||
| return "", ctxerr.Wrap(ctx, err, "retrieving app config") | ||
| } | ||
| manifestURL := fleetdbase.GetPKGManifestURL(cfg.AgentSettings.FleetdBaseURL) |
Comment on lines
+18
to
+19
| * [fleetd-base.pkg](https://download.fleetdm.com/stable/fleetd-base.pkg) - Used for MacOS enrollments | ||
| * [fleetd-base-manifest.plist](https://download.fleetdm.com/stable/fleetd-base-manifest.plist) - Used for MacOS enrollments |
Comment on lines
+5
to
+6
| This can be configured to ensure that host enrollments are successful in environments with strict network access | ||
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/worker/apple_mdm.go`:
- Around line 174-175: In the `runPostDEPEnrollment` function, `appCfg` is only
loaded when `args.TeamID == nil`, leaving it nil for team-based DEP enrollment.
When `!manualAgentInstall` is true, this causes `installFleetd` to receive a nil
`appCfg` and panic when dereferencing `cfg.AgentSettings.FleetdBaseURL`. Before
the call to `installFleetd(ctx, args.HostUUID, appCfg)` in the
`!manualAgentInstall` block, add logic to load `appCfg` for the team-based case
using the existing `getAppConfig` function (which handles lazy caching),
ensuring `appCfg` is never nil when passed to `installFleetd`.
🪄 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: 1d428cae-7bbc-42f7-aebe-ce89cb2317d1
⛔ Files ignored due to path filters (1)
articles/self-host-fleetd-for-automatic-enrollment.mdis excluded by!**/*.md
📒 Files selected for processing (2)
pkg/fleetdbase/fleetd_base.goserver/worker/apple_mdm.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/fleetdbase/fleetd_base.go
mike-j-thomas
requested review from
noahtalerman
and removed request for
mike-j-thomas
Member
|
Thanks @William-TecNQ! Up to @melpike as Product Designer per the community PR process: https://fleetdm.com/handbook/engineering#review-a-community-pull-request |
Member
|
Hey @William-TecNQ, are you allow listing Apple but not Fleet? Best practice is to let devices talk to Apple’s services and fleetdm.com. Apple services are required for ADE enrollment, while updates.fleetdm.com are required for Fleet’s agent to update. One option is to use the dev flag We'll close this PR for now and leave the feature request open. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #48060
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit
fleetdfrom.fleetdmetadata/manifest URLs for installer-related MDM flows, based on the latest app configuration returned to clients.