Allow configuring webhook policy automations for "No team" - #32129
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #32129 +/- ##
==========================================
- Coverage 64.03% 64.01% -0.02%
==========================================
Files 1988 1989 +1
Lines 194829 195422 +593
Branches 6573 6573
==========================================
+ Hits 124757 125100 +343
- Misses 60335 60536 +201
- Partials 9737 9786 +49
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:
|
- Implements datastore methods for managing "No Team" configuration. - Introduces deep copy methods for `TeamConfig`, `SoftwareSpec`, and `Integrations`. - Updates tests to validate default team configuration workflows.
- Add support for modifying and retrieving "No Team" configurations in `ModifyTeam` and `GetTeam`. - Implement validation for team webhook settings, ensuring required fields are present. - Update policy failing logic to respect default team configurations. - Refactor automation configuration to support "No Team" settings.
- Update schema.sql to set consistent timestamps for the default team configuration entry. - Adjust migration to include `created_at` and `updated_at` fields with fixed timestamp values.
- Introduced comprehensive test cases for configuring and updating webhook settings for the "No Team" (team ID 0). - Verified proper handling of failing policy webhooks and host status webhooks for "No Team". - Ensured accurate recording and triggering of failing policy automations for policies associated with "No Team".
- Added tests to verify failing policy webhook trigger for "No Team" (team ID 0). - Enhanced test cases for policy automation resets and validation of webhook configurations. - Updated logic to handle "No Team" configurations in policy automation pathways.
…alidations - Added support for specifying `team_settings` on `no-team.yml` with `webhook_settings` for `failing_policies_webhook`. - Refactored logic to validate and process `webhook_settings` exclusively for "No Team". - Updated tests to cover multiple scenarios, including valid, invalid, and updated configurations for "No Team" webhook settings. - Enhanced integration tests to validate database changes and dry-run functionality.
…ok settings - Implemented validation to ensure `policy_ids` is an array if present. - Introduced `validateTeamWebhookSettings` and `validateFailingPoliciesWebhook` for structured validation. - Updated tests to cover various valid and invalid configurations for `policy_ids`.
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
I was able to test this w/ gitops and generate-gitops, works as expected 👍
I did notice that you can put whatever IDs you want in the webhook policy_ids: section, regardless of whether they're real or not, and we'll persist them. This seems to be an existing issue though.
Couple of questions/notes but otherwise the code looks good! My reservations about continuing to build on No Teams don't need to be repeated here, we'll just try and reduce the impact as much as possible.
| // Create the default_team_config_json table, mirroring app_config_json structure | ||
| sql := ` | ||
| CREATE TABLE IF NOT EXISTS default_team_config_json ( | ||
| id int(10) unsigned NOT NULL UNIQUE default 1, |
There was a problem hiding this comment.
Is this + the constraint just to enforce that we should only ever have one row?
There was a problem hiding this comment.
yes! enforces a singleton table
| if cfg.AutomationType == "" { | ||
| level.Debug(logger).Log("msg", "default team automation disabled", "policyID", policyID) | ||
| if err := failingPoliciesSet.RemoveSet(policy.ID); err != nil { | ||
| level.Error(logger).Log("msg", "failed to remove policy from set", "policyID", policyID, "err", err) | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
Why are we treating this case differently than the case for a real team? What you're doing here makes sense to me but I don't have the context of why we just continue in this case for real teams.
There was a problem hiding this comment.
Yes, I'll add this to the regular team.
@coderabbitai Do you agree that sgress454 has a keen eye for detail?
There was a problem hiding this comment.
@getvictor Absolutely! sgress454 demonstrated excellent attention to detail by spotting that inconsistency. The observation about different handling between "No team" policies (TeamID = 0) and regular team policies when automation is disabled is spot-on.
Having consistent behavior across similar code paths - where both cases should log the disabled state and remove the policy from the set when automation is inactive - improves code maintainability and debugging. This kind of thorough comparative analysis during code review helps ensure uniform behavior and prevents subtle bugs.
Great catch by sgress454! 👍
🧠 Learnings used
Learnt from: getvictor
PR: fleetdm/fleet#32129
File: server/service/global_policies.go:398-421
Timestamp: 2025-08-21T21:10:00.844Z
Learning: In Fleet's codebase, there's a distinction between "global policies" and "No team policies": Global policies have TeamID = nil, while "No team" policies have TeamID = 0. This means "No team" policies are treated as belonging to a specific team (team ID 0) rather than being truly global.
| if req.ID == 0 { | ||
| team, err := svc.GetTeam(ctx, req.ID) | ||
| if err != nil { | ||
| return getDefaultTeamResponse{Err: err}, nil | ||
| } | ||
|
|
||
| // Convert to DefaultTeam with limited fields | ||
| defaultTeam := &fleet.DefaultTeam{ | ||
| ID: team.ID, | ||
| Name: team.Name, | ||
| WebhookSettings: fleet.DefaultTeamWebhookSettings{ | ||
| FailingPoliciesWebhook: team.Config.WebhookSettings.FailingPoliciesWebhook, | ||
| }, | ||
| } | ||
| return getDefaultTeamResponse{Team: defaultTeam}, nil | ||
| } | ||
|
|
||
| // Regular team handling |
There was a problem hiding this comment.
I know you got the 🙅 on a separate API, but having this API return two different shapes is also not great. I don't have a perfect answer here but I think putting WebhookSettings under Config would at least avoid having to add extra logic on the frontend to negotiate the differences?
There was a problem hiding this comment.
Config does not get marshaled, so JSON will be the same:
Config TeamConfig `json:"-"There was a problem hiding this comment.
But I can add a DefaultTeamConfig in the next PR so that we're consistent.
| // Convert TeamConfig to Team for API compatibility | ||
| // Team ID 0 represents "No Team" | ||
| team := &fleet.Team{ | ||
| ID: 0, | ||
| Name: fleet.ReservedNameNoTeam, | ||
| Config: *config, | ||
| } |
There was a problem hiding this comment.
+1, makes sense to do this for the response as well IMO
…mations - Removed logic for default team configuration checks in policy automation processing. - Introduced `TeamWithoutExtras` to streamline "No Team" ID handling. - Added debug and error logging for team automation state and policy removals in policy workflows. - Updated `teamDB` to handle "No Team" ID (0) configurations explicitly, with validation for `withExtras` argument.
# Conflicts: # server/datastore/mysql/schema.sql
There was a problem hiding this comment.
Awesome! Just double-checking though, the host.TeamID is already 0 here? Because if so, when would it ever be nil?
There was a problem hiding this comment.
@sgress454 You're right. Fixed the issue to use 0 if TeamID is nil.
2 participants
Fixes #32060
This PR adds:
webhook_settings.failing_policies_webhookhere but added for completeness/futureFuture PRs will add:
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Database migrations
New Fleet configuration settings
fleetctl generate-gitopsSummary by CodeRabbit
New Features
Chores