Maintained Apps: define app list, implement ingestion #21946

mna · 2024-09-10T15:14:25Z

Partial implementation of #21773 , a subsequent PR will take care of creating the cron job and some changes needed to the ingestion, should not impact work that needs to read from the library.

Checklist for submitter

Changes file added for user-visible changes in changes/, orbit/changes/ or ee/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)
Added/updated tests

mna

Note that the diff is huge, but a lot of this is testdata (the ~20 json files added for testing).

mna · 2024-09-10T15:16:13Z

pkg/optjson/stringor.go

+// StringOr is a JSON value that can be a string or a different type of object
+// (e.g. somewhat common for a string or an array of strings, but can also be
+// a string or an object, etc.).
+type StringOr[T any] struct {


So this is not technically an "optional JSON" type, but I feel it fits better in this package than floating elsewhere or creating another JSON-related package. Maybe we should rename the package, but I didn't go that far. Happy to adjust if anyone has a strong preference!

mna · 2024-09-10T15:19:29Z

server/datastore/mysql/maintained_apps_test.go

+}
+
+func testIngestWithBrew(t *testing.T, ds *Datastore) {
+	if os.Getenv("NETWORK_TEST") == "" {


This test calls the real brew API, so I put it behind that existing NETWORK_TEST env var. I believe this will still run on each CI, though. Are we ok doing this? It will generate a bit of load on the API, but on the other hand it's good to catch any unforeseen changes on their part.

random thought: could we cache their responses in some other repo and then we only hit their API infrequently (once a day or something)?

Maybe, I don't know ideally I'd like something easier, maybe we don't run that test in normal CI and only in our daily runs? I'll take a look if we already do this for some tests?

hmm looks like we don't really do that at the moment... it could run only if the race detector is enabled, but that feels hacky... I don't know, I'll leave it like that for now but open to ideas/suggestions! Maybe it's not that bad to let it run too, given that we will hit it daily from all fleet installations...

mna · 2024-09-10T15:20:55Z

server/mdm/maintainedapps/apps.json

@@ -0,0 +1,97 @@
+[


This is the (embedded) JSON file that defines the apps we're interested in (the maintained apps). It contains bits of metadata information that we can't get otherwise from the Brew API.

mna · 2024-09-10T15:21:38Z

server/mdm/maintainedapps/ingest.go

+	case http.StatusOK:
+		// success, go on
+	case http.StatusNotFound:
+		// TODO: delete the existing entry? do nothing and succeed? doing the latter for now.


I think doing nothing makes sense in that case? In all likelihood, the existing entry (if any) would still work - its installer URL is not tied to brew.

mna · 2024-09-10T15:22:13Z

server/mdm/maintainedapps/ingest.go

+
+func installScriptForApp(app maintainedApp, cask *brewCask) string {
+	// TODO: implement install script based on cask and app installer format
+	return "install"


Is that logic part of the dmg/zip ticket?

Note that this will be a significant effort. See

fleet/server/mdm/maintainedapps/ingest.go

Lines 181 to 220 in 7ac7ad3

type brewArtifact struct {

App []string `json:"app"`

// Pkg is a bit like Binary, it is an array with a string and an object as

// first two elements. The object has a choices field with an array of

// objects. See Microsoft Edge.

Pkg []optjson.StringOr[*brewPkgChoices] `json:"pkg"`

Uninstall []*brewUninstall `json:"uninstall"`

Zap []*brewZap `json:"zap"`

// Binary is an array with a string and an object as first two elements. See

// the "docker" and "firefox" casks.

Binary []optjson.StringOr[*brewBinaryTarget] `json:"binary"`

}

type brewPkgChoices struct {

// At the moment we don't care about the "choices" field on the pkg.

Choices []any `json:"choices"`

}

type brewBinaryTarget struct {

Target string `json:"target"`

}

// unlike brewArtifact, a single brewUninstall can have many fields set.

// All fields can have one or multiple strings (string or []string).

type brewUninstall struct {

LaunchCtl optjson.StringOr[[]string] `json:"launchctl"`

Quit optjson.StringOr[[]string] `json:"quit"`

PkgUtil optjson.StringOr[[]string] `json:"pkgutil"`

Script optjson.StringOr[[]string] `json:"script"`

// format: [0]=signal, [1]=process name

Signal optjson.StringOr[[]string] `json:"signal"`

Delete optjson.StringOr[[]string] `json:"delete"`

RmDir optjson.StringOr[[]string] `json:"rmdir"`

}

// same as brewUninstall, can be []string or string (see Microsoft Teams).

type brewZap struct {

Trash optjson.StringOr[[]string] `json:"trash"`

RmDir optjson.StringOr[[]string] `json:"rmdir"`

}

for a look at all the options we need to take into account (in addition to the dmg/zip extraction).

mna · 2024-09-10T15:23:27Z

server/mdm/maintainedapps/testdata/1password.json

Those testdata files are a snapshot in time of what brew API returns for our apps. Useful for tests, to build a library from well-known, static values.

mna · 2024-09-10T15:24:43Z

server/mdm/maintainedapps/testing_utils.go

+// It returns the expected results of the ingestion as a slice of
+// fleet.MaintainedApps with only a few fields filled - the result of
+// unmarshaling the testdata/expected_apps.json file.
+func IngestMaintainedApps(t *testing.T, ds fleet.Datastore) []*fleet.MaintainedApp {


Heads-up, those should generally be used for tests when the maintained apps are needed, will generate a cohesive, valid set of apps.

mna · 2024-09-10T15:25:17Z

server/mdm/maintainedapps/testing_utils.go

+// ExpectedAppTokens returns the list of app tokens (unique identifier) that are
+// expected to be in the maintained apps library after ingestion. The tokens are
+// taken from the apps.json list.
+func ExpectedAppTokens(t *testing.T) []string {


This is mostly useful to lightly validate that the real brew API test returned the expected tokens.

codecov · 2024-09-10T15:46:59Z

Codecov Report

Attention: Patch coverage is 86.39053% with 23 lines in your changes missing coverage. Please review.

Please upload report for BASE (feat-fleet-app-library@14b6546). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
server/mdm/maintainedapps/ingest.go	84.61%	6 Missing and 6 partials ⚠️
server/mdm/maintainedapps/testing_utils.go	79.41%	6 Missing and 1 partial ⚠️
server/datastore/mysql/maintained_apps.go	88.23%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@                    Coverage Diff                    @@
##             feat-fleet-app-library   #21946   +/-   ##
=========================================================
  Coverage                          ?   64.98%           
=========================================================
  Files                             ?     1497           
  Lines                             ?   116509           
  Branches                          ?     3450           
=========================================================
  Hits                              ?    75715           
  Misses                            ?    33769           
  Partials                          ?     7025

Flag	Coverage Δ
backend	`66.22% <86.39%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jahzielv

LGTM! just 1 small question

jahzielv · 2024-09-10T17:47:04Z

server/mdm/maintainedapps/apps.json

+	},
+	{
+		"identifier": "box-drive",
+		"bundle_identifier": "???",


Is this ("???") the expected string to use if we don't know the bundle identifier?

:D No, good thing to mention indeed, this will need to be figured out, I just couldn't find that app in dogfood to grab the bundle identifier. I'll try to get those filled for the next PR, testing those apps on my mac mini.

I added it as a checkbox to my ticket just to make sure I don't forget about it, thanks!

mna added 12 commits September 9, 2024 13:46

Add a bundle identifier to the fleet maintained apps

ca060e3

Start implementation of the ingestion logic

6bc72be

Complete the apps list, brew structs

88b2864

Implement StringOr JSON type for complex multi-type values

1fc9c8a

Add json responses from API to testdata

2520a43

Finish adjusting the brew structs

5e54ebb

Plan for rest of ingestion

9f7bbfe

Implement upsert of maintained apps

c12c198

Add ingestion test

fa62b4c

Add test for UpsertMaintainedApp

20a9033

Add a test against the real brew api

7db53d3

Add changes file

7ac7ad3

mna temporarily deployed to Docker Hub September 10, 2024 15:14 — with GitHub Actions Inactive

mna commented Sep 10, 2024

View reviewed changes

mna marked this pull request as ready for review September 10, 2024 15:36

mna requested a review from a team as a code owner September 10, 2024 15:36

jahzielv approved these changes Sep 10, 2024

View reviewed changes

mna merged commit 9abd5a5 into feat-fleet-app-library Sep 10, 2024
22 checks passed

mna deleted the mna-21773-ingest-fleet-library branch September 10, 2024 17:55

mna mentioned this pull request Sep 10, 2024

Ingest Fleet app library metadata from Homebrew Cask API #21773

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintained Apps: define app list, implement ingestion #21946

Maintained Apps: define app list, implement ingestion #21946

mna commented Sep 10, 2024 •

edited

Loading

mna left a comment

mna Sep 10, 2024

mna Sep 10, 2024

jahzielv Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

jahzielv left a comment

jahzielv Sep 10, 2024

mna Sep 10, 2024

mna Sep 10, 2024

	type brewArtifact struct {
	App []string `json:"app"`
	// Pkg is a bit like Binary, it is an array with a string and an object as
	// first two elements. The object has a choices field with an array of
	// objects. See Microsoft Edge.
	Pkg []optjson.StringOr[*brewPkgChoices] `json:"pkg"`
	Uninstall []*brewUninstall `json:"uninstall"`
	Zap []*brewZap `json:"zap"`
	// Binary is an array with a string and an object as first two elements. See
	// the "docker" and "firefox" casks.
	Binary []optjson.StringOr[*brewBinaryTarget] `json:"binary"`
	}

	type brewPkgChoices struct {
	// At the moment we don't care about the "choices" field on the pkg.
	Choices []any `json:"choices"`
	}

	type brewBinaryTarget struct {
	Target string `json:"target"`
	}

	// unlike brewArtifact, a single brewUninstall can have many fields set.
	// All fields can have one or multiple strings (string or []string).
	type brewUninstall struct {
	LaunchCtl optjson.StringOr[[]string] `json:"launchctl"`
	Quit optjson.StringOr[[]string] `json:"quit"`
	PkgUtil optjson.StringOr[[]string] `json:"pkgutil"`
	Script optjson.StringOr[[]string] `json:"script"`
	// format: [0]=signal, [1]=process name
	Signal optjson.StringOr[[]string] `json:"signal"`
	Delete optjson.StringOr[[]string] `json:"delete"`
	RmDir optjson.StringOr[[]string] `json:"rmdir"`
	}

	// same as brewUninstall, can be []string or string (see Microsoft Teams).
	type brewZap struct {
	Trash optjson.StringOr[[]string] `json:"trash"`
	RmDir optjson.StringOr[[]string] `json:"rmdir"`
	}

Maintained Apps: define app list, implement ingestion #21946

Maintained Apps: define app list, implement ingestion #21946

Conversation

mna commented Sep 10, 2024 • edited Loading

Checklist for submitter

mna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

jahzielv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mna commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading