From d508c36be2e36f87c353d0797698dbf26a6c210a Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Tue, 18 Aug 2026 15:08:56 -0400 Subject: [PATCH 1/3] feat(diff): deep-diff the no-team bucket Closes #41. Fleet's "hosts on no team" bucket was informational only: fleet-plan printed "2 policies, 2 scripts configured (no API diff available)" and moved on, so a changed policy or a removed script in teams/no-team.yml (fleets/unassigned.yml) produced no diff at all. The bucket is absent from GET /teams, but its resources are reachable: - policies: GET /teams/0/policies. This is a different set from /global/policies; the response also carries inherited_policies (the global ones), which are ignored because a no-team file does not own them. - profiles and scripts: team_id=0 on /configuration_profiles and /scripts. Note that teamID 0 previously meant "omit the filter" in this client, which returns every team's resources rather than the no-team bucket's. GetProfiles, GetScripts, and GetSoftware now always send team_id. No caller passed 0 to them before, so this only enables the new path. Changes: - api: FleetState.NoTeam holds the bucket. FetchAll takes a FetchOptions struct (Global, NoTeam) instead of a variadic bool, and fetches the bucket only when asked. Each fetch degrades to an "unavailable" flag on 403/404, matching how per-team resources already behave with a gitops-scoped token. - diff: diffNoTeam diffs policies, profiles, and scripts like any other team, and falls back to the old summary when the bucket was not fetched. - cmd: requests the bucket only when the repo has a no-team file. Software is deliberately not diffed for the bucket. Fleet reports configured software only through the teams list, which excludes no team, so there is nothing to compare against; the diff says so explicitly rather than reporting every configured item as an addition. Verified against the live Fleet instance with the production fleet-gitops repo: the real no-team file now diffs clean (previously an informational line), and a modified copy correctly reports a modified policy and a deleted script. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KEpzMNJnGaBLAfrPeqknCy --- cmd/fleet-plan/main.go | 17 ++++- docs/API-Endpoints.md | 3 + docs/Architecture.md | 2 + internal/api/client.go | 136 +++++++++++++++++++++++++++++++---- internal/api/client_test.go | 134 +++++++++++++++++++++++++++++++++- internal/diff/differ.go | 55 ++++++++++++-- internal/diff/differ_test.go | 136 +++++++++++++++++++++++++++++++++++ 7 files changed, 459 insertions(+), 24 deletions(-) diff --git a/cmd/fleet-plan/main.go b/cmd/fleet-plan/main.go index ae521e7..e66d560 100644 --- a/cmd/fleet-plan/main.go +++ b/cmd/fleet-plan/main.go @@ -168,7 +168,10 @@ func runDiff(cmd *cobra.Command, _ []string) error { } fmt.Fprintf(os.Stderr, "Fetching Fleet state from %s...\n", auth.URL) - state, err := client.FetchAll(ctx, repo.Global != nil) + state, err := client.FetchAll(ctx, api.FetchOptions{ + Global: repo.Global != nil, + NoTeam: hasNoTeam(repo.Teams), + }) if err != nil { return err } @@ -228,6 +231,18 @@ func runDiff(cmd *cobra.Command, _ []string) error { return nil } +// hasNoTeam reports whether the repo configures Fleet's "hosts on no team" +// bucket. Fetching that bucket costs extra API calls, so it is only requested +// when a file describes it. +func hasNoTeam(teams []parser.ParsedTeam) bool { + for _, t := range teams { + if parser.IsNoTeam(t.Name, t.SourceFile) { + return true + } + } + return false +} + // errChangesDetected signals --detailed-exitcodes exit status 2. It is not a // failure: main translates it to the exit code after runDiff's defers run. var errChangesDetected = errors.New("changes detected") diff --git a/docs/API-Endpoints.md b/docs/API-Endpoints.md index 2b683ba..b3c47da 100644 --- a/docs/API-Endpoints.md +++ b/docs/API-Endpoints.md @@ -9,6 +9,7 @@ All read-only. fleet-plan never writes to your Fleet server. | `GET` | `/api/v1/fleet/labels` | Label validation and host counts | | `GET` | `/api/v1/fleet/teams/{id}/policies` | Per-team policies | | `GET` | `/api/v1/fleet/global/policies` | Global policies (when default.yml parsed) | +| `GET` | `/api/v1/fleet/teams/0/policies` | Policies for hosts on no team (when the repo has a no-team file) | | `GET` | `/api/v1/fleet/queries` | Per-team and global queries | | `GET` | `/api/v1/fleet/configuration_profiles` | MDM configuration profiles | | `GET` | `/api/v1/fleet/software/titles` | Managed software titles (paginated) | @@ -19,4 +20,6 @@ All read-only. fleet-plan never writes to your Fleet server. Global endpoints (`/config`, `/global/policies`, `/queries` with teamID=0) are only called when `default.yml` defines global sections. +The "hosts on no team" bucket is fetched only when the repo has a no-team file (`teams/no-team.yml` or `fleets/unassigned.yml`). Its resources live behind `team_id=0` on `/configuration_profiles` and `/scripts`, and behind `/teams/0/policies` for policies — note that `/global/policies` is a *different* set. Fleet does not report configured software for this bucket, so software is not diffed there. + HTTPS enforced unless `FLEET_PLAN_INSECURE=1`. diff --git a/docs/Architecture.md b/docs/Architecture.md index 90ff4dc..ab52a3f 100644 --- a/docs/Architecture.md +++ b/docs/Architecture.md @@ -89,6 +89,8 @@ Walks `teams/*.yml`, resolves `path:` references, produces `ParsedRepo`. Also pa Compares `FleetState` (API) vs `ParsedRepo` (YAML). Produces `[]DiffResult` per team + a `(global)` result when `default.yml` is present. +Fleet's "hosts on no team" bucket is absent from `GET /teams`, so it is fetched separately (`team_id=0`) and diffed like any other team for policies, profiles, and scripts. Software is reported as skipped there: Fleet exposes configured software only through the teams list. When the bucket was not fetched, the diff falls back to summarizing what the repo configures for it. + | Resource | Match key | Diff fields | |----------|-----------|-------------| | Config sections (global) | dot-path key | old/new value (skips `$VAR` placeholders) | diff --git a/internal/api/client.go b/internal/api/client.go index bcc3760..bb02019 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -122,6 +122,32 @@ type FleetState struct { Config map[string]any // from GET /api/v1/fleet/config GlobalPolicies []Policy // from GET /api/v1/fleet/global/policies (teamID=0) GlobalQueries []Query // from GET /api/v1/fleet/queries (teamID=0) + NoTeam *NoTeam // "hosts on no team" bucket, when requested +} + +// NoTeam holds the current state of Fleet's "hosts on no team" bucket, which +// the /teams endpoint does not return. Its resources live behind team_id=0. +// +// Queries are absent by design: Fleet scopes queries to a real team or to the +// global scope, so a no-team file cannot define them. Managed software is +// absent too — Fleet only reports configured packages through the teams list, +// which excludes this bucket. +type NoTeam struct { + Policies []Policy + Profiles []Profile + Scripts []Script + PoliciesUnavailable bool + ProfilesUnavailable bool + ScriptsUnavailable bool +} + +// FetchOptions selects the optional scopes FetchAll retrieves. Both cost extra +// round trips, so callers ask for them only when the repo defines them. +type FetchOptions struct { + // Global fetches /config, /global/policies, and global queries. + Global bool + // NoTeam fetches the "hosts on no team" bucket (team_id=0). + NoTeam bool } // Team represents a Fleet team with its associated resources. @@ -415,6 +441,35 @@ func (c *Client) GetPolicies(ctx context.Context, teamID uint) ([]Policy, error) return all, nil } +// GetNoTeamPolicies fetches the policies that belong to Fleet's "hosts on no +// team" bucket. This is a different endpoint from GetPolicies(0), which +// returns global policies. The response also carries inherited_policies (the +// global ones, which apply to no-team hosts as well); those are deliberately +// ignored, since a no-team YAML file does not own them. +func (c *Client) GetNoTeamPolicies(ctx context.Context) ([]Policy, error) { + var all []Policy + page := 0 + for { + q := url.Values{ + "per_page": {"250"}, + "page": {strconv.Itoa(page)}, + } + var resp policiesResponse + if err := c.get(ctx, "/api/v1/fleet/teams/0/policies", q, &resp); err != nil { + return nil, fmt.Errorf("fetching no-team policies: %w", err) + } + all = append(all, resp.Policies...) + if len(resp.Policies) < 250 { + break + } + page++ + if page > 100 { // safety: max 25k policies + break + } + } + return all, nil +} + // GetQueries fetches queries, optionally filtered by team, with pagination. func (c *Client) GetQueries(ctx context.Context, teamID uint) ([]Query, error) { var all []Query @@ -443,7 +498,9 @@ func (c *Client) GetQueries(ctx context.Context, teamID uint) ([]Query, error) { return all, nil } -// GetSoftware fetches managed (available_for_install) software titles for a team. +// GetSoftware fetches managed (available_for_install) software titles for a +// team. team_id is always sent; see GetProfiles for why teamID 0 must not be +// omitted. // Uses available_for_install=true to exclude detected-only titles (OS packages, // browser extensions, etc.) and only return software deployed via Fleet/GitOps. // Paginates to collect all results. @@ -456,9 +513,7 @@ func (c *Client) GetSoftware(ctx context.Context, teamID uint) ([]SoftwareTitle, "page": {strconv.Itoa(page)}, "available_for_install": {"true"}, } - if teamID > 0 { - q.Set("team_id", strconv.FormatUint(uint64(teamID), 10)) - } + q.Set("team_id", strconv.FormatUint(uint64(teamID), 10)) var resp softwareResponse if err := c.get(ctx, "/api/v1/fleet/software/titles", q, &resp); err != nil { return nil, fmt.Errorf("fetching software (team %d): %w", teamID, err) @@ -570,7 +625,9 @@ func (c *Client) GetLabels(ctx context.Context) ([]Label, error) { return all, nil } -// GetProfiles fetches MDM profiles for a team with pagination. +// GetProfiles fetches MDM profiles for a team with pagination. team_id is +// always sent: Fleet reads teamID 0 as the "hosts on no team" bucket, whereas +// omitting the parameter returns every team's profiles. func (c *Client) GetProfiles(ctx context.Context, teamID uint) ([]Profile, error) { var all []Profile page := 0 @@ -579,9 +636,7 @@ func (c *Client) GetProfiles(ctx context.Context, teamID uint) ([]Profile, error "per_page": {"250"}, "page": {strconv.Itoa(page)}, } - if teamID > 0 { - q.Set("team_id", strconv.FormatUint(uint64(teamID), 10)) - } + q.Set("team_id", strconv.FormatUint(uint64(teamID), 10)) var resp profilesResponse if err := c.get(ctx, "/api/v1/fleet/configuration_profiles", q, &resp); err != nil { return nil, fmt.Errorf("fetching profiles (team %d): %w", teamID, err) @@ -598,7 +653,8 @@ func (c *Client) GetProfiles(ctx context.Context, teamID uint) ([]Profile, error return all, nil } -// GetScripts fetches scripts for a team with pagination. +// GetScripts fetches scripts for a team with pagination. team_id is always +// sent; see GetProfiles for why teamID 0 must not be omitted. func (c *Client) GetScripts(ctx context.Context, teamID uint) ([]Script, error) { var all []Script page := 0 @@ -607,9 +663,7 @@ func (c *Client) GetScripts(ctx context.Context, teamID uint) ([]Script, error) "per_page": {"250"}, "page": {strconv.Itoa(page)}, } - if teamID > 0 { - q.Set("team_id", strconv.FormatUint(uint64(teamID), 10)) - } + q.Set("team_id", strconv.FormatUint(uint64(teamID), 10)) var resp scriptsResponse if err := c.get(ctx, "/api/v1/fleet/scripts", q, &resp); err != nil { return nil, fmt.Errorf("fetching scripts (team %d): %w", teamID, err) @@ -680,9 +734,13 @@ func (c *Client) getScriptContent(ctx context.Context, scriptID uint) (string, e // FetchAll concurrently fetches the complete Fleet state. Uses errgroup for // parallel requests. If fetchGlobal is true, also fetches global config, // policies, and queries (for default.yml diffing). -func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState, error) { +func (c *Client) FetchAll(ctx context.Context, opts ...FetchOptions) (*FleetState, error) { state := &FleetState{} - wantGlobal := len(fetchGlobal) > 0 && fetchGlobal[0] + var o FetchOptions + if len(opts) > 0 { + o = opts[0] + } + wantGlobal := o.Global teams, err := c.GetTeams(ctx) if err != nil { @@ -743,6 +801,49 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState }) } + // noTeam is written only by the goroutines below, then attached to state + // after g.Wait(). + var noTeam *NoTeam + if o.NoTeam { + noTeam = &NoTeam{} + g.Go(func() error { + policies, err := c.GetNoTeamPolicies(gctx) + if err != nil { + if !isPermissionError(err) { + return err + } + noTeam.PoliciesUnavailable = true + return nil + } + noTeam.Policies = policies + return nil + }) + g.Go(func() error { + profiles, err := c.GetProfiles(gctx, 0) + if err != nil { + if !isPermissionError(err) { + return err + } + noTeam.ProfilesUnavailable = true + return nil + } + noTeam.Profiles = profiles + return nil + }) + g.Go(func() error { + scripts, err := c.GetScripts(gctx, 0) + if err != nil { + if !isPermissionError(err) { + return err + } + noTeam.ScriptsUnavailable = true + return nil + } + noTeam.Scripts = scripts + return nil + }) + } + // teamPartials holds per-goroutine results indexed by team slot. // Each field is written by exactly one goroutine, so there is no data race. type teamPartial struct { @@ -845,6 +946,13 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState teamResults[i].ScriptsUnavailable = p.scriptsUnavailable } + if noTeam != nil { + if !noTeam.ScriptsUnavailable && len(noTeam.Scripts) > 0 { + c.EnrichScriptContents(ctx, noTeam.Scripts) + } + state.NoTeam = noTeam + } + // Enrich script contents (second pass, needs script IDs from first pass) for i := range teamResults { if !teamResults[i].ScriptsUnavailable && len(teamResults[i].Scripts) > 0 { diff --git a/internal/api/client_test.go b/internal/api/client_test.go index 6952b0f..ebd2674 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -368,7 +368,10 @@ func TestGetProfilesTeamID(t *testing.T) { teamID uint wantTeamID string }{ - {name: "global profiles", teamID: 0, wantTeamID: ""}, + // teamID 0 is Fleet's "hosts on no team" bucket, so the parameter + // must be sent explicitly. Omitting it would return every team's + // profiles instead. + {name: "no-team profiles", teamID: 0, wantTeamID: "0"}, {name: "team 5 profiles", teamID: 5, wantTeamID: "5"}, } @@ -595,7 +598,7 @@ func TestFetchAllWithGlobal(t *testing.T) { defer ts.Close() c := testClient(t, ts, "tok") - state, err := c.FetchAll(context.Background(), true) + state, err := c.FetchAll(context.Background(), FetchOptions{Global: true}) if err != nil { t.Fatalf("FetchAll: %v", err) } @@ -1091,3 +1094,130 @@ func TestFetchAllFleetMaintainedFallback(t *testing.T) { }) } } + +// ---------- no-team bucket ---------- + +func TestGetNoTeamPolicies(t *testing.T) { + // /teams/0/policies returns the bucket's own policies plus the global ones + // it inherits. Only the former belong to a no-team YAML file. + const body = `{ + "policies": [{"id": 1, "name": "No-team policy", "query": "SELECT 1;"}], + "inherited_policies": [{"id": 99, "name": "Global policy", "query": "SELECT 2;"}] + }` + + var gotPath string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + fmt.Fprint(w, body) + })) + defer ts.Close() + + policies, err := testClient(t, ts, "tok").GetNoTeamPolicies(context.Background()) + if err != nil { + t.Fatalf("GetNoTeamPolicies: %v", err) + } + if gotPath != "/api/v1/fleet/teams/0/policies" { + t.Errorf("path: got %q", gotPath) + } + if len(policies) != 1 || policies[0].Name != "No-team policy" { + t.Fatalf("policies: got %+v, want only the no-team policy", policies) + } +} + +func TestFetchAllNoTeam(t *testing.T) { + var noTeamParams struct{ profiles, scripts string } + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/fleet/teams", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"teams":[]}`) + }) + mux.HandleFunc("/api/v1/fleet/labels", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"labels":[]}`) + }) + mux.HandleFunc("/api/v1/fleet/software/fleet_maintained_apps", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"fleet_maintained_apps":[]}`) + }) + mux.HandleFunc("/api/v1/fleet/teams/0/policies", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"policies":[{"id":1,"name":"No-team policy"}],"inherited_policies":[{"id":9,"name":"Global"}]}`) + }) + mux.HandleFunc("/api/v1/fleet/configuration_profiles", func(w http.ResponseWriter, r *http.Request) { + noTeamParams.profiles = r.URL.Query().Get("team_id") + fmt.Fprint(w, `{"profiles":[{"profile_uuid":"u1","name":"Conditional access"}]}`) + }) + mux.HandleFunc("/api/v1/fleet/scripts", func(w http.ResponseWriter, r *http.Request) { + noTeamParams.scripts = r.URL.Query().Get("team_id") + fmt.Fprint(w, `{"scripts":[{"id":0,"name":"uninstall.sh"}]}`) + }) + + ts := httptest.NewServer(mux) + defer ts.Close() + + t.Run("requested", func(t *testing.T) { + state, err := testClient(t, ts, "tok").FetchAll(context.Background(), FetchOptions{NoTeam: true}) + if err != nil { + t.Fatalf("FetchAll: %v", err) + } + if state.NoTeam == nil { + t.Fatal("NoTeam: got nil, want the fetched bucket") + } + if len(state.NoTeam.Policies) != 1 || state.NoTeam.Policies[0].Name != "No-team policy" { + t.Errorf("policies: got %+v", state.NoTeam.Policies) + } + if len(state.NoTeam.Profiles) != 1 || len(state.NoTeam.Scripts) != 1 { + t.Errorf("profiles=%d scripts=%d, want 1 each", len(state.NoTeam.Profiles), len(state.NoTeam.Scripts)) + } + // team_id=0 must be sent explicitly; omitting it returns every team's + // resources instead of the no-team bucket's. + if noTeamParams.profiles != "0" || noTeamParams.scripts != "0" { + t.Errorf("team_id params: profiles=%q scripts=%q, want 0 for both", + noTeamParams.profiles, noTeamParams.scripts) + } + }) + + t.Run("not requested", func(t *testing.T) { + state, err := testClient(t, ts, "tok").FetchAll(context.Background()) + if err != nil { + t.Fatalf("FetchAll: %v", err) + } + if state.NoTeam != nil { + t.Errorf("NoTeam: got %+v, want nil when not requested", state.NoTeam) + } + }) +} + +func TestFetchAllNoTeamPermissionErrors(t *testing.T) { + // A gitops-scoped token may be refused on some of these endpoints. That + // must degrade to "skipped", not fail the whole diff. + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/fleet/teams", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"teams":[]}`) + }) + mux.HandleFunc("/api/v1/fleet/labels", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"labels":[]}`) + }) + mux.HandleFunc("/api/v1/fleet/software/fleet_maintained_apps", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"fleet_maintained_apps":[]}`) + }) + mux.HandleFunc("/api/v1/fleet/teams/0/policies", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + }) + mux.HandleFunc("/api/v1/fleet/configuration_profiles", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + }) + mux.HandleFunc("/api/v1/fleet/scripts", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + + ts := httptest.NewServer(mux) + defer ts.Close() + + state, err := testClient(t, ts, "tok").FetchAll(context.Background(), FetchOptions{NoTeam: true}) + if err != nil { + t.Fatalf("FetchAll: %v", err) + } + if state.NoTeam == nil { + t.Fatal("NoTeam: got nil") + } + if !state.NoTeam.PoliciesUnavailable || !state.NoTeam.ProfilesUnavailable || !state.NoTeam.ScriptsUnavailable { + t.Errorf("unavailable flags: got %+v, want all true", state.NoTeam) + } +} diff --git a/internal/diff/differ.go b/internal/diff/differ.go index a794bc9..199eb0c 100644 --- a/internal/diff/differ.go +++ b/internal/diff/differ.go @@ -173,6 +173,53 @@ func noTeamSummary(t parser.ParsedTeam) string { return strings.Join(parts, ", ") } +// diffNoTeam fills in the diff for Fleet's "hosts on no team" bucket, whose +// resources live behind team_id=0 rather than in the /teams list. +// +// When the bucket was not fetched (older Fleet, or the caller did not ask for +// it), it falls back to reporting what the repo configures, so nothing +// silently disappears from the plan. +func diffNoTeam(result *DiffResult, current *api.NoTeam, proposed parser.ParsedTeam, changedFiles []string, cfg diffOptions) { + if current == nil { + if summary := noTeamSummary(proposed); summary != "" { + result.Errors = append(result.Errors, + fmt.Sprintf("%s configured (no API diff available for hosts on no team)", summary)) + } + return + } + + if current.PoliciesUnavailable { + result.Errors = append(result.Errors, "policies diff skipped: API token lacks permission to read no-team policies") + } else { + result.Policies = diffPolicies(current.Policies, proposed.Policies) + } + + if current.ProfilesUnavailable { + result.Errors = append(result.Errors, "profiles diff skipped: API token lacks permission to read profiles") + } else { + var warnings []string + result.Profiles, warnings = diffProfiles(current.Profiles, proposed.Profiles, changedFiles) + result.Errors = append(result.Errors, warnings...) + } + + if current.ScriptsUnavailable { + result.Errors = append(result.Errors, "scripts diff skipped: API token lacks permission to read scripts") + } else { + result.Scripts = diffScripts(current.Scripts, proposed.Scripts) + } + + // Fleet reports configured software only through the teams list, which + // excludes this bucket, so there is nothing to compare against. Say so + // rather than reporting every configured item as an addition. + if n := len(proposed.Software.Packages) + len(proposed.Software.FleetMaintained) + len(proposed.Software.AppStoreApps); n > 0 { + result.Errors = append(result.Errors, + fmt.Sprintf("software diff skipped: %d software items configured, but Fleet does not report software for hosts on no team", n)) + } + + vlog(cfg.verbose, "[%s] no-team diff: policies=%s profiles=%s scripts=%s", + proposed.Name, rdSummary(result.Policies), rdSummary(result.Profiles), rdSummary(result.Scripts)) +} + // rdNames returns names of changes for debugging. func rdNames(rd ResourceDiff) string { var names []string @@ -277,13 +324,7 @@ func Diff(current *api.FleetState, proposed *parser.ParsedRepo, teamFilters []st // returned by the /teams API endpoint. Skip the "will be created" // warning for it, and don't list its resources as additions. if parser.IsNoTeam(proposedTeam.Name, proposedTeam.SourceFile) { - // Can't deep-diff against API state since it's not in the teams - // list. Just report what the repo configures for it, so nothing - // silently disappears from the plan. - if summary := noTeamSummary(proposedTeam); summary != "" { - result.Errors = append(result.Errors, - fmt.Sprintf("%s configured (no API diff available for hosts on no team)", summary)) - } + diffNoTeam(&result, current.NoTeam, proposedTeam, changedFiles, cfg) } else { // Genuinely new team for _, p := range proposedTeam.Policies { diff --git a/internal/diff/differ_test.go b/internal/diff/differ_test.go index d00c26e..874ea6e 100644 --- a/internal/diff/differ_test.go +++ b/internal/diff/differ_test.go @@ -2654,3 +2654,139 @@ func TestDiffTestdataTeamSettings(t *testing.T) { } } } + +// When the no-team bucket has been fetched (team_id=0), it is diffed like any +// other team rather than being summarized. +func TestDiffNoTeamDeepDiff(t *testing.T) { + current := &api.FleetState{ + Teams: []api.Team{}, + Labels: []api.Label{}, + NoTeam: &api.NoTeam{ + Policies: []api.Policy{ + // Same name, different query → modified. + {Name: "RingCentral uninstalled", Query: "SELECT 1;", Platform: "darwin"}, + // Not in the YAML → deleted. + {Name: "Retired policy", Query: "SELECT 2;", PassingHostCount: 5}, + }, + Profiles: []api.Profile{{Name: "Conditional access", Platform: "darwin"}}, + Scripts: []api.Script{ + {ID: 1, Name: "uninstall-ringcentral.sh", Content: "echo one\n"}, + {ID: 2, Name: "gone.ps1", Content: "Write-Host removed\n"}, + }, + }, + } + + proposed := &parser.ParsedRepo{Teams: []parser.ParsedTeam{{ + Name: "Unassigned", + SourceFile: "fleets/unassigned.yml", + Policies: []parser.ParsedPolicy{ + {Name: "RingCentral uninstalled", Query: "SELECT 42;", Platform: "darwin"}, + {Name: "Brand new policy", Query: "SELECT 3;"}, + }, + Profiles: []parser.ParsedProfile{ + {Name: "Conditional access", Platform: "darwin", Path: "lib/profiles/ca.mobileconfig"}, + {Name: "Newly added profile", Platform: "darwin", Path: "lib/profiles/new.mobileconfig"}, + }, + Scripts: []parser.ParsedScript{ + {Name: "uninstall-ringcentral.sh", Content: "echo one\necho two\n"}, + }, + }}} + + results := Diff(current, proposed, nil, nil) + if len(results) != 1 { + t.Fatalf("got %d results, want 1", len(results)) + } + r := results[0] + + checks := []struct { + what string + got ResourceDiff + a int + m int + d int + }{ + {"policies", r.Policies, 1, 1, 1}, + {"profiles", r.Profiles, 1, 0, 0}, + {"scripts", r.Scripts, 0, 1, 1}, + } + for _, c := range checks { + if len(c.got.Added) != c.a || len(c.got.Modified) != c.m || len(c.got.Deleted) != c.d { + t.Errorf("%s: got +%d ~%d -%d, want +%d ~%d -%d", c.what, + len(c.got.Added), len(c.got.Modified), len(c.got.Deleted), c.a, c.m, c.d) + } + } + + // The summary fallback must not appear once a real diff is available. + for _, e := range r.Errors { + if strings.Contains(e, "no API diff available") { + t.Errorf("summary fallback still reported: %q", e) + } + if strings.Contains(e, "does not exist in Fleet yet") { + t.Errorf("no-team reported as a new team: %q", e) + } + } +} + +func TestDiffNoTeamUnavailableResources(t *testing.T) { + current := &api.FleetState{ + Teams: []api.Team{}, + Labels: []api.Label{}, + NoTeam: &api.NoTeam{ + PoliciesUnavailable: true, + ProfilesUnavailable: true, + ScriptsUnavailable: true, + }, + } + proposed := &parser.ParsedRepo{Teams: []parser.ParsedTeam{{ + Name: "No team", + SourceFile: "teams/no-team.yml", + Policies: []parser.ParsedPolicy{{Name: "P"}}, + }}} + + r := Diff(current, proposed, nil, nil)[0] + + // Nothing may be reported as an addition when the API side is unreadable: + // that would claim Fleet has none of it, which is not known. + if !r.Policies.IsEmpty() || !r.Profiles.IsEmpty() || !r.Scripts.IsEmpty() { + t.Errorf("expected empty diffs, got policies=%+v profiles=%+v scripts=%+v", + r.Policies, r.Profiles, r.Scripts) + } + for _, want := range []string{"policies diff skipped", "profiles diff skipped", "scripts diff skipped"} { + found := false + for _, e := range r.Errors { + if strings.Contains(e, want) { + found = true + } + } + if !found { + t.Errorf("missing %q in %v", want, r.Errors) + } + } +} + +func TestDiffNoTeamSoftwareIsReportedAsSkipped(t *testing.T) { + current := &api.FleetState{Teams: []api.Team{}, Labels: []api.Label{}, NoTeam: &api.NoTeam{}} + proposed := &parser.ParsedRepo{Teams: []parser.ParsedTeam{{ + Name: "Unassigned", + SourceFile: "fleets/unassigned.yml", + Software: parser.ParsedSoftware{ + Packages: []parser.ParsedSoftwarePackage{{URL: "https://example.com/a.pkg"}}, + FleetMaintained: []parser.ParsedFleetApp{{Slug: "zoom/darwin"}}, + }, + }}} + + r := Diff(current, proposed, nil, nil)[0] + + if !r.Software.IsEmpty() { + t.Errorf("software: got %+v, want empty (nothing to compare against)", r.Software) + } + found := false + for _, e := range r.Errors { + if strings.Contains(e, "software diff skipped: 2 software items configured") { + found = true + } + } + if !found { + t.Errorf("missing software skip note in %v", r.Errors) + } +} From 00726437adf3533c5f2d1c9c676436b91c89a311 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Tue, 18 Aug 2026 15:31:51 -0400 Subject: [PATCH 2/3] test(api,cmd): cover the no-team branches codecov flagged - GetNoTeamPolicies pagination, which the single-page test never reached - the three FetchAll goroutines' non-permission error paths: a 500 must fail the fetch rather than be reported as an empty bucket, unlike a 403/404 - hasNoTeam, for both the teams/ and fleets/ layouts Patch coverage for this branch is now 99.1%. The one remaining line is the page > 100 runaway guard, which matches every other paginator here and would need 25k synthetic policies to reach. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KEpzMNJnGaBLAfrPeqknCy --- cmd/fleet-plan/cmd_test.go | 36 ++++++++++++++++ internal/api/client_test.go | 85 +++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/cmd/fleet-plan/cmd_test.go b/cmd/fleet-plan/cmd_test.go index 17b2df0..ed67bd7 100644 --- a/cmd/fleet-plan/cmd_test.go +++ b/cmd/fleet-plan/cmd_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/CampusTech/fleet-plan/internal/git" + "github.com/CampusTech/fleet-plan/internal/parser" ) // ---------- version command ---------- @@ -755,3 +756,38 @@ func TestRunExitCodes(t *testing.T) { }) } } + +func TestHasNoTeam(t *testing.T) { + tests := []struct { + name string + teams []parser.ParsedTeam + want bool + }{ + {name: "no teams at all"}, + { + name: "ordinary teams only", + teams: []parser.ParsedTeam{{Name: "Workstations", SourceFile: "teams/workstations.yml"}}, + }, + { + name: "teams layout no-team file", + teams: []parser.ParsedTeam{ + {Name: "Workstations", SourceFile: "teams/workstations.yml"}, + {Name: "No team", SourceFile: "teams/no-team.yml"}, + }, + want: true, + }, + { + name: "fleets layout unassigned file", + teams: []parser.ParsedTeam{{Name: "Unassigned", SourceFile: "fleets/unassigned.yml"}}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := hasNoTeam(tt.teams); got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/api/client_test.go b/internal/api/client_test.go index ebd2674..3955fc3 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" ) @@ -1221,3 +1222,87 @@ func TestFetchAllNoTeamPermissionErrors(t *testing.T) { t.Errorf("unavailable flags: got %+v, want all true", state.NoTeam) } } + +func TestGetNoTeamPoliciesPagination(t *testing.T) { + // 250 results means "there may be more"; the client keeps paging until a + // short page comes back. + var pages []string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + page := r.URL.Query().Get("page") + pages = append(pages, page) + n := 250 + if page != "0" { + n = 2 + } + policies := make([]Policy, n) + for i := range policies { + policies[i] = Policy{ID: uint(i + 1), Name: fmt.Sprintf("p%s-%d", page, i)} + } + _ = json.NewEncoder(w).Encode(policiesResponse{Policies: policies}) + })) + defer ts.Close() + + policies, err := testClient(t, ts, "tok").GetNoTeamPolicies(context.Background()) + if err != nil { + t.Fatalf("GetNoTeamPolicies: %v", err) + } + if len(policies) != 252 { + t.Errorf("got %d policies, want 252", len(policies)) + } + if strings.Join(pages, ",") != "0,1" { + t.Errorf("pages requested: got %v, want [0 1]", pages) + } +} + +func TestFetchAllNoTeamFatalErrors(t *testing.T) { + // A 403/404 degrades to "unavailable", but any other failure is a real + // problem and must not be reported as an empty bucket. + tests := []struct { + name string + failPath string + }{ + {"policies", "/api/v1/fleet/teams/0/policies"}, + {"profiles", "/api/v1/fleet/configuration_profiles"}, + {"scripts", "/api/v1/fleet/scripts"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/fleet/teams", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"teams":[]}`) + }) + mux.HandleFunc("/api/v1/fleet/labels", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"labels":[]}`) + }) + mux.HandleFunc("/api/v1/fleet/software/fleet_maintained_apps", func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"fleet_maintained_apps":[]}`) + }) + // Every no-team endpoint succeeds except the one under test, + // which returns a server error rather than a 403/404. + ok := map[string]string{ + "/api/v1/fleet/teams/0/policies": `{"policies":[]}`, + "/api/v1/fleet/configuration_profiles": `{"profiles":[]}`, + "/api/v1/fleet/scripts": `{"scripts":[]}`, + } + for path, body := range ok { + if path == tt.failPath { + mux.HandleFunc(path, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + continue + } + mux.HandleFunc(path, func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, body) + }) + } + + ts := httptest.NewServer(mux) + defer ts.Close() + + if _, err := testClient(t, ts, "tok").FetchAll(context.Background(), FetchOptions{NoTeam: true}); err == nil { + t.Fatalf("expected FetchAll to fail when %s returns 500", tt.name) + } + }) + } +} From e785941d81c041f897049d00d5a21f4bc9166bba Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Tue, 18 Aug 2026 15:37:32 -0400 Subject: [PATCH 3/3] fix(diff): subtract baseline for no-team; report skipped no-team queries Two review findings on #56: - The no-team diff ignored the baseline, so a policy, profile, or script change that was merged to the base branch but not yet deployed was reported again on every later MR. It is now subtracted like any other team's. The baseline's no-team file is matched on no-team identity rather than display name, since the base and MR branches can spell it differently ("No team" vs "Unassigned") -- exactly what happens in the MR that migrates a repo from the teams/ layout to fleets/. - `queries:` in a no-team file were dropped silently. Fleet scopes queries to a real team or to the global scope, so they cannot be diffed there; the plan now says so, matching how skipped software is reported. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KEpzMNJnGaBLAfrPeqknCy --- docs/Architecture.md | 2 +- internal/diff/differ.go | 47 ++++++++++++++++++++++++ internal/diff/differ_test.go | 70 ++++++++++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 1 deletion(-) diff --git a/docs/Architecture.md b/docs/Architecture.md index ab52a3f..841d322 100644 --- a/docs/Architecture.md +++ b/docs/Architecture.md @@ -89,7 +89,7 @@ Walks `teams/*.yml`, resolves `path:` references, produces `ParsedRepo`. Also pa Compares `FleetState` (API) vs `ParsedRepo` (YAML). Produces `[]DiffResult` per team + a `(global)` result when `default.yml` is present. -Fleet's "hosts on no team" bucket is absent from `GET /teams`, so it is fetched separately (`team_id=0`) and diffed like any other team for policies, profiles, and scripts. Software is reported as skipped there: Fleet exposes configured software only through the teams list. When the bucket was not fetched, the diff falls back to summarizing what the repo configures for it. +Fleet's "hosts on no team" bucket is absent from `GET /teams`, so it is fetched separately (`team_id=0`) and diffed like any other team for policies, profiles, and scripts, baseline subtraction included. Software and queries are reported as skipped there: Fleet exposes configured software only through the teams list, and scopes queries to a real team or the global scope. When the bucket was not fetched, the diff falls back to summarizing what the repo configures for it. | Resource | Match key | Diff fields | |----------|-----------|-------------| diff --git a/internal/diff/differ.go b/internal/diff/differ.go index 199eb0c..8cd03cb 100644 --- a/internal/diff/differ.go +++ b/internal/diff/differ.go @@ -216,6 +216,40 @@ func diffNoTeam(result *DiffResult, current *api.NoTeam, proposed parser.ParsedT fmt.Sprintf("software diff skipped: %d software items configured, but Fleet does not report software for hosts on no team", n)) } + // Fleet scopes queries to a real team or to the global scope, so a + // no-team file cannot own them. The parser still accepts `queries:` and + // `reports:` in any team file, so say plainly that these are not diffed + // instead of dropping them without a word. + if n := len(proposed.Queries); n > 0 { + result.Errors = append(result.Errors, + fmt.Sprintf("queries diff skipped: %d queries configured, but Fleet has no query scope for hosts on no team", n)) + } + + // Subtract changes that already exist between the base branch and Fleet, + // so a no-team change that is merged but not yet deployed is not reported + // again on every later MR. + if cfg.baseline != nil { + if baseTeam, ok := findBaselineNoTeam(cfg.baseline); ok { + base := DiffResult{} + if !current.PoliciesUnavailable { + base.Policies = diffPolicies(current.Policies, baseTeam.Policies) + } + if !current.ProfilesUnavailable { + base.Profiles, _ = diffProfiles(current.Profiles, baseTeam.Profiles, nil) + } + if !current.ScriptsUnavailable { + base.Scripts = diffScripts(current.Scripts, baseTeam.Scripts) + } + result.Policies = subtractResourceDiff(result.Policies, base.Policies) + result.Profiles = subtractResourceDiff(result.Profiles, base.Profiles) + result.Scripts = subtractResourceDiff(result.Scripts, base.Scripts) + vlog(cfg.verbose, "[%s] after baseline subtraction: policies=%s profiles=%s scripts=%s", + proposed.Name, rdSummary(result.Policies), rdSummary(result.Profiles), rdSummary(result.Scripts)) + } else { + vlog(cfg.verbose, "[%s] no baseline no-team file found", proposed.Name) + } + } + vlog(cfg.verbose, "[%s] no-team diff: policies=%s profiles=%s scripts=%s", proposed.Name, rdSummary(result.Policies), rdSummary(result.Profiles), rdSummary(result.Scripts)) } @@ -540,6 +574,19 @@ func filterChanges(changes []ResourceChange, keep func(string) bool) []ResourceC // ---------- Baseline subtraction ---------- // findBaselineTeam looks up a team by name in the baseline parsed repo. +// findBaselineNoTeam returns the baseline's no-team file. It matches on the +// no-team identity rather than the display name, because the base branch and +// the MR branch may spell it differently ("No team" vs "Unassigned") -- for +// instance in the MR that migrates a repo from the teams/ layout to fleets/. +func findBaselineNoTeam(baseline *parser.ParsedRepo) (parser.ParsedTeam, bool) { + for _, t := range baseline.Teams { + if parser.IsNoTeam(t.Name, t.SourceFile) { + return t, true + } + } + return parser.ParsedTeam{}, false +} + func findBaselineTeam(baseline *parser.ParsedRepo, name string) (parser.ParsedTeam, bool) { for _, t := range baseline.Teams { if strings.EqualFold(t.Name, name) { diff --git a/internal/diff/differ_test.go b/internal/diff/differ_test.go index 874ea6e..4cb5518 100644 --- a/internal/diff/differ_test.go +++ b/internal/diff/differ_test.go @@ -2790,3 +2790,73 @@ func TestDiffNoTeamSoftwareIsReportedAsSkipped(t *testing.T) { t.Errorf("missing software skip note in %v", r.Errors) } } + +// A no-team change that is already merged to the base branch but not yet +// deployed must not be reported again on every later MR. +func TestDiffNoTeamBaselineSubtraction(t *testing.T) { + current := &api.FleetState{ + Teams: []api.Team{}, + Labels: []api.Label{}, + NoTeam: &api.NoTeam{ + Policies: []api.Policy{{Name: "Existing", Query: "SELECT 1;"}}, + Scripts: []api.Script{{ID: 1, Name: "keep.sh", Content: "echo one\n"}}, + }, + } + + // Already on the base branch: the added policy and the edited script. + baseline := &parser.ParsedRepo{Teams: []parser.ParsedTeam{{ + Name: "No team", + SourceFile: "teams/no-team.yml", + Policies: []parser.ParsedPolicy{ + {Name: "Existing", Query: "SELECT 1;"}, + {Name: "Merged not deployed", Query: "SELECT 2;"}, + }, + Scripts: []parser.ParsedScript{{Name: "keep.sh", Content: "echo one\necho two\n"}}, + }}} + + // The MR adds one more policy on top of the base branch's state. The + // branch spells the bucket differently, which must not defeat matching. + proposed := &parser.ParsedRepo{Teams: []parser.ParsedTeam{{ + Name: "Unassigned", + SourceFile: "fleets/unassigned.yml", + Policies: []parser.ParsedPolicy{ + {Name: "Existing", Query: "SELECT 1;"}, + {Name: "Merged not deployed", Query: "SELECT 2;"}, + {Name: "New in this MR", Query: "SELECT 3;"}, + }, + Scripts: []parser.ParsedScript{{Name: "keep.sh", Content: "echo one\necho two\n"}}, + }}} + + r := Diff(current, proposed, nil, nil, WithBaseline(baseline))[0] + + if len(r.Policies.Added) != 1 || r.Policies.Added[0].Name != "New in this MR" { + t.Errorf("policies added: got %+v, want only the MR's own addition", r.Policies.Added) + } + if !r.Scripts.IsEmpty() { + t.Errorf("scripts: got %+v, want empty (the edit is already on the base branch)", r.Scripts) + } +} + +func TestDiffNoTeamQueriesAreReportedAsSkipped(t *testing.T) { + current := &api.FleetState{Teams: []api.Team{}, Labels: []api.Label{}, NoTeam: &api.NoTeam{}} + proposed := &parser.ParsedRepo{Teams: []parser.ParsedTeam{{ + Name: "No team", + SourceFile: "teams/no-team.yml", + Queries: []parser.ParsedQuery{{Name: "Q1"}, {Name: "Q2"}}, + }}} + + r := Diff(current, proposed, nil, nil)[0] + + if !r.Queries.IsEmpty() { + t.Errorf("queries: got %+v, want empty (Fleet has no no-team query scope)", r.Queries) + } + found := false + for _, e := range r.Errors { + if strings.Contains(e, "queries diff skipped: 2 queries configured") { + found = true + } + } + if !found { + t.Errorf("missing query skip note in %v", r.Errors) + } +}