-
Notifications
You must be signed in to change notification settings - Fork 464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fleet UI: VPP auto install software on failed policies, filter software compatible to policy's target platform, etc #25202
Fleet UI: VPP auto install software on failed policies, filter software compatible to policy's target platform, etc #25202
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25202 +/- ##
==========================================
- Coverage 63.56% 63.53% -0.03%
==========================================
Files 1619 1618 -1
Lines 154680 154342 -338
Branches 3955 3994 +39
==========================================
- Hits 98315 98064 -251
+ Misses 48610 48547 -63
+ Partials 7755 7731 -24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -22,18 +27,18 @@ import Button from "components/buttons/Button"; | |||
import { ISoftwareTitle } from "interfaces/software"; | |||
import TooltipWrapper from "components/TooltipWrapper"; | |||
|
|||
const getPlatformDisplayFromPackageExtension = (ext: string | undefined) => { | |||
const getPlatformFromExtension = (ext: string | undefined): Platform | null => { | |||
switch (ext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this’ll break for zip files, which we might use for FMA for Windows. Will follow up in a PR comment. Thinking we should use for software title source instead as that’s more reliable.
@iansltx We are currently showing display text of macOS for zip files in the dropdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can fix that if/when we run across FMAs for Windows (or Linux?) that are delivered by zip file. Mainly trying to avoid writing new brittle code that we have to remember to fix elsewhere, and we know source
on the software title won't have that issue (though it requires a mapping rather than having an explicit platform field).
const availableSoftwareOptions = (policy: IFormPolicy) => { | ||
return titlesAFI | ||
?.filter((title) => { | ||
const splitName = title.software_package?.name.split(".") ?? ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As implemented, this breaks on VPP apps because we have app_store_app
rather than software_package
.
Also, we'll want to tweak the API call we make to:
- Explicitly ask for macos,windows,linux platforms (to filter out iOS/iPadOS)
- Filter by installable titles rather than titles with software packages, so we can see VPP apps
- Display the correct item in the dropdown when we're dealing with a VPP app
I realize this is expanding the scope of this PR, but these changes are intertwined enough that you'll want to do them all at once.
This also means this PR will need to be stacked on top of the backend PR so you have the API changes you need. I don't think we need a dedicated feature branch on this as we can merge the backend to main
first, at which point the FE PR will repoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't see 1 get software titles API call where I can get macos, windows, or linux all together without ios/ipados. Unless we want to make 3 calls every time we open this modal? So I did 1 call, but filter for each since there's only max 25 policies in the list that we will filter the software titles. I could refactor and make a list for macos, windows, linux and then apply to those policies only. In that case, policies that target more than 1 platform, should that show software for all platforms it targets, or no platforms it targets? wdyt @iansltx
- Done!
- Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, multi-platform support is new (was in-scope for the BE for this ticket; see the end of this table); it'll work if you code against the BE PR. Definitely not worth making three API calls for this.
For multi-platform policies, I think we go with showing all compatible titles (which IIRC is how you have it implemented now). While the chances of someone checking the automation box on a multi-platform automation are low (for that to be useful we'd really need grouped policies for e.g. "Install Firefox on all the things"), if they do they shouldn't be greeted with a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not worth making three API calls for this.
+1
which IIRC is how you have it implemented now
yup
if they do they shouldn't be greeted with a no-op
- 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Change titlesAFI call to not include ipados or ios done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we can do some simplification here, provided we're willing to add another mapping (source -> platform). Good catch on missing pieces of the type; hopefully the struct descriptions are helpful there.
frontend/interfaces/software.ts
Outdated
@@ -105,6 +105,10 @@ export interface IAppStoreApp { | |||
failed: number; | |||
}; | |||
install_during_setup?: boolean; | |||
automatic_installed_policies?: null; // why isn't this on this interface but in the api? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
automatic_installed_policies?: null; // why isn't this on this interface but in the api? | |
automatic_install_policies?: null; |
This is pretty new, and must've gotten missed. Maybe @ghernandez345 knows why it isn't here?
FWIW this value is an array of these objects:
type AutomaticInstallPolicy struct {
ID uint `json:"id" db:"id"`
Name string `json:"name" db:"name"`
TitleID uint `json:"-" db:"software_title_id"`
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference:
API call:
https://dogfood.fleetdm.com/api/latest/fleet/software/titles?page=0&per_page=20&order_direction=desc&order_key=hosts_count&team_id=2&vulnerable=false&exploit=false&available_for_install=true
returns a software with the following key
"app_store_app": {
"app_store_id": "1333542190",
"automatic_install_policies": null,
"version": "7.9.11",
"self_service": false,
"icon_url": "https://is1-ssl.mzstatic.com/image/thumb/Purple116/v4/55/48/f1/5548f17e-3ac3-d19c-a9b2-4e5e636c19b9/AppIcon-0-0-85-220-0-0-0-0-4-0-0-0-2x-sRGB-0-0-0-0-0.png/512x512bb.png",
"last_install": null,
"last_uninstall": null,
"package_url": null
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iansltx I'm wondering if it wasn't suppose to be returning with this API at all? that's why it's all null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do omitempty
on it, so while I'm not 100% why it's null (I'd expect an array length 0), it should show up.
frontend/interfaces/software.ts
Outdated
last_install?: string; // why isn't this on this interface but in the api | ||
last_uninstall?: string; // why isn't this on this interface but in the api | ||
version?: string; // why isn't this on this interface but in the api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. FWIW last_install
seems to already exist on the JS side with ISoftwareLastInstall
, though it doesn't include the entire struct. The last_uninstall
bit is MIA. Structs look like this:
// HostSoftwareInstall represents installation of software on a host from a
// Fleet software installer.
type HostSoftwareInstall struct {
// InstallUUID is the the UUID of the script execution issued to install the related software. This
// field is only used if the install we're describing was for an uploaded software installer.
// Empty if the install was for an App Store app.
InstallUUID string `json:"install_uuid,omitempty"`
// CommandUUID is the UUID of the MDM command issued to install the related software. This field
// is only used if the install we're describing was for an App Store app.
// Empty if the install was for an uploaded software installer.
CommandUUID string `json:"command_uuid,omitempty"`
InstalledAt time.Time `json:"installed_at"`
}
// HostSoftwareUninstall represents uninstallation of software from a host with a
// Fleet software installer.
type HostSoftwareUninstall struct {
// ExecutionID is the UUID of the script execution that uninstalled the software.
ExecutionID string `json:"script_execution_id,omitempty"`
UninstalledAt time.Time `json:"uninstalled_at"`
}
version
is indeed a string, and not sure why it isn't there.
const splitName = title.software_package?.name.split(".") ?? ""; | ||
const ext = | ||
splitName.length > 1 ? splitName[splitName.length - 1] : undefined; | ||
const platform = getPlatformFromExtension(ext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking we can unify/simplify the filtering by having:
- Platform list (splitting commas) defined ~L167.
packagePlatform = PLATFORMS_BY_SOURCE[title?.source] ?? null
which works for both VPP and installer-based titles- result of step 1 includes result of step 2 for the filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a PLATFORM_BY_SOURCE
conversion. I tried to update our interface for source since I realized it was missing linux sources, but theres a ton... I think they're all the keys of SOURCE_TYPE_CONVERSION
but I don't have a map to platform, so I'm just going to keep using the goofy getPlatformFromExtension
and leave a note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const SOURCE_PLATFORM_CONVERSION = {
apt_sources: "linux",
deb_packages: "linux",
portage_packages: "linux",
rpm_packages: "linux",
yum_sources: "linux",
apps: "darwin",
ios_apps: "ios",
ipados_apps: "ipados",
homebrew_packages: "darwin",
programs: "windows",
chocolatey_packages: "windows",
pkg_packages: "macos",
} as const;
Shorter list that's still a superset of software titles with installers that we'll actually see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comment committed here: e823c3a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iansltx goat. I'll put it in Monday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return isAvailableSoftwarePackageOption || isAvailableVPPOption; | ||
}) | ||
.map((title) => { | ||
const vppOption = title.source === "apps" && !!title.app_store_app; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this would wind up simpler if we handled all of the VPP logic behind an if + early return and then fell back to the installer logic if we got there. Means duplicating L210-212 but I think that in return we'll be able to get rid of a few lines of ternaries and get the code a bi cleaner.
c47b08d
to
0845e1d
Compare
"Fleet" is new for VPP. Actor will be literal "Fleet" from server side with no user ID or email, same as automated scripts and policies do now. |
It sounds like we're looking for something like so (figma link)? |
@eugkuo tbh I'm not sure what that tooltip adds (even before this change), as it's already out of date (zips and DMGs for Fleet Maintained Apps aren't mentioned, not are VPP apps). Swapping to source listings doesn't clear things up as we don't show the raw package sources in the UI anywhere (and the revised filtering shouldn't show them either). I'm thinking the compatibility tooltip got added as a workaround for us not filtering the software lists by platform so it's vestigial now that we filter. @noahtalerman does that sound familiar? |
@iansltx Ah. I'm all for dropping it if we don't think it's really adding value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! Going to do the tweak I'm commenting on, as well as drop the compatible platforms tooltip entirely, as those are quick fixes, and will use this branch on top of the BE branch to test.
Since this does need the BE branch to work, 30min from now would probably be a good time to stack it.
...d/pages/policies/ManagePoliciesPage/components/InstallSoftwareModal/InstallSoftwareModal.tsx
Outdated
Show resolved
Hide resolved
ed76903
to
536a935
Compare
…s for both installer types)
…s feature and we now have platform filtering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and we're looking good.
Note that I'm approving for the code I didn't write; @RachelElysia will comment to confirm review of the 4 commits of mine.
@iansltx all your change are belong to us fr everything looks good, let's merge asap after BE. lmk if there's anything else. I'm ready to test again once E2E is on main |
Issue
Part of #23528
Description
Screenrecording of improvement
Notice that each dropdown only shows installable software that matches the policy's platform(s) targeted
Screen.Recording.2025-01-07.at.11.10.41.AM.mov
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/
,orbit/changes/
oree/fleetd-chrome/changes
.